[PATCH v3 4/9] dmaengine: sun6i: serialize physical channel ownership
James Hilliard
james.hilliard1 at gmail.com
Mon Oct 26 14:42:18 PDT 2026
On Mon, Oct 26, 2026 at 3:14 PM Frank Li <Frank.li at oss.nxp.com> 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 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?
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
> >
More information about the linux-arm-kernel
mailing list