Re: [PATCH v9 1/3] watchdog: introduce watchdog.open_timeout commandline parameter

[Rasmus Villemoes]

On 29/01/2019 21.35, Rasmus Villemoes wrote:
> On 22/01/2019 18.29, Guenter Roeck wrote:
>> On Mon, Jan 21, 2019 at 08:45:39PM +0000, Rasmus Villemoes wrote:
>>>  
>>>  static void watchdog_ping_work(struct kthread_work *work)
>>> @@ -297,7 +317,7 @@ static int watchdog_stop(struct watchdog_device *wdd)
>>>  		return -EBUSY;
>>>  	}
>>>  
>>> -	if (wdd->ops->stop) {
>>> +	if (wdd->ops->stop && !open_timeout) {
>>
>> This changes the semantics of WDIOC_SETOPTIONS / WDIOS_DISABLECARD.
>> "Turn off the watchdog timer" is well defined and doesn't leave
>> the option of setting a timeout on it.
> 
> I can drop this hunk, since it's mostly irrelevant to the actual use
> cases I have in mind. It makes testing the feature on reference boards a
> little more awkward, but I can live with that.

Actually, that's not enough - if there was a non-zero open_timeout at
boot, the device has had its ->open_deadline set to some finite (not
KTIME_MAX) value, and ->open_deadline does not get updated on the
WDIOC_SETOPTIONS / WDIOS_DISABLECARD path.

So, I think one way to preserve the semantics of WDIOS_DISABLECARD
(which, for a non-stopable watchdog really means "please let the kernel
take care of this indefinitely, or until WDIOS_ENABLECARD") is to extend
watchdog_stop with a timeout parameter, and do the call of
watchdog_set_open_deadline() from watchdog_stop() using the passed
timeout value. When called from the WDIOS_DISABLECARD case, we'd pass 0
for that timeout, while the call from _release would pass the module
parameter open_timeout. [Obviously, watchdog_set_open_deadline() would
also take the timeout as a parameter instead of referring to the
open_timeout directly.]

Thanks for pointing out the WDIOS_DISABLECARD case. I'll try to make the
above into code to see how it looks.

Rasmus