[PATCH v1 0/2] Watchdog Core Global Parameters

[PATCH v1 0/2] Watchdog Core Global Parameters

[Flavio Suligoi]

This patch series add a new way to consider the module parameters for the
watchdog module.

Instead of adding this kind of module parameters independently to each
driver, the best solution is declaring each feature only once,
in the watchdog core.

Additionally, I added a implementation example of this "global" parameters
using the module "wdat_wdt"

In details:

===============================
Watchdog Core Global Parameters
===============================

Information for watchdog kernel modules developers.

Introduction
============

Different watchdog modules frequently require the same type of parameters
(for example: *timeout*, *nowayout* feature, *start_enabled* to start the
watchdog on module insertion, etc.).
Instead of adding this kind of module parameters independently to each
driver, the best solution is declaring each feature only once,
in the watchdog core.

In this way, each driver can read these "global" parameters and then,
if needed, can implement them, according to the particular hw watchdog
characteristic.

Using this approach, it is possible reduce some duplicate code in the *new*
watchdog drivers and simplify the code maintenance.  Moreover, the code
will be clearer, since the same kind of parameters are often called
with different names (see Documentation/watchdog/watchdog-parameters.rst).
Obviously, for compatibility reasons, we cannot remove the already existing
parameters from the code of the various watchdog modules, but we can use
this "global" approach for the new watchdog drivers.


Global parameters declaration
==============================

The global parameters data structure is declared in
include/linux/watchdog.h, as::

	struct watchdog_global_parameters_struct {
		int timeout;
		int ioport;
		int irq;
		unsigned long features;
		/* Bit numbers for features flags */
		#define WDOG_GLOBAL_PARAM_VERBOSE	0
		#define WDOG_GLOBAL_PARAM_TEST_MODE	1
		#define WDOG_GLOBAL_PARAM_START_ENABLED	2
		#define WDOG_GLOBAL_PARAM_NOWAYOUT	3
	};

The variable "feature" is a bitwise flags container, to store boolean
features, such as:

* nowayout
* start_enable
* etc...

Other variables can be added, to store some numerical values and other data
required.

The global parameters are declared (as usual for the module parameters)
in the first part of drivers/watchdog/watchdog_core.c file.
The above global data structure is then managed by the function
*void global_parameters_init()*, in the same file.

Global parameters use
=====================

Each watchdog driver, to check if one of the global parameters is enabled,
can use the corresponding in-line function declared in
include/linux/watchdog.h.
At the moment the following functions are ready to use:

* watchdog_global_param_verbose_enabled()
* watchdog_global_param_test_mode_enabled()
* watchdog_global_param_start_enabled()
* watchdog_global_param_nowayout_enabled()



Flavio Suligoi (2):
  watchdog: add global watchdog kernel module parameters structure
  watchdog: wdat: add start_enable global parameter

 Documentation/watchdog/index.rst              |  1 +
 .../watchdog-core-global-parameters.rst       | 74 +++++++++++++++++++
 drivers/watchdog/watchdog_core.c              | 74 +++++++++++++++++++
 drivers/watchdog/wdat_wdt.c                   |  2 +
 include/linux/watchdog.h                      | 42 +++++++++++
 5 files changed, 193 insertions(+)
 create mode 100644 Documentation/watchdog/watchdog-core-global-parameters.rst

-- 
2.25.1

[PATCH v1 1/2] watchdog: add global watchdog kernel module parameters structure

[Flavio Suligoi]

Different watchdog modules frequently require the same type of parameters
(for example: timeout, nowayout feature, start wdog on module insertion,
etc.).
Instead of adding this kind of module parameters independently to each
driver, the best solution is declaring each feature only once,
in the watchdog core.

In this way, each driver can read these "global" parameters and then,
if needed, implements them, according to the particular hw watchdog
characteristic.

Using this approach, it will be possible reduce some duplicate code
in the _new_ watchdog drivers and simplify the code maintenance.
Moreover, the code will be clearer, since the same kind of parameters
are often called with different names.
Just for example, reading the doc file:

Documentation/watchdog/watchdog-parameters.rst

the "timeout" feature is called in the following ways:

- timeout
- riowd_timeout
- margin
- heartbeat
- wdt_time
- timer_margin
- tmr_margin
- soft_margin
- timeout_ms

Obviously, we cannot remove these customized parameters from the code,
for compatibility reasons, but we can use this new "global" parameters
structure for the new watchdog drivers.

This patch adds the base structure to add the global parameters, starting
with some common integer data (timeout, ioport, irq) and a bitwise
"features" flag, where we can store some boolean features (verbose,
test_mode, start_enabled, nowayout, etc.)

Signed-off-by: Flavio Suligoi <f.suligoi@asem.it>
---
 Documentation/watchdog/index.rst              |  1 +
 .../watchdog-core-global-parameters.rst       | 74 +++++++++++++++++++
 drivers/watchdog/watchdog_core.c              | 74 +++++++++++++++++++
 include/linux/watchdog.h                      | 42 +++++++++++
 4 files changed, 191 insertions(+)
 create mode 100644 Documentation/watchdog/watchdog-core-global-parameters.rst

diff --git a/Documentation/watchdog/index.rst b/Documentation/watchdog/index.rst
index c177645081d8..5f7e0e694c42 100644
--- a/Documentation/watchdog/index.rst
+++ b/Documentation/watchdog/index.rst
@@ -13,6 +13,7 @@ Linux Watchdog Support
     watchdog-api
     watchdog-kernel-api
     watchdog-parameters
+    watchdog-core-global-parameters
     watchdog-pm
     wdt
     convert_drivers_to_kernel_api
