Re: [PATCH] dmaengine: mv_xor: use writel() instead of relaxed variant

[Frank Li <Frank.li@oss.nxp.com>]

On Wed, Sep 16, 2026 at 11:24:05AM -0700, Rosen Penev wrote:
> 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 <rosenp@gmail.com>
> ---
>  drivers/dma/mv_xor.c | 10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/dma/mv_xor.c b/drivers/dma/mv_xor.c
> index c4e0dce3ca64..0de2b1ad5c30 100644
> --- a/drivers/dma/mv_xor.c
> +++ b/drivers/dma/mv_xor.c
> @@ -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?

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

>  		/* 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
>