Re: [PATCH 3/5] dmaengine: xilinx_dma: Allocate AXI DMA segments in ring order

[Frank Li]

On Thu, Oct 22, 2026 at 03:11:32PM -0700, Jeff C. Jensen wrote:
> The AXI DMA driver builds a static ring of hardware descriptors
> (segments) when the channel is allocated, each linked to the next in
> pool order, and the engine follows those links.
> xilinx_dma_prep_slave_sg() writes no next pointers within a descriptor,
> and a descriptor appended while the engine is running is reached through
> the next pointer of the previous tail. Segments are handed out from
> free_seg_list, so this only works while the free list stays in pool
> order.
>
> Commit cee9c863ee68 ("dmaengine: xilinx_dma: Fix hardware buffer
> descriptor reuse order") fixed one way the free list falls out of that
> order. As the Sashiko review of that patch pointed out [1], others
> remain:
>
>  - xilinx_dma_chan_desc_cleanup() drops chan->lock while it runs a
>    completion callback, and frees the descriptor afterwards. If the
>    channel is terminated in that window, the newer segments are returned
>    first and the oldest descriptor's segments end up at the tail of the
>    free list.
>
>  - A preparation that runs out of segments returns its partial
>    allocation to the tail of the free list, possibly after a completion
>    on another CPU has freed newer ones. It does so without taking
>    chan->lock, so it can also corrupt the list.
>
>  - A submission rejected because a cyclic transfer is running frees its
>    segments while older ones are still in use.
>
> Once the free list and the ring disagree, a later descriptor gets
> segments that are not adjacent in the ring, and the engine follows a
> next pointer into a cleaned segment with a zero length field. With a
> delay added to widen the first window, a receive stream fails right
> after a terminate:
>
>   xilinx-vdma 40400000.dma: Channel 7a90d35b has errors 10, cdr 3f053780 tdr 3f059a00
>
> Rather than keeping the free list in order on every path, allocate by
> ring position. Count the segments a transaction needs, then reserve that
> many consecutive positions, starting at a cursor, under chan->lock: all
> of them or none. A bitmap tracks the positions in use. Allocation no
> longer depends on the order in which segments are freed, so neither the
> terminate path nor the completion path can disturb it. The count uses
> xilinx_dma_calc_copysize(), like the preparation loops, so the two
> always agree.
>
> The slave-sg, cyclic and dma_vec paths all count and reserve before
> building a descriptor, so a failed preparation leaves the channel
> unchanged. A submission rejected on a busy cyclic channel cancels its
> reservation and rewinds the cursor under the same lock.
>
> This relies on commit 7ed1e3070c9b ("dmaengine: xilinx_dma: Fix hardware
> buffer descriptor chain after cyclic DMA") to restore the segment links
> after a cyclic transfer. The AXI MCDMA pool keeps its free list and is
> not changed.
>
> Link: https://lore.kernel.org/all/20260817094004.A74621F000E9@xxxxxxxxxxxxxxx/ [1]
> Fixes: 23059408b6a3 ("dmaengine: xilinx_dma: Fix race condition in the driver for multiple descriptor scenario")
> Fixes: c0bba3a99f07 ("dmaengine: vdma: Add Support for Xilinx AXI Direct Memory Access Engine")
> Fixes: 92d794dfb699 ("dmaengine: vdma: Add support for cyclic dma mode")
> Assisted-by: Claude:claude-fable-5-1
> Signed-off-by: Jeff C. Jensen <kernel@xxxxxxxxxxx>
> ---

Reviewed-by: Frank Li <Frank.Li@xxxxxxx>

>  drivers/dma/xilinx/xilinx_dma.c | 279 ++++++++++++++++++++++++++++++++--------
>  1 file changed, 224 insertions(+), 55 deletions(-)
>
> diff --git a/drivers/dma/xilinx/xilinx_dma.c b/drivers/dma/xilinx/xilinx_dma.c
> index aa714effd10d..805cd3e2bc27 100644
> --- a/drivers/dma/xilinx/xilinx_dma.c
> +++ b/drivers/dma/xilinx/xilinx_dma.c
> @@ -33,6 +33,7 @@
>   *
>   */
>
> +#include <linux/bitmap.h>
>  #include <linux/bitops.h>
>  #include <linux/dmapool.h>
>  #include <linux/dma/xilinx_dma.h>
> @@ -390,7 +391,9 @@ struct xilinx_dma_tx_descriptor {
>   * @pending_list: Descriptors waiting
>   * @active_list: Descriptors ready to submit
>   * @done_list: Complete descriptors
> - * @free_seg_list: Free descriptors
> + * @free_seg_list: Free AXI MCDMA descriptors
> + * @seg_used: AXI DMA segments in use, indexed by ring position
> + * @seg_next: Ring position the next AXI DMA reservation starts at
>   * @common: DMA common channel
>   * @desc_pool: Descriptors pool
>   * @dev: The dma device
> @@ -430,6 +433,8 @@ struct xilinx_dma_chan {
>  	struct list_head active_list;
>  	struct list_head done_list;
>  	struct list_head free_seg_list;
> +	DECLARE_BITMAP(seg_used, XILINX_DMA_NUM_DESCS);
> +	u32 seg_next;
>  	struct dma_chan common;
>  	struct dma_pool *desc_pool;
>  	struct device *dev;
> @@ -706,30 +711,53 @@ xilinx_cdma_alloc_tx_segment(struct xilinx_dma_chan *chan)
>  }
>
>  /**
> - * xilinx_axidma_alloc_tx_segment - Allocate transaction segment
> + * xilinx_axidma_reserve_segments - Reserve a run of AXI DMA segments
>   * @chan: Driver specific DMA channel
> + * @count: Segments needed, at most XILINX_DMA_NUM_DESCS
> + * @start: Ring position of the first reserved segment, set on success
>   *
> - * Return: The allocated segment on success and NULL on failure.
> + * Reserves @count consecutive ring positions starting at @chan->seg_next,
> + * wrapping at the pool end. The engine fetches descriptors in ring order, so
> + * the run cannot be placed anywhere else.
> + *
> + * Return: 0 on success, -ENOMEM when the run is not free. A failure leaves the
> + * ownership bitmap and @chan->seg_next unchanged.
>   */
> -static struct xilinx_axidma_tx_segment *
> -xilinx_axidma_alloc_tx_segment(struct xilinx_dma_chan *chan)
> +static int xilinx_axidma_reserve_segments(struct xilinx_dma_chan *chan,
> +					  u32 count, u32 *start)
>  {
> -	struct xilinx_axidma_tx_segment *segment = NULL;
>  	unsigned long flags;
> +	int ret = -ENOMEM;
> +	u32 i;
>
>  	spin_lock_irqsave(&chan->lock, flags);
> -	if (!list_empty(&chan->free_seg_list)) {
> -		segment = list_first_entry(&chan->free_seg_list,
> -					   struct xilinx_axidma_tx_segment,
> -					   node);
> -		list_del(&segment->node);
> +
> +	for (i = 0; i < count; i++) {
> +		if (test_bit((chan->seg_next + i) % XILINX_DMA_NUM_DESCS,
> +			     chan->seg_used))
> +			goto out;
>  	}
> +
> +	*start = chan->seg_next;
> +	for (i = 0; i < count; i++)
> +		__set_bit((chan->seg_next + i) % XILINX_DMA_NUM_DESCS,
> +			  chan->seg_used);
> +	chan->seg_next = (chan->seg_next + count) % XILINX_DMA_NUM_DESCS;
> +	ret = 0;
> +
> +out:
>  	spin_unlock_irqrestore(&chan->lock, flags);
>
> -	if (!segment)
> -		dev_dbg(chan->dev, "Could not find free tx segment\n");
> +	if (ret)
> +		dev_dbg(chan->dev, "no run of %u free tx segments\n", count);
>
> -	return segment;
> +	return ret;
> +}
> +
> +static struct xilinx_axidma_tx_segment *
> +xilinx_axidma_run_segment(struct xilinx_dma_chan *chan, u32 start, u32 i)
> +{
> +	return &chan->seg_v[(start + i) % XILINX_DMA_NUM_DESCS];
>  }
>
>  /**
> @@ -792,13 +820,18 @@ static void xilinx_mcdma_clean_hw_desc(struct xilinx_aximcdma_desc_hw *hw)
>   * xilinx_dma_free_tx_segment - Free transaction segment
>   * @chan: Driver specific DMA channel
>   * @segment: DMA transaction segment
> + *
> + * Called with chan->lock held, so no reservation sees the segment between its
> + * links being restored and its ownership being released.
>   */
>  static void xilinx_dma_free_tx_segment(struct xilinx_dma_chan *chan,
>  				struct xilinx_axidma_tx_segment *segment)
>  {
> +	lockdep_assert_held(&chan->lock);
> +
>  	xilinx_dma_clean_hw_desc(chan, segment);
>
> -	list_add_tail(&segment->node, &chan->free_seg_list);
> +	__clear_bit(segment - chan->seg_v, chan->seg_used);
>  }
>
>  /**
> @@ -902,6 +935,34 @@ xilinx_dma_free_tx_descriptor(struct xilinx_dma_chan *chan,
>  	kfree(desc);
>  }
>
> +/**
> + * xilinx_dma_cancel_tx_descriptor - Discard a descriptor before submission
> + * @chan: Driver specific DMA channel
> + * @desc: Prepared descriptor
> + *
> + * Called with chan->lock held. On an AXI DMA channel, releases the reservation
> + * of @desc and rewinds @chan->seg_next to its first segment, so that no
> + * cleaned segment is left between two live descriptors. @desc must hold the
> + * most recent reservation on the channel.
> + */
> +static void xilinx_dma_cancel_tx_descriptor(struct xilinx_dma_chan *chan,
> +					    struct xilinx_dma_tx_descriptor *desc)
> +{
> +	struct xilinx_axidma_tx_segment *first;
> +
> +	lockdep_assert_held(&chan->lock);
> +
> +	if (chan->xdev->dma_config->dmatype == XDMA_TYPE_AXIDMA) {
> +		first = list_first_entry_or_null(&desc->segments,
> +						 struct xilinx_axidma_tx_segment,
> +						 node);
> +		if (first)
> +			chan->seg_next = first - chan->seg_v;
> +	}
> +
> +	xilinx_dma_free_tx_descriptor(chan, desc);
> +}
> +
>  /* Required functions */
>
>  /**
> @@ -951,10 +1012,6 @@ static void xilinx_dma_free_chan_resources(struct dma_chan *dchan)
>  	xilinx_dma_free_descriptors(chan);
>
>  	if (chan->xdev->dma_config->dmatype == XDMA_TYPE_AXIDMA) {
> -		spin_lock_irqsave(&chan->lock, flags);
> -		INIT_LIST_HEAD(&chan->free_seg_list);
> -		spin_unlock_irqrestore(&chan->lock, flags);
> -
>  		/* Free memory that is allocated for BD */
>  		dma_free_coherent(chan->dev, sizeof(*chan->seg_v) *
>  				  XILINX_DMA_NUM_DESCS, chan->seg_v,
> @@ -1195,9 +1252,9 @@ static int xilinx_dma_alloc_chan_resources(struct dma_chan *dchan)
>  				((i + 1) % XILINX_DMA_NUM_DESCS));
>  			chan->seg_v[i].phys = chan->seg_p +
>  				sizeof(*chan->seg_v) * i;
> -			list_add_tail(&chan->seg_v[i].node,
> -				      &chan->free_seg_list);
>  		}
> +		bitmap_zero(chan->seg_used, XILINX_DMA_NUM_DESCS);
> +		chan->seg_next = 0;
>  	} else if (chan->xdev->dma_config->dmatype == XDMA_TYPE_AXIMCDMA) {
>  		/* Allocate the buffer descriptors. */
>  		chan->seg_mv = dma_alloc_coherent(chan->dev,
> @@ -1282,6 +1339,98 @@ static int xilinx_dma_calc_copysize(struct xilinx_dma_chan *chan,
>  	return copy;
>  }
>
> +/**
> + * xilinx_axidma_count_split - Count the descriptors a length splits into
> + * @chan: Driver specific DMA channel
> + * @len: Length in bytes
> + *
> + * Splits @len with xilinx_dma_calc_copysize(), so the count equals the number
> + * of segments a preparation builds for @len. Intermediate chunks are rounded
> + * down for alignment, so the count depends on the channel alignment as well as
> + * on @len.
> + *
> + * Return: The descriptor count, which is zero for a zero length, or -EINVAL if
> + * a split makes no progress or @len needs more descriptors than the pool holds.
> + */
> +static int xilinx_axidma_count_split(struct xilinx_dma_chan *chan, size_t len)
> +{
> +	size_t used = 0;
> +	int count = 0;
> +
> +	while (used < len) {
> +		size_t copy = xilinx_dma_calc_copysize(chan, len, used);
> +
> +		if (!copy || count == XILINX_DMA_NUM_DESCS)
> +			return -EINVAL;
> +
> +		used += copy;
> +		count++;
> +	}
> +
> +	return count;
> +}
> +
> +/**
> + * xilinx_axidma_count_sg - Count the descriptors a mapped scatterlist needs
> + * @chan: Driver specific DMA channel
> + * @sgl: Mapped scatterlist
> + * @sg_len: Number of mapped entries in @sgl
> + *
> + * Return: The descriptor count, or -EINVAL if the list splits into no
> + * descriptors at all or into more than the pool holds.
> + */
> +static int xilinx_axidma_count_sg(struct xilinx_dma_chan *chan,
> +				  struct scatterlist *sgl, unsigned int sg_len)
> +{
> +	struct scatterlist *sg;
> +	unsigned int i;
> +	int total = 0;
> +
> +	for_each_sg(sgl, sg, sg_len, i) {
> +		int count = xilinx_axidma_count_split(chan, sg_dma_len(sg));
> +
> +		if (count < 0 || count > XILINX_DMA_NUM_DESCS - total)
> +			return -EINVAL;
> +
> +		total += count;
> +	}
> +
> +	if (!total)
> +		return -EINVAL;
> +
> +	return total;
> +}
> +
> +/**
> + * xilinx_axidma_count_vecs - Count the descriptors an array of DMA vectors needs
> + * @chan: Driver specific DMA channel
> + * @vecs: DMA vectors
> + * @nb: Entry count
> + *
> + * Return: The descriptor count, or -EINVAL if the vectors split into no
> + * descriptors at all or into more than the pool holds.
> + */
> +static int xilinx_axidma_count_vecs(struct xilinx_dma_chan *chan,
> +				    const struct dma_vec *vecs, size_t nb)
> +{
> +	size_t i;
> +	int total = 0;
> +
> +	for (i = 0; i < nb; i++) {
> +		int count = xilinx_axidma_count_split(chan, vecs[i].len);
> +
> +		if (count < 0 || count > XILINX_DMA_NUM_DESCS - total)
> +			return -EINVAL;
> +
> +		total += count;
> +	}
> +
> +	if (!total)
> +		return -EINVAL;
> +
> +	return total;
> +}
> +
>  /**
>   * xilinx_dma_tx_status - Get DMA transaction status
>   * @dchan: DMA channel
> @@ -2050,6 +2199,10 @@ static void append_desc_queue(struct xilinx_dma_chan *chan,
>   * xilinx_dma_tx_submit - Submit DMA transaction
>   * @tx: Async transaction descriptor
>   *
> + * An AXI DMA channel reserves segment runs in ring order, so a transaction must
> + * be submitted, or its rejection handled, before the next one is prepared on
> + * the same channel.
> + *
>   * Return: cookie value on success and failure value on error
>   */
>  static dma_cookie_t xilinx_dma_tx_submit(struct dma_async_tx_descriptor *tx)
> @@ -2060,8 +2213,11 @@ static dma_cookie_t xilinx_dma_tx_submit(struct dma_async_tx_descriptor *tx)
>  	unsigned long flags;
>  	int err;
>
> +	spin_lock_irqsave(&chan->lock, flags);
> +
>  	if (chan->cyclic) {
> -		xilinx_dma_free_tx_descriptor(chan, desc);
> +		xilinx_dma_cancel_tx_descriptor(chan, desc);
> +		spin_unlock_irqrestore(&chan->lock, flags);
>  		return -EBUSY;
>  	}
>
> @@ -2070,13 +2226,13 @@ static dma_cookie_t xilinx_dma_tx_submit(struct dma_async_tx_descriptor *tx)
>  		 * If reset fails, need to hard reset the system.
>  		 * Channel is no longer functional
>  		 */
> +		spin_unlock_irqrestore(&chan->lock, flags);
>  		err = xilinx_dma_chan_reset(chan);
>  		if (err < 0)
>  			return err;
> +		spin_lock_irqsave(&chan->lock, flags);
>  	}
>
> -	spin_lock_irqsave(&chan->lock, flags);
> -
>  	cookie = dma_cookie_assign(tx);
>
>  	/* Put this transaction onto the tail of the pending queue */
> @@ -2254,15 +2410,26 @@ static struct dma_async_tx_descriptor *xilinx_dma_prep_peripheral_dma_vec(
>  	struct xilinx_axidma_tx_segment *segment, *head, *prev = NULL;
>  	size_t copy;
>  	size_t sg_used;
> +	u32 start = 0, taken = 0;
>  	unsigned int i;
> +	int needed;
>
>  	if (!is_slave_direction(direction) || direction != chan->direction)
>  		return NULL;
>
> +	needed = xilinx_axidma_count_vecs(chan, vecs, nb);
> +	if (needed < 0)
> +		return NULL;
> +
>  	desc = xilinx_dma_alloc_tx_descriptor(chan);
>  	if (!desc)
>  		return NULL;
>
> +	if (xilinx_axidma_reserve_segments(chan, needed, &start)) {
> +		xilinx_dma_free_tx_descriptor(chan, desc);
> +		return NULL;
> +	}
> +
>  	dma_async_tx_descriptor_init(&desc->async_tx, &chan->common);
>  	desc->async_tx.tx_submit = xilinx_dma_tx_submit;
>
> @@ -2274,10 +2441,8 @@ static struct dma_async_tx_descriptor *xilinx_dma_prep_peripheral_dma_vec(
>  		while (sg_used < vecs[i].len) {
>  			struct xilinx_axidma_desc_hw *hw;
>
> -			/* Get a free segment */
> -			segment = xilinx_axidma_alloc_tx_segment(chan);
> -			if (!segment)
> -				goto error;
> +			segment = xilinx_axidma_run_segment(chan, start,
> +							    taken++);
>
>  			/*
>  			 * Calculate the maximum number of bytes to transfer,
> @@ -2305,9 +2470,6 @@ static struct dma_async_tx_descriptor *xilinx_dma_prep_peripheral_dma_vec(
>  		}
>  	}
>
> -	if (list_empty(&desc->segments))
> -		goto error;
> -
>  	head = list_first_entry(&desc->segments, struct xilinx_axidma_tx_segment, node);
>  	desc->async_tx.phys = head->phys;
>
> @@ -2324,10 +2486,6 @@ static struct dma_async_tx_descriptor *xilinx_dma_prep_peripheral_dma_vec(
>  		desc->async_tx.metadata_ops = &xilinx_dma_metadata_ops;
>
>  	return &desc->async_tx;
> -
> -error:
> -	xilinx_dma_free_tx_descriptor(chan, desc);
> -	return NULL;
>  }
>
>  /**
> @@ -2353,16 +2511,28 @@ static struct dma_async_tx_descriptor *xilinx_dma_prep_slave_sg(
>  	struct scatterlist *sg;
>  	size_t copy;
>  	size_t sg_used;
> +	u32 start = 0, taken = 0;
>  	unsigned int i;
> +	int needed;
>
>  	if (!is_slave_direction(direction))
>  		return NULL;
>
> +	/* reserve exactly the segments the loop below builds */
> +	needed = xilinx_axidma_count_sg(chan, sgl, sg_len);
> +	if (needed < 0)
> +		return NULL;
> +
>  	/* Allocate a transaction descriptor. */
>  	desc = xilinx_dma_alloc_tx_descriptor(chan);
>  	if (!desc)
>  		return NULL;
>
> +	if (xilinx_axidma_reserve_segments(chan, needed, &start)) {
> +		xilinx_dma_free_tx_descriptor(chan, desc);
> +		return NULL;
> +	}
> +
>  	dma_async_tx_descriptor_init(&desc->async_tx, &chan->common);
>  	desc->async_tx.tx_submit = xilinx_dma_tx_submit;
>
> @@ -2374,10 +2544,8 @@ static struct dma_async_tx_descriptor *xilinx_dma_prep_slave_sg(
>  		while (sg_used < sg_dma_len(sg)) {
>  			struct xilinx_axidma_desc_hw *hw;
>
> -			/* Get a free segment */
> -			segment = xilinx_axidma_alloc_tx_segment(chan);
> -			if (!segment)
> -				goto error;
> +			segment = xilinx_axidma_run_segment(chan, start,
> +							    taken++);
>
>  			/*
>  			 * Calculate the maximum number of bytes to transfer,
> @@ -2409,9 +2577,6 @@ static struct dma_async_tx_descriptor *xilinx_dma_prep_slave_sg(
>  		}
>  	}
>
> -	if (list_empty(&desc->segments))
> -		goto error;
> -
>  	segment = list_first_entry(&desc->segments,
>  				   struct xilinx_axidma_tx_segment, node);
>  	desc->async_tx.phys = segment->phys;
> @@ -2429,10 +2594,6 @@ static struct dma_async_tx_descriptor *xilinx_dma_prep_slave_sg(
>  		desc->async_tx.metadata_ops = &xilinx_dma_metadata_ops;
>
>  	return &desc->async_tx;
> -
> -error:
> -	xilinx_dma_free_tx_descriptor(chan, desc);
> -	return NULL;
>  }
>
>  /**
> @@ -2456,7 +2617,8 @@ static struct dma_async_tx_descriptor *xilinx_dma_prep_dma_cyclic(
>  	struct xilinx_axidma_tx_segment *segment, *head_segment, *prev = NULL;
>  	size_t copy, sg_used, periods;
>  	unsigned int num_periods;
> -	int i;
> +	u32 start = 0, taken = 0;
> +	int i, per_period;
>  	u32 reg;
>
>  	if (!period_len)
> @@ -2471,11 +2633,24 @@ static struct dma_async_tx_descriptor *xilinx_dma_prep_dma_cyclic(
>  	if (!is_slave_direction(direction))
>  		return NULL;
>
> +	/* the division bounds per_period * num_periods without overflow */
> +	per_period = xilinx_axidma_count_split(chan, period_len);
> +	if (per_period <= 0)
> +		return NULL;
> +	if ((u32)per_period > XILINX_DMA_NUM_DESCS / num_periods)
> +		return NULL;
> +
>  	/* Allocate a transaction descriptor. */
>  	desc = xilinx_dma_alloc_tx_descriptor(chan);
>  	if (!desc)
>  		return NULL;
>
> +	if (xilinx_axidma_reserve_segments(chan, per_period * num_periods,
> +					   &start)) {
> +		xilinx_dma_free_tx_descriptor(chan, desc);
> +		return NULL;
> +	}
> +
>  	chan->direction = direction;
>  	dma_async_tx_descriptor_init(&desc->async_tx, &chan->common);
>  	desc->async_tx.tx_submit = xilinx_dma_tx_submit;
> @@ -2486,10 +2661,8 @@ static struct dma_async_tx_descriptor *xilinx_dma_prep_dma_cyclic(
>  		while (sg_used < period_len) {
>  			struct xilinx_axidma_desc_hw *hw;
>
> -			/* Get a free segment */
> -			segment = xilinx_axidma_alloc_tx_segment(chan);
> -			if (!segment)
> -				goto error;
> +			segment = xilinx_axidma_run_segment(chan, start,
> +							    taken++);
>
>  			/*
>  			 * Calculate the maximum number of bytes to transfer,
> @@ -2537,10 +2710,6 @@ static struct dma_async_tx_descriptor *xilinx_dma_prep_dma_cyclic(
>  	}
>
>  	return &desc->async_tx;
> -
> -error:
> -	xilinx_dma_free_tx_descriptor(chan, desc);
> -	return NULL;
>  }
>
>  /**
>
> --
> 2.43.0
>