Skip to contentSkip to conversation

hwrng: cctrng - Fix an error handling path in cctrng_probe

PATCH In review Christophe JAILLET 3 participants 4 messages Oct 23 – Oct 27

Subsystems (4)
Kernel janitors Crypto API Hardware random number generator core Cctrng Arm trustzone cryptocell true random number generator (trng) driver
lore
Share & tools
lore

Christophe JAILLET

author
#0 lore raw

PATCH 1/1 hwrng: cctrng - Fix an error handling path in cctrng_probe

If an error occurs after a successful cc_trng_pm_init() call, it must be undone by a corresponding cc_trng_pm_fini() call.

The commit in Fixes has erroneously updated the error handling path if cc_trng_pm_get() fails.

Fixes 8bcd96893848 ("hwrng: cctrng - let devres enable the clock")
Signed-off-by Christophe JAILLET <[email protected]>

---
Compile tested only.

This patch also undo the clean-up done by commit a7ea3d63e897 ("hwrng:
cctrng - simplify cctrng_probe()"). Keeping an error path makes sense with 2 users. But if preferred, the missing cc_trng_pm_fini() could also be added after the test for errors in cc_trng_pm_get().

---
 drivers/char/hw_random/cctrng.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)
drivers/char/hw_random/cctrng.c +10 -4
index 03401ee59a8e..608b3321c05a 100644
@@ -524,8 +524,10 @@ static int cctrng_probe(struct platform_device *pdev)
 
 	/* increment device's usage counter */
 	rc = cc_trng_pm_get(dev);
-	if (rc)
-		return dev_err_probe(dev, rc, "cc_trng_pm_get returned %x\n", rc);
+	if (rc) {
+		dev_err_probe(dev, rc, "cc_trng_pm_get returned %x\n", rc);
+		goto post_pm_err;
+	}
 
 	/* set pending_hw to verify that HW won't be triggered from read */
 	atomic_set(&drvdata->pending_hw, 1);

@@ -534,8 +536,7 @@ static int cctrng_probe(struct platform_device *pdev)
 	rc = devm_hwrng_register(dev, &drvdata->rng);
 	if (rc) {
 		dev_err(dev, "Could not register hwrng device.\n");
-		cc_trng_pm_fini(drvdata);
-		return rc;
+		goto post_pm_err;
 	}
 
 	/* trigger HW to start generate data */

@@ -548,6 +549,11 @@ static int cctrng_probe(struct platform_device *pdev)
 	dev_info(dev, "ARM cctrng device initialized\n");
 
 	return 0;
+
+post_pm_err:
+	cc_trng_pm_fini(drvdata);
+
+	return rc;
 }
 
 static void cctrng_remove(struct platform_device *pdev)
Signature · 1 line
2.56.0
AI review
#1 lore raw
Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider: - [Medium] hwrng: cctrng: unbalanced pm_runtime_disable() in cctrng_probe() error path --

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] hwrng: cctrng: unbalanced pm_runtime_disable() in cctrng_probe() error path --

commit a9816f5e80718b9a879081c61af5922aa650d7de
Author: Christophe JAILLET <[email protected]>

hwrng: cctrng - Fix an error handling path in cctrng_probe

This commit routes cc_trng_pm_get() and devm_hwrng_register() failures in cctrng_probe() to a common post_pm_err label that calls cc_trng_pm_fini().

Quoted 28 lines
drivers/char/hw_random/cctrng.c +5 -4
index 03401ee59a8e8..608b3321c05aa 100644
@@ -524,8 +524,10 @@ static int cctrng_probe(struct platform_device *pdev)
 
 	/* increment device's usage counter */
 	rc = cc_trng_pm_get(dev);
-	if (rc)
-		return dev_err_probe(dev, rc, "cc_trng_pm_get returned %x\n", rc);
+	if (rc) {
+		dev_err_probe(dev, rc, "cc_trng_pm_get returned %x\n", rc);
+		goto post_pm_err;
+	}
 
 	/* set pending_hw to verify that HW won't be triggered from read */
 	atomic_set(&drvdata->pending_hw, 1);

@@ -534,8 +536,7 @@ static int cctrng_probe(struct platform_device *pdev)
 	rc = devm_hwrng_register(dev, &drvdata->rng);
 	if (rc) {
 		dev_err(dev, "Could not register hwrng device.\n");
-		cc_trng_pm_fini(drvdata);
-		return rc;
+		goto post_pm_err;
 	}
 
 	/* trigger HW to start generate data */

