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

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

On 6/21/2026 2:17 AM, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
> - [Critical] Removing the channel idle check unconditionally overwrites hardware registers in Direct (non-SG) mode, corrupting active transfers.
> - [Critical] Appending new transfers to a running Scatter-Gather chain breaks the hardware descriptor chain, as new descriptors are never physically linked in memory.
> - [High] Updating the interrupt coalesce threshold while the channel is active uses a partial descriptor count, disrupting interrupt generation.
> --
> 
> commit 59ccde00ae0d672019658b79ee8d2055ebb8fa7a
> Author: Suraj Gupta <suraj.gupta2@amd.com>
> 
> dmaengine: xilinx_dma: Enable transfer chaining for AXIDMA and MCDMA by removing idle restriction
> 
> This commit removes the idle check in xilinx_dma_start_transfer() and
> xilinx_mcdma_start_transfer() to allow queuing new transfers while the
> channel is busy. It also updates the CURDESC register only when the active
> list is empty in scatter-gather mode, relying on the hardware tail pointer
> extension mechanism to handle chaining.
> 
>> diff --git a/drivers/dma/xilinx/xilinx_dma.c b/drivers/dma/xilinx/xilinx_dma.c
>> index ca396b7097427..35b553ee3205c 100644
>> --- a/drivers/dma/xilinx/xilinx_dma.c
>> +++ b/drivers/dma/xilinx/xilinx_dma.c
>> @@ -1580,9 +1580,6 @@ static void xilinx_dma_start_transfer(struct xilinx_dma_chan *chan)
>>   		return;
>>   	}
>>   
>> -	if (!chan->idle)
>> -		return;
>> -
> 
> [Severity: Critical]
> Does removing this early return cause active transfers to be corrupted in
> Direct (non-SG) mode?
> 
> Without the idle check, xilinx_dma_start_transfer() can proceed while a
> transfer is running. In Direct DMA mode (has_sg is false), the code falls
> through and executes:
> 
>      xilinx_write(chan, XILINX_DMA_REG_SRCDSTADDR,
>                   xilinx_prep_dma_addr_t(hw->buf_addr));
> 
>      dma_ctrl_write(chan, XILINX_DMA_REG_BTT,
>                     hw->control & chan->xdev->max_buffer_len);
> 
> Could this unconditionally overwrite the active hardware registers if a
> transfer is already in progress?
> 
> Additionally, moving the newly queued descriptor into active_list appears to
> cause xilinx_dma_complete_descriptor() to prematurely complete newly appended
> descriptors, because the loop break condition is skipped for non-SG mode:
> 
>      if (!(seg->hw.status & XILINX_DMA_BD_COMP_MASK) && chan->has_sg)
>          break;
> 

Agreed, I'll restrict the idle removal to the has_sg path (and keep 
direct-mode transfers serialized as before) in next version.
> [Severity: Critical]
> Can appending new transfers to a running Scatter-Gather chain break the
> hardware descriptor chain?
> 
> When the idle check is removed, we update the TAILDESC register to append
> transfers on-the-fly. However, the Xilinx AXI DMA engine requires SG
> descriptors to be physically linked in memory via their next_desc pointers.
> 
> In the driver, descriptor linking occurs in append_desc_queue():
> 
>      if (list_empty(&chan->pending_list))
>          goto append;
>      ...
>      tail_desc = list_last_entry(&chan->pending_list, ...);
> 
> Since start_transfer() moves all queued descriptors to chan->active_list and
> leaves pending_list empty, a subsequent submission will find an empty
> pending_list.
> 
> Will this cause it to skip linking its next_desc pointer to the tail of the
> actively running descriptor chain in active_list?
> 
> When the DMA engine reaches the end of the original active chain, it might
> follow a stale next_desc pointer instead of the physically unlinked new
> descriptors.


I don't think this holds for the AXIDMA/MCDMA SG case -- the BD segments 
are kept permanently chained in coherent memory.

In xilinx_dma_alloc_chan_resources() every segment in the pool is wired 
into a fixed circular next_desc chain at allocation time:
       for (i = 0; i < XILINX_DMA_NUM_DESCS; i++) {
           chan->seg_v[i].hw.next_desc =
           lower_32_bits(chan->seg_p + sizeof(*chan->seg_v) *
               ((i + 1) % XILINX_DMA_NUM_DESCS));
           chan->seg_v[i].hw.next_desc_msb =
           upper_32_bits(chan->seg_p + sizeof(*chan->seg_v) *
               ((i + 1) % XILINX_DMA_NUM_DESCS));
           ...
       }
