[RFC] watchdog: pretimeout: add a notifier call chain governor

[RFC] watchdog: pretimeout: add a notifier call chain governor

[Stefan Eichenberger]

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

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 */
-- 
2.27.0

Re: [RFC] watchdog: pretimeout: add a notifier call chain governor

[Guenter Roeck]

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 */
> 

Re: [RFC] watchdog: pretimeout: add a notifier call chain governor

[Stefan Eichenberger]

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

Re: [RFC] watchdog: pretimeout: add a notifier call chain governor

[Guenter Roeck]

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

Re: [RFC] watchdog: pretimeout: add a notifier call chain governor

[Stefan Eichenberger]

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