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

[Frank Li]

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?

Frank
>  }
>
>  /* Initialize the structure for PL330 configuration, that can be used
> @@ -2358,9 +2374,15 @@
>         tasklet_kill(&pch->task);
>
>         pm_runtime_get_sync(pch->dmac->ddma.dev);
> -       spin_lock_irqsave(&pl330->lock, flags);
>
> +       /*
> +        * pl330_release_channel() takes pl330->lock itself and calls
> +        * dma_pl330_rqcb(), which takes the channel lock.  It must therefore
> +        * not be called with pl330->lock held (see the comment there).
> +        */
>         pl330_release_channel(pch->thread);
> +
> +       spin_lock_irqsave(&pl330->lock, flags);
>         pch->thread = NULL;
>
>         if (pch->cyclic)
> --
> 2.43.0