Re: [PATCH] USB: Allow transfer_buffer with transfer_dma, take #4

* Alan Stern <[email protected]> [070328 17:09]:
> On Wed, 28 Mar 2007, Tony Lindgren wrote:
> 
> > OK, attached is take #3 with ifndef CONFIG_HIGHMEM. Anybody have other
> > limitations in mind?
> 
> > +		/*
> > +		 * Some systems need both transfer_buffer and transfer_dma
> > +		 * to revert to PIO if DMA is temporarily unavailable.
> > +		 * Note that transfer_buffer may not be accessible on some
> > +		 * systems. In that case there is no need to use it either.
> > +		 */
> 
> It would be nice if this comment were more grammatical.  Consider this 
> instead:
> 
> 		/*
> 		 * Some systems can revert to PIO if DMA is temporarily
> 		 * unavailable; for their sake both transfer_buffer and
> 		 * transfer_dma must be set properly.  However this is
> 		 * possible only on systems with no high memory, since
> 		 * DMA buffers located in high memory are not directly
> 		 * addressable by the kernel for PIO.  When high memory
> 		 * is available, we NULL out transfer_buffer to avoid
> 		 * keeping a stale pointer and to help spot possible
> 		 * addressing bugs.
> 		 */

OK, updated comments. Also updated the patch commit text.

> >  		if (dma) {
> > -			/* hc may use _only_ transfer_dma */
> >  			io->urbs [i]->transfer_dma = sg_dma_address (sg + i);
> >  			len = sg_dma_len (sg + i);
> > -		} else {
> > -			/* hc may use _only_ transfer_buffer */
> > +#ifndef CONFIG_HIGHMEM
> >  			io->urbs [i]->transfer_buffer =
> >  				page_address (sg [i].page) + sg [i].offset;
> 
> Insert here:
> 
> +#else
> +			io->urbs[i]->transfer_buffer = NULL;
> 
> > +#endif
> > +		} else {
> >  			len = sg [i].length;
> > +			io->urbs [i]->transfer_buffer =
> > +				page_address (sg [i].page) + sg [i].offset;
> >  		}
> 
> Don't duplicate the old non-approved spacing style in new code.  This
> should say
> 
> +			io->urbs[i]->transfer_buffer =
> +				page_address(sg[i].page) + sg[i].offset;
> 
> If you like, you can change the other occurrence of this to match.

OK, changed. New patch attached.

Regards,

Tony

-------------------------------------------------------------------------
Take Surveys. Earn Cash. Influence the Future of IT
Join SourceForge.net's Techsay panel and you'll get the chance to share your
opinions on IT & business topics through brief surveys-and earn cash
http://www.techsay.com/default.php?page=join.php&p=sourceforge&CID=DEVDEV

_______________________________________________
[email protected]
To unsubscribe, use the last form field at:
https://lists.sourceforge.net/lists/listinfo/linux-usb-devel

From: Tony Lindgren <[email protected]>
Subject: USB: Allow transfer_buffer with transfer_dma

In case a DMA channel for an endpoint is temporarily
unavailable, the host controller driver may still be
able to transfer the data using PIO. Without this patch
the transfer_buffer is not mapped, and host controller
does not know what to use for PIO.

Signed-off-by: Tony Lindgren <[email protected]>

===================================================================
--- a/drivers/usb/core/message.c
+++ b/drivers/usb/core/message.c
@@ -412,15 +412,28 @@ int usb_sg_init (
 		io->urbs [i]->status = -EINPROGRESS;
 		io->urbs [i]->actual_length = 0;
 
+                /*
+		 * Some systems can revert to PIO if DMA is temporarily
+		 * unavailable; for their sake both transfer_buffer and
+		 * transfer_dma must be set properly.  However this is
+		 * possible only on systems with no high memory, since
+		 * DMA buffers located in high memory are not directly
+		 * addressable by the kernel for PIO.  When high memory
+		 * is available, we NULL out transfer_buffer to avoid
+		 * keeping a stale pointer and to help spot possible
+		 * addressing bugs.
+		 */
 		if (dma) {
-			/* hc may use _only_ transfer_dma */
-			io->urbs [i]->transfer_dma = sg_dma_address (sg + i);
-			len = sg_dma_len (sg + i);
+			io->urbs[i]->transfer_dma = sg_dma_address(sg + i);
+			len = sg_dma_len(sg + i);
+#ifndef CONFIG_HIGHMEM
+			io->urbs[i]->transfer_buffer =
+				page_address(sg[i].page) + sg[i].offset;
+#endif
 		} else {
-			/* hc may use _only_ transfer_buffer */
-			io->urbs [i]->transfer_buffer =
-				page_address (sg [i].page) + sg [i].offset;
-			len = sg [i].length;
+			len = sg[i].length;
+			io->urbs[i]->transfer_buffer =
+				page_address(sg[i].page) + sg[i].offset;
 		}
 
 		if (length) {