Skip to contentSkip to conversation

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

PATCH Superseded Gu Bin 2 participants 5 messages Nov 11

Subsystems (4)
lore
Share & tools
lore

Gu Bin

author
#0 lore raw

PATCH 1/1 hwrng: mtk - use pm_runtime_resume_and_get() in mtk_rng_read()

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")
---
 drivers/char/hw_random/mtk-rng.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)
drivers/char/hw_random/mtk-rng.c +3 -1
index 127b82d979e8..1e8e458dab25 100644
@@ -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))
Signature · 1 line
2.50.1.windows.1
#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] hwrng: mtk - premature hwrng…

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

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.

Quoted 5 lines
drivers/char/hw_random/mtk-rng.c +0 -0
index 127b82d979e8e..1e8e458dab25c 100644

drivers/char/hw_random/mtk-rng.c static int mtk_rng_read(struct hwrng *rng, void *buf, size_t max, bool wait) · line 103

Quoted 10 lines
drivers/char/hw_random/mtk-rng.c +3 -1
@@ -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

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

v2 cover hwrng: mtk - runtime PM fixes

In reply to 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(-)
Signature · 1 line
2.55.0.windows.3
author
#3 lore raw

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

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")
---
 drivers/char/hw_random/mtk-rng.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)
drivers/char/hw_random/mtk-rng.c +3 -1
index 127b82d979e8..1e8e458dab25 100644
@@ -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))
Signature · 1 line
2.55.0.windows.3
author
#4 lore raw

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

In reply to 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")
---
 drivers/char/hw_random/mtk-rng.c | 14 +++++++-------
 1 file changed, 7 insertions(+), 7 deletions(-)
drivers/char/hw_random/mtk-rng.c +7 -7
index 1e8e458dab25..f337473ba99f 100644
@@ -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;
Signature · 1 line
2.55.0.windows.3