Re: [PATCH 1/7] watchdog: mtk_wdt: add explicit compatibles for SoCs using mt6589 fallback

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

Pre-existing issues:
- [High] Watchdog is registered to userspace before its state is fully initialized from the device tree.
- [High] Watchdog hardware is not stopped during system suspend if it was running before boot but not opened by userspace.
--

--- Patch [1]: [PATCH 1/7] watchdog: mtk_wdt: add explicit compatibles for SoCs using mt6589 fallback ---
commit d59566877973f13ae59e846770f459a7512fd9eb
Author: Akari Tsuyukusa <[email protected]>

watchdog: mtk_wdt: add explicit compatibles for SoCs using mt6589 fallback

This patch adds dedicated of_device_id entries for several SoCs that
currently rely on the "mediatek,mt6589-wdt" fallback compatible. This
allows the driver to match directly on the SoC-specific string, which is
a prerequisite for removing the fallback string from DTS files.

> diff --git a/drivers/watchdog/mtk_wdt.c b/drivers/watchdog/mtk_wdt.c
> index 91d110646e16f..345d2e89f414e 100644
> --- a/drivers/watchdog/mtk_wdt.c
> +++ b/drivers/watchdog/mtk_wdt.c
> @@ -492,17 +492,28 @@ static int mtk_wdt_resume(struct device *dev)
>  }

[Severity: High]
This isn't a bug introduced by this patch, but I noticed a potential
problem with how the watchdog is handled during system suspend and resume.

In mtk_wdt_suspend() and mtk_wdt_resume(), the driver only checks
watchdog_active(), which is true if userspace has opened the device:

drivers/watchdog/mtk_wdt.c:mtk_wdt_suspend() {
    ...
	if (watchdog_active(&mtk_wdt->wdt_dev))
		mtk_wdt_stop(&mtk_wdt->wdt_dev);
    ...
}

Will this cause the system to reboot unexpectedly during suspend if the
hardware watchdog was already running before boot (WDOG_HW_RUNNING) but
hasn't been opened by userspace?

Should these functions also ensure the hardware is properly stopped even
when the device hasn't been opened?

[Severity: High]
This is also a pre-existing issue, but does mtk_wdt_probe() expose the
device to userspace too early?

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

	// ... [code omitted] ...

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

	mtk_wdt->reset_by_toprgu =
		of_property_read_bool(dev->of_node, "mediatek,reset-by-toprgu");
    ...
}

If userspace opens and starts the watchdog immediately after
devm_watchdog_register_device() completes, could it be started with
incorrect configurations before the device tree properties are parsed?

Additionally, if toprgu_register_reset_controller() fails after the watchdog
is registered, the probe will fail. Could this cause the watchdog to be
deregistered while it is actively in use by userspace?

>  
>  static const struct of_device_id mtk_wdt_dt_ids[] = {
> +	{ .compatible = "mediatek,mt2701-wdt" },
>  	{ .compatible = "mediatek,mt2712-wdt", .data = &mt2712_data },

[ ... ]

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