[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <444e0f7e-c937-49a4-d960-549e70a8390d@roeck-us.net>
Date: Mon, 12 Apr 2021 07:42:35 -0700
From: Guenter Roeck <linux@...ck-us.net>
To: Flavio Suligoi <f.suligoi@...m.it>
Cc: Mika Westerberg <mika.westerberg@...ux.intel.com>,
"linux-watchdog@...r.kernel.org" <linux-watchdog@...r.kernel.org>,
"linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
Wim Van Sebroeck <wim@...ux-watchdog.org>
Subject: Re: [PATCH v1] watchdog: add new parameter to start the watchdog on
module insertion
On 4/12/21 1:29 AM, Flavio Suligoi wrote:
> Hi Guenter,
>
> ...
>
>> On 4/9/21 2:34 AM, Flavio Suligoi wrote:
>>> The new parameter "start_enabled" starts the watchdog at the same time
>>> of the module insertion.
>>> This feature is very useful in embedded systems, to avoid cases where
>>> the system hangs before reaching userspace.
>>>
>>> This function can be also enabled in the kernel config, so can be
>>> used when the watchdog driver is build as built-in.
>>>
>>> This parameter involves the "core" section of the watchdog driver;
>>> in this way it is common for all the watchdog hardware implementations.
>>>
>>> Note: to use only for watchdog drivers which doesn't support this
>>> parameter by itself.
>>>
>>> Signed-off-by: Flavio Suligoi <f.suligoi@...m.it>
>>> ---
>>> Documentation/watchdog/watchdog-parameters.rst | 5 +++++
>>> drivers/watchdog/Kconfig | 14 ++++++++++++++
>>> drivers/watchdog/watchdog_core.c | 12 ++++++++++++
>>> 3 files changed, 31 insertions(+)
>>>
>>> diff --git a/Documentation/watchdog/watchdog-parameters.rst
>> b/Documentation/watchdog/watchdog-parameters.rst
>>> index 223c99361a30..623fd064df91 100644
>>> --- a/Documentation/watchdog/watchdog-parameters.rst
>>> +++ b/Documentation/watchdog/watchdog-parameters.rst
>>> @@ -21,6 +21,11 @@ watchdog core:
>>> timeout. Setting this to a non-zero value can be useful to ensure that
>>> either userspace comes up properly, or the board gets reset and allows
>>> fallback logic in the bootloader to try something else.
>>> + start_enabled:
>>> + Watchdog is started on module insertion. This option can be also
>>> + selected by kernel config (default=kernel config parameter).
>>> + Use only for watchdog drivers which doesn't support this parameter
>>> + by itself.
>>
>> Why ?
>
> There are two drivers with an analogous feature (pnx833x_wdt and
> omap_wdt) and it is important not to enable the watchdog twice.
>
Why ?
> Ok, I can substitute the sentence: " Use only for watchdog drivers
> which doesn't support this parameter itself." with another one, like:
> "If the driver supports this feature by itself, be carefully not to enable
> the watchdog twice".
>
> What do you think?
>
I am still missing the explanation _why_ it would be important not to enable
a watchdog twice. Why does it matter ? What is the difference ?
If there is a concern that the start function should not be called on an already
running watchdog, the code could check for that and ensure that WDOG_HW_RUNNING
is not already set before enabling it. That would probably make sense anyway.
But adding a limitation/restriction like the above, which is not enforceable,
is not a good idea. How would the common user know if a watchdog is already
running (eg because it was started in BIOS/ROMMON) ?
Thanks,
Guenter
Powered by blists - more mailing lists