Re: [PATCH v2] hwrng: stm32: fix usage_count leak when autosuspend_delay is negative

On 8/11/26 08:34, Guangshuo Li wrote:
> stm32_rng_probe() calls pm_runtime_use_autosuspend(), but runtime PM is
> enabled with pm_runtime_enable() and the matching
> pm_runtime_dont_use_autosuspend() is not called on driver teardown.
> 
> If the autosuspend delay is set to a negative value while autosuspend
> is enabled, the runtime PM core increments usage_count to prevent
> runtime suspend. Without calling pm_runtime_dont_use_autosuspend()
> during teardown, this reference is not dropped and usage_count remains
> unbalanced.
> 
> Use devm_pm_runtime_enable() so that pm_runtime_dont_use_autosuspend()
> and pm_runtime_disable() are automatically called on probe failure and
> driver teardown. With runtime PM cleanup handled by devres,
> stm32_rng_remove() is no longer needed.
> 
> This issue was found by manual code inspection.
> 
> Fixes: c6a97c42e399 ("hwrng: stm32 - add support for STM32 HW RNG")
> Cc: [email protected]
> Signed-off-by: Guangshuo Li <[email protected]>

Hi Guangshuo,

Thanks for your investigation. You've found one problem, but I think 
you've solved three issues with your patch, and the one you describe is 
the most minor of them.

The usage_count imbalance is real, but the driver never sets a negative 
autosuspend_delay_ms itself, and the count is rebalanced on the next 
probe anyway.

For the two others: when stm32_rng_read() arms a 100 ms autosuspend 
timer, if we unbind within this 100 ms window, stm32_rng_remove() 
cancels the pending suspend and leaves the device RPM_ACTIVE: 
stm32_rng_runtime_suspend() never runs, so the RNG is left enabled with 
its clocks prepared and its power domain held forever. Moreover, the 
remove operation runs before devres released the hwrng registration, so 
reads could still land on a device whose runtime PM was already disabled 
and fail with -EACCES.

Your patch fixes both, because devm_pm_runtime_enable() registered 
before devm_hwrng_register() unwinds in the right order and 
pm_runtime_dont_use_autosuspend() forces a synchronous suspend before 
the disable.

So I recommend adapting your commit message around those points, as I 
think the Cc: stable is justified by those two.

Reviewed-by: Maxime Méré <[email protected]>

Cheers,

Maxime