[PATCH 0/3] watchdog: omap: Preserve OMAP watchdog across boot to detect faulty boots

[PATCH 0/3] watchdog: omap: Preserve OMAP watchdog across boot to detect faulty boots

[Diogo Ivo]

Allow the OMAP watchdog to survive being stopped on kernel initialization
so it can detect a faulty boot in cases where the bootloader leaves it
running and the watchdog driver picks it up during kernel init.

- Patch 1 removes a duplicate omap_wdt_start() call left behind by
cd004d8299f1 ("watchdog: Fix OMAP watchdog early handling"). This is
unrelated to the main goal of the series and can be picked up
independently.

- Patch 2 adds support for reading the watchdog boot status. Probe now
checks whether the watchdog is already running and takes it over instead
of blindly stopping it based on early_enable alone. This introduces a
regression possibility, explained in detail in the patch's message.

- Patch 3 marks the OMAP4 watchdog node ti,no-reset-on-init so the
ti-sysc driver stops resetting it. A detailed explaination of why is
also provided in the commit message of the patch.

This series has been tested on a platform based on the VAR-SOM-OM44 from
Variscite, running a TI OMAP4460 SoC.

Signed-off-by: Diogo Ivo <diogo.ivo@bootlin.com>
---
Diogo Ivo (3):
      watchdog: omap: Remove duplicate start() with early_init
      watchdog: omap: Add support for reading boot status
      arm: dts: ti: omap: Prevent watchdog from being reset on kernel boot

 arch/arm/boot/dts/ti/omap/omap4-l4.dtsi |  1 +
 drivers/watchdog/omap_wdt.c             | 22 ++++++++++++++++++----
 2 files changed, 19 insertions(+), 4 deletions(-)
---
base-commit: 83d343e6246c83e694548fec49856819124d32c0
change-id: 20260910-omap4-wdt_boot-472b73d4af6f

Best regards,
--  
Diogo Ivo <diogo.ivo@bootlin.com>

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

[Diogo Ivo]

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.

Signed-off-by: Diogo Ivo <diogo.ivo@bootlin.com>
---
 drivers/watchdog/omap_wdt.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/watchdog/omap_wdt.c b/drivers/watchdog/omap_wdt.c
index 2482a1982c8b..e6d869e36c43 100644
--- a/drivers/watchdog/omap_wdt.c
+++ b/drivers/watchdog/omap_wdt.c
@@ -286,9 +286,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;

-- 
2.55.0

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

[Diogo Ivo]

Add support for determining the boot status of the watchdog.

Signed-off-by: Diogo Ivo <diogo.ivo@bootlin.com>
---
One case worth mentioning explicitly here in terms of regressions:

Consider a machine where:
 - the bootloader turns on the wd
 - early_init = 0
 - omap_wdt is compiled into the kernel
 - CONFIG_WATCHDOG_HANDLE_BOOT_ENABLED=n
 - userspace does not service the watchdog

The behaviour of such a machine up until this patch was that the
the system would not reboot as the watchdog would be unconditionally
stopped. However, after this patch such systems _will_ reboot since the
watchdog will be kept on and nothing will service it. In practice, with
just this patch this will not happen since the ti-sysc.c driver (that
probes prior to the watchdog driver) will anyway stop the watchdog, but
when adding the next patch in this series to stop that behaviour this
regression becomes a real scenario.
---
 drivers/watchdog/omap_wdt.c | 19 ++++++++++++++++++-
 1 file changed, 18 insertions(+), 1 deletion(-)

diff --git a/drivers/watchdog/omap_wdt.c b/drivers/watchdog/omap_wdt.c
index e6d869e36c43..7627d3c626c2 100644
--- a/drivers/watchdog/omap_wdt.c
+++ b/drivers/watchdog/omap_wdt.c
@@ -26,6 +26,7 @@
 
 #define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
 
+#include <linux/delay.h>
 #include <linux/module.h>
 #include <linux/types.h>
 #include <linux/kernel.h>
@@ -42,6 +43,8 @@
 
 #include "omap_wdt.h"
 
+#define RATE_32K 32768
+
 static bool nowayout = WATCHDOG_NOWAYOUT;
 module_param(nowayout, bool, 0);
 MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once started "
