Re: [PATCH v9 2/2] hwrng: starfive: rework clk/reset teardown order for JHB100

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] hwrng: starfive: unqueued need_reseed stalls non-blocking starfive_trng_read()
- [Medium] hwrng: starfive: unbalanced enable_irq() in starfive_trng_init()
--

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

hwrng: starfive: rework clk/reset teardown order for JHB100

This commit reworks clock and reset power-down ordering in the StarFive TRNG
driver to support the JHB100 SoC alongside JH7110. It also updates runtime PM
handling, moves SEU-triggered reseeding to a workqueue, and serializes
command execution with a mutex.

> diff --git a/drivers/char/hw_random/jh7110-trng.c b/drivers/char/hw_random/jh7110-trng.c
> index aee12caab5780..8678b35ecd4a5 100644
> --- a/drivers/char/hw_random/jh7110-trng.c
> +++ b/drivers/char/hw_random/jh7110-trng.c

[ ... ]

> @@ -120,276 +157,496 @@ MODULE_PARM_DESC(autoage, "Auto-reseeding after specified timer countdowns to 0:
>  				" 0 - disable timer\n"
>  				" other - reload value for internal timer");
>  
> -static inline int starfive_trng_wait_idle(struct starfive_trng *trng)
> +static bool starfive_trng_idle(struct starfive_trng *trng, bool wait)
>  {
> -	u32 stat;
> +	u32 stat, busy = STARFIVE_STAT_RAND_GENERATING | STARFIVE_STAT_RAND_SEEDING;
>  
> -	return readl_relaxed_poll_timeout(trng->base + STARFIVE_STAT, stat,
> -					  !(stat & (STARFIVE_STAT_RAND_GENERATING |
> -						    STARFIVE_STAT_RAND_SEEDING)),
> -					  10, 100000);
> +	if (!wait)
> +		return !(readl_relaxed(trng->base + STARFIVE_STAT) & busy);
> +
> +	return !readl_relaxed_poll_timeout(trng->base + STARFIVE_STAT, stat,
> +					   !(stat & busy),
> +					   10, 100000);
>  }
>  
> -static inline void starfive_trng_irq_mask_clear(struct starfive_trng *trng)
> +static void starfive_trng_irq_clear(struct starfive_trng *trng)
>  {
> -	/* clear register: ISTAT */
> -	u32 data = readl(trng->base + STARFIVE_ISTAT);
> +	u32 istat = readl(trng->base + STARFIVE_ISTAT);
> +
> +	/* Do not lose an LFSR lockup that has not been serviced yet. */
> +	if (istat & STARFIVE_ISTAT_LFSR_LOCKUP)
> +		WRITE_ONCE(trng->need_reseed, true);
>  
> -	writel(data, trng->base + STARFIVE_ISTAT);
> +	writel(istat, trng->base + STARFIVE_ISTAT);
>  }

[ ... ]

