Date: Tue, 28 Nov 2006 01:35:26 +0100 From: Marius Strobl <marius@alchemy.franken.de> To: Ian Dowse <iedowse@iedowse.com>, "M. Warner Losh" <imp@bsdimp.com> Cc: cvs-src@FreeBSD.org, src-committers@FreeBSD.org, cvs-all@FreeBSD.org Subject: Re: cvs commit: src/sys/dev/usb usbdi.c Message-ID: <20061128003526.GA76234@alchemy.franken.de> In-Reply-To: <200611272232.aa46722@nowhere.iedowse.com> References: <200611271839.kARId33k039747@repoman.freebsd.org> <200611272232.aa46722@nowhere.iedowse.com>
next in thread | previous in thread | raw e-mail | index | archive | help
On Mon, Nov 27, 2006 at 10:32:12PM +0000, Ian Dowse wrote: > In message <200611271839.kARId33k039747@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
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20061128003526.GA76234>