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

[Frank Li <Frank.li@oss.nxp.com>]

On Fri, Jul 10, 2026 at 05:08:59PM +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@valinux.co.jp>
> ---

Reviewed-by: Frank Li <Frank.Li@nxp.com>

> Changes in v2:
>   - Poll with usleep_range() (and include linux/delay.h for it) instead
>     of a cond_resched() busy loop in the termination wait; each
>     iteration does an MMIO read that is a non-posted round trip on
>     remote setups.
>   - Split out into this preparation series (was patch 04/17 of
>     the dynamic LL appends v1).
>   - Let dw_edma_free_chan_resources() reuse dw_edma_device_synchronize()
>     instead of open-coding the same wait-and-synchronize sequence.
>
>  drivers/dma/dw-edma/dw-edma-core.c | 79 ++++++++++++++++++++++++------
>  1 file changed, 65 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/dma/dw-edma/dw-edma-core.c b/drivers/dma/dw-edma/dw-edma-core.c
> index 4e0dc52397e2..1b493c104a5b 100644
> --- a/drivers/dma/dw-edma/dw-edma-core.c
> +++ b/drivers/dma/dw-edma/dw-edma-core.c
> @@ -7,6 +7,7 @@
>   */
>
>  #include <linux/module.h>
> +#include <linux/delay.h>
>  #include <linux/device.h>
>  #include <linux/kernel.h>
>  #include <linux/dmaengine.h>
> @@ -15,6 +16,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"
> @@ -208,6 +210,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);
> +}
> +
>  static void dw_edma_device_caps(struct dma_chan *dchan,
>  				struct dma_slave_caps *caps)
>  {
> @@ -313,20 +337,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) {
> @@ -334,6 +363,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;
>  }
> @@ -680,7 +710,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;
> @@ -862,28 +892,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;
> +
> +		usleep_range(1000, 2000);
> +		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_device_synchronize(dchan);
> +	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
>