Re: [PATCH v2 17/19] dmaengine: dw-edma: Detect and recover a stalled eDMA engine

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] A race condition in the channel stall recovery mechanism can leave a channel permanently stalled because `queue_work()` is manually bypassed while `rec->active` is true during the worker's cleanup phase.
--

--- Patch [17]: [PATCH v2 17/19] dmaengine: dw-edma: Detect and recover a stalled eDMA engine ---
commit 9ca900fbc673ab7db9a251eb4f96dc2eddcf8833
Author: Koichiro Den <[email protected]>

dmaengine: dw-edma: Detect and recover a stalled eDMA engine

This commit introduces a mechanism to detect and recover stalled legacy
eDMA engines. It adds a 30ms stall window during the doorbell path and
queues a recovery work if a channel stops making progress.

> diff --git a/drivers/dma/dw-edma/dw-edma-core.c b/drivers/dma/dw-edma/dw-edma-core.c
> index 14cd0038dbd27..1b9ba34b25c80 100644
> --- a/drivers/dma/dw-edma/dw-edma-core.c
> +++ b/drivers/dma/dw-edma/dw-edma-core.c

[ ... ]

> @@ -183,11 +185,52 @@ static bool dw_edma_ll_advance(struct dw_edma_chan *chan, int idx, u32 *old_done
>  
>  	*old_done = chan->ll_done;
>  	chan->ll_done = idx;
> +	chan->ll_stall_valid = false;
>  	chan->ll_recovery_pending = false;
>  
>  	return true;
>  }
>  
> +/*
> + * Called with vc.lock held for a stopped channel with LL work pending. Queue
> + * direction recovery if repeated doorbells show no progress for the stall
> + * timeout.
> + */
> +static void dw_edma_ll_stall_check(struct dw_edma_chan *chan)
> +{

[ ... ]

> +	if (!time_after(jiffies, chan->ll_stall_since +
> +			msecs_to_jiffies(DW_EDMA_LL_STALL_TIMEOUT_MS)))
> +		return;
> +
> +	rec = &chan->dw->eng_recovery[chan->dir];
> +	chan->ll_recovery_pending = true;
> +	if (!READ_ONCE(rec->active))
> +		queue_work(chan->dw->wq, &rec->work);
> +}

[Severity: High]
Does this custom active check bypass queue_work()'s built-in deduplication,
potentially leaving a channel permanently stalled?

In dw_edma_engine_recovery_release(), rec->active is still true when it
calls dw_edma_core_ch_maybe_doorbell() for all channels:

drivers/dma/dw-edma/dw-edma-core.c:dw_edma_engine_recovery_release() {
    ...
    for (i = 0; i < cnt; i++) {
        chan = &dw->chan[off + i];
        ...
        dw_edma_core_ch_maybe_doorbell(chan);
    }

    WRITE_ONCE(rec->active, false);
}

If another channel also stalled during the recovery wait, this sequence
would trigger dw_edma_ll_stall_check() while rec->active is still true.
The manual check here skips queue_work(), and when the release function
finishes by setting rec->active to false, no new worker is queued.

Since queue_work() already safely handles WORK_STRUCT_PENDING, could
this check be safely removed to ensure we don't drop recovery work?

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