Re: [PATCH v4 2/9] dmaengine: dw-edma: Terminate all descriptors without callbacks

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

On Fri, Jul 17, 2026 at 12:24:56AM +0900, Koichiro Den wrote:
> The DMA Engine client documentation says in the "Terminate APIs" section
> of Documentation/driver-api/dmaengine/client.rst:
>
> "No callback functions will be called for any incomplete transfers."
>
> dw-edma instead calls vchan_cookie_complete() when a deferred STOP reaches
> the interrupt handler. This schedules a callback for the active descriptor
> and leaves other issued or submitted descriptors queued. A late callback
> after dmaengine_terminate_sync() can dereference client state that has
> already been freed, while leftover descriptors may later restart into
> reused buffers or leak.
>
> Move all issued and submitted descriptors to the terminated list whenever
> termination completes. For a pending STOP, do this from both the DONE and
> ABORT paths. Complete their cookies in order without scheduling callbacks.
>
> A STOP can remain pending until the running transfer raises an
> interrupt. Make device_synchronize() wait for such a pending STOP to
> complete before releasing terminated descriptors. Reuse it from
> free_chan_resources(), then release the remaining virt-dma resources.
> Sleep instead of busy-polling while waiting, and warn if the existing
> 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 v4:
>   - Use fsleep(). (Frank)
>   - Use guard()/scoped_guard() for the new locking. (Frank)
>   - Keep chan->configured across device_synchronize() and clear it in
>     free_chan_resources() instead. (Sashiko)
>   - Wait only while a deferred STOP remains pending.
>
>  drivers/dma/dw-edma/dw-edma-core.c | 90 +++++++++++++++++++++++++-----
>  1 file changed, 76 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/dma/dw-edma/dw-edma-core.c b/drivers/dma/dw-edma/dw-edma-core.c
> index 89a4c498a17b..972c7ea18d46 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>
> @@ -201,6 +202,35 @@ static int dw_edma_start_transfer(struct dw_edma_chan *chan)
>  	return 1;
>  }
>
> +static void dw_edma_terminate_vdesc(struct virt_dma_desc *vd)
> +{
> +	list_del(&vd->node);
> +	dma_cookie_complete(&vd->tx);
> +	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)
>  {
> @@ -308,20 +338,22 @@ static int dw_edma_device_terminate_all(struct dma_chan *dchan)
>  	struct dw_edma_chan *chan = dchan2dw_edma_chan(dchan);
>  	int err = 0;
>
> +	guard(spinlock_irqsave)(&chan->vc.lock);
> +
>  	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) {
> -		chan->configured = false;
> +		dw_edma_terminate_all_descs(chan);
>  	} 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) {
>  		err = -EPERM;
>  	} else {
> @@ -673,8 +705,7 @@ static void dw_edma_done_interrupt(struct dw_edma_chan *chan)
>  			break;
>
>  		case EDMA_REQ_STOP:
> -			list_del(&vd->node);
> -			vchan_cookie_complete(vd);
> +			dw_edma_terminate_all_descs(chan);
>  			chan->request = EDMA_REQ_NONE;
>  			chan->status = EDMA_ST_IDLE;
>  			break;
> @@ -698,7 +729,9 @@ static void dw_edma_abort_interrupt(struct dw_edma_chan *chan)
>
>  	spin_lock_irqsave(&chan->vc.lock, flags);
>  	vd = vchan_next_desc(&chan->vc);
> -	if (vd) {
> +	if (vd && chan->request == EDMA_REQ_STOP) {
> +		dw_edma_terminate_all_descs(chan);
> +	} else if (vd) {
>  		dw_hdma_set_callback_result(vd, DMA_TRANS_ABORTED);
>  		list_del(&vd->node);
>  		vchan_cookie_complete(vd);
> @@ -856,21 +889,49 @@ static int dw_edma_alloc_chan_resources(struct dma_chan *dchan)
>  	return 0;
>  }
>
> -static void dw_edma_free_chan_resources(struct dma_chan *dchan)
> +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);
> -	int ret;
> +	bool stopping;
>
> +	/*
> +	 * A STOP may be deferred to a later interrupt while the channel is still
> +	 * running. Wait until that handler completes the termination.
> +	 */
>  	while (time_before(jiffies, timeout)) {
> -		ret = dw_edma_device_terminate_all(dchan);
> -		if (!ret)
> -			break;
> +		scoped_guard(spinlock_irqsave, &chan->vc.lock)
> +			stopping = chan->request == EDMA_REQ_STOP;
>
> -		if (time_after_eq(jiffies, timeout))
> +		if (!stopping)
>  			return;
>
> -		cpu_relax();
> +		fsleep(1000);
>  	}
> +
> +	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)
> +{
> +	struct dw_edma_chan *chan = dchan2dw_edma_chan(dchan);
> +
> +	dw_edma_device_terminate_all(dchan);
> +	dw_edma_device_synchronize(dchan);
> +
> +	scoped_guard(spinlock_irqsave, &chan->vc.lock)
> +		chan->configured = false;
> +
> +	vchan_free_chan_resources(&chan->vc);
>  }
>
>  static int dw_edma_channel_setup(struct dw_edma *dw, u32 wr_alloc, u32 rd_alloc)
> @@ -968,6 +1029,7 @@ static int dw_edma_channel_setup(struct dw_edma *dw, u32 wr_alloc, u32 rd_alloc)
>  	dma->device_pause = dw_edma_device_pause;
>  	dma->device_resume = dw_edma_device_resume;
>  	dma->device_terminate_all = dw_edma_device_terminate_all;
> +	dma->device_synchronize = dw_edma_device_synchronize;
>  	dma->device_issue_pending = dw_edma_device_issue_pending;
>  	dma->device_tx_status = dw_edma_device_tx_status;
>  	dma->device_prep_slave_sg = dw_edma_device_prep_slave_sg;
> --
> 2.51.0
>