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

New issues:
- [High] Unconditionally setting `chan->idle = true` after an early loop break causes the driver to incorrectly assume the DMA hardware has stopped, leading to channel corruption.
- [High] Missing read memory barrier (`dma_rmb()`) after polling the DMA descriptor completion bit from coherent memory.

Pre-existing issues:
- [High] MCDMA delay interrupt does not complete descriptors, causing descriptors stranded on `active_list` to hang indefinitely.
--

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

This commit modifies the MCDMA completion logic to check the descriptor
completion bit before moving entries from the active list to the done list.
This addresses an issue where interrupt coalescing could cause descriptors
to be reported as completed prematurely.

> 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))
> +				break;

[Severity: High]
Does this code need a read memory barrier (dma_rmb()) here?

Since the status is read from coherent memory, weakly-ordered CPU
architectures might speculatively reorder the reads of the segment payloads
and status fields to occur before the completion bit check resolves. This
could lead to reading stale, pre-transfer data from the descriptor or the
actual DMA buffer, causing incorrect residue reporting or corrupted network
packets.

[Severity: High]
Also, when this breaks early, unfinished descriptors are left in active_list.
Will this cause a DMA channel hang? Looking at xilinx_mcdma_irq_handler():

xilinx_mcdma_irq_handler() {
    ...
    if (status & XILINX_MCDMA_IRQ_IOC_MASK) {
        spin_lock(&chan->lock);
        xilinx_dma_complete_descriptor(chan);
        chan->idle = true;
        chan->start_transfer(chan);
        spin_unlock(&chan->lock);
    }
    ...
}

The interrupt handler unconditionally sets chan->idle = true and calls
chan->start_transfer(), even if active descriptors are still running due to
the early break.

Since chan->idle is true, start_transfer() attempts to program the CDESC
and TDESC registers with new, unlinked descriptors from pending_list while
the hardware is actively processing active_list, which seems invalid and
likely to cause a complete hardware desynchronization.

[Severity: High]
This is a pre-existing issue, but this patch highlights a fatal flaw for small
transfers. Unlike the standard AXI DMA handler, xilinx_mcdma_irq_handler() does
not invoke xilinx_dma_complete_descriptor() on a delay interrupt:

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 the number of submitted descriptors is below the interrupt coalescing
threshold, the hardware triggers a delay interrupt instead of an IOC interrupt.
Since the delay interrupt doesn't complete descriptors, trailing segments will
remain stranded in active_list indefinitely because they are never processed,
causing a channel hang.

>  		}
>  		if (chan->has_sg && chan->xdev->dma_config->dmatype !=
>  		    XDMA_TYPE_VDMA)

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