Date: Mon, 27 Nov 2006 23:02:21 -0700 From: Scott Long <scottl@samsco.org> To: Marius Strobl <marius@alchemy.franken.de> Cc: cvs-src@FreeBSD.org, Ian Dowse <iedowse@iedowse.com>, src-committers@FreeBSD.org, cvs-all@FreeBSD.org, "M. Warner Losh" <imp@bsdimp.com> Subject: Re: cvs commit: src/sys/dev/usb usbdi.c Message-ID: <456BD0ED.4050305@samsco.org> In-Reply-To: <20061128003526.GA76234@alchemy.franken.de> References: <200611271839.kARId33k039747@repoman.freebsd.org> <200611272232.aa46722@nowhere.iedowse.com> <20061128003526.GA76234@alchemy.franken.de>
index | next in thread | previous in thread | raw e-mail
Marius Strobl wrote: > 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 > 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. Scotthome | help
Want to link to this message? Use this
URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?456BD0ED.4050305>