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

[Marius Strobl]

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.
> 
> 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.

> 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