On Thu, Jun 11, 2026 at 8:20 AM Frank Li [off-list ref] wrote:

On Wed, Jun 10, 2026 at 08:52:32PM -0700, Rosen Penev wrote:

quoted

fsldma_run_tx_complete_actions() calls dmaengine_desc_get_callback_invoke()
while still holding chan->desc_lock.  If the client submits a new
transaction from their completion callback, fsl_dma_tx_submit()
tries to acquire the same non-recursive spinlock, causing a
self-deadlock.

Fix by extracting the callback info under the lock, removing the
descriptor from ld_running, dropping the lock, then invoking the
callback and running dependencies outside the lock.

Assisted-by: opencode:big-pickle
Signed-off-by: Rosen Penev <redacted>
---
 drivers/dma/fsldma.c | 108 ++++++++++++++++++++++---------------------
 1 file changed, 55 insertions(+), 53 deletions(-)

diff --git a/drivers/dma/fsldma.c b/drivers/dma/fsldma.c
index 0e2f84862261..455d21d738de 100644
--- a/drivers/dma/fsldma.c
+++ b/drivers/dma/fsldma.c

@@ -496,16 +496,19 @@ static void fsldma_clean_completed_descriptor(struct fsldma_chan *chan)
 }

 /**
- * fsldma_run_tx_complete_actions - cleanup a single link descriptor
+ * fsldma_run_tx_complete_actions - unmap and extract callback from a descriptor
  * @chan: Freescale DMA channel
- * @desc: descriptor to cleanup and free
+ * @desc: descriptor to process
  * @cookie: Freescale DMA transaction identifier
+ * @cb: returned callback information
  *
- * This function is used on a descriptor which has been executed by the DMA
- * controller. It will run any callbacks, submit any dependencies.
+ * Unmap the descriptor if it has been submitted and extract its callback
+ * into @cb.  The caller must invoke the callback and run dependencies
+ * after releasing chan->desc_lock.
  */
 static dma_cookie_t fsldma_run_tx_complete_actions(struct fsldma_chan *chan,
-             struct fsl_desc_sw *desc, dma_cookie_t cookie)
+             struct fsl_desc_sw *desc, dma_cookie_t cookie,
+             struct dmaengine_desc_callback *cb)
 {
      struct dma_async_tx_descriptor *txd = &desc->async_tx;
      dma_cookie_t ret = cookie;

@@ -514,49 +517,14 @@ static dma_cookie_t fsldma_run_tx_complete_actions(struct fsldma_chan *chan,

      if (txd->cookie > 0) {
              ret = txd->cookie;
-
              dma_descriptor_unmap(txd);
-             /* Run the link descriptor callback function */
-             dmaengine_desc_get_callback_invoke(txd, NULL);
      }

-     /* Run any dependencies */
-     dma_run_dependencies(txd);
+     dmaengine_desc_get_callback(txd, cb);

      return ret;
 }

-/**
- * fsldma_clean_running_descriptor - move the completed descriptor from
- * ld_running to ld_completed
- * @chan: Freescale DMA channel
- * @desc: the descriptor which is completed
- *
- * Free the descriptor directly if acked by async_tx api, or move it to
- * queue ld_completed.
- */
-static void fsldma_clean_running_descriptor(struct fsldma_chan *chan,
-             struct fsl_desc_sw *desc)
-{
-     /* Remove from the list of transactions */
-     list_del(&desc->node);
-
-     /*
-      * the client is allowed to attach dependent operations
-      * until 'ack' is set
-      */
-     if (!async_tx_test_ack(&desc->async_tx)) {
-             /*
-              * Move this descriptor to the list of descriptors which is
-              * completed, but still awaiting the 'ack' bit to be set.
-              */
-             list_add_tail(&desc->node, &chan->ld_completed);
-             return;
-     }
-
-     dma_pool_free(chan->desc_pool, desc, desc->async_tx.phys);
-}
-
 /**
  * fsl_chan_xfer_ld_queue - transfer any pending transactions
  * @chan : Freescale DMA channel

@@ -635,22 +603,23 @@ static void fsl_chan_xfer_ld_queue(struct fsldma_chan *chan)
  */
 static void fsldma_cleanup_descriptors(struct fsldma_chan *chan)
 {
-     struct fsl_desc_sw *desc, *_desc;
+     struct fsl_desc_sw *desc;
      dma_cookie_t cookie = 0;
      dma_addr_t curr_phys = get_cdar(chan);
      int seen_current = 0;

      fsldma_clean_completed_descriptor(chan);

-     /* Run the callback for each descriptor, in order */
-     list_for_each_entry_safe(desc, _desc, &chan->ld_running, node) {
-             /*
-              * do not advance past the current descriptor loaded into the
-              * hardware channel, subsequent descriptors are either in
-              * process or have not been submitted
-              */
-             if (seen_current)
-                     break;
+     /*
+      * Take descriptors one at a time from the front of the running
+      * queue.  We re-read the list each iteration so that we don't
+      * chase a stale next pointer across the lock-drop below.
+      */
+     while (!seen_current && !list_empty(&chan->ld_running)) {
+             struct dmaengine_desc_callback cb;
+
+             desc = list_first_entry(&chan->ld_running,
+                                     struct fsl_desc_sw, node);

              /*
               * stop the search if we reach the current descriptor and the

@@ -662,9 +631,42 @@ static void fsldma_cleanup_descriptors(struct fsldma_chan *chan)
                              break;
              }

-             cookie = fsldma_run_tx_complete_actions(chan, desc, cookie);
+             cookie = fsldma_run_tx_complete_actions(chan, desc, cookie, &cb);

-             fsldma_clean_running_descriptor(chan, desc);
+             /*
+              * Remove from the running list before dropping the lock so
+              * that terminate_all cannot free this descriptor while we
+              * call into the client below.
+              */
+             list_del(&desc->node);
+
+             /*
+              * Prevent dma_run_dependencies() from calling
+              * fsl_chan_xfer_ld_queue() while we are not holding the
+              * lock.  That would splice pending descriptors into
+              * ld_running before they have been completed by hardware.
+              * fsl_chan_xfer_ld_queue at the end of this function will
+              * re-evaluate the situation.
+              */
+             chan->idle = false;
+
+             /*
+              * Drop the lock before invoking the client callback, since
+              * the DMAengine API explicitly allows clients to submit new
+              * transactions from their completion callback.  Otherwise
+              * we self-deadlock on chan->desc_lock.
+              */
+             spin_unlock(&chan->desc_lock);
+             dmaengine_desc_callback_invoke(&cb, NULL);
+             dma_run_dependencies(&desc->async_tx);
+             spin_lock(&chan->desc_lock);

Not sure if you have hardware to test it. This change is quite big. Generally,
keep desc_lock and move these complete queue,  defer to tasklet or workqueue
run callback in in complete queue by hold complete queue's lock.

I do not. And because it is quite big, I will drop it.

quoted

+
+             chan->idle = true;
+
+             if (!async_tx_test_ack(&desc->async_tx))
+                     list_add_tail(&desc->node, &chan->ld_completed);
+             else
+                     dma_pool_free(chan->desc_pool, desc, desc->async_tx.phys);

desc already removed from list, needn't hold desc_lock, you can move it
just before spin_lock()

You should use difference lock for ld_completed.

Frank

quoted

      }

      /*
--
2.54.0