[PATCH 0/7] dmaengine: zynqmp_dma: fix lockdep warning and cleanup

[PATCH 0/7] dmaengine: zynqmp_dma: fix lockdep warning and cleanup

[Michael Tretter]

Hi,

I reported a lockdep warning in the ZynqMP DMA driver a few weeks ago [0].
This series fixes the reported warning and performs some cleanup that I found
while looking at the driver more closely.

Patches 1-4 are the cleanups. They affect the log output of the driver, allow
to compile the driver on other platforms when COMPILE_TEST is enabled, and
remove unused included header files from the driver.

Patches 5-7 aim to fix the lockdep warning. Patch 5 and 6 restructure the
locking in the driver to make it more fine-grained instead of holding the lock
for the entire tasklet. Patch 7 finally fixes the warning.

Michael

[0] https://lore.kernel.org/linux-arm-kernel/20210601130108.GA12967@pengutronix.de/

Michael Tretter (7):
  dmaengine: zynqmp_dma: simplify with dev_err_probe
  dmaengine: zynqmp_dma: drop message on probe success
  dmaengine: zynqmp_dma: enable COMPILE_TEST
  dmaengine: zynqmp_dma: cleanup includes
  dmaengine: zynqmp_dma: cleanup after completing all descriptors
  dmaengine: zynqmp_dma: refine dma descriptor locking
  dmaengine: zynqmp_dma: fix lockdep warning in tasklet

 drivers/dma/Kconfig             |  2 +-
 drivers/dma/xilinx/zynqmp_dma.c | 67 +++++++++++++++++----------------
 2 files changed, 35 insertions(+), 34 deletions(-)

-- 
2.30.2

[PATCH 1/7] dmaengine: zynqmp_dma: simplify with dev_err_probe

[Michael Tretter]

The clocks are provided by the ZynqMP firmware driver and are deferred
until the firmware driver has probed. This leads to misleading error
messages during probe of the zynqmp_dma driver.

Use dev_err_probe for printing errors during probe to avoid error
messages for -EPROBE_DEFER.

Signed-off-by: Michael Tretter <m.tretter@pengutronix.de>
---
 drivers/dma/xilinx/zynqmp_dma.c | 18 ++++++++----------
 1 file changed, 8 insertions(+), 10 deletions(-)

diff --git a/drivers/dma/xilinx/zynqmp_dma.c b/drivers/dma/xilinx/zynqmp_dma.c
index 5fecf5aa6e85..8423fbfc20c7 100644
--- a/drivers/dma/xilinx/zynqmp_dma.c
+++ b/drivers/dma/xilinx/zynqmp_dma.c
@@ -1062,16 +1062,14 @@ static int zynqmp_dma_probe(struct platform_device *pdev)
 	p->dev = &pdev->dev;
 
 	zdev->clk_main = devm_clk_get(&pdev->dev, "clk_main");
-	if (IS_ERR(zdev->clk_main)) {
-		dev_err(&pdev->dev, "main clock not found.\n");
-		return PTR_ERR(zdev->clk_main);
-	}
+	if (IS_ERR(zdev->clk_main))
+		return dev_err_probe(&pdev->dev, PTR_ERR(zdev->clk_main),
+				     "main clock not found.\n");
 
 	zdev->clk_apb = devm_clk_get(&pdev->dev, "clk_apb");
-	if (IS_ERR(zdev->clk_apb)) {
-		dev_err(&pdev->dev, "apb clock not found.\n");
-		return PTR_ERR(zdev->clk_apb);
-	}
+	if (IS_ERR(zdev->clk_apb))
+		return dev_err_probe(&pdev->dev, PTR_ERR(zdev->clk_apb),
+				     "apb clock not found.\n");
 
 	platform_set_drvdata(pdev, zdev);
 	pm_runtime_set_autosuspend_delay(zdev->dev, ZDMA_PM_TIMEOUT);
