cvs commit: src/sys/dev/usb ehci.c ohci.c
Marius Strobl
marius at alchemy.franken.de
Sun May 11 22:12:41 UTC 2008
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
More information about the cvs-src
mailing list