Re: [PATCH v2 1/6] watchdog: starfive: balance PM refcount when start operation fails

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Hardware registers are accessed in `set_timeout` and `get_timeleft` callbacks without ensuring the device is runtime-resumed.
--

commit 634df4af4e48b2df41f8b9f5b5a99c2ef88e4d38
Author: William Theesfeld <william@theesfeld.net>

watchdog: starfive: balance PM refcount when start operation fails

This commit fixes a PM runtime usage counter leak in starfive_wdt_pm_start()
by calling pm_runtime_put_sync() if starfive_wdt_start() fails. It also
updates the resume call to use pm_runtime_resume_and_get().

> diff --git a/drivers/watchdog/starfive-wdt.c b/drivers/watchdog/starfive-wdt.c
> index af55adc4a3c69..ed8c5711a3853 100644
> --- a/drivers/watchdog/starfive-wdt.c
> +++ b/drivers/watchdog/starfive-wdt.c
> @@ -371,12 +371,16 @@ static void starfive_wdt_stop(struct starfive_wdt *wdt)
>  static int starfive_wdt_pm_start(struct watchdog_device *wdd)
>  {
>  	struct starfive_wdt *wdt = watchdog_get_drvdata(wdd);
> -	int ret = pm_runtime_get_sync(wdd->parent);
> +	int ret = pm_runtime_resume_and_get(wdd->parent);
>  
>  	if (ret < 0)
>  		return ret;
>  
> -	return starfive_wdt_start(wdt);
> +	ret = starfive_wdt_start(wdt);
> +	if (ret)
> +		pm_runtime_put_sync(wdd->parent);
> +
> +	return ret;
>  }

[Severity: High]
This isn't a bug introduced by this patch, but since we are modifying PM
runtime handling in this driver, does the driver safely handle hardware
register accesses when the watchdog is inactive?

The watchdog framework allows ioctls like WDIOC_SETTIMEOUT and
WDIOC_GETTIMELEFT to be called even when the watchdog is stopped. In the
inactive state, the PM runtime usage counter drops to 0, which suspends the
device and gates its APB and core clocks.

In drivers/watchdog/starfive-wdt.c:starfive_wdt_set_timeout():

	starfive_wdt_unlock(wdt);
	starfive_wdt_disable(wdt);
	starfive_wdt_set_reload_count(wdt, wdt->count);
	starfive_wdt_enable(wdt);
	starfive_wdt_lock(wdt);

And in drivers/watchdog/starfive-wdt.c:starfive_wdt_get_timeleft():

	count = starfive_wdt_get_count(wdt);
	if (wdt->variant->double_timeout && !starfive_wdt_raise_irq_status(wdt))
		count += wdt->count;

Since these functions directly access MMIO registers without ensuring the
clocks are active via pm_runtime_resume_and_get(), could these unclocked
bus accesses trigger a bus fault or panic if called by userspace while the
watchdog is inactive?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1780664848.git.william@theesfeld.net?part=1