Messages in this thread | | | From | Guenter Roeck <> | Subject | Re: [PATCH] watchdog: Respect handle_boot_enabled when setting last last_hw_keepalive | Date | Fri, 30 Jul 2021 15:39:01 -0700 |
| |
On 7/30/21 2:37 PM, Jan Kiszka wrote: > On 30.07.21 22:49, Guenter Roeck wrote: >> On 7/30/21 12:39 PM, Jan Kiszka wrote: >>> From: Jan Kiszka <jan.kiszka@siemens.com> >>> >>> We must not pet a running watchdog when handle_boot_enabled is off >>> because this requests to only start doing that via userspace, not during >>> probing. >>> >> >> The scope of the changed function is quite limited. See the >> definition of watchdog_set_last_hw_keepalive(). On top of that, >> __watchdog_ping() does a bit more than just ping the watchdog, >> and it only pings the watchdog in limited circumstances. On top of that, >> the scope of handle_boot_enabled is different: If enabled, it tells >> the watchdog core to keep pinging a watchdog until userspace opens >> the device. This is about continuous pings, not about an initial one. >> Given that, I'd rather have the watchdog subsystem issue an additional >> ping than risking a regression. >> >> The only driver calling watchdog_set_last_hw_keepalive() is rti_wdt.c. >> Does this patch solve a specific problem observed with that watchdog ? > > Yes, it unbreaks support for handle_boot_enabled=no by not starting the > automatic pinging of the kernel until userspace opens the device. > Without this fix, the core will prematurely start kernel-side pinging, > and hanging userspace will never be detected. > Good point. You are correct.
I think it should also check for watchdog_hw_running(wdd), though. The function should not really be called if the watchdog isn't running, but it should still not ping the watchdog in that case.
Something like
if (watchdog_hw_running(wdd) && handle_boot_enabled) return __watchdog_ping(wdd);
return 0;
Also, I think it would make sense to add your additional comment to the patch description. The problem isn't only that the watchdog is pinged once, the problem is that it starts _automatic_ pinging which it really should not do.
Thanks, Guenter
| |