[PATCH v2 0/6] plx_dma fixes

[PATCH v2 0/6] plx_dma fixes

[Logan Gunthorpe]

This is v2 from my series last week. It fixes a bug Sashiko found,
updates the commit message of the dma_rmb() patch to better explain
the issue, collects Frank's Reviewed-by tags and rebases on the latest
rc2.

Also, this is series isn't high priority to me, I'm just trying to
fix Sashiko warnings. If there's spare review bandwidth I'd prefer
the time spent on my other series[1].

This series is based off of v7.4-rc2.

Thanks,

Logan

[v1] https://lore.kernel.org/all/20261103155202.461725-1-logang@deltatee.com/
[1]  https://lore.kernel.org/all/20261110173137.623764-1-logang@deltatee.com/

--

Changes since v1:
  * Serialize descriptor completion with a new complete_lock so
    callbacks stay in order and can't still be running after
    plx_dma_free_chan_resources() returns (per Sashiko)
  * Expand the commit message and comment for the dma_rmb() patch to
    explain why the barrier is needed (per Frank)
  * Collect Frank's Reviewed-by tags
  * Rebased onto v7.4-rc2 (no conflicts)

Logan Gunthorpe (6):
  dmaengine: plx_dma: don't call pci_irq_vector() under rcu_read_lock()
  dmaengine: plx_dma: fix descriptor ring leak in alloc_chan_resources()
  dmaengine: plx_dma: use writel() for the 32-bit PLX_REG_CTRL register
  dmaengine: plx_dma: add dma_rmb() after observing write-back status
  dmaengine: plx_dma: use spin_lock_bh() in plx_dma_process_desc()
  dmaengine: plx_dma: don't invoke client callbacks with ring_lock held

 drivers/dma/plx_dma.c | 94 ++++++++++++++++++++++++++++++++-----------
 1 file changed, 70 insertions(+), 24 deletions(-)


base-commit: 7475ea8b58616b33c04afb976cf0fbaad9dd63fb
-- 
2.47.3

[PATCH v2 1/6] dmaengine: plx_dma: don't call pci_irq_vector() under rcu_read_lock()

[Logan Gunthorpe]

plx_dma_free_chan_resources() calls pci_irq_vector() inside an RCU
read-side section, but msi_domain_get_virq() takes the mutex that
protects the device's MSI descriptors. Releasing a channel, for example
at the end of a dmatest run, triggers:

  BUG: sleeping function called from invalid context at kernel/locking/mutex.c:623
  ...
  RCU nest depth: 1, expected: 0
  Call Trace:
   __mutex_lock+0x113/0x2350
   msi_domain_get_virq+0x227/0x2f0
   pci_irq_vector+0x9a/0xc0
   plx_dma_free_chan_resources+0x178/0x58f [plx_dma]
   dma_chan_put+0x157/0x380
   dma_release_channel+0xac/0x2e0

Store the IRQ number at probe time and use it everywhere
pci_irq_vector() was called, so the teardown path no longer needs to
look it up.

