| Message ID | 1274287303.1911.1554545863744.JavaMail.zimbra@hofmannsweb.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Headers | show |
| Series | Fix set_timeout for big timeout values | expand |
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@hofmannsweb.com> > --- > 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
----- Original Message ----- > From: "Guenter Roeck" <linux@roeck-us.net> > To: "Georg Hofmann" <georg@hofmannsweb.com>, wim@linux-watchdog.org > Cc: linux-watchdog@vger.kernel.org > 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@hofmannsweb.com> >> --- >> 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. Georg
On 4/6/19 7:17 AM, Georg Hofmann wrote: > ----- Original Message ----- >> From: "Guenter Roeck" <linux@roeck-us.net> >> To: "Georg Hofmann" <georg@hofmannsweb.com>, wim@linux-watchdog.org >> Cc: linux-watchdog@vger.kernel.org >> 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@hofmannsweb.com> >>> --- >>> 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
----- Original Message ----- > From: "Guenter Roeck" <linux@roeck-us.net> > To: "Georg Hofmann" <georg@hofmannsweb.com> > Cc: wim@linux-watchdog.org, linux-watchdog@vger.kernel.org > Sent: Saturday, April 6, 2019 5:46:08 PM > Subject: Re: [PATCH] Fix set_timeout for big timeout values > On 4/6/19 7:17 AM, Georg Hofmann wrote: >> ----- Original Message ----- >>> From: "Guenter Roeck" <linux@roeck-us.net> >>> To: "Georg Hofmann" <georg@hofmannsweb.com>, wim@linux-watchdog.org >>> Cc: linux-watchdog@vger.kernel.org >>> 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@hofmannsweb.com> >>>> --- >>>> 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 ah, ok, I missed that. I will change test it and than resend the patch. Thanks for your time. Georg
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; return 0; }
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@hofmannsweb.com> --- drivers/watchdog/imx2_wdt.c | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-)