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

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

[Alex Bereza]

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

  xilinx-vdma 86000000.dma: Channel 00000000354d5c8d has errors 100, cdr 6de40000 tdr 6de40400

The only way to get out of this error state is to rebuild the hardware
buffer descriptor ring by releasing and re-acquiring the channel.

Fix using non-cyclic mode after cyclic mode by always restoring the
original buffer descriptor ring in the same manner as it is set up by
xilinx_dma_alloc_chan_resources().

Fixes: 23059408b6a3 ("dmaengine: xilinx_dma: Fix race condition in the driver for multiple descriptor scenario")
Signed-off-by: Alex Bereza <alex@bereza.email>
---
Changes in v2:
- Use pointer arithmetic instead of potential 64 bit division in
  xilinx_dma_clean_hw_desc() as suggested by Frank Li
  <Frank.Li@kernel.org> and sashiko-bot - Thanks!
- Link to v1: https://patch.msgid.link/20260817-fix-hw-buf-desc-after-cyclic-mode-v1-1-1fe47e701d6c@bereza.email

To: Vinod Koul <vkoul@kernel.org>
To: Frank Li <Frank.Li@kernel.org>
To: Michal Simek <michal.simek@amd.com>
To: Kedareswara rao Appana <appana.durga.rao@xilinx.com>
Cc: dmaengine@vger.kernel.org
Cc: linux-arm-kernel@lists.infradead.org
Cc: linux-kernel@vger.kernel.org
---
 drivers/dma/xilinx/xilinx_dma.c | 24 +++++++++++++++++-------
 1 file changed, 17 insertions(+), 7 deletions(-)

diff --git a/drivers/dma/xilinx/xilinx_dma.c b/drivers/dma/xilinx/xilinx_dma.c
index bef2b031dba1..650e078fcdb7 100644
--- a/drivers/dma/xilinx/xilinx_dma.c
+++ b/drivers/dma/xilinx/xilinx_dma.c
@@ -756,15 +756,25 @@ xilinx_aximcdma_alloc_tx_segment(struct xilinx_dma_chan *chan)
 	return segment;
 }
 
-static void xilinx_dma_clean_hw_desc(struct xilinx_axidma_desc_hw *hw)
+static void xilinx_dma_clean_hw_desc(struct xilinx_dma_chan *chan,
+				     struct xilinx_axidma_tx_segment *segment)
 {
-	u32 next_desc = hw->next_desc;
-	u32 next_desc_msb = hw->next_desc_msb;
+	dma_addr_t next;
+	u32 i;
 
-	memset(hw, 0, sizeof(struct xilinx_axidma_desc_hw));
+	/*
+	 * Restore the buffer descriptor's next descriptor pointer to the value
+	 * set up in xilinx_dma_alloc_chan_resources(). Otherwise using the DMA
+	 * in cyclic mode leaves the next descriptor pointer altered and
+	 * prevents subsequent non-cyclic transfers.
+	 */
+	i = segment - chan->seg_v;
+	next = chan->seg_p +
+	       sizeof(*chan->seg_v) * ((i + 1) % XILINX_DMA_NUM_DESCS);
 
-	hw->next_desc = next_desc;
-	hw->next_desc_msb = next_desc_msb;
+	memset(&segment->hw, 0, sizeof(segment->hw));
+	segment->hw.next_desc = lower_32_bits(next);
+	segment->hw.next_desc_msb = upper_32_bits(next);
 }
 
 static void xilinx_mcdma_clean_hw_desc(struct xilinx_aximcdma_desc_hw *hw)
@@ -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);
 }

---
base-commit: 0d995da5fb97e8c312834575604d4423eb6225b7
change-id: 20260817-fix-hw-buf-desc-after-cyclic-mode-cf0a7834ab50

Best regards,
--  
Alex Bereza <alex@bereza.email>

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

[sashiko-bot]

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