* Re: [PATCH] hwrng: imx-rngc: propagate clock enable errors
2026-08-30 13:12 [PATCH] hwrng: imx-rngc: propagate clock enable errors Pengpeng Hou
@ 2026-08-30 13:24 ` sashiko-bot
2026-08-31 15:49 ` Frank Li
2026-09-01 13:40 ` Martin Kaiser
2 siblings, 0 replies; 4+ messages in thread
From: sashiko-bot @ 2026-08-30 13:24 UTC (permalink / raw)
To: Pengpeng Hou; +Cc: Frank.Li, imx
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
Pre-existing issues:
- [High] The IRQ handler accesses hardware registers without ensuring the device is in an active PM state, which causes synchronous external aborts (bus errors) if the clock is disabled.
- [High] Stale completion state from lack of reinit_completion causes invalid hardware state reads.
--
commit 2504f9e54a2f61f40b7332a04558df267648996f
Author: Pengpeng Hou <pengpeng@iscas.ac.cn>
hwrng: imx-rngc: propagate clock enable errors
imx_rngc_probe() reads hardware registers after enabling its clock without
checking the result, and the runtime resume callback always reports success
after the same unchecked operation. Propagate clock-enable errors before
accessing the RNG and to the runtime-PM core on resume.
> diff --git a/drivers/char/hw_random/imx-rngc.c b/drivers/char/hw_random/imx-rngc.c
> index 7693328f10564..00a21d62c79c3 100644
> --- a/drivers/char/hw_random/imx-rngc.c
> +++ b/drivers/char/hw_random/imx-rngc.c
> @@ -266,7 +266,9 @@ static int __init imx_rngc_probe(struct platform_device *pdev)
> if (irq < 0)
> return irq;
>
> - clk_prepare_enable(rngc->clk);
> + ret = clk_prepare_enable(rngc->clk);
> + if (ret)
> + return ret;
[Severity: High]
This isn't a bug introduced by this patch, but I have a question about the
clock management during probe failure.
If imx_rngc_self_test() fails later in imx_rngc_probe(), the driver manually
disables the clock:
if (self_test) {
ret = imx_rngc_self_test(rngc);
if (ret) {
clk_disable_unprepare(rngc->clk);
return dev_err_probe(&pdev->dev, ret, "self test failed\n");
}
}
However, the IRQ is registered with devm_request_irq() just before the
self-test, so it remains active until devres unwinds.
If a spurious interrupt occurs, or if the hardware asserts an interrupt from
the failed self-test right after the clock is disabled, the IRQ handler will
execute:
drivers/char/hw_random/imx-rngc.c:imx_rngc_irq() {
...
status = readl(rngc->base + RNGC_STATUS);
rngc->err_reg = readl(rngc->base + RNGC_ERROR);
...
}
Could this cause a synchronous external abort due to the MMIO access without
the clock being enabled?
Should the IRQ be disabled or unregistered before disabling the clock in this
error path, or should the IRQ handler verify the PM state before accessing
registers?
[Severity: High]
This is also a pre-existing issue, but does this driver have a potential race
condition with its completion mechanism?
The driver initializes the completion once in imx_rngc_probe():
init_completion(&rngc->rng_op_done);
It then waits on this completion in imx_rngc_init():
ret = wait_for_completion_timeout(&rngc->rng_op_done,
msecs_to_jiffies(RNGC_SEED_TIMEOUT));
And also in imx_rngc_self_test():
ret = wait_for_completion_timeout(&rngc->rng_op_done,
usecs_to_jiffies(RNGC_SELFTEST_TIMEOUT));
If a hardware operation times out, the function returns an error without
consuming a completion event.
If the hardware finishes the operation shortly after the timeout and triggers
the interrupt handler:
drivers/char/hw_random/imx-rngc.c:imx_rngc_irq() {
...
if (status & (RNGC_STATUS_SEED_DONE | RNGC_STATUS_ST_DONE))
complete(&rngc->rng_op_done);
...
}
This increments the completion counter. Because the driver never calls
reinit_completion(), could the next call to imx_rngc_init() or
imx_rngc_self_test() immediately return success from
wait_for_completion_timeout() without actually waiting for the new operation
to finish?
Could this lead to reading uninitialized or stale data, compromising the
random number generator's security?
[ ... ]
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260830131253.2718-1-pengpeng@iscas.ac.cn?part=1
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH] hwrng: imx-rngc: propagate clock enable errors
2026-08-30 13:12 [PATCH] hwrng: imx-rngc: propagate clock enable errors Pengpeng Hou
2026-08-30 13:24 ` sashiko-bot
@ 2026-08-31 15:49 ` Frank Li
2026-09-01 13:40 ` Martin Kaiser
2 siblings, 0 replies; 4+ messages in thread
From: Frank Li @ 2026-08-31 15:49 UTC (permalink / raw)
To: Pengpeng Hou
Cc: Olivia Mackall, Herbert Xu, Frank Li, Sascha Hauer,
Pengutronix Kernel Team, Fabio Estevam, linux-crypto, imx,
linux-arm-kernel, linux-kernel
On Sun, Aug 30, 2026 at 09:12:53PM +0800, Pengpeng Hou wrote:
> imx_rngc_probe() reads hardware registers after enabling its clock without
> checking the result, and the runtime resume callback always reports success
> after the same unchecked operation.
>
> Propagate clock-enable errors before accessing the RNG and to the
> runtime-PM
> core on resume.
Nit: wrap to first line
Reviewed-by: Frank Li <Frank.Li@nxp.com>
>
> Fixes: 1d5449445bd0 ("hwrng: mx-rngc - add a driver for Freescale RNGC")
> Signed-off-by: Pengpeng Hou <pengpeng@iscas.ac.cn>
> ---
> drivers/char/hw_random/imx-rngc.c | 8 ++++----
> 1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/drivers/char/hw_random/imx-rngc.c b/drivers/char/hw_random/imx-rngc.c
> index 7693328f10564..00a21d62c79c3 100644
> --- a/drivers/char/hw_random/imx-rngc.c
> +++ b/drivers/char/hw_random/imx-rngc.c
> @@ -266,7 +266,9 @@ static int __init imx_rngc_probe(struct platform_device *pdev)
> if (irq < 0)
> return irq;
>
> - clk_prepare_enable(rngc->clk);
> + ret = clk_prepare_enable(rngc->clk);
> + if (ret)
> + return ret;
>
> ver_id = readl(rngc->base + RNGC_VER_ID);
> rng_type = FIELD_GET(RNG_TYPE, ver_id);
> @@ -338,9 +340,7 @@ static int imx_rngc_resume(struct device *dev)
> {
> struct imx_rngc *rngc = dev_get_drvdata(dev);
>
> - clk_prepare_enable(rngc->clk);
> -
> - return 0;
> + return clk_prepare_enable(rngc->clk);
> }
>
> static const struct dev_pm_ops imx_rngc_pm_ops = {
>
> base-commit: 08dbfad3f5040f5bdb6c529da20d6d4e81fefd72
> --
> 2.50.1
>
>
^ permalink raw reply [flat|nested] 4+ messages in thread* Re: [PATCH] hwrng: imx-rngc: propagate clock enable errors
2026-08-30 13:12 [PATCH] hwrng: imx-rngc: propagate clock enable errors Pengpeng Hou
2026-08-30 13:24 ` sashiko-bot
2026-08-31 15:49 ` Frank Li
@ 2026-09-01 13:40 ` Martin Kaiser
2 siblings, 0 replies; 4+ messages in thread
From: Martin Kaiser @ 2026-09-01 13:40 UTC (permalink / raw)
To: Pengpeng Hou
Cc: Olivia Mackall, Herbert Xu, Frank Li, Sascha Hauer,
Pengutronix Kernel Team, Fabio Estevam, linux-crypto, imx,
linux-arm-kernel, linux-kernel
Thus wrote Pengpeng Hou (pengpeng@iscas.ac.cn):
> imx_rngc_probe() reads hardware registers after enabling its clock without
> checking the result, and the runtime resume callback always reports success
> after the same unchecked operation.
> Propagate clock-enable errors before accessing the RNG and to the
> runtime-PM
> core on resume.
> Fixes: 1d5449445bd0 ("hwrng: mx-rngc - add a driver for Freescale RNGC")
> Signed-off-by: Pengpeng Hou <pengpeng@iscas.ac.cn>
> ---
> drivers/char/hw_random/imx-rngc.c | 8 ++++----
> 1 file changed, 4 insertions(+), 4 deletions(-)
> diff --git a/drivers/char/hw_random/imx-rngc.c b/drivers/char/hw_random/imx-rngc.c
> index 7693328f10564..00a21d62c79c3 100644
> --- a/drivers/char/hw_random/imx-rngc.c
> +++ b/drivers/char/hw_random/imx-rngc.c
> @@ -266,7 +266,9 @@ static int __init imx_rngc_probe(struct platform_device *pdev)
> if (irq < 0)
> return irq;
> - clk_prepare_enable(rngc->clk);
> + ret = clk_prepare_enable(rngc->clk);
> + if (ret)
> + return ret;
> ver_id = readl(rngc->base + RNGC_VER_ID);
> rng_type = FIELD_GET(RNG_TYPE, ver_id);
> @@ -338,9 +340,7 @@ static int imx_rngc_resume(struct device *dev)
> {
> struct imx_rngc *rngc = dev_get_drvdata(dev);
> - clk_prepare_enable(rngc->clk);
> -
> - return 0;
> + return clk_prepare_enable(rngc->clk);
> }
> static const struct dev_pm_ops imx_rngc_pm_ops = {
> base-commit: 08dbfad3f5040f5bdb6c529da20d6d4e81fefd72
> --
> 2.50.1
The same patch was sent 2 days ago.
https://lore.kernel.org/linux-crypto/apHlIGM3_GHf86fK@SMW015318/T/#t
^ permalink raw reply [flat|nested] 4+ messages in thread