diff --git a/Documentation/watchdog/watchdog-core-global-parameters.rst b/Documentation/watchdog/watchdog-core-global-parameters.rst
new file mode 100644
index 000000000000..332fe0c6ada0
--- /dev/null
+++ b/Documentation/watchdog/watchdog-core-global-parameters.rst
@@ -0,0 +1,74 @@
+.. SPDX-License-Identifier: GPL-2.0
+
+===============================
+Watchdog Core Global Parameters
+===============================
+
+Information for watchdog kernel modules developers.
+
+Introduction
+============
+
+Different watchdog modules frequently require the same type of parameters
+(for example: *timeout*, *nowayout* feature, *start_enabled* to start the
+watchdog on module insertion, etc.).
+Instead of adding this kind of module parameters independently to each driver,
+the best solution is declaring each feature only once, in the watchdog core.
+
+In this way, each driver can read these "global" parameters and then,
+if needed, can implement them, according to the particular hw watchdog
+characteristic.
+
+Using this approach, it is possible reduce some duplicate code in the *new*
+watchdog drivers and simplify the code maintenance.  Moreover, the code will
+be clearer, since the same kind of parameters are often called with different
+names (see Documentation/watchdog/watchdog-parameters.rst).
+Obviously, for compatibility reasons, we cannot remove the already existing
+parameters from the code of the various watchdog modules, but we can use this
+"global" approach for the new watchdog drivers.
+
+
+Global parameters declaration
+==============================
+
+The global parameters data structure is declared in include/linux/watchdog.h,
+as::
+
+	struct watchdog_global_parameters_struct {
+		int timeout;
+		int ioport;
+		int irq;
+		unsigned long features;
+		/* Bit numbers for features flags */
+		#define WDOG_GLOBAL_PARAM_VERBOSE	0
+		#define WDOG_GLOBAL_PARAM_TEST_MODE	1
+		#define WDOG_GLOBAL_PARAM_START_ENABLED	2
+		#define WDOG_GLOBAL_PARAM_NOWAYOUT	3
+	};
+
+The variable "feature" is a bitwise flags container, to store boolean
+features, such as:
+
+* nowayout
+* start_enable
+* etc...
+
+Other variables can be added, to store some numerical values and other data
+required.
+
+The global parameters are declared (as usual for the module parameters) in the
+first part of drivers/watchdog/watchdog_core.c file.
+The above global data structure is then managed by the function
+*void global_parameters_init()*, in the same file.
+
+Global parameters use
+=====================
+
+Each watchdog driver, to check if one of the global parameters is enabled, can
+use the corresponding in-line function declared in include/linux/watchdog.h.
+At the moment the following functions are ready to use:
+
+* watchdog_global_param_verbose_enabled()
+* watchdog_global_param_test_mode_enabled()
+* watchdog_global_param_start_enabled()
+* watchdog_global_param_nowayout_enabled()
diff --git a/drivers/watchdog/watchdog_core.c b/drivers/watchdog/watchdog_core.c
index 5df0a22e2cb4..fd710be22390 100644
--- a/drivers/watchdog/watchdog_core.c
+++ b/drivers/watchdog/watchdog_core.c
@@ -43,6 +43,78 @@ 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)");
 
+/* verbose - Global parameter */
+static bool glob_param_verbose;
+module_param_named(verbose, glob_param_verbose, bool, 0);
+MODULE_PARM_DESC(verbose, "Add verbosity/debug messages");
+
+/* test_mode - Global parameter */
+static bool glob_param_test_mode;
+module_param_named(test_mode, glob_param_test_mode, bool, 0);
+MODULE_PARM_DESC(test_mode, "Watchdog testmode (1 = no reboot), default=0");
+
+/* start_enable - Global parameter */
+static bool glob_param_start_enabled;
+module_param_named(start_enabled, glob_param_start_enabled, bool, 0);
+MODULE_PARM_DESC(start_enabled, "Watchdog is started on module insertion "
+		"(default=0)");
+
+/* nowayout - Global parameter */
+static bool glob_param_nowayout = WATCHDOG_NOWAYOUT;
+module_param_named(nowayout, glob_param_nowayout, bool, 0);
+MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once started "
+		"(default=" __MODULE_STRING(WATCHDOG_NOWAYOUT) ")");
+/*
+ * Watchdog "global" kernel parameters, common for all wdog drivers.
+ *
+ * Sometimes different watchdog modules need the same type of parameters
+ * (for example: timeout, nowayout feature, start wdog on module insertion,
+ * etc.).
+ * Instead of add this kind of module parameters independently to each driver,
+ * the best solution is declare each feature only once, in the watchdog core.
+ *
+ * In this way, each driver can read these "global" parameters and then,
+ * if needed, implements them, according to the particular hw watchdog
+ * characteristic.
+ */
+struct watchdog_global_parameters_struct watchdog_global_parameters;
+EXPORT_SYMBOL_GPL(watchdog_global_parameters);
+
+/*
+ * Global parameters initialization
+ *
+ * Fill the watchdog "global" kernel parameters data structure, using the
+ * above defined module parameters.
+ */
+static void global_parameters_init(void)
+{
+	watchdog_global_parameters.features = 0;
+
+	if (glob_param_verbose) {
+		set_bit(WDOG_GLOBAL_PARAM_VERBOSE,
+			&watchdog_global_parameters.features);
+		pr_info("add verbosity/debug messages\n");
+	}
+
+	if (glob_param_test_mode) {
+		set_bit(WDOG_GLOBAL_PARAM_TEST_MODE,
+			&watchdog_global_parameters.features);
+		pr_info("testmode activated\n");
+	}
+
+	if (glob_param_start_enabled) {
+		set_bit(WDOG_GLOBAL_PARAM_START_ENABLED,
+			&watchdog_global_parameters.features);
+		pr_info("watchdog is started on module insertion\n");
+	}
+
+	if (glob_param_nowayout) {
+		set_bit(WDOG_GLOBAL_PARAM_NOWAYOUT,
+			&watchdog_global_parameters.features);
+		pr_info("watchdog cannot be stopped once started\n");
+	}
+}
+
 /*
  * Deferred Registration infrastructure.
  *
@@ -421,6 +493,8 @@ static int __init watchdog_init(void)
 {
 	int err;
 
+	global_parameters_init();
+
 	err = watchdog_dev_init();
 	if (err < 0)
 		return err;
diff --git a/include/linux/watchdog.h b/include/linux/watchdog.h
index 9b19e6bb68b5..049c1f703824 100644
--- a/include/linux/watchdog.h
+++ b/include/linux/watchdog.h
@@ -119,6 +119,20 @@ struct watchdog_device {
 	struct list_head deferred;
 };
 
+struct watchdog_global_parameters_struct {
+	int timeout;
+	int ioport;
+	int irq;
+	unsigned long features;
+/* Bit numbers for features flags */
+#define WDOG_GLOBAL_PARAM_VERBOSE	0
+#define WDOG_GLOBAL_PARAM_TEST_MODE	1
+#define WDOG_GLOBAL_PARAM_START_ENABLED	2
+#define WDOG_GLOBAL_PARAM_NOWAYOUT	3
+};
+
+extern struct watchdog_global_parameters_struct watchdog_global_parameters;
+
 #define WATCHDOG_NOWAYOUT		IS_BUILTIN(CONFIG_WATCHDOG_NOWAYOUT)
 #define WATCHDOG_NOWAYOUT_INIT_STATUS	(WATCHDOG_NOWAYOUT << WDOG_NO_WAY_OUT)
 
