[PATCH v3 4/9] dmaengine: sun6i: serialize physical channel ownership
Frank Li
Frank.li at oss.nxp.com
Mon Oct 26 14:14:13 PDT 2026
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 at kernel.org>
> Closes: https://lore.kernel.org/r/20260706065650.34B131F00A3A@smtp.kernel.org
> Signed-off-by: James Hilliard <james.hilliard1 at gmail.com>
> ---
> 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
>
More information about the linux-arm-kernel
mailing list