Re: [PATCH v1 2/2] hwrng: starfive: Update clk and reset sequence

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] The patch fails to guarantee that clocks are physically gated before asserting reset in cleanup(), rendering the glitch prevention ineffective when the device is active.
- [High] Misuse of hwrng cleanup and init callbacks leads to system crashes on RNG toggle.
- [High] Unsafe IRQ handler execution after clocks are gated in cleanup() causes bus faults.
- [High] Broken Runtime PM state initialization causes permanent clock leakage.
--

commit 097b8dfdf62a7320bf5e3edd8a668729f1364217
Author: Lianfeng Ouyang <lianfeng.ouyang@starfivetech.com>

hwrng: starfive: Update clk and reset sequence

For jhb100, this conducts clock gating before asserting reset to prevent
generating glitches that propagate to downstream IP.

> diff --git a/drivers/char/hw_random/jh7110-trng.c b/drivers/char/hw_random/jh7110-trng.c
> --- a/drivers/char/hw_random/jh7110-trng.c
> +++ b/drivers/char/hw_random/jh7110-trng.c
> @@ -234,12 +234,18 @@ static irqreturn_t starfive_trng_irq(int irq, void *priv)
>  static void starfive_trng_cleanup(struct hwrng *rng)
>  {
>  	struct starfive_trng *trng = to_trng(rng);
> +	bool is_jhb100 = device_is_compatible(trng->dev, "starfive,jhb100-trng");
>  
>  	writel(0, trng->base + STARFIVE_CTRL);
>  
> -	reset_control_assert(trng->rst);
> +	if (!is_jhb100)
> +		reset_control_assert(trng->rst);
> +
>  	clk_disable_unprepare(trng->hclk);
>  	clk_disable_unprepare(trng->ahb);
> +
> +	if (is_jhb100)
> +		reset_control_assert(trng->rst);
>  }

Does this sequence guarantee the clocks are physically gated before asserting
the reset?

Because the probe function manually enables the clocks without calling
pm_runtime_set_active(), the physical clock count starts at 1. If the RNG is
deselected while the device is still active (for example, within the 100ms
autosuspend window after a read), the clk_disable_unprepare() calls here
will only decrement the clock count to 1.

Could the physical clocks remain running when reset_control_assert() is
executed, generating the exact downstream glitch this patch intends to
prevent? The driver may need to synchronize with Runtime PM first.

This isn't a bug introduced by this patch, but does managing hardware states
this way in cleanup() lead to a crash if the RNG is re-selected? The hwrng
core invokes cleanup() when the RNG is deselected via sysfs, which drops the
clock count.

If the user subsequently re-selects the RNG, the core invokes
starfive_trng_init(). Since init() does not re-enable the clocks or call
pm_runtime_get_sync(), wouldn't its immediate writel() to the
STARFIVE_AUTO_AGE register trigger a synchronous external abort?

Also pre-existing, but does gating the clocks here leave the device
vulnerable to a bus fault? The interrupt handler remains fully active
because cleanup() can be executed at runtime via sysfs without unbinding the
driver.

If a spurious interrupt or delayed lockup interrupt fires while the RNG is
deselected, wouldn't starfive_trng_irq() attempt to read registers with the
clocks gated? Should the driver clear the interrupt enable register and
synchronize IRQs before gating the clocks?

[ ... ]

> @@ -337,12 +343,19 @@ static int starfive_trng_probe(struct platform_device *pdev)
>  
>  	ret = devm_hwrng_register(&pdev->dev, &trng->rng);
>  	if (ret) {
> +		bool is_jhb100 = device_is_compatible(trng->dev, "starfive,jhb100-trng");
> +
>  		pm_runtime_disable(&pdev->dev);
>  
> -		reset_control_assert(trng->rst);
> +		if (!is_jhb100)
> +			reset_control_assert(trng->rst);
> +
>  		clk_disable_unprepare(trng->ahb);
>  		clk_disable_unprepare(trng->hclk);
>  
> +		if (is_jhb100)
> +			reset_control_assert(trng->rst);
> +
>  		return dev_err_probe(&pdev->dev, ret, "Failed to register hwrng\n");
>  	}

This isn't a bug in the new error handling, but earlier in the probe sequence,
the driver manually invokes clk_prepare_enable() for both clocks, initializing
their enable counts to 1, but fails to call pm_runtime_set_active().

Doesn't the PM core therefore default the runtime status to RPM_SUSPENDED?

Every time starfive_trng_read() executes, pm_runtime_get_sync() transitions
the device to RPM_ACTIVE, calling starfive_trng_resume() which increments
the clock counts to 2. When the autosuspend timer invokes
starfive_trng_suspend(), it decrements the counts back to 1.

Does this mean the clocks are never physically turned off during normal
operation, completely preventing the hardware from powering down?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260512062404.4540-1-lianfeng.ouyang@starfivetech.com?part=2