Re: [PATCH] dmaengine: sun4i: retain cyclic descriptors until callbacks drain

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

On Tue, Nov 03, 2026 at 11:25:58PM -0700, James Hilliard wrote:
> A cyclic IRQ queues a virt-dma tasklet with a pointer to the active
> descriptor. sun4i_dma_terminate_all() frees it without clearing the queued
> cyclic callback or waiting for one which has already started. The tasklet
> can subsequently access the freed descriptor. There is no
> device_synchronize callback to let clients drain these callbacks before
> releasing their own resources either.
>
> Serialize both half-transfer and end-transfer IRQ handling with termination
> under the virtual-channel lock. Recheck the physical-channel association
> after acquiring that lock so an IRQ which looked up the channel before
> termination cannot use its detached transfer state.
>
> Move terminated cyclic descriptors to the virt-dma terminated list. This
> clears any pending cyclic callback. Retain them until a new synchronize
> callback has waited for the IRQ handler and drained the tasklet. Drain
> callbacks before freeing channel resources as well. Termination itself
> remains usable from atomic context and from a completion callback.
>
> Keep freeing other collected descriptors immediately: an in-flight
> non-cyclic callback owns its descriptor on the tasklet's private list.
>
> Fixes: b096c1377d1e ("dmaengine: sun4i: Add support for the DMA engine on sun[457]i SoCs")
> Signed-off-by: James Hilliard <james.hilliard1@gmail.com>
> ---
>  drivers/dma/sun4i-dma.c | 46 +++++++++++++++++++++++++++++++++++-----------
>  1 file changed, 35 insertions(+), 11 deletions(-)
>
> diff --git a/drivers/dma/sun4i-dma.c b/drivers/dma/sun4i-dma.c
> index 225e4a53502b..b92cd9c8676e 100644
> --- a/drivers/dma/sun4i-dma.c
> +++ b/drivers/dma/sun4i-dma.c
> @@ -293,10 +293,20 @@ static int convert_buswidth(enum dma_slave_buswidth addr_width)
>  	return (addr_width >> 1);
>  }
>
> +static void sun4i_dma_synchronize(struct dma_chan *chan)
> +{
> +	struct sun4i_dma_dev *priv = to_sun4i_dma_dev(chan->device);
> +	struct sun4i_dma_vchan *vchan = to_sun4i_dma_vchan(chan);
> +
> +	synchronize_irq(priv->irq);
> +	vchan_synchronize(&vchan->vc);
> +}
> +
>  static void sun4i_dma_free_chan_resources(struct dma_chan *chan)
>  {
>  	struct sun4i_dma_vchan *vchan = to_sun4i_dma_vchan(chan);
>
> +	sun4i_dma_synchronize(chan);
>  	vchan_free_chan_resources(&vchan->vc);
>  }
>
> @@ -322,7 +332,7 @@ static struct sun4i_dma_pchan *find_and_use_pchan(struct sun4i_dma_dev *priv,
>  	spin_lock_irqsave(&priv->lock, flags);
>  	for_each_clear_bit_from(i, priv->pchans_used, max) {
>  		pchan = &pchans[i];
> -		pchan->vchan = vchan;
> +		WRITE_ONCE(pchan->vchan, vchan);
>  		set_bit(i, priv->pchans_used);
>  		break;
>  	}
> @@ -339,7 +349,7 @@ static void release_pchan(struct sun4i_dma_dev *priv,
>
>  	spin_lock_irqsave(&priv->lock, flags);
>
> -	pchan->vchan = NULL;
> +	WRITE_ONCE(pchan->vchan, NULL);
>  	clear_bit(nr, priv->pchans_used);
>
>  	spin_unlock_irqrestore(&priv->lock, flags);
> @@ -970,13 +980,13 @@ static int sun4i_dma_terminate_all(struct dma_chan *chan)
>  {
>  	struct sun4i_dma_dev *priv = to_sun4i_dma_dev(chan->device);
>  	struct sun4i_dma_vchan *vchan = to_sun4i_dma_vchan(chan);
> -	struct sun4i_dma_pchan *pchan = vchan->pchan;
> +	struct sun4i_dma_pchan *pchan;
> +	struct virt_dma_desc *vd, *tmp;
>  	LIST_HEAD(head);
>  	unsigned long flags;
>
>  	spin_lock_irqsave(&vchan->vc.lock, flags);
> -	vchan_get_all_descriptors(&vchan->vc, &head);
> -	spin_unlock_irqrestore(&vchan->vc.lock, flags);
> +	pchan = vchan->pchan;
>
>  	/*
>  	 * Clearing the configuration register will halt the pchan. Interrupts
> @@ -991,10 +1001,20 @@ static int sun4i_dma_terminate_all(struct dma_chan *chan)
>  		release_pchan(priv, pchan);
>  	}
>
> -	spin_lock_irqsave(&vchan->vc.lock, flags);
>  	/* Clear these so the vchan is usable again */
>  	vchan->processing = NULL;
>  	vchan->pchan = NULL;
> +	vchan->contract = NULL;
> +
> +	/* A tasklet may already be using the active cyclic descriptor. */
> +	vchan_get_all_descriptors(&vchan->vc, &head);
> +	list_for_each_entry_safe(vd, tmp, &head, node) {
> +		if (!to_sun4i_dma_contract(vd)->is_cyclic)
> +			continue;
> +
> +		list_del(&vd->node);
> +		vchan_terminate_vdesc(vd);
> +	}

Can you move this logic to vchan-dma.c/h ? I supose other vchan should use
this similar logic also.

Frank

>  	spin_unlock_irqrestore(&vchan->vc.lock, flags);
>
>  	vchan_dma_desc_free_list(&vchan->vc, &head);
> @@ -1128,9 +1148,15 @@ static irqreturn_t sun4i_dma_interrupt(int irq, void *dev_id)
>
>  	for_each_set_bit(bit, &pendirq, 32) {
>  		pchan = &pchans[bit >> 1];
> -		vchan = pchan->vchan;
> +		vchan = READ_ONCE(pchan->vchan);
>  		if (!vchan) /* a terminated channel may still interrupt */
>  			continue;
> +
> +		spin_lock(&vchan->vc.lock);
> +		if (vchan->pchan != pchan || !vchan->processing) {
> +			spin_unlock(&vchan->vc.lock);
> +			continue;
> +		}
>  		contract = vchan->contract;
>
>  		/*
> @@ -1138,8 +1164,6 @@ static irqreturn_t sun4i_dma_interrupt(int irq, void *dev_id)
>  		 * interrupt (odd bit)
>  		 */
>  		if (bit & 1) {
> -			spin_lock(&vchan->vc.lock);
> -
>  			/*
>  			 * Move the promise into the completed list now that
>  			 * we're done with it
> @@ -1173,8 +1197,6 @@ static irqreturn_t sun4i_dma_interrupt(int irq, void *dev_id)
>  				disableirqs |= BIT(bit);
>  				release_pchan(priv, pchan);
>  			}
> -
> -			spin_unlock(&vchan->vc.lock);
>  		} else {
>  			/* Half done interrupt */
>  			if (contract->is_cyclic)
> @@ -1182,6 +1204,7 @@ static irqreturn_t sun4i_dma_interrupt(int irq, void *dev_id)
>  			else
>  				disableirqs |= BIT(bit);
>  		}
> +		spin_unlock(&vchan->vc.lock);
>  	}
>
>  	/* Disable the IRQs for events we handled */
> @@ -1276,6 +1299,7 @@ static int sun4i_dma_probe(struct platform_device *pdev)
>  	priv->slave.device_prep_dma_cyclic	= sun4i_dma_prep_dma_cyclic;
>  	priv->slave.device_config		= sun4i_dma_config;
>  	priv->slave.device_terminate_all	= sun4i_dma_terminate_all;
> +	priv->slave.device_synchronize	= sun4i_dma_synchronize;
>  	priv->slave.copy_align			= 2;
>  	priv->slave.src_addr_widths		= BIT(DMA_SLAVE_BUSWIDTH_1_BYTE) |
>  						  BIT(DMA_SLAVE_BUSWIDTH_2_BYTES) |
>
> ---
> base-commit: 6628e7d4c6b25c8d18f1345e3a20d21829aae7e4
> change-id: 20261103-submit-sun4i-dma-callback-lifetime-v1-b6192060b76f
>
> Best regards,
> --
> James Hilliard <james.hilliard1@gmail.com>
>