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@xxxxx>

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

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!  /
  ---------------
    ¯\_(ツ)_/¯