Re: [PATCH RESEND 3/3] dmaengine: xilinx_dma: Optimize control register write and channel start logic for AXIDMA and MCDMA in corresponding start_transfer()

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Concurrent read-modify-write on shared global MCDMA registers due to lack of a controller-level lock.
--

commit ecde894a65e016199009c2bd5c7fcf89cdab08cf
Author: Suraj Gupta <suraj.gupta2@amd.com>

dmaengine: xilinx_dma: Optimize control register write and channel start logic for AXIDMA and MCDMA in corresponding start_transfer()

This commit optimizes AXI DMA control register programming by combining
coalesce count and delay configuration into a single register write to
reduce bus overhead. It also avoids redundant channel starts by only calling
xilinx_dma_start() when the channel is actually idle.

> diff --git a/drivers/dma/xilinx/xilinx_dma.c b/drivers/dma/xilinx/xilinx_dma.c
> index 35b553ee3205c..aa3dee0dc2fcb 100644
> --- a/drivers/dma/xilinx/xilinx_dma.c
> +++ b/drivers/dma/xilinx/xilinx_dma.c

[ ... ]

> @@ -1693,7 +1693,8 @@ static void xilinx_mcdma_start_transfer(struct xilinx_dma_chan *chan)
>  	reg |= XILINX_MCDMA_CR_RUNSTOP_MASK;
>  	dma_ctrl_write(chan, XILINX_MCDMA_CHAN_CR_OFFSET(chan->tdest), reg);
>  
> -	xilinx_dma_start(chan);
> +	if (chan->idle)
> +		xilinx_dma_start(chan);

[Severity: High]
This is a pre-existing issue, but does the lack of a controller-level lock
cause concurrent read-modify-write operations on shared global MCDMA
registers to race?

In xilinx_mcdma_start_transfer(), just before this diff context, there
is an unprotected read-modify-write on the shared XILINX_MCDMA_CHEN_OFFSET
register:

	reg = dma_ctrl_read(chan, XILINX_MCDMA_CHEN_OFFSET);
	reg |= BIT(chan->tdest);
	dma_ctrl_write(chan, XILINX_MCDMA_CHEN_OFFSET, reg);

Similarly, when xilinx_dma_start() is called here, it performs an
unprotected read-modify-write on the shared XILINX_DMA_REG_DMACR register:

	dma_ctrl_set(chan, XILINX_DMA_REG_DMACR, XILINX_DMA_DMACR_RUNSTOP);

These operations appear to be protected only by the per-channel spinlock
(chan->lock). If two channels on the same controller execute their start
transfer logic concurrently on different CPUs, can the read-modify-write
operations race and lead to a channel's enable bit being silently
overwritten and lost?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260620203417.4000360-1-suraj.gupta2@amd.com?part=3