[PATCH 0/2] hwrng: cctrng - simplify runtime pm

[PATCH 0/2] hwrng: cctrng - simplify runtime pm

[Martin Kaiser]

two patches to simplify the runtime pm code in the cctrng driver

compile-tested only
(the devres patch is similar to imx-rngc.c, which was tested on real hw)

Martin Kaiser (2):
  hwrng: cctrng - simplify runtime pm refcount code
  hwrng: cctrng - use device-managed api for runtime pm

 drivers/char/hw_random/cctrng.c | 47 +++++----------------------------
 1 file changed, 6 insertions(+), 41 deletions(-)

-- 
2.43.7

[PATCH 1/2] hwrng: cctrng - simplify runtime pm refcount code

[Martin Kaiser]

Clean up the code for runtime pm refcounts.

The cc_trng_pm_get helper function isn't really useful, call
pm_runtime_resume_and_get directly.

In dev_err_probe, show the error code only once.

Signed-off-by: Martin Kaiser <martin@kaiser.cx>
---
 drivers/char/hw_random/cctrng.c | 14 ++++----------
 1 file changed, 4 insertions(+), 10 deletions(-)

diff --git a/drivers/char/hw_random/cctrng.c b/drivers/char/hw_random/cctrng.c
index b5f2c75d2eb8..ef3ca6d8cc85 100644
--- a/drivers/char/hw_random/cctrng.c
+++ b/drivers/char/hw_random/cctrng.c
@@ -85,12 +85,6 @@ static inline u32 cc_ioread(struct cctrng_drvdata *drvdata, u32 reg)
 	return ioread32(drvdata->cc_base + reg);
 }
 
-
-static int cc_trng_pm_get(struct device *dev)
-{
-	return pm_runtime_resume_and_get(dev);
-}
-
 static void cc_trng_pm_put_suspend(struct device *dev)
 {
 	pm_runtime_put_autosuspend(dev);
@@ -239,11 +233,11 @@ static int cctrng_read(struct hwrng *rng, void *data, size_t max, bool wait)
 			/* re-check space in buffer to avoid potential race */
 			if (circ_buf_space(drvdata) >= CC_TRNG_EHR_IN_WORDS) {
 				/* increment device's usage counter */
-				int rc = cc_trng_pm_get(dev);
+				int rc = pm_runtime_resume_and_get(dev);
 
 				if (rc) {
 					dev_err(dev,
-						"cc_trng_pm_get returned %x\n",
+						"pm_runtime_resume_and_get returned %x\n",
 						rc);
 					return rc;
 				}
@@ -521,9 +515,9 @@ static int cctrng_probe(struct platform_device *pdev)
 		return dev_err_probe(dev, rc, "cc_trng_pm_init failed\n");
 
 	/* increment device's usage counter */
-	rc = cc_trng_pm_get(dev);
+	rc = pm_runtime_resume_and_get(dev);
 	if (rc)
-		return dev_err_probe(dev, rc, "cc_trng_pm_get returned %x\n", rc);
+		return dev_err_probe(dev, rc, "pm_runtime_resume_and_get failed\n");
 
 	/* set pending_hw to verify that HW won't be triggered from read */
 	atomic_set(&drvdata->pending_hw, 1);
-- 
2.43.7

[PATCH 2/2] hwrng: cctrng - use device-managed api for runtime pm

[Martin Kaiser]

Use the device-managed api to enable runtime pm in the probe function.
The disable function is called from the devres layer when the device
is removed.

Disabling runtime pm was the only operation in the driver's remove
function. It can now be deleted.

Signed-off-by: Martin Kaiser <martin@kaiser.cx>
---
 drivers/char/hw_random/cctrng.c | 33 ++-------------------------------
 1 file changed, 2 insertions(+), 31 deletions(-)

diff --git a/drivers/char/hw_random/cctrng.c b/drivers/char/hw_random/cctrng.c
index ef3ca6d8cc85..ac77c6afea5b 100644
--- a/drivers/char/hw_random/cctrng.c
+++ b/drivers/char/hw_random/cctrng.c
@@ -97,26 +97,10 @@ static int cc_trng_pm_init(struct cctrng_drvdata *drvdata)
 	/* 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()! */
+	/* set us as active - note we won't do PM ops until devm_pm_runtime_enable()! */
 	return pm_runtime_set_active(dev);
 }
 
-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);
-}
-
-
 static inline int cc_trng_parse_sampling_ratio(struct cctrng_drvdata *drvdata)
 {
 	struct device *dev = &(drvdata->pdev->dev);
@@ -534,25 +518,13 @@ static int cctrng_probe(struct platform_device *pdev)
 	cc_trng_hw_trigger(drvdata);
 
 	/* All set, we can allow auto-suspend */
-	cc_trng_pm_go(drvdata);
+	devm_pm_runtime_enable(dev);
 
 	dev_info(dev, "ARM cctrng device initialized\n");
 
 	return 0;
 }
 