@@ -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);
+
+	return readl_relaxed(base + OMAP_WATCHDOG_CRR) != value;
+}
+
 static int omap_wdt_probe(struct platform_device *pdev)
 {
 	struct omap_wd_timer_platform_data *pdata = dev_get_platdata(&pdev->dev);
@@ -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) {
 		omap_wdt_start(&wdev->wdog);
 		set_bit(WDOG_HW_RUNNING, &wdev->wdog.status);
 	} else {

-- 
2.55.0

[PATCH 3/3] arm: dts: ti: omap: Prevent watchdog from being reset on kernel boot

[Diogo Ivo]

Currently the ti-sysc driver resets and idles the watchdog when it probes
its target-module node. In a system where the watchdog is being used to
detect a faulty boot this makes it impossible for the watchdog to do its
function, in an unexpected and time-consuming manner to debug.

Explicitly mark the watchdog node with ti,no-reset-on-init to prevent
the reset portion from happening. This allows the watchdog driver proper
to maintain the watchdog running.

Signed-off-by: Diogo Ivo <diogo.ivo@bootlin.com>
---
The ti,no-idle property is deliberately not added to minimize the
possibility of a regression on systems that were relying on the ti-sysc
driver disabling the watchdog and did not service it. With just
ti,no-reset-on-init we get a 3 second window for the watchdog driver to
probe and maintain the watchdog alive, otherwise the system will behave
almost in the same way as before. The only difference comes from introducing
the small 3 second window that could lead to a watchdog expiration on systems
where the watchdog was being stopped right next to its expiration, but
that risk is much smaller regression-wise than always keeping its boot
status. If this approach is deemed appropriate this patch should be
mimicked in other TI SoC's with this watchdog.
---
 arch/arm/boot/dts/ti/omap/omap4-l4.dtsi | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm/boot/dts/ti/omap/omap4-l4.dtsi b/arch/arm/boot/dts/ti/omap/omap4-l4.dtsi
index c1afc49f456c..eb0e10e51ba4 100644
--- a/arch/arm/boot/dts/ti/omap/omap4-l4.dtsi
+++ b/arch/arm/boot/dts/ti/omap/omap4-l4.dtsi
@@ -1126,6 +1126,7 @@ target-module@4000 {			/* 0x4a314000, ap 7 18.0 */
 					<SYSC_IDLE_SMART>,
 					<SYSC_IDLE_SMART_WKUP>;
 			ti,syss-mask = <1>;
+			ti,no-reset-on-init;
 			/* Domains (V, P, C): wakeup, wkup_pwrdm, l4_wkup_clkdm */
 			clocks = <&l4_wkup_clkctrl OMAP4_WD_TIMER2_CLKCTRL 0>;
 			clock-names = "fck";

-- 
2.55.0

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

[sashiko-bot]

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

Re: [PATCH 3/3] arm: dts: ti: omap: Prevent watchdog from being reset on kernel boot

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] The patch exposes the system to unexpected reboots due to a 30-second window before the watchdog is idled, contrary to the '3 second window' claimed in the commit message.
--

commit 1abe23cd5472149baeeab9af481deaf65caccd1f
Author: Diogo Ivo <diogo.ivo@bootlin.com>
Subject: arm: dts: ti: omap: Prevent watchdog from being reset on kernel boot

This commit prevents the watchdog from being reset and idled during kernel
boot by adding the ti,no-reset-on-init property to the target-module node.
This is intended to provide a 3-second window for the watchdog driver to
probe without resetting the watchdog.

> diff --git a/arch/arm/boot/dts/ti/omap/omap4-l4.dtsi b/arch/arm/boot/dts/ti/omap/omap4-l4.dtsi
> index 4c78a0b28fab3..e061ea3fc8e70 100644
> --- a/arch/arm/boot/dts/ti/omap/omap4-l4.dtsi
> +++ b/arch/arm/boot/dts/ti/omap/omap4-l4.dtsi
> @@ -1126,6 +1126,7 @@ target-module@4000 {			/* 0x4a314000, ap 7 18.0 */
>  					<SYSC_IDLE_SMART>,
>  					<SYSC_IDLE_SMART_WKUP>;
>  			ti,syss-mask = <1>;
> +			ti,no-reset-on-init;

[Severity: High]
Does adding this property unintentionally cause a regression where the system
experiences unexpected reboots due to a 30-second delay instead of the
intended 3-second window?

Adding ti,no-reset-on-init skips the immediate hardware reset and triggers the
delayed work fallback in sysc_probe():

drivers/bus/ti-sysc.c:sysc_probe() {
    ...
    schedule_delayed_work(&ddata->idle_work, 3000);
    ...
}

Because schedule_delayed_work() takes its delay in jiffies rather than
milliseconds, passing 3000 evaluates to 3000 jiffies. On typical ARM
configurations where HZ=100 (the default), this results in a 30-second delay
instead of the expected 3 seconds.

This could lead to a boot loop if the bootloader configures a watchdog timeout
of less than 30 seconds and the system relies on the driver to stop it.

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

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

[sashiko-bot]

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

Re: [PATCH 0/3] watchdog: omap: Preserve OMAP watchdog across boot to detect faulty boots

[Guenter Roeck]

On 9/11/26 02:17, Diogo Ivo wrote:
> Allow the OMAP watchdog to survive being stopped on kernel initialization
> so it can detect a faulty boot in cases where the bootloader leaves it
> running and the watchdog driver picks it up during kernel init.
> 
> - Patch 1 removes a duplicate omap_wdt_start() call left behind by
> cd004d8299f1 ("watchdog: Fix OMAP watchdog early handling"). This is
> unrelated to the main goal of the series and can be picked up
> independently.
> 
> - Patch 2 adds support for reading the watchdog boot status. Probe now
> checks whether the watchdog is already running and takes it over instead
> of blindly stopping it based on early_enable alone. This introduces a
> regression possibility, explained in detail in the patch's message.
> 
> - Patch 3 marks the OMAP4 watchdog node ti,no-reset-on-init so the
> ti-sysc driver stops resetting it. A detailed explaination of why is
> also provided in the commit message of the patch.
> 
> This series has been tested on a platform based on the VAR-SOM-OM44 from
> Variscite, running a TI OMAP4460 SoC.
> 
Please address the issues reported by Sashiko, or explain why they don't apply.

Thanks,
Guenter

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

[Diogo Ivo]

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

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

[Diogo Ivo]

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

Re: [PATCH 3/3] arm: dts: ti: omap: Prevent watchdog from being reset on kernel boot

[Diogo Ivo]

On 9/11/26 11:30 AM, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> - [High] The patch exposes the system to unexpected reboots due to a 30-second window before the watchdog is idled, contrary to the '3 second window' claimed in the commit message.
> --
> 
> commit 1abe23cd5472149baeeab9af481deaf65caccd1f
> Author: Diogo Ivo <diogo.ivo@bootlin.com>
> Subject: arm: dts: ti: omap: Prevent watchdog from being reset on kernel boot
> 
> This commit prevents the watchdog from being reset and idled during kernel
> boot by adding the ti,no-reset-on-init property to the target-module node.
> This is intended to provide a 3-second window for the watchdog driver to
> probe without resetting the watchdog.
> 
>> diff --git a/arch/arm/boot/dts/ti/omap/omap4-l4.dtsi b/arch/arm/boot/dts/ti/omap/omap4-l4.dtsi
>> index 4c78a0b28fab3..e061ea3fc8e70 100644
>> --- a/arch/arm/boot/dts/ti/omap/omap4-l4.dtsi
>> +++ b/arch/arm/boot/dts/ti/omap/omap4-l4.dtsi
>> @@ -1126,6 +1126,7 @@ target-module@4000 {			/* 0x4a314000, ap 7 18.0 */
>>   					<SYSC_IDLE_SMART>,
>>   					<SYSC_IDLE_SMART_WKUP>;
>>   			ti,syss-mask = <1>;
>> +			ti,no-reset-on-init;
> 
> [Severity: High]
> Does adding this property unintentionally cause a regression where the system
> experiences unexpected reboots due to a 30-second delay instead of the
> intended 3-second window?
> 
> Adding ti,no-reset-on-init skips the immediate hardware reset and triggers the
> delayed work fallback in sysc_probe():
> 
> drivers/bus/ti-sysc.c:sysc_probe() {
>      ...
>      schedule_delayed_work(&ddata->idle_work, 3000);
>      ...
> }
> 
> Because schedule_delayed_work() takes its delay in jiffies rather than
> milliseconds, passing 3000 evaluates to 3000 jiffies. On typical ARM
> configurations where HZ=100 (the default), this results in a 30-second delay
> instead of the expected 3 seconds.
> 
> This could lead to a boot loop if the bootloader configures a watchdog timeout
> of less than 30 seconds and the system relies on the driver to stop it.

This is a valid point that I am not sure how to solve. I could change
the hardcoded 3000 but I don't know if that is the way to go. Any help
would be greatly appreciated.

Diogo

Re: [PATCH 0/3] watchdog: omap: Preserve OMAP watchdog across boot to detect faulty boots

[Diogo Ivo]

Hi Guenter,

On 9/11/26 4:29 PM, Guenter Roeck wrote:
> On 9/11/26 02:17, Diogo Ivo wrote:
>> Allow the OMAP watchdog to survive being stopped on kernel initialization
>> so it can detect a faulty boot in cases where the bootloader leaves it
>> running and the watchdog driver picks it up during kernel init.
>>
>> - Patch 1 removes a duplicate omap_wdt_start() call left behind by
>> cd004d8299f1 ("watchdog: Fix OMAP watchdog early handling"). This is
>> unrelated to the main goal of the series and can be picked up
>> independently.
>>
>> - Patch 2 adds support for reading the watchdog boot status. Probe now
>> checks whether the watchdog is already running and takes it over instead
>> of blindly stopping it based on early_enable alone. This introduces a
>> regression possibility, explained in detail in the patch's message.
>>
>> - Patch 3 marks the OMAP4 watchdog node ti,no-reset-on-init so the
>> ti-sysc driver stops resetting it. A detailed explaination of why is
>> also provided in the commit message of the patch.
>>
>> This series has been tested on a platform based on the VAR-SOM-OM44 from
>> Variscite, running a TI OMAP4460 SoC.
>>
> Please address the issues reported by Sashiko, or explain why they don't 
> apply.

I have just replied to the Sashiko reviews but I'm not sure if you got
the replies as Sashiko did not include your e-mail in its review. If you
did not receive them please let me know and I can resend them. In
any case if you could give your opinion on the comments I left on the
patches about regressions that would be great as I think after the
Sashiko points are addressed that is the main blocker for this series.

Thanks,
Diogo

> Thanks,
> Guenter

Re: [PATCH 0/3] watchdog: omap: Preserve OMAP watchdog across boot to detect faulty boots

[Guenter Roeck]

On 9/11/26 08:17, Diogo Ivo wrote:
> Hi Guenter,
> 
> On 9/11/26 4:29 PM, Guenter Roeck wrote:
>> On 9/11/26 02:17, Diogo Ivo wrote:
>>> Allow the OMAP watchdog to survive being stopped on kernel initialization
>>> so it can detect a faulty boot in cases where the bootloader leaves it
>>> running and the watchdog driver picks it up during kernel init.
>>>
>>> - Patch 1 removes a duplicate omap_wdt_start() call left behind by
>>> cd004d8299f1 ("watchdog: Fix OMAP watchdog early handling"). This is
>>> unrelated to the main goal of the series and can be picked up
>>> independently.
>>>
>>> - Patch 2 adds support for reading the watchdog boot status. Probe now
>>> checks whether the watchdog is already running and takes it over instead
>>> of blindly stopping it based on early_enable alone. This introduces a
>>> regression possibility, explained in detail in the patch's message.
>>>
>>> - Patch 3 marks the OMAP4 watchdog node ti,no-reset-on-init so the
>>> ti-sysc driver stops resetting it. A detailed explaination of why is
>>> also provided in the commit message of the patch.
>>>
>>> This series has been tested on a platform based on the VAR-SOM-OM44 from
>>> Variscite, running a TI OMAP4460 SoC.
>>>
>> Please address the issues reported by Sashiko, or explain why they don't apply.
> 
> I have just replied to the Sashiko reviews but I'm not sure if you got
> the replies as Sashiko did not include your e-mail in its review. If you
> did not receive them please let me know and I can resend them. In
> any case if you could give your opinion on the comments I left on the
> patches about regressions that would be great as I think after the
> Sashiko points are addressed that is the main blocker for this series.
> 
I did. I just wonder if the effort is worth the pain / cost.

Is there an actual use case ? Is the problem you are trying to solve
a real problem, or a theoretic one ? For example, the patches impose
a hard boot delay of more than 30 ms in omap_wdt_is_running().
Even though that could be optimized (there is no reason to wait
that long; the value could change a microsecond after the first read),
it is nevertheless a mandatory boot delay.

Another concern is the impact and potential side effects of setting
ti,no-reset-on-init (and the possible boot loop cause by it due to the odd
30-second init delay). After this change, a running watchdog is no longer
stopped. What happens on systems which do not load the watchdog at all
(for example because the driver was not configured) ? Will that also cause
a boot loop on such systems ?

This is just a couple of problems introduced by this series. You better have
a very good reason for it to warrant having to deal with the potential fallout.

Thanks,
Guenter