Skip to contentSkip to conversation

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

PATCH New Manush Prajwal 2 participants 2 messages Nov 11

Subsystems (3)
Arm i.MX and Layerscape Common clk NXP i.MX clock drivers
lore
Share & tools
lore

Manush Prajwal

author
#0 lore raw

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

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 <[email protected]>
---
 drivers/clk/imx/clk-imx95-blk-ctl.c | 30 +++++++++++++++++++++++------
 1 file changed, 24 insertions(+), 6 deletions(-)
drivers/clk/imx/clk-imx95-blk-ctl.c +24 -6
index bc6957299..ad386a48e 100644
@@ -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;
 }
Signature · 1 line
2.46.2.windows.1
AI review
#1 lore raw
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…

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 <[email protected]>

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.

Quoted 5 lines
drivers/clk/imx/clk-imx95-blk-ctl.c +0 -0
index bc6957299ceca..ad386a48ec2bb 100644
@@ -394,17 +394,29 @@ static int imx95_bc_probe(struct platform_device *pdev)

[ ... ]

drivers/clk/imx/clk-imx95-blk-ctl.c static int imx95_bc_probe(struct platform_device *pdev) · line 420

Quoted 11 lines
drivers/clk/imx/clk-imx95-blk-ctl.c +7 -2
 	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

Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1