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

PATCH New Santosh Shilimkar 5 participants 7 messages Aug 5, 2010

Subsystems (2)

USB MUSB multipoint high speed dual-role controller

lore

Share & tools

lore

Santosh Shilimkar

author

2010-08-05 09:53 UTC #0 lore raw

PATCH 1/1 usb: musb: host: Issue a memory barrier before staring the DMA

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 <[email protected]>

Signed-off-by Santosh Shilimkar <[email protected]>

Cc Russell King <[email protected]>

Cc Felipe Balbi <[email protected]>

---
 drivers/usb/musb/musb_host.c |    6 ++++++
 1 files changed, 6 insertions(+), 0 deletions(-)

drivers/usb/musb/musb_host.c +5 -0

Index: 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;

Sergei Shtylyov

2010-08-05 12:40 UTC #1 lore raw

PATCH 1/1 usb: musb: host: Issue a memory barrier before staring the DMA

Hello.

Quoted 26 lines · 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 <[email protected]>
Signed-off-by Santosh Shilimkar <[email protected]>
Cc Russell King <[email protected]>
Cc Felipe Balbi <[email protected]>
---
 drivers/usb/musb/musb_host.c |    6 ++++++
 1 files changed, 6 insertions(+), 0 deletions(-)
Index: omap4_integration/drivers/usb/musb/musb_host.c
===================================================================

drivers/usb/musb/musb_host.c static bool musb_tx_dma_program(struct d · line 635

Quoted 8 lines

drivers/usb/musb/musb_host.c +5 -0

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

Maulik

2010-08-05 12:54 UTC #2 lore raw

PATCH 1/1 usb: musb: host: Issue a memory barrier before staring the DMA

Quoted 43 lines

-----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.
Quoted 28 lines · 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 <[email protected]>
Signed-off-by Santosh Shilimkar <[email protected]>
Cc Russell King <[email protected]>
Cc Felipe Balbi <[email protected]>
---
 drivers/usb/musb/musb_host.c |    6 ++++++
 1 files changed, 6 insertions(+), 0 deletions(-)
Index: omap4_integration/drivers/usb/musb/musb_host.c
===================================================================
drivers/usb/musb/musb_host.c +0 -0
@@ -628,6 +628,11 @@ static bool musb_tx_dma_program(struct d
 	u8			mode;
 

Quoted 8 lines

drivers/usb/musb/musb_host.c static bool musb_tx_dma_program(struct d · line 635
Quoted 6 lines
drivers/usb/musb/musb_host.c +5 -0
 #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

Russell King - ARM Linux

2010-08-05 13:12 UTC #3 lore raw

PATCH 1/1 usb: musb: host: Issue a memory barrier before staring the DMA

Quoted 16 lines · Maulik

Quoted 7 lines
Quoted 7 lines
Index: omap4_integration/drivers/usb/musb/musb_host.c
===================================================================
drivers/usb/musb/musb_host.c +0 -0
@@ -628,6 +628,11 @@ static bool musb_tx_dma_program(struct d
 	u8			mode;
 
Quoted 8 lines
drivers/usb/musb/musb_host.c static bool musb_tx_dma_program(struct d · line 635
Quoted 6 lines
drivers/usb/musb/musb_host.c +5 -0
 #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?

Quoted 2 lines

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.

Shilimkar, Santosh

author

2010-08-05 13:27 UTC #4 lore raw

PATCH 1/1 usb: musb: host: Issue a memory barrier before staring the DMA

Quoted 37 lines

-----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
Quoted 16 lines · Maulik
Quoted 7 lines
Quoted 7 lines
Index: omap4_integration/drivers/usb/musb/musb_host.c
===================================================================
drivers/usb/musb/musb_host.c +0 -0
@@ -628,6 +628,11 @@ static bool musb_tx_dma_program(struct d
 	u8			mode;
 
Quoted 8 lines
drivers/usb/musb/musb_host.c static bool musb_tx_dma_program(struct d · line 635
Quoted 6 lines
drivers/usb/musb/musb_host.c +5 -0
 #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?
Quoted 1 line
Yes. This is seen only on SMP system such as OMAP4 which uses Inventra
DMA.
Quoted 1 line
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.

Quoted 5 lines

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

Alan Stern

2010-08-05 15:03 UTC #5 lore raw

PATCH 1/1 usb: musb: host: Issue a memory barrier before staring the DMA

In reply to Santosh Shilimkar

Quoted 9 lines · 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.

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

Shilimkar, Santosh

author

2010-08-05 15:10 UTC #6 lore raw

PATCH 1/1 usb: musb: host: Issue a memory barrier before staring the DMA

Quoted 21 lines

-----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
Quoted 9 lines · 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.

Quoted 4 lines

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