Re: [PATCH v3 2/2] hwrng: starfive: rework clk/reset teardown order for JHB100 & fix RPM

[sashiko-bot@kernel.org]

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

New issues:
- [High] IRQ handler incorrectly evaluates pm_runtime_get_if_active() return value, leading to PM usage counter underflow and fatal unclocked hardware access.

Pre-existing issues:
- [High] Missing return value checks for clk_prepare_enable() leads to potential kernel panics on resource activation failure.
--

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

hwrng: starfive: rework clk/reset teardown order for JHB100 & fix RPM
    
JHB100: to avoid RDC glitch, assert reset only after clocks are gated.
Introduce a per-compatible seq_rst_clk flag (RST_FIRST/CLK_FIRST)
selected via of_device_get_match_data().

> diff --git a/drivers/char/hw_random/jh7110-trng.c b/drivers/char/hw_random/jh7110-trng.c
> index 9776f4daa0445..9c164a0011ae6 100644
> --- a/drivers/char/hw_random/jh7110-trng.c
> +++ b/drivers/char/hw_random/jh7110-trng.c

[ ... ]

> @@ -209,16 +265,17 @@ static irqreturn_t starfive_trng_irq(int irq, void *priv)
>  	u32 status;
>  	struct starfive_trng *trng = (struct starfive_trng *)priv;
>  
> +	if (!pm_runtime_get_if_active(trng->dev)) {

[Severity: High]
Does this evaluation properly handle negative error codes?

If pm_runtime_get_if_active() returns -EINVAL when Runtime PM is disabled
(for example, during probe before pm_runtime_enable() is called), the
!(-EINVAL) check evaluates to false.

Since devm_request_irq() is called before clocks are enabled in probe, a
pending interrupt from a previous boot stage could trigger this handler. It
would bypass the early return and perform an MMIO read on STARFIVE_ISTAT
while clocks are off, which might cause a synchronous external abort.

Could we check for ret <= 0 instead?

> +		dev_err_ratelimited(trng->dev, "pm is inactive in irq\n");
> +		return IRQ_NONE;
> +	}

[ ... ]

> @@ -228,18 +285,37 @@ static irqreturn_t starfive_trng_irq(int irq, void *priv)
>  		spin_unlock(&trng->write_lock);
>  	}
>  
> +	if (status & STARFIVE_ISTAT_RAND_RDY)
> +		complete(&trng->random_done);
> +
> +	if (status & STARFIVE_ISTAT_SEED_DONE)
> +		complete(&trng->reseed_done);
> +
> +	pm_runtime_put_noidle(trng->dev);

[Severity: High]
Could this lead to a PM usage counter underflow?

If pm_runtime_get_if_active() returned -EINVAL earlier, we didn't increment
the usage counter, but we unconditionally decrement it here. This might steal
the reference held by the driver core during probe.

> +
>  	return IRQ_HANDLED;
>  }

[ ... ]

> @@ -333,18 +429,16 @@ static int starfive_trng_probe(struct platform_device *pdev)
>  
>  	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);

[Severity: High]
This isn't a bug introduced by this patch, but should the return values of
the clock enable calls be verified?

In starfive_trng_probe() and starfive_trng_resume(), clk_prepare_enable()
is called for both the hclk and ahb clocks without checking for failure:

clk_prepare_enable(trng->hclk);
clk_prepare_enable(trng->ahb);

If clock activation fails under resource pressure, the driver will proceed as
if the hardware is powered. Subsequent MMIO accesses might trigger a
fatal bus error.

> +	ret = devm_add_action_or_reset(&pdev->dev, starfive_trng_release, trng);
> +	if (ret)
> +		return ret;
>  
> +	ret = devm_hwrng_register(&pdev->dev, &trng->rng);
> +	if (ret)
>  		return dev_err_probe(&pdev->dev, ret, "Failed to register hwrng\n");
> -	}
>  
>  	return 0;
>  }

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