[PATCH v2] drm/arm/malidp: use clk_bulk API in runtime PM resume and suspend

[PATCH v2] drm/arm/malidp: use clk_bulk API in runtime PM resume and suspend

[Gustavo Kenji Mendonça Kaneko]

malidp_runtime_pm_resume() calls clk_prepare_enable() three times
without checking the return value. If any clock fails to enable, the
driver silently proceeds with unclocked hardware, leading to undefined
behavior.

Convert both the resume and suspend paths to use the clk_bulk API:
clk_bulk_prepare_enable() in resume checks the return value and rolls
back any successfully enabled clocks on failure;
clk_bulk_disable_unprepare() in suspend keeps the two paths symmetric.

This issue was found by code review without access to Mali DP hardware.

Signed-off-by: Gustavo Kenji Mendonça Kaneko <kaneko.dev@pm.me>
---
Changes in v2:
  - Also convert malidp_runtime_pm_suspend() to clk_bulk_disable_unprepare()
    to make both paths truly symmetric (Liviu Dudau)
  - Drop the incorrect claim about existing clk_bulk usage in the suspend
    path from the commit message (Liviu Dudau)

 drivers/gpu/drm/arm/malidp_drv.c | 22 ++++++++++++++++------
 1 file changed, 16 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/arm/malidp_drv.c b/drivers/gpu/drm/arm/malidp_drv.c
index b765f6c9eea4..90e7f14aacec 100644
--- a/drivers/gpu/drm/arm/malidp_drv.c
+++ b/drivers/gpu/drm/arm/malidp_drv.c
@@ -670,6 +670,11 @@ static int malidp_runtime_pm_suspend(struct device *dev)
 	struct drm_device *drm = dev_get_drvdata(dev);
 	struct malidp_drm *malidp = drm_to_malidp(drm);
 	struct malidp_hw_device *hwdev = malidp->dev;
+	struct clk_bulk_data clks[] = {
+		{ .clk = hwdev->pclk },
+		{ .clk = hwdev->aclk },
+		{ .clk = hwdev->mclk },
+	};
 
 	/* we can only suspend if the hardware is in config mode */
 	WARN_ON(!hwdev->hw->in_config_mode(hwdev));
@@ -677,9 +682,7 @@ static int malidp_runtime_pm_suspend(struct device *dev)
 	malidp_se_irq_fini(hwdev);
 	malidp_de_irq_fini(hwdev);
 	hwdev->pm_suspended = true;
-	clk_disable_unprepare(hwdev->mclk);
-	clk_disable_unprepare(hwdev->aclk);
-	clk_disable_unprepare(hwdev->pclk);
+	clk_bulk_disable_unprepare(ARRAY_SIZE(clks), clks);
 
 	return 0;
 }
@@ -689,10 +692,17 @@ static int malidp_runtime_pm_resume(struct device *dev)
 	struct drm_device *drm = dev_get_drvdata(dev);
 	struct malidp_drm *malidp = drm_to_malidp(drm);
 	struct malidp_hw_device *hwdev = malidp->dev;
+	struct clk_bulk_data clks[] = {
+		{ .clk = hwdev->pclk },
+		{ .clk = hwdev->aclk },
+		{ .clk = hwdev->mclk },
+	};
+	int err;
+
+	err = clk_bulk_prepare_enable(ARRAY_SIZE(clks), clks);
+	if (err)
+		return err;
 
-	clk_prepare_enable(hwdev->pclk);
-	clk_prepare_enable(hwdev->aclk);
-	clk_prepare_enable(hwdev->mclk);
 	hwdev->pm_suspended = false;
 	malidp_de_irq_hw_init(hwdev);
 	malidp_se_irq_hw_init(hwdev);
-- 
2.54.0

Re: [PATCH v2] drm/arm/malidp: use clk_bulk API in runtime PM resume and suspend

[sashiko-bot]

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

Pre-existing issues:
- [High] Unhandled error from malidp_runtime_pm_resume() and pm_runtime_get_sync() leads to kernel panic.
- [High] Missing synchronize_irq() in the runtime PM suspend path for shared IRQs causes a race condition.
- [High] Asynchronous pm_runtime_put() races with pm_runtime_disable(), leaving the hardware in an incorrect power state.
--

