[PATCH] usb: musb: host: Issue a memory barrier before staring the DMA

[PATCH] usb: musb: host: Issue a memory barrier before staring the DMA

[Santosh Shilimkar]

This patch fixes the issue which was observed while transfering
a large file ( > 20MB) over USB (OMAP MUSB controller acts as USB host)
to an attached USB thumb drive.

It was found that CDB field of CBW packet was set to 0x0. This was
due to missing a barrier before DMA engine starts transfer.
This  buffer is  allocated using dma_alloc_coherent which gives
non-cacheble but bufferable memory and hence needed a write
memory barrier to flush the write buffer.

More info on this thread is here:
	http://www.spinics.net/lists/linux-omap/msg33987.html

Signed-off-by: Maulik Mankad <x0082077@ti.com>
Signed-off-by: Santosh Shilimkar <santosh.shilimkar@ti.com>
Cc: Russell King <rmk+kernel@arm.linux.org.uk>
Cc: Felipe Balbi <felipebalbi@users.sourceforge.net>
---
 drivers/usb/musb/musb_host.c |    6 ++++++
 1 files changed, 6 insertions(+), 0 deletions(-)

Index: omap4_integration/drivers/usb/musb/musb_host.c
===================================================================
--- omap4_integration.orig/drivers/usb/musb/musb_host.c
+++ omap4_integration/drivers/usb/musb/musb_host.c
@@ -628,6 +628,11 @@ static bool musb_tx_dma_program(struct d
 	u8			mode;
 
 #ifdef	CONFIG_USB_INVENTRA_DMA
+	/*
+	 * Ensure the data reaches to main memory before starting
+	 * DMA transfer
+	 */
+	wmb();
 	if (length > channel->max_len)
 		length = channel->max_len;
 

[PATCH] usb: musb: host: Issue a memory barrier before staring the DMA

[Sergei Shtylyov]

Hello.

Santosh Shilimkar wrote:

> This patch fixes the issue which was observed while transfering
> a large file ( > 20MB) over USB (OMAP MUSB controller acts as USB host)
> to an attached USB thumb drive.

> It was found that CDB field of CBW packet was set to 0x0. This was
> due to missing a barrier before DMA engine starts transfer.
> This  buffer is  allocated using dma_alloc_coherent which gives
> non-cacheble but bufferable memory and hence needed a write
> memory barrier to flush the write buffer.

> More info on this thread is here:
> 	http://www.spinics.net/lists/linux-omap/msg33987.html

> Signed-off-by: Maulik Mankad <x0082077@ti.com>
> Signed-off-by: Santosh Shilimkar <santosh.shilimkar@ti.com>
> Cc: Russell King <rmk+kernel@arm.linux.org.uk>
> Cc: Felipe Balbi <felipebalbi@users.sourceforge.net>
> ---
>  drivers/usb/musb/musb_host.c |    6 ++++++
>  1 files changed, 6 insertions(+), 0 deletions(-)

> Index: omap4_integration/drivers/usb/musb/musb_host.c
> ===================================================================
> --- omap4_integration.orig/drivers/usb/musb/musb_host.c
> +++ omap4_integration/drivers/usb/musb/musb_host.c
> @@ -628,6 +628,11 @@ static bool musb_tx_dma_program(struct d
>  	u8			mode;
>  
>  #ifdef	CONFIG_USB_INVENTRA_DMA
> +	/*
> +	 * Ensure the data reaches to main memory before starting
> +	 * DMA transfer
> +	 */
> +	wmb();

    Are you sure this is only needed for Inventra DMA?

WBR, Sergei

[PATCH] usb: musb: host: Issue a memory barrier before staring the DMA

[Maulik]

> -----Original Message-----
> From: Sergei Shtylyov [mailto:sshtylyov at mvista.com]
> Sent: Thursday, August 05, 2010 6:11 PM
> To: Santosh Shilimkar
> Cc: linux-usb at vger.kernel.org; linux-arm-kernel at lists.infradead.org;
> Maulik Mankad; Russell King; Felipe Balbi
> Subject: Re: [PATCH] usb: musb: host: Issue a memory barrier before
> staring the DMA
> 
> Hello.
> 
> Santosh Shilimkar wrote:
> 
> > This patch fixes the issue which was observed while transfering
> > a large file ( > 20MB) over USB (OMAP MUSB controller acts as USB host)
> > to an attached USB thumb drive.
> 
> > It was found that CDB field of CBW packet was set to 0x0. This was
> > due to missing a barrier before DMA engine starts transfer.
> > This  buffer is  allocated using dma_alloc_coherent which gives
> > non-cacheble but bufferable memory and hence needed a write
> > memory barrier to flush the write buffer.
> 
> > More info on this thread is here:
> > 	http://www.spinics.net/lists/linux-omap/msg33987.html
> 
> > Signed-off-by: Maulik Mankad <x0082077@ti.com>
> > Signed-off-by: Santosh Shilimkar <santosh.shilimkar@ti.com>
> > Cc: Russell King <rmk+kernel@arm.linux.org.uk>
> > Cc: Felipe Balbi <felipebalbi@users.sourceforge.net>
> > ---
> >  drivers/usb/musb/musb_host.c |    6 ++++++
> >  1 files changed, 6 insertions(+), 0 deletions(-)
> 
> > Index: omap4_integration/drivers/usb/musb/musb_host.c
> > ===================================================================
> > --- omap4_integration.orig/drivers/usb/musb/musb_host.c
> > +++ omap4_integration/drivers/usb/musb/musb_host.c
> > @@ -628,6 +628,11 @@ static bool musb_tx_dma_program(struct d
> >  	u8			mode;
> >
> >  #ifdef	CONFIG_USB_INVENTRA_DMA
> > +	/*
> > +	 * Ensure the data reaches to main memory before starting
> > +	 * DMA transfer
> > +	 */
> > +	wmb();
> 
>     Are you sure this is only needed for Inventra DMA?

Yes. This is seen only on SMP system such as OMAP4 which uses Inventra DMA.
We did not see the issue when SMP is disabled. 

Regards,
Maulik

[PATCH] usb: musb: host: Issue a memory barrier before staring the DMA

[Russell King - ARM Linux]

On Thu, Aug 05, 2010 at 06:24:31PM +0530, Maulik wrote:
> > > Index: omap4_integration/drivers/usb/musb/musb_host.c
> > > ===================================================================
> > > --- omap4_integration.orig/drivers/usb/musb/musb_host.c
> > > +++ omap4_integration/drivers/usb/musb/musb_host.c
> > > @@ -628,6 +628,11 @@ static bool musb_tx_dma_program(struct d
> > >  	u8			mode;
> > >
> > >  #ifdef	CONFIG_USB_INVENTRA_DMA
> > > +	/*
> > > +	 * Ensure the data reaches to main memory before starting
> > > +	 * DMA transfer
> > > +	 */
> > > +	wmb();
> > 
> >     Are you sure this is only needed for Inventra DMA?
> 
> Yes. This is seen only on SMP system such as OMAP4 which uses Inventra DMA.
> We did not see the issue when SMP is disabled. 

It'll be far safer to actually consider what's going on here, and realise
that it's NOT a SMP issue but a write ordering issue.

What that means is that as OMAP systems progress, and the ordering becomes
weaker, you will see the problem appearing on non-SMP.  So why not fix it
properly now and avoid having to revisit this with yet more patches.

Think about what happens if writes to memory (DMA or otherwise) in the
program become mis-ordered with writes to the device registers - and fix
anything where that ordering matters by adding the necessary barriers.
This will save you having to constantly revisit the driver time and time
again to move the barriers around.

[PATCH] usb: musb: host: Issue a memory barrier before staring the DMA

[Shilimkar, Santosh]

> -----Original Message-----
> From: Russell King - ARM Linux [mailto:linux at arm.linux.org.uk]
> Sent: Thursday, August 05, 2010 6:42 PM
> To: Mankad, Maulik Ojas
> Cc: 'Sergei Shtylyov'; Shilimkar, Santosh; linux-usb at vger.kernel.org;
> linux-arm-kernel at lists.infradead.org; 'Felipe Balbi'
> Subject: Re: [PATCH] usb: musb: host: Issue a memory barrier before
> staring the DMA
> 
> On Thu, Aug 05, 2010 at 06:24:31PM +0530, Maulik wrote:
> > > > Index: omap4_integration/drivers/usb/musb/musb_host.c
> > > > ===================================================================
> > > > --- omap4_integration.orig/drivers/usb/musb/musb_host.c
> > > > +++ omap4_integration/drivers/usb/musb/musb_host.c
> > > > @@ -628,6 +628,11 @@ static bool musb_tx_dma_program(struct d
> > > >  	u8			mode;
> > > >
> > > >  #ifdef	CONFIG_USB_INVENTRA_DMA
> > > > +	/*
> > > > +	 * Ensure the data reaches to main memory before starting
> > > > +	 * DMA transfer
> > > > +	 */
> > > > +	wmb();
> > >
> > >     Are you sure this is only needed for Inventra DMA?
> >
> > Yes. This is seen only on SMP system such as OMAP4 which uses Inventra
> DMA.
> > We did not see the issue when SMP is disabled.
> 
> It'll be far safer to actually consider what's going on here, and realise
> that it's NOT a SMP issue but a write ordering issue.
> 
> What that means is that as OMAP systems progress, and the ordering becomes
> weaker, you will see the problem appearing on non-SMP.  So why not fix it
> properly now and avoid having to revisit this with yet more patches.
> 
> Think about what happens if writes to memory (DMA or otherwise) in the
> program become mis-ordered with writes to the device registers - and fix
> anything where that ordering matters by adding the necessary barriers.
> This will save you having to constantly revisit the driver time and time
> again to move the barriers around.

This patch fixes all OMAP processors. The other architecture Da-Vinci which uses same Mentor USB controller but different DMA engine(CPPI) may need this
fix but we are not sure about this.

Hence only OMAP is patched.

Regards,
Santosh

[PATCH] usb: musb: host: Issue a memory barrier before staring the DMA

[Alan Stern]

On Thu, 5 Aug 2010, Santosh Shilimkar wrote:

> This patch fixes the issue which was observed while transfering
> a large file ( > 20MB) over USB (OMAP MUSB controller acts as USB host)
> to an attached USB thumb drive.
> 
> It was found that CDB field of CBW packet was set to 0x0. This was
> due to missing a barrier before DMA engine starts transfer.
> This  buffer is  allocated using dma_alloc_coherent which gives
> non-cacheble but bufferable memory and hence needed a write
> memory barrier to flush the write buffer.

If you see this problem when the controller uses DMA acting as a host,
does this mean the same problem might occur when it uses DMA acting as
a peripheral?

Alan Stern

[PATCH] usb: musb: host: Issue a memory barrier before staring the DMA

[Shilimkar, Santosh]

> -----Original Message-----
> From: Alan Stern [mailto:stern at rowland.harvard.edu]
> Sent: Thursday, August 05, 2010 8:34 PM
> To: Shilimkar, Santosh
> Cc: linux-usb at vger.kernel.org; linux-arm-kernel at lists.infradead.org;
> Mankad, Maulik Ojas; Russell King; Felipe Balbi
> Subject: Re: [PATCH] usb: musb: host: Issue a memory barrier before
> staring the DMA
> 
> On Thu, 5 Aug 2010, Santosh Shilimkar wrote:
> 
> > This patch fixes the issue which was observed while transfering
> > a large file ( > 20MB) over USB (OMAP MUSB controller acts as USB host)
> > to an attached USB thumb drive.
> >
> > It was found that CDB field of CBW packet was set to 0x0. This was
> > due to missing a barrier before DMA engine starts transfer.
> > This  buffer is  allocated using dma_alloc_coherent which gives
> > non-cacheble but bufferable memory and hence needed a write
> > memory barrier to flush the write buffer.
> 
> If you see this problem when the controller uses DMA acting as a host,
> does this mean the same problem might occur when it uses DMA acting as
> a peripheral?
> 
The peripheral USB drivers seems to have using standard kernel dma-mapping
framework which internally takes care of the ordering issues.

This is one of the cases where buffer is allocated using "dma_alloc_coherent" for which except ordering issue , there is no
maintainace is necessary.

Regards,
Santosh