From owner-cvs-all@FreeBSD.ORG Tue Nov 28 07:06:04 2006 Return-Path: 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 086F916A416; Tue, 28 Nov 2006 07:06:04 +0000 (UTC) (envelope-from imp@bsdimp.com) Received: from harmony.bsdimp.com (vc4-2-0-87.dsl.netrack.net [199.45.160.85]) by mx1.FreeBSD.org (Postfix) with ESMTP id 5A9D643CAC; Tue, 28 Nov 2006 07:05:54 +0000 (GMT) (envelope-from imp@bsdimp.com) Received: from localhost (localhost [127.0.0.1]) by harmony.bsdimp.com (8.13.4/8.13.4) with ESMTP id kAS738bG047473; Tue, 28 Nov 2006 00:03:08 -0700 (MST) (envelope-from imp@bsdimp.com) Date: Tue, 28 Nov 2006 00:03:54 -0700 (MST) Message-Id: <20061128.000354.1653105648.imp@bsdimp.com> To: scottl@samsco.org From: "M. Warner Losh" In-Reply-To: <456BD0ED.4050305@samsco.org> References: <200611272232.aa46722@nowhere.iedowse.com> <20061128003526.GA76234@alchemy.franken.de> <456BD0ED.4050305@samsco.org> X-Mailer: Mew version 4.2 on Emacs 21.3 / Mule 5.0 (SAKAKI) Mime-Version: 1.0 Content-Type: Text/Plain; charset=us-ascii Content-Transfer-Encoding: 7bit X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-2.0 (harmony.bsdimp.com [127.0.0.1]); Tue, 28 Nov 2006 00:03:09 -0700 (MST) Cc: cvs-src@FreeBSD.org, iedowse@iedowse.com, 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 List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 28 Nov 2006 07:06:04 -0000 In message: <456BD0ED.4050305@samsco.org> Scott Long writes: : 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. That's weird, because it works on arm which does depend on it. : >> 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. The original problem that motivated the change was that there was data that was returned on some transfers. This data wasn't sync, which caused us to get weird results when enumerating the device's data structures. The flush was needed because we don't map the data transfers as coherent (I believe, I've not studied it in a while, and my memory may be failing me. : >> 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. COHERENT is still expensive to implement on some systems as it sometimes get mapped to uncached. This is OK for small data structures that the usb stack likes to push around internally, but isn't suitable for data that's pushed over the endpoints. NetBSD also offers a BUS_DMA_NOCACHE flag as well. I don't know how useful it is or where. : 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. Is that why it is needed? Warner