cvs commit: src/sys/dev/usb usbdi.c
scottl at samsco.org
Mon Nov 27 22:15:22 PST 2006
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
>>> 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
>>> 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
>> 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?
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.
More information about the cvs-src