@@ -193,6 +207,34 @@ static inline void *watchdog_get_drvdata(struct watchdog_device *wdd)
 	return wdd->driver_data;
 }
 
+/* Check if the global parameter verbose is enabled */
+static inline bool watchdog_global_param_verbose_enabled(void)
+{
+	return test_bit(WDOG_GLOBAL_PARAM_VERBOSE,
+		&watchdog_global_parameters.features);
+}
+
+/* Check if the global parameter test_mode is enabled */
+static inline bool watchdog_global_param_test_mode_enabled(void)
+{
+	return test_bit(WDOG_GLOBAL_PARAM_TEST_MODE,
+		&watchdog_global_parameters.features);
+}
+
+/* Check if the global parameter start_enabled is enabled */
+static inline bool watchdog_global_param_start_enabled(void)
+{
+	return test_bit(WDOG_GLOBAL_PARAM_START_ENABLED,
+		&watchdog_global_parameters.features);
+}
+
+/* Check if the global parameter nowayout is enabled */
+static inline bool watchdog_global_param_nowayout_enabled(void)
+{
+	return test_bit(WDOG_GLOBAL_PARAM_NOWAYOUT,
+		&watchdog_global_parameters.features);
+}
+
 /* Use the following functions to report watchdog pretimeout event */
 #if IS_ENABLED(CONFIG_WATCHDOG_PRETIMEOUT_GOV)
 void watchdog_notify_pretimeout(struct watchdog_device *wdd);
-- 
2.25.1

Re: [PATCH v1 1/2] watchdog: add global watchdog kernel module parameters structure

[Randy Dunlap]

Hi,

On 3/8/21 3:21 AM, Flavio Suligoi wrote:
> Different watchdog modules frequently require the same type of parameters
> (for example: timeout, nowayout feature, start wdog on module insertion,
> etc.).
> Instead of adding this kind of module parameters independently to each
> driver, the best solution is declaring each feature only once,
> in the watchdog core.
> 
> In this way, each driver can read these "global" parameters and then,
> if needed, implements them, according to the particular hw watchdog
> characteristic.
> 
> Using this approach, it will be possible reduce some duplicate code
> in the _new_ watchdog drivers and simplify the code maintenance.
> Moreover, the code will be clearer, since the same kind of parameters
> are often called with different names.
> Just for example, reading the doc file:
> 
> Documentation/watchdog/watchdog-parameters.rst
> 
> the "timeout" feature is called in the following ways:
> 
...

> 
> Obviously, we cannot remove these customized parameters from the code,
> for compatibility reasons, but we can use this new "global" parameters
> structure for the new watchdog drivers.
> 
> This patch adds the base structure to add the global parameters, starting
> with some common integer data (timeout, ioport, irq) and a bitwise
> "features" flag, where we can store some boolean features (verbose,
> test_mode, start_enabled, nowayout, etc.)
> 
> Signed-off-by: Flavio Suligoi <f.suligoi@asem.it>
> ---
>  Documentation/watchdog/index.rst              |  1 +
>  .../watchdog-core-global-parameters.rst       | 74 +++++++++++++++++++
>  drivers/watchdog/watchdog_core.c              | 74 +++++++++++++++++++
>  include/linux/watchdog.h                      | 42 +++++++++++
>  4 files changed, 191 insertions(+)
>  create mode 100644 Documentation/watchdog/watchdog-core-global-parameters.rst
> 

> diff --git a/Documentation/watchdog/watchdog-core-global-parameters.rst b/Documentation/watchdog/watchdog-core-global-parameters.rst
> new file mode 100644
> index 000000000000..332fe0c6ada0
> --- /dev/null
> +++ b/Documentation/watchdog/watchdog-core-global-parameters.rst
> @@ -0,0 +1,74 @@
> +.. SPDX-License-Identifier: GPL-2.0
> +
> +===============================
> +Watchdog Core Global Parameters
> +===============================
> +
> +Information for watchdog kernel modules developers.
> +
> +Introduction
> +============
> +
> +Different watchdog modules frequently require the same type of parameters
> +(for example: *timeout*, *nowayout* feature, *start_enabled* to start the
> +watchdog on module insertion, etc.).
> +Instead of adding this kind of module parameters independently to each driver,
> +the best solution is declaring each feature only once, in the watchdog core.
> +
> +In this way, each driver can read these "global" parameters and then,
> +if needed, can implement them, according to the particular hw watchdog

Please spell out "hardware" (not "hw").

> +characteristic.
> +
> +Using this approach, it is possible reduce some duplicate code in the *new*

                              possible to reduce

> +watchdog drivers and simplify the code maintenance.  Moreover, the code will
> +be clearer, since the same kind of parameters are often called with different
> +names (see Documentation/watchdog/watchdog-parameters.rst).
> +Obviously, for compatibility reasons, we cannot remove the already existing
> +parameters from the code of the various watchdog modules, but we can use this
> +"global" approach for the new watchdog drivers.
> +
> +
> +Global parameters declaration
> +==============================
> +
> +The global parameters data structure is declared in include/linux/watchdog.h,
> +as::
> +
> +	struct watchdog_global_parameters_struct {
> +		int timeout;
> +		int ioport;
> +		int irq;
> +		unsigned long features;
> +		/* Bit numbers for features flags */
> +		#define WDOG_GLOBAL_PARAM_VERBOSE	0
> +		#define WDOG_GLOBAL_PARAM_TEST_MODE	1
> +		#define WDOG_GLOBAL_PARAM_START_ENABLED	2
> +		#define WDOG_GLOBAL_PARAM_NOWAYOUT	3
> +	};
> +
> +The variable "feature" is a bitwise flags container, to store boolean
> +features, such as:
> +
> +* nowayout
> +* start_enable

     start_enabled