> +static int starfive_trng_init(struct hwrng *rng)
> +{
> +	struct starfive_trng *trng = to_trng(rng);
> +	int ret;
> +
> +	ret = pm_runtime_resume_and_get(trng->dev);
> +	if (ret < 0) {
> +		dev_err(trng->dev, "Failed to wake device for init: %d\n", ret);
> +		return ret;
> +	}
> +
> +	mutex_lock(&trng->lock);
> +
> +	starfive_trng_irq_clear(trng);
> +	starfive_trng_hw_setup(trng);
> +
> +	enable_irq(trng->irq);
> +	writel(STARFIVE_IE_ALL, trng->base + STARFIVE_IE);
> +
> +	ret = starfive_trng_reseed(trng);
> +	if (ret) {
> +		writel(0, trng->base + STARFIVE_IE);
> +		disable_irq(trng->irq);
> +		starfive_trng_irq_clear(trng);
> +	} else {
> +		trng->initialized = true;
> +	}

[Severity: Medium]
Can starfive_trng_init() in drivers/char/hw_random/jh7110-trng.c cause an
unbalanced enable_irq(trng->irq) call when the RNG is deselected and quickly
reselected via /sys/class/misc/hw_random/rng_current?

In drivers/char/hw_random/core.c, deselecting the current RNG in
drop_current_rng() drops rng->ref to 0 and schedules rng->cleanup_work
asynchronously via cleanup_rng() -> schedule_work(&rng->cleanup_work).

If the RNG is selected again via rng_current_store() -> set_current_rng() ->
hwrng_init() before cleanup_rng_work() acquires rng_mutex, hwrng_init() sees
rng->ref == 0, invokes starfive_trng_init(), and sets rng->ref back to 1.
When cleanup_rng_work() subsequently runs, it skips starfive_trng_cleanup():

drivers/char/hw_random/core.c:cleanup_rng_work() {
	...
	/* Skip if rng has been reinitialized. */
	if (kref_read(&rng->ref)) {
		mutex_unlock(&rng_mutex);
		return;
	}

	if (rng->cleanup)
		rng->cleanup(rng);
	...
}

In that case, starfive_trng_init() runs while trng->initialized is still true
and trng->irq is already enabled, so calling enable_irq(trng->irq)
unconditionally triggers an unbalanced IRQ enable warning in __enable_irq().

Furthermore, if starfive_trng_reseed() fails during that re-initialization,
the error path disables trng->irq without clearing trng->initialized, and
when cleanup_rng_work() later runs for the failed init,
starfive_trng_cleanup() will call disable_irq(trng->irq) a second time and
leave TRNG interrupts permanently disabled.

Should starfive_trng_init() check trng->initialized before enabling trng->irq
(and clear trng->initialized on the error path)?

[ ... ]

>  static int starfive_trng_read(struct hwrng *rng, void *buf, size_t max, bool wait)
>  {
>  	struct starfive_trng *trng = to_trng(rng);
> -	int ret;
> -
> -	pm_runtime_get_sync(trng->dev);
> +	unsigned long timeout;
> +	int ret, retry = 1;
>  
> -	if (trng->mode == PRNG_256BIT)
> -		max = min_t(size_t, max, (STARFIVE_RAND_LEN * 8));
> -	else
> -		max = min_t(size_t, max, (STARFIVE_RAND_LEN * 4));
> +	ret = pm_runtime_resume_and_get(trng->dev);
> +	if (ret < 0) {
> +		dev_warn_ratelimited(trng->dev, "Failed to wake device for read: %d\n", ret);
> +		return ret;
> +	}
>  
>  	if (wait) {
> -		ret = starfive_trng_wait_idle(trng);
> -		if (ret) {
> -			ret = -ETIMEDOUT;
> +		mutex_lock(&trng->lock);
> +		timeout = msecs_to_jiffies(STARFIVE_RANDGEN_TIMEOUT_MS);
> +	} else {
> +		/* allow up to 1 jiffy for wait == 0 */
> +		if (!mutex_trylock(&trng->lock)) {
> +			ret = 0;
>  			goto out_put;
>  		}
> +		timeout = STARFIVE_RANDGEN_NOWAIT_TIMEOUT;
>  	}
>  
> -	ret = starfive_trng_cmd(trng, STARFIVE_CTRL_GENE_RANDNUM, wait);
> -	if (ret)
> -		goto out_put;
> +	for (;;) {
> +		/*
> +		 * A pending LFSR lockup taints the next sample. Blocking
> +		 * callers reseed here; non-blocking callers return no data,
> +		 * the reseed work is already queued.
> +		 */
> +		if (READ_ONCE(trng->need_reseed)) {
> +			if (!wait) {
> +				ret = 0;
> +				goto out_unlock;
> +			}
> +			ret = starfive_trng_reseed(trng);
> +			if (ret)
> +				goto out_unlock;
> +		}

[Severity: Medium]
Could non-blocking calls to starfive_trng_read() (wait == false) in
drivers/char/hw_random/jh7110-trng.c stall indefinitely if trng->need_reseed
is set without trng->work being queued?

In starfive_trng_irq_clear(), if STARFIVE_ISTAT_LFSR_LOCKUP is set in
STARFIVE_ISTAT, trng->need_reseed is set to true and the hardware interrupt
status bit is cleared without queuing trng->work:

drivers/char/hw_random/jh7110-trng.c:starfive_trng_irq_clear() {
	u32 istat = readl(trng->base + STARFIVE_ISTAT);

	/* Do not lose an LFSR lockup that has not been serviced yet. */
	if (istat & STARFIVE_ISTAT_LFSR_LOCKUP)
		WRITE_ONCE(trng->need_reseed, true);

	writel(istat, trng->base + STARFIVE_ISTAT);
}

During runtime suspend and resume, starfive_trng_suspend() masks STARFIVE_IE
before calling starfive_trng_irq_clear(), and starfive_trng_resume() calls
starfive_trng_irq_clear() before re-enabling STARFIVE_IE_ALL:

drivers/char/hw_random/jh7110-trng.c:starfive_trng_suspend() {
	...
	writel(0, trng->base + STARFIVE_IE);
	starfive_trng_irq_clear(trng);
	...
}

drivers/char/hw_random/jh7110-trng.c:starfive_trng_resume() {
	...
	if (trng->initialized) {
		starfive_trng_irq_clear(trng);
		writel(STARFIVE_IE_ALL, trng->base + STARFIVE_IE);
	}
	...
}

If STARFIVE_ISTAT_LFSR_LOCKUP is observed and cleared in either path,
starfive_trng_irq() will not fire for that event and trng->work is never
queued. Similarly, if a previous reseed attempt in starfive_trng_reseed_work()
or starfive_trng_read() failed, trng->need_reseed remains true after
trng->work is no longer pending.

Because the !wait path in starfive_trng_read() assumes trng->work is already
queued and returns 0 immediately, wouldn't subsequent non-blocking reads keep
returning 0 until a blocking read occurs? Should the !wait path (or
starfive_trng_irq_clear() / starfive_trng_resume()) queue trng->work when
trng->need_reseed is true?

[ ... ]

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