@@ -1086,7 +1084,7 @@ static int zynqmp_dma_probe(struct platform_device *pdev)
 
 	ret = zynqmp_dma_chan_probe(zdev, pdev);
 	if (ret) {
-		dev_err(&pdev->dev, "Probing channel failed\n");
+		dev_err_probe(&pdev->dev, ret, "Probing channel failed\n");
 		goto err_disable_pm;
 	}
 
@@ -1098,7 +1096,7 @@ static int zynqmp_dma_probe(struct platform_device *pdev)
 	ret = of_dma_controller_register(pdev->dev.of_node,
 					 of_zynqmp_dma_xlate, zdev);
 	if (ret) {
-		dev_err(&pdev->dev, "Unable to register DMA to DT\n");
+		dev_err_probe(&pdev->dev, ret, "Unable to register DMA to DT\n");
 		dma_async_device_unregister(&zdev->common);
 		goto free_chan_resources;
 	}
-- 
2.30.2

[PATCH 2/7] dmaengine: zynqmp_dma: drop message on probe success

[Michael Tretter]

There is no need to print a message that the ZynqMP DMA driver probed
successfully, since it carries no additional information. Drop the
message.

Signed-off-by: Michael Tretter <m.tretter@pengutronix.de>
---
 drivers/dma/xilinx/zynqmp_dma.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/dma/xilinx/zynqmp_dma.c b/drivers/dma/xilinx/zynqmp_dma.c
index 8423fbfc20c7..2a8938806b91 100644
--- a/drivers/dma/xilinx/zynqmp_dma.c
+++ b/drivers/dma/xilinx/zynqmp_dma.c
@@ -1104,8 +1104,6 @@ static int zynqmp_dma_probe(struct platform_device *pdev)
 	pm_runtime_mark_last_busy(zdev->dev);
 	pm_runtime_put_sync_autosuspend(zdev->dev);
 
-	dev_info(&pdev->dev, "ZynqMP DMA driver Probe success\n");
-
 	return 0;
 
 free_chan_resources:
-- 
2.30.2

[PATCH 3/7] dmaengine: zynqmp_dma: enable COMPILE_TEST

[Michael Tretter]

The driver doesn't use anything architecture specific. Allow the
compilation on other architectures as well.

Signed-off-by: Michael Tretter <m.tretter@pengutronix.de>
---
 drivers/dma/Kconfig | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/dma/Kconfig b/drivers/dma/Kconfig
index 39b5b46e880f..20cab9612916 100644
--- a/drivers/dma/Kconfig
+++ b/drivers/dma/Kconfig
@@ -695,7 +695,7 @@ config XILINX_DMA
 
 config XILINX_ZYNQMP_DMA
 	tristate "Xilinx ZynqMP DMA Engine"
-	depends on (ARCH_ZYNQ || MICROBLAZE || ARM64)
+	depends on ARCH_ZYNQ || MICROBLAZE || ARM64 || COMPILE_TEST
 	select DMA_ENGINE
 	help
 	  Enable support for Xilinx ZynqMP DMA controller.
-- 
2.30.2

[PATCH 4/7] dmaengine: zynqmp_dma: cleanup includes

[Michael Tretter]

The driver includes a few headers that are not actually used, but are
probably copy paste errors. Remove them.

Signed-off-by: Michael Tretter <m.tretter@pengutronix.de>
---
 drivers/dma/xilinx/zynqmp_dma.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/drivers/dma/xilinx/zynqmp_dma.c b/drivers/dma/xilinx/zynqmp_dma.c
index 2a8938806b91..61bb1d7f6ec0 100644
--- a/drivers/dma/xilinx/zynqmp_dma.c
+++ b/drivers/dma/xilinx/zynqmp_dma.c
@@ -6,15 +6,12 @@
  */
 
 #include <linux/bitops.h>
-#include <linux/dmapool.h>
-#include <linux/dma/xilinx_dma.h>
+#include <linux/dma-mapping.h>
 #include <linux/init.h>
 #include <linux/interrupt.h>
 #include <linux/io.h>
 #include <linux/module.h>
