Re: [PATCH v5 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:

New issues:
- [High] The interrupt handler incorrectly marks the channel as idle when `active_list` is not empty, causing subsequent transfers to break the hardware descriptor chain and corrupt the DMA channel.

Pre-existing issues:
- [High] Missing validation of `sg_len` in DMA prep functions leads to a NULL pointer dereference or out-of-bounds access.
--

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

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

The patch adds a check for the descriptor completion bit before moving entries
from the active list to the done list for AXI MCDMA. This prevents descriptors
that have not been fully processed by the hardware from being falsely reported
as completed during early interrupt coalescing fires.

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

[Severity: High]
This isn't a bug introduced by this patch, but does the lack of sg_len
validation lead to out-of-bounds accesses here?

If a client driver passes sg_len == 0 to xilinx_mcdma_prep_slave_sg() (or
xilinx_dma_prep_slave_sg()), the for_each_sg() loop is entirely skipped,
leaving desc->segments empty. 

When this empty descriptor later reaches the newly added list_last_entry()
call, it yields a misaligned pointer into the desc structure, resulting in
an out-of-bounds read and memory corruption.

Additionally, back in the preparation functions, skipping the loop causes
immediate problems. In xilinx_mcdma_prep_slave_sg(), an empty segments
list causes an out-of-bounds dereference right away:

drivers/dma/xilinx/xilinx_dma.c:xilinx_mcdma_prep_slave_sg() {
    ...
    for_each_sg(sgl, sg, sg_len, i) { ... }

    segment = list_first_entry(&desc->segments,
                               struct xilinx_aximcdma_tx_segment, node);
    desc->async_tx.phys = segment->phys;
    
    if (chan->direction == DMA_MEM_TO_DEV) {
        segment->hw.control |= XILINX_MCDMA_BD_SOP;
    ...
}

> +			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 early break cause a hardware state corruption when combined with
the interrupt handler logic?

Because this patch intentionally leaves uncompleted descriptors in
chan->active_list by breaking early, xilinx_mcdma_irq_handler() will then
unconditionally set chan->idle to true and call start_transfer():

drivers/dma/xilinx/xilinx_dma.c: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);
    }
    ...
}

Because chan->idle is now true, start_transfer() takes pending descriptors
and writes their physical address to the Current Descriptor (CDESC) register.
Writing to CDESC while the hardware is still actively processing the
remainder of active_list violates the AXI MCDMA specification and can break
the descriptor chain.

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

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