----- Original Message -----
> From: "Guenter Roeck" <linux@xxxxxxxxxxxx>
> To: "Georg Hofmann" <georg@xxxxxxxxxxxxxxx>
> Cc: wim@xxxxxxxxxxxxxxxxxx, linux-watchdog@xxxxxxxxxxxxxxx
> 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@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
ah, ok, I missed that. I will change test it and than resend the patch.
Thanks for your time.
Georg
