[PATCH] dmaengine: xilinx_dma: Fix hardware buffer descriptor reuse order

[PATCH] dmaengine: xilinx_dma: Fix hardware buffer descriptor reuse order

[Alex Bereza]

xilinx_dma_alloc_chan_resources() builds a static ring of hardware
buffer descriptors once and the driver uses this ring throughout the
lifetime of a channel. This requires the allocation order of hardware
buffer descriptors from chan->free_seg_list to stay in sync with the
hardware buffer descriptor ring built at channel allocation time by
returning oldest descriptors to chan->free_seg_list first.

When chan->pending_list is not empty e.g. during
xilinx_dma_terminate_all() the chan->free_seg_list and the order of the
static hardware buffer descriptor ring get out of sync. Descriptors age
in this order: pending -> active -> done. So freeing pending_list first
returns the newest buffer descriptors to the chan->free_seg_list first
and thus breaks the order required by the static hardware buffer
descriptor ring. Then when the channel is reused, after a wrap around of
the free_seg_list the DMA will find a hardware buffer descriptor with a
length field that is still zeroed and stop with something like this:

  xilinx-vdma 86000000.dma: Channel 000000003a21d7b8 has errors 10, cdr 6de4c000 tdr 6de4c000

After this no more descriptors are completed and a consumer potentially
blocks and waits forever. The only way to get out of this error state is
to rebuild the static hardware buffer descriptor ring and the
free_seg_list by releasing and re-acquiring the channel.

Fix the order in which hardware buffer descriptors are returned to
free_seg_list to ensure the mentioned requirement holds.

Fixes: 23059408b6a3 ("dmaengine: xilinx_dma: Fix race condition in the driver for multiple descriptor scenario")
Signed-off-by: Alex Bereza <alex@bereza.email>
---
 drivers/dma/xilinx/xilinx_dma.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/dma/xilinx/xilinx_dma.c b/drivers/dma/xilinx/xilinx_dma.c
index bef2b031dba1..0817b74f7450 100644
--- a/drivers/dma/xilinx/xilinx_dma.c
+++ b/drivers/dma/xilinx/xilinx_dma.c
@@ -920,9 +920,9 @@ static void xilinx_dma_free_descriptors(struct xilinx_dma_chan *chan)
 
 	spin_lock_irqsave(&chan->lock, flags);
 
-	xilinx_dma_free_desc_list(chan, &chan->pending_list);
 	xilinx_dma_free_desc_list(chan, &chan->done_list);
 	xilinx_dma_free_desc_list(chan, &chan->active_list);
+	xilinx_dma_free_desc_list(chan, &chan->pending_list);
 
 	spin_unlock_irqrestore(&chan->lock, flags);
 }

---
base-commit: 0d995da5fb97e8c312834575604d4423eb6225b7
change-id: 20260817-fix-hw-buf-desc-reuse-b730e9e02185

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

Re: [PATCH] dmaengine: xilinx_dma: Fix hardware buffer descriptor reuse order

[sashiko-bot]

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

Pre-existing issues:
- [Critical] Missing `chan->lock` protection when freeing descriptors in error paths leads to data corruption in `chan->free_seg_list`.
- [Critical] Race condition between dmaengine_terminate_all() and dmaengine_submit() overwrites the chan->terminating flag, leading to a Use-After-Free.
- [High] Race condition between xilinx_dma_chan_desc_cleanup and xilinx_dma_terminate_all corrupts the static descriptor ring order, defeating the purpose of this patch.
- [High] Rejecting a transaction in xilinx_dma_tx_submit() due to cyclic mode frees descriptors out of order, permanently breaking the hardware static ring.
- [High] Calling dma_run_dependencies() while holding chan->lock causes a deadlock if the dependent transaction is on the same channel.
--

--- Patch [1]: [PATCH] dmaengine: xilinx_dma: Fix hardware buffer descriptor reuse order ---
commit 55ac771a816780299793bc5e59ad89b6ef3446b0
Author: Alex Bereza <alex@bereza.email>

dmaengine: xilinx_dma: Fix hardware buffer descriptor reuse order
    
This patch corrects the order in which hardware buffer descriptors are returned
to the free segment list during channel cleanup. By freeing the pending list
last, it ensures descriptors are returned in chronological order to maintain
the hardware static ring structure.

