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

[sashiko-bot@kernel.org]

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?

> +
> +	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.

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

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