i.e. seg[i].next_desc -> seg[(i+1) % N], a closed ring (same for the 
MCDMA seg_mv pool). These links are never torn down: when a segment is 
recycled, xilinx_dma_clean_hw_desc() zeroes the BD but explicitly 
preserves next_desc/next_desc_msb:
       u32 next_desc = hw->next_desc;
       u32 next_desc_msb = hw->next_desc_msb;
       memset(hw, 0, sizeof(struct xilinx_axidma_desc_hw));
       hw->next_desc = next_desc;
       hw->next_desc_msb = next_desc_msb;

Consequently xilinx_dma_prep_slave_sg() never programs next_desc itself. 
It relies entirely on this pre-established ring for both intra- and 
inter-descriptor linkage. Segments are allocated FIFO from the head of 
free_seg_list and freed back to the tail in order, so the head of a
newly appended descriptor is the physical ring-successor of the previous
chain's tail. Extending TAILDESC therefore walks an unbroken in-memory
chain into the new BDs; the descriptors are physically linked.

> 
>>   	head_desc = list_first_entry(&chan->pending_list,
>>   				     struct xilinx_dma_tx_descriptor, node);
>>   	tail_desc = list_last_entry(&chan->pending_list,
>> @@ -1599,7 +1596,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))
> 
> [Severity: High]
> Does updating the interrupt coalesce threshold while the channel is active
> disrupt interrupt generation?
> 
> The driver updates the hardware interrupt coalesce threshold using
> chan->desc_pendingcount before writing to the DMACR register:
> 
>      if (chan->desc_pendingcount <= XILINX_DMA_COALESCE_MAX) {
>          reg &= ~XILINX_DMA_CR_COALESCE_MAX;
>          reg |= chan->desc_pendingcount << XILINX_DMA_CR_COALESCE_SHIFT;
>      }
> 
> With the removal of the idle check, this function now executes while
> descriptors are actively running. Since desc_pendingcount is reset to 0 at
> the end of each start_transfer() call, it now only represents the number
> of newly submitted descriptors in the latest pending_list batch.
> 
> Will overwriting the active coalesce threshold with this potentially much
> smaller partial count disrupt the DMA engine's interrupt generation and cause
> erratic coalescing behavior?
> 
>>   		xilinx_write(chan, XILINX_DMA_REG_CURDESC,
>>   			     head_desc->async_tx.phys);
>>   	reg  &= ~XILINX_DMA_CR_DELAY_MAX;
> 
> [ ... ]
> 
>> @@ -1660,9 +1657,6 @@ static void xilinx_mcdma_start_transfer(struct xilinx_dma_chan *chan)
>>   	if (chan->err)
>>   		return;
>>   
>> -	if (!chan->idle)
>> -		return;
>> -
> 


This doesn't disrupt IRQ generation. The axienet driver does exactly 
this for adaptive IRQ moderation (DIM). axienet_rx_dim_work() recomputes 
the coalesce count from the DIM profile and axienet_update_coalesce_rx() 
writes the COALESCE field into the (same) CR register while 
lp->rx_dma_started is true, i.e. with transfers in flight.

Same applies to below MCDMA related comments.

Regards,
Suraj

> [Severity: Critical]
> Does MCDMA suffer from the same unlinked descriptor chain vulnerability when
> TDESC is updated while active?
> 
> Like xilinx_dma_start_transfer(), appending new transfers to a running
> Scatter-Gather chain might break the hardware descriptor chain because the
> new descriptors are not physically linked in memory to the active list.
> 
>>   	if (list_empty(&chan->pending_list))
>>   		return;
>>   
>> @@ -1685,8 +1679,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))
> 
> [Severity: High]
> Does this also overwrite the MCDMA CR coalesce field with a partial count
> while the engine may be active?
> 
>      if (chan->desc_pendingcount <= XILINX_MCDMA_COALESCE_MAX) {
>          reg &= ~XILINX_MCDMA_COALESCE_MASK;
>          reg |= chan->desc_pendingcount << XILINX_MCDMA_COALESCE_SHIFT;
>      }
> 
> Similar to the AXIDMA case, using desc_pendingcount here while active might
> disrupt interrupt generation.
> 
>> +		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);
>