Re: [PATCH 3/5] watchdog: starfive-wdt: Avoid unclocked register access in remove and PM paths

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] watchdog: starfive-wdt: unbalanced runtime PM put in starfive_wdt_shutdown()
--

commit 79e745d33ef2e011dcb21c8530361f6c5a3a1d9a
Author: Guenter Roeck <linux@roeck-us.net>

watchdog: starfive-wdt: Avoid unclocked register access in remove and PM paths

This commit avoids unclocked MMIO register accesses in the remove, shutdown,
suspend, and resume paths when the StarFive watchdog device is runtime
suspended with its clocks gated off. It also ignores non-zero return codes
from pm_runtime_put_sync() in starfive_wdt_pm_stop().

> diff --git a/drivers/watchdog/starfive-wdt.c b/drivers/watchdog/starfive-wdt.c
> index 4724fccbfe637..18301f5d1144c 100644
> --- a/drivers/watchdog/starfive-wdt.c
> +++ b/drivers/watchdog/starfive-wdt.c

[ ... ]

> @@ -540,18 +545,21 @@ static void starfive_wdt_shutdown(struct platform_device *pdev)
>  {
>  	struct starfive_wdt *wdt = platform_get_drvdata(pdev);
>  
> -	starfive_wdt_pm_stop(&wdt->wdd);
> +	if (!pm_runtime_suspended(&pdev->dev))
> +		starfive_wdt_pm_stop(&wdt->wdd);
>  }

[Severity: Medium]
Can calling starfive_wdt_pm_stop() here cause an unbalanced
pm_runtime_put_sync() or an unclocked register access?

In starfive_wdt_probe(), the driver registers a reboot notifier via
watchdog_stop_on_reboot(&wdt->wdd):

drivers/watchdog/starfive-wdt.c:starfive_wdt_probe()
	watchdog_set_nowayout(&wdt->wdd, nowayout);
	watchdog_stop_on_reboot(&wdt->wdd);
	watchdog_stop_on_unregister(&wdt->wdd);

During system reboot, halt, or power-off, kernel_restart_prepare() and
kernel_shutdown_prepare() run reboot_notifier_list before device_shutdown().
When the watchdog is running, watchdog_reboot_notifier() in
drivers/watchdog/watchdog_core.c already calls wdd->ops->stop(wdd), which is
starfive_wdt_pm_stop():

drivers/watchdog/starfive-wdt.c:starfive_wdt_pm_stop()
	starfive_wdt_stop(wdt);
	if (pm_runtime_enabled(wdd->parent))
		pm_runtime_put_sync(wdd->parent);

That call stops the hardware and drops the runtime PM reference acquired by
starfive_wdt_pm_start().

When device_shutdown() subsequently calls starfive_wdt_shutdown(), if the
device is not in RPM_SUSPENDED state (for example, if runtime suspend was
forbidden via sysfs power/control or blocked by a PM QoS constraint, either
when the watchdog was never started or after watchdog_reboot_notifier()
already stopped it), !pm_runtime_suspended(&pdev->dev) is true and
starfive_wdt_pm_stop() is called without a matching runtime PM reference held
by the driver.

Also, because pm_runtime_suspended(dev) checks both
dev->power.runtime_status == RPM_SUSPENDED and !dev->power.disable_depth, if
runtime PM is disabled while the device is suspended with core_clk and
apb_clk gated off (such as after pm_runtime_force_suspend() during
hibernation shutdown), !pm_runtime_suspended(&pdev->dev) evaluates to true
and starfive_wdt_stop() will access unclocked registers.

Since watchdog_stop_on_reboot() is already set in starfive_wdt_probe(),
should starfive_wdt_shutdown() check watchdog_hw_running(&wdt->wdd) (or be
removed so it also respects the watchdog.stop_on_reboot=0 module parameter)?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261104164445.3209560-1-linux@roeck-us.net?part=3