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

[sashiko-bot@kernel.org]

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

New issues:
- [High] `dmaengine_synchronize()` explicitly waits for and causes the channel to lose its configuration, breaking generic clients that expect to reuse the channel without calling `dmaengine_slave_config()` again.

Pre-existing issues:
- [High] State updates in `dw_edma_abort_interrupt` are performed outside the channel lock, creating a data race that can permanently stall the channel.
- [High] `dw_edma_device_pause` and `dw_edma_device_resume` read and modify channel state variables without acquiring `chan->vc.lock`.
--

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

dmaengine: dw-edma: Terminate all descriptors without callbacks

This commit changes the termination behavior of the dw-edma driver. It
moves all issued and submitted descriptors to the terminated list without
scheduling callbacks when termination completes. It also introduces
dw_edma_device_synchronize() to wait for channel termination.

> diff --git a/drivers/dma/dw-edma/dw-edma-core.c b/drivers/dma/dw-edma/dw-edma-core.c
> index 89a4c498a17b8..44ef5fbe3fd4a 100644
> --- a/drivers/dma/dw-edma/dw-edma-core.c
> +++ b/drivers/dma/dw-edma/dw-edma-core.c
[ ... ]
> @@ -306,20 +336,25 @@ static int dw_edma_device_resume(struct dma_chan *dchan)

[Severity: High]
This is a pre-existing issue, but does dw_edma_device_pause() and
dw_edma_device_resume() have a data race?

In drivers/dma/dw-edma/dw-edma-core.c:dw_edma_device_pause():

	if (!chan->configured)
		err = -EPERM;
	else if (chan->status != EDMA_ST_BUSY)
		err = -EPERM;
	else if (chan->request != EDMA_REQ_NONE)
		err = -EPERM;
	else
		chan->request = EDMA_REQ_PAUSE;

These functions read and modify chan->status and chan->request without
acquiring chan->vc.lock. If device_pause() runs concurrently with an
interrupt completing a transfer, it might overwrite chan->request to
EDMA_REQ_PAUSE on an already idle channel, preventing new transfers from
starting.

Similarly, device_resume() calls dw_edma_start_transfer() which manipulates
virt-dma lists without synchronization.

>  static int dw_edma_device_terminate_all(struct dma_chan *dchan)
>  {
[ ... ]
> @@ -698,7 +733,9 @@ static void dw_edma_abort_interrupt(struct dw_edma_chan *chan)
>  
>  	spin_lock_irqsave(&chan->vc.lock, flags);
>  	vd = vchan_next_desc(&chan->vc);
> -	if (vd) {
> +	if (vd && chan->request == EDMA_REQ_STOP) {
> +		dw_edma_terminate_all_descs(chan);
> +	} else if (vd) {
>  		dw_hdma_set_callback_result(vd, DMA_TRANS_ABORTED);
>  		list_del(&vd->node);
>  		vchan_cookie_complete(vd);

[Severity: High]
This is a pre-existing issue, but can this sequence permanently stall the
channel?

The end of dw_edma_abort_interrupt() looks like this:

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

Because chan->request and chan->status are updated after releasing
chan->vc.lock, a concurrent client completion callback could queue a new
descriptor and call dma_async_issue_pending().

The issue_pending() call would acquire the lock, observe the stale
chan->status (e.g. EDMA_ST_BUSY), and fail to start the hardware. The
interrupt handler then unconditionally writes EDMA_ST_IDLE, leaving the
hardware idle while the descriptor is stuck in the desc_issued list.

[ ... ]
> @@ -856,21 +893,48 @@ static int dw_edma_alloc_chan_resources(struct dma_chan *dchan)
>  	return 0;
>  }
>  
> -static void dw_edma_free_chan_resources(struct dma_chan *dchan)
> +static void dw_edma_wait_termination(struct dma_chan *dchan)
>  {
> +	struct dw_edma_chan *chan = dchan2dw_edma_chan(dchan);
>  	unsigned long timeout = jiffies + msecs_to_jiffies(5000);
> -	int ret;
> +	unsigned long flags;
> +	bool configured = true;
>  
> +	/*
> +	 * dw_edma_device_terminate_all() may defer cleanup to a later interrupt
> +	 * while the channel is still running. Retry until the channel is
> +	 * deconfigured, which means termination is complete.
> +	 */
>  	while (time_before(jiffies, timeout)) {
> -		ret = dw_edma_device_terminate_all(dchan);
> -		if (!ret)
> -			break;
> +		dw_edma_device_terminate_all(dchan);
>  
> -		if (time_after_eq(jiffies, timeout))
> +		spin_lock_irqsave(&chan->vc.lock, flags);
> +		configured = chan->configured;
> +		spin_unlock_irqrestore(&chan->vc.lock, flags);
> +		if (!configured)
>  			return;
>  

[Severity: High]
Will this break generic dmaengine clients?

The loop in dw_edma_wait_termination() forces the channel to become
unconfigured by polling dw_edma_device_terminate_all() until
chan->configured is set to false.

However, dw_edma_wait_termination() is called by dw_edma_device_synchronize().
Generic dmaengine clients expect the channel configuration to persist across
dmaengine_terminate_sync(). Because this explicitly clears the configuration,
subsequent dmaengine_prep_* calls from generic clients will return NULL
and fail.

> -		cpu_relax();
> +		usleep_range(1000, 2000);
>  	}

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