commit 15b02ca295e5d2b2ae68c0ebcf65a7fa13cad1b6
Author: Gustavo Kenji Mendonça Kaneko <kaneko.dev@pm.me>

drm/arm/malidp: use clk_bulk API in runtime PM resume and suspend
    
malidp_runtime_pm_resume() calls clk_prepare_enable() three times
without checking the return value. If any clock fails to enable, the
driver silently proceeds with unclocked hardware, leading to undefined
behavior.

> diff --git a/drivers/gpu/drm/arm/malidp_drv.c b/drivers/gpu/drm/arm/malidp_drv.c
> index 9abe800f598ac..23fa942ae4bbe 100644
> --- a/drivers/gpu/drm/arm/malidp_drv.c
> +++ b/drivers/gpu/drm/arm/malidp_drv.c
> @@ -670,6 +670,11 @@ static int malidp_runtime_pm_suspend(struct device *dev)
[ ... ]
>  	malidp_se_irq_fini(hwdev);
>  	malidp_de_irq_fini(hwdev);
>  	hwdev->pm_suspended = true;
> -	clk_disable_unprepare(hwdev->mclk);
> -	clk_disable_unprepare(hwdev->aclk);
> -	clk_disable_unprepare(hwdev->pclk);
> +	clk_bulk_disable_unprepare(ARRAY_SIZE(clks), clks);

[Severity: High]
This is a pre-existing issue, but does this missing synchronize_irq() in the
runtime PM suspend path for shared IRQs cause a race condition?

The driver registers its DE and SE interrupts with IRQF_SHARED.

If a shared interrupt triggers concurrently on another CPU, could the IRQ
handler (malidp_se_irq() or malidp_de_irq()) pass the pm_suspended check
just before the clocks are disabled here, and then attempt to read
hardware registers while the clocks are off, resulting in a synchronous
external abort?

>  	return 0;
>  }
> @@ -689,10 +692,17 @@ static int malidp_runtime_pm_resume(struct device *dev)
[ ... ]
> +	err = clk_bulk_prepare_enable(ARRAY_SIZE(clks), clks);
> +	if (err)
> +		return err;

[Severity: High]
This isn't a bug introduced by this patch, but since we are now correctly
propagating the clock enablement error from malidp_runtime_pm_resume(), do we
need to update the callers to handle it?

In malidp_bind(), the code ignores the return value:

	if (pm_runtime_enabled(dev))
		pm_runtime_get_sync(dev);
	else
		malidp_runtime_pm_resume(dev);

	ret = hwdev->hw->query_hw(hwdev);

If clock enablement fails, it seems the driver unconditionally proceeds
to read hardware registers (e.g., in query_hw()). Could this cause a
synchronous external abort when it accesses unclocked hardware?

Similarly, malidp_pm_resume_early() does not check the return value
and proceeds to mark the device as active:

static int __maybe_unused malidp_pm_resume_early(struct device *dev)
{
	malidp_runtime_pm_resume(dev);
	pm_runtime_set_active(dev);
	return 0;
}

[Severity: High]
This is a pre-existing issue, but does the asynchronous pm_runtime_put()
race with pm_runtime_disable() in the driver removal paths?

In malidp_bind() (on the query_hw_fail error unroll path):

query_hw_fail:
	pm_runtime_put(dev);
	if (pm_runtime_enabled(dev))
		pm_runtime_disable(dev);
	else
		malidp_runtime_pm_suspend(dev);

And similarly in malidp_unbind():

	pm_runtime_put(dev);
	if (pm_runtime_enabled(dev))
		pm_runtime_disable(dev);
	else
		malidp_runtime_pm_suspend(dev);

Because pm_runtime_put() queues an asynchronous idle notification, the
subsequent pm_runtime_disable() barrier could cancel the pending suspend
request before it completes.

Could this leave the device's clocks enabled indefinitely after the driver
is unloaded? Should this use pm_runtime_put_sync() instead?

>  
> -	clk_prepare_enable(hwdev->pclk);
> -	clk_prepare_enable(hwdev->aclk);
> -	clk_prepare_enable(hwdev->mclk);
>  	hwdev->pm_suspended = false;
>  	malidp_de_irq_hw_init(hwdev);
>  	malidp_se_irq_hw_init(hwdev);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260609130812.1065699-1-kaneko.dev@pm.me?part=1

