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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
- [High] Unhandled error from `pm_runtime_get_sync()` leads to unsafe register access.
- [High] `starfive_trng_release()` causes a clock double-disable and hardware sequence violation if the device is suspended.
- [High] Devres ordering creates a race condition where the IRQ handler remains active after clocks are disabled.
- [High] The IRQ handler accesses hardware registers without verifying the device's PM state.
- [High] The IRQ handler is registered before hardware clocks are enabled, creating a window for a fatal crash.
- [High] A stale IRQ completion race allows `starfive_trng_read()` to return non-random bytes prematurely.
--

commit 9fe8263576b446ca8c08d308b365ff7601cef468
Author: Lianfeng Ouyang <lianfeng.ouyang@starfivetech.com>
Subject: hwrng: starfive: Update clk and reset sequence

This commit updates the clock and reset sequencing for the StarFive TRNG
driver to prevent glitches on the jhb100 platform. It introduces a specific
release function and integrates runtime PM across initialization, reading,
and cleanup.

> 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
> @@ -138,6 +143,23 @@ static inline void starfive_trng_irq_mask_clear(struct starfive_trng *trng)
>  	writel(data, trng->base + STARFIVE_ISTAT);
>  }
>  
> +static void starfive_trng_release(void *data)
> +{
> +	struct starfive_trng *trng = data;
> +
> +	pm_runtime_disable(trng->dev);
> +	pm_runtime_dont_use_autosuspend(trng->dev);
> +
> +	if (trng->hw_seq == HW_SEQ_RESET_FIRST_THEN_CLK)
> +		reset_control_assert(trng->rst);
> +
> +	clk_disable_unprepare(trng->ahb);
> +	clk_disable_unprepare(trng->hclk);

If the device is already runtime-suspended (e.g., autosuspend timer expired),
starfive_trng_suspend() would have already disabled these clocks. Since
pm_runtime_disable() does not wake up a suspended device, will calling
clk_disable_unprepare() here result in a clock enable count underflow?

Also, for the HW_SEQ_RESET_FIRST_THEN_CLK variant, could this assert the
hardware reset on a clock-gated IP block? Should the release function wake
the device (via pm_runtime_get_sync) before performing hardware teardown?

[ ... ]
> @@ -174,13 +196,16 @@ static int starfive_trng_init(struct hwrng *rng)
>  {
>  	struct starfive_trng *trng = to_trng(rng);
>  	u32 mode, intr = 0;
> +	int ret;
> +
> +	pm_runtime_get_sync(trng->dev);

Is it safe to ignore the return value of pm_runtime_get_sync() here? If the
device fails to resume, it returns a negative error code. Would the
subsequent register accesses (like writel/readl) occur on a device with
disabled clocks, potentially causing a synchronous external abort?

Does this same unhandled return value issue apply to the pm_runtime_get_sync()
calls added in starfive_trng_read() and starfive_trng_cleanup() as well?

[ ... ]
> @@ -257,24 +292,26 @@ static int starfive_trng_read(struct hwrng *rng, void *buf, size_t max, bool wai
>  	if (wait) {
>  		ret = starfive_trng_wait_idle(trng);
>  		if (ret)
> -			return -ETIMEDOUT;
> +			goto end;
>  	}
>  
>  	ret = starfive_trng_cmd(trng, STARFIVE_CTRL_GENE_RANDNUM, wait);

Even though starfive_trng_wait_idle() polls until the hardware is idle,
could there still be a pending IRQ from a previous operation that hasn't
been processed by the CPU yet?

If that pending IRQ is handled right after reinit_completion() is called
inside starfive_trng_cmd(), would it complete the random_done completion
early, causing this function to read from STARFIVE_RAND0 while the hardware
is still actively generating the new random number?

[ ... ]
> @@ -283,21 +320,22 @@ static int starfive_trng_probe(struct platform_device *pdev)
[ ... ]
>  	init_completion(&trng->random_done);
>  	init_completion(&trng->reseed_done);
>  	spin_lock_init(&trng->write_lock);
>  
> -	ret = devm_request_irq(&pdev->dev, irq, starfive_trng_irq, 0, pdev->name,
> +	ret = devm_request_irq(&pdev->dev, trng->irq, starfive_trng_irq, 0, pdev->name,
>  			       (void *)trng);

This registers the IRQ before the hardware clocks are enabled below. If an
interrupt is pending from a prior boot stage, will starfive_trng_irq()
fire immediately and attempt to read STARFIVE_ISTAT while clocks are still
gated off, potentially triggering a bus fault?

Additionally, does starfive_trng_irq() need to check the PM state using
pm_runtime_get_if_active() before accessing registers? If a spurious or
delayed interrupt fires while the device is runtime suspended, the handler
currently reads registers unconditionally.

[ ... ]
>  	pm_runtime_use_autosuspend(&pdev->dev);
>  	pm_runtime_set_autosuspend_delay(&pdev->dev, 100);
> +	pm_runtime_set_active(&pdev->dev);
>  	pm_runtime_enable(&pdev->dev);
>  
> -	ret = devm_hwrng_register(&pdev->dev, &trng->rng);
> -	if (ret) {
> -		pm_runtime_disable(&pdev->dev);
> -
> -		reset_control_assert(trng->rst);
> -		clk_disable_unprepare(trng->ahb);
> -		clk_disable_unprepare(trng->hclk);
> +	ret = devm_add_action_or_reset(&pdev->dev, starfive_trng_release, trng);

Devres actions execute in reverse order of creation. Since
devm_request_irq() is called before devm_add_action_or_reset(), won't
starfive_trng_release() run first during device unbind and disable the
clocks, leaving the IRQ active?

If an interrupt fires during this window, wouldn't it cause a bus fault
when the handler reads from a clock-gated IP?

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