linux-watchdog.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v1] watchdog: add new parameter to start the watchdog on module insertion
@ 2021-04-09  9:34 Flavio Suligoi
  2021-04-09 15:56 ` Guenter Roeck
  0 siblings, 1 reply; 5+ messages in thread
From: Flavio Suligoi @ 2021-04-09  9:34 UTC (permalink / raw)
  To: Wim Van Sebroeck, Guenter Roeck
  Cc: Mika Westerberg, linux-watchdog, linux-kernel, Flavio Suligoi

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@asem.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.
 
 -------------------------------------------------
 
diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
index 0470dc15c085..c2a668d6bbbc 100644
--- a/drivers/watchdog/Kconfig
+++ b/drivers/watchdog/Kconfig
@@ -47,6 +47,20 @@ config WATCHDOG_NOWAYOUT
 	  get killed. If you say Y here, the watchdog cannot be stopped once
 	  it has been started.
 
+config WATCHDOG_START_ENABLED
+	bool "Start watchdog on module insertion"
+	help
+	  Say Y if you want to start the watchdog at the same time when the
+	  driver is loaded.
+	  This feature is very useful in embedded systems, to avoid cases where
+	  the system could hang before reaching userspace.
+	  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.
+
 config WATCHDOG_HANDLE_BOOT_ENABLED
 	bool "Update boot-enabled watchdog until userspace takes over"
 	default y
diff --git a/drivers/watchdog/watchdog_core.c b/drivers/watchdog/watchdog_core.c
index 5df0a22e2cb4..5052ae355219 100644
--- a/drivers/watchdog/watchdog_core.c
+++ b/drivers/watchdog/watchdog_core.c
@@ -43,6 +43,11 @@ static int stop_on_reboot = -1;
 module_param(stop_on_reboot, int, 0444);
 MODULE_PARM_DESC(stop_on_reboot, "Stop watchdogs on reboot (0=keep watching, 1=stop)");
 
+static bool start_enabled = IS_ENABLED(CONFIG_WATCHDOG_START_ENABLED);
+module_param(start_enabled, bool, 0444);
+MODULE_PARM_DESC(start_enabled, "Start watchdog on module insertion (default="
+	__MODULE_STRING(IS_ENABLED(CONFIG_WATCHDOG_START_ENABLED)) ")");
+
 /*
  * Deferred Registration infrastructure.
  *
@@ -224,6 +229,13 @@ static int __watchdog_register_device(struct watchdog_device *wdd)
 	 * corrupted in a later stage then we expect a kernel panic!
 	 */
 
+	/* If required, start the watchdog immediately */
+	if (start_enabled) {
+		set_bit(WDOG_HW_RUNNING, &wdd->status);
+		wdd->ops->start(wdd);
+		pr_info("Watchdog enabled\n");
+	}
+
 	/* Use alias for watchdog id if possible */
 	if (wdd->parent) {
 		ret = of_alias_get_id(wdd->parent->of_node, "watchdog");
-- 
2.25.1


^ permalink raw reply related	[flat|nested] 5+ messages in thread

* Re: [PATCH v1] watchdog: add new parameter to start the watchdog on module insertion
  2021-04-09  9:34 [PATCH v1] watchdog: add new parameter to start the watchdog on module insertion Flavio Suligoi
@ 2021-04-09 15:56 ` Guenter Roeck
  2021-04-12  8:29   ` Flavio Suligoi
  0 siblings, 1 reply; 5+ messages in thread
From: Guenter Roeck @ 2021-04-09 15:56 UTC (permalink / raw)
  To: Flavio Suligoi, Wim Van Sebroeck
  Cc: Mika Westerberg, linux-watchdog, linux-kernel

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@asem.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 ?

>  
>  -------------------------------------------------
>  
> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
> index 0470dc15c085..c2a668d6bbbc 100644
> --- a/drivers/watchdog/Kconfig
> +++ b/drivers/watchdog/Kconfig
> @@ -47,6 +47,20 @@ config WATCHDOG_NOWAYOUT
>  	  get killed. If you say Y here, the watchdog cannot be stopped once
>  	  it has been started.
>  
> +config WATCHDOG_START_ENABLED
> +	bool "Start watchdog on module insertion"
> +	help
> +	  Say Y if you want to start the watchdog at the same time when the
> +	  driver is loaded.
> +	  This feature is very useful in embedded systems, to avoid cases where
> +	  the system could hang before reaching userspace.
> +	  This parameter involves the "core" section of the watchdog driver,
> +	  in this way it is common for all the watchdog hardware
> +	  implementations.

"This parameter applies to all watchdog drivers.". The rest is implementation
detail and irrelevant here.

> +
> +	  Note: to use only for watchdog drivers which doesn't support this
> +	        parameter by itself.
> +

This comment is quite useless in the Kconfig description. If enabled, it is enabled,
period.

>  config WATCHDOG_HANDLE_BOOT_ENABLED
>  	bool "Update boot-enabled watchdog until userspace takes over"
>  	default y
> diff --git a/drivers/watchdog/watchdog_core.c b/drivers/watchdog/watchdog_core.c
> index 5df0a22e2cb4..5052ae355219 100644
> --- a/drivers/watchdog/watchdog_core.c
> +++ b/drivers/watchdog/watchdog_core.c
> @@ -43,6 +43,11 @@ static int stop_on_reboot = -1;
>  module_param(stop_on_reboot, int, 0444);
>  MODULE_PARM_DESC(stop_on_reboot, "Stop watchdogs on reboot (0=keep watching, 1=stop)");
>  
> +static bool start_enabled = IS_ENABLED(CONFIG_WATCHDOG_START_ENABLED);
> +module_param(start_enabled, bool, 0444);
> +MODULE_PARM_DESC(start_enabled, "Start watchdog on module insertion (default="
> +	__MODULE_STRING(IS_ENABLED(CONFIG_WATCHDOG_START_ENABLED)) ")");
> +
>  /*
>   * Deferred Registration infrastructure.
>   *
> @@ -224,6 +229,13 @@ static int __watchdog_register_device(struct watchdog_device *wdd)
>  	 * corrupted in a later stage then we expect a kernel panic!
>  	 */
>  
> +	/* If required, start the watchdog immediately */
> +	if (start_enabled) {
> +		set_bit(WDOG_HW_RUNNING, &wdd->status);
> +		wdd->ops->start(wdd);
> +		pr_info("Watchdog enabled\n");
> +	}
> +
>  	/* Use alias for watchdog id if possible */
>  	if (wdd->parent) {
>  		ret = of_alias_get_id(wdd->parent->of_node, "watchdog");
> 


^ permalink raw reply	[flat|nested] 5+ messages in thread

* RE: [PATCH v1] watchdog: add new parameter to start the watchdog on module insertion
  2021-04-09 15:56 ` Guenter Roeck
