Re: [PATCH v3 1/3] dmaengine: xilinx_dma: Fix channel idle state management in AXIDMA and MCDMA interrupt handlers

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] The patch claims to fix a race condition for MCDMA channels by conditionally marking the channel as idle only when `active_list` is empty. However, this fix is entirely ineffective because `xilinx_dma_complete_descriptor()` unconditionally empties `active_list` for MCDMA.
--

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

dmaengine: xilinx_dma: Fix channel idle state management in AXIDMA and MCDMA interrupt handlers

This patch fixes a race condition in AXIDMA and MCDMA irq handlers by only
marking the channel idle and starting a new transfer when the active list is
empty.

> diff --git a/drivers/dma/xilinx/xilinx_dma.c b/drivers/dma/xilinx/xilinx_dma.c
> index 404235c173538..ca396b7097427 100644
> --- a/drivers/dma/xilinx/xilinx_dma.c
> +++ b/drivers/dma/xilinx/xilinx_dma.c
> @@ -1893,8 +1893,10 @@ static irqreturn_t xilinx_mcdma_irq_handler(int irq, void *data)
>  	if (status & XILINX_MCDMA_IRQ_IOC_MASK) {
>  		spin_lock(&chan->lock);
>  		xilinx_dma_complete_descriptor(chan);
> -		chan->idle = true;
> -		chan->start_transfer(chan);
> +		if (list_empty(&chan->active_list)) {

[Severity: High]
This is a pre-existing issue, but does this check actually prevent the race
condition for MCDMA channels?

Looking at xilinx_dma_complete_descriptor(), the hardware completion check
is restricted to AXIDMA:

drivers/dma/xilinx/xilinx_dma.c:xilinx_dma_complete_descriptor() {
	list_for_each_entry_safe(desc, next, &chan->active_list, node) {
		if (chan->xdev->dma_config->dmatype == XDMA_TYPE_AXIDMA) {
			...
			if (!(seg->hw.status & XILINX_DMA_BD_COMP_MASK) && chan->has_sg)
				break;
		}
		...
		list_del(&desc->node);
		...
	}
}

Because MCDMA descriptors are unconditionally unlinked from active_list
regardless of their actual hardware completion status, active_list will
always be empty after xilinx_dma_complete_descriptor() returns. 

Will this cause the new idle check to always evaluate to true for MCDMA,
leaving the race condition unaddressed for those channels?

> +			chan->idle = true;
> +			chan->start_transfer(chan);
> +		}
>  		spin_unlock(&chan->lock);
>  	}

[ ... ]

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