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

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

[Manush Prajwal]

In imx95_bc_probe(), bc->clk_apb is enabled via clk_prepare_enable().
However, if subsequent operations fail:
1. When !bc->pdata, devm_of_platform_populate() failure returns directly
   without disabling bc->clk_apb.
2. When clk_hw_data allocation fails, probe returns -ENOMEM directly
   without disabling bc->clk_apb.
3. The return values of devm_pm_runtime_enable() and
   pm_runtime_resume_and_get() are ignored, potentially proceeding with
   an unresumed device or invalid runtime PM state.
4. If clock registration, of_clk_add_hw_provider(), or child
   devm_of_platform_populate() fails, execution jumps to cleanup:,
   which unregisters registered clocks but fails to drop the runtime PM
   reference acquired by pm_runtime_resume_and_get() and fails to call
   clk_disable_unprepare(bc->clk_apb).

Fix these issues by:
- Handling devm_of_platform_populate() failure on !bc->pdata by jumping
  to err_clk_disable.
- Jumping to err_clk_disable if clk_hw_data allocation fails.
- Checking the return values of devm_pm_runtime_enable() and
  pm_runtime_resume_and_get() and jumping to err_clk_disable on error.
- Calling pm_runtime_put_sync() in cleanup: if runtime PM is enabled.
- Adding err_clk_disable to call clk_disable_unprepare(bc->clk_apb).

Fixes: 5224b189462f ("clk: imx: add i.MX95 BLK CTL clk driver")
Fixes: b08217a25721 ("clk: imx95-blk-ctl: Fix synchronous abort")
Signed-off-by: Manush Prajwal <manushprajwal555@gmail.com>
---
 drivers/clk/imx/clk-imx95-blk-ctl.c | 30 +++++++++++++++++++++++------
 1 file changed, 24 insertions(+), 6 deletions(-)

diff --git a/drivers/clk/imx/clk-imx95-blk-ctl.c b/drivers/clk/imx/clk-imx95-blk-ctl.c
index bc6957299..ad386a48e 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)
 	}

 	bc->pdata = of_device_get_match_data(dev);
-	if (!bc->pdata)
-		return devm_of_platform_populate(dev);
+	if (!bc->pdata) {
+		ret = devm_of_platform_populate(dev);
+		if (ret)
+			goto err_clk_disable;
+
+		return 0;
+	}

 	clk_hw_data = devm_kzalloc(dev, struct_size(clk_hw_data, hws, bc->pdata->num_clks),
 				   GFP_KERNEL);
-	if (!clk_hw_data)
-		return -ENOMEM;
+	if (!clk_hw_data) {
+		ret = -ENOMEM;
+		goto err_clk_disable;
+	}

 	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;
 	}

 	clk_hw_data->num = bc->pdata->num_clks;
@@ -462,6 +474,12 @@ static int imx95_bc_probe(struct platform_device *pdev)
 		clk_hw_unregister(hws[i]);
 	}

+	if (pm_runtime_enabled(bc->dev))
+		pm_runtime_put_sync(&pdev->dev);
+
+err_clk_disable:
+	clk_disable_unprepare(bc->clk_apb);
+
 	return ret;
 }

-- 
2.46.2.windows.1

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

[sashiko-bot]

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