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