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

[Ian Dowse]

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.

>: 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?

Ian