Re: [PATCH] dmaengine: pl330: Fix lock-order inversion in the channel release path

[Ginger]

On Fri, Oct 9, 2026 at 5:23 AM Frank Li <Frank.li@xxxxxxxxxxx> wrote:
>
> On Wed, Oct 07, 2026 at 11:01:04PM +0800, Ginger Li wrote:
> > [You don't often get email from ginger.jzllee@xxxxxxxxx. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ]
> >
> > pl330 uses two locks per transfer: the channel lock pch->lock and the
> > controller lock pl330->lock. My static analyzer reported that they can
>
>                                The static analyzer ....
>
> > be taken concurrently in opposite orders, leading to potential deadlocks.
> >
> > The terminate/stop paths take pch->lock first and then pl330->lock, e.g.
> >   pl330_terminate_all()
> >     -> spin_lock_irqsave(&pch->lock, flags);
> >     -> spin_lock(&pl330->lock);
> > and
> >   pl330_pause()
> >     -> spin_lock_irqsave(&pch->lock, flags);
> >     -> spin_lock(&pl330->lock);
> >
> > While on the other hand, the channel release path takes pl330->lock first
> > and then pch->lock in pl330_free_chan_resources():
> >   pl330_free_chan_resources()
> >     -> spin_lock_irqsave(&pl330->lock, flags);
> >     -> pl330_release_channel(pch->thread);
> >     -> dma_pl330_rqcb()
> >       -> spin_lock_irqsave(&pch->lock, flags);
> >
> > Freeing a channel (dma_release_channel() -> dma_chan_put() ->
> > pl330_free_chan_resources()) while another CPU is in pl330_terminate_all()
> > or pl330_pause() on the same channel is therefore an ABBA deadlock: the
> > one thread spins on pl330->lock that the other holds, and vice versa.
>
> To here is enough. cut below message.
>
> >
> > Inspecting the driver code suggests the rule that dma_pl330_rqcb() must
> > not be called with pl330->lock held: pl330_dotask() and the callback
> > drain loop  in pl330_update() both releases pl330->lock before their
> > dma_pl330_rqcb() calls. Thus, pl330_release_channel() is expected
> > to follow the same manner.
> >
> > Let pl330_release_channel() manage pl330->lock itself and keep the two
> > dma_pl330_rqcb() calls outside the critical section, and stop holding
> > pl330->lock around the call in pl330_free_chan_resources().
> >
> > This was found by a static analyzer on Linux 7.3-rc4; it reported
> > DeadLock::AllLock (Certain) for
> >
> >     pl330_free_chan_resources() <-> pl330_terminate_all()
> >     pl330_free_chan_resources() <-> pl330_pause().
> >
> > Fixes: 91539eb1fda2 ("dmaengine: pl330: fix double lock")
> > Signed-off-by: Ginger Li <ginger.jzllee@xxxxxxxxx>
> > ---
> >  drivers/dma/pl330.c | 24 +++++++++++++++++++++++-
> >  1 file changed, 23 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c
> > --- a/drivers/dma/pl330.c
> > +++ b/drivers/dma/pl330.c
> > @@ -1805,16 +1805,32 @@
> >
> >  static void pl330_release_channel(struct pl330_thread *thrd)
> >  {
> > +       struct pl330_dmac *pl330;
> > +       unsigned long flags;
> > +
> >         if (!thrd || thrd->free)
> >                 return;
> >
> > +       pl330 = thrd->dmac;
> > +
> > +       spin_lock_irqsave(&pl330->lock, flags);
> >         _stop(thrd);
> > +       spin_unlock_irqrestore(&pl330->lock, flags);
>
> [1]
> >
> > +       /*
> > +        * dma_pl330_rqcb() takes the channel lock, which is acquired before
> > +        * pl330->lock on the terminate/pause/tx_status paths.  Calling it with
> > +        * pl330->lock held would invert the lock order, so keep it outside the
> > +        * critical section
> > +        * pl330_update() already follow.
> > +        */
> >         dma_pl330_rqcb(thrd->req[1 - thrd->lstenq].desc, PL330_ERR_ABORT);
> >         dma_pl330_rqcb(thrd->req[thrd->lstenq].desc, PL330_ERR_ABORT);
> [2]
> >
> > +       spin_lock_irqsave(&pl330->lock, flags);
> >         _free_event(thrd, thrd->ev);
> >         thrd->free = true;
> > +       spin_unlock_irqrestore(&pl330->lock, flags);
>
> [3],
>
> how do you make sure it is safe to do 1, 2 and 3 without pl330->lock?
>

It seems that [2] is not strictly needed inside pl330->lock (e.g., pl330_dotask
releases the lock before calling dma_pl330_rqcb()). However, if [1] and [3] are
expected to run atomically, would the following change do? This fix keeps the
original structure but moves the dma_pl330_rqcb() calls outside the critical
section of pl330->lock.

Sincerely,
Ginger

---
 drivers/dma/pl330.c | 36 +++++++++++++++++++++++++++++++-----
 1 file changed, 31 insertions(+), 5 deletions(-)

diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c
--- a/drivers/dma/pl330.c
+++ b/drivers/dma/pl330.c
@@ -1803,15 +1803,33 @@ static inline void _free_event(struct
pl330_thread *thrd, int ev)
  pl330->events[ev] = -1;
 }

-static void pl330_release_channel(struct pl330_thread *thrd)
+static void pl330_release_channel(struct pl330_thread *thrd,
+  struct dma_pl330_desc **done_a,
+  struct dma_pl330_desc **done_b)
 {
+ *done_a = NULL;
+ *done_b = NULL;
+
  if (!thrd || thrd->free)
  return;

  _stop(thrd);

- dma_pl330_rqcb(thrd->req[1 - thrd->lstenq].desc, PL330_ERR_ABORT);
- dma_pl330_rqcb(thrd->req[thrd->lstenq].desc, PL330_ERR_ABORT);
+ *done_a = thrd->req[1 - thrd->lstenq].desc;
+ *done_b = thrd->req[thrd->lstenq].desc;

  _free_event(thrd, thrd->ev);
  thrd->free = true;
@@ -1993,12 +2011,15 @@ static int pl330_add(struct pl330_dmac *pl330)
 static int dmac_free_threads(struct pl330_dmac *pl330)
 {
  struct pl330_thread *thrd;
+ struct dma_pl330_desc *done_a, *done_b;
  int i;

  /* Release Channel threads */
  for (i = 0; i < pl330->pcfg.num_chan; i++) {
  thrd = &pl330->channels[i];
- pl330_release_channel(thrd);
+ pl330_release_channel(thrd, &done_a, &done_b);
+ dma_pl330_rqcb(done_a, PL330_ERR_ABORT);
+ dma_pl330_rqcb(done_b, PL330_ERR_ABORT);
  }

  /* Free memory */
@@ -2353,6 +2374,7 @@ static void pl330_free_chan_resources(struct
dma_chan *chan)
 {
  struct dma_pl330_chan *pch = to_pchan(chan);
  struct pl330_dmac *pl330 = pch->dmac;
+ struct dma_pl330_desc *done_a, *done_b;
  unsigned long flags;

  tasklet_kill(&pch->task);
@@ -2360,13 +2382,17 @@ static void pl330_free_chan_resources(struct
dma_chan *chan)
  pm_runtime_get_sync(pch->dmac->ddma.dev);
  spin_lock_irqsave(&pl330->lock, flags);

- pl330_release_channel(pch->thread);
+ /* The two callbacks take pch->lock, so they run after the unlock below. */
+ pl330_release_channel(pch->thread, &done_a, &done_b);
  pch->thread = NULL;

  if (pch->cyclic)
  list_splice_tail_init(&pch->work_list, &pch->dmac->desc_pool);

  spin_unlock_irqrestore(&pl330->lock, flags);
+
+ dma_pl330_rqcb(done_a, PL330_ERR_ABORT);
+ dma_pl330_rqcb(done_b, PL330_ERR_ABORT);
  pm_runtime_put_autosuspend(pch->dmac->ddma.dev);
  pl330_unprep_slave_fifo(pch);
 }
-- 
2.43.0