Re: [PATCH v4 2/9] dmaengine: dw-edma: Terminate all descriptors without callbacks

[sashiko-bot@kernel.org]

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

New issues:
- [High] If channel termination times out, the channel state becomes permanently wedged in BUSY, rendering it unusable.

Pre-existing issues:
- [High] Pre-existing missing locking in `dw_edma_device_pause()` causes data races with interrupt handlers.
--

--- Patch [2]: [PATCH v4 2/9] dmaengine: dw-edma: Terminate all descriptors without callbacks ---
commit 9db3cc9880b9ba6f4fe534fd17ede7df5d2a503f
Author: Koichiro Den <den@valinux.co.jp>

dmaengine: dw-edma: Terminate all descriptors without callbacks

This commit moves all issued and submitted descriptors to the terminated
list when termination completes. It also introduces a wait for pending
STOPs before releasing resources to prevent late callbacks from
dereferencing freed client state.

> diff --git a/drivers/dma/dw-edma/dw-edma-core.c b/drivers/dma/dw-edma/dw-edma-core.c
> --- a/drivers/dma/dw-edma/dw-edma-core.c
> +++ b/drivers/dma/dw-edma/dw-edma-core.c
> @@ -673,8 +705,7 @@ static void dw_edma_done_interrupt(struct dw_edma_chan *chan)
>  			break;
>  
>  		case EDMA_REQ_STOP:
> -			list_del(&vd->node);
> -			vchan_cookie_complete(vd);
> +			dw_edma_terminate_all_descs(chan);
>  			chan->request = EDMA_REQ_NONE;
>  			chan->status = EDMA_ST_IDLE;
>  			break;

[Severity: High]
This is a pre-existing issue, but can dw_edma_device_pause() race with
the interrupt handler and wedge the channel?

dw_edma_device_pause() modifies chan->request and chan->status locklessly.

If it races with dw_edma_done_interrupt() (which holds the lock and sets
chan->status = EDMA_ST_IDLE here), dw_edma_device_pause() can concurrently
set chan->request = EDMA_REQ_PAUSE on an IDLE channel.

This leaves the channel in a state where dw_edma_device_resume() returns
-EPERM because it expects EDMA_ST_PAUSE, and dw_edma_device_issue_pending()
ignores it because chan->request is not EDMA_REQ_NONE, permanently
breaking the channel.

[ ... ]

> +static void dw_edma_free_chan_resources(struct dma_chan *dchan)
> +{
> +	struct dw_edma_chan *chan = dchan2dw_edma_chan(dchan);
> +
> +	dw_edma_device_terminate_all(dchan);
> +	dw_edma_device_synchronize(dchan);
> +
> +	scoped_guard(spinlock_irqsave, &chan->vc.lock)
> +		chan->configured = false;
> +
> +	vchan_free_chan_resources(&chan->vc);
>  }

[Severity: High]
If dw_edma_wait_termination() (called via dw_edma_device_synchronize)
times out, does the channel become permanently wedged in a BUSY state?

If the hardware fails to respond within 5 seconds, the timeout returns and
vchan_free_chan_resources() clears all descriptors.

If the hardware eventually finishes and raises a deferred DONE interrupt,
dw_edma_done_interrupt() is called. Because the descriptors were already
freed, vchan_next_desc() returns NULL.

Since the state reset logic (setting chan->request = EDMA_REQ_NONE and
chan->status = EDMA_ST_IDLE) is nested inside the if (vd) check in
dw_edma_done_interrupt(), it is completely skipped. This leaves the channel
stuck in EDMA_ST_BUSY and causes future dw_edma_alloc_chan_resources()
calls to fail with -EBUSY.

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