| Message ID | 4d82b8ce-bc34-e4b2-c5fe-9e883b0db59d@siemens.com (mailing list archive) |
|---|---|
| State | Rejected |
| Headers | show |
| Series | [RFC] watchdog: rti-wdt: Provide set_timeout handler to make existing userspace happy | expand |
On 9/13/21 4:41 AM, Jan Kiszka wrote: > From: Jan Kiszka <jan.kiszka@siemens.com> > > Prominent userspace - systemd - cannot handle watchdogs without > WDIOF_SETTIMEOUT, even if it was configured to the same time as the > driver selected or was used by firmware to start the watchdog. To avoid > failing in this case, implement a handler that only fails if a deviating > set_timeout is requested. > > Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com> NACK. This is a userspace problem. The ABI clearly states that WDIOF_SETTIMEOUT is optional, and userspace must not depend on it. We can not start making kernel changes just to make broken userspace code happy. systemd should be fixed instead. Guenter > --- > > See also https://github.com/systemd/systemd/issues/20683 > > drivers/watchdog/rti_wdt.c | 17 ++++++++++++++++- > 1 file changed, 16 insertions(+), 1 deletion(-) > > diff --git a/drivers/watchdog/rti_wdt.c b/drivers/watchdog/rti_wdt.c > index 359302f71f7e..365255b15a0d 100644 > --- a/drivers/watchdog/rti_wdt.c > +++ b/drivers/watchdog/rti_wdt.c > @@ -173,13 +173,27 @@ static unsigned int rti_wdt_get_timeleft_ms(struct watchdog_device *wdd) > return timer_counter; > } > > +static int rti_wdt_set_timeout(struct watchdog_device *wdd, > + unsigned int timeout) > +{ > + /* > + * Updating the timeout after start is actually not supported, but > + * let's ignore requests for the already configured value. Helps > + * existing userspace such as systemd. > + */ > + if (timeout != heartbeat) > + return -EOPNOTSUPP; > + > + return 0; > +} > + > static unsigned int rti_wdt_get_timeleft(struct watchdog_device *wdd) > { > return rti_wdt_get_timeleft_ms(wdd) / 1000; > } > > static const struct watchdog_info rti_wdt_info = { > - .options = WDIOF_KEEPALIVEPING, > + .options = WDIOF_KEEPALIVEPING | WDIOF_SETTIMEOUT, > .identity = "K3 RTI Watchdog", > }; > > @@ -187,6 +201,7 @@ static const struct watchdog_ops rti_wdt_ops = { > .owner = THIS_MODULE, > .start = rti_wdt_start, > .ping = rti_wdt_ping, > + .set_timeout = rti_wdt_set_timeout, > .get_timeleft = rti_wdt_get_timeleft, > }; > >
Hello Jan, On Mon, Sep 13, 2021 at 01:41:43PM +0200, Jan Kiszka wrote: > From: Jan Kiszka <jan.kiszka@siemens.com> > > Prominent userspace - systemd - cannot handle watchdogs without > WDIOF_SETTIMEOUT, even if it was configured to the same time as the > driver selected or was used by firmware to start the watchdog. To avoid > failing in this case, implement a handler that only fails if a deviating > set_timeout is requested. > > Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com> Are you aware of this patch https://lore.kernel.org/all/20240417205700.3947408-1-jm@ti.com/ ? Francesco
On Mon, Sep 13, 2021 at 01:41:43PM +0200, Jan Kiszka wrote: > From: Jan Kiszka <jan.kiszka@siemens.com> > > Prominent userspace - systemd - cannot handle watchdogs without > WDIOF_SETTIMEOUT, even if it was configured to the same time as the > driver selected or was used by firmware to start the watchdog. To avoid > failing in this case, implement a handler that only fails if a deviating > set_timeout is requested. > > Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com> NACK. This will need to be fixed in systemd. set_timeout is and will remain optional. Guenter > --- > > See also https://github.com/systemd/systemd/issues/20683 > > drivers/watchdog/rti_wdt.c | 17 ++++++++++++++++- > 1 file changed, 16 insertions(+), 1 deletion(-) > > diff --git a/drivers/watchdog/rti_wdt.c b/drivers/watchdog/rti_wdt.c > index 359302f71f7e..365255b15a0d 100644 > --- a/drivers/watchdog/rti_wdt.c > +++ b/drivers/watchdog/rti_wdt.c > @@ -173,13 +173,27 @@ static unsigned int rti_wdt_get_timeleft_ms(struct watchdog_device *wdd) > return timer_counter; > } > > +static int rti_wdt_set_timeout(struct watchdog_device *wdd, > + unsigned int timeout) > +{ > + /* > + * Updating the timeout after start is actually not supported, but > + * let's ignore requests for the already configured value. Helps > + * existing userspace such as systemd. > + */ > + if (timeout != heartbeat) > + return -EOPNOTSUPP; > + > + return 0; > +} > + > static unsigned int rti_wdt_get_timeleft(struct watchdog_device *wdd) > { > return rti_wdt_get_timeleft_ms(wdd) / 1000; > } > > static const struct watchdog_info rti_wdt_info = { > - .options = WDIOF_KEEPALIVEPING, > + .options = WDIOF_KEEPALIVEPING | WDIOF_SETTIMEOUT, > .identity = "K3 RTI Watchdog", > }; > > @@ -187,6 +201,7 @@ static const struct watchdog_ops rti_wdt_ops = { > .owner = THIS_MODULE, > .start = rti_wdt_start, > .ping = rti_wdt_ping, > + .set_timeout = rti_wdt_set_timeout, > .get_timeleft = rti_wdt_get_timeleft, > }; > > -- > 2.31.1
On 21.04.24 10:49, Francesco Dolcini wrote: > Hello Jan, > > On Mon, Sep 13, 2021 at 01:41:43PM +0200, Jan Kiszka wrote: >> From: Jan Kiszka <jan.kiszka@siemens.com> >> >> Prominent userspace - systemd - cannot handle watchdogs without >> WDIOF_SETTIMEOUT, even if it was configured to the same time as the >> driver selected or was used by firmware to start the watchdog. To avoid >> failing in this case, implement a handler that only fails if a deviating >> set_timeout is requested. >> >> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com> > > Are you aware of this patch https://lore.kernel.org/all/20240417205700.3947408-1-jm@ti.com/ ? > Thanks for the heads-up, we will surely try to test things on our setups as well. But commenting on this dead and not directly related thread may have caused some confusion... Jan
On 22.04.24 01:50, Guenter Roeck wrote: > On Mon, Sep 13, 2021 at 01:41:43PM +0200, Jan Kiszka wrote: >> From: Jan Kiszka <jan.kiszka@siemens.com> >> >> Prominent userspace - systemd - cannot handle watchdogs without >> WDIOF_SETTIMEOUT, even if it was configured to the same time as the >> driver selected or was used by firmware to start the watchdog. To avoid >> failing in this case, implement a handler that only fails if a deviating >> set_timeout is requested. >> >> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com> > > NACK. > > This will need to be fixed in systemd. set_timeout is and will remain > optional. Err, this patch was sent in 2021, and even systemd is long fixed (since v250). Jan
Il 22 aprile 2024 07:35:52 CEST, Jan Kiszka <jan.kiszka@siemens.com> ha scritto: >On 21.04.24 10:49, Francesco Dolcini wrote: >> On Mon, Sep 13, 2021 at 01:41:43PM +0200, Jan Kiszka wrote: >>> From: Jan Kiszka <jan.kiszka@siemens.com> >>> >>> Prominent userspace - systemd - cannot handle watchdogs without >>> WDIOF_SETTIMEOUT, even if it was configured to the same time as the >>> driver selected or was used by firmware to start the watchdog. To avoid >>> failing in this case, implement a handler that only fails if a deviating >>> set_timeout is requested. >>> >>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com> >> >> Are you aware of this patch https://lore.kernel.org/all/20240417205700.3947408-1-jm@ti.com/ ? >> > >Thanks for the heads-up, we will surely try to test things on our setups >as well. But commenting on this dead and not directly related thread may >have caused some confusion... I was too terse. We have working systemd watchdog with that patch applied, this is the reason I mentioned it here, it might also solve your issue. Francesco
Il 22 aprile 2024 10:20:07 CEST, Francesco Dolcini <francesco@dolcini.it> ha scritto: > > >Il 22 aprile 2024 07:35:52 CEST, Jan Kiszka <jan.kiszka@siemens.com> ha scritto: >>On 21.04.24 10:49, Francesco Dolcini wrote: >>> On Mon, Sep 13, 2021 at 01:41:43PM +0200, Jan Kiszka wrote: >>>> From: Jan Kiszka <jan.kiszka@siemens.com> >>>> >>>> Prominent userspace - systemd - cannot handle watchdogs without >>>> WDIOF_SETTIMEOUT, even if it was configured to the same time as the >>>> driver selected or was used by firmware to start the watchdog. To avoid >>>> failing in this case, implement a handler that only fails if a deviating >>>> set_timeout is requested. >>>> >>>> Signed-off-by: Jan Kiszka <jan.kiszka@siemens.com> >>> >>> Are you aware of this patch https://lore.kernel.org/all/20240417205700.3947408-1-jm@ti.com/ ? >>> >> >>Thanks for the heads-up, we will surely try to test things on our setups >>as well. But commenting on this dead and not directly related thread may >>have caused some confusion... > >I was too terse. And I did not realize your patch was from 2021. Sorry for the confusion. Francesco
diff --git a/drivers/watchdog/rti_wdt.c b/drivers/watchdog/rti_wdt.c index 359302f71f7e..365255b15a0d 100644 --- a/drivers/watchdog/rti_wdt.c +++ b/drivers/watchdog/rti_wdt.c @@ -173,13 +173,27 @@ static unsigned int rti_wdt_get_timeleft_ms(struct watchdog_device *wdd) return timer_counter; } +static int rti_wdt_set_timeout(struct watchdog_device *wdd, + unsigned int timeout) +{ + /* + * Updating the timeout after start is actually not supported, but + * let's ignore requests for the already configured value. Helps + * existing userspace such as systemd. + */ + if (timeout != heartbeat) + return -EOPNOTSUPP; + + return 0; +} + static unsigned int rti_wdt_get_timeleft(struct watchdog_device *wdd) { return rti_wdt_get_timeleft_ms(wdd) / 1000; } static const struct watchdog_info rti_wdt_info = { - .options = WDIOF_KEEPALIVEPING, + .options = WDIOF_KEEPALIVEPING | WDIOF_SETTIMEOUT, .identity = "K3 RTI Watchdog", }; @@ -187,6 +201,7 @@ static const struct watchdog_ops rti_wdt_ops = { .owner = THIS_MODULE, .start = rti_wdt_start, .ping = rti_wdt_ping, + .set_timeout = rti_wdt_set_timeout, .get_timeleft = rti_wdt_get_timeleft, };