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

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

[Ginger Li]

pl330 uses two locks per transfer: the channel lock pch->lock and the
controller lock pl330->lock. My static analyzer reported that they can
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.

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);
 
+	/*
+	 * 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 - the same convention pl330_dotask() and
+	 * 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);
 
+	spin_lock_irqsave(&pl330->lock, flags);
 	_free_event(thrd, thrd->ev);
 	thrd->free = true;
+	spin_unlock_irqrestore(&pl330->lock, flags);
 }
 
 /* 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

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

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [Critical] dmaengine: pl330: NULL pointer dereference in pl330_terminate_all() due to concurrent channel release
- [High] dmaengine: pl330: NULL pointer deref in tasklet if channel released active
- [High] dmaengine: pl330: Data Race and Use-After-Free in pl330_release_channel()
- [High] dmaengine: pl330: TOCTOU race in pl330_release_channel() causes descriptor corruption
--

commit bd2f2bca09f98fba1fc42efeff8f9f299c1f56dd
Author: Ginger Li <ginger.jzllee@gmail.com>

dmaengine: pl330: Fix lock-order inversion in the channel release path

This commit addresses a potential deadlock caused by taking the channel lock
and controller lock in different orders across the terminate, pause, and
channel release paths. It modifies pl330_release_channel() to manage the
controller lock internally. It also updates pl330_free_chan_resources() to
call the release function without holding the controller lock.

> diff --git a/drivers/dma/pl330.c b/drivers/dma/pl330.c
> index c2d0b473b8f48..a8d1e5749dc5e 100644
> --- a/drivers/dma/pl330.c
> +++ b/drivers/dma/pl330.c
[ ... ]
> @@ -1805,16 +1805,32 @@ static inline void _free_event(struct pl330_thread *thrd, int ev)
>  
>  static void pl330_release_channel(struct pl330_thread *thrd)
>  {
> +	struct pl330_dmac *pl330;
> +	unsigned long flags;
> +
>  	if (!thrd || thrd->free)
>  		return;

[Severity: High]
Can a time-of-check to time-of-use race happen here since thrd->free is
checked outside the pl330->lock?

If a dma client calls dma_release_channel() concurrently with the driver's
unbind path pl330_remove(), both paths could pass this check concurrently.
Both would sequentially execute _stop(thrd) and dma_pl330_rqcb() on the
same thread.

If the descriptor has already been returned to the global pool and plucked
by another active channel, the second execution could overwrite the
in-flight desc->status and erroneously schedule the new channel's tasklet.

>  
> +	pl330 = thrd->dmac;
> +
> +	spin_lock_irqsave(&pl330->lock, flags);
>  	_stop(thrd);
> +	spin_unlock_irqrestore(&pl330->lock, flags);
>  
> +	/*
> +	 * 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 - the same convention pl330_dotask() and
> +	 * 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);

[Severity: High]
Does dropping the lock here introduce a data race and use-after-free?

The descriptor pointers thrd->req[...].desc are protected by pl330->lock.
By reading them after unlocking pl330->lock, concurrent tasklets like
pl330_dotask() or pl330_update() can detach the same descriptor, pass it to
dma_pl330_rqcb(), and set the array pointer to NULL.

If cpu0 locklessly reads the pointer just before it is set to NULL by a
concurrent tasklet, cpu0 will hold a stale descriptor pointer. The tasklet
can then complete the descriptor and free it back to the dmac pool where
it gets reallocated to a new channel.

When cpu0 passes the stale pointer to dma_pl330_rqcb(), it could acquire
the new channel's lock, incorrectly mark the new transfer as DONE, and
prematurely schedule the new channel's tasklet.

Could we cache the descriptors in local variables and set the array entries
to NULL while still holding pl330->lock, then call dma_pl330_rqcb() on the
local variables outside the lock?

>  
> +	spin_lock_irqsave(&pl330->lock, flags);
>  	_free_event(thrd, thrd->ev);
>  	thrd->free = true;
> +	spin_unlock_irqrestore(&pl330->lock, flags);
>  }
[ ... ]
> @@ -2358,9 +2374,15 @@ static void pl330_free_chan_resources(struct dma_chan *chan)
>  	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);

[Severity: High]
Can calling pl330_release_channel() here cause the tasklet to be re-armed
after it was killed, leading to a NULL pointer dereference?

When a client calls dma_release_channel() on an active channel without first
calling dmaengine_terminate_sync(), the dma core invokes
pl330_free_chan_resources(). This first kills the tasklet, then invokes
pl330_release_channel(pch->thread).

Because there are active descriptors, pl330_release_channel() aborts them
via dma_pl330_rqcb():

drivers/dma/pl330.c:dma_pl330_rqcb() {
    ...
    desc->status = DONE;
    spin_unlock_irqrestore(&pch->lock, flags);

    tasklet_schedule(&pch->task);
}

This unconditionally re-arms the tasklet after it was killed.

Later in pl330_free_chan_resources(), pch->thread is set to NULL. When the
re-armed tasklet executes pl330_tasklet():

drivers/dma/pl330.c:pl330_tasklet() {
    ...
    if (list_empty(&pch->work_list)) {
        spin_lock(&pch->thread->dmac->lock);
        _stop(pch->thread);
    ...
}

Would this dereference the NULL pch->thread?

> +
> +	spin_lock_irqsave(&pl330->lock, flags);
>  	pch->thread = NULL;

[Severity: Critical]
Does setting pch->thread to NULL here while the lock is dropped around
pl330_release_channel() introduce a NULL pointer dereference in
pl330_terminate_all() or pl330_pause()?

The commit notes that pl330_free_chan_resources() and pl330_terminate_all()
can be called concurrently on the same channel. By dropping pl330->lock
around pl330_release_channel(), it is now possible for cpu2 running
pl330_free_chan_resources() to completely finish pl330_release_channel()
and set pch->thread = NULL while cpu1 running pl330_terminate_all() is
blocked waiting to acquire pl330->lock.

Once cpu1 acquires pl330->lock, it blindly dereferences pch->thread:

drivers/dma/pl330.c:pl330_terminate_all() {
    ...
    spin_lock(&pl330->lock);
    _stop(pch->thread);
    pch->thread->req[0].desc = NULL;
    ...
}

Because pch->thread was set to NULL by cpu2, would this cause an immediate
kernel crash? This seems to trade a deadlock for a NULL pointer dereference
under the same concurrency conditions.

>  
>  	if (pch->cyclic)

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261007150104.38253-1-ginger.jzllee@gmail.com?part=1

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

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

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

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

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

[Frank Li]

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