Re: [PATCH v3 19/24] dmaengine: dw-edma: Drain LL entries for STOP and PAUSE

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] The delayed stop recheck is not armed when draining a stopped channel, leading to an infinite hang in dw_edma_wait_termination().
--

--- Patch [19]: [PATCH v3 19/24] dmaengine: dw-edma: Drain LL entries for STOP and PAUSE ---
commit 2379a4182966c17e3764b5d21a024f5b0063f553
Author: Koichiro Den <[email protected]>

dmaengine: dw-edma: Drain LL entries for STOP and PAUSE

This commit modifies the handling of STOP and PAUSE requests to drain
published LL entries instead of discarding ring state immediately. It waits
for hardware to consume them and uses a delayed recheck to catch a later
stop.

> diff --git a/drivers/dma/dw-edma/dw-edma-core.c b/drivers/dma/dw-edma/dw-edma-core.c
> index 0c30c0f51fea2..bbd417e98a9d7 100644
> --- a/drivers/dma/dw-edma/dw-edma-core.c
> +++ b/drivers/dma/dw-edma/dw-edma-core.c
[ ... ]
> @@ -883,35 +916,52 @@ static int dw_edma_device_resume(struct dma_chan *dchan)
[ ... ]
>  static int dw_edma_device_terminate_all(struct dma_chan *dchan)
>  {
>  	struct dw_edma_chan *chan = dchan2dw_edma_chan(dchan);
> -	int err = 0;
>  
>  	guard(spinlock_irqsave)(&chan->vc.lock);
>  
>  	if (!chan->configured) {
>  		dw_edma_terminate_all_descs(chan);
> +		chan->request = EDMA_REQ_NONE;
>  	} else if (chan->status == EDMA_ST_PAUSE) {
> +		/* A paused channel has already stopped. */
>  		dw_edma_finish_termination(chan);
> -	} else if (chan->status == EDMA_ST_IDLE) {
> -		dw_edma_finish_termination(chan);
> -	} else if (dw_edma_core_ch_status(chan) == DMA_COMPLETE) {
> +	} else if (dw_edma_may_be_active(chan)) {
>  		/*
> -		 * The channel is in a false BUSY state, probably didn't
> -		 * receive or lost an interrupt
> +		 * Keep published entries intact until an IRQ sample confirms
> +		 * that the channel stopped. An outstanding EDMA_REQ_PAUSE is
> +		 * replaced by EDMA_REQ_STOP.
>  		 */
> -		dw_edma_finish_termination(chan);
> -	} else if (chan->request > EDMA_REQ_PAUSE) {
> -		err = -EPERM;
> -	} else {
>  		dw_edma_set_request(chan, EDMA_REQ_STOP);
> +		dw_edma_core_ch_maybe_doorbell_or_recheck(chan);

[Severity: High]
Does dw_edma_core_ch_maybe_doorbell() unconditionally return false after
kicking a stopped channel?

If dw_edma_core_ch_maybe_doorbell() returns false, it looks like
dw_edma_core_ch_maybe_doorbell_or_recheck() will fail to arm the delayed
recheck timer:

static void dw_edma_core_ch_maybe_doorbell_or_recheck(...)
{
    if (dw_edma_core_ch_maybe_doorbell(chan) &&
        !dw_edma_ll_has_hdma_stop_event(chan))
        dw_edma_ll_recheck_schedule(chan);
}

Without an interrupt and with no recheck timer armed, could chan->request
remain EDMA_REQ_STOP indefinitely?

>  	}
[ ... ]
> @@ -1591,18 +1639,21 @@ static void dw_edma_wait_termination(struct dma_chan *dchan)
>  	 * A STOP may be deferred to a later interrupt while the channel is still
>  	 * running. Wait until that handler completes the termination.
>  	 */
> -	while (time_before(jiffies, timeout)) {
> +	for (;;) {
>  		scoped_guard(spinlock_irqsave, &chan->vc.lock)
>  			stopping = chan->request == EDMA_REQ_STOP;
>  
>  		if (!stopping)
>  			return;
>  
> +		if (time_after_eq(jiffies, timeout)) {
> +			dev_warn(chan->dw->chip->dev,
> +				 "timeout waiting for channel termination; still waiting\n");
> +			timeout = jiffies + msecs_to_jiffies(5000);
> +		}
> +
>  		fsleep(1000);
>  	}

[Severity: High]
If the channel remains stuck in EDMA_REQ_STOP due to the missing recheck
timer, will a task calling dmaengine_terminate_sync() or
dmaengine_synchronize() hang indefinitely in this loop?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=19