Skip to contentSkip to conversation

dmaengine: mv_xor: use writel() instead of relaxed variant

PATCH In review Rosen Penev 2 participants 6 messages Sep 16 – Oct 20 dmaengine

lore
Share & tools
lore

Rosen Penev

author
#0 lore raw

PATCH 1/1 dmaengine: mv_xor: use writel() instead of relaxed variant

Fix the ordering guarantees between descriptor memory writes and the MMIO accesses that expose them to the engine. The descriptor pool is allocated with dma_alloc_wc(), so writes sit in the CPU write buffers and must be drained before the engine is allowed to read them.

Program the next-descriptor pointer with writel() instead of writel_relaxed(); the full accessor drains prior memory writes before the MMIO write.

In mv_xor_tx_submit() the appended chain link must be visible before the busy-status register is read. Use a full mb() so the engine cannot fetch a stale chain link and stall the channel after the CPU decides it is still busy.

Fixes ff7b04796d98 ("dmaengine: DMA engine driver for Marvell XOR engine")
Assisted-by LLM
Signed-off-by Rosen Penev <[email protected]>
---
 drivers/dma/mv_xor.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)
drivers/dma/mv_xor.c +9 -1
index c4e0dce3ca64..0de2b1ad5c30 100644
@@ -103,7 +103,10 @@ static u32 mv_chan_get_current_desc(struct mv_xor_chan *chan)
 static void mv_chan_set_next_descriptor(struct mv_xor_chan *chan,
 					u32 next_desc_addr)
 {
-	writel_relaxed(next_desc_addr, XOR_NEXT_DESC(chan));
+	/* writel drains descriptor writes to DRAM before the engine
+	 * is pointed at them
+	 */
+	writel(next_desc_addr, XOR_NEXT_DESC(chan));
 }
 
 static void mv_chan_unmask_interrupts(struct mv_xor_chan *chan)

@@ -410,6 +413,11 @@ mv_xor_tx_submit(struct dma_async_tx_descriptor *tx)
 		/* fix up the hardware chain */
 		mv_desc_set_next_desc(old_chain_tail, sw_desc->async_tx.phys);
 
+		/* make the new link visible to the engine before we read
+		 * the channel state, the device may fetch it at any point
+		 */
+		mb();
+
 		/* if the channel is not busy */
 		if (!mv_chan_is_busy(mv_chan)) {
 			u32 current_desc = mv_chan_get_current_desc(mv_chan);
Signature · 1 line
2.55.0
#1 lore raw
Quoted 26 lines · Rosen Penev

Fix the ordering guarantees between descriptor memory writes and the MMIO accesses that expose them to the engine. The descriptor pool is allocated with dma_alloc_wc(), so writes sit in the CPU write buffers and must be drained before the engine is allowed to read them.

Program the next-descriptor pointer with writel() instead of writel_relaxed(); the full accessor drains prior memory writes before the MMIO write.

In mv_xor_tx_submit() the appended chain link must be visible before the busy-status register is read. Use a full mb() so the engine cannot fetch a stale chain link and stall the channel after the CPU decides it is still busy.

Fixes ff7b04796d98 ("dmaengine: DMA engine driver for Marvell XOR engine")
Assisted-by LLM
Signed-off-by Rosen Penev <[email protected]>
---
 drivers/dma/mv_xor.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)
drivers/dma/mv_xor.c +0 -0
index c4e0dce3ca64..0de2b1ad5c30 100644

drivers/dma/mv_xor.c static u32 mv_chan_get_current_desc(struct mv_xor_chan *chan) · line 109

