| Message ID | 20210308112151.716315-1-f.suligoi@asem.it (mailing list archive) |
|---|---|
| Headers | show |
| Series | Watchdog Core Global Parameters | expand |
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 >
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 > >
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 >>> >
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 > >>> > >
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 >>>>> >>> >
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