everywhere else.

> +* etc...
> +
> +Other variables can be added, to store some numerical values and other data
> +required.
> +
> +The global parameters are declared (as usual for the module parameters) in the
> +first part of drivers/watchdog/watchdog_core.c file.
> +The above global data structure is then managed by the function
> +*void global_parameters_init()*, in the same file.
> +
> +Global parameters use
> +=====================
> +
> +Each watchdog driver, to check if one of the global parameters is enabled, can
> +use the corresponding in-line function declared in include/linux/watchdog.h.
> +At the moment the following functions are ready to use:
> +
> +* watchdog_global_param_verbose_enabled()
> +* watchdog_global_param_test_mode_enabled()
> +* watchdog_global_param_start_enabled()
> +* watchdog_global_param_nowayout_enabled()

> diff --git a/drivers/watchdog/watchdog_core.c b/drivers/watchdog/watchdog_core.c
> index 5df0a22e2cb4..fd710be22390 100644
> --- a/drivers/watchdog/watchdog_core.c
> +++ b/drivers/watchdog/watchdog_core.c
> @@ -43,6 +43,78 @@ 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)");
>  
> +/* verbose - Global parameter */
> +static bool glob_param_verbose;
> +module_param_named(verbose, glob_param_verbose, bool, 0);
> +MODULE_PARM_DESC(verbose, "Add verbosity/debug messages");
> +
> +/* test_mode - Global parameter */
> +static bool glob_param_test_mode;
> +module_param_named(test_mode, glob_param_test_mode, bool, 0);
> +MODULE_PARM_DESC(test_mode, "Watchdog testmode (1 = no reboot), default=0");
> +
> +/* start_enable - Global parameter */
> +static bool glob_param_start_enabled;
> +module_param_named(start_enabled, glob_param_start_enabled, bool, 0);
> +MODULE_PARM_DESC(start_enabled, "Watchdog is started on module insertion "
> +		"(default=0)");
> +
> +/* nowayout - Global parameter */
> +static bool glob_param_nowayout = WATCHDOG_NOWAYOUT;
> +module_param_named(nowayout, glob_param_nowayout, bool, 0);
> +MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once started "
> +		"(default=" __MODULE_STRING(WATCHDOG_NOWAYOUT) ")");
> +/*
> + * Watchdog "global" kernel parameters, common for all wdog drivers.
> + *
> + * Sometimes different watchdog modules need the same type of parameters
> + * (for example: timeout, nowayout feature, start wdog on module insertion,
> + * etc.).
> + * Instead of add this kind of module parameters independently to each driver,

                 adding

> + * the best solution is declare each feature only once, in the watchdog core.
> + *
> + * In this way, each driver can read these "global" parameters and then,
> + * if needed, implements them, according to the particular hw watchdog

s/hw/hardware/

> + * characteristic.
> + */
> +struct watchdog_global_parameters_struct watchdog_global_parameters;
> +EXPORT_SYMBOL_GPL(watchdog_global_parameters);


If I were doing (or using) this, I would probably want 'test_mode' and
'verbosity' to be unsigned int masks instead of a bool, so that there could
be multiple types of test_mode or verbosity.
That's something that some other subsystems do, but maybe watchdog is simple
enough that it's not needed.
If it is needed, then we are back to each driver doing it its own way (until
this patch is updated).


HTH. thanks.
-- 
~Randy

RE: [PATCH v1 1/2] watchdog: add global watchdog kernel module parameters structure

[Flavio Suligoi]

> Hi,
> 

Hi Randy,

> On 3/8/21 3:21 AM, Flavio Suligoi wrote:
> > Different watchdog modules frequently require the same type of
> > parameters (for example: timeout, nowayout feature, start wdog on
> > module insertion, etc.).

...

> > +In this way, each driver can read these "global" parameters and then,
> > +if needed, can implement them, according to the particular hw
> > +watchdog
> 
> Please spell out "hardware" (not "hw").

Ok!

> 
> > +characteristic.
> > +
> > +Using this approach, it is possible reduce some duplicate code in the
> > +*new*
> 
>                               possible to reduce

Thanks!

...

> > + * Instead of add this kind of module parameters independently to
> > +each driver,
> 
>                  adding

Thanks!

...
> > + * if needed, implements them, according to the particular hw
> > + watchdog
> 
> s/hw/hardware/

Ok

...

> If I were doing (or using) this, I would probably want 'test_mode' and
> 'verbosity' to be unsigned int masks instead of a bool, so that there could be
> multiple types of test_mode or verbosity.

I used bool as already done in some watchdog drivers, but your suggestion
is better, thanks.

> That's something that some other subsystems do, but maybe watchdog is
> simple enough that it's not needed.
> If it is needed, then we are back to each driver doing it its own way (until this
> patch is updated).
> 
> 
> HTH. thanks.
> --
> ~Randy

Thanks Randy.

Regards,
Flavio

[PATCH v1 2/2] watchdog: wdat: add start_enable global parameter

[Flavio Suligoi]

The "start_enable" global parameter, managed in watchdog_core.c,
forces the driver to start the watchdog countdown in the same moment of the
module insertion.
The driver also updates the watchdog status, setting the WDOG_HW_RUNNING
flag, to enable the watchdog ping feature managed by the watchdog_core
itself.

Signed-off-by: Flavio Suligoi <f.suligoi@asem.it>
---
 drivers/watchdog/wdat_wdt.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/watchdog/wdat_wdt.c b/drivers/watchdog/wdat_wdt.c
index cec7917790e5..7304a335227f 100644
--- a/drivers/watchdog/wdat_wdt.c
+++ b/drivers/watchdog/wdat_wdt.c
@@ -437,6 +437,8 @@ static int wdat_wdt_probe(struct platform_device *pdev)
 	}
 
 	wdat_wdt_boot_status(wdat);
