| Message ID | 20220806000706.3eeafc9c@endymion.delvare |
|---|---|
| State | Superseded |
| Headers | show |
| Series | watchdog: wdat_wdt: Set the min and max timeout values properly | expand |
On Sat, Aug 06, 2022 at 12:07:06AM +0200, Jean Delvare wrote: > The wdat_wdt driver is misusing the min_hw_heartbeat_ms field. This > field should only be used when the hardware watchdog device should not > be pinged more frequently than a specific period. The ACPI WDAT > "Minimum Count" field, on the other hand, specifies the minimum > timeout value that can be set. This corresponds to the min_timeout > field in Linux's watchdog infrastructure. > > Setting min_hw_heartbeat_ms instead can cause pings to the hardware > to be delayed when there is no reason for that, eventually leading to > unexpected firing of the watchdog timer (and thus unexpected reboot). > > I'm also changing max_hw_heartbeat_ms to max_timeout for symmetry, > although the use of this one isn't fundamentally wrong, but there is > also no reason to enable the software-driven ping mechanism for the > wdat_wdt driver. > > Signed-off-by: Jean Delvare <jdelvare@suse.de> > Fixes: 058dfc767008 ("ACPI / watchdog: Add support for WDAT hardware watchdog") > Cc: Wim Van Sebroeck <wim@linux-watchdog.org> > Cc: Guenter Roeck <linux@roeck-us.net> > Cc! Mika Westerberg <mika.westerberg@linux.intel.com> Reviewed-by: Mika Westerberg <mika.westerberg@linux.intel.com>
On 8/5/22 15:07, Jean Delvare wrote: > The wdat_wdt driver is misusing the min_hw_heartbeat_ms field. This > field should only be used when the hardware watchdog device should not > be pinged more frequently than a specific period. The ACPI WDAT > "Minimum Count" field, on the other hand, specifies the minimum > timeout value that can be set. This corresponds to the min_timeout > field in Linux's watchdog infrastructure. > > Setting min_hw_heartbeat_ms instead can cause pings to the hardware > to be delayed when there is no reason for that, eventually leading to > unexpected firing of the watchdog timer (and thus unexpected reboot). > > I'm also changing max_hw_heartbeat_ms to max_timeout for symmetry, > although the use of this one isn't fundamentally wrong, but there is > also no reason to enable the software-driven ping mechanism for the > wdat_wdt driver. > Normally I would reject this because it is not only unnecessary and unrelated to the problem at hand (remember: one logical change per patch), but it is hidden in an unrelated patch, it will only make life harder later on if/when full milli-second timeouts are introduced, and it may result in unexpected limitations on the maximum timeout. However, Mike accepted it, so who am I to complain. > Signed-off-by: Jean Delvare <jdelvare@suse.de> > Fixes: 058dfc767008 ("ACPI / watchdog: Add support for WDAT hardware watchdog") > Cc: Wim Van Sebroeck <wim@linux-watchdog.org> > Cc: Guenter Roeck <linux@roeck-us.net> > Cc! Mika Westerberg <mika.westerberg@linux.intel.com> > Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com> > --- > Untested, as I have no supported hardware at hand. > > Note to the watchdog subsystem maintainers: I must say I find the > whole thing pretty confusing. > > First of all, the name symmetry between min_hw_heartbeat_ms and > max_hw_heartbeat_ms, while these properties are completely unrelated, > is heavily misleading. max_hw_heartbeat_ms is really max_hw_timeout > and should be renamed to that IMHO, if we keep it at all. > Variable names are hardly ever perfect. I resist renaming variables to avoid rename wars. Feel free to submit patches to improve the documentation if you like. > Secondly, the coexistence of max_timeout and max_hw_heartbeat_ms is > also making the code pretty hard to understand and get right. > Historically, max_timeout was already supposed to be the maximum > hardware timeout value. I don't understand why a new field with that > meaning was introduced, subsequently changing the original meaning of > max_timeout to become a software-only limit... but only if > max_hw_heartbeat_ms is set. > Code is hardly ever perfect. Feel free to submit patches to help improve understanding if you like. > To be honest, I'm not sold to the idea of a software-emulated > maximum timeout value above what the hardware can do, but if doing > that makes sense in certain situations, then I believe it should be > implemented as a boolean flag (named emulate_large_timeout, for > example) to complement max_timeout instead of a separate time value. > Is there a reason I'm missing, why it was not done that way? > There are watchdogs with very low maximum timeout values, sometimes less than 3 seconds. gpio-wdt is one example - some have a maximum value of 2.5 seconds. rzn1_wd is even more extreme with a maximum of 1 second. With such low values, accuracy is important, second-based limits are insufficient, and there is an actual need for software timeout handling on top of hardware. At the same time, there is actually a need to make timeouts milli-second based instead of second-based, for uses such as medical devices where timeouts need to be short and accurate. The only reason for not implementing this is that the proposals I have seen so far (including mine) were too messy for my liking, and I never had the time to clean it up. Reverting milli-second support would be the completely wrong direction. > Currently, a comment in watchdog.h claims that max_timeout is ignored > when max_hw_heartbeat_ms is set. However in watchdog_dev.c, sysfs > attribute max_timeout is created unconditionally, and > max_hw_heartbeat_ms doesn't have a sysfs attribute. So userspace has > no way to know if max_timeout is the hardware limit, or whether > software emulation will kick in for a specified timeout value. Also, > there is no complaint if both max_hw_heartbeat_ms and max_timeout > are set. > As mentioned before, code is hardly ever perfect. Patches to improve the situation are welcome. Thanks, Guenter
Hi Guenter, A few questions from an old discussion: On Mon, 8 Aug 2022 04:36:42 -0700, Guenter Roeck wrote: > On 8/5/22 15:07, Jean Delvare wrote: > > To be honest, I'm not sold to the idea of a software-emulated > > maximum timeout value above what the hardware can do, but if doing > > that makes sense in certain situations, then I believe it should be > > implemented as a boolean flag (named emulate_large_timeout, for > > example) to complement max_timeout instead of a separate time value. > > Is there a reason I'm missing, why it was not done that way? > > There are watchdogs with very low maximum timeout values, sometimes less than > 3 seconds. gpio-wdt is one example - some have a maximum value of 2.5 seconds. > rzn1_wd is even more extreme with a maximum of 1 second. With such low values, > accuracy is important, second-based limits are insufficient, and there is an > actual need for software timeout handling on top of hardware. Out of curiosity, what prevents user-space itself from pinging /dev/watchdog every 0.5 second? I assume hardware using such watchdog devices is "special" and would be running finely tuned user-space, so the process pinging /dev/watchdog could be given higher priority or even real-time status to ensure it runs without delays. Is that not sufficient? > At the same time, there is actually a need to make timeouts milli-second based > instead of second-based, for uses such as medical devices where timeouts need > to be short and accurate. The only reason for not implementing this is that > the proposals I have seen so far (including mine) were too messy for my liking, > and I never had the time to clean it up. Reverting milli-second support would > be the completely wrong direction. I might look into this at some point (for example as a SUSE Hackweek project). Did you post your work somewhere? I'd like to take a look. Thanks,
On 9/19/22 02:33, Jean Delvare wrote: > Hi Guenter, > > A few questions from an old discussion: > > On Mon, 8 Aug 2022 04:36:42 -0700, Guenter Roeck wrote: >> On 8/5/22 15:07, Jean Delvare wrote: >>> To be honest, I'm not sold to the idea of a software-emulated >>> maximum timeout value above what the hardware can do, but if doing >>> that makes sense in certain situations, then I believe it should be >>> implemented as a boolean flag (named emulate_large_timeout, for >>> example) to complement max_timeout instead of a separate time value. >>> Is there a reason I'm missing, why it was not done that way? >> >> There are watchdogs with very low maximum timeout values, sometimes less than >> 3 seconds. gpio-wdt is one example - some have a maximum value of 2.5 seconds. >> rzn1_wd is even more extreme with a maximum of 1 second. With such low values, >> accuracy is important, second-based limits are insufficient, and there is an >> actual need for software timeout handling on top of hardware. > > Out of curiosity, what prevents user-space itself from pinging > /dev/watchdog every 0.5 second? I assume hardware using such watchdog > devices is "special" and would be running finely tuned user-space, so > the process pinging /dev/watchdog could be given higher priority or > even real-time status to ensure it runs without delays. Is that not > sufficient? > It took us forever to get the in-kernel support stable, using the right timers and making sure that the kernel actually executes the code fast enough. Maybe that would work nowadays from a userspace process with the right permissions, but I would not trust it. Then there is watchdog support in, for example, systemd. I would not want to force users to run systemd as high priority real-time process just to make an odd watchdog work. I also would not want to tell people that they must not use the systemd watchdog timer to make their watchdog work. Also, there is no guarantee that the odd hardware with the weird watchdog hardware is actually always used in an application where such a fast timeout is needed or even wanted. On top of that, the code in the kernel also now supports "ping until opened" for systems where the watchdog is already running when the system boots. Overall, I don't think it would be a good idea to revert the in-kernel support of pinging watchdogs. >> At the same time, there is actually a need to make timeouts milli-second based >> instead of second-based, for uses such as medical devices where timeouts need >> to be short and accurate. The only reason for not implementing this is that >> the proposals I have seen so far (including mine) were too messy for my liking, >> and I never had the time to clean it up. Reverting milli-second support would >> be the completely wrong direction. > > I might look into this at some point (for example as a SUSE Hackweek > project). Did you post your work somewhere? I'd like to take a look. > There was one submission from someone else if I recall correctly, but mine never got to the point where it was submittable. Guenter
--- linux-5.18.orig/drivers/watchdog/wdat_wdt.c 2022-07-27 07:32:33.336928967 +0200 +++ linux-5.18/drivers/watchdog/wdat_wdt.c 2022-08-05 19:49:19.607793835 +0200 @@ -342,8 +342,8 @@ static int wdat_wdt_probe(struct platfor return -EINVAL; wdat->period = tbl->timer_period; - wdat->wdd.min_hw_heartbeat_ms = wdat->period * tbl->min_count; - wdat->wdd.max_hw_heartbeat_ms = wdat->period * tbl->max_count; + wdat->wdd.min_timeout = DIV_ROUND_UP(wdat->period * tbl->min_count, 1000); + wdat->wdd.max_timeout = wdat->period * tbl->max_count / 1000; wdat->stopped_in_sleep = tbl->flags & ACPI_WDAT_STOPPED; wdat->wdd.info = &wdat_wdt_info; wdat->wdd.ops = &wdat_wdt_ops; @@ -450,8 +450,8 @@ static int wdat_wdt_probe(struct platfor * watchdog properly after it has opened the device. In some cases * the BIOS default is too short and causes immediate reboot. */ - if (timeout * 1000 < wdat->wdd.min_hw_heartbeat_ms || - timeout * 1000 > wdat->wdd.max_hw_heartbeat_ms) { + if (timeout < wdat->min_timeout || + timeout > wdat->max_timeout) { dev_warn(dev, "Invalid timeout %d given, using %d\n", timeout, WDAT_DEFAULT_TIMEOUT); timeout = WDAT_DEFAULT_TIMEOUT;
The wdat_wdt driver is misusing the min_hw_heartbeat_ms field. This field should only be used when the hardware watchdog device should not be pinged more frequently than a specific period. The ACPI WDAT "Minimum Count" field, on the other hand, specifies the minimum timeout value that can be set. This corresponds to the min_timeout field in Linux's watchdog infrastructure. Setting min_hw_heartbeat_ms instead can cause pings to the hardware to be delayed when there is no reason for that, eventually leading to unexpected firing of the watchdog timer (and thus unexpected reboot). I'm also changing max_hw_heartbeat_ms to max_timeout for symmetry, although the use of this one isn't fundamentally wrong, but there is also no reason to enable the software-driven ping mechanism for the wdat_wdt driver. Signed-off-by: Jean Delvare <jdelvare@suse.de> Fixes: 058dfc767008 ("ACPI / watchdog: Add support for WDAT hardware watchdog") Cc: Wim Van Sebroeck <wim@linux-watchdog.org> Cc: Guenter Roeck <linux@roeck-us.net> Cc! Mika Westerberg <mika.westerberg@linux.intel.com> Cc: Rafael J. Wysocki <rafael.j.wysocki@intel.com> --- Untested, as I have no supported hardware at hand. Note to the watchdog subsystem maintainers: I must say I find the whole thing pretty confusing. First of all, the name symmetry between min_hw_heartbeat_ms and max_hw_heartbeat_ms, while these properties are completely unrelated, is heavily misleading. max_hw_heartbeat_ms is really max_hw_timeout and should be renamed to that IMHO, if we keep it at all. Secondly, the coexistence of max_timeout and max_hw_heartbeat_ms is also making the code pretty hard to understand and get right. Historically, max_timeout was already supposed to be the maximum hardware timeout value. I don't understand why a new field with that meaning was introduced, subsequently changing the original meaning of max_timeout to become a software-only limit... but only if max_hw_heartbeat_ms is set. To be honest, I'm not sold to the idea of a software-emulated maximum timeout value above what the hardware can do, but if doing that makes sense in certain situations, then I believe it should be implemented as a boolean flag (named emulate_large_timeout, for example) to complement max_timeout instead of a separate time value. Is there a reason I'm missing, why it was not done that way? Currently, a comment in watchdog.h claims that max_timeout is ignored when max_hw_heartbeat_ms is set. However in watchdog_dev.c, sysfs attribute max_timeout is created unconditionally, and max_hw_heartbeat_ms doesn't have a sysfs attribute. So userspace has no way to know if max_timeout is the hardware limit, or whether software emulation will kick in for a specified timeout value. Also, there is no complaint if both max_hw_heartbeat_ms and max_timeout are set. drivers/watchdog/wdat_wdt.c | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-)