[PATCH v2 0/5] dmaengine: ti: omap-dma: various bug fixes

[PATCH v2 0/5] dmaengine: ti: omap-dma: various bug fixes

[Rosen Penev]

This series fixes four bugs in the TI OMAP DMA driver:

 - Patch 1: add missing return statement in the probe error path
 - Patch 2: fix a notifier leak in remove
 - Patch 3: fix dma_pool_destroy being called before omap_dma_free
 - Patch 4: fix interrupt handling in the remove path
 - Patch 5: use dmam for dmaengine registration and remove devm_free_irq

v2: fix sashiko comments and add extra patch

Rosen Penev (5):
  dmaengine: ti: omap-dma: fix missing return in probe error path
  dmaengine: ti: omap-dma: fix notifier leak in remove
  dmaengine: ti: omap-dma: fix dma_pool_destroy before omap_dma_free in
    error paths
  dmaengine: ti: omap-dma: fix interrupt handling in remove
  dmaengine: ti: omap-dma: use devm for dmaengine registration

 drivers/dma/ti/omap-dma.c | 50 ++++++++++++++++++++++++++-------------
 1 file changed, 33 insertions(+), 17 deletions(-)

--
2.54.0

[PATCH v2 1/5] dmaengine: ti: omap-dma: fix missing return in probe error path

[Rosen Penev]

If of_dma_controller_register() fails, the error path omits the return
statement, causing probe to continue (and eventually succeed) despite
the DMA controller not being registered. Add the missing return rc;.

Fixes: 2e1136acf8a8 ("dmaengine: omap-dma: fix dma_pool resource leak in error paths")
Cc: stable@vger.kernel.org
Assisted-by: Opencode:BigPickle
Signed-off-by: Rosen Penev <rosenp@gmail.com>
---
 drivers/dma/ti/omap-dma.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/dma/ti/omap-dma.c b/drivers/dma/ti/omap-dma.c
index 55ece7fd0d99..0f6dd6b0a301 100644
--- a/drivers/dma/ti/omap-dma.c
+++ b/drivers/dma/ti/omap-dma.c
@@ -1828,6 +1828,7 @@ static int omap_dma_probe(struct platform_device *pdev)
 			if (od->ll123_supported)
 				dma_pool_destroy(od->desc_pool);
 			omap_dma_free(od);
+			return rc;
 		}
 	}
 
-- 
2.54.0

[PATCH v2 2/5] dmaengine: ti: omap-dma: fix notifier leak in remove

[Rosen Penev]

The notifier may be registered for needs_busy_check (omap2420) rather than
may_lose_context (omap3). The remove path only checks may_lose_context,
leaving the notifier registered during driver removal. Check both flags.

Fixes: 2e1136acf8a8 ("dmaengine: omap-dma: fix dma_pool resource leak in error paths")
Cc: stable@vger.kernel.org
Assisted-by: Opencode:BigPickle
Signed-off-by: Rosen Penev <rosenp@gmail.com>
---
 drivers/dma/ti/omap-dma.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/dma/ti/omap-dma.c b/drivers/dma/ti/omap-dma.c
index 0f6dd6b0a301..839e04f53fc2 100644
--- a/drivers/dma/ti/omap-dma.c
+++ b/drivers/dma/ti/omap-dma.c
@@ -1853,7 +1853,7 @@ static void omap_dma_remove(struct platform_device *pdev)
 	struct omap_dmadev *od = platform_get_drvdata(pdev);
 	int irq;
 
-	if (od->cfg->may_lose_context)
+	if (od->cfg->needs_busy_check || od->cfg->may_lose_context)
 		cpu_pm_unregister_notifier(&od->nb);
 
 	if (pdev->dev.of_node)
-- 
2.54.0

[PATCH v2 3/5] dmaengine: ti: omap-dma: fix dma_pool_destroy before omap_dma_free in error paths

[Rosen Penev]

The probe error paths and remove path called omap_dma_free() after
dma_pool_destroy(), leaving the desc_pool dangling while freeing
channel descriptors that may reference it.  Reorder so omap_dma_free()
comes first.

While here, fix two additional pre-existing issues:

- The probe error paths that run after the IRQ handler is registered
  did not disable hardware interrupts before freeing channels.  If an
  IRQ fires concurrently, the handler could access freed channel
  memory via lch_map[].  Disable IRQENABLE_L1 and clear
  irq_enable_mask under the spinlock, then readback the register to
  flush the posted write, before calling omap_dma_free().

- omap_dma_free() did not drain the virt-dma descriptor lists
  (desc_allocated, desc_submitted, desc_issued, desc_completed) before
  kfree() of the channel.  This leaked pending descriptors when the
  error or remove path tore down channels without going through
  omap_dma_free_chan_resources().  Call vchan_free_chan_resources()
  before freeing the channel structure.

Fixes: 2e1136acf8a8 ("dmaengine: omap-dma: fix dma_pool resource leak in error paths")
Cc: stable@vger.kernel.org
Assisted-by: Opencode:BigPickle
Signed-off-by: Rosen Penev <rosenp@gmail.com>
---
 drivers/dma/ti/omap-dma.c | 19 +++++++++++++++----
 1 file changed, 15 insertions(+), 4 deletions(-)

