Date: Wed, 7 May 2008 22:21:35 +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: <20080507202135.GA65358@alchemy.franken.de> In-Reply-To: <480FA41F.20407@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>
next in thread | previous in thread | raw e-mail | index | archive | help
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);
}
Want to link to this message? Use this URL: <https://mail-archive.FreeBSD.org/cgi/mid.cgi?20080507202135.GA65358>