> diff --git a/drivers/dma/xilinx/xilinx_dma.c b/drivers/dma/xilinx/xilinx_dma.c
> index bef2b031dba19..0817b74f74505 100644
> --- a/drivers/dma/xilinx/xilinx_dma.c
> +++ b/drivers/dma/xilinx/xilinx_dma.c
> @@ -920,9 +920,9 @@ static void xilinx_dma_free_descriptors(struct xilinx_dma_chan *chan)
>  
>  	spin_lock_irqsave(&chan->lock, flags);
>  
> -	xilinx_dma_free_desc_list(chan, &chan->pending_list);
>  	xilinx_dma_free_desc_list(chan, &chan->done_list);
>  	xilinx_dma_free_desc_list(chan, &chan->active_list);
> +	xilinx_dma_free_desc_list(chan, &chan->pending_list);
>  
>  	spin_unlock_irqrestore(&chan->lock, flags);
>  }

[Severity: High]
This is a pre-existing issue, but does a race condition between
xilinx_dma_chan_desc_cleanup() and xilinx_dma_terminate_all() corrupt the
static descriptor ring order, defeating the purpose of this patch?

The tasklet xilinx_dma_chan_desc_cleanup() drops chan->lock to execute client
callbacks:

drivers/dma/xilinx/xilinx_dma.c:xilinx_dma_chan_desc_cleanup() {
        ...
        spin_unlock_irqrestore(&chan->lock, flags);
        dmaengine_desc_get_callback_invoke(&desc->async_tx, &result);
        spin_lock_irqsave(&chan->lock, flags);
        ...
        xilinx_dma_free_tx_descriptor(chan, desc);
}

During this window, xilinx_dma_terminate_all() can execute concurrently,
acquire the lock, and free newer descriptors into chan->free_seg_list
by calling xilinx_dma_free_descriptors(chan).

When the tasklet resumes and frees its older descriptor, it appends it after
the newer ones, which scrambles the static ring order the patch intended to
preserve. Can this lead to a fatal dma internal error when the channel
is reused?

[Severity: High]
This is a pre-existing issue, but does rejecting a transaction in
xilinx_dma_tx_submit() due to cyclic mode also break the hardware static ring?

If a normal transfer is submitted while chan->cyclic is true, the descriptors
are immediately freed:

drivers/dma/xilinx/xilinx_dma.c:xilinx_dma_tx_submit() {
        ...
        if (chan->cyclic) {
                xilinx_dma_free_tx_descriptor(chan, desc);
                return -EBUSY;
        }
        ...
}

Because older cyclic segments are still active and will be freed later, this
appends newer segments before older ones into chan->free_seg_list. Could this
permanently break the chronological order required by the hardware?

[Severity: Critical]
This is a pre-existing issue, but does missing chan->lock protection when
freeing descriptors in error paths lead to linked list corruption?

In the cyclic check above and in the error path of xilinx_dma_prep_slave_sg():

drivers/dma/xilinx/xilinx_dma.c:xilinx_dma_prep_slave_sg() {
        ...
error:
        xilinx_dma_free_tx_descriptor(chan, desc);
        return NULL;
}

The function xilinx_dma_free_tx_descriptor() is called without acquiring
chan->lock. This leads to an unlocked list_add_tail():

drivers/dma/xilinx/xilinx_dma.c:xilinx_dma_free_tx_segment() {
        ...
        xilinx_dma_clean_hw_desc(&segment->hw);
        list_add_tail(&segment->node, &chan->free_seg_list);
}

Could this race against other contexts like the tasklet or normal allocations
that modify the list under the lock, causing list corruption in
chan->free_seg_list?

[Severity: High]
This is a pre-existing issue, but does calling dma_run_dependencies() while
holding chan->lock cause a deadlock if the dependent transaction is on the
same channel?

The tasklet re-acquires the lock before running dependencies:

drivers/dma/xilinx/xilinx_dma.c:xilinx_dma_chan_desc_cleanup() {
        ...
        spin_lock_irqsave(&chan->lock, flags);

        /* Run any dependencies, then free the descriptor */
        dma_run_dependencies(&desc->async_tx);
        ...
}

If CONFIG_ASYNC_TX_ENABLE_CHANNEL_SWITCH is enabled and there is a dependent
descriptor on the same channel, dma_run_dependencies() invokes the submit
callback, routing back to xilinx_dma_tx_submit():

drivers/dma/xilinx/xilinx_dma.c:xilinx_dma_tx_submit() {
        ...
        spin_lock_irqsave(&chan->lock, flags);
        ...
}

Since spinlocks are not recursive, won't this attempt to acquire chan->lock
again result in an immediate deadlock?

[Severity: Critical]
This is a pre-existing issue, but does a race condition between
dmaengine_terminate_all() and dmaengine_submit() overwrite the
chan->terminating flag, causing a use-after-free?

