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

[Michal Simek <monstr@monstr.eu>]

[-- Attachment #1: Type: text/plain, Size: 3256 bytes --]

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@xilinx.com>
>>>>>> ---
>>>>>>  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?
Or my point of view is completely incorrect that this driver can't
support set timeout option.

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 #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 263 bytes --]