| Message ID | 20210413154553.1968039-1-eichest@gmail.com (mailing list archive) |
|---|---|
| State | Rejected |
| Headers | show |
| Series | [RFC] watchdog: pretimeout: add a notifier call chain governor | expand |
On 4/13/21 8:45 AM, Stefan Eichenberger wrote: > Add a governor which calls a notifier call chain. This allows the > registration of custom functions to the watchdog pretimeout. This is > useful when writing out-of-tree kernel modules and it is more similar to > the register_reboot_notifier or register_oom_notifier API. > > Signed-off-by: Stefan Eichenberger <eichest@gmail.com> It seems questionable to implement such a notification as governor. A governor is configurable, and letting userspace override notifications seems very odd and unusual. Please provide a use case. Guenter > --- > drivers/watchdog/Kconfig | 16 ++++++ > drivers/watchdog/Makefile | 1 + > drivers/watchdog/pretimeout_notifier.c | 73 ++++++++++++++++++++++++++ > drivers/watchdog/watchdog_pretimeout.h | 2 + > include/linux/watchdog.h | 5 ++ > 5 files changed, 97 insertions(+) > create mode 100644 drivers/watchdog/pretimeout_notifier.c > > diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig > index 355100dad60a..ec019120effc 100644 > --- a/drivers/watchdog/Kconfig > +++ b/drivers/watchdog/Kconfig > @@ -105,6 +105,14 @@ config WATCHDOG_PRETIMEOUT_GOV_PANIC > Panic watchdog pretimeout governor, on watchdog pretimeout > event put the kernel into panic. > > +config WATCHDOG_PRETIMEOUT_GOV_NOTIFIER > + tristate "Notifier watchdog pretimeout governor" > + depends on WATCHDOG_CORE > + default WATCHDOG_CORE > + help > + Notifier watchdog pretimeout governor, on watchdog pretimeout > + event call a notifier call chain. > + > choice > prompt "Default Watchdog Pretimeout Governor" > default WATCHDOG_PRETIMEOUT_DEFAULT_GOV_PANIC > @@ -129,6 +137,14 @@ config WATCHDOG_PRETIMEOUT_DEFAULT_GOV_PANIC > a watchdog pretimeout event happens, consider that > a watchdog feeder is dead and reboot is unavoidable. > > +config WATCHDOG_PRETIMEOUT_DEFAULT_GOV_NOTIFIER > + bool "notifier" > + depends on WATCHDOG_PRETIMEOUT_GOV_NOTIFIER > + help > + Use notifier watchdog pretimeout governor by default, > + if a watchdog pretiemout event happens, a notifier > + call chain is called. > + > endchoice > > endif # WATCHDOG_PRETIMEOUT_GOV > diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile > index a7eade8b4d45..cb63f4eb89bf 100644 > --- a/drivers/watchdog/Makefile > +++ b/drivers/watchdog/Makefile > @@ -12,6 +12,7 @@ watchdog-$(CONFIG_WATCHDOG_PRETIMEOUT_GOV) += watchdog_pretimeout.o > > obj-$(CONFIG_WATCHDOG_PRETIMEOUT_GOV_NOOP) += pretimeout_noop.o > obj-$(CONFIG_WATCHDOG_PRETIMEOUT_GOV_PANIC) += pretimeout_panic.o > +obj-$(CONFIG_WATCHDOG_PRETIMEOUT_GOV_NOTIFIER) += pretimeout_notifier.o > > # Only one watchdog can succeed. We probe the ISA/PCI/USB based > # watchdog-cards first, then the architecture specific watchdog > diff --git a/drivers/watchdog/pretimeout_notifier.c b/drivers/watchdog/pretimeout_notifier.c > new file mode 100644 > index 000000000000..3f845c77ec44 > --- /dev/null > +++ b/drivers/watchdog/pretimeout_notifier.c > @@ -0,0 +1,73 @@ > +// SPDX-License-Identifier: GPL-2.0-or-later > +/* > + * Copyright (C) 2021 Stefan Eichenberger <eichest@gmail.com> > + */ > + > +#include <linux/kernel.h> > +#include <linux/module.h> > +#include <linux/watchdog.h> > + > +#include "watchdog_pretimeout.h" > + > +BLOCKING_NOTIFIER_HEAD(watchdog_pretimeout_notifier_list); > + > +/** > + * register_watchdog_pretimeout_notifier - Register function to be called on pretimeout > + * @nb: Info about notifier function to be called > + * > + * Registers a function with the list of functions to be called when a watchdog pretimeout occurs. > + * > + * Currently always returns zero, as blocking_notifier_chain_register() > + * always returns zero. > + */ > +int register_watchdog_pretimeout_notifier(struct notifier_block *nb) > +{ > + return blocking_notifier_chain_register(&watchdog_pretimeout_notifier_list, nb); > +} > +EXPORT_SYMBOL(register_watchdog_pretimeout_notifier); > + > +/** > + * unregister_watchdog_pretimeout_notifier - Unregister previously registered pretimeout notifier > + * @nb: Hook to be unregistered > + * > + * Unregisters a previously registered watchdog pretimeout notifier function. > + * > + * Returns zero on success, or %-ENOENT on failure. > + */ > +int unregister_watchdog_pretimeout_notifier(struct notifier_block *nb) > +{ > + return blocking_notifier_chain_unregister(&watchdog_pretimeout_notifier_list, nb); > +} > +EXPORT_SYMBOL(unregister_watchdog_pretimeout_notifier); > + > +/** > + * pretimeout_notifier - Notify registred methods on pretimeout > + * @wdd - watchdog_device > + * > + * Notify, watchdog has not been fed till pretimeout event. > + */ > +static void pretimeout_notifier(struct watchdog_device *wdd) > +{ > + blocking_notifier_call_chain(&watchdog_pretimeout_notifier_list, 0, wdd); > +} > + > +static struct watchdog_governor watchdog_gov_notifier = { > + .name = "notifier", > + .pretimeout = pretimeout_notifier, > +}; > + > +static int __init watchdog_gov_notifier_register(void) > +{ > + return watchdog_register_governor(&watchdog_gov_notifier); > +} > + > +static void __exit watchdog_gov_notifier_unregister(void) > +{ > + watchdog_unregister_governor(&watchdog_gov_notifier); > +} > +module_init(watchdog_gov_notifier_register); > +module_exit(watchdog_gov_notifier_unregister); > + > +MODULE_AUTHOR("Stefan Eichenberger <eichest@gmail.com>"); > +MODULE_DESCRIPTION("Notifier watchdog pretimeout governor"); > +MODULE_LICENSE("GPL"); > diff --git a/drivers/watchdog/watchdog_pretimeout.h b/drivers/watchdog/watchdog_pretimeout.h > index a3f1abc68839..bb00ca1fcadd 100644 > --- a/drivers/watchdog/watchdog_pretimeout.h > +++ b/drivers/watchdog/watchdog_pretimeout.h > @@ -28,6 +28,8 @@ int watchdog_pretimeout_governor_set(struct watchdog_device *wdd, > #define WATCHDOG_PRETIMEOUT_DEFAULT_GOV "noop" > #elif IS_ENABLED(CONFIG_WATCHDOG_PRETIMEOUT_DEFAULT_GOV_PANIC) > #define WATCHDOG_PRETIMEOUT_DEFAULT_GOV "panic" > +#elif IS_ENABLED(CONFIG_WATCHDOG_PRETIMEOUT_DEFAULT_GOV_NOTIFIER) > +#define WATCHDOG_PRETIMEOUT_DEFAULT_GOV "notifier" > #endif > > #else > diff --git a/include/linux/watchdog.h b/include/linux/watchdog.h > index 9b19e6bb68b5..760beab1007f 100644 > --- a/include/linux/watchdog.h > +++ b/include/linux/watchdog.h > @@ -215,4 +215,9 @@ int watchdog_set_last_hw_keepalive(struct watchdog_device *, unsigned int); > /* devres register variant */ > int devm_watchdog_register_device(struct device *dev, struct watchdog_device *); > > +#if IS_ENABLED(CONFIG_WATCHDOG_PRETIMEOUT_GOV_NOTIFIER) > +int register_watchdog_pretimeout_notifier(struct notifier_block *nb); > +int unregister_watchdog_pretimeout_notifier(struct notifier_block *nb); > +#endif > + > #endif /* ifndef _LINUX_WATCHDOG_H */ >
Hi Guenter, Thanks a lot for the fast feedback! On Tue, Apr 13, 2021 at 09:15:35AM -0700, Guenter Roeck wrote: > On 4/13/21 8:45 AM, Stefan Eichenberger wrote: > > Add a governor which calls a notifier call chain. This allows the > > registration of custom functions to the watchdog pretimeout. This is > > useful when writing out-of-tree kernel modules and it is more similar to > > the register_reboot_notifier or register_oom_notifier API. > > > > Signed-off-by: Stefan Eichenberger <eichest@gmail.com> > > It seems questionable to implement such a notification as governor. > A governor is configurable, and letting userspace override notifications > seems very odd and unusual. Please provide a use case. What I would like to do is to have an out-of-tree module which will write a pattern into a reserved memory region when a watchdog occurs. After a reboot the module then read this reserved memory region and can differentiate between different reset reasons. Here the example kernel module: https://github.com/embear-engineering/sample-kernel-modules/blob/use-watchdog-register/reset-reason/reset-reason.c Registering to the watchdog happens on line 180. I think I could just implement a governor in the module but it is not really flexible because then I would have to compile the kernel with e.g. the noop governor and then switch to my governor after the module loaded. Then I thought why not using a notifier chain similar to reboot, panic, or oom. Regards, Stefan
On 4/13/21 9:56 AM, Stefan Eichenberger wrote: > Hi Guenter, > > Thanks a lot for the fast feedback! > > On Tue, Apr 13, 2021 at 09:15:35AM -0700, Guenter Roeck wrote: >> On 4/13/21 8:45 AM, Stefan Eichenberger wrote: >>> Add a governor which calls a notifier call chain. This allows the >>> registration of custom functions to the watchdog pretimeout. This is >>> useful when writing out-of-tree kernel modules and it is more similar to >>> the register_reboot_notifier or register_oom_notifier API. >>> >>> Signed-off-by: Stefan Eichenberger <eichest@gmail.com> >> >> It seems questionable to implement such a notification as governor. >> A governor is configurable, and letting userspace override notifications >> seems very odd and unusual. Please provide a use case. > > What I would like to do is to have an out-of-tree module which will > write a pattern into a reserved memory region when a watchdog occurs. > After a reboot the module then read this reserved memory region and can > differentiate between different reset reasons. Here the example kernel > module: > https://github.com/embear-engineering/sample-kernel-modules/blob/use-watchdog-register/reset-reason/reset-reason.c > > Registering to the watchdog happens on line 180. > > I think I could just implement a governor in the module but it is not > really flexible because then I would have to compile the kernel with > e.g. the noop governor and then switch to my governor after the module > loaded. Then I thought why not using a notifier chain similar to reboot, > panic, or oom. > I have two problems with this: First, we do not support in-kernel code purely for out-of-tree drivers. Second, the idea of using a configurable notifier (ie one that can be disabled by userspace action) seems odd. If anything, that would have to be an unconditional notifier. However, again, that would require a use case in the upstream kernel. Guenter
On Wed, Apr 14, 2021 at 04:50:38AM -0700, Guenter Roeck wrote: > On 4/13/21 9:56 AM, Stefan Eichenberger wrote: > > Hi Guenter, > > > > Thanks a lot for the fast feedback! > > > > On Tue, Apr 13, 2021 at 09:15:35AM -0700, Guenter Roeck wrote: > >> On 4/13/21 8:45 AM, Stefan Eichenberger wrote: > >>> Add a governor which calls a notifier call chain. This allows the > >>> registration of custom functions to the watchdog pretimeout. This is > >>> useful when writing out-of-tree kernel modules and it is more similar to > >>> the register_reboot_notifier or register_oom_notifier API. > >>> > >>> Signed-off-by: Stefan Eichenberger <eichest@gmail.com> > >> > >> It seems questionable to implement such a notification as governor. > >> A governor is configurable, and letting userspace override notifications > >> seems very odd and unusual. Please provide a use case. > > > > What I would like to do is to have an out-of-tree module which will > > write a pattern into a reserved memory region when a watchdog occurs. > > After a reboot the module then read this reserved memory region and can > > differentiate between different reset reasons. Here the example kernel > > module: > > https://github.com/embear-engineering/sample-kernel-modules/blob/use-watchdog-register/reset-reason/reset-reason.c > > > > Registering to the watchdog happens on line 180. > > > > I think I could just implement a governor in the module but it is not > > really flexible because then I would have to compile the kernel with > > e.g. the noop governor and then switch to my governor after the module > > loaded. Then I thought why not using a notifier chain similar to reboot, > > panic, or oom. > > > > I have two problems with this: First, we do not support in-kernel code purely > for out-of-tree drivers. Second, the idea of using a configurable notifier > (ie one that can be disabled by userspace action) seems odd. If anything, > that would have to be an unconditional notifier. However, again, that would > require a use case in the upstream kernel. I see your point. Thanks for checking anyways. Regards, Stefan
diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig index 355100dad60a..ec019120effc 100644 --- a/drivers/watchdog/Kconfig +++ b/drivers/watchdog/Kconfig @@ -105,6 +105,14 @@ config WATCHDOG_PRETIMEOUT_GOV_PANIC Panic watchdog pretimeout governor, on watchdog pretimeout event put the kernel into panic. +config WATCHDOG_PRETIMEOUT_GOV_NOTIFIER + tristate "Notifier watchdog pretimeout governor" + depends on WATCHDOG_CORE + default WATCHDOG_CORE + help + Notifier watchdog pretimeout governor, on watchdog pretimeout + event call a notifier call chain. + choice prompt "Default Watchdog Pretimeout Governor" default WATCHDOG_PRETIMEOUT_DEFAULT_GOV_PANIC @@ -129,6 +137,14 @@ config WATCHDOG_PRETIMEOUT_DEFAULT_GOV_PANIC a watchdog pretimeout event happens, consider that a watchdog feeder is dead and reboot is unavoidable. +config WATCHDOG_PRETIMEOUT_DEFAULT_GOV_NOTIFIER + bool "notifier" + depends on WATCHDOG_PRETIMEOUT_GOV_NOTIFIER + help + Use notifier watchdog pretimeout governor by default, + if a watchdog pretiemout event happens, a notifier + call chain is called. + endchoice endif # WATCHDOG_PRETIMEOUT_GOV diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile index a7eade8b4d45..cb63f4eb89bf 100644 --- a/drivers/watchdog/Makefile +++ b/drivers/watchdog/Makefile @@ -12,6 +12,7 @@ watchdog-$(CONFIG_WATCHDOG_PRETIMEOUT_GOV) += watchdog_pretimeout.o obj-$(CONFIG_WATCHDOG_PRETIMEOUT_GOV_NOOP) += pretimeout_noop.o obj-$(CONFIG_WATCHDOG_PRETIMEOUT_GOV_PANIC) += pretimeout_panic.o +obj-$(CONFIG_WATCHDOG_PRETIMEOUT_GOV_NOTIFIER) += pretimeout_notifier.o # Only one watchdog can succeed. We probe the ISA/PCI/USB based # watchdog-cards first, then the architecture specific watchdog diff --git a/drivers/watchdog/pretimeout_notifier.c b/drivers/watchdog/pretimeout_notifier.c new file mode 100644 index 000000000000..3f845c77ec44 --- /dev/null +++ b/drivers/watchdog/pretimeout_notifier.c @@ -0,0 +1,73 @@ +// SPDX-License-Identifier: GPL-2.0-or-later +/* + * Copyright (C) 2021 Stefan Eichenberger <eichest@gmail.com> + */ + +#include <linux/kernel.h> +#include <linux/module.h> +#include <linux/watchdog.h> + +#include "watchdog_pretimeout.h" + +BLOCKING_NOTIFIER_HEAD(watchdog_pretimeout_notifier_list); + +/** + * register_watchdog_pretimeout_notifier - Register function to be called on pretimeout + * @nb: Info about notifier function to be called + * + * Registers a function with the list of functions to be called when a watchdog pretimeout occurs. + * + * Currently always returns zero, as blocking_notifier_chain_register() + * always returns zero. + */ +int register_watchdog_pretimeout_notifier(struct notifier_block *nb) +{ + return blocking_notifier_chain_register(&watchdog_pretimeout_notifier_list, nb); +} +EXPORT_SYMBOL(register_watchdog_pretimeout_notifier); + +/** + * unregister_watchdog_pretimeout_notifier - Unregister previously registered pretimeout notifier + * @nb: Hook to be unregistered + * + * Unregisters a previously registered watchdog pretimeout notifier function. + * + * Returns zero on success, or %-ENOENT on failure. + */ +int unregister_watchdog_pretimeout_notifier(struct notifier_block *nb) +{ + return blocking_notifier_chain_unregister(&watchdog_pretimeout_notifier_list, nb); +} +EXPORT_SYMBOL(unregister_watchdog_pretimeout_notifier); + +/** + * pretimeout_notifier - Notify registred methods on pretimeout + * @wdd - watchdog_device + * + * Notify, watchdog has not been fed till pretimeout event. + */ +static void pretimeout_notifier(struct watchdog_device *wdd) +{ + blocking_notifier_call_chain(&watchdog_pretimeout_notifier_list, 0, wdd); +} + +static struct watchdog_governor watchdog_gov_notifier = { + .name = "notifier", + .pretimeout = pretimeout_notifier, +}; + +static int __init watchdog_gov_notifier_register(void) +{ + return watchdog_register_governor(&watchdog_gov_notifier); +} + +static void __exit watchdog_gov_notifier_unregister(void) +{ + watchdog_unregister_governor(&watchdog_gov_notifier); +} +module_init(watchdog_gov_notifier_register); +module_exit(watchdog_gov_notifier_unregister); + +MODULE_AUTHOR("Stefan Eichenberger <eichest@gmail.com>"); +MODULE_DESCRIPTION("Notifier watchdog pretimeout governor"); +MODULE_LICENSE("GPL"); diff --git a/drivers/watchdog/watchdog_pretimeout.h b/drivers/watchdog/watchdog_pretimeout.h index a3f1abc68839..bb00ca1fcadd 100644 --- a/drivers/watchdog/watchdog_pretimeout.h +++ b/drivers/watchdog/watchdog_pretimeout.h @@ -28,6 +28,8 @@ int watchdog_pretimeout_governor_set(struct watchdog_device *wdd, #define WATCHDOG_PRETIMEOUT_DEFAULT_GOV "noop" #elif IS_ENABLED(CONFIG_WATCHDOG_PRETIMEOUT_DEFAULT_GOV_PANIC) #define WATCHDOG_PRETIMEOUT_DEFAULT_GOV "panic" +#elif IS_ENABLED(CONFIG_WATCHDOG_PRETIMEOUT_DEFAULT_GOV_NOTIFIER) +#define WATCHDOG_PRETIMEOUT_DEFAULT_GOV "notifier" #endif #else diff --git a/include/linux/watchdog.h b/include/linux/watchdog.h index 9b19e6bb68b5..760beab1007f 100644 --- a/include/linux/watchdog.h +++ b/include/linux/watchdog.h @@ -215,4 +215,9 @@ int watchdog_set_last_hw_keepalive(struct watchdog_device *, unsigned int); /* devres register variant */ int devm_watchdog_register_device(struct device *dev, struct watchdog_device *); +#if IS_ENABLED(CONFIG_WATCHDOG_PRETIMEOUT_GOV_NOTIFIER) +int register_watchdog_pretimeout_notifier(struct notifier_block *nb); +int unregister_watchdog_pretimeout_notifier(struct notifier_block *nb); +#endif + #endif /* ifndef _LINUX_WATCHDOG_H */
Add a governor which calls a notifier call chain. This allows the registration of custom functions to the watchdog pretimeout. This is useful when writing out-of-tree kernel modules and it is more similar to the register_reboot_notifier or register_oom_notifier API. Signed-off-by: Stefan Eichenberger <eichest@gmail.com> --- drivers/watchdog/Kconfig | 16 ++++++ drivers/watchdog/Makefile | 1 + drivers/watchdog/pretimeout_notifier.c | 73 ++++++++++++++++++++++++++ drivers/watchdog/watchdog_pretimeout.h | 2 + include/linux/watchdog.h | 5 ++ 5 files changed, 97 insertions(+) create mode 100644 drivers/watchdog/pretimeout_notifier.c