Quoted 8 lines
drivers/dma/mv_xor.c +4 -1
@@ -103,7 +103,10 @@ static u32 mv_chan_get_current_desc(struct mv_xor_chan *chan)
 static void mv_chan_set_next_descriptor(struct mv_xor_chan *chan,
 					u32 next_desc_addr)
 {
-	writel_relaxed(next_desc_addr, XOR_NEXT_DESC(chan));
+	/* writel drains descriptor writes to DRAM before the engine
+	 * is pointed at them
+	 */
+	writel(next_desc_addr, XOR_NEXT_DESC(chan));

I remember this my comments before, should be v2 version?

drivers/dma/mv_xor.c mv_xor_tx_submit(struct dma_async_tx_descriptor *tx) · line 420

Quoted 12 lines
drivers/dma/mv_xor.c +5 -0
 }
 
 static void mv_chan_unmask_interrupts(struct mv_xor_chan *chan)

@@ -410,6 +413,11 @@ mv_xor_tx_submit(struct dma_async_tx_descriptor *tx)
 		/* fix up the hardware chain */
 		mv_desc_set_next_desc(old_chain_tail, sw_desc->async_tx.phys);
 
+		/* make the new link visible to the engine before we read
+		 * the channel state, the device may fetch it at any point
+		 */
+		mb();
+

The first is readl_relaxed() in mv_chan_is_busy(), all io readl and writel is ordered.

which part need mb() here?

Frank

Quoted 6 lines
drivers/dma/mv_xor.c +0 -1
 		/* if the channel is not busy */
 		if (!mv_chan_is_busy(mv_chan)) {
 			u32 current_desc = mv_chan_get_current_desc(mv_chan);
--

2.55.0

author
#2 lore raw
Quoted 55 lines · Frank Li
Quoted 26 lines · Rosen Penev

Fix the ordering guarantees between descriptor memory writes and the MMIO accesses that expose them to the engine. The descriptor pool is allocated with dma_alloc_wc(), so writes sit in the CPU write buffers and must be drained before the engine is allowed to read them.

Program the next-descriptor pointer with writel() instead of writel_relaxed(); the full accessor drains prior memory writes before the MMIO write.

In mv_xor_tx_submit() the appended chain link must be visible before the busy-status register is read. Use a full mb() so the engine cannot fetch a stale chain link and stall the channel after the CPU decides it is still busy.

Fixes ff7b04796d98 ("dmaengine: DMA engine driver for Marvell XOR engine")
Assisted-by LLM
Signed-off-by Rosen Penev <[email protected]>
---
 drivers/dma/mv_xor.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)
drivers/dma/mv_xor.c +0 -0
index c4e0dce3ca64..0de2b1ad5c30 100644

drivers/dma/mv_xor.c static u32 mv_chan_get_current_desc(struct mv_xor_chan *chan) · line 109

Quoted 8 lines
drivers/dma/mv_xor.c +4 -1
@@ -103,7 +103,10 @@ static u32 mv_chan_get_current_desc(struct mv_xor_chan *chan)
 static void mv_chan_set_next_descriptor(struct mv_xor_chan *chan,
                                      u32 next_desc_addr)
 {
-     writel_relaxed(next_desc_addr, XOR_NEXT_DESC(chan));
+     /* writel drains descriptor writes to DRAM before the engine
+      * is pointed at them
+      */
+     writel(next_desc_addr, XOR_NEXT_DESC(chan));

I remember this my comments before, should be v2 version?

drivers/dma/mv_xor.c mv_xor_tx_submit(struct dma_async_tx_descriptor *tx) · line 420

Quoted 12 lines
drivers/dma/mv_xor.c +5 -0
 }
 
 static void mv_chan_unmask_interrupts(struct mv_xor_chan *chan)

@@ -410,6 +413,11 @@ mv_xor_tx_submit(struct dma_async_tx_descriptor *tx)
              /* fix up the hardware chain */
              mv_desc_set_next_desc(old_chain_tail, sw_desc->async_tx.phys);
 
+             /* make the new link visible to the engine before we read
+              * the channel state, the device may fetch it at any point
+              */
+             mb();
+

The first is readl_relaxed() in mv_chan_is_busy(), all io readl and writel is ordered.

Quoted 1 line

which part need mb() here?

It's to deal with readl_relaxed in mv_chan_is_busy. I'll change to regular readl in v2.

Quoted 9 lines

Frank

