Re: [PATCH v3 2/3] dmaengine: xilinx_dma: Enable transfer chaining for AXIDMA and MCDMA by removing idle restriction

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Critical] Bypassing the idle check allows new descriptors to be appended to a running transfer, but the driver fails to link the active transfer's hardware `next_desc` pointer to the newly appended descriptors, leading to wild DMA fetches.
- [High] Overwriting the DMA Coalesce Threshold register while a channel is running resets the hardware coalescing counter, causing the channel to stall indefinitely.
--

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

dmaengine: xilinx_dma: Enable transfer chaining for AXIDMA and MCDMA by removing idle restriction

This commit relaxes the idle checks in xilinx_dma_start_transfer() and
xilinx_mcdma_start_transfer() to allow scatter-gather transfers to be chained
onto an in-flight transfer. It relies on the hardware tail pointer extension
mechanism to handle chaining automatically.

> diff --git a/drivers/dma/xilinx/xilinx_dma.c b/drivers/dma/xilinx/xilinx_dma.c
> index ca396b7097427..6e7b183cb4994 100644
> --- a/drivers/dma/xilinx/xilinx_dma.c
> +++ b/drivers/dma/xilinx/xilinx_dma.c
> @@ -1580,7 +1580,14 @@ static void xilinx_dma_start_transfer(struct xilinx_dma_chan *chan)
>  		return;
>  	}
>  
> -	if (!chan->idle)
> +	/*
> +	 * Direct (non-SG) mode has no descriptor queue: writing the BTT
> +	 * register launches a transfer immediately, so a new transfer must
> +	 * not be programmed while one is in flight. Keep such transfers
> +	 * serialized. SG mode supports chaining onto a running transfer via
> +	 * tail-pointer extension, so it is allowed to proceed when busy.
> +	 */
> +	if (!chan->has_sg && !chan->idle)
>  		return;

[Severity: Critical]
Does bypassing this idle check for SG mode result in wild DMA fetches when a new
transfer is queued on a busy channel?

If a new transfer is queued while the channel is busy, the pending_list will
be empty when processed by append_desc_queue():

drivers/dma/xilinx/xilinx_dma.c:append_desc_queue() {
    ...
    if (list_empty(&chan->pending_list))
        goto append;
    ...
}

Because of this, append_desc_queue() explicitly skips hardware chaining entirely
and fails to link the active transfer's tail descriptor to the newly submitted
descriptor.

Since out-of-order frees can scramble the ring at runtime, won't the hardware
finish the active transfer and follow a stale next_desc pointer to an unrelated,
uninitialized, or freed descriptor segment?

[ ... ]
> @@ -1599,7 +1606,7 @@ static void xilinx_dma_start_transfer(struct xilinx_dma_chan *chan)
>  		dma_ctrl_write(chan, XILINX_DMA_REG_DMACR, reg);
>  	}
>  
> -	if (chan->has_sg)
> +	if (chan->has_sg && list_empty(&chan->active_list))
>  		xilinx_write(chan, XILINX_DMA_REG_CURDESC,
>  			     head_desc->async_tx.phys);
>  	reg  &= ~XILINX_DMA_CR_DELAY_MAX;

[Severity: High]
Does updating the Coalesce Threshold register while a channel is running stall
the channel indefinitely?

Just before this CURDESC check in xilinx_dma_start_transfer(), the threshold
is updated:

    if (chan->desc_pendingcount <= XILINX_DMA_COALESCE_MAX) {
        reg &= ~XILINX_DMA_CR_COALESCE_MAX;
        reg |= chan->desc_pendingcount <<
                  XILINX_DMA_CR_COALESCE_SHIFT;
    }
    ...
    dma_ctrl_write(chan, XILINX_DMA_REG_DMACR, reg);
    
According to the Xilinx AXI DMA specification (PG021), writing this threshold
automatically resets the hardware coalescing counter to zero.

If there are already active transfers running, writing the new threshold will
reset the counter. As a result, the total number of remaining transfers
(active + pending) will no longer align with the new threshold multiple. The
hardware will finish the transfers and stop with a counter value strictly below
the threshold, never triggering the completion interrupt.

[ ... ]
> @@ -1685,8 +1689,9 @@ static void xilinx_mcdma_start_transfer(struct xilinx_dma_chan *chan)
>  	dma_ctrl_write(chan, XILINX_MCDMA_CHAN_CR_OFFSET(chan->tdest), reg);
>  
>  	/* Program current descriptor */
> -	xilinx_write(chan, XILINX_MCDMA_CHAN_CDESC_OFFSET(chan->tdest),
> -		     head_desc->async_tx.phys);
> +	if (chan->has_sg && list_empty(&chan->active_list))
> +		xilinx_write(chan, XILINX_MCDMA_CHAN_CDESC_OFFSET(chan->tdest),
> +			     head_desc->async_tx.phys);
>  
>  	/* Program channel enable register */
>  	reg = dma_ctrl_read(chan, XILINX_MCDMA_CHEN_OFFSET);

[Severity: High]
This appears to introduce the same coalescing regression as seen in
xilinx_dma_start_transfer().

Earlier in xilinx_mcdma_start_transfer(), the CR register is overwritten
with the new pending count:

    if (chan->desc_pendingcount <= XILINX_MCDMA_COALESCE_MAX) {
        reg &= ~XILINX_MCDMA_COALESCE_MASK;
        reg |= chan->desc_pendingcount <<
            XILINX_MCDMA_COALESCE_SHIFT;
    }
    ...
    dma_ctrl_write(chan, XILINX_MCDMA_CHAN_CR_OFFSET(chan->tdest), reg);
    
Won't this similarly reset the MCDMA hardware coalescing counter while the
channel is active, causing it to stall indefinitely?

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