-#include <linux/of_address.h>
 #include <linux/of_dma.h>
-#include <linux/of_irq.h>
 #include <linux/of_platform.h>
 #include <linux/slab.h>
 #include <linux/clk.h>
-- 
2.30.2

[PATCH 5/7] dmaengine: zynqmp_dma: cleanup after completing all descriptors

[Michael Tretter]

The current implementation iterates the entire done list for each
completed dma descriptor even if there are multiple completed
descriptors.

Avoid this by first moving all completed descriptors to the done list
and afterwards iterating the done list and finishing the descriptors.

Signed-off-by: Michael Tretter <m.tretter@pengutronix.de>
---
 drivers/dma/xilinx/zynqmp_dma.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/dma/xilinx/zynqmp_dma.c b/drivers/dma/xilinx/zynqmp_dma.c
index 61bb1d7f6ec0..f98ef5fe4902 100644
--- a/drivers/dma/xilinx/zynqmp_dma.c
+++ b/drivers/dma/xilinx/zynqmp_dma.c
@@ -759,10 +759,11 @@ static void zynqmp_dma_do_tasklet(struct tasklet_struct *t)
 
 	while (count) {
 		zynqmp_dma_complete_descriptor(chan);
-		zynqmp_dma_chan_desc_cleanup(chan);
 		count--;
 	}
 
+	zynqmp_dma_chan_desc_cleanup(chan);
+
 	if (chan->idle)
 		zynqmp_dma_start_transfer(chan);
 
-- 
2.30.2

[PATCH 6/7] dmaengine: zynqmp_dma: refine dma descriptor locking

[Michael Tretter]

The descriptor lists are locked for the entire tasklet that completes
the descriptors. This is not necessary, because the lock actually only
protects the descriptor lists.

Make the spin lock more fine-grained and only protect functions that
actually operate on the descriptor lists. This decreases the time when
the lock is held.

Signed-off-by: Michael Tretter <m.tretter@pengutronix.de>
---
 drivers/dma/xilinx/zynqmp_dma.c | 35 ++++++++++++++++++++-------------
 1 file changed, 21 insertions(+), 14 deletions(-)

diff --git a/drivers/dma/xilinx/zynqmp_dma.c b/drivers/dma/xilinx/zynqmp_dma.c
index f98ef5fe4902..ef32f7519e0a 100644
--- a/drivers/dma/xilinx/zynqmp_dma.c
+++ b/drivers/dma/xilinx/zynqmp_dma.c
@@ -601,6 +601,9 @@ static void zynqmp_dma_start_transfer(struct zynqmp_dma_chan *chan)
 static void zynqmp_dma_chan_desc_cleanup(struct zynqmp_dma_chan *chan)
 {
 	struct zynqmp_dma_desc_sw *desc, *next;
+	unsigned long irqflags;
+
+	spin_lock_irqsave(&chan->lock, irqflags);
 
 	list_for_each_entry_safe(desc, next, &chan->done_list, node) {
 		dma_async_tx_callback callback;
@@ -617,6 +620,8 @@ static void zynqmp_dma_chan_desc_cleanup(struct zynqmp_dma_chan *chan)
 		/* Run any dependencies, then free the descriptor */
 		zynqmp_dma_free_descriptor(chan, desc);
 	}
+
+	spin_unlock_irqrestore(&chan->lock, irqflags);
 }
 
 /**
@@ -656,9 +661,13 @@ static void zynqmp_dma_issue_pending(struct dma_chan *dchan)
  */
 static void zynqmp_dma_free_descriptors(struct zynqmp_dma_chan *chan)
 {
+	unsigned long irqflags;
+
+	spin_lock_irqsave(&chan->lock, irqflags);
 	zynqmp_dma_free_desc_list(chan, &chan->active_list);
 	zynqmp_dma_free_desc_list(chan, &chan->pending_list);
 	zynqmp_dma_free_desc_list(chan, &chan->done_list);
+	spin_unlock_irqrestore(&chan->lock, irqflags);
 }
 
 /**
@@ -668,11 +677,8 @@ static void zynqmp_dma_free_descriptors(struct zynqmp_dma_chan *chan)
 static void zynqmp_dma_free_chan_resources(struct dma_chan *dchan)
 {
 	struct zynqmp_dma_chan *chan = to_chan(dchan);
-	unsigned long irqflags;
 
-	spin_lock_irqsave(&chan->lock, irqflags);
 	zynqmp_dma_free_descriptors(chan);
-	spin_unlock_irqrestore(&chan->lock, irqflags);
 	dma_free_coherent(chan->dev,
 		(2 * ZYNQMP_DMA_DESC_SIZE(chan) * ZYNQMP_DMA_NUM_DESCS),
 		chan->desc_pool_v, chan->desc_pool_p);
@@ -687,11 +693,16 @@ static void zynqmp_dma_free_chan_resources(struct dma_chan *dchan)
  */
 static void zynqmp_dma_reset(struct zynqmp_dma_chan *chan)
 {
+	unsigned long irqflags;
+
 	writel(ZYNQMP_DMA_IDS_DEFAULT_MASK, chan->regs + ZYNQMP_DMA_IDS);
 
+	spin_lock_irqsave(&chan->lock, irqflags);
 	zynqmp_dma_complete_descriptor(chan);
+	spin_unlock_irqrestore(&chan->lock, irqflags);
 	zynqmp_dma_chan_desc_cleanup(chan);
 	zynqmp_dma_free_descriptors(chan);
+
 	zynqmp_dma_init(chan);
 }
 
