Re: [PATCH 3/3] watchdog: xilinx: Add WDIOC_SETTIMEOUT ioctl function

[Michal Simek]

On 05/31/2013 12:08 AM, Wim Van Sebroeck wrote:
> Hi All,
> 
>>> On Thu, May 30, 2013 at 05:12:24PM +0200, Michal Simek wrote:
>>>> On 05/30/2013 05:03 PM, Guenter Roeck wrote:
>>>>> On Thu, May 30, 2013 at 04:34:02PM +0200, Michal Simek wrote:
>>>>>> On 05/30/2013 04:21 PM, Guenter Roeck wrote:
>>>>>>> On Thu, May 30, 2013 at 04:15:45PM +0200, Michal Simek wrote:
>>>>>>>> On 05/30/2013 04:07 PM, Guenter Roeck wrote:
>>>>>>>>> On Thu, May 30, 2013 at 02:26:04PM +0200, Michal Simek wrote:
>>>>>>>>>> Standard watchdog programs try to setup timeout
>>>>>>>>>> via ioctl and this functionality should be implemented.
>>>>>>>>>> Timeout value is hardcoded in the hardware but
>>>>>>>>>> based on Documentation/watchdog/watchdog-api.txt
>>>>>>>>>> can return the real timeout used in the same variable.
>>>>>>>>>>
>>>>>>>>>> Signed-off-by: Michal Simek <michal.simek@xxxxxxxxxx>
>>>>>>>>>> ---
>>>>>>>>>>  drivers/watchdog/of_xilinx_wdt.c | 1 +
>>>>>>>>>>  1 file changed, 1 insertion(+)
>>>>>>>>>>
>>>>>>>>>> diff --git a/drivers/watchdog/of_xilinx_wdt.c
>>> b/drivers/watchdog/of_xilinx_wdt.c
>>>>>>>>>> index 79f358c..a3bbe72 100644
>>>>>>>>>> --- a/drivers/watchdog/of_xilinx_wdt.c
>>>>>>>>>> +++ b/drivers/watchdog/of_xilinx_wdt.c
>>>>>>>>>> @@ -253,6 +253,7 @@ static long xwdt_ioctl(struct file *file,
>>> unsigned int cmd, unsigned long arg)
>>>>>>>>>>                xwdt_keepalive();
>>>>>>>>>>                return 0;
>>>>>>>>>>
>>>>>>>>>> +      case WDIOC_SETTIMEOUT:
>>>>>>>>>>        case WDIOC_GETTIMEOUT:
>>>>>>>>>>                if (no_timeout)
>>>>>>>>>>                        return -ENOTTY;
>>>>>>>>>
>>>>>>>>> Watchdog programs should check ident.options before trying to set
>>> the timeout.
>>>>>>>>> If they don't, there is an application bug. I don't think it is a
>>> good idea
>>>>>>>>> to start hacking the kernel to work around application bugs.
>>>>>>>>
>>>>>>>> Based on Documentation/watchdog/watchdog-api.txt
>>>>>>>>
>>>>>>>> "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
>>>>>>>
>>>>>>> Yes, but WDIOF_SETTIMEOUT is not set in the driver's option field.
>>>>>>
>>>>>> ok. It means I should probably enable it.
>>>>>>
>>>>> I am missing your point. Applications should not try to write the
>>> timeout
>>>>> since WDIOF_SETTIMEOUT is not set. Any application doing it anyway is
>>> buggy
>>>>> and should be fixed.
>>>>
>>>> I fully understand your points and 100% agree with you
>>>> 1. the application is broken and should be fixed
>>>> 2. also the kernel shouldn't fix any problem with stupid application
>>>>
>>>> But based on documentation the driver can support setup timeout
>>>> and based on description "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"
>>>> Based on this I still think that the driver can support set timeout
>>>> feature and if the driver supports this option then WDIOF_SETTIMEOUT
>>>> should be set in driver's option field. And I would add this to v2.
>>>>
>>>> Can you see my point now?
>>>
>>> No. The driver doesn't support setting the timeout. You just want it
>>> to falsely claim that it does to work around an application problem.
>>> With your logic, _every_ watchdog driver would "support" setting
>>> the timeout.
>>>
>>
>> I don't want to falsely claim anything.
>> I am just saying this is written in the documentation and
>> it is my understanding that this can be implemented it this way
>> for this xilinx device and behaviour of the driver will be correct
>> according to documentation which I copied to this thread.
>>
>> If Wim says that if device doesn't support setting timeout in HW
>> then this ioctl can't be implemented in this way I am definitely OK with
>> it.
>> And I will remove this patch and will also remove this change
>> from our xilinx repo.
>> The main purpose for me is to cleanup our repo and push all changes
>> to the mainline. Of course if this change goes against watchdog
>> logic and it is broken I will the first who will revert it in our repo
>> and will have proper description based on our discussion.
>>
>> I really appreciate your inputs for this discussion and both
>> resolution ACK/NACK are OK because I will know what's the correct
>> way and I can fix it in mainline or our repo and both will be in sync.
> 
> Logic is: if device supports setting timeout values, then and only then
> you indicate this by setting the WDIOF_SETTIMEOUT flag and adding the code
> for it. And then you can do minor adjustments if needed and report this back.
> But that's only to make sure that some roundings (like a timer in minutes 
> gives back 60 or 120 seconds if you would set a new timeout of 70 seconds).
> 
> So in this case: the HW doesn't support setting timeout values,
> so we don't add the WDIOF_SETTIMEOUT flag and thus we don't add the ioctl
> call neither. So NACK on this patch.

ok. Good. I will revert this change in our tree and will send v2 without
it.

Thanks,
Michal

-- 
Michal Simek, Ing. (M.Eng), OpenPGP -> KeyID: FE3D1F91
w: www.monstr.eu p: +42-0-721842854
Maintainer of Linux kernel - Microblaze cpu - http://www.monstr.eu/fdt/
Maintainer of Linux kernel - Xilinx Zynq ARM architecture
Microblaze U-BOOT custodian and responsible for u-boot arm zynq platform

Attachment: signature.asc
Description: OpenPGP digital signature