+	if (watchdog_global_param_start_enabled())
+		wdat_wdt_start(&wdat->wdd);
 	wdat_wdt_set_running(wdat);
 
 	ret = wdat_wdt_enable_reboot(wdat);
-- 
2.25.1

Re: [PATCH v1 0/2] Watchdog Core Global Parameters

[Guenter Roeck]

On 3/8/21 3:21 AM, Flavio Suligoi wrote:
> This patch series add a new way to consider the module parameters for the
> watchdog module.
> 
> Instead of adding this kind of module parameters independently to each
> driver, the best solution is declaring each feature only once,
> in the watchdog core.
> 

I agree to and like the idea, but I don't see the point of letting drivers
opt in or opt out. This adds a lot of complexity for little if any gain.

Guenter

> Additionally, I added a implementation example of this "global" parameters
> using the module "wdat_wdt"
> 
> In details:
> 
> ===============================
> Watchdog Core Global Parameters
> ===============================
> 
> Information for watchdog kernel modules developers.
> 
> Introduction
> ============
> 
> Different watchdog modules frequently require the same type of parameters
> (for example: *timeout*, *nowayout* feature, *start_enabled* to start the
> watchdog on module insertion, etc.).
> Instead of adding this kind of module parameters independently to each
> driver, the best solution is declaring each feature only once,
> in the watchdog core.
> 
> In this way, each driver can read these "global" parameters and then,
> if needed, can implement them, according to the particular hw watchdog
> characteristic.
> 
> Using this approach, it is possible reduce some duplicate code in the *new*
> watchdog drivers and simplify the code maintenance.  Moreover, the code
> will be clearer, since the same kind of parameters are often called
> with different names (see Documentation/watchdog/watchdog-parameters.rst).
> Obviously, for compatibility reasons, we cannot remove the already existing
> parameters from the code of the various watchdog modules, but we can use
> this "global" approach for the new watchdog drivers.
> 
> 
> Global parameters declaration
> ==============================
> 
> The global parameters data structure is declared in
> include/linux/watchdog.h, as::
> 
> 	struct watchdog_global_parameters_struct {
> 		int timeout;
> 		int ioport;
> 		int irq;
> 		unsigned long features;
> 		/* Bit numbers for features flags */
> 		#define WDOG_GLOBAL_PARAM_VERBOSE	0
> 		#define WDOG_GLOBAL_PARAM_TEST_MODE	1
> 		#define WDOG_GLOBAL_PARAM_START_ENABLED	2
> 		#define WDOG_GLOBAL_PARAM_NOWAYOUT	3
> 	};
> 
> The variable "feature" is a bitwise flags container, to store boolean
> features, such as:
> 
> * nowayout
> * start_enable
> * etc...
> 
> Other variables can be added, to store some numerical values and other data
> required.
> 
> The global parameters are declared (as usual for the module parameters)
> in the first part of drivers/watchdog/watchdog_core.c file.
> The above global data structure is then managed by the function
> *void global_parameters_init()*, in the same file.
> 
> Global parameters use
> =====================
> 
> Each watchdog driver, to check if one of the global parameters is enabled,
> can use the corresponding in-line function declared in
> include/linux/watchdog.h.
> At the moment the following functions are ready to use:
> 
> * watchdog_global_param_verbose_enabled()
> * watchdog_global_param_test_mode_enabled()
> * watchdog_global_param_start_enabled()
> * watchdog_global_param_nowayout_enabled()
> 
> 
> 
> Flavio Suligoi (2):
>   watchdog: add global watchdog kernel module parameters structure
>   watchdog: wdat: add start_enable global parameter
> 
>  Documentation/watchdog/index.rst              |  1 +
>  .../watchdog-core-global-parameters.rst       | 74 +++++++++++++++++++
>  drivers/watchdog/watchdog_core.c              | 74 +++++++++++++++++++
>  drivers/watchdog/wdat_wdt.c                   |  2 +
>  include/linux/watchdog.h                      | 42 +++++++++++
>  5 files changed, 193 insertions(+)
>  create mode 100644 Documentation/watchdog/watchdog-core-global-parameters.rst
> 

RE: [PATCH v1 0/2] Watchdog Core Global Parameters

[Flavio Suligoi]

Hi Guenter,

> > Instead of adding this kind of module parameters independently to each
> > driver, the best solution is declaring each feature only once, in the
> > watchdog core.
> >
> 
> I agree to and like the idea, but I don't see the point of letting drivers opt in
> or opt out. This adds a lot of complexity for little if any gain.

Do you mean that all the support for this "global parameters" should be done
in the watchdog-core only, without write any code in each single
"hardware" driver?
> 
> Guenter

Regards,

Flavio

