Re: [PATCH 2/3] watchdog: omap: Add support for reading boot status

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

On 9/11/26 11:30 AM, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
> 
> New issues:
> - [Medium] omap_wdt_is_running() fails to detect a running watchdog if the bootloader configured a hardware prescaler, causing a false negative.
> 
> Pre-existing issues:
> - [High] Missing return value check for pm_runtime_get_sync() leads to potential system crash on unclocked MMIO access.
> --
> 
> commit a118768ddc7f3bc1aec5e3a2e71d0f63ab1a2766
> Author: Diogo Ivo <diogo.ivo@bootlin.com>
> Subject: watchdog: omap: Add support for reading boot status
> 
> This commit adds support for determining the boot status of the watchdog
> during driver probe. By checking if the counter is actively incrementing,
> the driver can maintain a running watchdog previously enabled by the
> bootloader.
> 
>> diff --git a/drivers/watchdog/omap_wdt.c b/drivers/watchdog/omap_wdt.c
>> index f8a76df3c7882..d9ab46c70f8c8 100644
>> --- a/drivers/watchdog/omap_wdt.c
>> +++ b/drivers/watchdog/omap_wdt.c
> 
> [ ... ]
> 
>> @@ -225,6 +228,20 @@ static const struct watchdog_ops omap_wdt_ops = {
>>   	.get_timeleft	= omap_wdt_get_timeleft,
>>   };
>>   
>> +static bool omap_wdt_is_running(struct omap_wdt_dev *wdev)
>> +{
>> +	unsigned long period_us = USEC_PER_SEC / RATE_32K;
>> +	void __iomem *base = wdev->base;
>> +	u32 value;
>> +
>> +	value = readl_relaxed(base + OMAP_WATCHDOG_CRR);
>> +
>> +	/* Give the watchdog some time to count if it's on */
>> +	usleep_range(period_us * 10, period_us * 11);
> 
> [Severity: Medium]
> Could this wait duration fail to detect a running watchdog if the bootloader
> configured a hardware prescaler?
> 
> The duration statically assumes an unscaled 32kHz clock. If the bootloader
> enabled the watchdog and configured a non-zero prescaler via the PTV bits in
> OMAP_WATCHDOG_CNTRL, a single clock tick will take much longer.
> 
> For example, with a large prescaler like PTV=7, the 32kHz clock is divided
> by 128, meaning a single clock tick takes around 3.9ms. The hardcoded wait
> of ~330us would complete before the counter increments.
> 
> Would this cause omap_wdt_is_running() to return a false negative, leading
> the probe function to incorrectly conclude the watchdog is stopped and
> subsequently disable it?

This is a valid point that I will address in v2.

>> +
>> +	return readl_relaxed(base + OMAP_WATCHDOG_CRR) != value;
>> +}
>> +
>>   static int omap_wdt_probe(struct platform_device *pdev)
> 
> [ ... ]
> 
>> @@ -267,7 +284,7 @@ static int omap_wdt_probe(struct platform_device *pdev)
>>   			wdev->wdog.bootstatus = WDIOF_CARDRESET;
>>   	}
>>   
>> -	if (early_enable) {
>> +	if (omap_wdt_is_running(wdev) || early_enable) {
> 
> [Severity: High]
> This is a pre-existing issue, but does pm_runtime_get_sync() need a return
> value check before we attempt hardware register accesses in this path?
> 
> If the clock domain cannot be enabled and pm_runtime_get_sync() fails earlier
> in omap_wdt_probe(), proceeding to call the newly added omap_wdt_is_running()
> will result in reading from unclocked OMAP registers.
> 
> Can this unclocked MMIO access trigger an asynchronous external abort and
> crash the system? The same concern applies to omap_wdt_start() and
> omap_wdt_disable() where pm_runtime_get_sync() is also left unchecked.

Again, pre-existing issue that I can take a look at in a separate patch
series.

Diogo

>>   		omap_wdt_start(&wdev->wdog);
>>   		set_bit(WDOG_HW_RUNNING, &wdev->wdog.status);
>>   	} else {
>