cvs commit: src/sys/dev/usb usbdi.c

Tue Nov 28 05:57:24 PST 2006

On Tuesday 28 November 2006 10:39, Ian Dowse wrote:
> In message <20061128.000354.1653105648.imp at bsdimp.com>, "M. Warner Losh"
> writes
>
> >In message: <456BD0ED.4050305 at samsco.org>
> >
> >            Scott Long <scottl at 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