Re: [PATCH 2/2] watchdog: ath79: convert to watchdog_device

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] The timeout value is no longer properly clamped to the hardware maximum, which can cause an integer overflow when computing timer ticks and lead to premature system resets.
- [High] The `WDIOF_MAGICCLOSE` flag was incorrectly removed from the `watchdog_info` options, silently disabling the magic close safety mechanism.
- [Medium] The `parent` field of the `watchdog_device` structure is not initialized, breaking sysfs topology and device tree alias resolution.
--

commit dcea60e57fc7eacb788b783d8fe1cc986ea83bb1
Author: Rosen Penev <rosenp@gmail.com>

watchdog: ath79: convert to watchdog_device

Converts from the legacy miscdevice/file_operations interface to the
watchdog_device framework, simplifying the driver and properly handling
EPROBE_DEFER.

> diff --git a/drivers/watchdog/ath79_wdt.c b/drivers/watchdog/ath79_wdt.c
> --- a/drivers/watchdog/ath79_wdt.c
> +++ b/drivers/watchdog/ath79_wdt.c

[ ... ]

>  static const struct watchdog_info ath79_wdt_info = {
> -	.options		= WDIOF_SETTIMEOUT | WDIOF_KEEPALIVEPING |
> -				  WDIOF_MAGICCLOSE | WDIOF_CARDRESET,
> -	.firmware_version	= 0,
> -	.identity		= "ATH79 watchdog",
> +	.options = WDIOF_SETTIMEOUT | WDIOF_KEEPALIVEPING | WDIOF_CARDRESET,
> +	.firmware_version = 0,
> +	.identity = "ATH79 watchdog",
>  };

[Severity: High]
Does the removal of WDIOF_MAGICCLOSE silently disable the magic close safety
mechanism?

The legacy driver explicitly supported magic close. Without this flag, the
watchdog core might assume magic close is unsupported and unconditionally stop
the watchdog when the file descriptor is closed.

If a userspace watchdog daemon crashes unexpectedly, the watchdog would be
disabled instead of resetting the system.

[ ... ]

>  static int ath79_wdt_probe(struct platform_device *pdev)
>  {

[ ... ]

> -	max_timeout = (0xfffffffful / wdt_freq);
> -	if (timeout < 1 || timeout > max_timeout) {
> -		timeout = max_timeout;
> -		dev_info(&pdev->dev,
> -			"timeout value must be 0 < timeout < %d, using %d\n",
> -			max_timeout, timeout);
> -	}
> -
> -	ctrl = ath79_wdt_rr(WDOG_REG_CTRL);
> -	boot_status = (ctrl & WDOG_CTRL_LAST_RESET) ? WDIOF_CARDRESET : 0;
> +	wdt->wdd.max_timeout = U32_MAX / wdt->freq;
>  
> -	err = misc_register(&ath79_wdt_miscdev);
> -	if (err) {
> -		dev_err(&pdev->dev,
> -			"unable to register misc device, err=%d\n", err);
> -		return err;
> -	}
> +	ctrl = ath79_wdt_rr(wdt, WDOG_REG_CTRL);
> +	wdt->wdd.bootstatus = (ctrl & WDOG_CTRL_LAST_RESET) ? WDIOF_CARDRESET : 0;
>  
> -	return 0;
> -}
> +	wdt->wdd.info = &ath79_wdt_info;
> +	wdt->wdd.ops = &ath79_wdt_ops;

[Severity: Medium]
Is the parent field of the watchdog_device structure missing initialization?

Without setting wdt->wdd.parent = &pdev->dev, the sysfs class device might be
created at the root of sysfs instead of correctly nesting under the physical
platform device. This could break sysfs topology and device tree alias
resolution.

> +	wdt->wdd.min_timeout = 1;
> +	wdt->wdd.timeout = WDT_TIMEOUT;
> +	watchdog_init_timeout(&wdt->wdd, timeout, &pdev->dev);
> +	watchdog_set_nowayout(&wdt->wdd, nowayout);
> +	watchdog_stop_on_reboot(&wdt->wdd);

[Severity: High]
Can the timeout value overflow when computing timer ticks if max_timeout is
less than WDT_TIMEOUT?

The legacy driver explicitly clamped the timeout variable to max_timeout. In
this patch, if the system's WDT clock frequency is fast enough that
max_timeout is less than 15 seconds, watchdog_init_timeout() will fall back
to WDT_TIMEOUT (15) without clamping it.

Since the watchdog core doesn't validate the initial fallback timeout against
max_timeout, starting the watchdog will execute wdt->freq * 15. This could
overflow a 32-bit integer and program the hardware timer with a small
residual value, causing premature system resets.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260522232553.44748-1-rosenp@gmail.com?part=2