drivers/char/hw_random/cctrng.c static int cctrng_probe(struct platform_device *pdev) · line 556

Quoted 8 lines
drivers/char/hw_random/cctrng.c +5 -0
@@ -548,6 +549,11 @@ static int cctrng_probe(struct platform_device *pdev)
 	dev_info(dev, "ARM cctrng device initialized\n");
 
 	return 0;
+
+post_pm_err:
+	cc_trng_pm_fini(drvdata);
+
+	return rc;

[Severity: Medium]
Does calling cc_trng_pm_fini() in the post_pm_err error path cause an unbalanced pm_runtime_disable() and leak a dev->power.disable_depth reference when cc_trng_pm_get() or devm_hwrng_register() fails?

In drivers/char/hw_random/cctrng.c, cc_trng_pm_init() only configures autosuspend and sets the runtime status to active without enabling runtime PM:

static int cc_trng_pm_init(struct cctrng_drvdata *drvdata)

{
	struct device *dev = &(drvdata->pdev->dev);

	/* must be before the enabling to avoid redundant suspending */
	pm_runtime_set_autosuspend_delay(dev, CC_TRNG_SUSPEND_TIMEOUT);
	pm_runtime_use_autosuspend(dev);
	/* set us as active - note we won't do PM ops until cc_trng_pm_go()! */
	return pm_runtime_set_active(dev);
}

Runtime PM is only enabled later in cctrng_probe() via cc_trng_pm_go() -> pm_runtime_enable(), after both cc_trng_pm_get() and devm_hwrng_register() have succeeded:

static void cc_trng_pm_go(struct cctrng_drvdata *drvdata)

{
	struct device *dev = &(drvdata->pdev->dev);

	/* enable the PM module*/
	pm_runtime_enable(dev);
}

static void cc_trng_pm_fini(struct cctrng_drvdata *drvdata)

{
	struct device *dev = &(drvdata->pdev->dev);

	pm_runtime_disable(dev);
}

Because cc_trng_pm_fini() calls pm_runtime_disable(), it pairs with cc_trng_pm_go() rather than cc_trng_pm_init(). Jumping to post_pm_err before cc_trng_pm_go() has run calls pm_runtime_disable() when runtime PM was never enabled, incrementing dev->power.disable_depth from 1 to 2.

When cctrng_probe() returns an error, device_unbind_cleanup() calls pm_runtime_reinit() in drivers/base/power/runtime.c, which already resets runtime_status to RPM_SUSPENDED and clears use_autosuspend when runtime PM is disabled, but does not reset dev->power.disable_depth. If the device is subsequently re-probed, cc_trng_pm_go() only decrements disable_depth from 2 to 1, leaving runtime PM permanently disabled for the device.

In addition, if devm_hwrng_register() fails after cc_trng_pm_get() succeeds, wouldn't cctrng_probe() need to undo the usage count increment from cc_trng_pm_get() rather than calling cc_trng_pm_fini()?

Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/b66b3df3d178a04b35690d0eb36dee409a95aa5d.1792782868.git.christophe.jaillet@wanadoo.fr?part=1
#2 lore raw
In reply to Christophe JAILLET

The sashiko review makes sense to me. We should probably call pm_runtime_put_noidle after post_pm_err.

Best regards,
Martin

Thus wrote Christophe JAILLET ([email protected]):

Quoted 60 lines

If an error occurs after a successful cc_trng_pm_init() call, it must be undone by a corresponding cc_trng_pm_fini() call.

The commit in Fixes has erroneously updated the error handling path if cc_trng_pm_get() fails.

Fixes 8bcd96893848 ("hwrng: cctrng - let devres enable the clock")
Signed-off-by Christophe JAILLET <[email protected]>

---
Compile tested only.

This patch also undo the clean-up done by commit a7ea3d63e897 ("hwrng:
cctrng - simplify cctrng_probe()"). Keeping an error path makes sense with 2 users. But if preferred, the missing cc_trng_pm_fini() could also be added after the test for errors in cc_trng_pm_get().

---
 drivers/char/hw_random/cctrng.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)
