Re: [PATCH v1] hwrng: imx-rngc: Disable clock on registration failure

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:

New issues:
- [High] The error path for `devm_hwrng_register()` introduces a potential double clock disable and PM state inconsistency by manually disabling the clock but leaving the runtime PM state as `RPM_ACTIVE`.

Pre-existing issues:
- [High] If `set_current_rng()` fails during `hwrng_register()`, the RNG is left in `rng_list` but an error is returned, leading to a freed object remaining on the global list.
- [High] Clock leaked on driver unbind due to an asynchronous `pm_runtime_put()` being cancelled by `pm_runtime_disable()`.
- [High] Unclocked hardware register access in the IRQ handler risks bus faults.
- [Medium] The device clock is permanently leaked if the device is successfully registered but not selected as the system's current RNG.
--

--- Patch [1]: [PATCH v1] hwrng: imx-rngc: Disable clock on registration failure ---
commit b5aba9259f12063cf29726871d90c2519f6fb937
Author: Yuho Choi <dbgh9129@gmail.com>

hwrng: imx-rngc: Disable clock on registration failure
    
The RNGC clock is enabled manually before runtime PM is configured. If
devm_hwrng_register() fails, probe returns without disabling the clock.
The devm_pm_runtime_enable() cleanup only disables runtime PM and does not
call imx_rngc_suspend().
    
Disable the clock before returning from this failure path.

> diff --git a/drivers/char/hw_random/imx-rngc.c b/drivers/char/hw_random/imx-rngc.c
> index 28c56c2d1bf6d..a2db26d7f790e 100644
> --- a/drivers/char/hw_random/imx-rngc.c
> +++ b/drivers/char/hw_random/imx-rngc.c
> @@ -313,8 +313,10 @@ static int __init imx_rngc_probe(struct platform_device *pdev)
>  	devm_pm_runtime_enable(&pdev->dev);
>  
>  	ret = devm_hwrng_register(&pdev->dev, &rngc->rng);
> -	if (ret)
> +	if (ret) {
> +		clk_disable_unprepare(rngc->clk);
>  		return dev_err_probe(&pdev->dev, ret, "hwrng registration failed\n");
> +	}

[Severity: High]
Does this manual clock disable leave the PM state inconsistent or risk a double
disable?

The device was previously set to RPM_ACTIVE and runtime PM was enabled. If
runtime PM asynchronously suspends the device before this failure path is
reached, imx_rngc_suspend() may have already disabled the clock. Additionally,
leaving the PM state as RPM_ACTIVE while the hardware is powered down might
require calling pm_runtime_disable() and pm_runtime_set_suspended() to
synchronize the state.

[Severity: Medium]
This is a pre-existing issue, but does probe leak the clock if the device is
successfully registered but not selected as the system's current RNG?

Probe leaves the device in RPM_ACTIVE without queuing an autosuspend. If
hwrng_register() completes successfully but does not select this RNG as active,
its .init callback (which normally performs a PM put) is never invoked, leaving
the device stranded in RPM_ACTIVE and the clock permanently enabled.


[Severity: High]
This isn't a bug introduced by this patch, but can a registration failure
corrupt the global rng_list?

If set_current_rng() fails during hwrng_register() because the RNG's .init
callback returns an error:

drivers/char/hw_random/core.c:hwrng_register() {
    ...
	list_add_tail(&rng->list, &rng_list);
    ...
		err = set_current_rng(rng);
		if (err)
			goto out_unlock;
    ...
out_unlock:
	mutex_unlock(&rng_mutex);
	return err;
}

The RNG is left in rng_list, but an error is returned. Devres will then free
the struct hwrng memory, leaving a freed pointer on the global list.


[Severity: High]
This is a pre-existing issue, but does driver unbind leak the clock?

During unbind, devm_hwrng_release() invokes the cleanup callback:

drivers/char/hw_random/imx-rngc.c:imx_rngc_cleanup() {
    ...
	pm_runtime_put(rngc->dev);
}

This performs an asynchronous put. The devres stack then cleans up
devm_pm_runtime_enable() by calling pm_runtime_disable(), which synchronously
cancels any pending async suspend work. Because imx_rngc_suspend() may never
execute, the hardware can remain powered indefinitely.


[Severity: High]
This is a pre-existing issue, but can the IRQ handler trigger a bus fault by
accessing unclocked hardware registers?

drivers/char/hw_random/imx-rngc.c:imx_rngc_irq() {
    ...
	status = readl(rngc->base + RNGC_STATUS);
	rngc->err_reg = readl(rngc->base + RNGC_ERROR);
    ...
}

Because the driver configures autosuspend and does not mask the interrupt
during runtime suspend, a spurious or delayed interrupt can fire concurrently
while the clock is disabled. Without synchronize_irq() in the suspend path or
pm_runtime_get_if_active() before register access, this causes a synchronous
external abort.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260802222259.577200-1-dbgh9129@gmail.com?part=1