From owner-cvs-all@FreeBSD.ORG Sun May 11 22:12:40 2008 Return-Path: Delivered-To: cvs-all@freebsd.org Received: from mx1.freebsd.org (mx1.freebsd.org [IPv6:2001:4f8:fff6::34]) by hub.freebsd.org (Postfix) with ESMTP id C3ECB1065675; Sun, 11 May 2008 22:12:40 +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 3F0E68FC13; Sun, 11 May 2008 22:12:39 +0000 (UTC) (envelope-from marius@alchemy.franken.de) Received: from alchemy.franken.de (localhost [127.0.0.1]) by alchemy.franken.de (8.14.1/8.14.1/ALCHEMY.FRANKEN.DE) with ESMTP id m4BMCcdK083676; Mon, 12 May 2008 00:12:39 +0200 (CEST) (envelope-from marius@alchemy.franken.de) Received: (from marius@localhost) by alchemy.franken.de (8.14.1/8.14.1/Submit) id m4BMCcx3083675; Mon, 12 May 2008 00:12:38 +0200 (CEST) (envelope-from marius) Date: Mon, 12 May 2008 00:12:38 +0200 From: Marius Strobl To: Sam Leffler , scottl@freebsd.org Message-ID: <20080511221238.GB36894@alchemy.franken.de> References: <200803201619.m2KGJQr7033985@repoman.freebsd.org> <20080412193358.GA44768@alchemy.franken.de> <20080423203622.GA66545@alchemy.franken.de> <480F9F8B.5050209@freebsd.org> <20080423205943.GG99566@alchemy.franken.de> <480FA41F.20407@freebsd.org> <20080507202135.GA65358@alchemy.franken.de> <482479EE.8060701@freebsd.org> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <482479EE.8060701@freebsd.org> User-Agent: Mutt/1.4.2.3i Cc: cvs-src@freebsd.org, src-committers@freebsd.org, cvs-all@freebsd.org Subject: Re: cvs commit: src/sys/dev/usb ehci.c ohci.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: Sun, 11 May 2008 22:12:41 -0000 On Fri, May 09, 2008 at 09:21:02AM -0700, Sam Leffler wrote: > Marius Strobl wrote: > >On Wed, Apr 23, 2008 at 02:03:27PM -0700, Sam Leffler wrote: > > > >>Marius Strobl wrote: > >> > >>>On Wed, Apr 23, 2008 at 01:43:55PM -0700, Sam Leffler wrote: > >>> > >>> > >>>>Marius Strobl wrote: > >>>> > >>>> > >>>>>On Sat, Apr 12, 2008 at 09:33:58PM +0200, Marius Strobl wrote: > >>>>> > >>>>> > >>>>> > >>>>>>On Thu, Mar 20, 2008 at 04:19:26PM +0000, Sam Leffler wrote: > >>>>>> > >>>>>> > >>>>>> > >>>>>>>sam 2008-03-20 16:19:25 UTC > >>>>>>> > >>>>>>>FreeBSD src repository > >>>>>>> > >>>>>>>Modified files: > >>>>>>> sys/dev/usb ehci.c ohci.c > >>>>>>>Log: > >>>>>>>Workaround design botch in usb: blindly mixing bus_dma with PIO does > >>>>>>>not > >>>>>>>work on architectures with a write-back cache as the PIO writes end > >>>>>>>up > >>>>>>>in the cache which the sync(BUS_DMASYNC_POSTREAD) in > >>>>>>>usb_transfer_complete > >>>>>>>then discards; compensate in the xfer methods that do PIO by pushing > >>>>>>>the > >>>>>>>writes out of the cache before usb_transfer_complete is called. > >>>>>>> > >>>>>>>This fixes USB on xscale and likely other places. > >>>>>>> > >>>>>>>Sponsored by: hobnob > >>>>>>>Reviewed by: cognet, imp > >>>>>>>MFC after: 1 month > >>>>>>> > >>>>>>>Revision Changes Path > >>>>>>>1.62 +16 -0 src/sys/dev/usb/ehci.c > >>>>>>>1.171 +16 -0 src/sys/dev/usb/ohci.c > >>>>>>> > >>>>>>> > >>>>>>> > >>>>>>This causes a crash during boot on sparc64. Looks like map is still > >>>>>>NULL at that point. > >>>>>> > >>>>>> > >>>>>> > >>>>>> > >>>>>Are you ok with the change below or would that also prevent > >>>>>your kludge from taking effect? > >>>>> > >>>>>Marius > >>>>> > >>>>>Index: ehci.c > >>>>>=================================================================== > >>>>>RCS file: /usr/data/bsd/cvs/fbsd/src/sys/dev/usb/ehci.c,v > >>>>>retrieving revision 1.62 > >>>>>diff -u -r1.62 ehci.c > >>>>>--- ehci.c 20 Mar 2008 16:19:25 -0000 1.62 > >>>>>+++ ehci.c 23 Apr 2008 20:23:58 -0000 > >>>>>@@ -664,6 +664,8 @@ > >>>>> usbd_pipe_handle pipe = xfer->pipe; > >>>>> bus_dma_tag_t tag = pipe->device->bus->buffer_dmatag; > >>>>> struct usb_dma_mapping *dmap = &xfer->dmamap; > >>>>>+ if (dmap->map == NULL) > >>>>>+ return; > >>>>> bus_dmamap_sync(tag, dmap->map, BUS_DMASYNC_PREWRITE); > >>>>>} > >>>>> > >>>>>Index: ohci.c > >>>>>=================================================================== > >>>>>RCS file: /usr/data/bsd/cvs/fbsd/src/sys/dev/usb/ohci.c,v > >>>>>retrieving revision 1.171 > >>>>>diff -u -r1.171 ohci.c > >>>>>--- ohci.c 20 Mar 2008 16:19:25 -0000 1.171 > >>>>>+++ ohci.c 21 Apr 2008 19:13:54 -0000 > >>>>>@@ -1571,6 +1571,8 @@ > >>>>> usbd_pipe_handle pipe = xfer->pipe; > >>>>> bus_dma_tag_t tag = pipe->device->bus->buffer_dmatag; > >>>>> struct usb_dma_mapping *dmap = &xfer->dmamap; > >>>>>+ if (dmap->map == NULL) > >>>>>+ return; > >>>>> bus_dmamap_sync(tag, dmap->map, BUS_DMASYNC_PREWRITE); > >>>>>} > >>>>> > >>>>> > >>>>> > >>>>> > >>>>> > >>>>> > >>>>You have not identified why you don't have a dma map. I don't have a > >>>>way to diagnose your problem and so far as I know no other platform had > >>>>an issue w/ the change. I suggest you figure out why your map is not > >>>>setup instead of adding a hack. > >>>> > >>>> > >>>> > >>>It's because the usb(4) code doesn't create DMA maps for > >>>zero-length transfers, see usbd_transfer(). In the case of > >>>the backtrace I posted not for usbd_set_address(), which > >>>does USETW(req.wLength, 0) so later on size is 0 in > >>>usbd_transfer() hence no DMA map. I don't know why your > >>>hack doesn't also crash other platforms. > >>> > >>> > >>Thanks for explaining, I will look. Please hold off for a bit. > >> > >> > > > >Style-wise the version below probably is more appropriate than > >the above one. The question still is whether that fix prevents > >hacksync() taking effect as desired, which would make it a very > >evil hack though as hacksync() then relies on bus_dmamap_sync() > >working on uninitialized DMA maps. > > > >Marius > > > >Index: ehci.c > >=================================================================== > >RCS file: /usr/data/bsd/cvs/fbsd/src/sys/dev/usb/ehci.c,v > >retrieving revision 1.62 > >diff -u -p -r1.62 ehci.c > >--- ehci.c 20 Mar 2008 16:19:25 -0000 1.62 > >+++ ehci.c 27 Apr 2008 14:09:53 -0000 > >@@ -661,9 +661,13 @@ ehci_pcd_enable(void *v_sc) > > static __inline void > > hacksync(usbd_xfer_handle xfer) > > { > >- usbd_pipe_handle pipe = xfer->pipe; > >- bus_dma_tag_t tag = pipe->device->bus->buffer_dmatag; > >- struct usb_dma_mapping *dmap = &xfer->dmamap; > >+ bus_dma_tag_t tag; > >+ struct usb_dma_mapping *dmap; > >+ > >+ if (xfer->length == 0) > >+ return; > >+ tag = xfer->pipe->device->bus->buffer_dmatag; > >+ dmap = &xfer->dmamap; > > bus_dmamap_sync(tag, dmap->map, BUS_DMASYNC_PREWRITE); > > } > > > >Index: ohci.c > >=================================================================== > >RCS file: /usr/data/bsd/cvs/fbsd/src/sys/dev/usb/ohci.c,v > >retrieving revision 1.171 > >diff -u -p -r1.171 ohci.c > >--- ohci.c 20 Mar 2008 16:19:25 -0000 1.171 > >+++ ohci.c 27 Apr 2008 14:09:37 -0000 > >@@ -1568,9 +1568,13 @@ ohci_device_bulk_done(usbd_xfer_handle x > > static __inline void > > hacksync(usbd_xfer_handle xfer) > > { > >- usbd_pipe_handle pipe = xfer->pipe; > >- bus_dma_tag_t tag = pipe->device->bus->buffer_dmatag; > >- struct usb_dma_mapping *dmap = &xfer->dmamap; > >+ bus_dma_tag_t tag; > >+ struct usb_dma_mapping *dmap; > >+ > >+ if (xfer->length == 0) > >+ return; > >+ tag = xfer->pipe->device->bus->buffer_dmatag; > >+ dmap = &xfer->dmamap; > > bus_dmamap_sync(tag, dmap->map, BUS_DMASYNC_PREWRITE); > > } > > > > > > > > > Sorry for taking so long to look at this. It appears the reason things > work everywhere but sparc is because all the other archs use the > definition of bus_dmamap_sync which looks like this: > > #define bus_dmamap_sync(dmat, dmamap, op) \ > do { \ > if ((dmamap) != NULL) \ > _bus_dmamap_sync(dmat, dmamap, op); \ > } while (0) > > So it explicitly checks for the map being NULL and since sparc does not > it blows up. Now checking for a NULL map seems very wrong (as the map > should be opaque as scott noted) but having sparc have different > semantics seems wrong. So rather than add yet another hack in the usb > code perhaps we should make sparc's bus_dma code consistent with > everyone else on this? > > There's no mention of this in the man page. My guess would be that this is due to the other archs letting bus_dmamem_alloc() return a NULL DMA map to denote no mapping or bouncing and thus no syncing is required as jhb@ explained. This check is irrelevant to sparc64 and sun4v as these require syncing in any case. I think letting hacksync() rely on this internal optimization of bus_dmamap_sync() is very wrong also. Neither do I see why letting hacksync() just do nothing if xfer->length == 0 is a hack as it's basically the same check usbd_transfer() uses to decide whether to create a DMA map in the first place. Besides, hacksync() already is a very crude hack so one hardly can make it worse. I can just #ifdef arm or #ifndef sparc64 it if you prefer. Marius