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 feature can be enabled in the kernel config, so it can be also
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.
Signed-off-by: Flavio Suligoi <[email protected]>
---
v4: - improve code legibility in returned error checking
- stop the watchdog if driver registration fails
v3: - add check for the returned error of wdog start function
v2: - check WDOG_HW_RUNNING before starting watchdog;
- remove useless comments in commit text, watchdog-parameters.rst and
Kconfig;
v1: - first version;
Documentation/watchdog/watchdog-parameters.rst | 3 +++
drivers/watchdog/Kconfig | 9 +++++++++
drivers/watchdog/watchdog_core.c | 17 +++++++++++++++++
3 files changed, 29 insertions(+)
diff --git a/Documentation/watchdog/watchdog-parameters.rst b/Documentation/watchdog/watchdog-parameters.rst
index 223c99361a30..7780d0c1fb4a 100644
--- a/Documentation/watchdog/watchdog-parameters.rst
+++ b/Documentation/watchdog/watchdog-parameters.rst
@@ -21,6 +21,9 @@ 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).
-------------------------------------------------
diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
index b81fe4f7d434..19be709e01ac 100644
--- a/drivers/watchdog/Kconfig
+++ b/drivers/watchdog/Kconfig
@@ -47,6 +47,15 @@ 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 applies to all watchdog drivers.
+
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 3fe8a7edc252..d9211fea45d7 100644
--- a/drivers/watchdog/watchdog_core.c
+++ b/drivers/watchdog/watchdog_core.c
@@ -44,6 +44,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.
*
@@ -252,6 +257,15 @@ 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 && !watchdog_hw_running(wdd)) {
+ set_bit(WDOG_HW_RUNNING, &wdd->status);
+ ret = wdd->ops->start(wdd);
+ if (ret)
+ return ret;
+ 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");
@@ -356,6 +370,9 @@ int watchdog_register_device(struct watchdog_device *wdd)
mutex_unlock(&wtd_deferred_reg_mutex);
if (ret) {
+ if (start_enabled && watchdog_hw_running(wdd) &&
+ wdd->ops->stop)
+ wdd->ops->stop(wdd);
dev_str = wdd->parent ? dev_name(wdd->parent) :
(const char *)wdd->info->identity;
pr_err("%s: failed to register watchdog device (err = %d)\n",
--
2.25.1
On Fri, Sep 24, 2021 at 03:35:09PM +0200, 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 feature can be enabled in the kernel config, so it can be also
> 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.
>
> Signed-off-by: Flavio Suligoi <[email protected]>
> ---
>
> v4: - improve code legibility in returned error checking
> - stop the watchdog if driver registration fails
> v3: - add check for the returned error of wdog start function
> v2: - check WDOG_HW_RUNNING before starting watchdog;
> - remove useless comments in commit text, watchdog-parameters.rst and
> Kconfig;
> v1: - first version;
>
> Documentation/watchdog/watchdog-parameters.rst | 3 +++
> drivers/watchdog/Kconfig | 9 +++++++++
> drivers/watchdog/watchdog_core.c | 17 +++++++++++++++++
> 3 files changed, 29 insertions(+)
>
> diff --git a/Documentation/watchdog/watchdog-parameters.rst b/Documentation/watchdog/watchdog-parameters.rst
> index 223c99361a30..7780d0c1fb4a 100644
> --- a/Documentation/watchdog/watchdog-parameters.rst
> +++ b/Documentation/watchdog/watchdog-parameters.rst
> @@ -21,6 +21,9 @@ 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).
>
> -------------------------------------------------
>
> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
> index b81fe4f7d434..19be709e01ac 100644
> --- a/drivers/watchdog/Kconfig
> +++ b/drivers/watchdog/Kconfig
> @@ -47,6 +47,15 @@ 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 applies to all watchdog drivers.
> +
> 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 3fe8a7edc252..d9211fea45d7 100644
> --- a/drivers/watchdog/watchdog_core.c
> +++ b/drivers/watchdog/watchdog_core.c
> @@ -44,6 +44,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.
> *
> @@ -252,6 +257,15 @@ 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 && !watchdog_hw_running(wdd)) {
> + set_bit(WDOG_HW_RUNNING, &wdd->status);
> + ret = wdd->ops->start(wdd);
> + if (ret)
> + return ret;
> + pr_info("Watchdog enabled\n");
> + }
> +
I am not convinced that this is the best location to start the watchdog.
Maybe that should be done in watchdog_cdev_register().
> /* Use alias for watchdog id if possible */
> if (wdd->parent) {
> ret = of_alias_get_id(wdd->parent->of_node, "watchdog");
> @@ -356,6 +370,9 @@ int watchdog_register_device(struct watchdog_device *wdd)
> mutex_unlock(&wtd_deferred_reg_mutex);
>
> if (ret) {
> + if (start_enabled && watchdog_hw_running(wdd) &&
> + wdd->ops->stop)
> + wdd->ops->stop(wdd);
This code stops the watchdog if watchdog registration failed even and
especially if it was already running when the module was inserted.
This changes semantics and is thus not aceptable.
> dev_str = wdd->parent ? dev_name(wdd->parent) :
> (const char *)wdd->info->identity;
> pr_err("%s: failed to register watchdog device (err = %d)\n",
Hi Guenter,
> -----Original Message-----
> From: Guenter Roeck <[email protected]> On Behalf Of Guenter Roeck
> Sent: luned? 11 ottobre 2021 03:26
> To: Flavio Suligoi <[email protected]>
> Cc: Wim Van Sebroeck <[email protected]>; Mika Westerberg
> <[email protected]>; [email protected]; linux-
> [email protected]
> Subject: Re: [PATCH v4] watchdog: add new parameter to start the watchdog
> on module insertion
>
> On Fri, Sep 24, 2021 at 03:35:09PM +0200, Flavio Suligoi wrote:
> > The new parameter "start_enabled" starts the watchdog at the same time
> > of the module insertion.
...
> > diff --git a/drivers/watchdog/watchdog_core.c
> b/drivers/watchdog/watchdog_core.c
> > index 3fe8a7edc252..d9211fea45d7 100644
> > --- a/drivers/watchdog/watchdog_core.c
> > +++ b/drivers/watchdog/watchdog_core.c
> > @@ -44,6 +44,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.
> > *
> > @@ -252,6 +257,15 @@ 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 && !watchdog_hw_running(wdd)) {
> > + set_bit(WDOG_HW_RUNNING, &wdd->status);
> > + ret = wdd->ops->start(wdd);
> > + if (ret)
> > + return ret;
> > + pr_info("Watchdog enabled\n");
> > + }
> > +
> I am not convinced that this is the best location to start the watchdog.
> Maybe that should be done in watchdog_cdev_register().
Ok, thanks, I'll also explore this way
>
> > /* Use alias for watchdog id if possible */
> > if (wdd->parent) {
> > ret = of_alias_get_id(wdd->parent->of_node, "watchdog");
> > @@ -356,6 +370,9 @@ int watchdog_register_device(struct
> watchdog_device *wdd)
> > mutex_unlock(&wtd_deferred_reg_mutex);
> >
> > if (ret) {
> > + if (start_enabled && watchdog_hw_running(wdd) &&
> > + wdd->ops->stop)
> > + wdd->ops->stop(wdd);
>
> This code stops the watchdog if watchdog registration failed even and
> especially if it was already running when the module was inserted.
> This changes semantics and is thus not aceptable.
Ok, right, I'll found a different solution.
>
> > dev_str = wdd->parent ? dev_name(wdd->parent) :
> > (const char *)wdd->info->identity;
> > pr_err("%s: failed to register watchdog device (err = %d)\n",
Thanks Guenter,
Flavio