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

[Frank Li <Frank.li@oss.nxp.com>]

On Mon, Oct 12, 2026 at 11:15:16AM +0800, Ginger wrote:
> On Fri, Oct 9, 2026 at 5:23 AM Frank Li <Frank.li@oss.nxp.com> wrote:
> >
> > On Wed, Oct 07, 2026 at 11:01:04PM +0800, Ginger Li wrote:
> > > [You don't often get email from ginger.jzllee@gmail.com. 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@gmail.com>
> > > ---
> > >  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.

I am not familar pl330 drivers. The key point here is that avoid introduce
new problem by resolve one exitings problems.

Frank

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