Re: [PATCH] clk: imx: clk-imx95-blk-ctl: fix error unwinding in probe

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found no new issues
in this patch, but noticed 1 related pre-existing issue it does not
introduce:
- [High] clk: imx: uninitialized register overwrite in
  imx95_bc_runtime_resume() [1]
--

commit 19a6efb010aee7e2b721c30ed7ad173ca7b7d1df
Author: Manush Prajwal <manushprajwal555@gmail.com>

clk: imx: clk-imx95-blk-ctl: fix error unwinding in probe

This commit fixes error handling in imx95_bc_probe() by checking runtime PM
return values, dropping the runtime PM reference on failure, and disabling
bc->clk_apb on error paths.

> diff --git a/drivers/clk/imx/clk-imx95-blk-ctl.c b/drivers/clk/imx/clk-imx95-blk-ctl.c
> index bc6957299ceca..ad386a48ec2bb 100644
> --- a/drivers/clk/imx/clk-imx95-blk-ctl.c
> +++ b/drivers/clk/imx/clk-imx95-blk-ctl.c
> @@ -394,17 +394,29 @@ static int imx95_bc_probe(struct platform_device *pdev)

[ ... ]

>  	if (bc->pdata->rpm_enabled) {
> -		devm_pm_runtime_enable(&pdev->dev);
> -		pm_runtime_resume_and_get(&pdev->dev);
> +		ret = devm_pm_runtime_enable(&pdev->dev);
> +		if (ret)
> +			goto err_clk_disable;
> +
> +		ret = pm_runtime_resume_and_get(&pdev->dev);
> +		if (ret)
> +			goto err_clk_disable;
>  	}

[Severity: High]
This problem wasn't introduced by this patch, but doesn't calling
pm_runtime_resume_and_get() here [1] overwrite the hardware clock
control register with zero before any state has been saved?

bc is allocated with devm_kzalloc(), so bc->clk_reg_restore is initially
0.  When pm_runtime_resume_and_get() runs during probe, it synchronously
invokes imx95_bc_runtime_resume():

drivers/clk/imx/clk-imx95-blk-ctl.c:imx95_bc_runtime_resume() {
	...
	writel(bc->clk_reg_restore, bc->base + bc->pdata->clk_reg_offset);

	return 0;
}

Writing the uninitialized zero value clobbers the clock gate and mux
settings configured by the bootloader, disabling active clocks and hanging
dependent blocks such as display or LVDS.  Initializing
bc->clk_reg_restore from the register before enabling runtime PM (or
calling pm_runtime_set_active() before devm_pm_runtime_enable()) would
preserve the hardware state.

[1] https://sashiko.dev/#/bug/linux-2f2aa951-60e6-4afb-8098-949100502ab9

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/6af44b5c.67914289.fe7ab.6559@mx.google.com?part=1