Re: [PATCH v5 4/8] watchdog: mediatek: Add support for mt8167 toprgu/watchdog

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] The watchdog character device is registered and exposed to userspace before its configuration properties are parsed from the Device Tree, creating a time-of-check to time-of-use race condition. This problem wasn't introduced by this patch, but it remains unfixed.
--

commit 636d28c87d7152b3a57c046cb13f6a93c3f5aaef
Author: Luca Leonardo Scorcia <l.scorcia@gmail.com>

watchdog: mediatek: Add support for mt8167 toprgu/watchdog

Add support for the Top Reset Generation Unit/Watchdog Timer found on mt8167.
Since according to its data sheet mt8167 toprgu reset bits are not contiguous,
add support for a reset table to the driver. Also address a preexisting Sashiko
finding regarding has_swsysrst_en configuration.

> diff --git a/drivers/watchdog/mtk_wdt.c b/drivers/watchdog/mtk_wdt.c
> index d609f0fa35e32..abe58ed6a5045 100644
> --- a/drivers/watchdog/mtk_wdt.c
> +++ b/drivers/watchdog/mtk_wdt.c

[ ... ]

> @@ -457,12 +493,14 @@ static int mtk_wdt_probe(struct platform_device *pdev)
>  
>  	wdt_data = of_device_get_match_data(dev);
>  	if (wdt_data) {
> +		mtk_wdt->toprgu_sw_rst_num = wdt_data->toprgu_sw_rst_num;
> +		mtk_wdt->toprgu_sw_rst_tb = wdt_data->toprgu_sw_rst_tb;
> +		mtk_wdt->has_swsysrst_en = wdt_data->has_swsysrst_en;
> +
>  		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 =

[Severity: High]
This is a pre-existing issue, but it looks like the watchdog device is
exposed to userspace before its configuration properties are fully parsed
from the Device Tree.

In mtk_wdt_probe() in drivers/watchdog/mtk_wdt.c, the device is registered
earlier:

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

This registration immediately fires a KOBJ_ADD uevent. If a process opens
/dev/watchdog during boot while the probe thread is blocked (for example,
if there is mutex contention in toprgu_register_reset_controller()),
watchdog_open() will call mtk_wdt_start().

However, mtk_wdt_start() relies on properties that are parsed after the
registration:

	if (mtk_wdt->disable_wdt_extrst)
		reg &= ~WDT_MODE_EXRST_EN;
	else
		reg |= WDT_MODE_EXRST_EN;

	if (mtk_wdt->reset_by_toprgu)
		reg |= WDT_MODE_CNT_SEL;
	else
		reg &= ~WDT_MODE_CNT_SEL;

Since mtk_wdt->disable_wdt_extrst and mtk_wdt->reset_by_toprgu are still
uninitialized (false) when this race occurs, could mtk_wdt_start() configure
the hardware incorrectly, potentially causing a system hang instead of a
reboot when a watchdog timeout triggers?

Would it be safer to move the property parsing before the call to
devm_watchdog_register_device()?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260908174259.9009-1-l.scorcia@gmail.com?part=4