Quoted 6 lines
drivers/dma/mv_xor.c +0 -1
              /* if the channel is not busy */
              if (!mv_chan_is_busy(mv_chan)) {
                      u32 current_desc = mv_chan_get_current_desc(mv_chan);
--

2.55.0

#3 lore raw
Quoted 56 lines · Rosen Penev
Quoted 55 lines · Frank Li
Quoted 26 lines · Rosen Penev

Fix the ordering guarantees between descriptor memory writes and the MMIO accesses that expose them to the engine. The descriptor pool is allocated with dma_alloc_wc(), so writes sit in the CPU write buffers and must be drained before the engine is allowed to read them.

Program the next-descriptor pointer with writel() instead of writel_relaxed(); the full accessor drains prior memory writes before the MMIO write.

In mv_xor_tx_submit() the appended chain link must be visible before the busy-status register is read. Use a full mb() so the engine cannot fetch a stale chain link and stall the channel after the CPU decides it is still busy.

Fixes ff7b04796d98 ("dmaengine: DMA engine driver for Marvell XOR engine")
Assisted-by LLM
Signed-off-by Rosen Penev <[email protected]>
---
 drivers/dma/mv_xor.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)
drivers/dma/mv_xor.c +0 -0
index c4e0dce3ca64..0de2b1ad5c30 100644

drivers/dma/mv_xor.c static u32 mv_chan_get_current_desc(struct mv_xor_chan *chan) · line 109

Quoted 8 lines
drivers/dma/mv_xor.c +4 -1
@@ -103,7 +103,10 @@ static u32 mv_chan_get_current_desc(struct mv_xor_chan *chan)
 static void mv_chan_set_next_descriptor(struct mv_xor_chan *chan,
                                      u32 next_desc_addr)
 {
-     writel_relaxed(next_desc_addr, XOR_NEXT_DESC(chan));
+     /* writel drains descriptor writes to DRAM before the engine
+      * is pointed at them
+      */
+     writel(next_desc_addr, XOR_NEXT_DESC(chan));

I remember this my comments before, should be v2 version?

drivers/dma/mv_xor.c mv_xor_tx_submit(struct dma_async_tx_descriptor *tx) · line 420

Quoted 12 lines
drivers/dma/mv_xor.c +5 -0
 }
 
 static void mv_chan_unmask_interrupts(struct mv_xor_chan *chan)

@@ -410,6 +413,11 @@ mv_xor_tx_submit(struct dma_async_tx_descriptor *tx)
              /* fix up the hardware chain */
              mv_desc_set_next_desc(old_chain_tail, sw_desc->async_tx.phys);
 
+             /* make the new link visible to the engine before we read
+              * the channel state, the device may fetch it at any point
+              */
+             mb();
+

The first is readl_relaxed() in mv_chan_is_busy(), all io readl and writel is ordered.

Quoted 3 lines
Quoted 1 line

which part need mb() here?

It's to deal with readl_relaxed in mv_chan_is_busy. I'll change to regular readl in v2.

I just want to know which variable need read barrier() ?

Frank

Quoted 9 lines
Quoted 9 lines

Frank

Quoted 6 lines
drivers/dma/mv_xor.c +0 -1
              /* if the channel is not busy */
              if (!mv_chan_is_busy(mv_chan)) {
                      u32 current_desc = mv_chan_get_current_desc(mv_chan);
--

2.55.0

author
#4 lore raw
Quoted 58 lines · Frank Li
Quoted 56 lines · Rosen Penev
Quoted 55 lines · Frank Li
Quoted 26 lines · Rosen Penev

Fix the ordering guarantees between descriptor memory writes and the MMIO accesses that expose them to the engine. The descriptor pool is allocated with dma_alloc_wc(), so writes sit in the CPU write buffers and must be drained before the engine is allowed to read them.

Program the next-descriptor pointer with writel() instead of writel_relaxed(); the full accessor drains prior memory writes before the MMIO write.

In mv_xor_tx_submit() the appended chain link must be visible before the busy-status register is read. Use a full mb() so the engine cannot fetch a stale chain link and stall the channel after the CPU decides it is still busy.

Fixes ff7b04796d98 ("dmaengine: DMA engine driver for Marvell XOR engine")
Assisted-by LLM
Signed-off-by Rosen Penev <[email protected]>
---
 drivers/dma/mv_xor.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)
