From owner-cvs-all@FreeBSD.ORG Tue Nov 28 05:32:38 2006 Return-Path: <owner-cvs-all@FreeBSD.ORG> X-Original-To: cvs-all@FreeBSD.org Delivered-To: cvs-all@FreeBSD.org Received: from mx1.FreeBSD.org (mx1.freebsd.org [69.147.83.52]) by hub.freebsd.org (Postfix) with ESMTP id 68A1516A556; Tue, 28 Nov 2006 05:32:38 +0000 (UTC) (envelope-from marius@alchemy.franken.de) Received: from alchemy.franken.de (alchemy.franken.de [194.94.249.214]) by mx1.FreeBSD.org (Postfix) with ESMTP id 24031451B2; Tue, 28 Nov 2006 04:43:37 +0000 (GMT) (envelope-from marius@alchemy.franken.de) Received: from alchemy.franken.de (localhost [127.0.0.1]) by alchemy.franken.de (8.13.8/8.13.8/ALCHEMY.FRANKEN.DE) with ESMTP id kAS0ZQeO022951; Tue, 28 Nov 2006 01:35:28 +0100 (CET) (envelope-from marius@alchemy.franken.de) Received: (from marius@localhost) by alchemy.franken.de (8.13.8/8.13.8/Submit) id kAS0ZQdq022950; Tue, 28 Nov 2006 01:35:26 +0100 (CET) (envelope-from marius) 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> Message-ID: <20061128003526.GA76234@alchemy.franken.de> References: <200611271839.kARId33k039747@repoman.freebsd.org> <200611272232.aa46722@nowhere.iedowse.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <200611272232.aa46722@nowhere.iedowse.com> User-Agent: Mutt/1.4.2.2i Cc: cvs-src@FreeBSD.org, src-committers@FreeBSD.org, cvs-all@FreeBSD.org Subject: Re: cvs commit: src/sys/dev/usb usbdi.c X-BeenThere: cvs-all@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: CVS commit messages for the entire tree <cvs-all.freebsd.org> List-Unsubscribe: <http://lists.freebsd.org/mailman/listinfo/cvs-all>, <mailto:cvs-all-request@freebsd.org?subject=unsubscribe> List-Archive: <http://lists.freebsd.org/pipermail/cvs-all> List-Post: <mailto:cvs-all@freebsd.org> List-Help: <mailto:cvs-all-request@freebsd.org?subject=help> List-Subscribe: <http://lists.freebsd.org/mailman/listinfo/cvs-all>, <mailto:cvs-all-request@freebsd.org?subject=subscribe> X-List-Received-Date: Tue, 28 Nov 2006 05:32:38 -0000 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