Re: [PATCH v7 5/5] watchdog: aaeon: Add watchdog driver for SRG-IMX8P MCU

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] The hardware watchdog is not stopped during system suspend if it was started by the bootloader but not yet opened by userspace, leading to a system reset.
- [High] The watchdog core's ping worker is not stopped on suspend, leading to I2C transactions on a suspended MFD parent.
- [Medium] The driver's `min_timeout` is incorrectly set to 1, violating the API contract for the hardware's fixed 25-second timeout.
--

--- Patch [5]: [PATCH v7 5/5] watchdog: aaeon: Add watchdog driver for SRG-IMX8P MCU ---
commit 7a0cc8bfbb00c11d5c1b8e4bd30df8f3d386356e
Author: Thomas Perrot (Schneider Electric) <[email protected]>

watchdog: aaeon: Add watchdog driver for SRG-IMX8P MCU

Add watchdog driver for the Aaeon SRG-IMX8P embedded controller.
This driver provides system monitoring and recovery capabilities
through the MCU's watchdog timer.

> diff --git a/drivers/watchdog/aaeon_mcu_wdt.c b/drivers/watchdog/aaeon_mcu_wdt.c
> new file mode 100644
> index 0000000000000..9ff559a69fd7b
> --- /dev/null
> +++ b/drivers/watchdog/aaeon_mcu_wdt.c
[ ... ]
> +static int aaeon_mcu_wdt_probe(struct platform_device *pdev)
> +{
[ ... ]
> +	wdt->timeout = AAEON_MCU_WDT_TIMEOUT;
> +	wdt->min_timeout = AAEON_MCU_WDT_MIN_TIMEOUT;

[Severity: Medium]
Is it correct to set min_timeout to 1 here?

Since the hardware has a fixed 25-second timeout, the watchdog core's ping
worker can only extend timeouts, not shorten them. If a user requests a
timeout shorter than 25 seconds, the core will accept it, but the system
will still take the full 25 seconds to reboot.

Should min_timeout be set to 25 to reject impossible configurations?

> +	wdt->max_hw_heartbeat_ms = AAEON_MCU_WDT_HEARTBEAT_MS;
> +	watchdog_init_timeout(wdt, timeout, dev->parent);
> +
> +	watchdog_set_drvdata(wdt, data);

[Severity: High]
Does this driver need to call watchdog_stop_ping_on_suspend() here?

Because max_hw_heartbeat_ms is set, the watchdog core spawns a kthread
worker to periodically ping the hardware. Without registering the PM
notifier to stop the ping worker on suspend, the unfrozen kthread could
call aaeon_mcu_wdt_ping() while the parent I2C/MFD device is already
suspended.

Could this lead to I2C bus lockups or transfer failures during sleep?

> +	watchdog_stop_on_reboot(wdt);
> +	dev_set_drvdata(dev, wdt);
> +
> +	ret = aaeon_mcu_wdt_status(wdt, &enabled);
> +	if (ret)
> +		return ret;
> +
> +	if (enabled)
> +		set_bit(WDOG_HW_RUNNING, &wdt->status);
> +
> +	return devm_watchdog_register_device(dev, wdt);
> +}
> +
> +static int aaeon_mcu_wdt_suspend(struct device *dev)
> +{
> +	struct watchdog_device *wdt = dev_get_drvdata(dev);
> +
> +	if (watchdog_active(wdt))
> +		return aaeon_mcu_wdt_stop(wdt);

[Severity: High]
Will this properly handle the case where the bootloader started the watchdog
but userspace hasn't opened it yet?

In that scenario, watchdog_active() is false but WDOG_HW_RUNNING is set.
This suspend routine would return early, leaving the 25-second hardware
timer running. This might trigger an unintended system reset while the
system is asleep.

Should this also check watchdog_hw_running()?

> +
> +	return 0;
> +}
> +
> +static int aaeon_mcu_wdt_resume(struct device *dev)
> +{
> +	struct watchdog_device *wdt = dev_get_drvdata(dev);
> +
> +	if (watchdog_active(wdt))
> +		return aaeon_mcu_wdt_start(wdt);

[Severity: High]
Similar to the suspend callback, if the watchdog was solely in the
WDOG_HW_RUNNING state, it won't be restarted on resume.

Would checking watchdog_hw_running() ensure it resumes correctly?

> +
> +	return 0;
> +}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=5