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

[Scott Long]

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

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.

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.

Scott