linux-kernel - RE: [PATCH v3] watchdog: add new parameter to start the watchdog on module insertion

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Fri, 17 Sep 2021 15:21:44 +0000
From:   Flavio Suligoi <f.suligoi@...m.it>
To:     Guenter Roeck <linux@...ck-us.net>,
        Wim Van Sebroeck <wim@...ux-watchdog.org>
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>
Subject: RE: [PATCH v3] watchdog: add new parameter to start the watchdog on
 module insertion

Hi Guenter,

...
> > 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               | 16 ++++++++++++++++
> >   3 files changed, 28 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..cb530b097223 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,17 @@ 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 == 0) {
> > +			pr_info("Watchdog enabled\n");
> > +		} else {
> > +			return ret;
> > +		}
> 
> This should be
> 		if (ret)
> 			return ret;
> 		pr_info("Watchdog enabled\n");
>

Right, it is simpler!!! 😊

> Also, this needs an error handler to stop the watchdog if driver registration
> fails (if the driver supports stopping the watchdog).

Ok, I'll think how to do this.

> 
> Thanks,
> Guenter
> 
> > +	}
> > +
> >   	/* Use alias for watchdog id if possible */
> >   	if (wdd->parent) {
> >   		ret = of_alias_get_id(wdd->parent->of_node, "watchdog");
> >

Flavio