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

["Gupta, Suraj" <suraj.gupta2@amd.com>]

On 6/26/2026 3:18 PM, sashiko-bot@kernel.org wrote:
> 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.
> --

I replied for same comments in V2 here:

https://lore.kernel.org/all/0f00a643-8aa0-48c0-b6e8-d1956e623cc4@amd.com/

Regards,
Suraj

> 
> 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?
>