-static void cctrng_remove(struct platform_device *pdev)
-{
-	struct cctrng_drvdata *drvdata = platform_get_drvdata(pdev);
-	struct device *dev = &pdev->dev;
-
-	dev_dbg(dev, "Releasing cctrng resources...\n");
-
-	cc_trng_pm_fini(drvdata);
-
-	dev_info(dev, "ARM cctrng device terminated\n");
-}
-
 static int __maybe_unused cctrng_suspend(struct device *dev)
 {
 	struct cctrng_drvdata *drvdata = dev_get_drvdata(dev);
@@ -634,7 +606,6 @@ static struct platform_driver cctrng_driver = {
 		.pm = &cctrng_pm,
 	},
 	.probe = cctrng_probe,
-	.remove = cctrng_remove,
 };
 
 module_platform_driver(cctrng_driver);
-- 
2.43.7

Re: [PATCH 2/2] hwrng: cctrng - use device-managed api for runtime pm

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [Medium] hwrng: cctrng: unchecked devm_pm_runtime_enable() and late
  devres registration
- [Medium] hwrng: cctrng: double clk_disable_unprepare() on unbind via
  devm_pm_runtime_enable()
It also noticed 1 related pre-existing issue this patch does not
introduce:
- [Critical] hwrng: cctrng: use-after-free due to uncancelled works in
  cctrng_remove() [1]
--

commit b87f66a9bcd0c0b30376ba4eb9926de4dd58fc40
Author: Martin Kaiser <martin@kaiser.cx>

hwrng: cctrng - use device-managed api for runtime pm

This commit replaces manual pm_runtime_enable() and pm_runtime_disable()
calls with devm_pm_runtime_enable() and removes cctrng_remove().

> diff --git a/drivers/char/hw_random/cctrng.c b/drivers/char/hw_random/cctrng.c
> index ef3ca6d8cc850..ac77c6afea5b7 100644
> --- a/drivers/char/hw_random/cctrng.c
> +++ b/drivers/char/hw_random/cctrng.c
[ ... ]
> @@ -534,25 +518,13 @@ static int cctrng_probe(struct platform_device *pdev)
>  	cc_trng_hw_trigger(drvdata);
>  
>  	/* All set, we can allow auto-suspend */
> -	cc_trng_pm_go(drvdata);
> +	devm_pm_runtime_enable(dev);

[Severity: Medium]
Should cctrng_probe() check the return value of devm_pm_runtime_enable(dev)
and call it in cc_trng_pm_init() before devm_hwrng_register()?

If devm_pm_runtime_enable() fails to allocate its devres entry,
devm_add_action_or_reset() immediately runs pm_runtime_disable_action(),
leaving runtime PM disabled, autosuspend cleared, and the usage count from
pm_runtime_resume_and_get() leaked while cctrng_probe() returns 0.

Calling devm_pm_runtime_enable() after devm_hwrng_register() and
cc_trng_hw_trigger() also means devres teardown on unbind disables runtime
PM before unregistering the hwrng device, and allows cc_trng_compwork_handler()
to call pm_runtime_put_autosuspend() while runtime PM is still disabled.

[Severity: Medium]
Can switching to devm_pm_runtime_enable(dev) cause clk_disable_unprepare()
to be called twice on drvdata->clk when unbinding the driver?

cctrng_probe() enables drvdata->clk via devm_clk_get_optional_enabled().
Unlike the previous cctrng_remove() which only called pm_runtime_disable(),
devm_pm_runtime_enable()'s cleanup action calls
pm_runtime_dont_use_autosuspend() before pm_runtime_disable().  When the
device is unbound while idle, clearing autosuspend with runtime PM still
enabled synchronously invokes cctrng_suspend(), which calls
clk_disable_unprepare(drvdata->clk) before the devres release for
devm_clk_get_optional_enabled() disables and unprepares drvdata->clk a
second time.

>  	dev_info(dev, "ARM cctrng device initialized\n");
>  
>  	return 0;
>  }
>  
> -static void cctrng_remove(struct platform_device *pdev)
> -{
> -	struct cctrng_drvdata *drvdata = platform_get_drvdata(pdev);
> -	struct device *dev = &pdev->dev;
> -
> -	dev_dbg(dev, "Releasing cctrng resources...\n");
> -
> -	cc_trng_pm_fini(drvdata);
> -
> -	dev_info(dev, "ARM cctrng device terminated\n");
> -}

[Severity: Critical]
This problem wasn't introduced by this patch, but can unbinding the driver
without cancelling drvdata->compwork and drvdata->startwork cause a
use-after-free on drvdata [1]?

CPU0                                    CPU1 (kworker)
device_release_driver()
  devres_release_all()
    devm_hwrng_release()
    devm_irq_release()
    devm_ioremap_release()
    ...                                 cc_trng_compwork_handler()
    /* drvdata freed */                   cc_iowrite(drvdata, ...)
                                            <-- use-after-free

cc_isr() schedules drvdata->compwork and cctrng_read() schedules
drvdata->startwork, and compwork can also reschedule startwork.  Nothing
cancels either work item during teardown before devres unmaps drvdata->cc_base
and frees drvdata.

[1] https://sashiko.dev/#/bug/linux-c156c6f0-25a8-4aa0-8a30-20ef6163ee06

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261114170335.1680546-2-martin@kaiser.cx?part=2