The timeout should be invalid when it is out of the hardware
timeout range.
ACPI watchdog: Using watchdog_timeout_invalid to check parameter
timeout invalid
Signed-off-by: Liu Xinpeng <[email protected]>
---
drivers/watchdog/wdat_wdt.c | 3 +--
include/linux/watchdog.h | 17 ++++++++++++-----
2 files changed, 13 insertions(+), 7 deletions(-)
diff --git a/drivers/watchdog/wdat_wdt.c b/drivers/watchdog/wdat_wdt.c
index 195c8c004b69..d166d33ce7ae 100644
--- a/drivers/watchdog/wdat_wdt.c
+++ b/drivers/watchdog/wdat_wdt.c
@@ -450,8 +450,7 @@ static int wdat_wdt_probe(struct platform_device *pdev)
* 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 (watchdog_timeout_invalid(&wdat->wdd, timeout)) {
dev_warn(dev, "Invalid timeout %d given, using %d\n",
timeout, WDAT_DEFAULT_TIMEOUT);
timeout = WDAT_DEFAULT_TIMEOUT;
diff --git a/include/linux/watchdog.h b/include/linux/watchdog.h
index 99660197a36c..e82daeef0b26 100644
--- a/include/linux/watchdog.h
+++ b/include/linux/watchdog.h
@@ -167,6 +167,15 @@ static inline void watchdog_stop_ping_on_suspend(struct watchdog_device *wdd)
/* Use the following function to check if a timeout value is invalid */
static inline bool watchdog_timeout_invalid(struct watchdog_device *wdd, unsigned int t)
{
+ /*
+ * If a maximum/minimum hardware timeout is configured,
+ * the timeout is invalid when it is out of the range.
+ */
+ if (wdd->max_hw_heartbeat_ms)
+ return t * 1000 > wdd->max_hw_heartbeat_ms;
+ if (wdd->min_hw_heartbeat_ms)
+ return t * 1000 < wdd->min_hw_heartbeat_ms;
+
/*
* The timeout is invalid if
* - the requested value is larger than UINT_MAX / 1000
@@ -174,13 +183,11 @@ static inline bool watchdog_timeout_invalid(struct watchdog_device *wdd, unsigne
* or
* - the requested value is smaller than the configured minimum timeout,
* or
- * - a maximum hardware timeout is not configured, a maximum timeout
- * is configured, and the requested value is larger than the
- * configured maximum timeout.
+ * - maximum timeout is configured, and the requested value is larger than
+ * the configured maximum timeout.
*/
return t > UINT_MAX / 1000 || t < wdd->min_timeout ||
- (!wdd->max_hw_heartbeat_ms && wdd->max_timeout &&
- t > wdd->max_timeout);
+ (wdd->max_timeout && t > wdd->max_timeout);
}
/* Use the following function to check if a pretimeout value is invalid */
--
2.23.0
On 4/21/22 07:22, Liu Xinpeng wrote:
> The timeout should be invalid when it is out of the hardware
> timeout range.
>
> ACPI watchdog: Using watchdog_timeout_invalid to check parameter
> timeout invalid
>
> Signed-off-by: Liu Xinpeng <[email protected]>
> ---
> drivers/watchdog/wdat_wdt.c | 3 +--
> include/linux/watchdog.h | 17 ++++++++++++-----
> 2 files changed, 13 insertions(+), 7 deletions(-)
>
> diff --git a/drivers/watchdog/wdat_wdt.c b/drivers/watchdog/wdat_wdt.c
> index 195c8c004b69..d166d33ce7ae 100644
> --- a/drivers/watchdog/wdat_wdt.c
> +++ b/drivers/watchdog/wdat_wdt.c
> @@ -450,8 +450,7 @@ static int wdat_wdt_probe(struct platform_device *pdev)
> * 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 (watchdog_timeout_invalid(&wdat->wdd, timeout)) {
> dev_warn(dev, "Invalid timeout %d given, using %d\n",
> timeout, WDAT_DEFAULT_TIMEOUT);
> timeout = WDAT_DEFAULT_TIMEOUT;
> diff --git a/include/linux/watchdog.h b/include/linux/watchdog.h
> index 99660197a36c..e82daeef0b26 100644
> --- a/include/linux/watchdog.h
> +++ b/include/linux/watchdog.h
> @@ -167,6 +167,15 @@ static inline void watchdog_stop_ping_on_suspend(struct watchdog_device *wdd)
> /* Use the following function to check if a timeout value is invalid */
> static inline bool watchdog_timeout_invalid(struct watchdog_device *wdd, unsigned int t)
> {
> + /*
> + * If a maximum/minimum hardware timeout is configured,
> + * the timeout is invalid when it is out of the range.
> + */
> + if (wdd->max_hw_heartbeat_ms)
> + return t * 1000 > wdd->max_hw_heartbeat_ms;
> + if (wdd->min_hw_heartbeat_ms)
> + return t * 1000 < wdd->min_hw_heartbeat_ms;
> +
I have no idea what problem you are trying to solve, but this is
completely wrong: It defeats the purpose of having separate minimum
and maximum HW timeouts and configured timeout values. The watchdog
core takes care of those situations.
NACK.
Guenter
> /*
> * The timeout is invalid if
> * - the requested value is larger than UINT_MAX / 1000
> @@ -174,13 +183,11 @@ static inline bool watchdog_timeout_invalid(struct watchdog_device *wdd, unsigne
> * or
> * - the requested value is smaller than the configured minimum timeout,
> * or
> - * - a maximum hardware timeout is not configured, a maximum timeout
> - * is configured, and the requested value is larger than the
> - * configured maximum timeout.
> + * - maximum timeout is configured, and the requested value is larger than
> + * the configured maximum timeout.
> */
> return t > UINT_MAX / 1000 || t < wdd->min_timeout ||
> - (!wdd->max_hw_heartbeat_ms && wdd->max_timeout &&
> - t > wdd->max_timeout);
> + (wdd->max_timeout && t > wdd->max_timeout);
> }
>
> /* Use the following function to check if a pretimeout value is invalid */
On 4/21/22 16:31, liuxp11 wrote:
> > diff --git a/drivers/watchdog/wdat_wdt.c b/drivers/watchdog/wdat_wdt.c
>
> > index 195c8c004b69..d166d33ce7ae 100644
> > --- a/drivers/watchdog/wdat_wdt.c
> > +++ b/drivers/watchdog/wdat_wdt.c
> > @@ -450,8 +450,7 @@ static int wdat_wdt_probe(struct platform_device *pdev)
> > * 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 (watchdog_timeout_invalid(&wdat->wdd, timeout)) {
> > dev_warn(dev, "Invalid timeout %d given, using %d\n",
> > timeout, WDAT_DEFAULT_TIMEOUT);
> > timeout = WDAT_DEFAULT_TIMEOUT;
> Thanks your reply, read these code,thinking can put them into watchdog_timeout_invalid.
Again, no. If anything the above code is wrong; there should be no
upper limit if max_hw_heartbeat_ms is provided. The code should
probably set min_timeout and just call watchdog_timeout_invalid()
without any change in that function.
Guenter