[PATCH] isp1760: Flush the D-cache for the pipe-in transfer buffers

[PATCH] isp1760: Flush the D-cache for the pipe-in transfer buffers

[Catalin Marinas]

When the HDC driver writes the data to the transfer buffers it pollutes
the D-cache (unlike DMA drivers where the device writes the data). If
the corresponding pages get mapped into user space, there are no
additional cache flushing operations performed and this causes random
user space faults on architectures with separate I and D caches
(Harvard) or those with aliasing D-cache.

Signed-off-by: Catalin Marinas <catalin.marinas@arm.com>
Cc: Matthew Dharm <mdharm-kernel@one-eyed-alien.net>
Cc: Greg KH <greg@kroah.com>
Cc: Sebastian Siewior <bigeasy@linutronix.de>
---
 drivers/usb/host/isp1760-hcd.c |   10 ++++++++++
 1 files changed, 10 insertions(+), 0 deletions(-)

diff --git a/drivers/usb/host/isp1760-hcd.c b/drivers/usb/host/isp1760-hcd.c
index 27b8f7c..51445ad 100644
--- a/drivers/usb/host/isp1760-hcd.c
+++ b/drivers/usb/host/isp1760-hcd.c
@@ -17,7 +17,9 @@
 #include <linux/debugfs.h>
 #include <linux/uaccess.h>
 #include <linux/io.h>
+#include <linux/mm.h>
 #include <asm/unaligned.h>
+#include <asm/cacheflush.h>
 
 #include "../core/hcd.h"
 #include "isp1760-hcd.h"
@@ -904,6 +906,14 @@ __acquires(priv->lock)
 			status = 0;
 	}
 
+	if (usb_pipein(urb->pipe) && usb_pipetype(urb->pipe) == PIPE_BULK) {
+		void *ptr;
+		for (ptr = urb->transfer_buffer;
+		     ptr < urb->transfer_buffer + urb->transfer_buffer_length;
+		     ptr += PAGE_SIZE)
+			flush_dcache_page(virt_to_page(ptr));
+	}
+
 	/* complete() can reenter this HCD */
 	usb_hcd_unlink_urb_from_ep(priv_to_hcd(priv), urb);
 	spin_unlock(&priv->lock);

Re: [PATCH] isp1760: Flush the D-cache for the pipe-in transfer buffers

[Sebastian Andrzej Siewior]

* Catalin Marinas | 2010-02-02 11:11:35 [+0000]:

>When the HDC driver writes the data to the transfer buffers it pollutes
>the D-cache (unlike DMA drivers where the device writes the data). If
>the corresponding pages get mapped into user space, there are no
>additional cache flushing operations performed and this causes random
>user space faults on architectures with separate I and D caches
>(Harvard) or those with aliasing D-cache.

After looking through lib/scatterlist.c it uses 
#include <linux/highmem.h>

and 

flush_kernel_dcache_page().
Wouldn't this do the job here as well?

Sebastain

Re: [PATCH] isp1760: Flush the D-cache for the pipe-in transfer buffers

[Catalin Marinas]

On Tue, 2010-02-02 at 12:08 +0000, Sebastian Andrzej Siewior wrote:
> * Catalin Marinas | 2010-02-02 11:11:35 [+0000]:
> 
> >When the HDC driver writes the data to the transfer buffers it pollutes
> >the D-cache (unlike DMA drivers where the device writes the data). If
> >the corresponding pages get mapped into user space, there are no
> >additional cache flushing operations performed and this causes random
> >user space faults on architectures with separate I and D caches
> >(Harvard) or those with aliasing D-cache.
> 
> After looking through lib/scatterlist.c it uses
> #include <linux/highmem.h>
> 
> and
> 
> flush_kernel_dcache_page().
> Wouldn't this do the job here as well?

The documentation implies that this is to be used only with pages
obtained with kmap(), which doesn't seem to be the case here. On some
ARM processors it isn't even implemented (kunmap does the necessary
flushing).

-- 
Catalin

Re: [PATCH] isp1760: Flush the D-cache for the pipe-in transfer buffers

[Ming Lei]

2010/2/2 Catalin Marinas <catalin.marinas@arm.com>:

> +       if (usb_pipein(urb->pipe) && usb_pipetype(urb->pipe) == PIPE_BULK) {
> +               void *ptr;
> +               for (ptr = urb->transfer_buffer;
> +                    ptr < urb->transfer_buffer + urb->transfer_buffer_length;
> +                    ptr += PAGE_SIZE)
> +                       flush_dcache_page(virt_to_page(ptr));

If the page is mapped into highmem, seems virt_to_page doesn't work well.

-- 
Lei Ming

Re: [PATCH] isp1760: Flush the D-cache for the pipe-in transfer buffers

[Catalin Marinas]

On Tue, 2010-02-02 at 15:41 +0000, Ming Lei wrote:
> 2010/2/2 Catalin Marinas <catalin.marinas@arm.com>:
> 
> > +       if (usb_pipein(urb->pipe) && usb_pipetype(urb->pipe) == PIPE_BULK) {
> > +               void *ptr;
> > +               for (ptr = urb->transfer_buffer;
> > +                    ptr < urb->transfer_buffer + urb->transfer_buffer_length;
> > +                    ptr += PAGE_SIZE)
> > +                       flush_dcache_page(virt_to_page(ptr));
> 
> If the page is mapped into highmem, seems virt_to_page doesn't work well.

You are right but is it possible that we get a highmem virtual address
in this case?

As for a general solution, maybe trying to define something like a pio_*
API would work better.

-- 
Catalin

Re: [PATCH] isp1760: Flush the D-cache for the pipe-in transfer buffers

[Sebastian Andrzej Siewior]

Ming Lei wrote:
> 2010/2/2 Catalin Marinas <catalin.marinas@arm.com>:
> 
>> +       if (usb_pipein(urb->pipe) && usb_pipetype(urb->pipe) == PIPE_BULK) {
>> +               void *ptr;
>> +               for (ptr = urb->transfer_buffer;
>> +                    ptr < urb->transfer_buffer + urb->transfer_buffer_length;
>> +                    ptr += PAGE_SIZE)
>> +                       flush_dcache_page(virt_to_page(ptr));
> 
> If the page is mapped into highmem, seems virt_to_page doesn't work well.

This does not matter because this can not happen :). If the buffer is
highmem then a bounce buffer is used. See commit 96983d2d8 aka "USB:
storage: set bounce limit for non-DMA-capable host controllers"


Sebastian