Re: [PATCH v2 4/6] watchdog: starfive: guard system suspend/resume hardware access

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] A watchdog started via early_enable is permanently stopped after system suspend/resume.
- [High] starfive_wdt_remove unconditionally accesses gated hardware, triggering a synchronous external abort.
--

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

watchdog: starfive: guard system suspend/resume hardware access

This commit guards hardware access in starfive_wdt_suspend() and
starfive_wdt_resume() with pm_runtime_status_suspended() checks. It prevents
register reads and writes when the device is runtime-suspended and its clocks
are gated, avoiding synchronous external aborts.

> diff --git a/drivers/watchdog/starfive-wdt.c b/drivers/watchdog/starfive-wdt.c
> index 6539bf0893583..4926984bf1e93 100644
> --- a/drivers/watchdog/starfive-wdt.c
> +++ b/drivers/watchdog/starfive-wdt.c
[ ... ]
> @@ -574,8 +579,16 @@ static int starfive_wdt_resume(struct device *dev)
>  	if (ret)
>  		return ret;
>  
> +	/*
> +	 * pm_runtime_force_resume() leaves the device in whichever runtime
> +	 * PM state it was in before system suspend.  Only restore hardware
> +	 * state when the device is actually resumed; otherwise the register
> +	 * writes below would race with gated clocks.
> +	 */
> +	if (pm_runtime_status_suspended(dev))
> +		return 0;
> +
>  	starfive_wdt_unlock(wdt);
> -	/* Restore watchdog state. */
>  	starfive_wdt_set_reload_count(wdt, wdt->reload);
>  	starfive_wdt_lock(wdt);
>  

[Severity: High]
This isn't a bug introduced by this patch, but does this skip restarting the
hardware for early-enabled watchdogs?

Immediately following this block, the function checks:

    if (watchdog_active(&wdt->wdd))
        return starfive_wdt_start(wdt);

When early_enable is used but userspace hasn't opened the watchdog, the
framework state is WDOG_HW_RUNNING without WDOG_ACTIVE. Since
watchdog_active() only checks WDOG_ACTIVE, it evaluates to false here.

Does this leave the hardware watchdog permanently stopped after a system
resume?

[Severity: High]
This is a pre-existing issue, but I noticed a similar hardware access issue
can happen during driver unbind:

drivers/watchdog/starfive-wdt.c:starfive_wdt_remove() {
    struct starfive_wdt *wdt = platform_get_drvdata(pdev);

    starfive_wdt_stop(wdt);
    ...
}

During driver unbind, the driver core (__device_release_driver() in
drivers/base/dd.c) drops the runtime PM usage counter to 0 immediately before
calling device_remove().

If the watchdog is not active (e.g. early_enable is false and userspace
never opened it), the device is suspended and its clocks are gated. Does
calling starfive_wdt_stop() here unconditionally access the WDOGCONTROL
and WDOGLOCK registers while clocks are gated, triggering a synchronous
external abort?

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