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

[sashiko-bot@kernel.org]

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

New issues:
- [High] The devm release action skips hardware reset assertion due to a synchronous suspend in the cleanup path.
- [High] Failing to mask interrupts during runtime suspend leads to an unhandled interrupt storm during device resume.

Pre-existing issues:
- [High] The device IRQ is requested before hardware and PM are initialized, risking an immediate IRQ storm during boot.
- [High] Non-blocking reads invariably fail with -ETIMEDOUT due to a 0-jiffy timeout calculation.
--

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

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

Reworks the StarFive TRNG driver to handle JHB100 hardware-specific
requirements for clk/reset teardown. Adds Runtime PM integration to manage
device power state and includes a devm action for cleanup.

> diff --git a/drivers/char/hw_random/jh7110-trng.c b/drivers/char/hw_random/jh7110-trng.c
> index 9776f4daa0445..cafc873b9ebf8 100644
> --- a/drivers/char/hw_random/jh7110-trng.c
> +++ b/drivers/char/hw_random/jh7110-trng.c
> @@ -138,6 +154,31 @@ 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;
> +
> +	if (!pm_runtime_status_suspended(trng->dev)) {

[Severity: High]
Does this PM state check accidentally skip hardware reset assertion during
driver unbind?

The HWRNG unregisters first during teardown, which calls
starfive_trng_cleanup() where pm_runtime_put_sync() synchronously suspends
the device. When this devm action executes immediately after, the device is
already suspended, making this condition false and completely bypassing the
reset_control_assert(trng->rst) logic.

Could checking pm_runtime_status_suspended() outside a lock also leave a
race window where autosuspend might trigger and cause double clock disables?

> +		writel(0, trng->base + STARFIVE_IE);
> +		starfive_trng_irq_clear(trng);
> +
> +		if (trng->irq >= 0)
> +			synchronize_irq(trng->irq);
> +
> +		if (trng->data->seq_rst_clk == SEQ_RST_FIRST)
> +			reset_control_assert(trng->rst);
> +
> +		clk_disable_unprepare(trng->ahb);
> +		clk_disable_unprepare(trng->hclk);
> +
> +		if (trng->data->seq_rst_clk == SEQ_CLK_FIRST)
> +			reset_control_assert(trng->rst);
> +	}
> +
> +	pm_runtime_dont_use_autosuspend(trng->dev);
> +	pm_runtime_disable(trng->dev);
> +}
> +

[ ... ]

> @@ -201,24 +251,33 @@ static int starfive_trng_init(struct hwrng *rng)
>  
>  static irqreturn_t starfive_trng_irq(int irq, void *priv)
>  {
> +	int ret;
>  	u32 status;
>  	struct starfive_trng *trng = (struct starfive_trng *)priv;
>  
> +	ret = pm_runtime_get_if_active(trng->dev);
> +	if (ret <= 0) {
> +		dev_err_ratelimited(trng->dev, "pm is inactive in irq\n");
> +		return IRQ_NONE;
> +	}

[Severity: High]
Will this return cause an infinite interrupt storm if an interrupt fires
during device resume?

If starfive_trng_suspend() doesn't mask device interrupts via STARFIVE_IE,
an interrupt could trigger while suspended or when clocks are re-enabled in
starfive_trng_resume().

Because the PM core transitions the device through RPM_RESUMING,
pm_runtime_get_if_active() returns 0. Returning IRQ_NONE without clearing
the interrupt leaves the level-triggered IRQ asserted, locking up the CPU.

>  
>  	status = readl(trng->base + STARFIVE_ISTAT);
> -	if (status & STARFIVE_ISTAT_RAND_RDY) {

[ ... ]

> @@ -247,7 +325,13 @@ 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);
>  	if (ret)
> -		return ret;
> +		goto end;

[Severity: High]
This isn't a bug introduced by this patch, but will non-blocking reads
invariably fail with -ETIMEDOUT due to a 0-jiffy timeout calculation?

In starfive_trng_cmd(), when wait is false, wait_time is 40. This evaluates
as usecs_to_jiffies(40). On systems with standard HZ=1000 or lower, this
results in 0 jiffies. 

Calling wait_for_completion_timeout() with 0 causes an immediate failure if
the completion is not already signaled, guaranteeing that non-blocking reads
always time out.

>  
>  	memcpy_fromio(buf, trng->base + STARFIVE_RAND0, max);
>  
> -	pm_runtime_put_sync_autosuspend(trng->dev);
> +	ret = max;
> +
> +end:
> +	mutex_unlock(&trng->lock);

[ ... ]

> @@ -282,22 +370,32 @@ static int starfive_trng_probe(struct platform_device *pdev)
>  	trng->irq = platform_get_irq(pdev, 0);
>  	if (trng->irq < 0)
>  		return trng->irq;
>  
>  	init_completion(&trng->random_done);
>  	init_completion(&trng->reseed_done);
> +	mutex_init(&trng->lock);
>  	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);

[Severity: High]
This is a pre-existing issue, but does requesting the IRQ here before
hardware and Runtime PM are initialized risk an immediate IRQ storm?

If the bootloader leaves the TRNG with a pending interrupt, the IRQ can fire
immediately. The newly added PM checks change this from a potential crash
into an interrupt storm, since starfive_trng_irq() will see
pm_runtime_get_if_active() return 0 and bail with IRQ_NONE before
acknowledging the hardware interrupt.

>  	if (ret)
>  		return dev_err_probe(&pdev->dev, ret,
>  				     "Failed to register interrupt handler\n");
>

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