Re: [PATCH 04/17] dmaengine: dw-edma: Clean up vchan descriptors on termination

[Koichiro Den]

On Mon, Jun 15, 2026 at 01:43:58PM -0500, Frank Li wrote:
> On Tue, Jun 16, 2026 at 12:40:58AM +0900, Koichiro Den wrote:
> > dw-edma resets channel state from terminate_all() paths, but pending
> > virt-dma descriptors can remain on the submitted and issued lists. A
> > later issue_pending() may then restart work that the client already
> > terminated, possibly into buffers that were already reused. Descriptors
> > that are never restarted leak instead.
> >
> > Move issued and submitted descriptors to the terminated list whenever a
> > termination request completes. Also release virt-dma resources from
> > free_chan_resources().
> >
> > If termination was deferred because the channel was still running, wait
> > until the STOP path deconfigures the channel before synchronizing or
> > freeing virt-dma resources. Otherwise dmaengine_terminate_sync() can
> > return before the deferred STOP cleanup has moved issued descriptors to
> > the terminated list and before the channel is known to have stopped.
> >
> > The old free_chan_resources() loop usually broke as soon as
> > terminate_all() returned zero, so it did not effectively spin until the
> > timeout. This wait can now last until the existing timeout, so use
> > cond_resched() instead of busy-polling with cpu_relax(), and warn if the
> > timeout expires.
> >
> > Fixes: e63d79d1ffcd ("dmaengine: Add Synopsys eDMA IP core driver")
> > Signed-off-by: Koichiro Den <den@xxxxxxxxxxxxx>
> > ---
> >  drivers/dma/dw-edma/dw-edma-core.c | 78 ++++++++++++++++++++++++------
> >  1 file changed, 64 insertions(+), 14 deletions(-)
> >
> > diff --git a/drivers/dma/dw-edma/dw-edma-core.c b/drivers/dma/dw-edma/dw-edma-core.c
> > index bedaee6d30ab..2777dc0b2aed 100644
> > --- a/drivers/dma/dw-edma/dw-edma-core.c
> > +++ b/drivers/dma/dw-edma/dw-edma-core.c
> > @@ -15,6 +15,7 @@
> >  #include <linux/irq.h>
> >  #include <linux/dma/edma.h>
> >  #include <linux/dma-mapping.h>
> > +#include <linux/sched.h>
> >  #include <linux/string_choices.h>
> >
> >  #include "dw-edma-core.h"
> > @@ -113,6 +114,28 @@ static void dw_edma_terminate_vdesc(struct virt_dma_desc *vd)
> >  	vchan_terminate_vdesc(vd);
> >  }
> >
> > +static void dw_edma_terminate_vdesc_list(struct list_head *head)
> > +{
> > +	struct virt_dma_desc *vd, *_vd;
> > +
> > +	list_for_each_entry_safe(vd, _vd, head, node)
> > +		dw_edma_terminate_vdesc(vd);
> > +}
> > +
> > +/* Must be called with vc.lock held. */
> > +static void dw_edma_terminate_all_descs(struct dw_edma_chan *chan)
> > +{
> > +	/*
> > +	 * This order must not be reversed. Cookies are assigned when
> > +	 * descriptors are submitted, so desc_issued contains older cookies
> > +	 * than desc_submitted. Completing desc_submitted first could move
> > +	 * chan->vc.chan.completed_cookie backwards when desc_issued is
> > +	 * terminated afterwards.
> > +	 */
> > +	dw_edma_terminate_vdesc_list(&chan->vc.desc_issued);
> > +	dw_edma_terminate_vdesc_list(&chan->vc.desc_submitted);
> > +}
> > +
> 
> Is it possible move every thing to temp termniate queue by hold lock, then
> call dw_edma_terminate_vdesc(vd) outside lock.

dw_edma_terminate_vdesc() itself, or at least the cookie updating and
vchan_terminate_vdesc(), still has to run under vc.lock. But I think your point
is to avoid one potentially long critical section, and yes, I agree with that.

The way to do it would be to move desc_issued/desc_submitted to a separate
per-channel terminating list (say, chan->desc_terminating) under vc.lock, then
make the channel look as if terminated from the hardware/state-machine point of
view. After that we can retire the descriptors from that list one by one with
short vc.lock holds.

Then, extend the dw_edma_wait_termination(), which this patch (04/17)
introduces, with something like this:

    while (time_before(jiffies, timeout)) {
            dw_edma_device_terminate_all(dchan);
  +         dw_edma_drain_terminating_descs(chan);

            spin_lock_irqsave(&chan->vc.lock, flags);
  -         configured = chan->configured;
  +         done = !chan->configured && list_empty(&chan->desc_terminating);
            spin_unlock_irqrestore(&chan->vc.lock, flags);

  -         if (!configured)
  +         if (done)
                    return;

            cond_resched();
    }

