From owner-cvs-src@FreeBSD.ORG Tue Nov 28 06:15:22 2006 Return-Path: X-Original-To: cvs-src@FreeBSD.org Delivered-To: cvs-src@FreeBSD.org Received: from mx1.FreeBSD.org (mx1.freebsd.org [69.147.83.52]) by hub.freebsd.org (Postfix) with ESMTP id 5DD6116A407; Tue, 28 Nov 2006 06:15:22 +0000 (UTC) (envelope-from scottl@samsco.org) Received: from pooker.samsco.org (pooker.samsco.org [168.103.85.57]) by mx1.FreeBSD.org (Postfix) with ESMTP id 7DF9343F5A; Tue, 28 Nov 2006 06:03:19 +0000 (GMT) (envelope-from scottl@samsco.org) Received: from [192.168.254.11] (phobos.samsco.home [192.168.254.11]) (authenticated bits=0) by pooker.samsco.org (8.13.4/8.13.4) with ESMTP id kAS62QXY049602; Mon, 27 Nov 2006 23:02:31 -0700 (MST) (envelope-from scottl@samsco.org) Message-ID: <456BD0ED.4050305@samsco.org> Date: Mon, 27 Nov 2006 23:02:21 -0700 From: Scott Long User-Agent: Mozilla/5.0 (Macintosh; U; Intel Mac OS X; en-US; rv:1.8.0.7) Gecko/20060910 SeaMonkey/1.0.5 MIME-Version: 1.0 To: Marius Strobl References: <200611271839.kARId33k039747@repoman.freebsd.org> <200611272232.aa46722@nowhere.iedowse.com> <20061128003526.GA76234@alchemy.franken.de> In-Reply-To: <20061128003526.GA76234@alchemy.franken.de> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit X-Spam-Status: No, score=-1.4 required=3.8 tests=ALL_TRUSTED autolearn=failed version=3.1.1 X-Spam-Checker-Version: SpamAssassin 3.1.1 (2006-03-10) on pooker.samsco.org Cc: cvs-src@FreeBSD.org, Ian Dowse , src-committers@FreeBSD.org, cvs-all@FreeBSD.org, "M. Warner Losh" Subject: Re: cvs commit: src/sys/dev/usb usbdi.c X-BeenThere: cvs-src@freebsd.org X-Mailman-Version: 2.1.5 Precedence: list List-Id: CVS commit messages for the src tree List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 28 Nov 2006 06:15:22 -0000 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. Scott