[v5] dmaengine: sun6i-dma: Fix reclaim descriptors while terminating DMA

When terminating DMA transfers, active descriptors are not properly
reclaimed. Only cyclic descriptors were handled, leaving non-cyclic
descriptors and their LLI chains to be permanently leaked.

Fix by using vchan_terminate_vdesc() which handles both cyclic and
non-cyclic descriptors by adding them to desc_terminated queue for
proper cleanup.

Add pchan->desc != pchan->done check to prevent double-adding completed
descriptors, which would corrupt the list.

Fixes: 555859308723 ("dmaengine: sun6i: Add driver for the Allwinner A31 DMA controller")
Signed-off-by: Hongling Zeng <zenghongling@kylinos.cn>
Acked-by: Jernej Skrabec <jernej.skrabec@gmail.com>
Suggested-by: Frank Li <Frank.li@oss.nxp.com>
Reviewed-by: Frank Li <Frank.Li@nxp.com>

---
 Change in v2;
 -Add pchan->desc != pchan->done check to prevent race condition
  where completed descriptors could be double-added to desc_completed
  list, causing list corruption
---
 Change in v3:
 -Fix by using vchan_terminate_vdesc() as suggested by Frank Li
---
 Change in v4:
 -Correct the commit message
---
 Chaneg in v5:
 -Correct the subject
 -Add reviewed-by
---
 drivers/dma/sun6i-dma.c | 13 +++++--------
 1 file changed, 5 insertions(+), 8 deletions(-)
  

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

Pre-existing issues:
- [High] Unprotected out-of-lock read of `vchan->phy` leads to a severe TOCTOU race causing list corruption and wrong channel teardown.
- [High] TOCTOU race in hardware interrupt handler leads to a fatal NULL pointer dereference when terminating DMA.
- [High] Missing IRQ masking when acquiring `sdev->lock` in process context causes a deadlock with the softirq tasklet.
- [High] Incorrect array indexing in `sun6i_dma_interrupt` incorrectly routes upper hardware channels to lower channel states.
--

commit 494d0d1002ddd0015efbf5a3a6ef1f6894fb2914
Author: Hongling Zeng <zenghongling@kylinos.cn>

dmaengine: sun6i-dma: Fix reclaim descriptors while terminating DMA

This commit updates sun6i_dma_terminate_all() to use vchan_terminate_vdesc() 
so that both cyclic and non-cyclic descriptors are properly reclaimed. It 
also adds a check to prevent double-adding completed descriptors to the list.

> diff --git a/drivers/dma/sun6i-dma.c b/drivers/dma/sun6i-dma.c
> index a9a254dbf8cb9..f47a326dd7ffa 100644
> --- a/drivers/dma/sun6i-dma.c
> +++ b/drivers/dma/sun6i-dma.c
> @@ -945,16 +945,13 @@ static int sun6i_dma_terminate_all(struct dma_chan *chan)
>  

[Severity: High]
This is a pre-existing issue, but just above this hunk, the function acquires
sdev->lock using a plain spin_lock():

