Re: [PATCH v3 4/9] dmaengine: sun6i: serialize physical channel ownership

[Frank Li]

On Fri, Oct 23, 2026 at 05:38:23PM -0600, James Hilliard wrote:
> The allocator changes vchan->phy and pchan->vchan under sdev->lock, but
> termination reads the physical channel before taking either lock. The IRQ
> handler also takes an unprotected owner snapshot. Termination can miss a
> new assignment or detach an owner while an interrupt is about to complete
> its descriptor. A sampled IRQ can also be applied after reassignment.
>
> Protect physical-channel ownership with sdev->lock across allocation,
> interrupt handling, pause, termination and residue queries. Read and
> acknowledge IRQ status under that lock, and allocate and start each pending
> channel in the same critical section. This also serializes updates to the
> shared interrupt-enable registers.
>
> Nest vc.lock inside the device lock when accessing descriptor lists or
> updating cookies, including cyclic notifications. virt-dma takes vc.lock
> independently when preparing, submitting and completing descriptors, so
> the device lock does not replace it. Client callbacks run outside both
> locks in the virtual-channel tasklet.
>
> Fixes: 555859308723 ("dmaengine: sun6i: Add driver for the Allwinner A31 DMA controller")
> Reported-by: Sashiko <sashiko-bot@xxxxxxxxxx>
> Closes: https://lore.kernel.org/r/20260706065650.34B131F00A3A@xxxxxxxxxxxxxxx
> Signed-off-by: James Hilliard <james.hilliard1@xxxxxxxxx>
> ---
>  drivers/dma/sun6i-dma.c | 67 ++++++++++++++++++++++++-------------------------
>  1 file changed, 33 insertions(+), 34 deletions(-)
>
> diff --git a/drivers/dma/sun6i-dma.c b/drivers/dma/sun6i-dma.c
> index 435d7daa1c0f..d3a63c866645 100644
> --- a/drivers/dma/sun6i-dma.c
> +++ b/drivers/dma/sun6i-dma.c
> @@ -217,6 +217,10 @@ struct sun6i_dma_dev {
>  	struct clk		*clk;
>  	struct clk		*clk_mbus;
>  	int			irq;
> +	/*
> +	 * Physical channels, pending list and registers; nests outside vc.lock.
> +	 * virt-dma also takes vc.lock independently for its lists and cookies.
> +	 */
>  	spinlock_t		lock;
>  	struct reset_control	*rstc;
>  	struct tasklet_struct	task;
> @@ -524,11 +528,11 @@ static void sun6i_dma_tasklet(struct tasklet_struct *t)
>  	struct sun6i_dma_dev *sdev = from_tasklet(sdev, t, task);
>  	struct sun6i_vchan *vchan;
>  	struct sun6i_pchan *pchan;
> -	unsigned int pchan_alloc = 0;
>  	unsigned int pchan_idx;
>
> +	spin_lock_irq(&sdev->lock);
>  	list_for_each_entry(vchan, &sdev->slave.channels, vc.chan.device_node) {
> -		spin_lock_irq(&vchan->vc.lock);
> +		spin_lock(&vchan->vc.lock);
>
>  		pchan = vchan->phy;
>
> @@ -545,10 +549,9 @@ static void sun6i_dma_tasklet(struct tasklet_struct *t)
>  				pchan->vchan = NULL;
>  			}
>  		}
> -		spin_unlock_irq(&vchan->vc.lock);
> +		spin_unlock(&vchan->vc.lock);
>  	}
>
> -	spin_lock_irq(&sdev->lock);
>  	for (pchan_idx = 0; pchan_idx < sdev->num_pchans; pchan_idx++) {
>  		pchan = &sdev->pchans[pchan_idx];
>
> @@ -557,31 +560,20 @@ static void sun6i_dma_tasklet(struct tasklet_struct *t)
>
>  		vchan = list_first_entry(&sdev->pending,
>  					 struct sun6i_vchan, node);
> +		spin_lock(&vchan->vc.lock);
>
>  		/* Remove from pending channels */
>  		list_del_init(&vchan->node);
> -		pchan_alloc |= BIT(pchan_idx);
>
>  		/* Mark this channel allocated */
>  		pchan->vchan = vchan;
>  		vchan->phy = pchan;
>  		dev_dbg(sdev->slave.dev, "pchan %u: alloc vchan %p\n",
>  			pchan->idx, &vchan->vc);
> +		sun6i_dma_start_desc(vchan);
> +		spin_unlock(&vchan->vc.lock);
>  	}
>  	spin_unlock_irq(&sdev->lock);
> -
> -	for (pchan_idx = 0; pchan_idx < sdev->num_pchans; pchan_idx++) {
> -		if (!(pchan_alloc & BIT(pchan_idx)))
> -			continue;
> -
> -		pchan = sdev->pchans + pchan_idx;
> -		vchan = pchan->vchan;
> -		if (vchan) {
> -			spin_lock_irq(&vchan->vc.lock);
> -			sun6i_dma_start_desc(vchan);
> -			spin_unlock_irq(&vchan->vc.lock);
> -		}
> -	}
>  }
>
>  static irqreturn_t sun6i_dma_interrupt(int irq, void *dev_id)
> @@ -590,9 +582,11 @@ static irqreturn_t sun6i_dma_interrupt(int irq, void *dev_id)
>  	u32 channels_per_reg = sdev->cfg->num_channels_per_reg;
>  	struct sun6i_vchan *vchan;
>  	struct sun6i_pchan *pchan;
> +	unsigned long flags;
>  	int i, j, ret = IRQ_NONE;
>  	u32 status;
>
> +	spin_lock_irqsave(&sdev->lock, flags);
>  	for (i = 0; i < DIV_ROUND_UP(sdev->num_pchans, channels_per_reg); i++) {
>  		status = sun6i_read_irq_stat(sdev, i);
>  		if (!status)
> @@ -608,14 +602,14 @@ static irqreturn_t sun6i_dma_interrupt(int irq, void *dev_id)
>  			pchan = sdev->pchans + i * channels_per_reg + j;
>  			vchan = pchan->vchan;
>  			if (vchan && (status & vchan->irq_type)) {
> +				spin_lock(&vchan->vc.lock);
>  				if (vchan->cyclic) {
>  					vchan_cyclic_callback(&pchan->desc->vd);
>  				} else {
> -					spin_lock(&vchan->vc.lock);
>  					vchan_cookie_complete(&pchan->desc->vd);
>  					pchan->done = pchan->desc;
> -					spin_unlock(&vchan->vc.lock);
>  				}
> +				spin_unlock(&vchan->vc.lock);
>  			}
>
>  			status = status >> DMA_IRQ_CHAN_WIDTH;
> @@ -625,6 +619,7 @@ static irqreturn_t sun6i_dma_interrupt(int irq, void *dev_id)
>  			tasklet_schedule(&sdev->task);
>  		ret = IRQ_HANDLED;
>  	}
> +	spin_unlock_irqrestore(&sdev->lock, flags);
>
>  	return ret;
>  }
> @@ -931,19 +926,20 @@ static int sun6i_dma_pause(struct dma_chan *chan)
>  {
>  	struct sun6i_dma_dev *sdev = to_sun6i_dma_dev(chan->device);
>  	struct sun6i_vchan *vchan = to_sun6i_vchan(chan);
> -	struct sun6i_pchan *pchan = vchan->phy;
> +	struct sun6i_pchan *pchan;
>  	unsigned long flags;
>
>  	dev_dbg(dmaengine_chan_dev(chan), "vchan %p: pause\n", &vchan->vc);
>
> +	spin_lock_irqsave(&sdev->lock, flags);
> +	pchan = vchan->phy;
>  	if (pchan) {
>  		writel(DMA_CHAN_PAUSE_PAUSE,
>  		       pchan->base + DMA_CHAN_PAUSE);
>  	} else {
> -		spin_lock_irqsave(&sdev->lock, flags);
>  		list_del_init(&vchan->node);

suppose need vchan lock here?

Frank
> -		spin_unlock_irqrestore(&sdev->lock, flags);
>  	}
> +	spin_unlock_irqrestore(&sdev->lock, flags);
>
>  	return 0;
>  }
> @@ -978,15 +974,14 @@ static int sun6i_dma_terminate_all(struct dma_chan *chan)
>  {
>  	struct sun6i_dma_dev *sdev = to_sun6i_dma_dev(chan->device);
>  	struct sun6i_vchan *vchan = to_sun6i_vchan(chan);
> -	struct sun6i_pchan *pchan = vchan->phy;
> +	struct sun6i_pchan *pchan;
>  	unsigned long flags;
>  	LIST_HEAD(head);
>
>  	spin_lock_irqsave(&sdev->lock, flags);
> +	spin_lock(&vchan->vc.lock);
> +	pchan = vchan->phy;
>  	list_del_init(&vchan->node);
> -	spin_unlock_irqrestore(&sdev->lock, flags);
> -
> -	spin_lock_irqsave(&vchan->vc.lock, flags);
>
>  	if (pchan && pchan->desc && pchan->desc != pchan->done) {
>  		struct virt_dma_desc *vd = &pchan->desc->vd;
> @@ -1007,7 +1002,8 @@ static int sun6i_dma_terminate_all(struct dma_chan *chan)
>  		pchan->done = NULL;
>  	}
>
> -	spin_unlock_irqrestore(&vchan->vc.lock, flags);
> +	spin_unlock(&vchan->vc.lock);
> +	spin_unlock_irqrestore(&sdev->lock, flags);
>
>  	vchan_dma_desc_free_list(&vchan->vc, &head);
>
> @@ -1018,8 +1014,9 @@ static enum dma_status sun6i_dma_tx_status(struct dma_chan *chan,
>  					   dma_cookie_t cookie,
>  					   struct dma_tx_state *state)
>  {
> +	struct sun6i_dma_dev *sdev = to_sun6i_dma_dev(chan->device);
>  	struct sun6i_vchan *vchan = to_sun6i_vchan(chan);
> -	struct sun6i_pchan *pchan = vchan->phy;
> +	struct sun6i_pchan *pchan;
>  	struct sun6i_dma_lli *lli;
>  	struct virt_dma_desc *vd;
>  	struct sun6i_desc *txd;
> @@ -1027,11 +1024,12 @@ static enum dma_status sun6i_dma_tx_status(struct dma_chan *chan,
>  	unsigned long flags;
>  	size_t bytes = 0;
>
> +	spin_lock_irqsave(&sdev->lock, flags);
> +	spin_lock(&vchan->vc.lock);
>  	ret = dma_cookie_status(chan, cookie, state);
>  	if (ret == DMA_COMPLETE || !state)
> -		return ret;
> -
> -	spin_lock_irqsave(&vchan->vc.lock, flags);
> +		goto out;
> +	pchan = vchan->phy;
>
>  	vd = vchan_find_desc(&vchan->vc, cookie);
>  	txd = to_sun6i_desc(&vd->tx);
> @@ -1045,9 +1043,10 @@ static enum dma_status sun6i_dma_tx_status(struct dma_chan *chan,
>  		bytes = sun6i_get_chan_size(pchan);
>  	}
>
> -	spin_unlock_irqrestore(&vchan->vc.lock, flags);
> -
>  	dma_set_residue(state, bytes);
> +out:
> +	spin_unlock(&vchan->vc.lock);
> +	spin_unlock_irqrestore(&sdev->lock, flags);
>
>  	return ret;
>  }
>
> --
> 2.53.0
>