On 11/17/19 2:19 PM, Brandon Maier wrote:
On Sun, Nov 17, 2019 at 3:44 PM Guenter Roeck <linux@xxxxxxxxxxxx> wrote:
On 11/17/19 12:03 PM, Brandon Maier wrote:
When the 'early_enable' module_param is enabled, Linux's watchdogd
thread does not start, causing the watchdog to eventually fire.
What does early_enable have to do with watchdogd ?
The early_enable doesn't directly control the watchdogd, it only
controls starting the watchdog. The problem is that, if the watchdog
is started, we need the watchdogd to ping the watchdog. It watchdogd
is not running, then the watchdog will trip and reset the system.
Why would watchdogd not start if this flag is set ?
watchdog_register_device() checks this flag, and only enables the
watchdogd if it's set. See following:
https://elixir.bootlin.com/linux/latest/source/drivers/watchdog/watchdog_dev.c#L1031
The purpose of early_enable in this driver, as I understand it,
was to force watchdogd to start within the timeout period. So
it does exactly what it is supposed to be doing.
I'm not sure what you're referring to by "timeout period". But
early_enable does force the watchdog to start during module insertion,
that works correctly. The issue is that the driver doesn't tell the
watchdog core to launch the watchdogd.
If you refer to the kernel "watchdogd", that is not of interest here.
_Userspace_ is supposed to start a watchdog daemon if early_enable
is set. If userspace doesn't do that, it is a userspace problem,
not a kernel problem.
The whole point of early_enable is to force _userspace_ to start its watchdog
daemon. Your proposed patch would defeat that by pinging the watchdog in the
kernel, and it would keep doing so forever even if userspace hangs completely.
This would be just wrong.
Guenter
This change is based off many of the other watchdog drivers. They also
set the WDOG_HW_RUNNING flag if the watchdog is running. The only
significant difference is that those drivers don't typically have an
early_enable, they just detect if the watchdog was already running
before insertion. But the end result is the same, the watchdog is
running and will trigger if watchdogd isn't.
Guenter
For the watchdogd to be started, the WDOG_HW_RUNNING flag must be set
before watchdog_register_device().
Signed-off-by: Brandon Maier <brandon.maier@xxxxxxxxxxxxxxxxxxx>
---
drivers/watchdog/omap_wdt.c | 10 ++++++----
1 file changed, 6 insertions(+), 4 deletions(-)
diff --git a/drivers/watchdog/omap_wdt.c b/drivers/watchdog/omap_wdt.c
index 9b91882fe3c4..ecc8592c00a5 100644
--- a/drivers/watchdog/omap_wdt.c
+++ b/drivers/watchdog/omap_wdt.c
@@ -268,8 +268,13 @@ static int omap_wdt_probe(struct platform_device *pdev)
wdev->wdog.bootstatus = WDIOF_CARDRESET;
}
- if (!early_enable)
+ if (!early_enable) {
omap_wdt_disable(wdev);
+ clear_bit(WDOG_HW_RUNNING, &wdev->wdog.status);
+ } else {
+ omap_wdt_start(&wdev->wdog);
+ set_bit(WDOG_HW_RUNNING, &wdev->wdog.status);
+ }
ret = watchdog_register_device(&wdev->wdog);
if (ret) {
@@ -281,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;
Brandon
