Date: Tue, 28 Nov 2006 14:53:26 +0100 From: "Daan Vreeken [PA4DAN]" <Danovitsch@vitsch.net> To: Ian Dowse <iedowse@iedowse.com> Cc: scottl@samsco.org, src-committers@freebsd.org, cvs-src@freebsd.org, cvs-all@freebsd.org, "M. Warner Losh" <imp@bsdimp.com> Subject: Re: cvs commit: src/sys/dev/usb usbdi.c Message-ID: <200611281453.26820.Danovitsch@vitsch.net> In-Reply-To: <200611280939.aa56740@nowhere.iedowse.com> References: <200611280939.aa56740@nowhere.iedowse.com>
next in thread | previous in thread | raw e-mail | index | archive | help
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 <scottl@samsco.org> 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: <vendor 0x05e3 U, class 9/0, rev 1.01/0.12, addr 2> 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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?200611281453.26820.Danovitsch>