drivers/dma/mv_xor.c +0 -0
index c4e0dce3ca64..0de2b1ad5c30 100644

drivers/dma/mv_xor.c static u32 mv_chan_get_current_desc(struct mv_xor_chan *chan) · line 109

Quoted 8 lines
drivers/dma/mv_xor.c +4 -1
@@ -103,7 +103,10 @@ static u32 mv_chan_get_current_desc(struct mv_xor_chan *chan)
 static void mv_chan_set_next_descriptor(struct mv_xor_chan *chan,
                                      u32 next_desc_addr)
 {
-     writel_relaxed(next_desc_addr, XOR_NEXT_DESC(chan));
+     /* writel drains descriptor writes to DRAM before the engine
+      * is pointed at them
+      */
+     writel(next_desc_addr, XOR_NEXT_DESC(chan));

I remember this my comments before, should be v2 version?

drivers/dma/mv_xor.c mv_xor_tx_submit(struct dma_async_tx_descriptor *tx) · line 420

Quoted 12 lines
drivers/dma/mv_xor.c +5 -0
 }
 
 static void mv_chan_unmask_interrupts(struct mv_xor_chan *chan)

@@ -410,6 +413,11 @@ mv_xor_tx_submit(struct dma_async_tx_descriptor *tx)
              /* fix up the hardware chain */
              mv_desc_set_next_desc(old_chain_tail, sw_desc->async_tx.phys);
 
+             /* make the new link visible to the engine before we read
+              * the channel state, the device may fetch it at any point
+              */
+             mb();
+

The first is readl_relaxed() in mv_chan_is_busy(), all io readl and writel is ordered.

Quoted 5 lines
Quoted 3 lines
Quoted 1 line

which part need mb() here?

It's to deal with readl_relaxed in mv_chan_is_busy. I'll change to regular readl in v2.

I just want to know which variable need read barrier() ?

Review says:

#### Validity of mb() after mv_desc_set_next_desc()

• Ordering the memory store (old_chain_tail->phy_next_desc) before
the MMIO register load (readl_relaxed(XOR_ACTIVATION) in mv_xor.c:181-188) is necessary to prevent the
CPU from seeing busy == 1 while the link store is still in the CPU's
store buffer, followed by the hardware completing old_chain_tail, reading a stale 0-pointer, and
halting without the CPU restarting it.
• Since readl_relaxed() does not order against prior memory stores,
and dma_wmb() / wmb() only order stores against stores, a mandatory full barrier (mb()) is the
appropriate barrier for ordering a normal/WC memory store before an MMIO read.

Quoted 12 lines

Frank

Quoted 9 lines
Quoted 9 lines

Frank

Quoted 6 lines
drivers/dma/mv_xor.c +0 -1
              /* if the channel is not busy */
              if (!mv_chan_is_busy(mv_chan)) {
                      u32 current_desc = mv_chan_get_current_desc(mv_chan);
--

2.55.0

#5 lore raw
Quoted 65 lines · Rosen Penev
Quoted 58 lines · Frank Li
Quoted 56 lines · Rosen Penev
Quoted 55 lines · Frank Li
Quoted 26 lines · Rosen Penev

Fix the ordering guarantees between descriptor memory writes and the MMIO accesses that expose them to the engine. The descriptor pool is allocated with dma_alloc_wc(), so writes sit in the CPU write buffers and must be drained before the engine is allowed to read them.

Program the next-descriptor pointer with writel() instead of writel_relaxed(); the full accessor drains prior memory writes before the MMIO write.

In mv_xor_tx_submit() the appended chain link must be visible before the busy-status register is read. Use a full mb() so the engine cannot fetch a stale chain link and stall the channel after the CPU decides it is still busy.

Fixes ff7b04796d98 ("dmaengine: DMA engine driver for Marvell XOR engine")
Assisted-by LLM
Signed-off-by Rosen Penev <[email protected]>
---
 drivers/dma/mv_xor.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)
