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