Re: [PATCH] Fix set_timeout for big timeout values

[Georg Hofmann <georg@hofmannsweb.com>]

----- 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