> 
> > Additionally, I added a implementation example of this "global"
> > parameters using the module "wdat_wdt"
> >
> > In details:
> >
> > ===============================
> > Watchdog Core Global Parameters
> > ===============================
> >
> > Information for watchdog kernel modules developers.
> >
> > Introduction
> > ============
> >
> > Different watchdog modules frequently require the same type of
> > parameters (for example: *timeout*, *nowayout* feature,
> > *start_enabled* to start the watchdog on module insertion, etc.).
> > Instead of adding this kind of module parameters independently to each
> > driver, the best solution is declaring each feature only once, in the
> > watchdog core.
> >
> > In this way, each driver can read these "global" parameters and then,
> > if needed, can implement them, according to the particular hw watchdog
> > characteristic.
> >
> > Using this approach, it is possible reduce some duplicate code in the
> > *new* watchdog drivers and simplify the code maintenance.  Moreover,
> > the code will be clearer, since the same kind of parameters are often
> > called with different names (see Documentation/watchdog/watchdog-
> parameters.rst).
> > Obviously, for compatibility reasons, we cannot remove the already
> > existing parameters from the code of the various watchdog modules, but
> > we can use this "global" approach for the new watchdog drivers.
> >
> >
> > Global parameters declaration
> > ==============================
> >
> > The global parameters data structure is declared in
> > include/linux/watchdog.h, as::
> >
> > 	struct watchdog_global_parameters_struct {
> > 		int timeout;
> > 		int ioport;
> > 		int irq;
> > 		unsigned long features;
> > 		/* Bit numbers for features flags */
> > 		#define WDOG_GLOBAL_PARAM_VERBOSE	0
> > 		#define WDOG_GLOBAL_PARAM_TEST_MODE	1
> > 		#define WDOG_GLOBAL_PARAM_START_ENABLED	2
> > 		#define WDOG_GLOBAL_PARAM_NOWAYOUT	3
> > 	};
> >
> > The variable "feature" is a bitwise flags container, to store boolean
> > features, such as:
> >
> > * nowayout
> > * start_enable
> > * etc...
> >
> > Other variables can be added, to store some numerical values and other
> > data required.
> >
> > The global parameters are declared (as usual for the module
> > parameters) in the first part of drivers/watchdog/watchdog_core.c file.
> > The above global data structure is then managed by the function *void
> > global_parameters_init()*, in the same file.
> >
> > Global parameters use
> > =====================
> >
> > Each watchdog driver, to check if one of the global parameters is
> > enabled, can use the corresponding in-line function declared in
> > include/linux/watchdog.h.
> > At the moment the following functions are ready to use:
> >
> > * watchdog_global_param_verbose_enabled()
> > * watchdog_global_param_test_mode_enabled()
> > * watchdog_global_param_start_enabled()
> > * watchdog_global_param_nowayout_enabled()
> >
> >
> >
> > Flavio Suligoi (2):
> >   watchdog: add global watchdog kernel module parameters structure
> >   watchdog: wdat: add start_enable global parameter
> >
> >  Documentation/watchdog/index.rst              |  1 +
> >  .../watchdog-core-global-parameters.rst       | 74 +++++++++++++++++++
> >  drivers/watchdog/watchdog_core.c              | 74 +++++++++++++++++++
> >  drivers/watchdog/wdat_wdt.c                   |  2 +
> >  include/linux/watchdog.h                      | 42 +++++++++++
> >  5 files changed, 193 insertions(+)
> >  create mode 100644
> > Documentation/watchdog/watchdog-core-global-parameters.rst
> >

Re: [PATCH v1 0/2] Watchdog Core Global Parameters

[Guenter Roeck]

On 3/9/21 2:26 AM, Flavio Suligoi wrote:
> Hi Guenter,
> 
>>> Instead of adding this kind of module parameters independently to each
>>> driver, the best solution is declaring each feature only once, in the
>>> watchdog core.
>>>
>>
>> I agree to and like the idea, but I don't see the point of letting drivers opt in
>> or opt out. This adds a lot of complexity for little if any gain.
> 
> Do you mean that all the support for this "global parameters" should be done
> in the watchdog-core only, without write any code in each single
> "hardware" driver?

Correct. It should not be up to the driver author to decide if they
want to opt out from global parameters or not. It should be up to
users, and users can opt out by not providing the parameters.

Guenter

>>
>> Guenter
> 
> Regards,
> 
> Flavio
> 
>>
>>> Additionally, I added a implementation example of this "global"
>>> parameters using the module "wdat_wdt"
>>>
>>> In details:
>>>
>>> ===============================
>>> Watchdog Core Global Parameters
>>> ===============================
>>>
>>> Information for watchdog kernel modules developers.
>>>
>>> Introduction
>>> ============
>>>
>>> Different watchdog modules frequently require the same type of
>>> parameters (for example: *timeout*, *nowayout* feature,
>>> *start_enabled* to start the watchdog on module insertion, etc.).
>>> Instead of adding this kind of module parameters independently to each
>>> driver, the best solution is declaring each feature only once, in the
>>> watchdog core.
>>>
>>> In this way, each driver can read these "global" parameters and then,
>>> if needed, can implement them, according to the particular hw watchdog
>>> characteristic.
>>>
>>> Using this approach, it is possible reduce some duplicate code in the
>>> *new* watchdog drivers and simplify the code maintenance.  Moreover,
>>> the code will be clearer, since the same kind of parameters are often
>>> called with different names (see Documentation/watchdog/watchdog-
>> parameters.rst).
>>> Obviously, for compatibility reasons, we cannot remove the already
>>> existing parameters from the code of the various watchdog modules, but
>>> we can use this "global" approach for the new watchdog drivers.
>>>
>>>
>>> Global parameters declaration
>>> ==============================
>>>
>>> The global parameters data structure is declared in
>>> include/linux/watchdog.h, as::
>>>
>>> 	struct watchdog_global_parameters_struct {
>>> 		int timeout;
>>> 		int ioport;
>>> 		int irq;
>>> 		unsigned long features;
>>> 		/* Bit numbers for features flags */
>>> 		#define WDOG_GLOBAL_PARAM_VERBOSE	0
>>> 		#define WDOG_GLOBAL_PARAM_TEST_MODE	1
>>> 		#define WDOG_GLOBAL_PARAM_START_ENABLED	2
>>> 		#define WDOG_GLOBAL_PARAM_NOWAYOUT	3
>>> 	};
>>>
>>> The variable "feature" is a bitwise flags container, to store boolean
>>> features, such as:
>>>
>>> * nowayout
>>> * start_enable
>>> * etc...
>>>
>>> Other variables can be added, to store some numerical values and other
>>> data required.
>>>
>>> The global parameters are declared (as usual for the module
>>> parameters) in the first part of drivers/watchdog/watchdog_core.c file.
>>> The above global data structure is then managed by the function *void
>>> global_parameters_init()*, in the same file.
>>>
>>> Global parameters use
>>> =====================
>>>
>>> Each watchdog driver, to check if one of the global parameters is
>>> enabled, can use the corresponding in-line function declared in
>>> include/linux/watchdog.h.
>>> At the moment the following functions are ready to use:
>>>
>>> * watchdog_global_param_verbose_enabled()
>>> * watchdog_global_param_test_mode_enabled()
>>> * watchdog_global_param_start_enabled()
>>> * watchdog_global_param_nowayout_enabled()
>>>
>>>
>>>
>>> Flavio Suligoi (2):
>>>   watchdog: add global watchdog kernel module parameters structure
>>>   watchdog: wdat: add start_enable global parameter
>>>
>>>  Documentation/watchdog/index.rst              |  1 +
>>>  .../watchdog-core-global-parameters.rst       | 74 +++++++++++++++++++
>>>  drivers/watchdog/watchdog_core.c              | 74 +++++++++++++++++++
>>>  drivers/watchdog/wdat_wdt.c                   |  2 +
>>>  include/linux/watchdog.h                      | 42 +++++++++++
>>>  5 files changed, 193 insertions(+)
>>>  create mode 100644
>>> Documentation/watchdog/watchdog-core-global-parameters.rst
>>>
> 

