From owner-cvs-all@FreeBSD.ORG Mon Nov 27 23:48:14 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 B1DB916A47B; Mon, 27 Nov 2006 23:48:14 +0000 (UTC) (envelope-from iedowse@iedowse.com) Received: from nowhere.iedowse.com (nowhere.iedowse.com [82.195.144.75]) by mx1.FreeBSD.org (Postfix) with SMTP id BDF9D4530B; Mon, 27 Nov 2006 22:31:18 +0000 (GMT) (envelope-from iedowse@iedowse.com) Received: from localhost ([127.0.0.1] helo=iedowse.com) by nowhere.iedowse.com via local-iedowse id ; 27 Nov 2006 22:32:13 +0000 (GMT) To: Marius Strobl In-Reply-To: Your message of "Mon, 27 Nov 2006 18:39:03 GMT." <200611271839.kARId33k039747@repoman.freebsd.org> Date: Mon, 27 Nov 2006 22:32:12 +0000 From: Ian Dowse Message-ID: <200611272232.aa46722@nowhere.iedowse.com> 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 List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , X-List-Received-Date: Mon, 27 Nov 2006 23:48:14 -0000 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?) 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. Ian