cvs commit: src/sys/dev/usb ehci.c ohci.c

[Marius Strobl]

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);
 }