Re: [PATCH 4/7] dmaengine: dw-edma: Serialize channel state checks

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [Critical] Resuming a channel paused on the final chunk of a descriptor causes a permanent deadlock due to bypassed descriptor completion.
- [High] Race condition in `dw_edma_abort_interrupt` updating state outside of `vc.lock`
- [High] `dw_edma_device_terminate_all()` fails to reset `chan->request`, permanently wedging the channel on false-busy or timeout.
--

--- Patch [4]: [PATCH 4/7] dmaengine: dw-edma: Serialize channel state checks ---
commit d1992615b101e2e305e248b8535efc7b3e2c9e5f
Author: Koichiro Den <den@valinux.co.jp>
dmaengine: dw-edma: Serialize channel state checks

pause() and resume() read and update channel state without holding vc.lock,
while the interrupt handlers update the same state under it. Take the same
lock around those state checks so that request, status, and configured stay
consistent.

> diff --git a/drivers/dma/dw-edma/dw-edma-core.c b/drivers/dma/dw-edma/dw-edma-core.c
> index 1b493c104a5b1..5664421c6f15b 100644
> --- a/drivers/dma/dw-edma/dw-edma-core.c
> +++ b/drivers/dma/dw-edma/dw-edma-core.c
[ ... ]
> @@ -318,8 +321,10 @@ static int dw_edma_device_pause(struct dma_chan *dchan)
>  static int dw_edma_device_resume(struct dma_chan *dchan)
>  {
>  	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) {
>  		err = -EPERM;
>  	} else if (chan->status != EDMA_ST_PAUSE) {
> @@ -330,6 +335,7 @@ static int dw_edma_device_resume(struct dma_chan *dchan)
>  		chan->status = EDMA_ST_BUSY;
>  		dw_edma_start_transfer(chan);
>  	}

[Severity: Critical]
This is a pre-existing issue, but I wanted to point out a potential deadlock
when resuming a paused channel.

If a channel is paused exactly as the final chunk of a descriptor finishes,
the interrupt handler dw_edma_done_interrupt() bypasses the normal chunk
depletion check:

drivers/dma/dw-edma/dw-edma-core.c:dw_edma_done_interrupt() {
    ...
		case EDMA_REQ_PAUSE:
			chan->request = EDMA_REQ_NONE;
			chan->status = EDMA_ST_PAUSE;
			break;
    ...
}

This leaves the depleted descriptor in the issued queue. Later, when resume is
called, dw_edma_start_transfer() attempts to start this empty descriptor,
finds no chunks, and returns 0.

However, dw_edma_device_resume() ignores this return value and unconditionally
sets chan->status = EDMA_ST_BUSY. Would this result in the hardware never
starting and the channel remaining irreversibly stuck in the EDMA_ST_BUSY
state?

> +	spin_unlock_irqrestore(&chan->vc.lock, flags);
>  
>  	return err;
>  }

While looking at the state locking changes in this patch, I noticed a couple
of other pre-existing issues in this file related to state transitions:

[Severity: High]
This isn't a bug introduced by this patch, but does dw_edma_abort_interrupt()
have a similar race condition to the ones being fixed here?

drivers/dma/dw-edma/dw-edma-core.c:dw_edma_abort_interrupt() {
    ...
	spin_lock_irqsave(&chan->vc.lock, flags);
	vd = vchan_next_desc(&chan->vc);
	if (vd) {
		dw_hdma_set_callback_result(vd, DMA_TRANS_ABORTED);
		list_del(&vd->node);
		vchan_cookie_complete(vd);
	}
	spin_unlock_irqrestore(&chan->vc.lock, flags);
	chan->request = EDMA_REQ_NONE;
	chan->status = EDMA_ST_IDLE;
}

The lock is dropped before updating chan->request and chan->status.

If dw_edma_device_issue_pending() executes on another CPU in this unprotected
window, it could acquire vc.lock, observe a stale EDMA_ST_BUSY state, and
correctly skip starting the hardware for newly queued descriptors. Once
dw_edma_abort_interrupt() resumes and sets the state to EDMA_ST_IDLE, would
the engine remain stopped, leaving the new descriptors stranded forever?

[Severity: High]
This is also a pre-existing issue, but does dw_edma_device_terminate_all()
need to clear chan->request in its error paths?

drivers/dma/dw-edma/dw-edma-core.c:dw_edma_device_terminate_all() {
    ...
	} else if (dw_edma_core_ch_status(chan) == DMA_COMPLETE) {
		/*
		 * The channel is in a false BUSY state, probably didn't
		 * receive or lost an interrupt
		 */
		dw_edma_terminate_all_descs(chan);
		chan->status = EDMA_ST_IDLE;
		chan->configured = false;
	}
    ...
}

If a channel is paused (chan->request == EDMA_REQ_PAUSE) and the transfer
finished in hardware but the interrupt was lost, it seems we take this branch
and set chan->status = EDMA_ST_IDLE without clearing chan->request.

Since future calls to dw_edma_device_issue_pending() require chan->request ==
EDMA_REQ_NONE, would this stale request state persist and indefinitely wedge
the channel?

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