Re: [PATCH v2] drm/arm/malidp: use clk_bulk API in runtime PM resume and suspend

[Liviu Dudau]

On Tue, Jun 09, 2026 at 01:08:19PM +0000, Gustavo Kenji Mendonça Kaneko wrote:
> malidp_runtime_pm_resume() calls clk_prepare_enable() three times
> without checking the return value. If any clock fails to enable, the
> driver silently proceeds with unclocked hardware, leading to undefined
> behavior.
> 
> Convert both the resume and suspend paths to use the clk_bulk API:
> clk_bulk_prepare_enable() in resume checks the return value and rolls
> back any successfully enabled clocks on failure;
> clk_bulk_disable_unprepare() in suspend keeps the two paths symmetric.
> 
> This issue was found by code review without access to Mali DP hardware.
> 
> Signed-off-by: Gustavo Kenji Mendonça Kaneko <kaneko.dev@pm.me>

Reviewed-by: Liviu Dudau <liviu.dudau@arm.com>

Best regards,
Liviu

> ---
> Changes in v2:
>   - Also convert malidp_runtime_pm_suspend() to clk_bulk_disable_unprepare()
>     to make both paths truly symmetric (Liviu Dudau)
>   - Drop the incorrect claim about existing clk_bulk usage in the suspend
>     path from the commit message (Liviu Dudau)
> 
>  drivers/gpu/drm/arm/malidp_drv.c | 22 ++++++++++++++++------
>  1 file changed, 16 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/arm/malidp_drv.c b/drivers/gpu/drm/arm/malidp_drv.c
> index b765f6c9eea4..90e7f14aacec 100644
> --- a/drivers/gpu/drm/arm/malidp_drv.c
> +++ b/drivers/gpu/drm/arm/malidp_drv.c
> @@ -670,6 +670,11 @@ static int malidp_runtime_pm_suspend(struct device *dev)
>  	struct drm_device *drm = dev_get_drvdata(dev);
>  	struct malidp_drm *malidp = drm_to_malidp(drm);
>  	struct malidp_hw_device *hwdev = malidp->dev;
> +	struct clk_bulk_data clks[] = {
> +		{ .clk = hwdev->pclk },
> +		{ .clk = hwdev->aclk },
> +		{ .clk = hwdev->mclk },
> +	};
>  
>  	/* we can only suspend if the hardware is in config mode */
>  	WARN_ON(!hwdev->hw->in_config_mode(hwdev));
> @@ -677,9 +682,7 @@ static int malidp_runtime_pm_suspend(struct device *dev)
>  	malidp_se_irq_fini(hwdev);
>  	malidp_de_irq_fini(hwdev);
>  	hwdev->pm_suspended = true;
> -	clk_disable_unprepare(hwdev->mclk);
> -	clk_disable_unprepare(hwdev->aclk);
> -	clk_disable_unprepare(hwdev->pclk);
> +	clk_bulk_disable_unprepare(ARRAY_SIZE(clks), clks);
>  
>  	return 0;
>  }
> @@ -689,10 +692,17 @@ static int malidp_runtime_pm_resume(struct device *dev)
>  	struct drm_device *drm = dev_get_drvdata(dev);
>  	struct malidp_drm *malidp = drm_to_malidp(drm);
>  	struct malidp_hw_device *hwdev = malidp->dev;
> +	struct clk_bulk_data clks[] = {
> +		{ .clk = hwdev->pclk },
> +		{ .clk = hwdev->aclk },
> +		{ .clk = hwdev->mclk },
> +	};
> +	int err;
> +
> +	err = clk_bulk_prepare_enable(ARRAY_SIZE(clks), clks);
> +	if (err)
> +		return err;
>  
> -	clk_prepare_enable(hwdev->pclk);
> -	clk_prepare_enable(hwdev->aclk);
> -	clk_prepare_enable(hwdev->mclk);
>  	hwdev->pm_suspended = false;
>  	malidp_de_irq_hw_init(hwdev);
>  	malidp_se_irq_hw_init(hwdev);
> -- 
> 2.54.0
> 
> 

-- 
====================
| I would like to |
| fix the world,  |
| but they're not |
| giving me the   |
 \ source code!  /
  ---------------
    ¯\_(ツ)_/¯

