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

[M. Warner Losh]

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:
: >> In message <200611271839.kARId33k039747 at repoman.freebsd.org>, Marius Strobl wri
: >> tes:
: >>>  Refine the previous change to only call bus_dmamap_sync() in case of
: >>>  an URQ_REQUEST when DMA segments are passed to usbd_start_transfer();
: >>>  when the request doesn't include the optional data buffer the size of
: >>>  the transfer (xfer->length) is 0, in which case usbd_transfer() won't
: >>>  create a DMA map but call usbd_start_transfer() with no DMA segments.
: >>>  With the previous change this could result in the bus_dmamap_sync()
: >>>  implementation dereferencing the NULL-pointer passed as the DMA map
: >>>  argument.
: >>>  While at it fix what appears to be a typo in usbd_start_transfer();
: >>>  in order to determine wheter usbd_start_transfer() was called with
: >>>  DMA segments check whether the number of segments is > 0 rather than
: >>>  the pointer to them being > 0.
: >> 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.

: >> The only bus_dmamap_sync() calls in the usb tree at the moment are
: >> ones I added as part of the scatter-gather work a while ago, and
: >> they all relate to the data buffer associated with a transfer. For
: >> the control transfer SETUP packet buffer, each host controller driver
: >> has a "reqdma" field that holds the DMA mapping information. It's
: >> probably easiest to make the changes in the individual host controller
: >> drivers so they do something like
: >>
: >> 	bus_dmamap_sync(reqdma.block->tag,
: >>             reqdma.block->map, BUS_DMASYNC_PREWRITE);
: >>
: >> after filling out the setup packet.
: >>
: >> I guess other memory structures such as descriptors and queue heads
: >> might need bus_dmamap_sync calls too - what are the features of the
: >> platform(s) where the original issues were seen? (e.g. is some IOMMU
: >> operation or memory barrier required between host and I/O access
: >> to memory?)
: > 
: > Your suggestion sounds reasonable to me, but please talk to
: > imp@ about it as I was merely trying to fix fallout seen on
: > sparc64 which was caused by his change but don't know about
: > the original problam that motivated that change.

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.

: >> Apart from the handling of data buffers, the USB code
: >> appears to currently assume that with BUS_DMA_COHERENT it doesn't
: >> need any further synchronisation, which can't be right in general.
: > 
: > Hrm, because a certain platform might silently ignore
: > BUS_DMA_COHERENT and provide a non-coherent mapping instead
: > or because of another reason?
: > 
: > Marius
: > 
: 
: 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.

: I don't know of an architecture that FreeBSD supports or is likely to
: support that doesn't have a working concept of coherent memory.  It
: might be time to start breaking these crufty design considerations that
: where meant for old m68k and sun3 systems.

Is that why it is needed?

Warner