I may be missing something, but I think this is the basic direction we can take
to eliminate the potentially long critical section.

Thanks for the review,
Koichiro

> 
> Frank
> >  static void dw_edma_device_caps(struct dma_chan *dchan,
> >  				struct dma_slave_caps *caps)
> >  {
> > @@ -190,20 +213,25 @@ 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);
> > +	unsigned long flags;
> >  	int err = 0;
> >
> > +	spin_lock_irqsave(&chan->vc.lock, flags);
> >  	if (!chan->configured) {
> > -		/* Do nothing */
> > +		dw_edma_terminate_all_descs(chan);
> >  	} else if (chan->status == EDMA_ST_PAUSE) {
> > +		dw_edma_terminate_all_descs(chan);
> >  		chan->status = EDMA_ST_IDLE;
> >  		chan->configured = false;
> >  	} else if (chan->status == EDMA_ST_IDLE) {
> > +		dw_edma_terminate_all_descs(chan);
> >  		chan->configured = false;
> >  	} else if (dw_edma_core_ch_status(chan) == DMA_COMPLETE) {
> >  		/*
> >  		 * The channel is in a false BUSY state, probably didn't
> >  		 * receive or lost an interrupt
> >  		 */
> > +		dw_edma_terminate_all_descs(chan);
> >  		chan->status = EDMA_ST_IDLE;
> >  		chan->configured = false;
> >  	} else if (chan->request > EDMA_REQ_PAUSE) {
> > @@ -211,6 +239,7 @@ static int dw_edma_device_terminate_all(struct dma_chan *dchan)
> >  	} else {
> >  		chan->request = EDMA_REQ_STOP;
> >  	}
> > +	spin_unlock_irqrestore(&chan->vc.lock, flags);
> >
> >  	return err;
> >  }
> > @@ -544,7 +573,7 @@ static void dw_edma_done_interrupt(struct dw_edma_chan *chan)
> >  			break;
> >
> >  		case EDMA_REQ_STOP:
> > -			dw_edma_terminate_vdesc(vd);
> > +			dw_edma_terminate_all_descs(chan);
> >  			chan->request = EDMA_REQ_NONE;
> >  			chan->status = EDMA_ST_IDLE;
> >  			break;
> > @@ -616,28 +645,49 @@ static int dw_edma_alloc_chan_resources(struct dma_chan *dchan)
> >  	return 0;
> >  }
> >
> > +static void dw_edma_wait_termination(struct dma_chan *dchan)
> > +{
> > +	struct dw_edma_chan *chan = dchan2dw_edma_chan(dchan);
> > +	unsigned long timeout = jiffies + msecs_to_jiffies(5000);
> > +	unsigned long flags;
> > +	bool configured = true;
> > +
> > +	/*
> > +	 * dw_edma_device_terminate_all() may defer cleanup to a later interrupt
> > +	 * while the channel is still running. Retry until the channel is
> > +	 * deconfigured, which marks that termination completed.
> > +	 */
> > +	while (time_before(jiffies, timeout)) {
> > +		dw_edma_device_terminate_all(dchan);
> > +
> > +		spin_lock_irqsave(&chan->vc.lock, flags);
> > +		configured = chan->configured;
> > +		spin_unlock_irqrestore(&chan->vc.lock, flags);
> > +		if (!configured)
> > +			return;
> > +
> > +		cond_resched();
> > +	}
> > +
> > +	dev_warn(chan->dw->chip->dev,
> > +		 "timeout waiting for channel termination\n");
> > +}
> > +
> >  static void dw_edma_device_synchronize(struct dma_chan *dchan)
> >  {
> >  	struct dw_edma_chan *chan = dchan2dw_edma_chan(dchan);
> >
> > +	dw_edma_wait_termination(dchan);
> >  	vchan_synchronize(&chan->vc);
> >  }
> >
> >  static void dw_edma_free_chan_resources(struct dma_chan *dchan)
> >  {
> > -	unsigned long timeout = jiffies + msecs_to_jiffies(5000);
> > -	int ret;
> > -
> > -	while (time_before(jiffies, timeout)) {
> > -		ret = dw_edma_device_terminate_all(dchan);
> > -		if (!ret)
> > -			break;
> > -
> > -		if (time_after_eq(jiffies, timeout))
> > -			return;
> > +	struct dw_edma_chan *chan = dchan2dw_edma_chan(dchan);
> >
> > -		cpu_relax();
> > -	}
> > +	dw_edma_wait_termination(dchan);
> > +	vchan_synchronize(&chan->vc);
> > +	vchan_free_chan_resources(&chan->vc);
> >  }
> >
> >  static int dw_edma_channel_setup(struct dw_edma *dw, u32 wr_alloc, u32 rd_alloc)
> > --
> > 2.51.0
> >