Re: [PATCH 3/7] dmaengine: dw-edma: Clean up vchan descriptors on termination

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Data race in dw_edma_abort_interrupt leads to permanently stalled DMA channels.
- [High] Missing lock protection in pause and resume paths leads to list corruption and data races.
--

--- Patch [3]: [PATCH 3/7] dmaengine: dw-edma: Clean up vchan descriptors on termination ---
commit 7bc17c43c74d58634aed20f3d0b0b4c9f907d26a
Author: Koichiro Den <den@valinux.co.jp>

dmaengine: dw-edma: Clean up vchan descriptors on termination

This patch cleans up virtual channel descriptors when a channel is terminated.
It moves issued and submitted descriptors to the terminated list whenever a
termination request completes, preventing leaked descriptors or restarting
work that was already terminated. It also changes the busy-polling wait
loop to use usleep_range() and cond_resched() instead of busy-polling.

> diff --git a/drivers/dma/dw-edma/dw-edma-core.c b/drivers/dma/dw-edma/dw-edma-core.c
> index 4e0dc52397e2e..1b493c104a5b1 100644
> --- a/drivers/dma/dw-edma/dw-edma-core.c
> +++ b/drivers/dma/dw-edma/dw-edma-core.c
[ ... ]
> @@ -313,20 +337,25 @@ static int dw_edma_device_resume(struct dma_chan *dchan)
>  static int dw_edma_device_terminate_all(struct dma_chan *dchan)
>  {

[Severity: High]
This is a pre-existing issue, but can the lack of lock protection in the
pause and resume paths lead to list corruption and data races?

In dw_edma_device_resume() and dw_edma_device_pause(), the channel state is
modified concurrently without holding chan->vc.lock:

dw_edma_device_resume() {
    ...
    } else {
        chan->status = EDMA_ST_BUSY;
        dw_edma_start_transfer(chan);
    }
    ...
}

Calling dw_edma_start_transfer() unlocked invokes vchan_next_desc() (which
triggers lockdep_assert_held()) and modifies child->list with list_del().
Concurrent calls to dw_edma_device_pause() or dw_edma_device_resume(), or
in-flight interrupts, will corrupt the list pointers leading to a kernel
panic.

Should these functions acquire chan->vc.lock before modifying state and
calling dw_edma_start_transfer()?

>  	struct dw_edma_chan *chan = dchan2dw_edma_chan(dchan);
> +	unsigned long flags;
>  	int err = 0;
>  
> +	spin_lock_irqsave(&chan->vc.lock, flags);
>  	if (!chan->configured) {
[ ... ]
> @@ -680,7 +710,7 @@ static void dw_edma_done_interrupt(struct dw_edma_chan *chan)
>  			break;
>  
>  		case EDMA_REQ_STOP:
> -			dw_edma_terminate_vdesc(vd);
> +			dw_edma_terminate_all_descs(chan);
>  			chan->request = EDMA_REQ_NONE;
>  			chan->status = EDMA_ST_IDLE;
>  			break;

[Severity: High]
This isn't a bug introduced by this patch, but I noticed a potential data
race in dw_edma_abort_interrupt() that could lead to permanently stalled
DMA channels.

In dw_edma_abort_interrupt(), the spinlock is released before chan->request
and chan->status are updated:

dw_edma_abort_interrupt() {
    ...
    spin_unlock_irqrestore(&chan->vc.lock, flags);
    chan->request = EDMA_REQ_NONE;
    chan->status = EDMA_ST_IDLE;
}

If a client concurrently calls dw_edma_device_issue_pending(), it will
acquire the lock, observe the old state (not IDLE), and skip starting the
transfer. Then the interrupt handler updates the state to IDLE outside the
lock. The pending descriptors remain on the queue but the hardware is never
started.

Could the state updates be moved inside the locked region to prevent this?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260710080903.2392888-1-den@valinux.co.jp?part=3