Re: [PATCH] USB: Always map transfer_buffer (Fixes some musb_hdrc issues), take #3

* David Brownell <[email protected]> [070328 14:46]:
> On Wednesday 28 March 2007 9:09 am, Alan Stern wrote:
> > On Wed, 28 Mar 2007, Tony Lindgren wrote:
> > 
> > > Hi,
> > > 
> > > Here's a little patch to make sure tranfer_buffer is always if DMA
> > > is temporarily unavailable. I ran into this with tusb6010 that
> > > currently can only do limited DMA. This fixes a problem of connecting
> > > storage devices and doing mixed DMA/PIO access.
> 
> And isn't that just nasty code, too.  Three different DMA engines,
> three different blocks of unsharable code in the RX and TX paths
> of both host and peripheral side.  Partly that's just nasty code
> leftover from Mentor ... but a lot of it seems to be because of
> some software-unfriendly factoring of the DMA interface.

Yes, yuk!

> > Your patch isn't correct, or at least, it is very misleading.  It calls 
> > page_address() in a situation where it cannot be certain that the page 
> > frame lies in accessible kernel memory.  On some systems the value it 
> > calculates will be garbage.
> 
> That is, the usb_buffer_map_sg() can work on highmem pages (which don't
> tend to exist on ARM!) so that there *is* no valid address to use with
> transfer_buffer.  If you're going to have such code, then it needs to
> encapsulate some "ifdef CONFIG_HIGHMEM" logic to ensure that the current
> behavior applies unless there's no highmem.

OK, attached is take #3 with ifndef CONFIG_HIGHMEM. Anybody have other
limitations in mind?

> ISTR another issue motivating the either/or use of dma/cpu addresses.
> It had to do with the dma unmapping operation ... in some cases it
> will invalidate buffer copies in the CPU cache.  That'd mean that if
> it were appropriate in some case to use the CPU address when a DMA
> address is provided, the lowlevel code should write back those parts
> of the dcache, so that if the unmap invalidates them then the data in
> memory is still correct.

Yeah, the tusb6010_omap.c is already doing dma_sync_single_for_cpu()
or dma_sync_single_for_device() but it may be needed elsewhere too.

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: Always map transfer_buffer

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]>

Index: linux-omap-2.6/drivers/usb/core/message.c
===================================================================
--- linux-omap-2.6.orig/drivers/usb/core/message.c	2007-03-28 10:59:21.000000000 -0400
+++ linux-omap-2.6/drivers/usb/core/message.c	2007-03-28 15:26:59.000000000 -0400
@@ -412,15 +412,23 @@ int usb_sg_init (
 		io->urbs [i]->status = -EINPROGRESS;
 		io->urbs [i]->actual_length = 0;
 
+		/*
+		 * 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.
+		 */
 		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;
+#endif
+		} else {
 			len = sg [i].length;
+			io->urbs [i]->transfer_buffer =
+				page_address (sg [i].page) + sg [i].offset;
 		}
 
 		if (length) {