@@ -747,28 +758,27 @@ static void zynqmp_dma_do_tasklet(struct tasklet_struct *t)
 	u32 count;
 	unsigned long irqflags;
 
-	spin_lock_irqsave(&chan->lock, irqflags);
-
 	if (chan->err) {
 		zynqmp_dma_reset(chan);
 		chan->err = false;
-		goto unlock;
+		return;
 	}
 
+	spin_lock_irqsave(&chan->lock, irqflags);
 	count = readl(chan->regs + ZYNQMP_DMA_IRQ_DST_ACCT);
-
 	while (count) {
 		zynqmp_dma_complete_descriptor(chan);
 		count--;
 	}
+	spin_unlock_irqrestore(&chan->lock, irqflags);
 
 	zynqmp_dma_chan_desc_cleanup(chan);
 
-	if (chan->idle)
+	if (chan->idle) {
+		spin_lock_irqsave(&chan->lock, irqflags);
 		zynqmp_dma_start_transfer(chan);
-
-unlock:
-	spin_unlock_irqrestore(&chan->lock, irqflags);
+		spin_unlock_irqrestore(&chan->lock, irqflags);
+	}
 }
 
 /**
@@ -780,12 +790,9 @@ static void zynqmp_dma_do_tasklet(struct tasklet_struct *t)
 static int zynqmp_dma_device_terminate_all(struct dma_chan *dchan)
 {
 	struct zynqmp_dma_chan *chan = to_chan(dchan);
-	unsigned long irqflags;
 
-	spin_lock_irqsave(&chan->lock, irqflags);
 	writel(ZYNQMP_DMA_IDS_DEFAULT_MASK, chan->regs + ZYNQMP_DMA_IDS);
 	zynqmp_dma_free_descriptors(chan);
-	spin_unlock_irqrestore(&chan->lock, irqflags);
 
 	return 0;
 }
-- 
2.30.2

[PATCH 7/7] dmaengine: zynqmp_dma: fix lockdep warning in tasklet

[Michael Tretter]

The tasklet that handles the completed dma transfers uses spin_unlock
for unlocking a spin lock that was previously locked with
spin_lock_irqsave.

This caused the following lockdep warning about an inconsistent lock
state:

	inconsistent {HARDIRQ-ON-W} -> {IN-HARDIRQ-W} usage.

We must use spin_lock_irqsave, because it is possible to queue DMA
transfers from an irq handler.

Replace the spin_unlock and spin_lock by spin_unlock_irqrestore and
spin_lock_irqsave.

Signed-off-by: Michael Tretter <m.tretter@pengutronix.de>
---
 drivers/dma/xilinx/zynqmp_dma.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/dma/xilinx/zynqmp_dma.c b/drivers/dma/xilinx/zynqmp_dma.c
index ef32f7519e0a..5f5f9e244ec6 100644
--- a/drivers/dma/xilinx/zynqmp_dma.c
+++ b/drivers/dma/xilinx/zynqmp_dma.c
@@ -612,9 +612,9 @@ static void zynqmp_dma_chan_desc_cleanup(struct zynqmp_dma_chan *chan)
 		callback = desc->async_tx.callback;
 		callback_param = desc->async_tx.callback_param;
 		if (callback) {
-			spin_unlock(&chan->lock);
+			spin_unlock_irqrestore(&chan->lock, irqflags);
 			callback(callback_param);
-			spin_lock(&chan->lock);
+			spin_lock_irqsave(&chan->lock, irqflags);
 		}
 
 		/* Run any dependencies, then free the descriptor */
