Re: [PATCHv2 1/2] watchdog: ath79_wdt: select OF and COMPILE_TEST

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] The global pointer `wdt_base` is assigned during probe but is never cleared on error paths or during driver removal, breaking deferred probing and driver rebinding.
- [High] The driver fails to prevent concurrent access from open miscdevice file descriptors after the device is unbound, leading to a use-after-free or page fault.
- [High] The watchdog hardware is not disabled during driver removal before its clock is disabled, potentially leading to uncontrolled system resets or hardware locks.
--

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

watchdog: ath79_wdt: select OF and COMPILE_TEST

This commit makes the driver depend on OF in Kconfig, as it is inherently
an OF driver. It also adds COMPILE_TEST support for all platforms instead
of just ARM.

> diff --git a/drivers/watchdog/ath79_wdt.c b/drivers/watchdog/ath79_wdt.c
> index 7df703e9852a6..409a40b149015 100644
> --- a/drivers/watchdog/ath79_wdt.c
> +++ b/drivers/watchdog/ath79_wdt.c

[ ... ]

> @@ -295,13 +295,11 @@ static void ath79_wdt_shutdown(struct platform_device *pdev)
>  	ath79_wdt_disable();
>  }
>  
> -#ifdef CONFIG_OF
>  static const struct of_device_id ath79_wdt_match[] = {
>  	{ .compatible = "qca,ar7130-wdt" },
>  	{},
>  };
>  MODULE_DEVICE_TABLE(of, ath79_wdt_match);
> -#endif

[Severity: High]
This is a pre-existing issue, but does the driver properly clear the global
wdt_base pointer on error paths and during removal?

The wdt_base pointer is assigned during probe, but if a subsequent
step like devm_clk_get_enabled() or misc_register() fails, wdt_base is
never cleared.

When the driver core later retries the probe, the initial check evaluates
to true, causing the probe to indefinitely return -EBUSY:

drivers/watchdog/ath79_wdt.c:ath79_wdt_probe() {
    ...
	if (wdt_base)
		return -EBUSY;

	wdt_base = devm_platform_ioremap_resource(pdev, 0);
	if (IS_ERR(wdt_base))
		return PTR_ERR(wdt_base);
    ...
}

Similarly, ath79_wdt_remove() does not clear wdt_base when the device is
unbound. Because devres automatically unmaps the memory, wdt_base is left
as a dangling pointer, preventing future unbind/rebind operations.

Should wdt_base be cleared on error paths and in the remove function?

[Severity: High]
This is a pre-existing issue, but does ath79_wdt_remove() prevent concurrent
access from open miscdevice file descriptors after the device is unbound?

The driver uses devm_platform_ioremap_resource(), which automatically unmaps
wdt_base when the device is removed. However, misc_deregister() does not
revoke existing open file descriptors.

If a user space application keeps /dev/watchdog open and calls write,
ioctl, or close after the driver is unbound, functions like
ath79_wdt_release() or ath79_wdt_write() will dereference the unmapped
wdt_base pointer.

[Severity: High]
This is a pre-existing issue, but is the watchdog hardware safely disabled
during removal?

The watchdog hardware is not explicitly disabled before its clock is
disabled. Because wdt_clk is acquired using devm_clk_get_enabled(), the
device manager will automatically disable the clock immediately after the
remove function completes.

Disabling the clock while the watchdog timer is still running leaves the
hardware in an active but unclocked state.

drivers/watchdog/ath79_wdt.c:ath79_wdt_remove() {
	misc_deregister(&ath79_wdt_miscdev);
}

Should the timer be stopped explicitly before the clock is disabled?

> @@ -309,7 +307,7 @@ static struct platform_driver ath79_wdt_driver = {
>  	.shutdown	= ath79_wdt_shutdown,
>  	.driver		= {
>  		.name	= DRIVER_NAME,
> -		.of_match_table = of_match_ptr(ath79_wdt_match),
> +		.of_match_table = ath79_wdt_match,
>  	},
>  };

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