[PATCH] dmaengine: sprd: fix clock imbalance when enabling ashb_clk fails

[PATCH] dmaengine: sprd: fix clock imbalance when enabling ashb_clk fails

[Chaithanya Lagisetty]

sprd_dma_enable() enables sdev->clk and then enables the optional
ashb_clk. If clk_prepare_enable() for ashb_clk fails, the function
returns the error without disabling sdev->clk, which was already
enabled. Both callers (probe and resume) treat a non-zero return as
"nothing was enabled" and bail out, so sdev->clk is leaked.

Disable sdev->clk on the ashb_clk enable failure path.

Fixes: 9b3b8171f7f4 ("dmaengine: sprd: Add Spreadtrum DMA driver")
Signed-off-by: Chaithanya Lagisetty <nagachaithanya9911@gmail.com>
---
 drivers/dma/sprd-dma.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/dma/sprd-dma.c b/drivers/dma/sprd-dma.c
index 087fea3af2e4..3ea5c3ba1a8c 100644
--- a/drivers/dma/sprd-dma.c
+++ b/drivers/dma/sprd-dma.c
@@ -270,9 +270,16 @@ static int sprd_dma_enable(struct sprd_dma_dev *sdev)
 	 * The ashb_clk is optional and only for AGCP DMA controller, so we
 	 * need add one condition to check if the ashb_clk need enable.
 	 */
-	if (!IS_ERR(sdev->ashb_clk))
+	if (!IS_ERR(sdev->ashb_clk)) {
 		ret = clk_prepare_enable(sdev->ashb_clk);
+		if (ret)
+			goto disable_clk;
+	}
 
+	return 0;
+
+disable_clk:
+	clk_disable_unprepare(sdev->clk);
 	return ret;
 }
 
-- 
2.43.0

Re: [PATCH] dmaengine: sprd: fix clock imbalance when enabling ashb_clk fails

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:

Pre-existing issues:
- [High] The driver ignores `-EPROBE_DEFER` for the optional `ashb_clk`, which prevents proper probe deferral.
- [High] Unchecked `pm_runtime_get_sync()` failures leak the PM usage counter and cause unbalanced clock teardowns.
- [High] The interrupt handler returns without clearing the hardware interrupt if a channel has no active descriptor, triggering an infinite IRQ storm.
- [High] The interrupt handler is registered before critical resources are initialized, leading to a NULL pointer dereference.
- [High] Channels are manually removed from the device's channel list before unregistering the DMA device, leaking channel resources.
--

--- Patch [1]: [PATCH] dmaengine: sprd: fix clock imbalance when enabling ashb_clk fails ---
commit db2ba297caa82c1c816130ae2b34acb419871f3c
Author: Chaithanya Lagisetty <nagachaithanya9911@gmail.com>

dmaengine: sprd: fix clock imbalance when enabling ashb_clk fails

sprd_dma_enable() enables sdev->clk and then enables the optional
ashb_clk. If clk_prepare_enable() for ashb_clk fails, the function
returns the error without disabling sdev->clk, which was already
enabled. Both callers (probe and resume) treat a non-zero return as
nothing was enabled and bail out, so sdev->clk is leaked.

> diff --git a/drivers/dma/sprd-dma.c b/drivers/dma/sprd-dma.c
> index 087fea3af2e41..3ea5c3ba1a8c8 100644
> --- a/drivers/dma/sprd-dma.c
> +++ b/drivers/dma/sprd-dma.c
> @@ -270,9 +270,16 @@ static int sprd_dma_enable(struct sprd_dma_dev *sdev)
>  	 * The ashb_clk is optional and only for AGCP DMA controller, so we
>  	 * need add one condition to check if the ashb_clk need enable.
>  	 */
> -	if (!IS_ERR(sdev->ashb_clk))
> +	if (!IS_ERR(sdev->ashb_clk)) {
>  		ret = clk_prepare_enable(sdev->ashb_clk);
> +		if (ret)
> +			goto disable_clk;
> +	}
>  
> +	return 0;
> +
> +disable_clk:
> +	clk_disable_unprepare(sdev->clk);
>  	return ret;
>  }

While reviewing this patch, I noticed some pre-existing regressions in the same
file.

