Re: [PATCH 1/3] watchdog: omap: Remove duplicate start() with early_init

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Asynchronous pm_runtime_put() is used immediately before pm_runtime_disable() in the probe error path.
- [High] PM runtime usage count and active hardware leaked on watchdog registration failure.
- [High] Incorrect teardown order in omap_wdt_remove() exposes the device to userspace accesses after runtime PM is disabled.
--

commit 569cfe0fa3feafe4258fc44d697167c767425987
Author: Diogo Ivo <diogo.ivo@bootlin.com>

watchdog: omap: Remove duplicate start() with early_init
    
Commit cd004d8299f1 ("watchdog: Fix OMAP watchdog early handling")
reworked the handling of the early_enable parameter and added an
explicit call to omap_wdt_start() in case that parameter is true,
but left the call that was previously there, which became duplicate.
    
Fix this by removing the unnecessary duplicate call.

> diff --git a/drivers/watchdog/omap_wdt.c b/drivers/watchdog/omap_wdt.c
> index 95c7e44b7baac..f8a76df3c7882 100644
> --- a/drivers/watchdog/omap_wdt.c
> +++ b/drivers/watchdog/omap_wdt.c
> @@ -285,9 +285,6 @@ static int omap_wdt_probe(struct platform_device *pdev)
>  		readl_relaxed(wdev->base + OMAP_WATCHDOG_REV) & 0xFF,
>  		wdev->wdog.timeout);
>  
> -	if (early_enable)
> -		omap_wdt_start(&wdev->wdog);
> -
>  	pm_runtime_put(wdev->dev);
>  
>  	return 0;

[Severity: High]
This isn't a bug introduced by this patch, but does the error path for
watchdog_register_device() in omap_wdt_probe() initiate an asynchronous PM put
just before disabling PM runtime entirely?

drivers/watchdog/omap_wdt.c:omap_wdt_probe() {
	...
	ret = watchdog_register_device(&wdev->wdog);
	if (ret) {
		pm_runtime_put(wdev->dev);
		pm_runtime_disable(wdev->dev);
		return ret;
	}
	...
}

The asynchronous idle/suspend work is cancelled by pm_runtime_disable(), which
leaves the hardware in an incorrect power state.

Should this use pm_runtime_put_sync() instead to avoid this suspend
cancellation race condition?

[Severity: High]
This is a pre-existing issue, but does this same error path leak a PM runtime
usage count and leave active hardware running on watchdog registration failure?

The probe function starts the watchdog via omap_wdt_start() (which takes a PM
reference), but the error path only calls pm_runtime_put() once.

Does this fail to balance the second PM reference and leave the hardware timer
running without a registered device, which will inevitably lead to a system
reboot?

[Severity: High]
This is also a pre-existing issue, but does the teardown order in
omap_wdt_remove() expose the device to userspace accesses after runtime PM is
disabled?

drivers/watchdog/omap_wdt.c:omap_wdt_remove() {
	pm_runtime_disable(wdev->dev);
	watchdog_unregister_device(&wdev->wdog);
}

Concurrently, a userspace process with the watchdog open could trigger
omap_wdt_ping() or omap_wdt_start(). Because runtime PM is disabled,
pm_runtime_get_sync() fails to power on the hardware, but omap_wdt_start()
ignores the failure and proceeds to write to unclocked hardware registers
(OMAP_WATCHDOG_SPR).

Could this cause a kernel panic due to an imprecise external abort or bus fault?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260911-omap4-wdt_boot-v1-0-637d778a4a1e@bootlin.com?part=1