[PATCH] OMAP: fix DMA vs memory ordering

[PATCH] OMAP: fix DMA vs memory ordering

[Russell King - ARM Linux]

Using coherent DMA memory with the OMAP DMA engine results in
unpredictable behaviour due to memory ordering issues; as things stand,
there is no guarantee that data written to coherent DMA memory will be
visible to the DMA hardware.

This is because the OMAP dma_write() accessor contains no barriers,
necessary on ARMv6 and above.  The effect of this can be seen in comments
in the OMAP serial driver, which incorrectly talks about cache flushing
for the coherent DMA stuff.

Rather than adding barriers to the accessors, add it in the DMA support
code just before we enable DMA, and just after we disable DMA.  This
avoids having barriers for every DMA register access.

Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
---
Is this why no one uses DMA with the serial driver?

 arch/arm/plat-omap/dma.c |   14 ++++++++++++++
 1 files changed, 14 insertions(+), 0 deletions(-)

diff --git a/arch/arm/plat-omap/dma.c b/arch/arm/plat-omap/dma.c
index ecdb3da..c58d896 100644
--- a/arch/arm/plat-omap/dma.c
+++ b/arch/arm/plat-omap/dma.c
@@ -916,6 +916,13 @@ void omap_start_dma(int lch)
 			l |= OMAP_DMA_CCR_BUFFERING_DISABLE;
 	l |= OMAP_DMA_CCR_EN;
 
+	/*
+	 * As dma_write() uses IO accessors which are weakly ordered, there
+	 * is no guarantee that data in coherent DMA memory will be visible
+	 * to the DMA device.  Add a memory barrier here to ensure that any
+	 * such data is visible prior to enabling DMA.
+	 */
+	mb();
 	p->dma_write(l, CCR, lch);
 
 	dma_chan[lch].flags |= OMAP_DMA_ACTIVE;
@@ -965,6 +972,13 @@ void omap_stop_dma(int lch)
 		p->dma_write(l, CCR, lch);
 	}
 
+	/*
+	 * Ensure that data transferred by DMA is visible to any access
+	 * after DMA has been disabled.  This is important for coherent
+	 * DMA regions.
+	 */
+	mb();
+
 	if (!omap_dma_in_1510_mode() && dma_chan[lch].next_lch != -1) {
 		int next_lch, cur_lch = lch;
 		char dma_chan_link_map[dma_lch_count];

[PATCH] OMAP: fix DMA vs memory ordering

[Santosh Shilimkar]

On Saturday 14 April 2012 06:54 PM, Russell King - ARM Linux wrote:
> Using coherent DMA memory with the OMAP DMA engine results in
> unpredictable behaviour due to memory ordering issues; as things stand,
> there is no guarantee that data written to coherent DMA memory will be
> visible to the DMA hardware.
> 
> This is because the OMAP dma_write() accessor contains no barriers,
> necessary on ARMv6 and above.  The effect of this can be seen in comments
> in the OMAP serial driver, which incorrectly talks about cache flushing
> for the coherent DMA stuff.
> 
> Rather than adding barriers to the accessors, add it in the DMA support
> code just before we enable DMA, and just after we disable DMA.  This
> avoids having barriers for every DMA register access.
> 
> Signed-off-by: Russell King <rmk+kernel@arm.linux.org.uk>
> ---
I don't why the synchronisation in dma_[start/stop] was
missing. The patch indeed make complete sense.



> Is this why no one uses DMA with the serial driver?
>
Will check if this patch helps in some of the DMA issues
seen in serial driver.

Regards
Santosh

[PATCH] OMAP: fix DMA vs memory ordering

[Tony Lindgren]

Hi,

* Russell King - ARM Linux <linux@arm.linux.org.uk> [120414 06:27]:
> Using coherent DMA memory with the OMAP DMA engine results in
> unpredictable behaviour due to memory ordering issues; as things stand,
> there is no guarantee that data written to coherent DMA memory will be
> visible to the DMA hardware.
> 
> This is because the OMAP dma_write() accessor contains no barriers,
> necessary on ARMv6 and above.  The effect of this can be seen in comments
> in the OMAP serial driver, which incorrectly talks about cache flushing
> for the coherent DMA stuff.
> 
> Rather than adding barriers to the accessors, add it in the DMA support
> code just before we enable DMA, and just after we disable DMA.  This
> avoids having barriers for every DMA register access.

Good catch.

I guess what you're saying is we don't want to do mb() for each completed
transfer that calls the driver specific callback function?

Other than that:

Acked-by: Tony Lindgren <tony@atomide.com>