From owner-cvs-all@FreeBSD.ORG Sun May 11 22:27:42 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 6F24D1065681; Sun, 11 May 2008 22:27:42 +0000 (UTC) (envelope-from sam@freebsd.org) Received: from ebb.errno.com (ebb.errno.com [69.12.149.25]) by mx1.freebsd.org (Postfix) with ESMTP id 28E658FC1D; Sun, 11 May 2008 22:27:42 +0000 (UTC) (envelope-from sam@freebsd.org) Received: from trouble.errno.com (trouble.errno.com [10.0.0.248]) (authenticated bits=0) by ebb.errno.com (8.13.6/8.12.6) with ESMTP id m4BMRcvt047413 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO); Sun, 11 May 2008 15:27:39 -0700 (PDT) (envelope-from sam@freebsd.org) Message-ID: <482772DA.2070104@freebsd.org> Date: Sun, 11 May 2008 15:27:38 -0700 From: Sam Leffler Organization: FreeBSD Project User-Agent: Thunderbird 2.0.0.9 (X11/20071125) MIME-Version: 1.0 To: Marius Strobl 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> <20080511221238.GB36894@alchemy.franken.de> In-Reply-To: <20080511221238.GB36894@alchemy.franken.de> Content-Type: text/plain; charset=ISO-8859-1; format=flowed Content-Transfer-Encoding: 7bit X-DCC-sonic.net-Metrics: ebb.errno.com; whitelist 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:27:42 -0000 Marius Strobl wrote: > 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. > > The check for NULL in bus_dmamap_sync is a dubious optimization. I simply suggested you might want to apply it to sparc's bus_dma code since there's no way of knowing whether there are other places that this is likewise assumed. At this point I don't care; if you want to hack the hacksync routine to check for a null dmapmap or xfer length zero or whatever feel free. It's all just covering up for the brokeness of the usb code. Sam