Re: [PATCH v2] dmaengine: xilinx_dma: Fix hardware buffer descriptor chain after cyclic DMA

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

Pre-existing issues:
- [Critical] Hardware executes physically non-contiguous multi-segment transfers out of order, causing silent data corruption.
- [High] Concurrent lockless modifications to chan->free_seg_list directly corrupt list pointers and trigger kernel panics.
- [High] Multiple cyclic descriptors are simultaneously queued, violating hardware limits and breaking cyclic execution.
--

--- Patch [1]: [PATCH v2] dmaengine: xilinx_dma: Fix hardware buffer descriptor chain after cyclic DMA ---
commit 9ccc605c4c1fab881ccb3335638f86979c8c66c7
Author: Alex Bereza <alex@bereza.email>

dmaengine: xilinx_dma: Fix hardware buffer descriptor chain after cyclic DMA

Using the DMA in cyclic mode modifies the hardware buffer descriptor chain so
that the last descriptor used by the cyclic transfer points back to the first
descriptor. However, it never restores the original descriptor ring. This
breaks using non-cyclic mode after cyclic mode.

Link: https://patch.msgid.link/20260817-fix-hw-buf-desc-after-cyclic-mode-v1-1-1fe47e701d6c@bereza.email

> diff --git a/drivers/dma/xilinx/xilinx_dma.c b/drivers/dma/xilinx/xilinx_dma.c
> index bef2b031dba19..650e078fcdb74 100644
> --- a/drivers/dma/xilinx/xilinx_dma.c
> +++ b/drivers/dma/xilinx/xilinx_dma.c

[ ... ]

> @@ -786,7 +796,7 @@ static void xilinx_mcdma_clean_hw_desc(struct xilinx_aximcdma_desc_hw *hw)
>  static void xilinx_dma_free_tx_segment(struct xilinx_dma_chan *chan,
>  				struct xilinx_axidma_tx_segment *segment)
>  {
> -	xilinx_dma_clean_hw_desc(&segment->hw);
> +	xilinx_dma_clean_hw_desc(chan, segment);
>  
>  	list_add_tail(&segment->node, &chan->free_seg_list);
>  }

[Severity: High]
This isn't a bug introduced by this patch, but does this list manipulation
require locking to avoid concurrent modifications?

Looking at xilinx_dma_tx_submit(), it can call into this function on an error
path without acquiring chan->lock:

xilinx_dma_tx_submit() {
	...
	if (chan->cyclic) {
		xilinx_dma_free_tx_descriptor(chan, desc);
		return -EBUSY;
	}
	...
}

xilinx_dma_prep_slave_sg() also has a similar unlocked error path:

xilinx_dma_prep_slave_sg() {
	...
error:
	xilinx_dma_free_tx_descriptor(chan, desc);
	return NULL;
}

If another thread simultaneously allocates descriptors via
xilinx_axidma_alloc_tx_segment() (which acquires chan->lock and calls
list_del()), could this corrupt the free_seg_list pointers and lead to a
kernel panic?

[Severity: Critical]
This is a pre-existing issue, but can the hardware execute descriptors in the
wrong order here and cause silent data corruption?

xilinx_dma_prep_slave_sg() dynamically allocates segments from the
free_seg_list without explicitly updating their hw->next_desc links:

xilinx_dma_prep_slave_sg() {
	...
	/* Get a free segment */
	segment = xilinx_axidma_alloc_tx_segment(chan);
	...
}

If the free_seg_list becomes fragmented from aborted transfers freeing segments
out of order, or concurrent allocations, the allocated segments for a transfer
may not be physically contiguous.

Since this relies entirely on the static next_desc linkages programmed during
allocation, will the hardware blindly follow the static next_desc into a
physically adjacent element that might belong to another transaction?

[Severity: High]
This is a pre-existing issue, but can multiple cyclic descriptors be queued
simultaneously and break the cyclic descriptor ring?

In xilinx_dma_tx_submit(), the check for chan->cyclic is performed before
acquiring chan->lock:

xilinx_dma_tx_submit() {
	...
	if (chan->cyclic) {
		xilinx_dma_free_tx_descriptor(chan, desc);
		return -EBUSY;
	}
	...
}

If two cyclic descriptors are submitted simultaneously, they could both bypass
this check and then serialize on chan->lock.

Once serialized, append_desc_queue() alters the next_desc of the first
descriptor's tail segment:

append_desc_queue() {
	...
	axidma_tail_segment->hw.next_desc = (u32)desc->async_tx.phys;
	...
}

Could this overwrite the tail descriptor of the first cyclic transfer to point
to the second descriptor, completely breaking the cyclic ring?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260818-fix-hw-buf-desc-after-cyclic-mode-v2-1-530ff44c6a81@bereza.email?part=1