Re: [PATCH v7 6/7] dmaengine: dw-edma: Dynamically append requests while running

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] dmaengine: dw-edma: premature completion in dw_edma_ll_advance() on append
- [Medium] dmaengine: dw-edma: stranded or aborted queued descriptors on LL reset
--

commit 09d38b6dc815a996191d342c3fb4a8c29aef4401
Author: Frank Li <Frank.Li@nxp.com>

dmaengine: dw-edma: Dynamically append requests while running

This commit updates dw_edma_start_transfer() and
dw_edma_device_issue_pending() to append descriptors into free LL ring slots
while a channel is already busy, and updates dw_edma_core_reset_ll() and
dw_edma_device_resume() to handle multiple pending descriptors in the LL
ring.

> diff --git a/drivers/dma/dw-edma/dw-edma-core.c b/drivers/dma/dw-edma/dw-edma-core.c
> index e2f2a535d5ad1..69b5488282de3 100644
> --- a/drivers/dma/dw-edma/dw-edma-core.c
> +++ b/drivers/dma/dw-edma/dw-edma-core.c
> @@ -111,8 +111,22 @@ static void dw_edma_complete_vdesc(struct virt_dma_desc *vd,
>  
>  static void dw_edma_core_reset_ll(struct dw_edma_chan *chan)
>  {
> +	struct virt_dma_desc *vd, *tmp;
>  	u32 i;
>  
> +	/*
> +	 * We cannot tell which pending entries completed before the reset.
> +	 * Abort their descriptors to avoid repeating a transfer.
> +	 */
> +	list_for_each_entry_safe(vd, tmp, &chan->vc.desc_issued, node) {
> +		struct dw_edma_desc *desc = vd2dw_edma_desc(vd);
> +
> +		if (desc->start_burst == desc->done_burst)
> +			continue;
> +
> +		dw_edma_complete_vdesc(vd, DMA_TRANS_ABORTED);
> +	}

[Severity: Medium]
What happens to unstarted descriptors that remain in chan->vc.desc_issued
when a hardware abort occurs?

When dw_edma_device_issue_pending() moves submitted descriptors to
chan->vc.desc_issued and dw_edma_start_transfer() fills the LL ring, any
trailing descriptors stay in chan->vc.desc_issued with start_burst == 0 and
done_burst == 0.

If an abort occurs on an earlier in-flight descriptor,
dw_edma_core_reset_ll() skips those unstarted descriptors because
desc->start_burst == desc->done_burst, leaving them in chan->vc.desc_issued.
However, dw_edma_abort_interrupt() then transitions the channel straight to
EDMA_ST_IDLE without starting them:

drivers/dma/dw-edma/dw-edma-core.c:dw_edma_abort_interrupt() {
	...
	if (!chan->non_ll)
		dw_edma_core_reset_ll(chan);
	dw_edma_set_request(chan, EDMA_REQ_NONE);
	chan->status = EDMA_ST_IDLE;
	spin_unlock_irqrestore(&chan->vc.lock, flags);
}

Won't those remaining descriptors stay stranded in chan->vc.desc_issued on an
idle channel unless the client later issues another transfer?

Also, if an earlier descriptor in the ring raised DW_EDMA_DEFERRED_DONE
before a subsequent entry raised DW_EDMA_DEFERRED_ABORT, dw_edma_irq_work()
processes DW_EDMA_DEFERRED_DONE first:

drivers/dma/dw-edma/dw-edma-core.c:dw_edma_irq_work() {
		if (events & DW_EDMA_DEFERRED_DONE) {
			if (chan->non_ll)
				dw_edma_done_interrupt(chan);
			else
				dw_edma_ll_interrupt(chan);
		}
		if (events & DW_EDMA_DEFERRED_ABORT)
			dw_edma_abort_interrupt(chan);
}

In that case, dw_edma_ll_interrupt() -> dw_edma_ll_advance() reclaims the
completed slots and calls dw_edma_start_transfer(), publishing the waiting
unstarted descriptors into the LL ring (setting start_burst > 0), only for
dw_edma_abort_interrupt() -> dw_edma_core_reset_ll() to immediately abort
them in the same work pass.

[ ... ]

> @@ -610,11 +638,16 @@ static void dw_edma_device_issue_pending(struct dma_chan *dchan)
>  
>  	spin_lock_irqsave(&chan->vc.lock, flags);
>  	if (chan->configured && vchan_issue_pending(&chan->vc) &&
> -	    chan->request == EDMA_REQ_NONE &&
> -	    chan->status == EDMA_ST_IDLE) {
> -		chan->status = EDMA_ST_BUSY;
> -		dw_edma_start_transfer(chan);
> -		dw_edma_core_ch_maybe_doorbell(chan);
> +	    chan->request == EDMA_REQ_NONE) {
> +		if (chan->status == EDMA_ST_IDLE) {
> +			chan->status = EDMA_ST_BUSY;
> +			dw_edma_start_transfer(chan);
> +			dw_edma_core_ch_maybe_doorbell(chan);
> +		} else if (!chan->non_ll &&
> +			   chan->status == EDMA_ST_BUSY) {
> +			dw_edma_start_transfer(chan);
> +			dw_edma_ll_advance(chan, false);
> +		}
>  	}
>  	spin_unlock_irqrestore(&chan->vc.lock, flags);
>  }

[Severity: High]
Could calling dw_edma_ll_advance(chan, false) immediately after appending new
LL entries here cause an in-flight descriptor to be completed prematurely?

When a first dw_edma_device_issue_pending() call transitions the channel from
EDMA_ST_IDLE to EDMA_ST_BUSY and rings the doorbell, the hardware channel
status can briefly continue to report DMA_COMPLETE with transfer_size == 0
before the engine begins fetching the entry at ll_done.

If a second dw_edma_device_issue_pending() call runs during that window,
dw_edma_start_transfer() appends a second entry (making used >= 2) and calls
dw_edma_ll_advance(chan, false):

drivers/dma/dw-edma/dw-edma-core.c:dw_edma_ll_advance() {
	...
	stopped = dw_edma_ll_stopped(chan);
	/*
	 * HDMA updates LLP at watermark events while running. Use running
	 * samples only when processing an LL interrupt.
	 */
	if (stopped || event)
		dw_edma_ll_consume(chan, dw_edma_core_ll_cur_idx(chan),
				   stopped);
}

Because dw_edma_ll_stopped(chan) and dw_edma_core_ll_cur_idx(chan) are
separate MMIO reads, dw_edma_ll_stopped(chan) can observe the stale
post-doorbell stopped state (stopped = true) just before the engine starts
processing the first entry at ll_done and advances DMA_LLP to ll_done + 1
while the payload transfer is still in flight.

Passing idx = ll_done + 1 and stopped = true to dw_edma_ll_consume() then
uses the stopped boundary (idx instead of idx - 1) and bypasses the
(!stopped && done == used) check:

drivers/dma/dw-edma/dw-edma-core.c:dw_edma_ll_boundary() {
	...
	/* After a stop, LLP points past the completed entries. */
	if (stopped)
		return idx == chan->ll_max ? 0 : idx;

	/*
	 * DMA_LLP can advance before payload completion. Keep the boundary
	 * one entry behind it, wrapping at the start of the ring.
	 */
	return (idx + chan->ll_max - 1) % chan->ll_max;
}

Wouldn't that advance chan->ll_done past the first entry and complete the
first descriptor with DMA_TRANS_NOERROR while hardware is still transferring
its payload, as well as ring a duplicate doorbell at the end of
dw_edma_ll_advance()?

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