diff --git a/drivers/dma/ti/omap-dma.c b/drivers/dma/ti/omap-dma.c
index 839e04f53fc2..dde270646bb9 100644
--- a/drivers/dma/ti/omap-dma.c
+++ b/drivers/dma/ti/omap-dma.c
@@ -1522,6 +1522,7 @@ static void omap_dma_free(struct omap_dmadev *od)
 
 		list_del(&c->vc.chan.device_node);
 		tasklet_kill(&c->vc.task);
+		vchan_free_chan_resources(&c->vc);
 		kfree(c);
 	}
 }
@@ -1808,9 +1809,14 @@ static int omap_dma_probe(struct platform_device *pdev)
 	if (rc) {
 		pr_warn("OMAP-DMA: failed to register slave DMA engine device: %d\n",
 			rc);
+		spin_lock_irq(&od->irq_lock);
+		od->irq_enable_mask = 0;
+		omap_dma_glbl_write(od, IRQENABLE_L1, 0);
+		spin_unlock_irq(&od->irq_lock);
+		omap_dma_glbl_read(od, IRQENABLE_L1);
+		omap_dma_free(od);
 		if (od->ll123_supported)
 			dma_pool_destroy(od->desc_pool);
-		omap_dma_free(od);
 		return rc;
 	}
 
@@ -1825,9 +1831,14 @@ static int omap_dma_probe(struct platform_device *pdev)
 		if (rc) {
 			pr_warn("OMAP-DMA: failed to register DMA controller\n");
 			dma_async_device_unregister(&od->ddev);
+			spin_lock_irq(&od->irq_lock);
+			od->irq_enable_mask = 0;
+			omap_dma_glbl_write(od, IRQENABLE_L1, 0);
+			spin_unlock_irq(&od->irq_lock);
+			omap_dma_glbl_read(od, IRQENABLE_L1);
+			omap_dma_free(od);
 			if (od->ll123_supported)
 				dma_pool_destroy(od->desc_pool);
-			omap_dma_free(od);
 			return rc;
 		}
 	}
@@ -1869,10 +1880,10 @@ static void omap_dma_remove(struct platform_device *pdev)
 		omap_dma_glbl_write(od, IRQENABLE_L0, 0);
 	}
 
+	omap_dma_free(od);
+
 	if (od->ll123_supported)
 		dma_pool_destroy(od->desc_pool);