drivers/dma/mv_xor.c +0 -0
index c4e0dce3ca64..0de2b1ad5c30 100644

drivers/dma/mv_xor.c static u32 mv_chan_get_current_desc(struct mv_xor_chan *chan) · line 109

Quoted 8 lines
drivers/dma/mv_xor.c +4 -1
@@ -103,7 +103,10 @@ static u32 mv_chan_get_current_desc(struct mv_xor_chan *chan)
 static void mv_chan_set_next_descriptor(struct mv_xor_chan *chan,
                                      u32 next_desc_addr)
 {
-     writel_relaxed(next_desc_addr, XOR_NEXT_DESC(chan));
+     /* writel drains descriptor writes to DRAM before the engine
+      * is pointed at them
+      */
+     writel(next_desc_addr, XOR_NEXT_DESC(chan));

I remember this my comments before, should be v2 version?

drivers/dma/mv_xor.c mv_xor_tx_submit(struct dma_async_tx_descriptor *tx) · line 420

Quoted 12 lines
drivers/dma/mv_xor.c +5 -0
 }
 
 static void mv_chan_unmask_interrupts(struct mv_xor_chan *chan)

@@ -410,6 +413,11 @@ mv_xor_tx_submit(struct dma_async_tx_descriptor *tx)
              /* fix up the hardware chain */
              mv_desc_set_next_desc(old_chain_tail, sw_desc->async_tx.phys);
 
+             /* make the new link visible to the engine before we read
+              * the channel state, the device may fetch it at any point
+              */
+             mb();
+

The first is readl_relaxed() in mv_chan_is_busy(), all io readl and writel is ordered.

Quoted 5 lines
Quoted 3 lines
Quoted 1 line

which part need mb() here?

It's to deal with readl_relaxed in mv_chan_is_busy. I'll change to regular readl in v2.

I just want to know which variable need read barrier() ?

Quoted 7 lines

Review says:

#### Validity of mb() after mv_desc_set_next_desc()

• Ordering the memory store (old_chain_tail->phy_next_desc) before
the MMIO register load (readl_relaxed(XOR_ACTIVATION) in mv_xor.c:181-188),

According to Documentation/memory-barriers.txt, read_relaxed() order with writeX for the same peripherial.

mv_desc_set_next_desc(old_chain_tail, sw_desc->async_tx.phys); use writel(),

mv_chan_is_busy() call readl_relaxed(XOR_ACTIVATION(chan)); readlX() must happen after previous writel().

Quoted 3 lines

is necessary to prevent the
CPU from seeing busy == 1 while the link store is still in the CPU's
store buffer, followed by the hardware completing old_chain_tail,

It is impossible. readl_relatex() guaratee get register value after previous write data reach finial taget place. Many place use the same method to guaratee previous write complete.

Quoted 3 lines

reading a stale 0-pointer, and
halting without the CPU restarting it.
• Since readl_relaxed() does not order against prior memory stores,

readl_relatex() don't order against prior memory stores, but order with previous writel()

Quoted 3 lines

and dma_wmb() / wmb() only order stores against stores, a mandatory full barrier (mb()) is the
appropriate barrier for ordering a normal/WC memory store before an MMIO read.

simples said, io read and write is order itself.

1. mv_desc_set_next_desc() writle(),
2. mv_chan_is_busy() read_relax(),

1 and 2 is order, all data if have dependence, it is also order.

a = readl()
b = a * 2. >> this must happen after readl().

Frank

Quoted 12 lines
Quoted 12 lines

Frank

Quoted 9 lines
Quoted 9 lines

Frank

Quoted 6 lines
drivers/dma/mv_xor.c +0 -1
              /* if the channel is not busy */
              if (!mv_chan_is_busy(mv_chan)) {
                      u32 current_desc = mv_chan_get_current_desc(mv_chan);
--

2.55.0