Re: [PATCH v3 2/8] watchdog: mediatek: Add wdt/toprgu resets for MT6589

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

Pre-existing issues:
- [High] Device configuration properties are parsed from the device tree after the watchdog device is registered and exposed to userspace.
- [High] The pretimeout interrupt service routine does not clear the hardware interrupt status, leading to an interrupt storm.
--

--- Patch [2]: [PATCH v3 2/8] watchdog: mediatek: Add wdt/toprgu resets for MT6589 ---
commit b68277d0ead460e42db1066fa50165b7b3c50d48
Author: Luca Leonardo Scorcia <[email protected]>

watchdog: mediatek: Add wdt/toprgu resets for MT6589

This commit populates the toprgu_sw_rst_num field for the MT6589 watchdog to
allow toprgu resets in the device tree for compatible devices.

> diff --git a/drivers/watchdog/mtk_wdt.c b/drivers/watchdog/mtk_wdt.c
> index 91d110646e16f..e61f6ae743276 100644
> --- a/drivers/watchdog/mtk_wdt.c
> +++ b/drivers/watchdog/mtk_wdt.c

[ ... ]

drivers/watchdog/mtk_wdt.c:mtk_wdt_probe() {
	...
	err = devm_watchdog_register_device(dev, &mtk_wdt->wdt_dev);
	if (unlikely(err))
		return err;

	dev_info(dev, "Watchdog enabled (timeout=%d sec, nowayout=%d)\n",
		 mtk_wdt->wdt_dev.timeout, nowayout);

	wdt_data = of_device_get_match_data(dev);
	if (wdt_data) {
		err = toprgu_register_reset_controller(pdev,
						       wdt_data->toprgu_sw_rst_num);
		if (err)
			return err;

		mtk_wdt->has_swsysrst_en = wdt_data->has_swsysrst_en;
	}

	mtk_wdt->disable_wdt_extrst =
		of_property_read_bool(dev->of_node, "mediatek,disable-extrst");
	...
}

[Severity: High]
This isn't a bug introduced by this patch, but could this cause the watchdog
hardware to be misconfigured if it's opened immediately after registration?

The watchdog character device is fully registered and exposed to
userspace before the configuration flags (like mediatek,disable-extrst
and mediatek,reset-by-toprgu) are parsed from the device tree.

If a userspace process opens /dev/watchdog and starts it right after
devm_watchdog_register_device() completes, but before mtk_wdt_probe()
finishes parsing these properties, could the system fail to reset properly
when the watchdog triggers?


drivers/watchdog/mtk_wdt.c:mtk_wdt_isr() {
	struct watchdog_device *wdd = arg;

	watchdog_notify_pretimeout(wdd);

	return IRQ_HANDLED;
}

[Severity: High]
This is a pre-existing issue, but does this interrupt handler need to clear
the hardware interrupt status?

It successfully notifies the core but returns IRQ_HANDLED without
interacting with the device registers (like calling mtk_wdt_ping()) to
acknowledge and clear the interrupt source.

Since this is a level-triggered IRQ, could returning IRQ_HANDLED without
explicitly clearing the hardware interrupt status cause the CPU to endlessly
loop servicing the unacknowledged interrupt, leading to an interrupt storm
and system hang?

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