Fixes: c2dbcaa8c672 ("dmaengine: plx-dma: Implement hardware initialization and cleanup")
Reviewed-by: Frank Li <Frank.Li@nxp.com>
Signed-off-by: Logan Gunthorpe <logang@deltatee.com>
---
 drivers/dma/plx_dma.c | 16 ++++++++--------
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/drivers/dma/plx_dma.c b/drivers/dma/plx_dma.c
index 409898e92c32..0a7ad2473cb1 100644
--- a/drivers/dma/plx_dma.c
+++ b/drivers/dma/plx_dma.c
@@ -104,6 +104,7 @@ struct plx_dma_dev {
 	struct dma_device dma_dev;
 	struct dma_chan dma_chan;
 	struct pci_dev __rcu *pdev;
+	int irq;
 	void __iomem *bar;
 	struct tasklet_struct desc_task;
 
@@ -452,7 +453,6 @@ static void plx_dma_free_chan_resources(struct dma_chan *chan)
 {
 	struct plx_dma_dev *plxdev = chan_to_plx_dma_dev(chan);
 	size_t ring_sz = PLX_DMA_RING_COUNT * sizeof(*plxdev->hw_ring);
-	struct pci_dev *pdev;
 	int irq = -1;
 	int i;
 
@@ -463,9 +463,8 @@ static void plx_dma_free_chan_resources(struct dma_chan *chan)
 	plx_dma_stop(plxdev);
 
 	rcu_read_lock();
-	pdev = rcu_dereference(plxdev->pdev);
-	if (pdev)
-		irq = pci_irq_vector(pdev, 0);
+	if (rcu_dereference(plxdev->pdev))
+		irq = plxdev->irq;
 	rcu_read_unlock();
 
 	if (irq > 0)
@@ -510,8 +509,9 @@ static int plx_dma_create(struct pci_dev *pdev)
 	RCU_INIT_POINTER(plxdev->pdev, pdev);
 	plxdev->bar = pcim_iomap_table(pdev)[0];
 
-	rc = request_irq(pci_irq_vector(pdev, 0), plx_dma_isr, 0,
-			 KBUILD_MODNAME, plxdev);
+	plxdev->irq = pci_irq_vector(pdev, 0);
+
+	rc = request_irq(plxdev->irq, plx_dma_isr, 0, KBUILD_MODNAME, plxdev);
 	if (rc)
 		goto free_plx;
 
@@ -545,7 +545,7 @@ static int plx_dma_create(struct pci_dev *pdev)
 
 put_device:
 	put_device(&pdev->dev);
-	free_irq(pci_irq_vector(pdev, 0),  plxdev);
+	free_irq(plxdev->irq, plxdev);
 free_plx:
 	kfree(plxdev);
 
@@ -594,7 +594,7 @@ static void plx_dma_remove(struct pci_dev *pdev)
 {
 	struct plx_dma_dev *plxdev = pci_get_drvdata(pdev);
 
-	free_irq(pci_irq_vector(pdev, 0),  plxdev);
+	free_irq(plxdev->irq, plxdev);
 
 	rcu_assign_pointer(plxdev->pdev, NULL);
 	synchronize_rcu();
-- 
2.47.3

[PATCH v2 2/6] dmaengine: plx_dma: fix descriptor ring leak in alloc_chan_resources()

[Logan Gunthorpe]

If the device is removed between plx_dma_alloc_desc() and the pdev
check, the error path frees hw_ring but leaks desc_ring and every
descriptor in it.

Add plx_dma_free_desc() and use it here and in
plx_dma_free_chan_resources().

Reported-by: Sashiko <sashiko-bot@kernel.org>
Link: https://lore.kernel.org/dmaengine/20260717223217.B6FBF1F000E9@smtp.kernel.org
Reviewed-by: Frank Li <Frank.Li@nxp.com>
Signed-off-by: Logan Gunthorpe <logang@deltatee.com>
---
 drivers/dma/plx_dma.c | 20 ++++++++++++++------
 1 file changed, 14 insertions(+), 6 deletions(-)

diff --git a/drivers/dma/plx_dma.c b/drivers/dma/plx_dma.c
index 0a7ad2473cb1..bfccbb0c70e8 100644
--- a/drivers/dma/plx_dma.c
+++ b/drivers/dma/plx_dma.c
@@ -404,6 +404,16 @@ static int plx_dma_alloc_desc(struct plx_dma_dev *plxdev)
 	return -ENOMEM;
 }
 
+static void plx_dma_free_desc(struct plx_dma_dev *plxdev)
+{
+	int i;
+
+	for (i = 0; i < PLX_DMA_RING_COUNT; i++)
+		kfree(plxdev->desc_ring[i]);
+
+	kfree(plxdev->desc_ring);
+}
+
 static int plx_dma_alloc_chan_resources(struct dma_chan *chan)
 {
 	struct plx_dma_dev *plxdev = chan_to_plx_dma_dev(chan);
@@ -424,7 +434,7 @@ static int plx_dma_alloc_chan_resources(struct dma_chan *chan)
 	if (!rcu_dereference(plxdev->pdev)) {
 		rcu_read_unlock();
 		rc = -ENODEV;
-		goto out_free_hw_ring;
+		goto out_free_desc;
 	}
 
 	writel(PLX_REG_CTRL_RESET_VAL, plxdev->bar + PLX_REG_CTRL);
@@ -443,6 +453,8 @@ static int plx_dma_alloc_chan_resources(struct dma_chan *chan)
 
 	return PLX_DMA_RING_COUNT;
 
+out_free_desc:
+	plx_dma_free_desc(plxdev);
 out_free_hw_ring:
 	dma_free_coherent(plxdev->dma_dev.dev, ring_sz, plxdev->hw_ring,
 			  plxdev->hw_ring_dma);
@@ -454,7 +466,6 @@ static void plx_dma_free_chan_resources(struct dma_chan *chan)
 	struct plx_dma_dev *plxdev = chan_to_plx_dma_dev(chan);
 	size_t ring_sz = PLX_DMA_RING_COUNT * sizeof(*plxdev->hw_ring);
 	int irq = -1;
-	int i;
 
 	spin_lock_bh(&plxdev->ring_lock);
 	plxdev->ring_active = false;
@@ -474,10 +485,7 @@ static void plx_dma_free_chan_resources(struct dma_chan *chan)
 
 	plx_dma_abort_desc(plxdev);
 
-	for (i = 0; i < PLX_DMA_RING_COUNT; i++)
-		kfree(plxdev->desc_ring[i]);
-
-	kfree(plxdev->desc_ring);
+	plx_dma_free_desc(plxdev);
 	dma_free_coherent(plxdev->dma_dev.dev, ring_sz, plxdev->hw_ring,
 			  plxdev->hw_ring_dma);
 
-- 
2.47.3

[PATCH v2 3/6] dmaengine: plx_dma: use writel() for the 32-bit PLX_REG_CTRL register

[Logan Gunthorpe]

plx_dma_issue_pending() writes PLX_REG_CTRL with writew(), but it is a
32-bit register and every other access uses writel()/readl().

Reported-by: Sashiko <sashiko-bot@kernel.org>
Link: https://lore.kernel.org/dmaengine/20260717223217.B6FBF1F000E9@smtp.kernel.org
Reviewed-by: Frank Li <Frank.Li@nxp.com>
Signed-off-by: Logan Gunthorpe <logang@deltatee.com>
---
 drivers/dma/plx_dma.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/dma/plx_dma.c b/drivers/dma/plx_dma.c
index bfccbb0c70e8..1c995f0c4463 100644
--- a/drivers/dma/plx_dma.c
+++ b/drivers/dma/plx_dma.c
@@ -351,7 +351,7 @@ static void plx_dma_issue_pending(struct dma_chan *chan)
 	 */
 	wmb();
 
-	writew(PLX_REG_CTRL_START_VAL, plxdev->bar + PLX_REG_CTRL);
+	writel(PLX_REG_CTRL_START_VAL, plxdev->bar + PLX_REG_CTRL);
 
 	rcu_read_unlock();
 }
-- 
2.47.3

[PATCH v2 4/6] dmaengine: plx_dma: add dma_rmb() after observing write-back status

[Logan Gunthorpe]

Completing a descriptor involves two loads that must be ordered:

1. Reading desc->hw->flags_and_size and checking that VALID is clear.
2. Reading the destination buffer, which may happen in:
   a. dma_unmap_page() via dmaengine_unmap(), when a bounce buffer
      is in use.
   b. The client's callback, or its processing after polling with
      tx_status().

On a weakly ordered CPU like arm64, nothing forces load 2 to happen
after load 1, even though load 2 is only reached when load 1 says the
transfer is done. The CPU can predict that the VALID check won't
break out of the loop and issue load 2 early. If the device hadn't
finished writing at that moment, load 2 returns stale data, and when
load 1 then confirms the prediction nothing is redone, so the stale
data is used. This is the load-load control dependency case described
in Documentation/memory-barriers.txt.

Add a dma_rmb() after the status check so loads after it can't be
satisfied before the status read.

Reported-by: Sashiko <sashiko-bot@kernel.org>
Link: https://lore.kernel.org/dmaengine/20260717223217.B6FBF1F000E9@smtp.kernel.org
Signed-off-by: Logan Gunthorpe <logang@deltatee.com>
---
 drivers/dma/plx_dma.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/dma/plx_dma.c b/drivers/dma/plx_dma.c
index 1c995f0c4463..83c920eb173d 100644
--- a/drivers/dma/plx_dma.c
+++ b/drivers/dma/plx_dma.c
@@ -148,6 +148,13 @@ static void plx_dma_process_desc(struct plx_dma_dev *plxdev)
 		if (flags & PLX_DESC_FLAG_VALID)
 			break;
 
+		/*
+		 * Don't allow any read of the data the device wrote to be
+		 * performed before the status read above shows the
+		 * descriptor is complete.
+		 */
+		dma_rmb();
+
 		res.residue = desc->orig_size - (flags & PLX_DESC_SIZE_MASK);
 
 		if (flags & PLX_DESC_WB_SUCCESS)
-- 
2.47.3

[PATCH v2 5/6] dmaengine: plx_dma: use spin_lock_bh() in plx_dma_process_desc()

[Logan Gunthorpe]

plx_dma_process_desc() is also called from plx_dma_tx_status() in
process context. With a plain spin_lock(), the desc_task tasklet can
run on the same CPU while the lock is held, and spin on it forever.

Reported-by: Sashiko <sashiko-bot@kernel.org>
Link: https://lore.kernel.org/dmaengine/20260717223217.B6FBF1F000E9@smtp.kernel.org
Reviewed-by: Frank Li <Frank.Li@nxp.com>
Signed-off-by: Logan Gunthorpe <logang@deltatee.com>
---
 drivers/dma/plx_dma.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/dma/plx_dma.c b/drivers/dma/plx_dma.c
index 83c920eb173d..d1bdff1b7310 100644
--- a/drivers/dma/plx_dma.c
+++ b/drivers/dma/plx_dma.c
@@ -138,7 +138,7 @@ static void plx_dma_process_desc(struct plx_dma_dev *plxdev)
 	struct plx_dma_desc *desc;
 	u32 flags;
 
-	spin_lock(&plxdev->ring_lock);
+	spin_lock_bh(&plxdev->ring_lock);
 
 	while (plxdev->tail != plxdev->head) {
 		desc = plx_dma_get_desc(plxdev, plxdev->tail);
@@ -173,7 +173,7 @@ static void plx_dma_process_desc(struct plx_dma_dev *plxdev)
 		plxdev->tail++;
 	}
 
-	spin_unlock(&plxdev->ring_lock);
+	spin_unlock_bh(&plxdev->ring_lock);
 }
 
 static void plx_dma_abort_desc(struct plx_dma_dev *plxdev)
-- 
2.47.3

[PATCH v2 6/6] dmaengine: plx_dma: don't invoke client callbacks with ring_lock held

[Logan Gunthorpe]

plx_dma_process_desc() and plx_dma_abort_desc() invoke the client's
completion callback with ring_lock held. A callback that submits a new
transfer, which is common, deadlocks taking the same lock in
plx_dma_prep_memcpy().

Copy and clear the callback and advance tail under ring_lock, then drop
it to invoke the callback. To keep callbacks in order and ensure
plx_dma_free_chan_resources() can't return while one is still running
from plx_dma_remove(), serialize completion with a new complete_lock
held across the whole loop. plx_dma_tx_status() only trylocks it, so a
callback that polls the channel won't deadlock.

Reported-by: Sashiko <sashiko-bot@kernel.org>
Link: https://lore.kernel.org/dmaengine/20260717223217.B6FBF1F000E9@smtp.kernel.org
Signed-off-by: Logan Gunthorpe <logang@deltatee.com>
---
 drivers/dma/plx_dma.c | 45 ++++++++++++++++++++++++++++++++++++-------
 1 file changed, 38 insertions(+), 7 deletions(-)

diff --git a/drivers/dma/plx_dma.c b/drivers/dma/plx_dma.c
index d1bdff1b7310..394ca5483434 100644
--- a/drivers/dma/plx_dma.c
+++ b/drivers/dma/plx_dma.c
@@ -108,6 +108,8 @@ struct plx_dma_dev {
 	void __iomem *bar;
 	struct tasklet_struct desc_task;
 
+	/* serializes descriptor completion and callback invocation */
+	spinlock_t complete_lock;
 	spinlock_t ring_lock;
 	bool ring_active;
 	int head;
@@ -132,12 +134,15 @@ static struct plx_dma_desc *plx_dma_get_desc(struct plx_dma_dev *plxdev, int i)
 	return plxdev->desc_ring[i & (PLX_DMA_RING_COUNT - 1)];
 }
 
-static void plx_dma_process_desc(struct plx_dma_dev *plxdev)
+static void plx_dma_process_desc_unlocked(struct plx_dma_dev *plxdev)
 {
+	struct dmaengine_desc_callback cb;
 	struct dmaengine_result res;
 	struct plx_dma_desc *desc;
 	u32 flags;
 
+	lockdep_assert_held(&plxdev->complete_lock);
+
 	spin_lock_bh(&plxdev->ring_lock);
 
 	while (plxdev->tail != plxdev->head) {
@@ -166,22 +171,35 @@ static void plx_dma_process_desc(struct plx_dma_dev *plxdev)
 
 		dma_cookie_complete(&desc->txd);
 		dma_descriptor_unmap(&desc->txd);
-		dmaengine_desc_get_callback_invoke(&desc->txd, &res);
+		dmaengine_desc_get_callback(&desc->txd, &cb);
 		desc->txd.callback = NULL;
 		desc->txd.callback_result = NULL;
-
 		plxdev->tail++;
+
+		spin_unlock_bh(&plxdev->ring_lock);
+		dmaengine_desc_callback_invoke(&cb, &res);
+		spin_lock_bh(&plxdev->ring_lock);
 	}
 
 	spin_unlock_bh(&plxdev->ring_lock);
 }
 
+static void plx_dma_process_desc(struct plx_dma_dev *plxdev)
+{
+	spin_lock_bh(&plxdev->complete_lock);
+	plx_dma_process_desc_unlocked(plxdev);
+	spin_unlock_bh(&plxdev->complete_lock);
+}
+
 static void plx_dma_abort_desc(struct plx_dma_dev *plxdev)
 {
+	struct dmaengine_desc_callback cb;
 	struct dmaengine_result res;
 	struct plx_dma_desc *desc;
 
-	plx_dma_process_desc(plxdev);
+	spin_lock_bh(&plxdev->complete_lock);
+
+	plx_dma_process_desc_unlocked(plxdev);
 
 	spin_lock_bh(&plxdev->ring_lock);
 
@@ -193,14 +211,18 @@ static void plx_dma_abort_desc(struct plx_dma_dev *plxdev)
 
 		dma_cookie_complete(&desc->txd);
 		dma_descriptor_unmap(&desc->txd);
-		dmaengine_desc_get_callback_invoke(&desc->txd, &res);
+		dmaengine_desc_get_callback(&desc->txd, &cb);
 		desc->txd.callback = NULL;
 		desc->txd.callback_result = NULL;
-
 		plxdev->tail++;
+
+		spin_unlock_bh(&plxdev->ring_lock);
+		dmaengine_desc_callback_invoke(&cb, &res);
+		spin_lock_bh(&plxdev->ring_lock);
 	}
 
 	spin_unlock_bh(&plxdev->ring_lock);
+	spin_unlock_bh(&plxdev->complete_lock);
 }
 
 static void __plx_dma_stop(struct plx_dma_dev *plxdev)
@@ -337,7 +359,15 @@ static enum dma_status plx_dma_tx_status(struct dma_chan *chan,
 	if (ret == DMA_COMPLETE)
 		return ret;
 
-	plx_dma_process_desc(plxdev);
+	/*
+	 * If another context is already completing descriptors (possibly
+	 * a callback on this CPU that is polling the channel), let it
+	 * finish rather than waiting on it.
+	 */
+	if (spin_trylock_bh(&plxdev->complete_lock)) {
+		plx_dma_process_desc_unlocked(plxdev);
+		spin_unlock_bh(&plxdev->complete_lock);
+	}
 
 	return dma_cookie_status(chan, cookie, txstate);
 }
@@ -518,6 +548,7 @@ static int plx_dma_create(struct pci_dev *pdev)
 	if (!plxdev)
 		return -ENOMEM;
 
+	spin_lock_init(&plxdev->complete_lock);
 	spin_lock_init(&plxdev->ring_lock);
 	tasklet_setup(&plxdev->desc_task, plx_dma_desc_task);
 
-- 
2.47.3