drivers/dma/sprd-dma.c:sprd_dma_probe() {
[ ... ]
	sdev->ashb_clk = devm_clk_get(&pdev->dev, "ashb_eb");
	if (IS_ERR(sdev->ashb_clk))
		dev_warn(&pdev->dev, "no optional ashb eb clock\n");
[ ... ]

[Severity: High]
This is a pre-existing issue, but does this code ignore -EPROBE_DEFER?

If the clock provider is probed after this driver, treating the clock as
permanently absent could prevent probe deferral and lead to missing hardware
clocks.


drivers/dma/sprd-dma.c:sprd_dma_alloc_chan_resources() {
	return pm_runtime_get_sync(chan->device->dev);
}

drivers/dma/sprd-dma.c:sprd_dma_probe() {
[ ... ]
	ret = pm_runtime_get_sync(&pdev->dev);
	if (ret < 0)
		goto err_rpm;
[ ... ]
err_register:
	pm_runtime_put_noidle(&pdev->dev);
	pm_runtime_disable(&pdev->dev);
err_rpm:
	sprd_dma_disable(sdev);
	return ret;
}

drivers/dma/sprd-dma.c:sprd_dma_remove() {
	struct sprd_dma_dev *sdev = platform_get_drvdata(pdev);
	struct sprd_dma_chn *c, *cn;

	pm_runtime_get_sync(&pdev->dev);

[Severity: High]
This is a pre-existing issue, but do these pm_runtime_get_sync() calls
properly handle failure?

In sprd_dma_alloc_chan_resources(), the error is returned directly,
abandoning the incremented usage counter.

In sprd_dma_probe(), the err_rpm path skips pm_runtime_put_noidle() and
pm_runtime_disable().

In sprd_dma_remove(), failing to check the return value can lead to
unbalanced clock teardowns if the device is already suspended.


drivers/dma/sprd-dma.c:dma_irq_handle() {
[ ... ]
	while (irq_status) {
		i = __ffs(irq_status);
		irq_status &= (irq_status - 1);
		schan = &sdev->channels[i];

		spin_lock(&schan->vc.lock);

		sdesc = schan->cur_desc;
		if (!sdesc) {
			spin_unlock(&schan->vc.lock);
			return IRQ_HANDLED;
		}

[Severity: High]
This isn't a bug introduced by this patch, but does this early return cause an
interrupt storm?

If the handler exits here without calling sprd_dma_clear_int() to acknowledge
the hardware interrupt, the interrupt controller might immediately refire it
upon exit.


drivers/dma/sprd-dma.c:sprd_dma_probe() {
[ ... ]
	sdev->irq = platform_get_irq(pdev, 0);
	if (sdev->irq > 0) {
		ret = devm_request_irq(&pdev->dev, sdev->irq, dma_irq_handle,
				       0, "sprd_dma", (void *)sdev);
		if (ret < 0) {
			dev_err(&pdev->dev, "request dma irq failed\n");
			return ret;
		}
	} else {
		dev_warn(&pdev->dev, "no interrupts for the dma controller\n");
	}

	sdev->glb_base = devm_platform_ioremap_resource(pdev, 0);

[Severity: High]
This is a pre-existing issue, but could this lead to a NULL pointer
dereference?

The interrupt is unmasked by devm_request_irq() before
devm_platform_ioremap_resource() initializes sdev->glb_base.

If an interrupt fires early, the handler will blindly dereference the
uninitialized base address in dma_irq_handle():
u32 irq_status = readl(sdev->glb_base + SPRD_DMA_GLB_INT_MSK_STS);


drivers/dma/sprd-dma.c:sprd_dma_remove() {
[ ... ]
	list_for_each_entry_safe(c, cn, &sdev->dma_dev.channels,
				 vc.chan.device_node) {
		list_del(&c->vc.chan.device_node);
		tasklet_kill(&c->vc.task);
	}

	of_dma_controller_free(pdev->dev.of_node);
	dma_async_device_unregister(&sdev->dma_dev);

[Severity: High]
This isn't a bug introduced by this patch, but does this manual list_del()
leak channel resources?

Removing the channels from the list causes dma_async_device_unregister() to
see an empty list and skip freeing its resources.

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

Re: [PATCH] dmaengine: sprd: fix clock imbalance when enabling ashb_clk fails

[Frank Li]

On Thu, Aug 13, 2026 at 10:53:54AM +0000, Chaithanya Lagisetty wrote:
> [You don't often get email from nagachaithanya9911@gmail.com. Learn why this is important at https://aka.ms/LearnAboutSenderIdentification ]
>
> sprd_dma_enable() enables sdev->clk and then enables the optional
> ashb_clk. If clk_prepare_enable() for ashb_clk fails, the function
> returns the error without disabling sdev->clk, which was already
> enabled. Both callers (probe and resume) treat a non-zero return as
> "nothing was enabled" and bail out, so sdev->clk is leaked.
>
> Disable sdev->clk on the ashb_clk enable failure path.
>
> Fixes: 9b3b8171f7f4 ("dmaengine: sprd: Add Spreadtrum DMA driver")
> Signed-off-by: Chaithanya Lagisetty <nagachaithanya9911@gmail.com>
> ---
>  drivers/dma/sprd-dma.c | 9 ++++++++-
>  1 file changed, 8 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/dma/sprd-dma.c b/drivers/dma/sprd-dma.c
> index 087fea3af2e4..3ea5c3ba1a8c 100644
> --- a/drivers/dma/sprd-dma.c
> +++ b/drivers/dma/sprd-dma.c
> @@ -270,9 +270,16 @@ static int sprd_dma_enable(struct sprd_dma_dev *sdev)
>          * The ashb_clk is optional and only for AGCP DMA controller, so we
>          * need add one condition to check if the ashb_clk need enable.
>          */
> -       if (!IS_ERR(sdev->ashb_clk))
> +       if (!IS_ERR(sdev->ashb_clk)) {
>                 ret = clk_prepare_enable(sdev->ashb_clk);
> +               if (ret)
> +                       goto disable_clk;
> +       }
>
> +       return 0;
> +
> +disable_clk:
> +       clk_disable_unprepare(sdev->clk);

Convert to clk bulk API.

Frank
>         return ret;
>  }
>
> --
> 2.43.0
>

[PATCH v2] dmaengine: sprd: use clk_bulk API to fix clock imbalance

[Chaithanya Lagisetty]

sprd_dma_enable() enabled sdev->clk and then the optional ashb_clk. If
enabling ashb_clk failed, sdev->clk was left enabled: both callers
(probe and resume) treat a non-zero return as "nothing was enabled" and
bail out, leaking sdev->clk.

Convert the driver to the clk_bulk API. clk_bulk_prepare_enable()
enables all clocks and unwinds them on failure, and
clk_bulk_disable_unprepare() disables them, which fixes the imbalance
and simplifies the enable/disable paths. The optional ashb_eb clock is
fetched with devm_clk_get_optional(), so a missing clock becomes a NULL
entry that the bulk helpers treat as a no-op.

Fixes: 9b3b8171f7f4 ("dmaengine: sprd: Add Spreadtrum DMA driver")
Signed-off-by: Chaithanya Lagisetty <nagachaithanya9911@gmail.com>
---
Changes since v1:
- Convert the enable/disable paths to the clk_bulk API instead of
  manually disabling sdev->clk on the ashb_clk failure path, and fetch
  the optional ashb_eb clock with devm_clk_get_optional() (Frank Li).

v1: https://lore.kernel.org/dmaengine/20260813105354.2577040-1-nagachaithanya9911@gmail.com/

 drivers/dma/sprd-dma.c | 44 +++++++++++++-----------------------------
 1 file changed, 13 insertions(+), 31 deletions(-)

diff --git a/drivers/dma/sprd-dma.c b/drivers/dma/sprd-dma.c
index 087fea3af2e4..ac6fe6118ad1 100644
--- a/drivers/dma/sprd-dma.c
+++ b/drivers/dma/sprd-dma.c
@@ -208,8 +208,7 @@ struct sprd_dma_chn {
 struct sprd_dma_dev {
 	struct dma_device	dma_dev;
 	void __iomem		*glb_base;
-	struct clk		*clk;
-	struct clk		*ashb_clk;
+	struct clk_bulk_data	clks[2];
 	int			irq;
 	u32			total_chns;
 	struct sprd_dma_chn	channels[] __counted_by(total_chns);
@@ -260,31 +259,12 @@ static void sprd_dma_chn_update(struct sprd_dma_chn *schan, u32 reg,
 
 static int sprd_dma_enable(struct sprd_dma_dev *sdev)
 {
-	int ret;
-
-	ret = clk_prepare_enable(sdev->clk);
-	if (ret)
-		return ret;
-
-	/*
-	 * The ashb_clk is optional and only for AGCP DMA controller, so we
-	 * need add one condition to check if the ashb_clk need enable.
-	 */
-	if (!IS_ERR(sdev->ashb_clk))
-		ret = clk_prepare_enable(sdev->ashb_clk);
-
-	return ret;
+	return clk_bulk_prepare_enable(ARRAY_SIZE(sdev->clks), sdev->clks);
 }
 
 static void sprd_dma_disable(struct sprd_dma_dev *sdev)
 {
-	clk_disable_unprepare(sdev->clk);
-
-	/*
-	 * Need to check if we need disable the optional ashb_clk for AGCP DMA.
-	 */
-	if (!IS_ERR(sdev->ashb_clk))
-		clk_disable_unprepare(sdev->ashb_clk);
+	clk_bulk_disable_unprepare(ARRAY_SIZE(sdev->clks), sdev->clks);
 }
 
 static void sprd_dma_set_uid(struct sprd_dma_chn *schan)
@@ -1140,16 +1120,18 @@ static int sprd_dma_probe(struct platform_device *pdev)
 	if (!sdev)
 		return -ENOMEM;
 
-	sdev->clk = devm_clk_get(&pdev->dev, "enable");
-	if (IS_ERR(sdev->clk)) {
-		dev_err(&pdev->dev, "get enable clock failed\n");
-		return PTR_ERR(sdev->clk);
-	}
+	sdev->clks[0].id = "enable";
+	sdev->clks[0].clk = devm_clk_get(&pdev->dev, "enable");
+	if (IS_ERR(sdev->clks[0].clk))
+		return dev_err_probe(&pdev->dev, PTR_ERR(sdev->clks[0].clk),
+				     "get enable clock failed\n");
 
 	/* ashb clock is optional for AGCP DMA */
-	sdev->ashb_clk = devm_clk_get(&pdev->dev, "ashb_eb");
-	if (IS_ERR(sdev->ashb_clk))
-		dev_warn(&pdev->dev, "no optional ashb eb clock\n");
+	sdev->clks[1].id = "ashb_eb";
+	sdev->clks[1].clk = devm_clk_get_optional(&pdev->dev, "ashb_eb");
+	if (IS_ERR(sdev->clks[1].clk))
+		return dev_err_probe(&pdev->dev, PTR_ERR(sdev->clks[1].clk),
+				     "get ashb_eb clock failed\n");
 
 	/*
 	 * We have three DMA controllers: AP DMA, AON DMA and AGCP DMA. For AGCP
-- 
2.43.0

Re: [PATCH v2] dmaengine: sprd: use clk_bulk API to fix clock imbalance

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [High] Runtime PM usage counter and enablement state leak on error paths.
- [High] Device interrupt requested before hardware IO memory is mapped and spinlocks are initialized.
--

--- Patch [1]: [PATCH v2] dmaengine: sprd: use clk_bulk API to fix clock imbalance ---
commit ed070633d7ec91a8fe862c830443d077ba0540c0
Author: Chaithanya Lagisetty <nagachaithanya9911@gmail.com>

dmaengine: sprd: use clk_bulk API to fix clock imbalance

This patch converts the driver to use the clk_bulk API to handle clock
enabling and disabling. It simplifies error paths and fixes a clock
imbalance issue where sdev->clk was leaked if the optional ashb_clk failed
to enable.

> diff --git a/drivers/dma/sprd-dma.c b/drivers/dma/sprd-dma.c
> index 087fea3af2e41..ac6fe6118ad19 100644
> --- a/drivers/dma/sprd-dma.c
> +++ b/drivers/dma/sprd-dma.c
[ ... ]
> @@ -1140,16 +1120,18 @@ static int sprd_dma_probe(struct platform_device *pdev)
>  	if (!sdev)
>  		return -ENOMEM;
>  
> -	sdev->clk = devm_clk_get(&pdev->dev, "enable");
> -	if (IS_ERR(sdev->clk)) {
> -		dev_err(&pdev->dev, "get enable clock failed\n");
> -		return PTR_ERR(sdev->clk);
> -	}
> +	sdev->clks[0].id = "enable";
> +	sdev->clks[0].clk = devm_clk_get(&pdev->dev, "enable");
> +	if (IS_ERR(sdev->clks[0].clk))
> +		return dev_err_probe(&pdev->dev, PTR_ERR(sdev->clks[0].clk),
> +				     "get enable clock failed\n");
>  
>  	/* ashb clock is optional for AGCP DMA */
> -	sdev->ashb_clk = devm_clk_get(&pdev->dev, "ashb_eb");
> -	if (IS_ERR(sdev->ashb_clk))
> -		dev_warn(&pdev->dev, "no optional ashb eb clock\n");
> +	sdev->clks[1].id = "ashb_eb";
> +	sdev->clks[1].clk = devm_clk_get_optional(&pdev->dev, "ashb_eb");
> +	if (IS_ERR(sdev->clks[1].clk))
> +		return dev_err_probe(&pdev->dev, PTR_ERR(sdev->clks[1].clk),
> +				     "get ashb_eb clock failed\n");
>  
>  	/*
>  	 * We have three DMA controllers: AP DMA, AON DMA and AGCP DMA. For AGCP

[Severity: High]
This is a pre-existing issue, but does this code request the interrupt too
early?

Looking at the sequence further down in sprd_dma_probe():

	sdev->irq = platform_get_irq(pdev, 0);
	if (sdev->irq > 0) {
		ret = devm_request_irq(&pdev->dev, sdev->irq, dma_irq_handle,
				       0, "sprd_dma", (void *)sdev);
		...
	}

	sdev->glb_base = devm_platform_ioremap_resource(pdev, 0);

The interrupt is requested before sdev->glb_base is mapped and before the
virtual channels are initialized. If an interrupt is pending (for example,
left over from a bootloader or kexec), could the IRQ handler dma_irq_handle()
fire immediately?

If it fires early, the handler will attempt to read from sdev->glb_base:

drivers/dma/sprd-dma.c:dma_irq_handle() {
	...
	u32 irq_status = readl(sdev->glb_base + SPRD_DMA_GLB_INT_MSK_STS);
	...
}

Since sdev was allocated with devm_kzalloc(), sdev->glb_base is still NULL,
which would lead to a NULL pointer dereference. The handler might also lock
schan->vc.lock before it has been initialized by vchan_init(). Would it be
safer to request the interrupt after all necessary resources are mapped and
initialized?


[Severity: High]
This isn't a bug introduced by this patch, but is there a PM usage counter
leak on the error path further down in sprd_dma_probe()?

When pm_runtime_get_sync() fails, the code jumps to err_rpm:

	ret = pm_runtime_get_sync(&pdev->dev);
	if (ret < 0)
		goto err_rpm;
...
err_rpm:
	sprd_dma_disable(sdev);
	return ret;

This skips calling pm_runtime_put_noidle() and pm_runtime_disable(), which
could leave the runtime PM state unbalanced and prevent the device from ever
suspending if the probe is retried.

A similar issue appears to exist in sprd_dma_alloc_chan_resources(), where
it directly returns the result of pm_runtime_get_sync():

drivers/dma/sprd-dma.c:sprd_dma_alloc_chan_resources() {
	...
	return pm_runtime_get_sync(chan->device->dev);
}

Could this also leak the usage counter if pm_runtime_get_sync() fails?

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

Re: [PATCH v2] dmaengine: sprd: use clk_bulk API to fix clock imbalance

[Frank Li]

On Sun, Aug 16, 2026 at 05:50:05AM +0000, Chaithanya Lagisetty wrote:
> sprd_dma_enable() enabled sdev->clk and then the optional ashb_clk. If
> enabling ashb_clk failed, sdev->clk was left enabled: both callers
> (probe and resume) treat a non-zero return as "nothing was enabled" and
> bail out, leaking sdev->clk.
>
> Convert the driver to the clk_bulk API. clk_bulk_prepare_enable()
> enables all clocks and unwinds them on failure, and
> clk_bulk_disable_unprepare() disables them, which fixes the imbalance
> and simplifies the enable/disable paths. The optional ashb_eb clock is
> fetched with devm_clk_get_optional(), so a missing clock becomes a NULL
> entry that the bulk helpers treat as a no-op.
>
> Fixes: 9b3b8171f7f4 ("dmaengine: sprd: Add Spreadtrum DMA driver")
> Signed-off-by: Chaithanya Lagisetty <nagachaithanya9911@gmail.com>
> ---
> Changes since v1:
> - Convert the enable/disable paths to the clk_bulk API instead of
>   manually disabling sdev->clk on the ashb_clk failure path, and fetch
>   the optional ashb_eb clock with devm_clk_get_optional() (Frank Li).
>
> v1: https://lore.kernel.org/dmaengine/20260813105354.2577040-1-nagachaithanya9911@gmail.com/
>
>  drivers/dma/sprd-dma.c | 44 +++++++++++++-----------------------------
>  1 file changed, 13 insertions(+), 31 deletions(-)
>
> diff --git a/drivers/dma/sprd-dma.c b/drivers/dma/sprd-dma.c
> index 087fea3af2e4..ac6fe6118ad1 100644
> --- a/drivers/dma/sprd-dma.c
> +++ b/drivers/dma/sprd-dma.c
> @@ -208,8 +208,7 @@ struct sprd_dma_chn {
>  struct sprd_dma_dev {
>  	struct dma_device	dma_dev;
>  	void __iomem		*glb_base;
> -	struct clk		*clk;
> -	struct clk		*ashb_clk;
> +	struct clk_bulk_data	clks[2];
>  	int			irq;
>  	u32			total_chns;
>  	struct sprd_dma_chn	channels[] __counted_by(total_chns);
> @@ -260,31 +259,12 @@ static void sprd_dma_chn_update(struct sprd_dma_chn *schan, u32 reg,
>
>  static int sprd_dma_enable(struct sprd_dma_dev *sdev)
>  {
> -	int ret;
> -
> -	ret = clk_prepare_enable(sdev->clk);
> -	if (ret)
> -		return ret;
> -
> -	/*
> -	 * The ashb_clk is optional and only for AGCP DMA controller, so we
> -	 * need add one condition to check if the ashb_clk need enable.
> -	 */
> -	if (!IS_ERR(sdev->ashb_clk))
> -		ret = clk_prepare_enable(sdev->ashb_clk);
> -
> -	return ret;
> +	return clk_bulk_prepare_enable(ARRAY_SIZE(sdev->clks), sdev->clks);
>  }
>
>  static void sprd_dma_disable(struct sprd_dma_dev *sdev)
>  {
> -	clk_disable_unprepare(sdev->clk);
> -
> -	/*
> -	 * Need to check if we need disable the optional ashb_clk for AGCP DMA.
> -	 */
> -	if (!IS_ERR(sdev->ashb_clk))
> -		clk_disable_unprepare(sdev->ashb_clk);
> +	clk_bulk_disable_unprepare(ARRAY_SIZE(sdev->clks), sdev->clks);
>  }
>
>  static void sprd_dma_set_uid(struct sprd_dma_chn *schan)
> @@ -1140,16 +1120,18 @@ static int sprd_dma_probe(struct platform_device *pdev)
>  	if (!sdev)
>  		return -ENOMEM;
>
> -	sdev->clk = devm_clk_get(&pdev->dev, "enable");
> -	if (IS_ERR(sdev->clk)) {
> -		dev_err(&pdev->dev, "get enable clock failed\n");
> -		return PTR_ERR(sdev->clk);
> -	}
> +	sdev->clks[0].id = "enable";
> +	sdev->clks[0].clk = devm_clk_get(&pdev->dev, "enable");
> +	if (IS_ERR(sdev->clks[0].clk))
> +		return dev_err_probe(&pdev->dev, PTR_ERR(sdev->clks[0].clk),
> +				     "get enable clock failed\n");
>
>  	/* ashb clock is optional for AGCP DMA */
> -	sdev->ashb_clk = devm_clk_get(&pdev->dev, "ashb_eb");
> -	if (IS_ERR(sdev->ashb_clk))
> -		dev_warn(&pdev->dev, "no optional ashb eb clock\n");
> +	sdev->clks[1].id = "ashb_eb";
> +	sdev->clks[1].clk = devm_clk_get_optional(&pdev->dev, "ashb_eb");
> +	if (IS_ERR(sdev->clks[1].clk))
> +		return dev_err_probe(&pdev->dev, PTR_ERR(sdev->clks[1].clk),
> +				     "get ashb_eb clock failed\n");

sdev->clks[0].id = "enable";
sdev->clks[1].id = "ashb_eb";
ret = devm_clk_bulk_get_optional(dev, ARRAY_SIZE(sdev->clks), sdev->clks);
if (ret && !sdev->clks[0].clk)
	return sdev->clks[0].clk();

Frank

>
>  	/*
>  	 * We have three DMA controllers: AP DMA, AON DMA and AGCP DMA. For AGCP
> --
> 2.43.0
>

[PATCH v3] dmaengine: sprd: use clk_bulk API to fix clock imbalance

[Chaithanya Lagisetty]

sprd_dma_enable() enabled sdev->clk and then the optional ashb_clk. If
enabling ashb_clk failed, sdev->clk was left enabled: both callers
(probe and resume) treat a non-zero return as "nothing was enabled" and
bail out, leaking sdev->clk.

Convert the driver to the clk_bulk API. clk_bulk_prepare_enable()
enables all clocks and unwinds them on failure, and
clk_bulk_disable_unprepare() disables them, which fixes the imbalance
and simplifies the enable/disable paths. Both clocks are fetched with
devm_clk_bulk_get_optional(); the optional ashb_eb clock simply becomes
a NULL entry that the bulk helpers treat as a no-op, while the mandatory
enable clock is still checked explicitly.

Fixes: 9b3b8171f7f4 ("dmaengine: sprd: Add Spreadtrum DMA driver")
Signed-off-by: Chaithanya Lagisetty <nagachaithanya9911@gmail.com>
---
Changes since v2:
- Fetch both clocks with a single devm_clk_bulk_get_optional() call
  instead of separate devm_clk_get()/devm_clk_get_optional() calls, and
  keep an explicit check for the mandatory "enable" clock (Frank Li).

Changes since v1:
- Convert the enable/disable paths to the clk_bulk API instead of
  manually disabling sdev->clk on the ashb_clk failure path (Frank Li).

v2: https://lore.kernel.org/dmaengine/20260816055005.2667930-1-nagachaithanya9911@gmail.com/
v1: https://lore.kernel.org/dmaengine/20260813105354.2577040-1-nagachaithanya9911@gmail.com/

 drivers/dma/sprd-dma.c | 45 ++++++++++++------------------------------
 1 file changed, 13 insertions(+), 32 deletions(-)

diff --git a/drivers/dma/sprd-dma.c b/drivers/dma/sprd-dma.c
index 087fea3af2e4..749cabd984eb 100644
--- a/drivers/dma/sprd-dma.c
+++ b/drivers/dma/sprd-dma.c
@@ -208,8 +208,7 @@ struct sprd_dma_chn {
 struct sprd_dma_dev {
 	struct dma_device	dma_dev;
 	void __iomem		*glb_base;
-	struct clk		*clk;
-	struct clk		*ashb_clk;
+	struct clk_bulk_data	clks[2];
 	int			irq;
 	u32			total_chns;
 	struct sprd_dma_chn	channels[] __counted_by(total_chns);
@@ -260,31 +259,12 @@ static void sprd_dma_chn_update(struct sprd_dma_chn *schan, u32 reg,
 
 static int sprd_dma_enable(struct sprd_dma_dev *sdev)
 {
-	int ret;
-
-	ret = clk_prepare_enable(sdev->clk);
-	if (ret)
-		return ret;
-
-	/*
-	 * The ashb_clk is optional and only for AGCP DMA controller, so we
-	 * need add one condition to check if the ashb_clk need enable.
-	 */
-	if (!IS_ERR(sdev->ashb_clk))
-		ret = clk_prepare_enable(sdev->ashb_clk);
-
-	return ret;
+	return clk_bulk_prepare_enable(ARRAY_SIZE(sdev->clks), sdev->clks);
 }
 
 static void sprd_dma_disable(struct sprd_dma_dev *sdev)
 {
-	clk_disable_unprepare(sdev->clk);
-
-	/*
-	 * Need to check if we need disable the optional ashb_clk for AGCP DMA.
-	 */
-	if (!IS_ERR(sdev->ashb_clk))
-		clk_disable_unprepare(sdev->ashb_clk);
+	clk_bulk_disable_unprepare(ARRAY_SIZE(sdev->clks), sdev->clks);
 }
 
 static void sprd_dma_set_uid(struct sprd_dma_chn *schan)
@@ -1140,16 +1120,17 @@ static int sprd_dma_probe(struct platform_device *pdev)
 	if (!sdev)
 		return -ENOMEM;
 
-	sdev->clk = devm_clk_get(&pdev->dev, "enable");
-	if (IS_ERR(sdev->clk)) {
-		dev_err(&pdev->dev, "get enable clock failed\n");
-		return PTR_ERR(sdev->clk);
-	}
+	/* The ashb_eb clock is optional and only present for AGCP DMA */
+	sdev->clks[0].id = "enable";
+	sdev->clks[1].id = "ashb_eb";
+	ret = devm_clk_bulk_get_optional(&pdev->dev, ARRAY_SIZE(sdev->clks),
+					 sdev->clks);
+	if (ret)
+		return dev_err_probe(&pdev->dev, ret, "get clocks failed\n");
 
-	/* ashb clock is optional for AGCP DMA */
-	sdev->ashb_clk = devm_clk_get(&pdev->dev, "ashb_eb");
-	if (IS_ERR(sdev->ashb_clk))
-		dev_warn(&pdev->dev, "no optional ashb eb clock\n");
+	if (!sdev->clks[0].clk)
+		return dev_err_probe(&pdev->dev, -ENOENT,
+				     "get enable clock failed\n");
 
 	/*
 	 * We have three DMA controllers: AP DMA, AON DMA and AGCP DMA. For AGCP
-- 
2.43.0

Re: [PATCH v3] dmaengine: sprd: use clk_bulk API to fix clock imbalance

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:

New issues:
- [High] The driver incorrectly enforces a mandatory clock using the optional `clk_bulk` API, breaking probe for `!CONFIG_HAVE_CLK` configurations and systems using dummy clocks.

Pre-existing issues:
- [High] The runtime PM usage counter is leaked and PM runtime is left enabled if `pm_runtime_get_sync()` fails in `sprd_dma_probe()`.
- [High] The interrupt handler `dma_irq_handle` can be invoked before `sdev->glb_base` is mapped, leading to a NULL pointer dereference.
- [High] The driver improperly unlinks DMA channels before calling `dma_async_device_unregister()`, breaking the DMA engine core cleanup.
- [High] Failing `pm_runtime_get_sync()` calls do not decrement the usage count in `sprd_dma_alloc_chan_resources()`, leading to PM runtime counter leaks.
--

--- Patch [1]: [PATCH v3] dmaengine: sprd: use clk_bulk API to fix clock imbalance ---
commit b974e2961641a4c2c3cf6810facb62f07deec1e8
Author: Chaithanya Lagisetty <nagachaithanya9911@gmail.com>
Subject: dmaengine: sprd: use clk_bulk API to fix clock imbalance

The commit converts the driver to use the clk_bulk API for managing its clocks
to fix a clock imbalance. This ensures both clocks are properly disabled on
failure paths. The optional ashb_eb clock is fetched alongside the mandatory
enable clock via devm_clk_bulk_get_optional().

> diff --git a/drivers/dma/sprd-dma.c b/drivers/dma/sprd-dma.c
> index 087fea3af2e41..749cabd984ebd 100644
> --- a/drivers/dma/sprd-dma.c
> +++ b/drivers/dma/sprd-dma.c

[ ... ]

> @@ -1140,16 +1120,17 @@ static int sprd_dma_probe(struct platform_device *pdev)
>  	if (!sdev)
>  		return -ENOMEM;
>  
> -	sdev->clk = devm_clk_get(&pdev->dev, "enable");
> -	if (IS_ERR(sdev->clk)) {
> -		dev_err(&pdev->dev, "get enable clock failed\n");
> -		return PTR_ERR(sdev->clk);
> -	}
> +	/* The ashb_eb clock is optional and only present for AGCP DMA */
> +	sdev->clks[0].id = "enable";
> +	sdev->clks[1].id = "ashb_eb";
> +	ret = devm_clk_bulk_get_optional(&pdev->dev, ARRAY_SIZE(sdev->clks),
> +					 sdev->clks);
> +	if (ret)
> +		return dev_err_probe(&pdev->dev, ret, "get clocks failed\n");
>  
> -	/* ashb clock is optional for AGCP DMA */
> -	sdev->ashb_clk = devm_clk_get(&pdev->dev, "ashb_eb");
> -	if (IS_ERR(sdev->ashb_clk))
> -		dev_warn(&pdev->dev, "no optional ashb eb clock\n");
> +	if (!sdev->clks[0].clk)
> +		return dev_err_probe(&pdev->dev, -ENOENT,
> +				     "get enable clock failed\n");

[Severity: High]
Does this manual NULL check break probe for systems without CONFIG_HAVE_CLK or
when dummy clocks are used?

When using devm_clk_bulk_get_optional(), if CONFIG_HAVE_CLK is disabled, the
API returns 0 but sets the clock pointer to NULL. The previous IS_ERR() check
would safely ignore this, but the new strict NULL check will unconditionally
fail with -ENOENT.

>  	/*
>  	 * We have three DMA controllers: AP DMA, AON DMA and AGCP DMA. For AGCP


While reviewing this patch, I also noticed a few pre-existing issues in the file.

In sprd_dma_probe():

> 	sdev->irq = platform_get_irq(pdev, 0);
> 	if (sdev->irq > 0) {
> 		ret = devm_request_irq(&pdev->dev, sdev->irq, dma_irq_handle,
> 				       0, "sprd_dma", (void *)sdev);
[ ... ]
> 	sdev->glb_base = devm_platform_ioremap_resource(pdev, 0);

[Severity: High]
This is a pre-existing issue, but could this cause a NULL pointer dereference
if the interrupt fires immediately?

The interrupt handler dma_irq_handle() dereferences sdev->glb_base to read
SPRD_DMA_GLB_INT_MSK_STS. Because devm_request_irq() is called before
devm_platform_ioremap_resource(), an early interrupt would access the unmapped
glb_base which is still NULL.


Also in sprd_dma_probe():

> 	pm_runtime_set_active(&pdev->dev);
> 	pm_runtime_enable(&pdev->dev);
> 
> 	ret = pm_runtime_get_sync(&pdev->dev);
> 	if (ret < 0)
> 		goto err_rpm;
[ ... ]
> err_register:
> 	pm_runtime_put_noidle(&pdev->dev);
> 	pm_runtime_disable(&pdev->dev);
> err_rpm:
> 	sprd_dma_disable(sdev);
> 	return ret;

[Severity: High]
This isn't a bug introduced by this patch, but does this error path leak the
runtime PM usage counter?

If pm_runtime_get_sync() fails, it still increments the usage count.
Jumping directly to err_rpm skips pm_runtime_put_noidle() and
pm_runtime_disable(), leaving PM runtime enabled and the counter unbalanced.


In sprd_dma_alloc_chan_resources():

> static int sprd_dma_alloc_chan_resources(struct dma_chan *chan)
> {
> 	return pm_runtime_get_sync(chan->device->dev);
> }

[Severity: High]
This is a pre-existing issue, but does this leak the PM usage count if it fails?

Because pm_runtime_get_sync() increments the counter even on failure, directly
returning its negative error code without calling pm_runtime_put_noidle() leaves
the usage count permanently elevated, preventing the device from suspending.
Should this use pm_runtime_resume_and_get() instead?


In sprd_dma_remove():

> 	list_for_each_entry_safe(c, cn, &sdev->dma_dev.channels,
> 				 vc.chan.device_node) {
> 		list_del(&c->vc.chan.device_node);
> 		tasklet_kill(&c->vc.task);
> 	}
> 
> 	of_dma_controller_free(pdev->dev.of_node);
> 	dma_async_device_unregister(&sdev->dma_dev);

[Severity: High]
This is a pre-existing issue, but does explicitly removing the channels from
the list here break the DMA engine core cleanup?

The channels are unlinked with list_del() before calling
dma_async_device_unregister(). When the core DMA engine loops over the
channels to free resources (such as sysfs entries), the list is empty, resulting
in leaked channel resources.

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