A concurrent xilinx_dma_terminate_all() sets the flag without the lock:

drivers/dma/xilinx/xilinx_dma.c:xilinx_dma_terminate_all() {
        ...
        if (!chan->cyclic)
                chan->terminating = true;
        ...
}

Then xilinx_dma_tx_submit() unconditionally clears it under the lock:

drivers/dma/xilinx/xilinx_dma.c:xilinx_dma_tx_submit() {
        ...
        chan->terminating = false;

        spin_unlock_irqrestore(&chan->lock, flags);
        ...
}

If the tasklet xilinx_dma_chan_desc_cleanup() dropped the lock to run
callbacks, and a client terminates and then submits a new transfer,
chan->terminating becomes false.

drivers/dma/xilinx/xilinx_dma.c:xilinx_dma_chan_desc_cleanup() {
        ...
        /*
         * While we ran a callback the user called a terminate function,
         * which takes care of cleaning up any remaining descriptors
         */
        if (chan->terminating)
                break;
        ...
}

When the tasklet resumes, it fails to break out of the loop. Does this result
in dereferencing a freed and recycled next pointer from the safe list
iterator?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260817-fix-hw-buf-desc-reuse-v1-1-d79827a844c7@bereza.email?part=1

Re: [PATCH] dmaengine: xilinx_dma: Fix hardware buffer descriptor reuse order

[Frank Li]

On Mon, Aug 17, 2026 at 11:23:55AM +0200, Alex Bereza wrote:
> [You don't often get email from alex@bereza.email. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ]
>
> xilinx_dma_alloc_chan_resources() builds a static ring of hardware
> buffer descriptors once and the driver uses this ring throughout the
> lifetime of a channel. This requires the allocation order of hardware
> buffer descriptors from chan->free_seg_list to stay in sync with the
> hardware buffer descriptor ring built at channel allocation time by
> returning oldest descriptors to chan->free_seg_list first.
>
> When chan->pending_list is not empty e.g. during
> xilinx_dma_terminate_all() the chan->free_seg_list and the order of the
> static hardware buffer descriptor ring get out of sync. Descriptors age
> in this order: pending -> active -> done. So freeing pending_list first
> returns the newest buffer descriptors to the chan->free_seg_list first
> and thus breaks the order required by the static hardware buffer
> descriptor ring. Then when the channel is reused, after a wrap around of
> the free_seg_list the DMA will find a hardware buffer descriptor with a
> length field that is still zeroed and stop with something like this:
>
>   xilinx-vdma 86000000.dma: Channel 000000003a21d7b8 has errors 10, cdr 6de4c000 tdr 6de4c000
>
> After this no more descriptors are completed and a consumer potentially
> blocks and waits forever. The only way to get out of this error state is
> to rebuild the static hardware buffer descriptor ring and the
> free_seg_list by releasing and re-acquiring the channel.
>
> Fix the order in which hardware buffer descriptors are returned to
> free_seg_list to ensure the mentioned requirement holds.
>
> Fixes: 23059408b6a3 ("dmaengine: xilinx_dma: Fix race condition in the driver for multiple descriptor scenario")
> Signed-off-by: Alex Bereza <alex@bereza.email>
> ---

Reviewed-by: Frank Li <Frank.Li@nxp.com>

>  drivers/dma/xilinx/xilinx_dma.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/dma/xilinx/xilinx_dma.c b/drivers/dma/xilinx/xilinx_dma.c
> index bef2b031dba1..0817b74f7450 100644
> --- a/drivers/dma/xilinx/xilinx_dma.c
> +++ b/drivers/dma/xilinx/xilinx_dma.c
> @@ -920,9 +920,9 @@ static void xilinx_dma_free_descriptors(struct xilinx_dma_chan *chan)
>
>         spin_lock_irqsave(&chan->lock, flags);
>
> -       xilinx_dma_free_desc_list(chan, &chan->pending_list);
>         xilinx_dma_free_desc_list(chan, &chan->done_list);
>         xilinx_dma_free_desc_list(chan, &chan->active_list);
> +       xilinx_dma_free_desc_list(chan, &chan->pending_list);
>
>         spin_unlock_irqrestore(&chan->lock, flags);
>  }
>
> ---
> base-commit: 0d995da5fb97e8c312834575604d4423eb6225b7
> change-id: 20260817-fix-hw-buf-desc-reuse-b730e9e02185
>
> Best regards,
> --
> Alex Bereza <alex@bereza.email>
>