Re: [PATCH v4 2/7] 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] Watchdog device is registered before its driver data is fully initialized, allowing userspace to trigger `mtk_wdt_start()` with uninitialized flags.
- [High] Reset controller is registered before `has_swsysrst_en` is initialized, causing a race condition where reset operations use incorrect configurations.
--

--- Patch [2]: [PATCH v4 2/7] watchdog: mediatek: Add wdt/toprgu resets for mt6589 ---
commit 62a8c76fbad14150f6f3dabaecc0a569e1dfdcb5
Author: Luca Leonardo Scorcia <[email protected]>

watchdog: mediatek: Add wdt/toprgu resets for mt6589

According to Android sources, mt6589 has 12 reset bits in the
WDT_SWSYSRST register. Populate toprgu_sw_rst_num to allow toprgu resets
in device trees of the many compatible devices.

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

[ ... ]

> @@ -493,7 +498,7 @@ static int mtk_wdt_resume(struct device *dev)
>  
>  static const struct of_device_id mtk_wdt_dt_ids[] = {
>  	{ .compatible = "mediatek,mt2712-wdt", .data = &mt2712_data },
> -	{ .compatible = "mediatek,mt6589-wdt" },
> +	{ .compatible = "mediatek,mt6589-wdt", .data = &mt6589_data },

[Severity: High]
This isn't a bug introduced by this patch, but I noticed a potential race
condition during probe that might affect all watchdog devices here.

In mtk_wdt_probe(), the watchdog device is registered before its configuration
flags are read:

        err = devm_watchdog_register_device(dev, &mtk_wdt->wdt_dev);
        if (unlikely(err))
                return err;
        
        ...

        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 (like systemd) opens the device immediately after registration,
could mtk_wdt_start() run before these flags are initialized?

Since these flags are written to the WDT_MODE register and not re-applied
later, could this permanently misconfigure the watchdog for the system uptime
and cause a hang instead of a reboot upon timeout?


[Severity: High]
This is also a pre-existing issue, but while reviewing the probe path that
uses the newly assigned device data, I noticed a potential race condition
with the reset controller.

In mtk_wdt_probe(), the reset controller is registered before the
has_swsysrst_en flag is initialized:

        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;
        }

If a consumer device requests and asserts a reset line immediately upon
registration (e.g. from a concurrent deferred probe), could
toprgu_reset_update() observe has_swsysrst_en as false before it is
assigned?

If so, would it skip calling toprgu_reset_sw_en_unlocked() and fail to
enable software control for the reset bit in WDT_SWSYSRST_EN, leading to a
probe failure for the consumer device?

>  	{ .compatible = "mediatek,mt6735-wdt", .data = &mt6735_data },
>  	{ .compatible = "mediatek,mt6795-wdt", .data = &mt6795_data },
>  	{ .compatible = "mediatek,mt7986-wdt", .data = &mt7986_data },

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