Re: [PATCH v2 14/19] dmaengine: dw-edma: Reset LL state after terminate and abort

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

New issues:
- [High] Deferring `EDMA_REQ_STOP` for Legacy eDMA when `ll_pending > 0` causes a permanent channel stall if the hardware has already stopped without a pending doorbell.

Pre-existing issues:
- [High] Virtual descriptors are permanently leaked during channel termination because `dw_edma_terminate_vdesc` removes them from the tracking lists without ever freeing their backing memory.
--

--- Patch [14]: [PATCH v2 14/19] dmaengine: dw-edma: Reset LL state after terminate and abort ---
commit 88e57ef5a9d3ec5120e32fad81665662de2e275a
Author: Koichiro Den <[email protected]>

dmaengine: dw-edma: Reset LL state after terminate and abort

Termination and abort can leave published entries in the circular ring.
Reset such a ring before reuse, but leave an already clean ring alone.
This avoids clearing the whole ring after every synchronous transfer.

> diff --git a/drivers/dma/dw-edma/dw-edma-core.c b/drivers/dma/dw-edma/dw-edma-core.c
> index 963abecdc59ed..9305137f85f67 100644
> --- a/drivers/dma/dw-edma/dw-edma-core.c
> +++ b/drivers/dma/dw-edma/dw-edma-core.c
[ ... ]
> @@ -616,36 +617,64 @@ static int dw_edma_device_resume(struct dma_chan *dchan)
>  	return err;
>  }
>  
> +/* Must be called with vc.lock held. */
> +static bool dw_edma_may_be_active(struct dw_edma_chan *chan)
> +{
> +	if (chan->non_ll)
> +		return dw_edma_core_ch_status(chan) == DMA_IN_PROGRESS;
> +
> +	if (!dw_edma_ll_pending(chan))
> +		return false;
> +
> +	/*
> +	 * Legacy eDMA can report STOPPED while a doorbell-triggered start is
> +	 * still pending. Native HDMA has a dedicated STOP event.
> +	 */
> +	return !dw_edma_ll_has_hdma_stop_event(chan) ||
> +	       dw_edma_core_ch_status(chan) == DMA_IN_PROGRESS;
> +}
> +
> +/* Must be called with vc.lock held after the channel has stopped. */
> +static void dw_edma_finish_termination(struct dw_edma_chan *chan)
> +{
> +	dw_edma_terminate_all_descs(chan);

[Severity: High]
This is a pre-existing issue, but does dw_edma_terminate_all_descs() leak
virtual descriptors during channel termination?

Looking at the termination path, dw_edma_terminate_all_descs() uses
dw_edma_terminate_vdesc_list() to move descriptors to the desc_terminated list:

drivers/dma/dw-edma/dw-edma-core.c:dw_edma_terminate_all_descs() {
    dw_edma_terminate_vdesc_list(&chan->vc.desc_issued);
    dw_edma_terminate_vdesc_list(&chan->vc.desc_submitted);
}

However, there appears to be no call to vchan_dma_desc_free_list() within the
termination path.

Are these descriptors permanently leaked until the entire channel is released
via free_chan_resources(), potentially exhausting system memory if triggered
repeatedly by userspace?

> +
> +	/* Preserve a clean ring; resync only if entries remain published. */
> +	if (!chan->non_ll && dw_edma_ll_pending(chan))
> +		dw_edma_core_reset_ll(chan);
> +
> +	chan->request = EDMA_REQ_NONE;
> +	chan->status = EDMA_ST_IDLE;
> +}
[ ... ]
> @@ -965,17 +994,30 @@ static void dw_edma_done_interrupt(struct dw_edma_chan *chan)
>  	bool active;
>  
>  	spin_lock_irqsave(&chan->vc.lock, flags);
> -	if (chan->status == EDMA_ST_PAUSE) {
> -		spin_unlock_irqrestore(&chan->vc.lock, flags);
> -		return;
> -	}
> +	if (chan->status == EDMA_ST_PAUSE)
> +		goto out;
> +
> +	/*
> +	 * A stopped event sampled after the request can complete it directly.
> +	 * If it was sampled just before the request, complete the request only
> +	 * when its boundary drains all published entries. A running event only
> +	 * advances LL progress.
> +	 */
> +	if (chan->request != EDMA_REQ_NONE && !chan->non_ll &&
> +	    (!chan->ll_irq_stopped || !chan->ll_irq_requested)) {
> +		bool stopped = chan->ll_irq_stopped;
>  
> -	if (!chan->non_ll)
>  		dw_edma_ll_consume_progress(chan);
> +		if (!stopped || dw_edma_ll_pending(chan))
> +			goto out;

[Severity: High]
Can this cause a permanent channel stall for Legacy eDMA when descriptors
are pending?

If dw_edma_device_terminate_all() is called while ll_pending > 0, it defers
termination by setting EDMA_REQ_STOP and waits for a DONE IRQ.

If the hardware has already stopped (e.g., reaching the end of the previous
chain) and the doorbell was bypassed, the DONE IRQ handler processes completed
elements but sees ll_pending > 0. It takes the goto out path:

drivers/dma/dw-edma/dw-edma-core.c:dw_edma_done_interrupt() {
    if (!stopped || dw_edma_ll_pending(chan))
        goto out;
    ...
out:
    dw_edma_core_ch_maybe_doorbell(chan);
}

However, dw_edma_core_ch_maybe_doorbell() will refuse to ring the doorbell
because EDMA_REQ_STOP is set:

drivers/dma/dw-edma/dw-edma-core.c:dw_edma_core_ch_maybe_doorbell() {
    if (... || chan->request != EDMA_REQ_NONE || ...)
        return;
    ...
}

With the hardware stopped and no doorbell rung, will any further IRQs arrive,
or does this permanently stall the channel?

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