@ 2021-04-12  8:29   ` Flavio Suligoi
  2021-04-12 14:42     ` Guenter Roeck
  0 siblings, 1 reply; 5+ messages in thread
From: Flavio Suligoi @ 2021-04-12  8:29 UTC (permalink / raw)
  To: Guenter Roeck
  Cc: Mika Westerberg, linux-watchdog, linux-kernel, Wim Van Sebroeck

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@asem.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.

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?

> 
> >
> >  -------------------------------------------------
> >
> > diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
> > index 0470dc15c085..c2a668d6bbbc 100644
> > --- a/drivers/watchdog/Kconfig
> > +++ b/drivers/watchdog/Kconfig
> > @@ -47,6 +47,20 @@ config WATCHDOG_NOWAYOUT
> >  	  get killed. If you say Y here, the watchdog cannot be stopped once
> >  	  it has been started.
> >
> > +config WATCHDOG_START_ENABLED
> > +	bool "Start watchdog on module insertion"
> > +	help
> > +	  Say Y if you want to start the watchdog at the same time when the
> > +	  driver is loaded.
> > +	  This feature is very useful in embedded systems, to avoid cases where
> > +	  the system could hang before reaching userspace.
> > +	  This parameter involves the "core" section of the watchdog driver,
> > +	  in this way it is common for all the watchdog hardware
> > +	  implementations.
> 
> "This parameter applies to all watchdog drivers.". The rest is implementation
> detail and irrelevant here.

Ok

