Re: [PATCH V3 2/4] dmaengine: xilinx_dma: Move descriptors to done list based on completion bit

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Software state mismatch leading to hardware corruption and channel hang in AXI MCDMA.
- [Medium] Missing read memory barrier (`dma_rmb()`) after checking the DMA completion flag in coherent memory.
--

--- Patch [2]: [PATCH V3 2/4] dmaengine: xilinx_dma: Move descriptors to done list based on completion bit ---
commit d59255ad1a6d4a4beb643022da1f9820716a0752
Author: Srinivas Neeli <srinivas.neeli@amd.com>

dmaengine: xilinx_dma: Move descriptors to done list based on completion bit

In AXI MCDMA scatter-gather mode, xilinx_dma_complete_descriptor() previously
moved all active descriptors to the done list unconditionally. This commit adds
a check for the descriptor completion bit before moving entries, handling
scenarios where interrupt coalescing causes interrupts before all queued
descriptors are processed.

> diff --git a/drivers/dma/xilinx/xilinx_dma.c b/drivers/dma/xilinx/xilinx_dma.c
> index 965fb69a2a3ad..9b8b6f1acd4ea 100644
> --- a/drivers/dma/xilinx/xilinx_dma.c
> +++ b/drivers/dma/xilinx/xilinx_dma.c
> @@ -1773,6 +1773,17 @@ static void xilinx_dma_complete_descriptor(struct xilinx_dma_chan *chan)
>  					      struct xilinx_axidma_tx_segment, node);
>  			if (!(seg->hw.status & XILINX_DMA_BD_COMP_MASK) && chan->has_sg)
>  				break;
> +		} else if (chan->xdev->dma_config->dmatype == XDMA_TYPE_AXIMCDMA) {
> +			struct xilinx_aximcdma_tx_segment *seg;
> +			u32 status;
> +
> +			seg = list_last_entry(&desc->segments,
> +					      struct xilinx_aximcdma_tx_segment,
> +					      node);
> +			status = (chan->direction == DMA_DEV_TO_MEM) ?
> +				seg->hw.s2mm_status : seg->hw.mm2s_status;
> +			if (!(status & XILINX_DMA_BD_COMP_MASK))

[Severity: Medium]
Is a read memory barrier needed after checking this completion status?

The CPU polls the s2mm_status or mm2s_status field in coherent memory to
check the completion bit. Without a dma_rmb() here, could weakly ordered
architectures speculatively execute subsequent descriptor reads out of
order?

This might result in stale reads of the descriptor fields later on
(for instance, during residue calculation) before the hardware has
actually confirmed completion.

> +				break;

[Severity: High]
Does breaking out early here create a state mismatch with the IRQ handler?

If this check breaks out leaving descriptors in the active list, the
channel is technically still running. However, looking at the call chain,
xilinx_mcdma_irq_handler() immediately forces the channel idle and
starts a new transfer:

xilinx_mcdma_irq_handler() {
    ...
    xilinx_dma_complete_descriptor(chan);
    chan->idle = true;
    chan->start_transfer(chan);
    ...
}

This appears to bypass the idle safety check in
xilinx_mcdma_start_transfer() and overwrite the CDESC hardware register
while the DMA engine is actively processing the remaining active list.
Could this software-splice new descriptors without establishing hardware
links, leading to a permanent channel hang?

> +		}
>  		if (chan->has_sg && chan->xdev->dma_config->dmatype !=
>  		    XDMA_TYPE_VDMA)
[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260708100652.603074-1-srinivas.neeli@amd.com?part=2