Marius Strobl wrote:
On Mon, Nov 27, 2006 at 10:32:12PM +0000, Ian Dowse wrote:
In message <[EMAIL PROTECTED]>, 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
_______________________________________________
[email protected] mailing list
http://lists.freebsd.org/mailman/listinfo/cvs-all
To unsubscribe, send any mail to "[EMAIL PROTECTED]"
