[PATCH] hwrng: mtk - use pm_runtime_resume_and_get() in mtk_rng_read()

[PATCH] hwrng: mtk - use pm_runtime_resume_and_get() in mtk_rng_read()

[Gu Bin]

mtk_rng_read() calls pm_runtime_get_sync() without checking the return
value and immediately starts polling and reading the RNG registers:
when resuming the device fails (negative return), the reads hit a
device that never resumed, and the failure is only surfaced indirectly
(if at all) through the -EIO fallback of the final return.

Use pm_runtime_resume_and_get() instead and bail out on the error: the
helper reports the resume failure and drops the usage counter reference
it took, which a failed pm_runtime_get_sync() leaves behind (the usage
counter is incremented before the resume attempt, so returning
directly after a failed get_sync() would leak the reference and the
device could never autosuspend again). The tail
pm_runtime_put_sync_autosuspend() must not be used for this bailout:
the device never resumed, so balancing the counter is all that is
needed. On success the helper returns 0, which is exactly what the
read counter expects at that point.

Fixes: 81d2b34508c6 ("hwrng: mtk - add runtime PM support")
Signed-off-by: Gu Bin <gubin@cmss.chinamobile.com>
---
 drivers/char/hw_random/mtk-rng.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/char/hw_random/mtk-rng.c b/drivers/char/hw_random/mtk-rng.c
index 127b82d979e8..1e8e458dab25 100644
--- a/drivers/char/hw_random/mtk-rng.c
+++ b/drivers/char/hw_random/mtk-rng.c
@@ -95,7 +95,9 @@ static int mtk_rng_read(struct hwrng *rng, void *buf, size_t max, bool wait)
 	struct mtk_rng *priv = to_mtk_rng(rng);
 	int retval = 0;
 
