Re: [PATCH v6 2/5] dmaengine: dw-edma: Reclaim LL entries from hardware progress

[Koichiro Den]

On Fri, Oct 30, 2026 at 11:54:50AM -0500, Frank Li wrote:
> On Sat, Oct 31, 2026 at 12:07:39AM +0900, Koichiro Den wrote:
> > Once requests are appended while the channel runs, a DONE interrupt no
> > longer means that all published entries have completed.
> >
> > Use DMA_LLP to advance ll_done and complete descriptors in issue order.
> > Reclaim entries as they complete so descriptors larger than the ring
> > can be submitted in batches.
> >
> > DMA_LLP may advance before payload completion, so keep the running
> > completion boundary one entry behind it. Use DMA_LLP itself once the
> > channel is stopped with a zero transfer size. Hold vc.lock to serialize
> > progress with ring resets and doorbells.
> >
> > Refill reclaimed entries and restart a stopped channel if work remains.
> > Defer STOP and PAUSE completion until the channel stops.
> >
> > Signed-off-by: Koichiro Den <den@xxxxxxxxxxxxx>
> > ---
> > Reworked v5 patch 13:
> > https://lore.kernel.org/r/20260812155721.2807506-14-den@xxxxxxxxxxxxx/
> >
> > Changes since v5:
> >   - Read the registers in the progress handler instead of retaining an
> >     IRQ-paired LLP snapshot.
> >   - Allow index 0 to wrap. Reject running samples that would reclaim all
> >     pending entries, so an unchanged start pointer cannot look like a
> >     full lap.
> >
> >  drivers/dma/dw-edma/dw-edma-core.c | 140 ++++++++++++++++++++++++++---
> >  1 file changed, 126 insertions(+), 14 deletions(-)
> >
> > diff --git a/drivers/dma/dw-edma/dw-edma-core.c b/drivers/dma/dw-edma/dw-edma-core.c
> > index b08bb31657c9..74613c95ae11 100644
> > --- a/drivers/dma/dw-edma/dw-edma-core.c
> > +++ b/drivers/dma/dw-edma/dw-edma-core.c
> > @@ -53,13 +53,6 @@ dw_edma_alloc_desc(struct dw_edma_chan *chan, size_t nburst)
> >  {
> >  	struct dw_edma_desc *desc;
> >
> > -	/*
> > -	 * For now, a descriptor that does not fit would stall the channel
> > -	 * forever: reject it up front.
> > -	 */
> > -	if (!chan->non_ll && nburst > chan->ll_max - 1)
> > -		return NULL;
> > -
> >  	desc = kzalloc_flex(*desc, burst, nburst, GFP_NOWAIT);
> >  	if (unlikely(!desc))
> >  		return NULL;
> > @@ -146,6 +139,76 @@ static bool dw_edma_ll_pending(struct dw_edma_chan *chan)
> >  	return chan->ll_head != chan->ll_done;
> >  }
> >
> > +static bool dw_edma_ll_stopped(struct dw_edma_chan *chan)
> > +{
> > +	return dw_edma_core_ch_status(chan) == DMA_COMPLETE &&
> > +	       !dw_edma_core_ch_transfer_size(chan);
> > +}
> > +
> > +/* Return the first entry we cannot reclaim yet. */
> > +static int dw_edma_ll_boundary(struct dw_edma_chan *chan, int idx,
> > +			       bool stopped)
> > +{
> > +	if (idx < 0 || (u32)idx > chan->ll_max)
> > +		return -EINVAL;
> > +
> > +	/* 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 ? idx - 1 : chan->ll_max - 1;
> 
> use mod
> 
> 	(idx + (chan->ll_max - 1) - 1) % chan->ll_max - 1;

I believe what you meant was:

  (idx + chan->ll_max - 1) % chan->ll_max

and if yes, I agree. I'll update it in the next revision.

> 
> 
> > +}
> > +
> > +/* Must be called with vc.lock held. */
> > +static void dw_edma_ll_consume(struct dw_edma_chan *chan, int idx,
> > +			       bool stopped)
> > +{
> > +	struct virt_dma_desc *vd, *_vd;
> > +	u32 done, used;
> 
> lockdep_assert_held()

Will add it.

> 
> > +
> > +	idx = dw_edma_ll_boundary(chan, idx, stopped);
> > +	if (idx < 0)
> > +		return;
> > +
> > +	done = dw_edma_core_get_ll_dist(chan, chan->ll_done, idx);
> > +	used = dw_edma_core_get_used_num(chan);
> > +
> > +	/*
> > +	 * LLP may still point to the first pending entry. Subtracting one
> > +	 * can wrap the index and falsely mark all pending entries complete.
> > +	 * Reclaim them all only after the channel has stopped.
> > +	 */
> 
> Do you means !done for this comments?

No, it was about the last term (ie. "!stopped && done == used") only. In
retrospect I should've separated the if blocks like the following. WDYT?

    if (!done || done > used)
            return;

    /*
     * LLP may still point to the first pending entry. Subtracting one
     * can wrap the index and falsely mark all pending entries complete.
     * Reclaim them all only after the channel has stopped.
     */
    if (!stopped && done == used)
            return;

> 
> > +	if (!done || done > used || (!stopped && done == used))
> > +		return;
> > +
> > +	chan->ll_done = idx;
> > +
> > +	/* Match reclaimed entries to descriptors in issue order. */
> > +	list_for_each_entry_safe(vd, _vd, &chan->vc.desc_issued, node) {
> > +		struct dw_edma_desc *desc = vd2dw_edma_desc(vd);
> > +		u32 consumed = min_t(u32, done,
> > +				     desc->start_burst - desc->done_burst);
> > +
> > +		if (!consumed)
> > +			break;
> > +
> > +		desc->done_burst += consumed;
> > +		done -= consumed;
> > +		if (desc->done_burst != desc->nburst)
> > +			break;
> > +
> > +		dw_hdma_set_callback_result(vd, DMA_TRANS_NOERROR);
> > +		list_del(&vd->node);
> > +		vchan_cookie_complete(vd);
> > +	}
> > +
> > +	WARN_ON_ONCE(done);
> > +}
> > +
> >  static void dw_edma_core_ll_start(struct dw_edma_desc *desc)
> >  {
> >  	struct dw_edma_chan *chan = desc->chan;
> > @@ -174,7 +237,6 @@ static void dw_edma_core_ll_start(struct dw_edma_desc *desc)
> >  		}
> >  	}
> >
> > -	desc->done_burst = desc->start_burst;
> >  	desc->start_burst = i;
> >  }
> >
> > @@ -283,6 +345,45 @@ static void dw_edma_core_ch_maybe_doorbell(struct dw_edma_chan *chan)
> >  	dw_edma_core_ch_doorbell(chan);
> >  }
> >
> > +/*
> > + * Must be called with vc.lock held.
> > + *
> > + * HDMA does not update LLP continuously. Only use it to track running
> > + * transfers when handling an interrupt.
> > + */
> > +static void dw_edma_ll_advance(struct dw_edma_chan *chan, bool event)
> > +{
> > +	bool stopped = dw_edma_ll_stopped(chan);
> > +
> > +	if (stopped || event)
> > +		dw_edma_ll_consume(chan, dw_edma_core_ll_cur_idx(chan),
> > +				   stopped);
> > +
> > +	if (chan->request != EDMA_REQ_NONE) {
> > +		/* Wait for the channel to stop before completing STOP or PAUSE. */
> > +		if (!stopped)
> > +			return;
> > +
> > +		if (chan->request == EDMA_REQ_STOP) {
> > +			dw_edma_finish_termination(chan);
> > +		} else if (chan->request == EDMA_REQ_PAUSE) {
> > +			dw_edma_set_request(chan, EDMA_REQ_NONE);
> > +			chan->status = EDMA_ST_PAUSE;
> > +		}
> > +		return;
> > +	}
> > +
> > +	dw_edma_start_transfer(chan);
> > +	if (!dw_edma_ll_pending(chan)) {
> > +		chan->status = EDMA_ST_IDLE;
> > +		return;
> > +	}
> > +
> > +	chan->status = EDMA_ST_BUSY;
> > +	if (stopped)
> > +		dw_edma_core_ch_doorbell(chan);
> > +}
> > +
> >  static void dw_edma_device_caps(struct dma_chan *dchan,
> >  				struct dma_slave_caps *caps)
> >  {
> > @@ -749,7 +850,7 @@ dw_edma_device_prep_interleaved_dma(struct dma_chan *dchan,
> >  	return dw_edma_device_transfer(&xfer, dw_edma_device_get_config(dchan, NULL));
> >  }
> >
> > -/* Must be called with vc.lock held. */
> > +/* Non-LL completion; vc.lock must be held. */
        ^^^^^^(a)
> >  static void dw_edma_done_interrupt_locked(struct dw_edma_chan *chan)
> >  {
> >  	struct dw_edma_desc *desc;
> > @@ -772,8 +873,6 @@ static void dw_edma_done_interrupt_locked(struct dw_edma_chan *chan)
> >  			dw_hdma_set_callback_result(vd, DMA_TRANS_NOERROR);
> >  			list_del(&vd->node);
> >  			vchan_cookie_complete(vd);
> 
> why not use dw_edma_ll_consume(), let vchan_cookie_complete() at one
> place only

With this patch, the existing dw_edma_done_interrupt_locked() now serves the
non-LL case only (see (a) above), so we cannot simply use dw_edma_ll_consume()
here, as it works on ring indices.

That said, avoiding scattering vchan_cookie_complete() around makes sense.
Would it be a middle ground to add a new helper like:

   static void dw_edma_complete_vdesc(struct virt_dma_desc *vd,
                                   enum dmaengine_tx_result result)
   {
        dw_hdma_set_callback_result(vd, result);
        list_del(&vd->node);
        vchan_cookie_complete(vd);
   }

and replace the remaining open-coded vchan_cookie_complete() call sites with it?

Thanks for the review!

Best regards,
Koichiro Den

> 
> Frank
> 
> > -			if (!chan->non_ll)
> > -				chan->ll_done = chan->ll_head;
> >  		}
> >
> >  		if (chan->request == EDMA_REQ_PAUSE) {
> > @@ -796,7 +895,6 @@ static void dw_edma_done_interrupt_locked(struct dw_edma_chan *chan)
> >  	default:
> >  		break;
> >  	}
> > -	dw_edma_core_ch_maybe_doorbell(chan);
> >  }
> >
> >  static void dw_edma_done_interrupt(struct dw_edma_chan *chan)
> > @@ -808,6 +906,16 @@ static void dw_edma_done_interrupt(struct dw_edma_chan *chan)
> >  	spin_unlock_irqrestore(&chan->vc.lock, flags);
> >  }
> >
> > +static void dw_edma_ll_interrupt(struct dw_edma_chan *chan)
> > +{
> > +	guard(spinlock_irqsave)(&chan->vc.lock);
> > +
> > +	if (chan->status == EDMA_ST_PAUSE)
> > +		return;
> > +
> > +	dw_edma_ll_advance(chan, true);
> > +}
> > +
> >  static void dw_edma_abort_interrupt(struct dw_edma_chan *chan)
> >  {
> >  	struct virt_dma_desc *vd;
> > @@ -838,8 +946,12 @@ static void dw_edma_irq_work(struct work_struct *work)
> >  	do {
> >  		events = atomic_xchg(&chan->irq_pending, 0);
> >
> > -		if (events & DW_EDMA_DEFERRED_DONE)
> > -			dw_edma_done_interrupt(chan);
> > +		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);
> >  	} while (atomic_read(&chan->irq_pending));
> > --
> > 2.54.0
> >