-
-	omap_dma_free(od);
 }
 
 static const struct omap_dma_config omap2420_data = {
-- 
2.54.0

[PATCH v2 4/5] dmaengine: ti: omap-dma: fix interrupt handling in remove

[Rosen Penev]

The remove path had several pre-existing bugs:

1. Interrupts are enabled via IRQENABLE_L1 in probe and alloc_chan_resources,
   but the remove path writes to IRQENABLE_L0, which has no effect on the L1
   interrupt line. The DMA engine can continue asserting its IRQ during
   removal. Write to IRQENABLE_L1 instead.

2. devm_free_irq() was called before disabling hardware interrupts. With
   IRQF_SHARED, the hardware may still assert the IRQ line after the handler
   is freed, causing unhandled interrupts that can lead to the kernel
   permanently disabling the shared IRQ line. Disable interrupts first.

3. platform_get_irq() return value was not checked before devm_free_irq().
   If it returns an error code (<= 0), passing it to devm_free_irq() is
   incorrect. Add a guard.

4. Clearing od->irq_enable_mask and writing to IRQENABLE_L1 raced with the
   interrupt handler, which reads irq_enable_mask under the spinlock.
   Hold irq_lock around the disable.

5. The posted write to IRQENABLE_L1 used _relaxed accessors with no
   readback to drain the write buffer. Add a readback flush before
   devm_free_irq() to ensure the hardware has actually disabled the
   interrupt line.

6. omap_dma_free() unconditionally freed all channel memory without
   checking whether clients still held references. A sysfs unbind of the
   DMA controller does not synchronously unbind consumers, so active
   clients could access freed channel memory. Skip freeing channels
   that still have active clients.

Fixes: 2e1136acf8a8 ("dmaengine: omap-dma: fix dma_pool resource leak in error paths")
Cc: stable@vger.kernel.org
Assisted-by: Opencode:BigPickle
Signed-off-by: Rosen Penev <rosenp@gmail.com>
---
 drivers/dma/ti/omap-dma.c | 26 +++++++++++++++++++-------
 1 file changed, 19 insertions(+), 7 deletions(-)

diff --git a/drivers/dma/ti/omap-dma.c b/drivers/dma/ti/omap-dma.c
index dde270646bb9..8c32b7ab50f6 100644
--- a/drivers/dma/ti/omap-dma.c
+++ b/drivers/dma/ti/omap-dma.c
@@ -1516,13 +1516,21 @@ static int omap_dma_chan_init(struct omap_dmadev *od)
 
 static void omap_dma_free(struct omap_dmadev *od)
 {
+	struct omap_chan *c;
+
 	while (!list_empty(&od->ddev.channels)) {
-		struct omap_chan *c = list_first_entry(&od->ddev.channels,
-			struct omap_chan, vc.chan.device_node);
+		c = list_first_entry(&od->ddev.channels,
+				     struct omap_chan, vc.chan.device_node);
 
 		list_del(&c->vc.chan.device_node);
 		tasklet_kill(&c->vc.task);
 		vchan_free_chan_resources(&c->vc);
+		if (c->vc.chan.client_count) {
+			dev_warn(od->ddev.dev,
+				 "chan%d freed with %u client(s)\n",
+				 c->dma_ch, c->vc.chan.client_count);
+			continue;
+		}
 		kfree(c);
 	}
 }
@@ -1870,16 +1878,20 @@ static void omap_dma_remove(struct platform_device *pdev)
 	if (pdev->dev.of_node)
 		of_dma_controller_free(pdev->dev.of_node);
 
-	irq = platform_get_irq(pdev, 1);
-	devm_free_irq(&pdev->dev, irq, od);
-
 	dma_async_device_unregister(&od->ddev);
 
 	if (!omap_dma_legacy(od)) {
-		/* Disable all interrupts */
-		omap_dma_glbl_write(od, IRQENABLE_L0, 0);
+		spin_lock_irq(&od->irq_lock);
+		od->irq_enable_mask = 0;
+		omap_dma_glbl_write(od, IRQENABLE_L1, 0);
+		spin_unlock_irq(&od->irq_lock);
+		omap_dma_glbl_read(od, IRQENABLE_L1);
 	}
 
+	irq = platform_get_irq(pdev, 1);
+	if (irq > 0)
+		devm_free_irq(&pdev->dev, irq, od);
+
 	omap_dma_free(od);
 
 	if (od->ll123_supported)
-- 
2.54.0

[PATCH v2 5/5] dmaengine: ti: omap-dma: use devm for dmaengine registration

[Rosen Penev]

Use dmaenginem_async_device_register() instead of
dma_async_device_register() so that unregistration is handled
automatically by devres on probe failure or driver detach. This
lets us drop:
- The explicit dma_async_device_unregister() in the
  of_dma_controller_register error path (devres unwind runs it
  after omap_dma_free() has emptied the channels list, making the
  iteration a safe no-op).
- The explicit dma_async_device_unregister() and the redundant
  devm_free_irq() call in the remove path (devres handles both in
  the correct order after the remove callback returns).

Assisted-by: Opencode:Big-Pickle
Signed-off-by: Rosen Penev <rosenp@gmail.com>
---
 drivers/dma/ti/omap-dma.c | 10 +---------
 1 file changed, 1 insertion(+), 9 deletions(-)

diff --git a/drivers/dma/ti/omap-dma.c b/drivers/dma/ti/omap-dma.c
index 8c32b7ab50f6..4bf34569d82b 100644
--- a/drivers/dma/ti/omap-dma.c
+++ b/drivers/dma/ti/omap-dma.c
@@ -1813,7 +1813,7 @@ static int omap_dma_probe(struct platform_device *pdev)
 		}
 	}
 
-	rc = dma_async_device_register(&od->ddev);
+	rc = dmaenginem_async_device_register(&od->ddev);
 	if (rc) {
 		pr_warn("OMAP-DMA: failed to register slave DMA engine device: %d\n",
 			rc);
@@ -1838,7 +1838,6 @@ static int omap_dma_probe(struct platform_device *pdev)
 				of_dma_simple_xlate, &omap_dma_info);
 		if (rc) {
 			pr_warn("OMAP-DMA: failed to register DMA controller\n");
-			dma_async_device_unregister(&od->ddev);
 			spin_lock_irq(&od->irq_lock);
 			od->irq_enable_mask = 0;
 			omap_dma_glbl_write(od, IRQENABLE_L1, 0);
@@ -1870,7 +1869,6 @@ static int omap_dma_probe(struct platform_device *pdev)
 static void omap_dma_remove(struct platform_device *pdev)
 {
 	struct omap_dmadev *od = platform_get_drvdata(pdev);
-	int irq;
 
 	if (od->cfg->needs_busy_check || od->cfg->may_lose_context)
 		cpu_pm_unregister_notifier(&od->nb);
@@ -1878,8 +1876,6 @@ static void omap_dma_remove(struct platform_device *pdev)
 	if (pdev->dev.of_node)
 		of_dma_controller_free(pdev->dev.of_node);
 
-	dma_async_device_unregister(&od->ddev);
-
 	if (!omap_dma_legacy(od)) {
 		spin_lock_irq(&od->irq_lock);
 		od->irq_enable_mask = 0;
@@ -1888,10 +1884,6 @@ static void omap_dma_remove(struct platform_device *pdev)
 		omap_dma_glbl_read(od, IRQENABLE_L1);
 	}
 
-	irq = platform_get_irq(pdev, 1);
-	if (irq > 0)
-		devm_free_irq(&pdev->dev, irq, od);
-
 	omap_dma_free(od);
 
 	if (od->ll123_supported)
-- 
2.54.0