From owner-cvs-all@FreeBSD.ORG Tue Nov 28 13:57:24 2006 Return-Path: X-Original-To: cvs-all@freebsd.org Delivered-To: cvs-all@freebsd.org Received: from mx1.FreeBSD.org (mx1.freebsd.org [69.147.83.52]) by hub.freebsd.org (Postfix) with ESMTP id 8083E16A407; Tue, 28 Nov 2006 13:57:24 +0000 (UTC) (envelope-from Danovitsch@vitsch.net) Received: from amsfep14-int.chello.nl (amsfep17-int.chello.nl [213.46.243.15]) by mx1.FreeBSD.org (Postfix) with ESMTP id 5DA7F43C9E; Tue, 28 Nov 2006 13:53:56 +0000 (GMT) (envelope-from Danovitsch@vitsch.net) Received: from Tuinhuisje.Vitsch.net ([62.195.87.223]) by amsfep14-int.chello.nl (InterMail vM.6.01.04.04 201-2131-118-104-20050224) with ESMTP id <20061128135346.SKRG7165.amsfep14-int.chello.nl@Tuinhuisje.Vitsch.net>; Tue, 28 Nov 2006 14:53:46 +0100 Received: from self (f23025.upc-f.chello.nl [80.56.23.25]) (authenticated bits=0) by Tuinhuisje.Vitsch.net (8.13.1/8.13.1) with ESMTP id kASDrMkm090050; Tue, 28 Nov 2006 14:53:31 +0100 (CET) (envelope-from Danovitsch@vitsch.net) From: "Daan Vreeken [PA4DAN]" Organization: Vitsch Electronics To: Ian Dowse Date: Tue, 28 Nov 2006 14:53:26 +0100 User-Agent: KMail/1.9.1 References: <200611280939.aa56740@nowhere.iedowse.com> In-Reply-To: <200611280939.aa56740@nowhere.iedowse.com> MIME-Version: 1.0 Content-Type: text/plain; charset="iso-8859-6" Content-Transfer-Encoding: 7bit Content-Disposition: inline Message-Id: <200611281453.26820.Danovitsch@vitsch.net> Cc: scottl@samsco.org, src-committers@freebsd.org, cvs-src@freebsd.org, cvs-all@freebsd.org, "M. Warner Losh" Subject: Re: cvs commit: src/sys/dev/usb usbdi.c X-BeenThere: cvs-all@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: CVS commit messages for the entire tree List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 28 Nov 2006 13:57:24 -0000 On Tuesday 28 November 2006 10:39, Ian Dowse wrote: > In message <20061128.000354.1653105648.imp@bsdimp.com>, "M. Warner Losh" > writes > > >In message: <456BD0ED.4050305@samsco.org> > > > > Scott Long writes: > >: Marius Strobl wrote: > >: > On Mon, Nov 27, 2006 at 10:32:12PM +0000, Ian Dowse wrote: > >: >> Thanks for spotting the typo - note though that the recently added > >: >> bus_dmamap_sync() call appears to be using the wrong bus_dma tag > >: >> and a potentially uninitialised map, so it is likely to only work > >: >> on architectures where bus_dmamap_sync doesn't depend on the tag > >: >> and map. > > > >That's weird, because it works on arm which does depend on it. > > There is a cpu_drain_writebuf() call in _bus_dmamap_sync() that > might be doing the trick on arm (assuming I'm looking at the right > file). The other operations there appear to rely on being passed > the correct map. > > >The original problem that motivated the change was that there was data > >that was returned on some transfers. This data wasn't sync, which > >caused us to get weird results when enumerating the device's data > >structures. The flush was needed because we don't map the data > >transfers as coherent (I believe, I've not studied it in a while, and > >my memory may be failing me. > > The SETUP packets that were mentioned in relation to the change are > allocated using usb_block_allocmem(), which does specify BUS_DMA_COHERENT > as far as I can tell (the contents of xfer->request are memcpy'd > into the reqdma buffer and it is then used). The other bus_dmamap_sync() > calls in usbdi.c all related to buffers passed in from outside USB > code, so the bus_dmamap_sync() calls are needed. I am the one who came up with the patch. Before I go into detail, let me say that I'm not a bus-dma guru (yet ;-), so what I did might be completely wrong or inappropriate. Last week I have started debugging the USB problems that I was seeing on the ARM board (KB9202B) that I have here. The board has an OHCI controller that is recognised by sys/arm/at91/ohci_atmelarm.c . The version that is currently in CVS doesn't work at al and crashes. After an initial patch to the ARM ohci driver I got the board to detect devices and data started moving around. ( http://docs.freebsd.org/cgi/getmsg.cgi?fetch=11034+0+archive/2006/freebsd-arm/20061126.freebsd-arm ) But there still where some problems with some devices. The first test I did seemed to have worked "by accident". Devices seemed to fail at random places and times. ( http://docs.freebsd.org/cgi/getmsg.cgi?fetch=31144+0+archive/2006/freebsd-arm/20061126.freebsd-arm ) Sometimes a device would get detected, sometimes it wouldn't. I even saw a simple "SET_ADDRESS" command fail once. Since SET_ADDRESS only consists of a SETUP packet and no data (in or out) I started to suspect the content of the SETUP packets (the content of xfer->request). Tracing the function calls from usbd_set_address() leas me to usbd_start_transfer(). In this function the data portion of a transfer is bus_dmamap_sync'd, but the request portion isn't. This made me believe that it might be needed to also sync the request portion there. Adding the call to bus_dmamap_sync() there as I did solved all the problems I was seeing and with it USB (on ARM at least) works like a charm and without problems. > >: BUS_DMA_COHERENT is meant to be a hint in terms of cross-platform > >: portability. A platform that supports it is supposed to then know > >: to short-circuit the sync calls if they aren't needed. > > > >COHERENT is still expensive to implement on some systems as it > >sometimes get mapped to uncached. This is OK for small data > >structures that the usb stack likes to push around internally, but > >isn't suitable for data that's pushed over the endpoints. > > > >NetBSD also offers a BUS_DMA_NOCACHE flag as well. I don't know how > >useful it is or where. > > If the arm platform requires bus_dmamap_sync() calls to flush out > writes even with BUS_DMA_COHERENT allocations, then that would > explain the original problem. We'll need to add bus_dmamap_sync() > calls in a number of places in each host controller driver where > there are data ordering requirements. Can someone confirm that the > arm platform needs these for correct write ordering? > > BTW, it looks like at the time the newly added bus_dmamap_sync() > call is executed, the SETUP packet data hasn't yet been copied into > the BUS_DMA_COHERENT buffer it will eventually use (that happens > in the pipe->methods->transfer() call afterwards). Is there more > information such as USB_DEBUG traces showing the original problem? Ah, you're right, it's called too early. I didn't dig deep enough into the code. To spare others the same route, I have written down the path through the code that's taken when calling usbd_set_address() : usbd_set_address() in usbdi_util.c usbd_do_request() in usbdi.c usbd_do_request_flags() in usbdi.c usbd_do_request_flags_pipe() in usbdi.c usbd_alloc_xfer() usbd_setup_default_xfer() in usbdi.c fills in xfer->* (especially it does "xfer->request = *req;" and "xfer->buffer=buffer;") usbd_sync_transfer() in usbdi.c usbd_transfer() in usbdi.c usbd_start_transfer() in usbdi.c if (have segs) && (isread) bus_dmamap_sync(PREWRITE) // my patch adds if (have a request) bus_dmamap_sync(PREWRITE) here pipe->methods->transfer() -> ohci_device_ctrl_transfer() in ohci.c usb_insert_transfer() in usbdi.c STAILQ_INSERT_TAIL(pipe->queue, xfer) ohci_device_ctrl_start() in ohci.c ohci_device_request() in ohci.c .. // only here the actual request data is copied... memcpy(KERNADDR(&opipe->u.ctl.reqdma), req, sizeof(*req)) .. My patch adds the sync call in usbd_start_transfer(), while the actual request data is copied into the right buffer later on in ohci_device_request(). For some reason the patch does work. If I disable this one-liner, all devices fail (most of the time). Re-enabling the line get the devices to work again. I have found one device that seems to fail consistently at the same spot (a USB HUB). With USB_DEBUG enabled I get the following dmesg output : --- log --- uhub_intr: sc=0xc0756950 usb_needs_explore usb_event_thread: woke up usb_discover uhub_explore: uhub0 port 1 status 0x0101 0x0001 uhub_explore: status change hub=1 port=1 uhub_intr: sc=0xc0756950 usb_needs_explore usbd_reset_port: port 1 reset done, error=NORMAL_COMPLETION uhub_intr: sc=0xc0756950 usb_needs_explore usbd_new_device bus=0xc0721000 port=1 depth=1 speed=2 usbd_setup_pipe: dev=0xc0818a80 iface=0 ep=0xc0818aa4 pipe=0xc0818a84 usbd_ar_pipe: pipe=0xc0818b80 usbd_setup_pipe: dev=0xc0818a80 iface=0 ep=0xc0818aa4 pipe=0xc0818a84 usbd_get_desc: type=1, index=0, len=8 usbd_new_device: adding unit addr=2, rev=101, class=9, subclass=0, protocol=0, usbd_ar_pipe: pipe=0xc0771b80 usbd_setup_pipe: dev=0xc0818a80 iface=0 ep=0xc0818aa4 pipe=0xc0818a84 usbd_get_device_desc: usbd_get_desc: type=1, index=0, len=18 usbd_new_device: new dev (addr 2), dev=0xc0818a80, parent=0xc074de00 usbd_probe_and_attach: trying device specific drivers usbd_get_string_desc: expected 16, got 4 uhub_match, dd=0xc0818abc uhub_match, dd=0xc0818abc uhub1: on uhub0 uhub_attach usbd_get_config_desc: confidx=0 usbd_get_desc: type=2, index=0, len=9 usbd_get_config_desc: confidx=0, bad desc len=116 type=136 uhub1: configuration failed, error=INVAL device_attach: uhub1 attach returned 6 --- /log --- It seems to fail trying to load a 16 byte descriptor. (Once again, this HUB works with the patch in place). I don't believe my patch is at the right place where it's placed now. Any ideas from someone with more bus_dma-foo and/or usb-foo are highly appreciated :) If more debugging information is needed, just ask. -- Daan