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 <[email protected]>
Reviewed-by: Liviu Dudau <[email protected]> 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! / --------------- ¯\_(ツ)_/¯
