Re: [PATCH v1] watchdog: add new parameter to start the watchdog on module insertion

[Guenter Roeck]

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