Date: Mon, 12 May 2008 00:36:36 +0200 From: Marius Strobl <marius@alchemy.franken.de> To: Sam Leffler <sam@freebsd.org> 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 Message-ID: <20080511223636.GC36894@alchemy.franken.de> In-Reply-To: <482772DA.2070104@freebsd.org> 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> <482772DA.2070104@freebsd.org>
next in thread | previous in thread | raw e-mail | index | archive | help
On Sun, May 11, 2008 at 03:27:38PM -0700, Sam Leffler wrote:
> 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.
>
I'd like to go for letting hacksync() only call bus_dmamap_sync(9)
if xfer->length is not 0 as this seems to be the cleanest solution
regarding treating the DMA map as opaque to me. So could you please
verify that this doesn't break hacksync() solving the problem you
were seeing on arm?
Marius
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20080511223636.GC36894>