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

[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.

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

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

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