> 
> > +
> > +	  Note: to use only for watchdog drivers which doesn't support this
> > +	        parameter by itself.
> > +
> 
> This comment is quite useless in the Kconfig description. If enabled, it is enabled,
> period.

Ok, I'll remove this comment.

> 
> >  config WATCHDOG_HANDLE_BOOT_ENABLED
> >  	bool "Update boot-enabled watchdog until userspace takes over"
> >  	default y
> > diff --git a/drivers/watchdog/watchdog_core.c
> b/drivers/watchdog/watchdog_core.c
> > index 5df0a22e2cb4..5052ae355219 100644
> > --- a/drivers/watchdog/watchdog_core.c
> > +++ b/drivers/watchdog/watchdog_core.c
> > @@ -43,6 +43,11 @@ static int stop_on_reboot = -1;
> >  module_param(stop_on_reboot, int, 0444);
> >  MODULE_PARM_DESC(stop_on_reboot, "Stop watchdogs on reboot (0=keep
> watching, 1=stop)");
> >
> > +static bool start_enabled =
> IS_ENABLED(CONFIG_WATCHDOG_START_ENABLED);
> > +module_param(start_enabled, bool, 0444);
> > +MODULE_PARM_DESC(start_enabled, "Start watchdog on module insertion
> (default="
> > +
> 	__MODULE_STRING(IS_ENABLED(CONFIG_WATCHDOG_START_ENABL
> ED)) ")");
> > +
> >  /*
> >   * Deferred Registration infrastructure.
> >   *
> > @@ -224,6 +229,13 @@ static int __watchdog_register_device(struct
> watchdog_device *wdd)
> >  	 * corrupted in a later stage then we expect a kernel panic!
> >  	 */
> >
> > +	/* If required, start the watchdog immediately */
> > +	if (start_enabled) {
> > +		set_bit(WDOG_HW_RUNNING, &wdd->status);
> > +		wdd->ops->start(wdd);
> > +		pr_info("Watchdog enabled\n");
> > +	}
> > +
> >  	/* Use alias for watchdog id if possible */
> >  	if (wdd->parent) {
> >  		ret = of_alias_get_id(wdd->parent->of_node, "watchdog");
> >

Best regards,
Flavio Suligoi


^ permalink raw reply	[flat|nested] 5+ messages in thread

* Re: [PATCH v1] watchdog: add new parameter to start the watchdog on module insertion
  2021-04-12  8:29   ` Flavio Suligoi
@ 2021-04-12 14:42     ` Guenter Roeck
  2021-04-13  9:09       ` Flavio Suligoi
  0 siblings, 1 reply; 5+ messages in thread
From: Guenter Roeck @ 2021-04-12 14:42 UTC (permalink / raw)
  To: Flavio Suligoi
  Cc: Mika Westerberg, linux-watchdog, linux-kernel, Wim Van Sebroeck

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@asem.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

^ permalink raw reply	[flat|nested] 5+ messages in thread

* RE: [PATCH v1] watchdog: add new parameter to start the watchdog on module insertion
  2021-04-12 14:42     ` Guenter Roeck
@ 2021-04-13  9:09       ` Flavio Suligoi
  0 siblings, 0 replies; 5+ messages in thread
From: Flavio Suligoi @ 2021-04-13  9:09 UTC (permalink / raw)
  To: Guenter Roeck
  Cc: Mika Westerberg, linux-watchdog, linux-kernel, Wim Van Sebroeck

Hi Guenter,

...

> >>> 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@asem.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 ?

right, it is a nonsense ...

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

Yes, right! I have to check WDOG_HW_RUNNING before starting the watchdog.
It is the correct way to manage this feature, in any case and for any wdog driver.

> 
> Thanks,
> Guenter

Thank you very much Guenter!
Regards,
Flavio

^ permalink raw reply	[flat|nested] 5+ messages in thread

end of thread, other threads:[~2021-04-13  9:09 UTC | newest]

Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-04-09  9:34 [PATCH v1] watchdog: add new parameter to start the watchdog on module insertion Flavio Suligoi
2021-04-09 15:56 ` Guenter Roeck
2021-04-12  8:29   ` Flavio Suligoi
2021-04-12 14:42     ` Guenter Roeck
2021-04-13  9:09       ` Flavio Suligoi

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).