Re: [PATCH] watchdog: orion_wdt: stop the watchdog during system suspend

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] watchdog: orion_wdt: uninitialized pretimeout timer on Armada 370 resume
- [Low] pm: dev_pm_ops structure wrapped with pm_sleep_ptr() instead of pm_ptr()
--

commit 56ff9bfe90545f6af378431628185580afe76771
Author: Igor Velkov <iav@iav.lv>

watchdog: orion_wdt: stop the watchdog during system suspend

The watchdog timer keeps counting across system suspend while the
userspace that pings it is frozen, so it expires and resets the system.
Stop the timer on suspend and restart it on resume when it is active or
running in hardware.

> diff --git a/drivers/watchdog/orion_wdt.c b/drivers/watchdog/orion_wdt.c
> index 5d56e93f04064..0e3d896fe930d 100644
> --- a/drivers/watchdog/orion_wdt.c
> +++ b/drivers/watchdog/orion_wdt.c

[ ... ]

> @@ -668,6 +668,29 @@ static void orion_wdt_shutdown(struct platform_device *pdev)
>  	orion_wdt_stop(wdt_dev);
>  }
>  
> +static int orion_wdt_suspend(struct device *dev)
> +{
> +	struct watchdog_device *wdt_dev = dev_get_drvdata(dev);
> +
> +	if (watchdog_active(wdt_dev) || watchdog_hw_running(wdt_dev))
> +		return orion_wdt_stop(wdt_dev);
> +
> +	return 0;
> +}
> +
> +static int orion_wdt_resume(struct device *dev)
> +{
> +	struct watchdog_device *wdt_dev = dev_get_drvdata(dev);
> +
> +	if (watchdog_active(wdt_dev) || watchdog_hw_running(wdt_dev))
> +		return orion_wdt_start(wdt_dev);

[Severity: High]
Can this cause the pretimeout timer to fire prematurely or immediately upon
system resume?

When orion_wdt_resume() calls orion_wdt_start(), it eventually invokes
armada370_start() on Armada 370/XP:

drivers/watchdog/orion_wdt.c:armada370_start() {
    ...
	/* Enable watchdog timer */
	reg = dev->data->wdt_enable_bit;
	if (dev->wdt.info->options & WDIOF_PRETIMEOUT)
		reg |= TIMER1_ENABLE_BIT;
	atomic_io_modify(dev->reg + TIMER_CTRL, reg, reg);
    ...
}

The pretimeout timer (TIMER1) is enabled here without restoring or reloading its
duration (TIMER1_VAL_OFF). If context was lost or the timer resumes from its
residual value before suspend, could this trigger a kernel panic depending on
the pretimeout governor?

> +
> +	return 0;
> +}
> +
> +static DEFINE_SIMPLE_DEV_PM_OPS(orion_wdt_pm_ops, orion_wdt_suspend,
> +				orion_wdt_resume);
> +
>  static struct platform_driver orion_wdt_driver = {
>  	.probe		= orion_wdt_probe,
>  	.remove		= orion_wdt_remove,
> @@ -675,6 +698,7 @@ static struct platform_driver orion_wdt_driver = {
>  	.driver		= {
>  		.name	= "orion_wdt",
>  		.of_match_table = orion_wdt_of_match_table,
> +		.pm	= pm_sleep_ptr(&orion_wdt_pm_ops),

[Severity: Low]
Does this violate the PM subsystem guidelines by using pm_sleep_ptr() instead
of pm_ptr() for the dev_pm_ops structure pointer?

>  	},
>  };

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261012-orion-wdt-suspend-v1-1-1676cb79e29e@iav.lv?part=1