Re: [PATCH v1 0/2] Watchdog Core Global Parameters

[Jerry Hoemann]

On Tue, Mar 09, 2021 at 07:22:28AM -0800, Guenter Roeck wrote:
> On 3/9/21 2:26 AM, Flavio Suligoi wrote:
> > Hi Guenter,
> > 
> >>> Instead of adding this kind of module parameters independently to each
> >>> driver, the best solution is declaring each feature only once, in the
> >>> watchdog core.
> >>>
> >>
> >> I agree to and like the idea, but I don't see the point of letting drivers opt in
> >> or opt out. This adds a lot of complexity for little if any gain.
> > 
> > Do you mean that all the support for this "global parameters" should be done
> > in the watchdog-core only, without write any code in each single
> > "hardware" driver?
> 
> Correct. It should not be up to the driver author to decide if they
> want to opt out from global parameters or not. It should be up to
> users, and users can opt out by not providing the parameters.


Guenter,

What about parameters like "pretimeout"  that only some WD drivers have
hw to support?

Might be nice to centralize these parameters as well, but leaving it up
to users to decide might not make sense.

Or do you see the recent work to allow for a software pretimeout
mechanism covering this?

thanks

Jerry

> 
> Guenter
> 
> >>
> >> Guenter
> > 
> > Regards,
> > 
> > Flavio
> > 
> >>
> >>> Additionally, I added a implementation example of this "global"
> >>> parameters using the module "wdat_wdt"
> >>>
> >>> In details:
> >>>
> >>> ===============================
> >>> Watchdog Core Global Parameters
> >>> ===============================
> >>>
> >>> Information for watchdog kernel modules developers.
> >>>
> >>> Introduction
> >>> ============
> >>>
> >>> Different watchdog modules frequently require the same type of
> >>> parameters (for example: *timeout*, *nowayout* feature,
> >>> *start_enabled* to start the watchdog on module insertion, etc.).
> >>> Instead of adding this kind of module parameters independently to each
> >>> driver, the best solution is declaring each feature only once, in the
> >>> watchdog core.
> >>>
> >>> In this way, each driver can read these "global" parameters and then,
> >>> if needed, can implement them, according to the particular hw watchdog
> >>> characteristic.
> >>>
> >>> Using this approach, it is possible reduce some duplicate code in the
> >>> *new* watchdog drivers and simplify the code maintenance.  Moreover,
> >>> the code will be clearer, since the same kind of parameters are often
> >>> called with different names (see Documentation/watchdog/watchdog-
> >> parameters.rst).
> >>> Obviously, for compatibility reasons, we cannot remove the already
> >>> existing parameters from the code of the various watchdog modules, but
> >>> we can use this "global" approach for the new watchdog drivers.
> >>>
> >>>
> >>> Global parameters declaration
> >>> ==============================
> >>>
> >>> The global parameters data structure is declared in
> >>> include/linux/watchdog.h, as::
> >>>
> >>> 	struct watchdog_global_parameters_struct {
> >>> 		int timeout;
> >>> 		int ioport;
> >>> 		int irq;
> >>> 		unsigned long features;
> >>> 		/* Bit numbers for features flags */
> >>> 		#define WDOG_GLOBAL_PARAM_VERBOSE	0
> >>> 		#define WDOG_GLOBAL_PARAM_TEST_MODE	1
> >>> 		#define WDOG_GLOBAL_PARAM_START_ENABLED	2
> >>> 		#define WDOG_GLOBAL_PARAM_NOWAYOUT	3
> >>> 	};
> >>>
> >>> The variable "feature" is a bitwise flags container, to store boolean
> >>> features, such as:
> >>>
> >>> * nowayout
> >>> * start_enable
> >>> * etc...
> >>>
> >>> Other variables can be added, to store some numerical values and other
> >>> data required.
> >>>
> >>> The global parameters are declared (as usual for the module
> >>> parameters) in the first part of drivers/watchdog/watchdog_core.c file.
> >>> The above global data structure is then managed by the function *void
> >>> global_parameters_init()*, in the same file.
> >>>
> >>> Global parameters use
> >>> =====================
> >>>
> >>> Each watchdog driver, to check if one of the global parameters is
> >>> enabled, can use the corresponding in-line function declared in
> >>> include/linux/watchdog.h.
> >>> At the moment the following functions are ready to use:
> >>>
> >>> * watchdog_global_param_verbose_enabled()
> >>> * watchdog_global_param_test_mode_enabled()
> >>> * watchdog_global_param_start_enabled()
> >>> * watchdog_global_param_nowayout_enabled()
> >>>
> >>>
> >>>
> >>> Flavio Suligoi (2):
> >>>   watchdog: add global watchdog kernel module parameters structure
> >>>   watchdog: wdat: add start_enable global parameter
> >>>
> >>>  Documentation/watchdog/index.rst              |  1 +
> >>>  .../watchdog-core-global-parameters.rst       | 74 +++++++++++++++++++
> >>>  drivers/watchdog/watchdog_core.c              | 74 +++++++++++++++++++
> >>>  drivers/watchdog/wdat_wdt.c                   |  2 +
> >>>  include/linux/watchdog.h                      | 42 +++++++++++
> >>>  5 files changed, 193 insertions(+)
> >>>  create mode 100644
> >>> Documentation/watchdog/watchdog-core-global-parameters.rst
> >>>
> > 

-- 

-----------------------------------------------------------------------------
Jerry Hoemann                  Software Engineer   Hewlett Packard Enterprise
-----------------------------------------------------------------------------

Re: [PATCH v1 0/2] Watchdog Core Global Parameters

