[PATCH] dmaengine: mv_xor: add missing dma_wmb() barriers before descriptor chain start

[PATCH] dmaengine: mv_xor: add missing dma_wmb() barriers before descriptor chain start

[Rosen Penev]

The descriptor data written to coherent memory must be visible to the
DMA engine before the HW registers that trigger processing are written.

Add a dma_wmb() before mv_chan_set_next_descriptor() in
mv_chan_start_new_chain() to order descriptor fields before the
descriptor-pointer write.

Add a dma_wmb() after mv_desc_set_next_desc() in
mv_chan_handle_chain_append() to order the chain link write before
the subsequent check of the current-descriptor register. Without the
barrier the hardware may observe stale descriptor data.

Fixes: ff7b04796d98 ("DMA engine driver for Marvell XOR engine")
Assisted-by: opencode:big-pickle
Signed-off-by: Rosen Penev <rosenp@gmail.com>
---
 drivers/dma/mv_xor.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/dma/mv_xor.c b/drivers/dma/mv_xor.c
index 24f22db5dfa1..6db7dafac9c8 100644
--- a/drivers/dma/mv_xor.c
+++ b/drivers/dma/mv_xor.c
@@ -181,6 +181,7 @@ static void mv_chan_start_new_chain(struct mv_xor_chan *mv_chan,
 	dev_dbg(mv_chan_to_devp(mv_chan), "%s %d: sw_desc %p\n",
 		__func__, __LINE__, sw_desc);
 
+	dma_wmb();
 	/* set the hardware chain */
 	mv_chan_set_next_descriptor(mv_chan, sw_desc->async_tx.phys);
 
@@ -409,6 +410,7 @@ 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);
+		dma_wmb();
 
 		/* if the channel is not busy */
 		if (!mv_chan_is_busy(mv_chan)) {
-- 
2.55.0

Re: [PATCH] dmaengine: mv_xor: add missing dma_wmb() barriers before descriptor chain start

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

New issues:
- [High] dma_wmb() is insufficient to enforce Store-Load ordering between the memory write and MMIO read in mv_xor_tx_submit().
- [Medium] dma_wmb() is incorrectly used to order a coherent memory write against a subsequent MMIO write in mv_chan_start_new_chain().

Pre-existing issues:
- [High] Missing memory barrier before linking a new descriptor to an active hardware chain in mv_xor_tx_submit().
--

commit a54d735383cc294f5f7387dfea7d5abca9cdec6f
Author: Rosen Penev <rosenp@gmail.com>

dmaengine: mv_xor: add missing dma_wmb() barriers before descriptor chain start

This commit adds dma_wmb() barriers in the DMA submission and chain starting
paths to ensure descriptor data written to coherent memory is visible to the
DMA engine before the hardware registers that trigger processing are written.

> diff --git a/drivers/dma/mv_xor.c b/drivers/dma/mv_xor.c
> index 25ed61f1b0899..8e7c7a6c5c5d0 100644
> --- a/drivers/dma/mv_xor.c
> +++ b/drivers/dma/mv_xor.c
> @@ -181,6 +181,7 @@ static void mv_chan_start_new_chain(struct mv_xor_chan *mv_chan,
>  	dev_dbg(mv_chan_to_devp(mv_chan), "%s %d: sw_desc %p\n",
>  		__func__, __LINE__, sw_desc);
>  
> +	dma_wmb();
>  	/* set the hardware chain */
>  	mv_chan_set_next_descriptor(mv_chan, sw_desc->async_tx.phys);

[Severity: Medium]
Does this incorrectly use the memory-to-memory barrier API here?

According to Documentation/memory-barriers.txt, dma_*() barriers do not
provide ordering guarantees for accesses to MMIO regions. Here, we are
trying to order a coherent memory write against a subsequent relaxed MMIO
write inside mv_chan_set_next_descriptor().

Should this use wmb() instead to properly enforce memory-to-MMIO ordering?

[ ... ]

> @@ -409,6 +410,7 @@ mv_xor_tx_submit(struct dma_async_tx_descriptor *tx)
>  
>  		/* fix up the hardware chain */

[Severity: High]
This isn't a bug introduced by this patch, but does the descriptor link
pointer update need a preceding memory barrier?

Before this function is called, the descriptor is initialized with
operational parameters. The spin_lock_bh() acquired at the start of
mv_xor_tx_submit() only acts as an acquire barrier, which doesn't prevent
preceding stores from being reordered after the link write.

Should there be a dma_wmb() placed immediately before
mv_desc_set_next_desc() to guarantee that the hardware doesn't follow the
new next-descriptor pointer and read an uninitialized descriptor?

>  		mv_desc_set_next_desc(old_chain_tail, sw_desc->async_tx.phys);
> +		dma_wmb();
>  
>  		/* if the channel is not busy */
>  		if (!mv_chan_is_busy(mv_chan)) {

[Severity: High]
Is dma_wmb() sufficient to enforce store-load ordering here?

The patch adds dma_wmb() after mv_desc_set_next_desc() to order the memory
store before the MMIO load in mv_chan_is_busy(), which uses readl_relaxed().

However, dma_wmb() strictly guarantees store-store ordering and does not
prevent the CPU from executing a subsequent load before a preceding store
is globally visible.

If the CPU reorders its MMIO load of the status register to happen before
its memory write is globally visible, it could incorrectly read 'busy',
causing the CPU to skip kicking the hardware. If the hardware simultaneously
fetches the next descriptor right before the CPU writes the new link, it
will see a null pointer and stop, leaving the channel stalled indefinitely.

Does this sequence require a full memory barrier like mb() instead?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260909234646.102344-1-rosenp@gmail.com?part=1

Re: [PATCH] dmaengine: mv_xor: add missing dma_wmb() barriers before descriptor chain start

[Frank Li]

On Wed, Sep 09, 2026 at 04:46:46PM -0700, Rosen Penev wrote:
> The descriptor data written to coherent memory must be visible to the
> DMA engine before the HW registers that trigger processing are written.
>
> Add a dma_wmb() before mv_chan_set_next_descriptor() in
> mv_chan_start_new_chain() to order descriptor fields before the
> descriptor-pointer write.
>
> Add a dma_wmb() after mv_desc_set_next_desc() in
> mv_chan_handle_chain_append() to order the chain link write before
> the subsequent check of the current-descriptor register. Without the
> barrier the hardware may observe stale descriptor data.
>
> Fixes: ff7b04796d98 ("DMA engine driver for Marvell XOR engine")
> Assisted-by: opencode:big-pickle
> Signed-off-by: Rosen Penev <rosenp@gmail.com>
> ---
>  drivers/dma/mv_xor.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/drivers/dma/mv_xor.c b/drivers/dma/mv_xor.c
> index 24f22db5dfa1..6db7dafac9c8 100644
> --- a/drivers/dma/mv_xor.c
> +++ b/drivers/dma/mv_xor.c
> @@ -181,6 +181,7 @@ static void mv_chan_start_new_chain(struct mv_xor_chan *mv_chan,
>  	dev_dbg(mv_chan_to_devp(mv_chan), "%s %d: sw_desc %p\n",
>  		__func__, __LINE__, sw_desc);
>
> +	dma_wmb();
>  	/* set the hardware chain */
>  	mv_chan_set_next_descriptor(mv_chan, sw_desc->async_tx.phys);
>
> @@ -409,6 +410,7 @@ 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);
> +		dma_wmb();

mv_chan_set_next_descriptor() implement is wrong, it should use writel()
instead of writel_relax().

Frank

>
>  		/* if the channel is not busy */
>  		if (!mv_chan_is_busy(mv_chan)) {
> --
> 2.55.0
>