| Message ID | fe8cf65f-f949-9326-8f32-fda7134c8da6@siemens.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | watchdog: Respect handle_boot_enabled when setting last last_hw_keepalive | expand |
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 ? Guenter > Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com> > --- > drivers/watchdog/watchdog_dev.c | 5 ++++- > 1 file changed, 4 insertions(+), 1 deletion(-) > > diff --git a/drivers/watchdog/watchdog_dev.c b/drivers/watchdog/watchdog_dev.c > index 3bab32485273..3c93d00bb284 100644 > --- a/drivers/watchdog/watchdog_dev.c > +++ b/drivers/watchdog/watchdog_dev.c > @@ -1172,7 +1172,10 @@ int watchdog_set_last_hw_keepalive(struct watchdog_device *wdd, > > wd_data->last_hw_keepalive = ktime_sub(now, ms_to_ktime(last_ping_ms)); > > - return __watchdog_ping(wdd); > + if (handle_boot_enabled) > + return __watchdog_ping(wdd); > + > + return 0; > } > EXPORT_SYMBOL_GPL(watchdog_set_last_hw_keepalive); > >
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. Jan
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
diff --git a/drivers/watchdog/watchdog_dev.c b/drivers/watchdog/watchdog_dev.c index 3bab32485273..3c93d00bb284 100644 --- a/drivers/watchdog/watchdog_dev.c +++ b/drivers/watchdog/watchdog_dev.c @@ -1172,7 +1172,10 @@ int watchdog_set_last_hw_keepalive(struct watchdog_device *wdd, wd_data->last_hw_keepalive = ktime_sub(now, ms_to_ktime(last_ping_ms)); - return __watchdog_ping(wdd); + if (handle_boot_enabled) + return __watchdog_ping(wdd); + + return 0; } EXPORT_SYMBOL_GPL(watchdog_set_last_hw_keepalive);