Re: [PATCH v2 6/6] watchdog: starfive: release early_enable PM refcount on remove

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] `starfive_wdt_remove` unconditionally accesses hardware registers before checking PM state, causing a crash on unbind if the device is PM-suspended.
- [High] System resume fails to restart the hardware watchdog if it was running purely due to `early_enable`.
- [Medium] Double decrement of runtime PM usage counter in `starfive_wdt_probe` error path.
--

commit 2328d244e274598a728c0306f052404720065d11
Author: William Theesfeld <william@theesfeld.net>

watchdog: starfive: release early_enable PM refcount on remove

This commit drops the runtime PM reference taken at probe time if the
watchdog was started by early_enable but never opened by userspace. It
ensures the usage counter does not leak when the driver is removed.

> diff --git a/drivers/watchdog/starfive-wdt.c b/drivers/watchdog/starfive-wdt.c
> index 848935dc6dca6..d9175b045d60f 100644
> --- a/drivers/watchdog/starfive-wdt.c
> +++ b/drivers/watchdog/starfive-wdt.c
> @@ -538,6 +538,19 @@ static void starfive_wdt_remove(struct platform_device *pdev)
>  {
>  	struct starfive_wdt *wdt = platform_get_drvdata(pdev);
>  
>  	starfive_wdt_stop(wdt);
>  	watchdog_unregister_device(&wdt->wdd);

[Severity: High]
This isn't a bug introduced by this patch, but does starfive_wdt_stop()
access hardware registers before checking the PM state?

If the device is runtime suspended (for example, early_enable is false and
the watchdog is not open), its clocks would be gated. Calling
starfive_wdt_stop() without waking up the device via pm_runtime_get_sync()
could access memory-mapped registers with the clock gated.

Could this trigger a synchronous external abort or bus fault during driver
unbind?

[ ... ]

While reviewing the early_enable and PM logic in this driver, I noticed a few
other pre-existing issues:

[Severity: High]
This is a pre-existing issue, but looking at the suspend and resume
behavior with early_enable:

drivers/watchdog/starfive-wdt.c:starfive_wdt_resume() {
    ...
    if (watchdog_active(&wdt->wdd))
        return starfive_wdt_start(wdt);

    return 0;
}

During suspend, starfive_wdt_suspend() unconditionally stops the hardware if
it's active. On resume, the code above only restarts the hardware if
WDOG_ACTIVE is set. If the watchdog was running solely due to early_enable,
WDOG_ACTIVE is false, so it is left stopped, but WDOG_HW_RUNNING remains
set in the watchdog core.

If userspace later opens the watchdog, would the core only ping it via
starfive_wdt_keepalive() instead of explicitly starting it? It seems this
might leave the hardware watchdog disabled indefinitely.

[Severity: Medium]
This is also a pre-existing issue, but looking at the error path in
starfive_wdt_probe():

drivers/watchdog/starfive-wdt.c:starfive_wdt_probe() {
        ...
            ret = pm_runtime_put_sync(&pdev->dev);
            if (ret < 0)
                goto err_unregister_wdt;
        ...
err_unregister_wdt:
    watchdog_unregister_device(&wdt->wdd);
err_put_pm:
    if (pm_runtime_enabled(&pdev->dev))
        pm_runtime_put_sync(&pdev->dev);
}

If the initial pm_runtime_put_sync() returns an error (like -EAGAIN), the
code jumps to err_unregister_wdt and falls through to err_put_pm, where it
calls pm_runtime_put_sync() a second time.

Because __pm_runtime_idle() decrements the usage counter regardless of its
return value, could this cause a usage counter underflow warning in the PM
core?

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