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

[James Hilliard]

On Mon, Oct 26, 2026 at 3:14 PM Frank Li <Frank.li@xxxxxxxxxxx> wrote:
>
> 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?

vchan->node links the channel into sdev->pending; it is separate from
virt-dma's descriptor-list nodes. All runtime accesses to that pending
list are protected by sdev->lock.

The same lock protects vchan->phy, so it also keeps the physical-channel
assignment stable while pause() checks it and either pauses the hardware
or removes the channel from the pending list.

resume() additionally needs vc.lock because it inspects vc.desc_issued,
and issue_pending() needs it to move descriptors between virt-dma lists.
pause() does not access those lists or cookies, so taking vc.lock here
would not protect any additional state.

>
> 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
> >