On 4/6/19 7:17 AM, Georg Hofmann wrote:
----- Original Message -----
From: "Guenter Roeck" <linux@xxxxxxxxxxxx>
To: "Georg Hofmann" <georg@xxxxxxxxxxxxxxx>, wim@xxxxxxxxxxxxxxxxxx
Cc: linux-watchdog@xxxxxxxxxxxxxxx
Sent: Saturday, April 6, 2019 3:25:44 PM
Subject: Re: [PATCH] Fix set_timeout for big timeout values
On 4/6/19 3:17 AM, Georg Hofmann wrote:
This patch implements the documented behavior: if max_hw_heartbeat_ms is
implemented, the minimum of the set_timeout argument and
max_hw_heartbeat_ms should be used.
Previously only the first 7 bits were used and the input argument was
returned.
Signed-off-by: Georg Hofmann <georg@xxxxxxxxxxxxxxx>
---
drivers/watchdog/imx2_wdt.c | 6 ++++--
1 file changed, 4 insertions(+), 2 deletions(-)
diff --git a/drivers/watchdog/imx2_wdt.c b/drivers/watchdog/imx2_wdt.c
index 2b52514..3c13adc 100644
--- a/drivers/watchdog/imx2_wdt.c
+++ b/drivers/watchdog/imx2_wdt.c
@@ -178,9 +178,11 @@ static void __imx2_wdt_set_timeout(struct watchdog_device
*wdog,
static int imx2_wdt_set_timeout(struct watchdog_device *wdog,
unsigned int new_timeout)
{
- __imx2_wdt_set_timeout(wdog, new_timeout);
+ unsigned int actual;
- wdog->timeout = new_timeout;
+ actual = min(new_timeout, wdog->max_hw_heartbeat_ms * 1000);
+ __imx2_wdt_set_timeout(wdog, actual);
+ wdog->timeout = actual;
That defeats the purpose of having an internal maximum. wdog->timeout
should still be set to the requested value.
Guenter
Hi,
I don't understand, the internal maximum is max_hw_heartbeat_ms.
I have used the same code as other watchdog drivers
(e.g. aspeed_wdt.c, loongson1_wdt.c, ...).
I have submitted this patch because I was writing a userspace
program and I expected a different behavior on the ioctl.
The watchdog documentation says (Documentation/watchdog/watchdog-api.txt):
Setting and getting the timeout:
For some drivers it is possible to modify the watchdog timeout on the
fly with the SETTIMEOUT ioctl, those drivers have the WDIOF_SETTIMEOUT
flag set in their option field. The argument is an integer
representing the timeout in seconds. The driver returns the real
timeout used in the same variable, and this timeout might differ from
the requested one due to limitation of the hardware.
int timeout = 45;
ioctl(fd, WDIOC_SETTIMEOUT, &timeout);
printf("The timeout was set to %d seconds\n", timeout);
This example might actually print "The timeout was set to 60 seconds"
if the device has a granularity of minutes for its timeout.
As the watchdog core driver reads the timeout just after write, I have
to set the applied value to timeout.
Initially I thought I should get a error message if the timeout can't
be applied, but the documentation describes another behavior.
The whole point of max_hw_heartbeat_ms is to be able to specify a larger
timeout than the maximum supported by the hardware. In extreme cases,
max_hw_heartbeat_ms could be as low as 1 second (or less). Limiting
wdog->timeout to that value is identical to not having max_hw_heartbeat_ms
in the first place, and thus quite pointless.
aspeed_wdt.c does _not_ set wdd->timeout to the 'actual' value.
loongson1_wdt.c doesn't do it either. If any other driver does it,
it is wrong.
Your patch is almost correct, except it should keep
"wdog->timeout = new_timeout;".
Guenter