sun6i_dma_terminate_all() {
        ...
        spin_lock(&sdev->lock);
        list_del_init(&vchan->node);
        spin_unlock(&sdev->lock);

Could this plain spin_lock cause a deadlock?

Since this is called from process context, hardware interrupts remain
enabled. If a DMA interrupt fires on this CPU while the lock is held,
the softirq can execute sun6i_dma_tasklet() which attempts to acquire
the same lock with spin_lock_irq():

sun6i_dma_tasklet() {
        spin_lock_irq(&sdev->lock);
}

Could this cause the tasklet to spin forever waiting for the interrupted
process to release it? The same pattern appears in sun6i_dma_pause().

>  	spin_lock_irqsave(&vchan->vc.lock, flags);
>  
> -	if (vchan->cyclic) {
> -		vchan->cyclic = false;
> -		if (pchan && pchan->desc) {
> -			struct virt_dma_desc *vd = &pchan->desc->vd;
> -			struct virt_dma_chan *vc = &vchan->vc;
> +	if (pchan && pchan->desc && pchan->desc != pchan->done) {
> +		struct virt_dma_desc *vd = &pchan->desc->vd;
>  
> -			list_add_tail(&vd->node, &vc->desc_completed);
> -		}
> +		vchan_terminate_vdesc(vd);

[Severity: High]
This is a pre-existing issue, but is it safe to trust the pchan pointer
here?

The pchan pointer is read locklessly at the start of the function
(struct sun6i_pchan *pchan = vchan->phy;). If a tasklet runs concurrently
before the locks are acquired, couldn't it complete the current descriptor,
free this physical channel, and reallocate it to a different virtual
channel?

If that happens, this code would evaluate the stale pointer inside the
critical section and mistakenly call vchan_terminate_vdesc() on another
channel's descriptors.

>  	}
>  
> +	vchan->cyclic = false;
>  	vchan_get_all_descriptors(&vchan->vc, &head);
>  
>  	if (pchan) {

[Severity: High]
This is a pre-existing issue, but inside this if block, the descriptor
pointer is cleared:

sun6i_dma_terminate_all() {
        ...
        if (pchan) {
                ...
                pchan->desc = NULL;
        }
}

Could clearing the descriptor pointer here race with the interrupt handler?

If another CPU is actively processing a hardware interrupt when this channel
is terminated:

CPU1: sun6i_dma_interrupt() validates vchan locklessly
CPU2: sun6i_dma_terminate_all() sets pchan->desc = NULL;
CPU1: sun6i_dma_interrupt() calls vchan_cyclic_callback(&pchan->desc->vd);

Would this lead to a NULL pointer dereference when evaluating the
descriptor on CPU1?


While reviewing the driver, I noticed another pre-existing issue in
sun6i_dma_interrupt():

drivers/dma/sun6i-dma.c:sun6i_dma_interrupt() {
        ...
        for (i = 0; i < sdev->num_pchans / DMA_IRQ_CHAN_NR; i++) {
                ...
                for (j = 0; j < DMA_IRQ_CHAN_NR; j++) {
                        ...
                        pchan = sdev->pchans + j;

[Severity: High]
This is a pre-existing issue, but does this inner loop incorrectly route
upper-bank interrupts to the lower physical channels?

For SoCs with more than 8 DMA channels, the outer loop checks register
banks (i = 0, 1). However, the physical channel lookup uses
sdev->pchans + j instead of including the offset for the current bank
(sdev->pchans + i * DMA_IRQ_CHAN_NR + j).

Could an interrupt from physical channel 8 or above incorrectly process
the virtual channel attached to physical channel 0, potentially causing
descriptors to complete for the wrong channel?
  

On 01-07-26, 12:57, Hongling Zeng wrote:
> When terminating DMA transfers, active descriptors are not properly
> reclaimed. Only cyclic descriptors were handled, leaving non-cyclic
> descriptors and their LLI chains to be permanently leaked.
> 
> Fix by using vchan_terminate_vdesc() which handles both cyclic and
> non-cyclic descriptors by adding them to desc_terminated queue for
> proper cleanup.
> 
> Add pchan->desc != pchan->done check to prevent double-adding completed
> descriptors, which would corrupt the list.

You should run checkpatch to fix the formatting issues.
ERROR: trailing whitespace
#65: FILE: drivers/dma/sun6i-dma.c:95

For now, I have fixed and applied
  

On Wed, 01 Jul 2026 12:57:33 +0800, Hongling Zeng wrote:
> When terminating DMA transfers, active descriptors are not properly
> reclaimed. Only cyclic descriptors were handled, leaving non-cyclic
> descriptors and their LLI chains to be permanently leaked.
> 
> Fix by using vchan_terminate_vdesc() which handles both cyclic and
> non-cyclic descriptors by adding them to desc_terminated queue for
> proper cleanup.
> 
> [...]

Applied, thanks!

[1/1] dmaengine: sun6i-dma: Fix reclaim descriptors while terminating DMA
      commit: ab1150115e68a46b687eb38c1ab92782018c9f2c

Best regards,