Re: [PATCH RESEND 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 3 potential issue(s) to consider:

Pre-existing issues:
- [Critical] The patch's intended fix is completely ineffective for MCDMA and CDMA channels due to a pre-existing bug where descriptors are unconditionally completed without checking hardware status.
- [High] Cyclic DMA completion callbacks are permanently starved and will never be executed because the cyclic descriptor is moved out of `done_list` before the cleanup tasklet runs.
- [High] The MCDMA IRQ handler fails to process descriptor completions when a delay interrupt occurs, leading to indefinite stalls.
--


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

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

Fix a race condition in AXIDMA and MCDMA irq handlers where the channel
could be incorrectly marked as idle and attempt spurious transfers when
descriptors are still being processed.

> 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)) {
> +			chan->idle = true;
> +			chan->start_transfer(chan);
> +		}

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

Looking at drivers/dma/xilinx/xilinx_dma.c in xilinx_dma_complete_descriptor(),
hardware completion status is only validated for AXIDMA:

xilinx_dma_complete_descriptor() {
	...
	if (chan->xdev->dma_config->dmatype == XDMA_TYPE_AXIDMA) {
		struct xilinx_axidma_tx_segment *seg;

		seg = list_last_entry(&desc->segments,
				      struct xilinx_axidma_tx_segment, node);
		if (!(seg->hw.status & XILINX_DMA_BD_COMP_MASK) && chan->has_sg)
			break;
	}
	...
}

For MCDMA and CDMA channels, descriptors are unconditionally unlinked and
moved to done_list. This means active_list is always emptied by
xilinx_dma_complete_descriptor(), causing list_empty(&chan->active_list)
to always evaluate to true.

Doesn't this make the patch's fix ineffective for MCDMA, while also risking
data corruption since clients might reuse memory before the hardware actually
completes the transfer?


[Severity: High]
This is a pre-existing issue, but does the MCDMA IRQ handler properly process
descriptor completions when a delay interrupt occurs?

Unlike the AXIDMA handler which checks both FRM_CNT_IRQ and DLY_CNT_IRQ,
if a delay interrupt fires for MCDMA in drivers/dma/xilinx/xilinx_dma.c,
the handler only logs a message:

xilinx_mcdma_irq_handler() {
	...
	if (status & XILINX_MCDMA_IRQ_DELAY_MASK) {
		/*
		 * Device takes too long to do the transfer when user requires
		 * responsiveness.
		 */
		dev_dbg(chan->dev, "Inter-packet latency too long\n");
	}
	...
}

If a delay interrupt occurs without an accompanying IOC interrupt, which
happens when the transferred count hasn't reached the coalesce threshold but
the timer expires, descriptor completions are never processed.

Could this lead to indefinite stalls for the DMA client since the channel
becomes unresponsive?

>  		spin_unlock(&chan->lock);
>  	}
>  
> @@ -1950,8 +1952,10 @@ static irqreturn_t xilinx_dma_irq_handler(int irq, void *data)
>  		      XILINX_DMA_DMASR_DLY_CNT_IRQ)) {
>  		spin_lock(&chan->lock);
>  		xilinx_dma_complete_descriptor(chan);
> -		chan->idle = true;
> -		chan->start_transfer(chan);
> +		if (list_empty(&chan->active_list)) {
> +			chan->idle = true;
> +			chan->start_transfer(chan);
> +		}

[Severity: High]
This is a pre-existing issue, but will cyclic DMA completion callbacks ever
be executed since start_transfer() is called here?

When a cyclic transfer completes, xilinx_dma_complete_descriptor() moves
the cyclic descriptor from active_list to done_list. When start_transfer()
is invoked immediately after, it preemptively moves the descriptor back into
active_list because pending_list is empty in cyclic mode:

drivers/dma/xilinx/xilinx_dma.c:xilinx_dma_start_transfer() {
	...
	list_splice_tail_init(&chan->done_list, &chan->active_list);
	...
}

After the IRQ handler returns, the cleanup tasklet executes to iterate over
done_list and trigger the callback:

drivers/dma/xilinx/xilinx_dma.c:xilinx_dma_chan_desc_cleanup() {
	...
	list_for_each_entry_safe(desc, next, &chan->done_list, node) {
		struct dmaengine_result result;

		if (desc->cyclic) {
			xilinx_dma_chan_handle_cyclic(chan, desc, &flags);
	...
}

Because done_list was already completely emptied by start_transfer(), won't
this loop be bypassed entirely, causing the cyclic notification mechanism
to fail and clients to stall?

>  		spin_unlock(&chan->lock);
>  	}

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