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

[sashiko-bot@kernel.org]

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