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

[Diogo Ivo <diogo.ivo@bootlin.com>]

On 9/11/26 11:29 AM, sashiko-bot@kernel.org wrote:
> 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.
> --

For this patch all of Sashiko's comments are for pre-existing issues. I
can take a look at them in a separate series but IMO this lies outside
of the scope of this patch and does not influence its review.

Diogo

> 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?
>