[Guenter Roeck]

On 3/9/21 10:42 AM, Jerry Hoemann wrote:
> On Tue, Mar 09, 2021 at 07:22:28AM -0800, Guenter Roeck wrote:
>> On 3/9/21 2:26 AM, Flavio Suligoi wrote:
>>> Hi Guenter,
>>>
>>>>> Instead of adding this kind of module parameters independently to each
>>>>> driver, the best solution is declaring each feature only once, in the
>>>>> watchdog core.
>>>>>
>>>>
>>>> I agree to and like the idea, but I don't see the point of letting drivers opt in
>>>> or opt out. This adds a lot of complexity for little if any gain.
>>>
>>> Do you mean that all the support for this "global parameters" should be done
>>> in the watchdog-core only, without write any code in each single
>>> "hardware" driver?
>>
>> Correct. It should not be up to the driver author to decide if they
>> want to opt out from global parameters or not. It should be up to
>> users, and users can opt out by not providing the parameters.
> 
> 
> Guenter,
> 
> What about parameters like "pretimeout"  that only some WD drivers have
> hw to support?
> 

Those drivers are supposed to set WDIOF_PRETIMEOUT. If they don't, any associated
module parameter would be ignored. That is quite similar to any other non-existing
module parameter.

> Might be nice to centralize these parameters as well, but leaving it up
> to users to decide might not make sense.
> 

Decide what, exactly ? Users can still provide a pretimeout module
parameter even if pretimeout is not supported for a given watchdog.
That is the case today, and it won't change.

Given that, I must admit that I don't really understand your concern.

> Or do you see the recent work to allow for a software pretimeout
> mechanism covering this?
>

That is completely orthogonal.

Guenter

> thanks
> 
> Jerry
> 
>>
>> Guenter
>>
>>>>
>>>> Guenter
>>>
>>> Regards,
>>>
>>> Flavio
>>>
>>>>
>>>>> Additionally, I added a implementation example of this "global"
>>>>> parameters using the module "wdat_wdt"
>>>>>
>>>>> In details:
>>>>>
>>>>> ===============================
>>>>> Watchdog Core Global Parameters
>>>>> ===============================
>>>>>
>>>>> Information for watchdog kernel modules developers.
>>>>>
>>>>> Introduction
>>>>> ============
>>>>>
>>>>> Different watchdog modules frequently require the same type of
>>>>> parameters (for example: *timeout*, *nowayout* feature,
>>>>> *start_enabled* to start the watchdog on module insertion, etc.).
>>>>> Instead of adding this kind of module parameters independently to each
>>>>> driver, the best solution is declaring each feature only once, in the
>>>>> watchdog core.
>>>>>
>>>>> In this way, each driver can read these "global" parameters and then,
>>>>> if needed, can implement them, according to the particular hw watchdog
>>>>> characteristic.
>>>>>
>>>>> Using this approach, it is possible reduce some duplicate code in the
>>>>> *new* watchdog drivers and simplify the code maintenance.  Moreover,
>>>>> the code will be clearer, since the same kind of parameters are often
>>>>> called with different names (see Documentation/watchdog/watchdog-
>>>> parameters.rst).
>>>>> Obviously, for compatibility reasons, we cannot remove the already
>>>>> existing parameters from the code of the various watchdog modules, but
>>>>> we can use this "global" approach for the new watchdog drivers.
>>>>>
>>>>>
>>>>> Global parameters declaration
>>>>> ==============================
>>>>>
>>>>> The global parameters data structure is declared in
>>>>> include/linux/watchdog.h, as::
>>>>>
>>>>> 	struct watchdog_global_parameters_struct {
>>>>> 		int timeout;
>>>>> 		int ioport;
>>>>> 		int irq;
>>>>> 		unsigned long features;
>>>>> 		/* Bit numbers for features flags */
>>>>> 		#define WDOG_GLOBAL_PARAM_VERBOSE	0
>>>>> 		#define WDOG_GLOBAL_PARAM_TEST_MODE	1
>>>>> 		#define WDOG_GLOBAL_PARAM_START_ENABLED	2
>>>>> 		#define WDOG_GLOBAL_PARAM_NOWAYOUT	3
>>>>> 	};
>>>>>
>>>>> The variable "feature" is a bitwise flags container, to store boolean
>>>>> features, such as:
>>>>>
>>>>> * nowayout
>>>>> * start_enable
>>>>> * etc...
>>>>>
>>>>> Other variables can be added, to store some numerical values and other
>>>>> data required.
>>>>>
>>>>> The global parameters are declared (as usual for the module
>>>>> parameters) in the first part of drivers/watchdog/watchdog_core.c file.
>>>>> The above global data structure is then managed by the function *void
>>>>> global_parameters_init()*, in the same file.
>>>>>
>>>>> Global parameters use
>>>>> =====================
>>>>>
>>>>> Each watchdog driver, to check if one of the global parameters is
>>>>> enabled, can use the corresponding in-line function declared in
>>>>> include/linux/watchdog.h.
>>>>> At the moment the following functions are ready to use:
>>>>>
>>>>> * watchdog_global_param_verbose_enabled()
>>>>> * watchdog_global_param_test_mode_enabled()
>>>>> * watchdog_global_param_start_enabled()
>>>>> * watchdog_global_param_nowayout_enabled()
>>>>>
>>>>>
>>>>>
>>>>> Flavio Suligoi (2):
>>>>>   watchdog: add global watchdog kernel module parameters structure
>>>>>   watchdog: wdat: add start_enable global parameter
>>>>>
>>>>>  Documentation/watchdog/index.rst              |  1 +
>>>>>  .../watchdog-core-global-parameters.rst       | 74 +++++++++++++++++++
>>>>>  drivers/watchdog/watchdog_core.c              | 74 +++++++++++++++++++
>>>>>  drivers/watchdog/wdat_wdt.c                   |  2 +
>>>>>  include/linux/watchdog.h                      | 42 +++++++++++
>>>>>  5 files changed, 193 insertions(+)
>>>>>  create mode 100644
>>>>> Documentation/watchdog/watchdog-core-global-parameters.rst
>>>>>
>>>
>