Re: [PATCH v2 6/8] watchdog: pm8916_wdt: Convert to DEFINE_SIMPLE_DEV_PM_OPS()

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] The watchdog will trigger a system hard-reset during suspend if it was started by the bootloader but has not been opened by userspace.
--

commit b8ef3a1624c2c7748578b550a5051929133ba830
Author: Triet Hoang <triet.hoang.dev@gmail.com>

watchdog: pm8916_wdt: Convert to DEFINE_SIMPLE_DEV_PM_OPS()

Convert deprecated SIMPLE_DEV_PM_OPS() to DEFINE_SIMPLE_DEV_PM_OPS()
and pm_ptr().

> diff --git a/drivers/watchdog/pm8916_wdt.c b/drivers/watchdog/pm8916_wdt.c
> index 007ed139ab965..b97c132de3216 100644
> --- a/drivers/watchdog/pm8916_wdt.c
> +++ b/drivers/watchdog/pm8916_wdt.c
[ ... ]
> -static int __maybe_unused pm8916_wdt_suspend(struct device *dev)
> +static int pm8916_wdt_suspend(struct device *dev)
>  {
>  	struct pm8916_wdt *wdt = dev_get_drvdata(dev);

[Severity: High]
This isn't a bug introduced by this patch, but I noticed a potential problem
in how pm8916_wdt_suspend() handles the hardware watchdog.

Could this trigger a system hard-reset during sleep?

Looking at the rest of pm8916_wdt_suspend(), it only stops the watchdog
if it is active:

	if (watchdog_active(&wdt->wdev))
		return pm8916_wdt_stop(&wdt->wdev);

If the bootloader enables the hardware watchdog, the driver's probe function
detects this and sets WDOG_HW_RUNNING. However, if the system suspends before
userspace opens the watchdog device, WDOG_ACTIVE will not be set.

Since watchdog_dev_suspend() pauses its background ping worker thread during
suspend, and pm8916_wdt_suspend() only checks watchdog_active(), the
hardware watchdog is never stopped.

Would it make sense to check both watchdog_active() and watchdog_hw_running()
to ensure we stop the hardware timer before sleep?

[ ... ]
> -static int __maybe_unused pm8916_wdt_resume(struct device *dev)
> +static int pm8916_wdt_resume(struct device *dev)
>  {
>  	struct pm8916_wdt *wdt = dev_get_drvdata(dev);

[Severity: High]
This is also a pre-existing issue, but the resume path also fails to restart
the watchdog if it was previously kept running by the kernel without an
active userspace owner:

	if (watchdog_active(&wdt->wdev))
		return pm8916_wdt_start(&wdt->wdev);

Would we need to check watchdog_hw_running() here as well?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260914085451.891675-1-triet.hoang.dev@gmail.com?part=6