-- 
2.30.2

Re: [PATCH 0/7] dmaengine: zynqmp_dma: fix lockdep warning and cleanup

[Michael Tretter]

On Thu, 26 Aug 2021 11:47:35 +0200, Michael Tretter wrote:
> I reported a lockdep warning in the ZynqMP DMA driver a few weeks ago [0].
> This series fixes the reported warning and performs some cleanup that I found
> while looking at the driver more closely.
> 
> Patches 1-4 are the cleanups. They affect the log output of the driver, allow
> to compile the driver on other platforms when COMPILE_TEST is enabled, and
> remove unused included header files from the driver.
> 
> Patches 5-7 aim to fix the lockdep warning. Patch 5 and 6 restructure the
> locking in the driver to make it more fine-grained instead of holding the lock
> for the entire tasklet. Patch 7 finally fixes the warning.

Gentle ping.

Michael

> 
> Michael
> 
> [0] https://lore.kernel.org/linux-arm-kernel/20210601130108.GA12967@pengutronix.de/
> 
> Michael Tretter (7):
>   dmaengine: zynqmp_dma: simplify with dev_err_probe
>   dmaengine: zynqmp_dma: drop message on probe success
>   dmaengine: zynqmp_dma: enable COMPILE_TEST
>   dmaengine: zynqmp_dma: cleanup includes
>   dmaengine: zynqmp_dma: cleanup after completing all descriptors
>   dmaengine: zynqmp_dma: refine dma descriptor locking
>   dmaengine: zynqmp_dma: fix lockdep warning in tasklet
> 
>  drivers/dma/Kconfig             |  2 +-
>  drivers/dma/xilinx/zynqmp_dma.c | 67 +++++++++++++++++----------------
>  2 files changed, 35 insertions(+), 34 deletions(-)
> 
> -- 
> 2.30.2
> 
> 

Re: [PATCH 0/7] dmaengine: zynqmp_dma: fix lockdep warning and cleanup

[Vinod Koul]

On 26-08-21, 11:47, Michael Tretter wrote:
> Hi,
> 
> I reported a lockdep warning in the ZynqMP DMA driver a few weeks ago [0].
> This series fixes the reported warning and performs some cleanup that I found
> while looking at the driver more closely.
> 
> Patches 1-4 are the cleanups. They affect the log output of the driver, allow
> to compile the driver on other platforms when COMPILE_TEST is enabled, and
> remove unused included header files from the driver.
> 
> Patches 5-7 aim to fix the lockdep warning. Patch 5 and 6 restructure the
> locking in the driver to make it more fine-grained instead of holding the lock
> for the entire tasklet. Patch 7 finally fixes the warning.

Applied, thanks

-- 
~Vinod