Re: [PATCH v2] drm/arm/malidp: use clk_bulk API in runtime PM resume and suspend

[Liviu Dudau]

On Tue, Jun 09, 2026 at 01:08:19PM +0000, Gustavo Kenji Mendonça Kaneko wrote:
> malidp_runtime_pm_resume() calls clk_prepare_enable() three times
> without checking the return value. If any clock fails to enable, the
> driver silently proceeds with unclocked hardware, leading to undefined
> behavior.
> 
> Convert both the resume and suspend paths to use the clk_bulk API:
> clk_bulk_prepare_enable() in resume checks the return value and rolls
> back any successfully enabled clocks on failure;
> clk_bulk_disable_unprepare() in suspend keeps the two paths symmetric.
> 
> This issue was found by code review without access to Mali DP hardware.
> 
> Signed-off-by: Gustavo Kenji Mendonça Kaneko <kaneko.dev@pm.me>

Sorry for the delay, I've pushed both your patches into drm-misc-fixes today.

Best regards,
Liviu

> ---
> Changes in v2:
>   - Also convert malidp_runtime_pm_suspend() to clk_bulk_disable_unprepare()
>     to make both paths truly symmetric (Liviu Dudau)
>   - Drop the incorrect claim about existing clk_bulk usage in the suspend
>     path from the commit message (Liviu Dudau)
> 
>  drivers/gpu/drm/arm/malidp_drv.c | 22 ++++++++++++++++------
>  1 file changed, 16 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/arm/malidp_drv.c b/drivers/gpu/drm/arm/malidp_drv.c
> index b765f6c9eea4..90e7f14aacec 100644
> --- a/drivers/gpu/drm/arm/malidp_drv.c
> +++ b/drivers/gpu/drm/arm/malidp_drv.c
> @@ -670,6 +670,11 @@ static int malidp_runtime_pm_suspend(struct device *dev)
>  	struct drm_device *drm = dev_get_drvdata(dev);
>  	struct malidp_drm *malidp = drm_to_malidp(drm);
>  	struct malidp_hw_device *hwdev = malidp->dev;
> +	struct clk_bulk_data clks[] = {
> +		{ .clk = hwdev->pclk },
> +		{ .clk = hwdev->aclk },
> +		{ .clk = hwdev->mclk },
> +	};
>  
>  	/* we can only suspend if the hardware is in config mode */
>  	WARN_ON(!hwdev->hw->in_config_mode(hwdev));
> @@ -677,9 +682,7 @@ static int malidp_runtime_pm_suspend(struct device *dev)
>  	malidp_se_irq_fini(hwdev);
>  	malidp_de_irq_fini(hwdev);
>  	hwdev->pm_suspended = true;
> -	clk_disable_unprepare(hwdev->mclk);
> -	clk_disable_unprepare(hwdev->aclk);
> -	clk_disable_unprepare(hwdev->pclk);
> +	clk_bulk_disable_unprepare(ARRAY_SIZE(clks), clks);
>  
>  	return 0;
>  }
> @@ -689,10 +692,17 @@ static int malidp_runtime_pm_resume(struct device *dev)
>  	struct drm_device *drm = dev_get_drvdata(dev);
>  	struct malidp_drm *malidp = drm_to_malidp(drm);
>  	struct malidp_hw_device *hwdev = malidp->dev;
> +	struct clk_bulk_data clks[] = {
> +		{ .clk = hwdev->pclk },
> +		{ .clk = hwdev->aclk },
> +		{ .clk = hwdev->mclk },
> +	};
> +	int err;
> +
> +	err = clk_bulk_prepare_enable(ARRAY_SIZE(clks), clks);
> +	if (err)
> +		return err;
>  
> -	clk_prepare_enable(hwdev->pclk);
> -	clk_prepare_enable(hwdev->aclk);
> -	clk_prepare_enable(hwdev->mclk);
>  	hwdev->pm_suspended = false;
>  	malidp_de_irq_hw_init(hwdev);
>  	malidp_se_irq_hw_init(hwdev);
> -- 
> 2.54.0
> 
> 

-- 
====================
| I would like to |
| fix the world,  |
| but they're not |
| giving me the   |
 \ source code!  /
  ---------------
    ¯\_(ツ)_/¯