Re: [PATCH 6/7] watchdog: mtk_wdt: add reset controller support for MT6589

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

Pre-existing issues:
- [High] The watchdog device and reset controller are registered before the driver has finished reading its configuration from the device tree.
--

--- Patch [6]: [PATCH 6/7] watchdog: mtk_wdt: add reset controller support for MT6589 ---
commit f121306c9b4fd3b69e7cdcb03d9c4a147f872914
Author: Akari Tsuyukusa <[email protected]>

watchdog: mtk_wdt: add reset controller support for MT6589
    
This commit adds reset data for the MT6589 fallback, assigning the proper
reset controller information to the mt6589 entry in the device tree match
table.

> diff --git a/drivers/watchdog/mtk_wdt.c b/drivers/watchdog/mtk_wdt.c
> index 345d2e89f414e..c8ce45a597956 100644
> --- a/drivers/watchdog/mtk_wdt.c
> +++ b/drivers/watchdog/mtk_wdt.c

[ ... ]

> @@ -496,7 +501,7 @@ static const struct of_device_id mtk_wdt_dt_ids[] = {
>  	{ .compatible = "mediatek,mt2712-wdt", .data = &mt2712_data },
>  	{ .compatible = "mediatek,mt6572-wdt" },
>  	{ .compatible = "mediatek,mt6582-wdt" },
> -	{ .compatible = "mediatek,mt6589-wdt" },
> +	{ .compatible = "mediatek,mt6589-wdt", .data = &mt6589_data },

[Severity: High]
This is a pre-existing issue, but while reviewing the watchdog data
initialization, I noticed a potential race condition during device probe.

Is it possible for the watchdog device or reset controller to be accessed 
before their configuration flags are fully initialized?

In 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");

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

If a userspace process opens the watchdog character device immediately after 
devm_watchdog_register_device() completes, or a concurrently probing driver 
acquires the reset controller after toprgu_register_reset_controller() 
returns, could they operate with uninitialized state variables 
(disable_wdt_extrst, reset_by_toprgu, and has_swsysrst_en)?

This could potentially misconfigure the hardware, failing to disable external 
resets or missing software reset enables, which might cause system lockups or 
failed peripheral resets.

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

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