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
>