-	pm_runtime_get_sync(priv->dev);
+	retval = pm_runtime_resume_and_get(priv->dev);
+	if (retval < 0)
+		return retval;
 
 	while (max >= sizeof(u32)) {
 		if (!mtk_rng_wait_ready(rng, wait))
-- 
2.50.1.windows.1

Re: [PATCH] hwrng: mtk - use pm_runtime_resume_and_get() in mtk_rng_read()

[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] hwrng: mtk - premature hwrng registration and unchecked runtime
  PM in mtk_rng_read() [1]
--

commit dfb825399ed095d750b85ef745a6c83b7ac41f98
Author: Gu Bin <gubin@cmss.chinamobile.com>

hwrng: mtk - use pm_runtime_resume_and_get() in mtk_rng_read()

This commit replaces pm_runtime_get_sync() with pm_runtime_resume_and_get()
in mtk_rng_read() and returns early if resuming the device fails.

> diff --git a/drivers/char/hw_random/mtk-rng.c b/drivers/char/hw_random/mtk-rng.c
> index 127b82d979e8e..1e8e458dab25c 100644
> --- a/drivers/char/hw_random/mtk-rng.c
> +++ b/drivers/char/hw_random/mtk-rng.c
> @@ -95,7 +95,9 @@ static int mtk_rng_read(struct hwrng *rng, void *buf, size_t max, bool wait)
>  	struct mtk_rng *priv = to_mtk_rng(rng);
>  	int retval = 0;
>  
> -	pm_runtime_get_sync(priv->dev);
> +	retval = pm_runtime_resume_and_get(priv->dev);
> +	if (retval < 0)
> +		return retval;
>  
>  	while (max >= sizeof(u32)) {
>  		if (!mtk_rng_wait_ready(rng, wait))

[Severity: High]
This problem wasn't introduced by this patch, but can mtk_rng_read() be
called while runtime PM is disabled [1]?

In mtk_rng_probe(), devm_hwrng_register() is called before dev_set_drvdata()
and devm_pm_runtime_enable():

drivers/char/hw_random/mtk-rng.c:mtk_rng_probe() {
	...
	ret = devm_hwrng_register(&pdev->dev, &priv->rng);
	if (ret) {
		dev_err(&pdev->dev, "failed to register rng device: %d\n",
			ret);
		return ret;
	}

	if (!(priv->flags & MTK_RNG_SMC)) {
		dev_set_drvdata(&pdev->dev, priv);
		pm_runtime_set_autosuspend_delay(&pdev->dev,
						 RNG_AUTOSUSPEND_TIMEOUT);
		pm_runtime_use_autosuspend(&pdev->dev);
		ret = devm_pm_runtime_enable(&pdev->dev);
	...
}

Because devm_hwrng_register() immediately exposes the RNG and starts
hwrng_fillfn(), and because reverse-order devres teardown on unbind disables
runtime PM before unregistering the RNG, mtk_rng_read() can run while
runtime PM is disabled, causing pm_runtime_resume_and_get() to fail with
-EACCES (or previously causing unclocked MMIO reads when CONFIG_PM is
enabled).  Enabling runtime PM and setting drvdata before calling
devm_hwrng_register() would avoid this window during both probe and unbind.

[1] https://sashiko.dev/#/bug/linux-371c6254-7dda-4dcc-bf98-1919a85f9f5b

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261111073237.1647-1-gubin@cmss.chinamobile.com?part=1

[PATCH v2 0/2] hwrng: mtk - runtime PM fixes

[Gu Bin]

The sashiko review of v1 pointed out a pre-existing ordering problem:
mtk_rng_probe() registers the RNG before enabling runtime PM, so
mtk_rng_read() can run while runtime PM is disabled - both in the
probe window (hwrng_register() starts the fillfn kthread right away)
and on unbind (the devres teardown disables runtime PM before the RNG
is unregistered). Patch 2 moves the drvdata assignment and the
runtime PM setup above the registration, which closes both windows.
Patch 1 is unchanged.

Gu Bin (2):
  hwrng: mtk - use pm_runtime_resume_and_get() in mtk_rng_read()
  hwrng: mtk - enable runtime PM before registering the RNG

 drivers/char/hw_random/mtk-rng.c | 18 ++++++++++--------
 1 file changed, 10 insertions(+), 8 deletions(-)

-- 
2.55.0.windows.3

[PATCH v2 1/2] hwrng: mtk - use pm_runtime_resume_and_get() in mtk_rng_read()

[Gu Bin]

mtk_rng_read() calls pm_runtime_get_sync() without checking the return
value and immediately starts polling and reading the RNG registers:
when resuming the device fails (negative return), the reads hit a
device that never resumed, and the failure is only surfaced indirectly
(if at all) through the -EIO fallback of the final return.

Use pm_runtime_resume_and_get() instead and bail out on the error: the
helper reports the resume failure and drops the usage counter reference
it took, which a failed pm_runtime_get_sync() leaves behind (the usage
counter is incremented before the resume attempt, so returning
directly after a failed get_sync() would leak the reference and the
device could never autosuspend again). The tail
pm_runtime_put_sync_autosuspend() must not be used for this bailout:
the device never resumed, so balancing the counter is all that is
needed. On success the helper returns 0, which is exactly what the
read counter expects at that point.

Fixes: 81d2b34508c6 ("hwrng: mtk - add runtime PM support")
Signed-off-by: Gu Bin <gubin@cmss.chinamobile.com>
---
 drivers/char/hw_random/mtk-rng.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/char/hw_random/mtk-rng.c b/drivers/char/hw_random/mtk-rng.c
index 127b82d979e8..1e8e458dab25 100644
--- a/drivers/char/hw_random/mtk-rng.c
+++ b/drivers/char/hw_random/mtk-rng.c
@@ -95,7 +95,9 @@ static int mtk_rng_read(struct hwrng *rng, void *buf, size_t max, bool wait)
 	struct mtk_rng *priv = to_mtk_rng(rng);
 	int retval = 0;
 
-	pm_runtime_get_sync(priv->dev);
+	retval = pm_runtime_resume_and_get(priv->dev);
+	if (retval < 0)
+		return retval;
 
 	while (max >= sizeof(u32)) {
 		if (!mtk_rng_wait_ready(rng, wait))
-- 
2.55.0.windows.3

[PATCH v2 2/2] hwrng: mtk - enable runtime PM before registering the RNG

[Gu Bin]

mtk_rng_probe() registers the RNG before setting up runtime PM (and
before dev_set_drvdata()). devm_hwrng_register() immediately exposes
the RNG: hwrng_register() makes it current when it has the best
quality and starts the hwrng_fillfn kthread right away, so
mtk_rng_read() can run in the window where runtime PM is not enabled
yet - pm_runtime_resume_and_get() then fails with -EACCES.

On unbind the devres teardown runs in reverse order: the RNG is
unregistered after runtime PM has been disabled, so the same failure
is possible during teardown.

Move the drvdata assignment and the runtime PM setup above
devm_hwrng_register(). The runtime PM resume callback needs the
drvdata, so the assignment has to come first anyway, and with this
order the devres teardown also unregisters the RNG before it disables
runtime PM, closing both windows. The SMC variant keeps skipping
runtime PM.

Fixes: 81d2b34508c6 ("hwrng: mtk - add runtime PM support")
Fixes: 78cb66caa6ab ("hwrng: mtk - Use devm_pm_runtime_enable")
Signed-off-by: Gu Bin <gubin@cmss.chinamobile.com>
---
 drivers/char/hw_random/mtk-rng.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/char/hw_random/mtk-rng.c b/drivers/char/hw_random/mtk-rng.c
index 1e8e458dab25..f337473ba99f 100644
--- a/drivers/char/hw_random/mtk-rng.c
+++ b/drivers/char/hw_random/mtk-rng.c
@@ -212,13 +212,6 @@ static int mtk_rng_probe(struct platform_device *pdev)
 		priv->rng.read = mtk_rng_read;
 	}
 
-	ret = devm_hwrng_register(&pdev->dev, &priv->rng);
-	if (ret) {
-		dev_err(&pdev->dev, "failed to register rng device: %d\n",
-			ret);
-		return ret;
-	}
-
 	if (!(priv->flags & MTK_RNG_SMC)) {
 		dev_set_drvdata(&pdev->dev, priv);
 		pm_runtime_set_autosuspend_delay(&pdev->dev,
@@ -229,6 +222,13 @@ static int mtk_rng_probe(struct platform_device *pdev)
 			return ret;
 	}
 
+	ret = devm_hwrng_register(&pdev->dev, &priv->rng);
+	if (ret) {
+		dev_err(&pdev->dev, "failed to register rng device: %d\n",
+			ret);
+		return ret;
+	}
+
 	dev_info(&pdev->dev, "registered RNG driver\n");
 
 	return 0;
-- 
2.55.0.windows.3