drivers/char/hw_random/cctrng.c +10 -4
index 03401ee59a8e..608b3321c05a 100644
@@ -524,8 +524,10 @@ static int cctrng_probe(struct platform_device *pdev)
 
 	/* increment device's usage counter */
 	rc = cc_trng_pm_get(dev);
-	if (rc)
-		return dev_err_probe(dev, rc, "cc_trng_pm_get returned %x\n", rc);
+	if (rc) {
+		dev_err_probe(dev, rc, "cc_trng_pm_get returned %x\n", rc);
+		goto post_pm_err;
+	}
 
 	/* set pending_hw to verify that HW won't be triggered from read */
 	atomic_set(&drvdata->pending_hw, 1);

@@ -534,8 +536,7 @@ static int cctrng_probe(struct platform_device *pdev)
 	rc = devm_hwrng_register(dev, &drvdata->rng);
 	if (rc) {
 		dev_err(dev, "Could not register hwrng device.\n");
-		cc_trng_pm_fini(drvdata);
-		return rc;
+		goto post_pm_err;
 	}
 
 	/* trigger HW to start generate data */

@@ -548,6 +549,11 @@ static int cctrng_probe(struct platform_device *pdev)
 	dev_info(dev, "ARM cctrng device initialized\n");
 
 	return 0;
+
+post_pm_err:
+	cc_trng_pm_fini(drvdata);
+
+	return rc;
 }
 
 static void cctrng_remove(struct platform_device *pdev)
Signature · 1 line
2.56.0
author
#3 lore raw

Le 27/10/2026 à 09:22, Martin Kaiser a écrit :

Quoted 6 lines

The sashiko review makes sense to me. We should probably call pm_runtime_put_noidle after post_pm_err.

Best regards,
Martin

I agree that my patch looks broken.

I've not followed all sashiko explanation, but usually I try to stay away from pm code, which most of the time looks too tricky for me.

So should something be fixed here, I'll leave it to you.

CJ

Quoted 63 lines

Thus wrote Christophe JAILLET ([email protected]):

Quoted 60 lines

If an error occurs after a successful cc_trng_pm_init() call, it must be undone by a corresponding cc_trng_pm_fini() call.

The commit in Fixes has erroneously updated the error handling path if cc_trng_pm_get() fails.

Fixes 8bcd96893848 ("hwrng: cctrng - let devres enable the clock")
Signed-off-by Christophe JAILLET <[email protected]>

---
Compile tested only.

This patch also undo the clean-up done by commit a7ea3d63e897 ("hwrng:
cctrng - simplify cctrng_probe()"). Keeping an error path makes sense with 2 users. But if preferred, the missing cc_trng_pm_fini() could also be added after the test for errors in cc_trng_pm_get().

---
  drivers/char/hw_random/cctrng.c | 14 ++++++++++----
  1 file changed, 10 insertions(+), 4 deletions(-)
drivers/char/hw_random/cctrng.c +10 -4
index 03401ee59a8e..608b3321c05a 100644
@@ -524,8 +524,10 @@ static int cctrng_probe(struct platform_device *pdev)
 
  	/* increment device's usage counter */
  	rc = cc_trng_pm_get(dev);
-	if (rc)
-		return dev_err_probe(dev, rc, "cc_trng_pm_get returned %x\n", rc);
+	if (rc) {
+		dev_err_probe(dev, rc, "cc_trng_pm_get returned %x\n", rc);
+		goto post_pm_err;
+	}
 
  	/* set pending_hw to verify that HW won't be triggered from read */
  	atomic_set(&drvdata->pending_hw, 1);

@@ -534,8 +536,7 @@ static int cctrng_probe(struct platform_device *pdev)
  	rc = devm_hwrng_register(dev, &drvdata->rng);
  	if (rc) {
  		dev_err(dev, "Could not register hwrng device.\n");
-		cc_trng_pm_fini(drvdata);
-		return rc;
+		goto post_pm_err;
  	}
 
  	/* trigger HW to start generate data */

@@ -548,6 +549,11 @@ static int cctrng_probe(struct platform_device *pdev)
  	dev_info(dev, "ARM cctrng device initialized\n");
 
  	return 0;
+
+post_pm_err:
+	cc_trng_pm_fini(drvdata);
+
+	return rc;
  }
 
  static void cctrng_remove(struct platform_device *pdev)
Signature · 1 line
2.56.0