[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <354edbef-1ad5-78d8-e210-e55c38798962@prevas.se>
Date: Wed, 30 Jan 2019 07:40:15 +0000
From: Rasmus Villemoes <Rasmus.Villemoes@...vas.se>
To: Guenter Roeck <linux@...ck-us.net>
CC: "linux-watchdog@...r.kernel.org" <linux-watchdog@...r.kernel.org>,
Wim Van Sebroeck <wim@...ux-watchdog.org>,
Jonathan Corbet <corbet@....net>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
"linux-doc@...r.kernel.org" <linux-doc@...r.kernel.org>,
Esben Haabendal <esben@...bendal.dk>,
"martin@...deboll.net" <martin@...deboll.net>
Subject: Re: [PATCH v9 1/3] watchdog: introduce watchdog.open_timeout
commandline parameter
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
Powered by blists - more mailing lists