[PATCH 0/6] watchdog: add watchdog pretimeout framework

[PATCH 0/6] watchdog: add watchdog pretimeout framework

[Vladimir Zapolskiy]

The change adds a simple watchdog pretimeout framework infrastructure,
its purpose is to allow users to select a desired handling of watchdog
pretimeout events, which may be generated by a watchdog driver.

The idea of adding this kind of a framework appeared after reviewing
several attempts to add hardcoded pretimeout event handling to some
watchdog driver and after a discussion with Guenter, see
https://lkml.org/lkml/2015/11/4/346

By design every watchdog pretimeout governor may be compiled as a
kernel module, a user selects a default watchdog pretimeout
governor during compilation stage and can select another governor in
runtime.

Watchdogs with WDIOF_PRETIMEOUT capability now have two device
attributes in sysfs: read/write pretimeout_governor attribute and read
only pretimeout_available_governors attribute.

To throw a pretimeout event for further processing a watchdog driver
should call exported  watchdog_notify_pretimeout(wdd) interface.

In addition to the framework a number of simple watchdog pretimeout
governors are added for review.

Vladimir Zapolskiy (6):
  watchdog: add watchdog pretimeout framework
  watchdog: pretimeout: add noop pretimeout governor
  watchdog: pretimeout: add panic pretimeout governor
  watchdog: pretimeout: add userspace notifier pretimeout governor
  watchdog: pretimeout: add device specific notifier pretimeout governor
  watchdog: pretimeout: add ping pretimeout governor

 drivers/watchdog/Kconfig                |  91 +++++++++
 drivers/watchdog/Makefile               |  10 +-
 drivers/watchdog/pretimeout_device.c    |  49 +++++
 drivers/watchdog/pretimeout_noop.c      |  49 +++++
 drivers/watchdog/pretimeout_panic.c     |  49 +++++
 drivers/watchdog/pretimeout_ping.c      |  48 +++++
 drivers/watchdog/pretimeout_userspace.c |  49 +++++
 drivers/watchdog/watchdog_core.c        |  14 +-
 drivers/watchdog/watchdog_pretimeout.c  | 348 ++++++++++++++++++++++++++++++++
 drivers/watchdog/watchdog_pretimeout.h  |  31 +++
 include/linux/watchdog.h                |  12 ++
 11 files changed, 747 insertions(+), 3 deletions(-)
 create mode 100644 drivers/watchdog/pretimeout_device.c
 create mode 100644 drivers/watchdog/pretimeout_noop.c
 create mode 100644 drivers/watchdog/pretimeout_panic.c
 create mode 100644 drivers/watchdog/pretimeout_ping.c
 create mode 100644 drivers/watchdog/pretimeout_userspace.c
 create mode 100644 drivers/watchdog/watchdog_pretimeout.c
 create mode 100644 drivers/watchdog/watchdog_pretimeout.h

-- 
2.5.0

[PATCH 1/6] watchdog: add watchdog pretimeout framework

[Vladimir Zapolskiy]

The change adds a simple watchdog pretimeout framework infrastructure,
its purpose is to allow users to select a desired handling of watchdog
pretimeout events, which may be generated by a watchdog driver.

By design every watchdog pretimeout governor may be compiled as a
kernel module, a user selects a default watchdog pretimeout
governor during compilation stage and can select another governor in
runtime.

Watchdogs with WDIOF_PRETIMEOUT capability now have two device
attributes in sysfs: read/write pretimeout_governor attribute and read
only pretimeout_available_governors attribute.

Watchdogs with no WDIOF_PRETIMEOUT capability has no changes in
sysfs.

Signed-off-by: Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com>
---
 drivers/watchdog/Kconfig               |   8 +
 drivers/watchdog/Makefile              |   5 +-
 drivers/watchdog/watchdog_core.c       |  14 +-
 drivers/watchdog/watchdog_pretimeout.c | 348 +++++++++++++++++++++++++++++++++
 drivers/watchdog/watchdog_pretimeout.h |  31 +++
 include/linux/watchdog.h               |  10 +
 6 files changed, 413 insertions(+), 3 deletions(-)
 create mode 100644 drivers/watchdog/watchdog_pretimeout.c
 create mode 100644 drivers/watchdog/watchdog_pretimeout.h

diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
index 7a8a6c6..eeb515c 100644
--- a/drivers/watchdog/Kconfig
+++ b/drivers/watchdog/Kconfig
@@ -1625,4 +1625,12 @@ config USBPCWATCHDOG
 
 	  Most people will say N.
 
+comment "Watchdog Pretimeout Governors"
+
+config WATCHDOG_PRETIMEOUT_GOV
+	bool "Enable watchdog pretimeout governors"
+	default n
+	help
+	  The option allows to select watchdog pretimeout governors.
+
 endif # WATCHDOG
diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
index 53d4827..6749cac 100644
--- a/drivers/watchdog/Makefile
+++ b/drivers/watchdog/Makefile
@@ -3,9 +3,12 @@
 #
 
 # The WatchDog Timer Driver Core.
-watchdog-objs	+= watchdog_core.o watchdog_dev.o
 obj-$(CONFIG_WATCHDOG_CORE)	+= watchdog.o
 
+watchdog-objs	+= watchdog_core.o watchdog_dev.o
+
+watchdog-$(CONFIG_WATCHDOG_PRETIMEOUT_GOV)	+= watchdog_pretimeout.o
+
 # Only one watchdog can succeed. We probe the ISA/PCI/USB based
 # watchdog-cards first, then the architecture specific watchdog
 # drivers and then the architecture independent "softdog" driver.
diff --git a/drivers/watchdog/watchdog_core.c b/drivers/watchdog/watchdog_core.c
index 873f139..ca99585 100644
--- a/drivers/watchdog/watchdog_core.c
+++ b/drivers/watchdog/watchdog_core.c
@@ -39,6 +39,7 @@
 #include <linux/of.h>		/* For of_get_timeout_sec */
 
 #include "watchdog_core.h"	/* For watchdog_dev_register/... */
+#include "watchdog_pretimeout.h"
 
 static DEFINE_IDA(watchdog_ida);
 static struct class *watchdog_class;
@@ -194,7 +195,7 @@ static int __watchdog_register_device(struct watchdog_device *wdd)
 
 	devno = wdd->cdev.dev;
 	wdd->dev = device_create(watchdog_class, wdd->parent, devno,
-					NULL, "watchdog%d", wdd->id);
+				 wdd, "watchdog%d", wdd->id);
 	if (IS_ERR(wdd->dev)) {
 		watchdog_dev_unregister(wdd);
 		ida_simple_remove(&watchdog_ida, id);
@@ -202,7 +203,14 @@ static int __watchdog_register_device(struct watchdog_device *wdd)
 		return ret;
 	}
 
-	return 0;
+	ret = watchdog_register_pretimeout(wdd);
+	if (ret) {
+		device_destroy(watchdog_class, devno);
+		watchdog_dev_unregister(wdd);
+		ida_simple_remove(&watchdog_ida, id);
+	}
+
+	return ret;
 }
 
 /**
@@ -238,6 +246,8 @@ static void __watchdog_unregister_device(struct watchdog_device *wdd)
 	if (wdd == NULL)
 		return;
 
+	watchdog_unregister_pretimeout(wdd);
+
 	devno = wdd->cdev.dev;
 	ret = watchdog_dev_unregister(wdd);
 	if (ret)
diff --git a/drivers/watchdog/watchdog_pretimeout.c b/drivers/watchdog/watchdog_pretimeout.c
new file mode 100644
index 0000000..27c531f
--- /dev/null
+++ b/drivers/watchdog/watchdog_pretimeout.c
@@ -0,0 +1,348 @@
+/*
+ * Copyright (C) 2015 Mentor Graphics
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ */
+
+#include <linux/module.h>
+#include <linux/slab.h>
+#include <linux/watchdog.h>
+#include <linux/workqueue.h>
+
+#include "watchdog_pretimeout.h"
+
+/* The mutex protects governor list and serializes external interfaces */
+static DEFINE_MUTEX(governor_lock);
+
+/* List of registered watchdog pretimeout governors */
+static LIST_HEAD(governor_list);
+
+/* The spinlock protects wdd->gov and pretimeout_list */
+static DEFINE_SPINLOCK(pretimeout_lock);
+
+/* List of watchdog devices, which can generate a pretimeout event */
+static LIST_HEAD(pretimeout_list);
+
+/* Single workqueue to handle pretimeout events from all watchdogs */
+static struct workqueue_struct *pretimeout_wq;
+
+struct watchdog_pretimeout {
+	struct watchdog_device	*wdd;
+	struct work_struct	work;
+	struct list_head	entry;
+};
+
+#define to_pretimeout(wk) container_of(work, struct watchdog_pretimeout, wk)
+
+static struct watchdog_governor *find_governor(const char *gov_name)
+{
+	struct watchdog_governor *gov;
+
+	list_for_each_entry(gov, &governor_list, entry)
+		if (!strncasecmp(gov_name, gov->name, WATCHDOG_GOV_NAME_LEN))
+			return gov;
+
+	return NULL;
+}
+
+static struct watchdog_governor *find_default_governor(void)
+{
+	struct watchdog_governor *gov;
+
+	list_for_each_entry(gov, &governor_list, entry)
+		if (gov->is_default)
+			return gov;
+
+	return NULL;
+}
+
+static void watchdog_pretimeout_execute(struct work_struct *work)
+{
+	struct watchdog_pretimeout *p;
+
+	mutex_lock(&governor_lock);
+
+	p = to_pretimeout(work);
+
+	/* While we were scheduled a governer may be unregistered */
+	spin_lock_irq(&pretimeout_lock);
+	if (!p->wdd->gov) {
+		spin_unlock_irq(&pretimeout_lock);
+		return;
+	}
+	spin_unlock_irq(&pretimeout_lock);
+
+	p->wdd->gov->pretimeout(p->wdd);
+	put_device(p->wdd->dev);
+
+	mutex_unlock(&governor_lock);
+}
+
+void watchdog_notify_pretimeout(struct watchdog_device *wdd)
+{
+	struct watchdog_pretimeout *p;
+	unsigned long flags;
+
+	if (!wdd)
+		return;
+
+	spin_lock_irqsave(&pretimeout_lock, flags);
+	if (!wdd->gov) {
+		spin_unlock_irqrestore(&pretimeout_lock, flags);
+		return;
+	}
+
+	list_for_each_entry(p, &pretimeout_list, entry) {
+		if (p->wdd == wdd) {
+			get_device(wdd->dev);
+			queue_work(pretimeout_wq, &p->work);
+			break;
+		}
+	}
+	spin_unlock_irqrestore(&pretimeout_lock, flags);
+}
+EXPORT_SYMBOL_GPL(watchdog_notify_pretimeout);
+
+int watchdog_register_governor(struct watchdog_governor *gov)
+{
+	struct watchdog_pretimeout *p;
+
+	if (!gov || !gov->name || !gov->pretimeout ||
+	    strlen(gov->name) >= WATCHDOG_GOV_NAME_LEN)
+		return -EINVAL;
+
+	mutex_lock(&governor_lock);
+
+	if (find_governor(gov->name)) {
+		mutex_unlock(&governor_lock);
+		return -EBUSY;
+	}
+
+	if (gov->is_default) {
+		if (find_default_governor()) {
+			mutex_unlock(&governor_lock);
+			return -EBUSY;
+		}
+
+		spin_lock_irq(&pretimeout_lock);
+		list_for_each_entry(p, &pretimeout_list, entry)
+			if (!p->wdd->gov)
+				p->wdd->gov = gov;
+		spin_unlock_irq(&pretimeout_lock);
+	}
+
+	list_add(&gov->entry, &governor_list);
+
+	mutex_unlock(&governor_lock);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(watchdog_register_governor);
+
+void watchdog_unregister_governor(struct watchdog_governor *gov)
+{
+	struct watchdog_pretimeout *p;
+	struct watchdog_governor *gov_default;
+
+	if (!gov)
+		return;
+
+	mutex_lock(&governor_lock);
+
+	gov_default = find_default_governor();
+	if (gov_default == gov)
+		gov_default = NULL;
+
+	spin_lock_irq(&pretimeout_lock);
+	list_for_each_entry(p, &pretimeout_list, entry)
+		if (p->wdd->gov == gov)
+			p->wdd->gov = gov_default;
+	spin_unlock_irq(&pretimeout_lock);
+
+	list_del(&gov->entry);
+
+	mutex_unlock(&governor_lock);
+}
+EXPORT_SYMBOL_GPL(watchdog_unregister_governor);
+
+static ssize_t pretimeout_governor_store(struct device *dev,
+					 struct device_attribute *attr,
+					 const char *buf, size_t count)
+{
+	struct watchdog_device *wdd = dev_get_drvdata(dev);
+	struct watchdog_governor *gov;
+
+	mutex_lock(&governor_lock);
+
+	gov = find_governor(buf);
+	if (!gov) {
+		mutex_unlock(&governor_lock);
+		return -EINVAL;
+	}
+
+	spin_lock_irq(&pretimeout_lock);
+	wdd->gov = gov;
+	spin_unlock_irq(&pretimeout_lock);
+
+	mutex_unlock(&governor_lock);
+
+	return count;
+}
+
+static ssize_t pretimeout_governor_show(struct device *dev,
+				struct device_attribute *devattr, char *buf)
+{
+	struct watchdog_device *wdd = dev_get_drvdata(dev);
+	char gov_name[WATCHDOG_GOV_NAME_LEN] = { 0 };
+	int count;
+
+	mutex_lock(&governor_lock);
+
+	spin_lock_irq(&pretimeout_lock);
+	if (wdd->gov)
+		strncpy(gov_name, wdd->gov->name, WATCHDOG_GOV_NAME_LEN);
+	spin_unlock_irq(&pretimeout_lock);
+
+	count = sprintf(buf, "%s\n", gov_name);
+
+	mutex_unlock(&governor_lock);
+
+	return count;
+}
+static DEVICE_ATTR_RW(pretimeout_governor);
+
+static ssize_t pretimeout_available_governors_show(struct device *dev,
+				struct device_attribute *devattr, char *buf)
+{
+	struct watchdog_governor *gov;
+	int count = 0;
+
+	mutex_lock(&governor_lock);
+
+	list_for_each_entry(gov, &governor_list, entry)
+		count += sprintf(buf + count, "%s\n", gov->name);
+
+	mutex_unlock(&governor_lock);
+
+	return count;
+}
+static DEVICE_ATTR_RO(pretimeout_available_governors);
+
+static struct attribute *wdd_pretimeout_attrs[] = {
+	&dev_attr_pretimeout_governor.attr,
+	&dev_attr_pretimeout_available_governors.attr,
+	NULL,
+};
+
+static umode_t wdd_pretimeout_attr_is_visible(struct kobject *kobj,
+				   struct attribute *attr, int n)
+{
+	struct device *dev = container_of(kobj, struct device, kobj);
+	struct watchdog_device *wdd = dev_get_drvdata(dev);
+
+	if (wdd->info->options & WDIOF_PRETIMEOUT)
+		return attr->mode;
+
+	return 0;
+}
+
+static const struct attribute_group wdd_pretimeout_group = {
+	.is_visible = wdd_pretimeout_attr_is_visible,
+	.attrs = wdd_pretimeout_attrs,
+};
+
+static const struct attribute_group *wdd_pretimeout_groups[] = {
+	&wdd_pretimeout_group,
+	NULL,
+};
+
+int watchdog_register_pretimeout(struct watchdog_device *wdd)
+{
+	struct watchdog_pretimeout *p;
+	struct watchdog_governor *gov;
+	int ret;
+
+	if (!(wdd->info->options & WDIOF_PRETIMEOUT))
+		return 0;
+
+	p = kzalloc(sizeof(*p), GFP_KERNEL);
+	if (!p)
+		return -ENOMEM;
+	INIT_WORK(&p->work, watchdog_pretimeout_execute);
+
+	ret = sysfs_create_groups(&wdd->dev->kobj, wdd_pretimeout_groups);
+	if (ret) {
+		kfree(p);
+		return ret;
+	}
+
+	mutex_lock(&governor_lock);
+	gov = find_default_governor();
+
+	spin_lock_irq(&pretimeout_lock);
+	list_add(&p->entry, &pretimeout_list);
+	p->wdd = wdd;
+	wdd->gov = gov;
+	spin_unlock_irq(&pretimeout_lock);
+
+	mutex_unlock(&governor_lock);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(watchdog_register_pretimeout);
+
+void watchdog_unregister_pretimeout(struct watchdog_device *wdd)
+{
+	struct watchdog_pretimeout *p;
+
+	if (!(wdd->info->options & WDIOF_PRETIMEOUT))
+		return;
+
+	mutex_lock(&governor_lock);
+
+	spin_lock_irq(&pretimeout_lock);
+	wdd->gov = NULL;
+	spin_unlock_irq(&pretimeout_lock);
+
+	flush_workqueue(pretimeout_wq);
+
+	/* No need to use list_for_each_entry_safe(), remove only one entry */
+	spin_lock_irq(&pretimeout_lock);
+	list_for_each_entry(p, &pretimeout_list, entry) {
+		if (p->wdd == wdd) {
+			list_del(&p->entry);
+			break;
+		}
+	}
+	spin_unlock_irq(&pretimeout_lock);
+
+	kfree(p);
+
+	mutex_unlock(&governor_lock);
+}
+EXPORT_SYMBOL_GPL(watchdog_unregister_pretimeout);
+
+static int __init watchdog_register_pretimeout_gov(void)
+{
+	pretimeout_wq = create_workqueue("watchdog pretimeout");
+	if (!pretimeout_wq)
+		return -ENOMEM;
+
+	return 0;
+}
+subsys_initcall(watchdog_register_pretimeout_gov);
+
+static void __exit watchdog_unregister_pretimeout_gov(void)
+{
+	flush_workqueue(pretimeout_wq);
+	destroy_workqueue(pretimeout_wq);
+}
+module_exit(watchdog_unregister_pretimeout_gov);
+
+MODULE_AUTHOR("Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com>");
+MODULE_DESCRIPTION("Watchdog pretimeout governor framework");
+MODULE_LICENSE("GPL");
diff --git a/drivers/watchdog/watchdog_pretimeout.h b/drivers/watchdog/watchdog_pretimeout.h
new file mode 100644
index 0000000..3af714f
--- /dev/null
+++ b/drivers/watchdog/watchdog_pretimeout.h
@@ -0,0 +1,31 @@
+#ifndef __WATCHDOG_PRETIMEOUT_H
+#define __WATCHDOG_PRETIMEOUT_H
+
+#include <linux/watchdog.h>
+
+#define WATCHDOG_GOV_NAME_LEN	20
+
+struct watchdog_governor {
+	const char		name[WATCHDOG_GOV_NAME_LEN];
+	void			(*pretimeout)(struct watchdog_device *wdd);
+	bool			is_default;
+	struct list_head	entry;
+};
+
+/* Interfaces to watchdog pretimeout governors */
+int watchdog_register_governor(struct watchdog_governor *gov);
+void watchdog_unregister_governor(struct watchdog_governor *gov);
+
+/* Interfaces to watchdog_core.c */
+#ifdef CONFIG_WATCHDOG_PRETIMEOUT_GOV
+int watchdog_register_pretimeout(struct watchdog_device *wdd);
+void watchdog_unregister_pretimeout(struct watchdog_device *wdd);
+#else
+static inline int watchdog_register_pretimeout(struct watchdog_device *wdd)
+{
+	return 0;
+}
+static inline void watchdog_unregister_pretimeout(struct watchdog_device *) {}
+#endif
+
+#endif
diff --git a/include/linux/watchdog.h b/include/linux/watchdog.h
index 027b1f4..960223e 100644
--- a/include/linux/watchdog.h
+++ b/include/linux/watchdog.h
@@ -16,6 +16,7 @@
 
 struct watchdog_ops;
 struct watchdog_device;
+struct watchdog_governor;
 
 /** struct watchdog_ops - The watchdog-devices operations
  *
@@ -58,6 +59,7 @@ struct watchdog_ops {
  * @parent:	The parent bus device
  * @info:	Pointer to a watchdog_info structure.
  * @ops:	Pointer to the list of watchdog operations.
+ * @gov:	Pointer to watchdog pretimeout governor.
  * @bootstatus:	Status of the watchdog device at boot.
  * @timeout:	The watchdog devices timeout value (in seconds).
  * @min_timeout:The watchdog devices minimum timeout value (in seconds).
@@ -84,6 +86,7 @@ struct watchdog_device {
 	struct device *parent;
 	const struct watchdog_info *info;
 	const struct watchdog_ops *ops;
+	struct watchdog_governor *gov;
 	unsigned int bootstatus;
 	unsigned int timeout;
 	unsigned int min_timeout;
@@ -147,4 +150,11 @@ extern int watchdog_init_timeout(struct watchdog_device *wdd,
 extern int watchdog_register_device(struct watchdog_device *);
 extern void watchdog_unregister_device(struct watchdog_device *);
 
+/* drivers/watchdog/watchdog_pretimeout.c */
+#ifdef CONFIG_WATCHDOG_PRETIMEOUT_GOV
+void watchdog_notify_pretimeout(struct watchdog_device *wdd);
+#else
+static inline void watchdog_notify_pretimeout(struct watchdog_device *wdd) {}
+#endif
+
 #endif  /* ifndef _LINUX_WATCHDOG_H */
-- 
2.1.4

[PATCH 2/6] watchdog: pretimeout: add noop pretimeout governor

[Vladimir Zapolskiy]

Noop watchdog pretimeout governor, only an informational message is
added to the kernel log buffer.

Signed-off-by: Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com>
---
 drivers/watchdog/Kconfig           | 26 ++++++++++++++++++++
 drivers/watchdog/Makefile          |  1 +
 drivers/watchdog/pretimeout_noop.c | 49 ++++++++++++++++++++++++++++++++++++++
 3 files changed, 76 insertions(+)
 create mode 100644 drivers/watchdog/pretimeout_noop.c

diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
index eeb515c..4940c3b 100644
--- a/drivers/watchdog/Kconfig
+++ b/drivers/watchdog/Kconfig
@@ -1633,4 +1633,30 @@ config WATCHDOG_PRETIMEOUT_GOV
 	help
 	  The option allows to select watchdog pretimeout governors.
 
+if WATCHDOG_PRETIMEOUT_GOV
+
+choice
+	prompt "Default Watchdog Pretimeout Governor"
+	default WATCHDOG_PRETIMEOUT_DEFAULT_GOV_NOOP
+	help
+	  This option selects a default watchdog pretimeout governor.
+	  The governor takes its action, if a watchdog is capable
+	  to report a pretimeout event.
+
+config WATCHDOG_PRETIMEOUT_DEFAULT_GOV_NOOP
+	bool "noop"
+	select WATCHDOG_PRETIMEOUT_GOV_NOOP
+	help
+	  Use noop watchdog pretimeout governor by default.
+
+endchoice
+
+config WATCHDOG_PRETIMEOUT_GOV_NOOP
+	tristate "Noop watchdog pretimeout governor"
+	help
+	  Noop watchdog pretimeout governor, only an informational
+	  message is added to kernel log buffer.
+
+endif # WATCHDOG_PRETIMEOUT_GOV
+
 endif # WATCHDOG
diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
index 6749cac..16a9e32 100644
--- a/drivers/watchdog/Makefile
+++ b/drivers/watchdog/Makefile
@@ -8,6 +8,7 @@ obj-$(CONFIG_WATCHDOG_CORE)	+= watchdog.o
 watchdog-objs	+= watchdog_core.o watchdog_dev.o
 
 watchdog-$(CONFIG_WATCHDOG_PRETIMEOUT_GOV)	+= watchdog_pretimeout.o
+obj-$(CONFIG_WATCHDOG_PRETIMEOUT_GOV_NOOP)	+= pretimeout_noop.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_noop.c b/drivers/watchdog/pretimeout_noop.c
new file mode 100644
index 0000000..27c3bf9
--- /dev/null
+++ b/drivers/watchdog/pretimeout_noop.c
@@ -0,0 +1,49 @@
+/*
+ * Copyright (C) 2015 Mentor Graphics
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ */
+
+#include <linux/module.h>
+#include <linux/watchdog.h>
+
+#include <watchdog_pretimeout.h>
+
+/**
+ * pretimeout_noop - No operation on watchdog pretimeout event
+ * @wdd - watchdog_device
+ *
+ * This function prints a message about pretimeout to kernel log.
+ */
+static void pretimeout_noop(struct watchdog_device *wdd)
+{
+	dev_info(wdd->dev, "watchdog pretimeout event\n");
+}
+
+static struct watchdog_governor watchdog_gov_noop = {
+	.name		= "noop",
+	.pretimeout	= pretimeout_noop,
+#ifdef CONFIG_WATCHDOG_PRETIMEOUT_DEFAULT_GOV_NOOP
+	.is_default	= true,
+#endif
+};
+
+static int __init watchdog_gov_noop_register(void)
+{
+	return watchdog_register_governor(&watchdog_gov_noop);
+}
+
+static void __exit watchdog_gov_noop_unregister(void)
+{
+	watchdog_unregister_governor(&watchdog_gov_noop);
+}
+module_init(watchdog_gov_noop_register);
+module_exit(watchdog_gov_noop_unregister);
+
+MODULE_AUTHOR("Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com>");
+MODULE_DESCRIPTION("Noop watchdog pretimeout governor");
+MODULE_LICENSE("GPL");
-- 
2.1.4

[PATCH 3/6] watchdog: pretimeout: add panic pretimeout governor

[Vladimir Zapolskiy]

Panic watchdog pretimeout governor, on watchdog pretimeout event the
kernel shall panic.

Signed-off-by: Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com>
---
 drivers/watchdog/Kconfig            | 12 +++++++++
 drivers/watchdog/Makefile           |  1 +
 drivers/watchdog/pretimeout_panic.c | 49 +++++++++++++++++++++++++++++++++++++
 3 files changed, 62 insertions(+)
 create mode 100644 drivers/watchdog/pretimeout_panic.c

diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
index 4940c3b..6c20765 100644
--- a/drivers/watchdog/Kconfig
+++ b/drivers/watchdog/Kconfig
@@ -1649,6 +1649,12 @@ config WATCHDOG_PRETIMEOUT_DEFAULT_GOV_NOOP
 	help
 	  Use noop watchdog pretimeout governor by default.
 
+config WATCHDOG_PRETIMEOUT_DEFAULT_GOV_PANIC
+	bool "panic"
+	select WATCHDOG_PRETIMEOUT_GOV_PANIC
+	help
+	  Use panic watchdog pretimeout governor by default.
+
 endchoice
 
 config WATCHDOG_PRETIMEOUT_GOV_NOOP
@@ -1657,6 +1663,12 @@ config WATCHDOG_PRETIMEOUT_GOV_NOOP
 	  Noop watchdog pretimeout governor, only an informational
 	  message is added to kernel log buffer.
 
+config WATCHDOG_PRETIMEOUT_GOV_PANIC
+	tristate "Panic watchdog pretimeout governor"
+	help
+	  Panic watchdog pretimeout governor, on watchdog pretimeout
+	  event the kernel shall panic.
+
 endif # WATCHDOG_PRETIMEOUT_GOV
 
 endif # WATCHDOG
diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
index 16a9e32..cdcaa8a 100644
--- a/drivers/watchdog/Makefile
+++ b/drivers/watchdog/Makefile
@@ -9,6 +9,7 @@ watchdog-objs	+= watchdog_core.o watchdog_dev.o
 
 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
 
 # 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_panic.c b/drivers/watchdog/pretimeout_panic.c
new file mode 100644
index 0000000..555de7e
--- /dev/null
+++ b/drivers/watchdog/pretimeout_panic.c
@@ -0,0 +1,49 @@
+/*
+ * Copyright (C) 2015 Mentor Graphics
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ */
+
+#include <linux/module.h>
+#include <linux/watchdog.h>
+
+#include <watchdog_pretimeout.h>
+
+/**
+ * pretimeout_panic - Panic on watchdog pretimeout event
+ * @wdd - watchdog_device
+ *
+ * Panic, watchdog has not been fed till pretimeout event.
+ */
+static void pretimeout_panic(struct watchdog_device *wdd)
+{
+	panic("panic on watchdog pretimeout event\n");
+}
+
+static struct watchdog_governor watchdog_gov_panic = {
+	.name		= "panic",
+	.pretimeout	= pretimeout_panic,
+#ifdef CONFIG_WATCHDOG_PRETIMEOUT_DEFAULT_GOV_PANIC
+	.is_default	= true,
+#endif
+};
+
+static int __init watchdog_gov_panic_register(void)
+{
+	return watchdog_register_governor(&watchdog_gov_panic);
+}
+
+static void __exit watchdog_gov_panic_unregister(void)
+{
+	watchdog_unregister_governor(&watchdog_gov_panic);
+}
+module_init(watchdog_gov_panic_register);
+module_exit(watchdog_gov_panic_unregister);
+
+MODULE_AUTHOR("Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com>");
+MODULE_DESCRIPTION("Panic watchdog pretimeout governor");
+MODULE_LICENSE("GPL");
-- 
2.1.4

[PATCH 4/6] watchdog: pretimeout: add userspace notifier pretimeout governor

[Vladimir Zapolskiy]

Userspace notifier watchdog pretimeout governor, on watchdog
pretimeout event sends a notification to userspace for further
handling.

Signed-off-by: Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com>
---
 drivers/watchdog/Kconfig                | 14 ++++++++++
 drivers/watchdog/Makefile               |  1 +
 drivers/watchdog/pretimeout_userspace.c | 49 +++++++++++++++++++++++++++++++++
 3 files changed, 64 insertions(+)
 create mode 100644 drivers/watchdog/pretimeout_userspace.c

diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
index 6c20765..7e9e2bb 100644
--- a/drivers/watchdog/Kconfig
+++ b/drivers/watchdog/Kconfig
@@ -1655,6 +1655,13 @@ config WATCHDOG_PRETIMEOUT_DEFAULT_GOV_PANIC
 	help
 	  Use panic watchdog pretimeout governor by default.
 
+config WATCHDOG_PRETIMEOUT_DEFAULT_GOV_USERSPACE
+	bool "userspace"
+	select WATCHDOG_PRETIMEOUT_GOV_USERSPACE
+	help
+	  Use userspace notifier watchdog pretimeout governor
+	  by default.
+
 endchoice
 
 config WATCHDOG_PRETIMEOUT_GOV_NOOP
@@ -1669,6 +1676,13 @@ config WATCHDOG_PRETIMEOUT_GOV_PANIC
 	  Panic watchdog pretimeout governor, on watchdog pretimeout
 	  event the kernel shall panic.
 
+config WATCHDOG_PRETIMEOUT_GOV_USERSPACE
+	tristate "Userspace notifier watchdog pretimeout governor"
+	help
+	  Userspace notifier watchdog pretimeout governor, on watchdog
+	  pretimeout event send a notification to userspace for
+	  further handling.
+
 endif # WATCHDOG_PRETIMEOUT_GOV
 
 endif # WATCHDOG
diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
index cdcaa8a..7d6755b 100644
--- a/drivers/watchdog/Makefile
+++ b/drivers/watchdog/Makefile
@@ -10,6 +10,7 @@ watchdog-objs	+= watchdog_core.o watchdog_dev.o
 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_USERSPACE)	+= pretimeout_userspace.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_userspace.c b/drivers/watchdog/pretimeout_userspace.c
new file mode 100644
index 0000000..ff3c9e7
--- /dev/null
+++ b/drivers/watchdog/pretimeout_userspace.c
@@ -0,0 +1,49 @@
+/*
+ * Copyright (C) 2015 Mentor Graphics
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ */
+
+#include <linux/module.h>
+#include <linux/watchdog.h>
+
+#include <watchdog_pretimeout.h>
+
+/**
+ * pretimeout_userspace - Notify userspace on watchdog pretimeout event
+ * @wdd - watchdog_device
+ *
+ * Send watchdog device uevent to userspace to handle pretimeout event
+ */
+static void pretimeout_userspace(struct watchdog_device *wdd)
+{
+	kobject_uevent(&wdd->dev->kobj, KOBJ_CHANGE);
+}
+
+static struct watchdog_governor watchdog_gov_userspace = {
+	.name		= "userspace",
+	.pretimeout	= pretimeout_userspace,
+#ifdef CONFIG_WATCHDOG_PRETIMEOUT_DEFAULT_GOV_USERSPACE
+	.is_default	= true,
+#endif
+};
+
+static int __init watchdog_gov_userspace_register(void)
+{
+	return watchdog_register_governor(&watchdog_gov_userspace);
+}
+
+static void __exit watchdog_gov_userspace_unregister(void)
+{
+	watchdog_unregister_governor(&watchdog_gov_userspace);
+}
+module_init(watchdog_gov_userspace_register);
+module_exit(watchdog_gov_userspace_unregister);
+
+MODULE_AUTHOR("Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com>");
+MODULE_DESCRIPTION("Userspace notifier watchdog pretimeout governor");
+MODULE_LICENSE("GPL");
-- 
2.1.4

[PATCH 5/6] watchdog: pretimeout: add device specific notifier pretimeout governor

[Vladimir Zapolskiy]

Device watchdog pretimeout governor sends a notification back to a
watchdog device for further handling. This governor does nothing, if
watchdog driver does not define a pretimeout callback.

Signed-off-by: Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com>
---
 drivers/watchdog/Kconfig             | 15 +++++++++++
 drivers/watchdog/Makefile            |  1 +
 drivers/watchdog/pretimeout_device.c | 49 ++++++++++++++++++++++++++++++++++++
 include/linux/watchdog.h             |  2 ++
 4 files changed, 67 insertions(+)
 create mode 100644 drivers/watchdog/pretimeout_device.c

diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
index 7e9e2bb..6c1f7e1 100644
--- a/drivers/watchdog/Kconfig
+++ b/drivers/watchdog/Kconfig
@@ -1662,6 +1662,13 @@ config WATCHDOG_PRETIMEOUT_DEFAULT_GOV_USERSPACE
 	  Use userspace notifier watchdog pretimeout governor
 	  by default.
 
+config WATCHDOG_PRETIMEOUT_DEFAULT_GOV_DEVICE
+	bool "device"
+	select WATCHDOG_PRETIMEOUT_GOV_DEVICE
+	help
+	  Use device specific watchdog pretimeout event handler
+	  by default.
+
 endchoice
 
 config WATCHDOG_PRETIMEOUT_GOV_NOOP
@@ -1683,6 +1690,14 @@ config WATCHDOG_PRETIMEOUT_GOV_USERSPACE
 	  pretimeout event send a notification to userspace for
 	  further handling.
 
+config WATCHDOG_PRETIMEOUT_GOV_DEVICE
+	tristate "Own device watchdog pretimeout governor"
+	help
+	  Device watchdog pretimeout governor sends a notification
+	  back to a watchdog device for further handling. This governor
+	  does nothing, if watchdog driver does not define a pretimeout
+	  callback.
+
 endif # WATCHDOG_PRETIMEOUT_GOV
 
 endif # WATCHDOG
diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
index 7d6755b..0717b64 100644
--- a/drivers/watchdog/Makefile
+++ b/drivers/watchdog/Makefile
@@ -11,6 +11,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_USERSPACE)	+= pretimeout_userspace.o
+obj-$(CONFIG_WATCHDOG_PRETIMEOUT_GOV_DEVICE)	+= pretimeout_device.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_device.c b/drivers/watchdog/pretimeout_device.c
new file mode 100644
index 0000000..4ce992b
--- /dev/null
+++ b/drivers/watchdog/pretimeout_device.c
@@ -0,0 +1,49 @@
+/*
+ * Copyright (C) 2015 Mentor Graphics
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ */
+
+#include <linux/module.h>
+#include <linux/watchdog.h>
+
+#include <watchdog_pretimeout.h>
+
+/**
+ * pretimeout_device - Run device specific handler on watchdog pretimeout event
+ * @wdd - watchdog_device
+ *
+ */
+static void pretimeout_device(struct watchdog_device *wdd)
+{
+	if (wdd->ops->pretimeout)
+		wdd->ops->pretimeout(wdd);
+}
+
+static struct watchdog_governor watchdog_gov_device = {
+	.name		= "device",
+	.pretimeout	= pretimeout_device,
+#ifdef CONFIG_WATCHDOG_PRETIMEOUT_DEFAULT_GOV_DEVICE
+	.is_default	= true,
+#endif
+};
+
+static int __init watchdog_gov_device_register(void)
+{
+	return watchdog_register_governor(&watchdog_gov_device);
+}
+
+static void __exit watchdog_gov_device_unregister(void)
+{
+	watchdog_unregister_governor(&watchdog_gov_device);
+}
+module_init(watchdog_gov_device_register);
+module_exit(watchdog_gov_device_unregister);
+
+MODULE_AUTHOR("Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com>");
+MODULE_DESCRIPTION("Device specific watchdog pretimeout governor");
+MODULE_LICENSE("GPL");
diff --git a/include/linux/watchdog.h b/include/linux/watchdog.h
index 960223e..597dcfb 100644
--- a/include/linux/watchdog.h
+++ b/include/linux/watchdog.h
@@ -27,6 +27,7 @@ struct watchdog_governor;
  * @status:	The routine that shows the status of the watchdog device.
  * @set_timeout:The routine for setting the watchdog devices timeout value (in seconds).
  * @get_timeleft:The routine that gets the time left before a reset (in seconds).
+ * @pretimeout: The routine that runs driver specific handler of pretimeout event.
  * @ref:	The ref operation for dyn. allocated watchdog_device structs
  * @unref:	The unref operation for dyn. allocated watchdog_device structs
  * @ioctl:	The routines that handles extra ioctl calls.
@@ -46,6 +47,7 @@ struct watchdog_ops {
 	unsigned int (*status)(struct watchdog_device *);
 	int (*set_timeout)(struct watchdog_device *, unsigned int);
 	unsigned int (*get_timeleft)(struct watchdog_device *);
+	void (*pretimeout)(struct watchdog_device *);
 	void (*ref)(struct watchdog_device *);
 	void (*unref)(struct watchdog_device *);
 	long (*ioctl)(struct watchdog_device *, unsigned int, unsigned long);
-- 
2.1.4

Re: [PATCH 5/6] watchdog: pretimeout: add device specific notifier pretimeout governor

[Guenter Roeck]

On 11/20/2015 11:11 PM, Vladimir Zapolskiy wrote:
> Device watchdog pretimeout governor sends a notification back to a
> watchdog device for further handling. This governor does nothing, if
> watchdog driver does not define a pretimeout callback.
>

I can not make up my mind - should this be "device" or "driver" specific ?

Guenter

> Signed-off-by: Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com>
> ---
>   drivers/watchdog/Kconfig             | 15 +++++++++++
>   drivers/watchdog/Makefile            |  1 +
>   drivers/watchdog/pretimeout_device.c | 49 ++++++++++++++++++++++++++++++++++++
>   include/linux/watchdog.h             |  2 ++
>   4 files changed, 67 insertions(+)
>   create mode 100644 drivers/watchdog/pretimeout_device.c
>
> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
> index 7e9e2bb..6c1f7e1 100644
> --- a/drivers/watchdog/Kconfig
> +++ b/drivers/watchdog/Kconfig
> @@ -1662,6 +1662,13 @@ config WATCHDOG_PRETIMEOUT_DEFAULT_GOV_USERSPACE
>   	  Use userspace notifier watchdog pretimeout governor
>   	  by default.
>
> +config WATCHDOG_PRETIMEOUT_DEFAULT_GOV_DEVICE
> +	bool "device"
> +	select WATCHDOG_PRETIMEOUT_GOV_DEVICE
> +	help
> +	  Use device specific watchdog pretimeout event handler
> +	  by default.
> +
>   endchoice
>
>   config WATCHDOG_PRETIMEOUT_GOV_NOOP
> @@ -1683,6 +1690,14 @@ config WATCHDOG_PRETIMEOUT_GOV_USERSPACE
>   	  pretimeout event send a notification to userspace for
>   	  further handling.
>
> +config WATCHDOG_PRETIMEOUT_GOV_DEVICE
> +	tristate "Own device watchdog pretimeout governor"
> +	help
> +	  Device watchdog pretimeout governor sends a notification
> +	  back to a watchdog device for further handling. This governor
> +	  does nothing, if watchdog driver does not define a pretimeout
> +	  callback.
> +
>   endif # WATCHDOG_PRETIMEOUT_GOV
>
>   endif # WATCHDOG
> diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
> index 7d6755b..0717b64 100644
> --- a/drivers/watchdog/Makefile
> +++ b/drivers/watchdog/Makefile
> @@ -11,6 +11,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_USERSPACE)	+= pretimeout_userspace.o
> +obj-$(CONFIG_WATCHDOG_PRETIMEOUT_GOV_DEVICE)	+= pretimeout_device.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_device.c b/drivers/watchdog/pretimeout_device.c
> new file mode 100644
> index 0000000..4ce992b
> --- /dev/null
> +++ b/drivers/watchdog/pretimeout_device.c
> @@ -0,0 +1,49 @@
> +/*
> + * Copyright (C) 2015 Mentor Graphics
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + */
> +
> +#include <linux/module.h>
> +#include <linux/watchdog.h>
> +
> +#include <watchdog_pretimeout.h>
> +
> +/**
> + * pretimeout_device - Run device specific handler on watchdog pretimeout event
> + * @wdd - watchdog_device
> + *
> + */
> +static void pretimeout_device(struct watchdog_device *wdd)
> +{
> +	if (wdd->ops->pretimeout)
> +		wdd->ops->pretimeout(wdd);
> +}
> +
> +static struct watchdog_governor watchdog_gov_device = {
> +	.name		= "device",
> +	.pretimeout	= pretimeout_device,
> +#ifdef CONFIG_WATCHDOG_PRETIMEOUT_DEFAULT_GOV_DEVICE
> +	.is_default	= true,
> +#endif
> +};
> +
> +static int __init watchdog_gov_device_register(void)
> +{
> +	return watchdog_register_governor(&watchdog_gov_device);
> +}
> +
> +static void __exit watchdog_gov_device_unregister(void)
> +{
> +	watchdog_unregister_governor(&watchdog_gov_device);
> +}
> +module_init(watchdog_gov_device_register);
> +module_exit(watchdog_gov_device_unregister);
> +
> +MODULE_AUTHOR("Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com>");
> +MODULE_DESCRIPTION("Device specific watchdog pretimeout governor");
> +MODULE_LICENSE("GPL");
> diff --git a/include/linux/watchdog.h b/include/linux/watchdog.h
> index 960223e..597dcfb 100644
> --- a/include/linux/watchdog.h
> +++ b/include/linux/watchdog.h
> @@ -27,6 +27,7 @@ struct watchdog_governor;
>    * @status:	The routine that shows the status of the watchdog device.
>    * @set_timeout:The routine for setting the watchdog devices timeout value (in seconds).
>    * @get_timeleft:The routine that gets the time left before a reset (in seconds).
> + * @pretimeout: The routine that runs driver specific handler of pretimeout event.
>    * @ref:	The ref operation for dyn. allocated watchdog_device structs
>    * @unref:	The unref operation for dyn. allocated watchdog_device structs
>    * @ioctl:	The routines that handles extra ioctl calls.
> @@ -46,6 +47,7 @@ struct watchdog_ops {
>   	unsigned int (*status)(struct watchdog_device *);
>   	int (*set_timeout)(struct watchdog_device *, unsigned int);
>   	unsigned int (*get_timeleft)(struct watchdog_device *);
> +	void (*pretimeout)(struct watchdog_device *);
>   	void (*ref)(struct watchdog_device *);
>   	void (*unref)(struct watchdog_device *);
>   	long (*ioctl)(struct watchdog_device *, unsigned int, unsigned long);
>

Re: [PATCH 5/6] watchdog: pretimeout: add device specific notifier pretimeout governor

[Vladimir Zapolskiy]

Hi Guenter,

On 24.11.2015 08:37, Guenter Roeck wrote:
> On 11/20/2015 11:11 PM, Vladimir Zapolskiy wrote:
>> Device watchdog pretimeout governor sends a notification back to a
>> watchdog device for further handling. This governor does nothing, if
>> watchdog driver does not define a pretimeout callback.
>>
> 
> I can not make up my mind - should this be "device" or "driver" specific ?

I was hesitating to select between "device" and "driver", technically
"driver" should be more appropriate, but I'm not aware of users' accepted
terminology, I believe ordinary users more often operate with "devices"
rather than "drivers".

Anyway it is not sufficient for me, I can rename it, if you ask.

Best wishes,
Vladimir

>> Signed-off-by: Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com>
>> ---
>>   drivers/watchdog/Kconfig             | 15 +++++++++++
>>   drivers/watchdog/Makefile            |  1 +
>>   drivers/watchdog/pretimeout_device.c | 49 ++++++++++++++++++++++++++++++++++++
>>   include/linux/watchdog.h             |  2 ++
>>   4 files changed, 67 insertions(+)
>>   create mode 100644 drivers/watchdog/pretimeout_device.c
>>
>> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
>> index 7e9e2bb..6c1f7e1 100644
>> --- a/drivers/watchdog/Kconfig
>> +++ b/drivers/watchdog/Kconfig
>> @@ -1662,6 +1662,13 @@ config WATCHDOG_PRETIMEOUT_DEFAULT_GOV_USERSPACE
>>   	  Use userspace notifier watchdog pretimeout governor
>>   	  by default.
>>
>> +config WATCHDOG_PRETIMEOUT_DEFAULT_GOV_DEVICE
>> +	bool "device"
>> +	select WATCHDOG_PRETIMEOUT_GOV_DEVICE
>> +	help
>> +	  Use device specific watchdog pretimeout event handler
>> +	  by default.
>> +
>>   endchoice
>>
>>   config WATCHDOG_PRETIMEOUT_GOV_NOOP
>> @@ -1683,6 +1690,14 @@ config WATCHDOG_PRETIMEOUT_GOV_USERSPACE
>>   	  pretimeout event send a notification to userspace for
>>   	  further handling.
>>
>> +config WATCHDOG_PRETIMEOUT_GOV_DEVICE
>> +	tristate "Own device watchdog pretimeout governor"
>> +	help
>> +	  Device watchdog pretimeout governor sends a notification
>> +	  back to a watchdog device for further handling. This governor
>> +	  does nothing, if watchdog driver does not define a pretimeout
>> +	  callback.
>> +
>>   endif # WATCHDOG_PRETIMEOUT_GOV
>>
>>   endif # WATCHDOG
>> diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
>> index 7d6755b..0717b64 100644
>> --- a/drivers/watchdog/Makefile
>> +++ b/drivers/watchdog/Makefile
>> @@ -11,6 +11,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_USERSPACE)	+= pretimeout_userspace.o
>> +obj-$(CONFIG_WATCHDOG_PRETIMEOUT_GOV_DEVICE)	+= pretimeout_device.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_device.c b/drivers/watchdog/pretimeout_device.c
>> new file mode 100644
>> index 0000000..4ce992b
>> --- /dev/null
>> +++ b/drivers/watchdog/pretimeout_device.c
>> @@ -0,0 +1,49 @@
>> +/*
>> + * Copyright (C) 2015 Mentor Graphics
>> + *
>> + * This program is free software; you can redistribute it and/or modify
>> + * it under the terms of the GNU General Public License as published by
>> + * the Free Software Foundation; either version 2 of the License, or
>> + * (at your option) any later version.
>> + *
>> + */
>> +
>> +#include <linux/module.h>
>> +#include <linux/watchdog.h>
>> +
>> +#include <watchdog_pretimeout.h>
>> +
>> +/**
>> + * pretimeout_device - Run device specific handler on watchdog pretimeout event
>> + * @wdd - watchdog_device
>> + *
>> + */
>> +static void pretimeout_device(struct watchdog_device *wdd)
>> +{
>> +	if (wdd->ops->pretimeout)
>> +		wdd->ops->pretimeout(wdd);
>> +}
>> +
>> +static struct watchdog_governor watchdog_gov_device = {
>> +	.name		= "device",
>> +	.pretimeout	= pretimeout_device,
>> +#ifdef CONFIG_WATCHDOG_PRETIMEOUT_DEFAULT_GOV_DEVICE
>> +	.is_default	= true,
>> +#endif
>> +};
>> +
>> +static int __init watchdog_gov_device_register(void)
>> +{
>> +	return watchdog_register_governor(&watchdog_gov_device);
>> +}
>> +
>> +static void __exit watchdog_gov_device_unregister(void)
>> +{
>> +	watchdog_unregister_governor(&watchdog_gov_device);
>> +}
>> +module_init(watchdog_gov_device_register);
>> +module_exit(watchdog_gov_device_unregister);
>> +
>> +MODULE_AUTHOR("Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com>");
>> +MODULE_DESCRIPTION("Device specific watchdog pretimeout governor");
>> +MODULE_LICENSE("GPL");
>> diff --git a/include/linux/watchdog.h b/include/linux/watchdog.h
>> index 960223e..597dcfb 100644
>> --- a/include/linux/watchdog.h
>> +++ b/include/linux/watchdog.h
>> @@ -27,6 +27,7 @@ struct watchdog_governor;
>>    * @status:	The routine that shows the status of the watchdog device.
>>    * @set_timeout:The routine for setting the watchdog devices timeout value (in seconds).
>>    * @get_timeleft:The routine that gets the time left before a reset (in seconds).
>> + * @pretimeout: The routine that runs driver specific handler of pretimeout event.
>>    * @ref:	The ref operation for dyn. allocated watchdog_device structs
>>    * @unref:	The unref operation for dyn. allocated watchdog_device structs
>>    * @ioctl:	The routines that handles extra ioctl calls.
>> @@ -46,6 +47,7 @@ struct watchdog_ops {
>>   	unsigned int (*status)(struct watchdog_device *);
>>   	int (*set_timeout)(struct watchdog_device *, unsigned int);
>>   	unsigned int (*get_timeleft)(struct watchdog_device *);
>> +	void (*pretimeout)(struct watchdog_device *);
>>   	void (*ref)(struct watchdog_device *);
>>   	void (*unref)(struct watchdog_device *);
>>   	long (*ioctl)(struct watchdog_device *, unsigned int, unsigned long);
>>
> 

Re: [PATCH 5/6] watchdog: pretimeout: add device specific notifier pretimeout governor

[Guenter Roeck]

Hi Vladimir,

On 11/24/2015 05:25 AM, Vladimir Zapolskiy wrote:
> Hi Guenter,
>
> On 24.11.2015 08:37, Guenter Roeck wrote:
>> On 11/20/2015 11:11 PM, Vladimir Zapolskiy wrote:
>>> Device watchdog pretimeout governor sends a notification back to a
>>> watchdog device for further handling. This governor does nothing, if
>>> watchdog driver does not define a pretimeout callback.
>>>
>>
>> I can not make up my mind - should this be "device" or "driver" specific ?
>
> I was hesitating to select between "device" and "driver", technically
> "driver" should be more appropriate, but I'm not aware of users' accepted
> terminology, I believe ordinary users more often operate with "devices"
> rather than "drivers".
>
> Anyway it is not sufficient for me, I can rename it, if you ask.
>

Another one of those questions where additional input would help.

Thanks,
Guenter

> Best wishes,
> Vladimir
>
>>> Signed-off-by: Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com>
>>> ---
>>>    drivers/watchdog/Kconfig             | 15 +++++++++++
>>>    drivers/watchdog/Makefile            |  1 +
>>>    drivers/watchdog/pretimeout_device.c | 49 ++++++++++++++++++++++++++++++++++++
>>>    include/linux/watchdog.h             |  2 ++
>>>    4 files changed, 67 insertions(+)
>>>    create mode 100644 drivers/watchdog/pretimeout_device.c
>>>
>>> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
>>> index 7e9e2bb..6c1f7e1 100644
>>> --- a/drivers/watchdog/Kconfig
>>> +++ b/drivers/watchdog/Kconfig
>>> @@ -1662,6 +1662,13 @@ config WATCHDOG_PRETIMEOUT_DEFAULT_GOV_USERSPACE
>>>    	  Use userspace notifier watchdog pretimeout governor
>>>    	  by default.
>>>
>>> +config WATCHDOG_PRETIMEOUT_DEFAULT_GOV_DEVICE
>>> +	bool "device"
>>> +	select WATCHDOG_PRETIMEOUT_GOV_DEVICE
>>> +	help
>>> +	  Use device specific watchdog pretimeout event handler
>>> +	  by default.
>>> +
>>>    endchoice
>>>
>>>    config WATCHDOG_PRETIMEOUT_GOV_NOOP
>>> @@ -1683,6 +1690,14 @@ config WATCHDOG_PRETIMEOUT_GOV_USERSPACE
>>>    	  pretimeout event send a notification to userspace for
>>>    	  further handling.
>>>
>>> +config WATCHDOG_PRETIMEOUT_GOV_DEVICE
>>> +	tristate "Own device watchdog pretimeout governor"
>>> +	help
>>> +	  Device watchdog pretimeout governor sends a notification
>>> +	  back to a watchdog device for further handling. This governor
>>> +	  does nothing, if watchdog driver does not define a pretimeout
>>> +	  callback.
>>> +
>>>    endif # WATCHDOG_PRETIMEOUT_GOV
>>>
>>>    endif # WATCHDOG
>>> diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
>>> index 7d6755b..0717b64 100644
>>> --- a/drivers/watchdog/Makefile
>>> +++ b/drivers/watchdog/Makefile
>>> @@ -11,6 +11,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_USERSPACE)	+= pretimeout_userspace.o
>>> +obj-$(CONFIG_WATCHDOG_PRETIMEOUT_GOV_DEVICE)	+= pretimeout_device.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_device.c b/drivers/watchdog/pretimeout_device.c
>>> new file mode 100644
>>> index 0000000..4ce992b
>>> --- /dev/null
>>> +++ b/drivers/watchdog/pretimeout_device.c
>>> @@ -0,0 +1,49 @@
>>> +/*
>>> + * Copyright (C) 2015 Mentor Graphics
>>> + *
>>> + * This program is free software; you can redistribute it and/or modify
>>> + * it under the terms of the GNU General Public License as published by
>>> + * the Free Software Foundation; either version 2 of the License, or
>>> + * (at your option) any later version.
>>> + *
>>> + */
>>> +
>>> +#include <linux/module.h>
>>> +#include <linux/watchdog.h>
>>> +
>>> +#include <watchdog_pretimeout.h>
>>> +
>>> +/**
>>> + * pretimeout_device - Run device specific handler on watchdog pretimeout event
>>> + * @wdd - watchdog_device
>>> + *
>>> + */
>>> +static void pretimeout_device(struct watchdog_device *wdd)
>>> +{
>>> +	if (wdd->ops->pretimeout)
>>> +		wdd->ops->pretimeout(wdd);
>>> +}
>>> +
>>> +static struct watchdog_governor watchdog_gov_device = {
>>> +	.name		= "device",
>>> +	.pretimeout	= pretimeout_device,
>>> +#ifdef CONFIG_WATCHDOG_PRETIMEOUT_DEFAULT_GOV_DEVICE
>>> +	.is_default	= true,
>>> +#endif
>>> +};
>>> +
>>> +static int __init watchdog_gov_device_register(void)
>>> +{
>>> +	return watchdog_register_governor(&watchdog_gov_device);
>>> +}
>>> +
>>> +static void __exit watchdog_gov_device_unregister(void)
>>> +{
>>> +	watchdog_unregister_governor(&watchdog_gov_device);
>>> +}
>>> +module_init(watchdog_gov_device_register);
>>> +module_exit(watchdog_gov_device_unregister);
>>> +
>>> +MODULE_AUTHOR("Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com>");
>>> +MODULE_DESCRIPTION("Device specific watchdog pretimeout governor");
>>> +MODULE_LICENSE("GPL");
>>> diff --git a/include/linux/watchdog.h b/include/linux/watchdog.h
>>> index 960223e..597dcfb 100644
>>> --- a/include/linux/watchdog.h
>>> +++ b/include/linux/watchdog.h
>>> @@ -27,6 +27,7 @@ struct watchdog_governor;
>>>     * @status:	The routine that shows the status of the watchdog device.
>>>     * @set_timeout:The routine for setting the watchdog devices timeout value (in seconds).
>>>     * @get_timeleft:The routine that gets the time left before a reset (in seconds).
>>> + * @pretimeout: The routine that runs driver specific handler of pretimeout event.
>>>     * @ref:	The ref operation for dyn. allocated watchdog_device structs
>>>     * @unref:	The unref operation for dyn. allocated watchdog_device structs
>>>     * @ioctl:	The routines that handles extra ioctl calls.
>>> @@ -46,6 +47,7 @@ struct watchdog_ops {
>>>    	unsigned int (*status)(struct watchdog_device *);
>>>    	int (*set_timeout)(struct watchdog_device *, unsigned int);
>>>    	unsigned int (*get_timeleft)(struct watchdog_device *);
>>> +	void (*pretimeout)(struct watchdog_device *);
>>>    	void (*ref)(struct watchdog_device *);
>>>    	void (*unref)(struct watchdog_device *);
>>>    	long (*ioctl)(struct watchdog_device *, unsigned int, unsigned long);
>>>
>>
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

[PATCH 6/6] watchdog: pretimeout: add ping watchdog pretimeout governor

[Vladimir Zapolskiy]

Ping watchdog pretimeout governor, on watchdog pretimeout event ping
watchdog device. Use this watchdog pretimeout governor with caution,
it may humiliate watchdog work, however it may be helpful in some
particular situations, for instance if once started watchdog can not
be stopped, but reboot caused by the watchdog is undesired.

Signed-off-by: Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com>
---
 drivers/watchdog/Kconfig           | 16 +++++++++++++
 drivers/watchdog/Makefile          |  1 +
 drivers/watchdog/pretimeout_ping.c | 48 ++++++++++++++++++++++++++++++++++++++
 3 files changed, 65 insertions(+)
 create mode 100644 drivers/watchdog/pretimeout_ping.c

diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
index 6c1f7e1..cb0885a 100644
--- a/drivers/watchdog/Kconfig
+++ b/drivers/watchdog/Kconfig
@@ -1669,6 +1669,12 @@ config WATCHDOG_PRETIMEOUT_DEFAULT_GOV_DEVICE
 	  Use device specific watchdog pretimeout event handler
 	  by default.
 
+config WATCHDOG_PRETIMEOUT_DEFAULT_GOV_PING
+	bool "ping"
+	select WATCHDOG_PRETIMEOUT_GOV_PING
+	help
+	  Use ping watchdog pretimeout event handler by default.
+
 endchoice
 
 config WATCHDOG_PRETIMEOUT_GOV_NOOP
@@ -1698,6 +1704,16 @@ config WATCHDOG_PRETIMEOUT_GOV_DEVICE
 	  does nothing, if watchdog driver does not define a pretimeout
 	  callback.
 
+config WATCHDOG_PRETIMEOUT_GOV_PING
+	tristate "Ping watchdog pretimeout governor"
+	help
+	  Ping watchdog pretimeout governor, on watchdog pretimeout
+	  event ping watchdog device. Use this watchdog pretimeout
+	  governor with caution, it may humiliate watchdog work,
+	  however it may be helpful in some particular situations,
+	  for instance if once started watchdog can not be stopped,
+	  but reboot caused by the watchdog is undesired.
+
 endif # WATCHDOG_PRETIMEOUT_GOV
 
 endif # WATCHDOG
diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
index 0717b64..bbf6af8 100644
--- a/drivers/watchdog/Makefile
+++ b/drivers/watchdog/Makefile
@@ -12,6 +12,7 @@ obj-$(CONFIG_WATCHDOG_PRETIMEOUT_GOV_NOOP)	+= pretimeout_noop.o
 obj-$(CONFIG_WATCHDOG_PRETIMEOUT_GOV_PANIC)	+= pretimeout_panic.o
 obj-$(CONFIG_WATCHDOG_PRETIMEOUT_GOV_USERSPACE)	+= pretimeout_userspace.o
 obj-$(CONFIG_WATCHDOG_PRETIMEOUT_GOV_DEVICE)	+= pretimeout_device.o
+obj-$(CONFIG_WATCHDOG_PRETIMEOUT_GOV_PING)	+= pretimeout_ping.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_ping.c b/drivers/watchdog/pretimeout_ping.c
new file mode 100644
index 0000000..0d55205
--- /dev/null
+++ b/drivers/watchdog/pretimeout_ping.c
@@ -0,0 +1,48 @@
+/*
+ * Copyright (C) 2015 Mentor Graphics
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License as published by
+ * the Free Software Foundation; either version 2 of the License, or
+ * (at your option) any later version.
+ *
+ */
+
+#include <linux/module.h>
+#include <linux/watchdog.h>
+
+#include <watchdog_pretimeout.h>
+
+/**
+ * pretimeout_ping - Ping watchdog on pretimeout event
+ * @wdd - watchdog_device
+ *
+ */
+static void pretimeout_ping(struct watchdog_device *wdd)
+{
+	wdd->ops->ping(wdd);
+}
+
+static struct watchdog_governor watchdog_gov_ping = {
+	.name		= "ping",
+	.pretimeout	= pretimeout_ping,
+#ifdef CONFIG_WATCHDOG_PRETIMEOUT_DEFAULT_GOV_PING
+	.is_default	= true,
+#endif
+};
+
+static int __init watchdog_gov_ping_register(void)
+{
+	return watchdog_register_governor(&watchdog_gov_ping);
+}
+
+static void __exit watchdog_gov_ping_unregister(void)
+{
+	watchdog_unregister_governor(&watchdog_gov_ping);
+}
+module_init(watchdog_gov_ping_register);
+module_exit(watchdog_gov_ping_unregister);
+
+MODULE_AUTHOR("Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com>");
+MODULE_DESCRIPTION("Ping watchdog pretimeout governor");
+MODULE_LICENSE("GPL");
-- 
2.1.4

Re: [PATCH 6/6] watchdog: pretimeout: add ping watchdog pretimeout governor

[Guenter Roeck]

On 11/20/2015 11:11 PM, Vladimir Zapolskiy wrote:
> Ping watchdog pretimeout governor, on watchdog pretimeout event ping
> watchdog device. Use this watchdog pretimeout governor with caution,
> it may humiliate watchdog work, however it may be helpful in some
> particular situations, for instance if once started watchdog can not
> be stopped, but reboot caused by the watchdog is undesired.
>
> Signed-off-by: Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com>
> ---
>   drivers/watchdog/Kconfig           | 16 +++++++++++++
>   drivers/watchdog/Makefile          |  1 +
>   drivers/watchdog/pretimeout_ping.c | 48 ++++++++++++++++++++++++++++++++++++++
>   3 files changed, 65 insertions(+)
>   create mode 100644 drivers/watchdog/pretimeout_ping.c
>
> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
> index 6c1f7e1..cb0885a 100644
> --- a/drivers/watchdog/Kconfig
> +++ b/drivers/watchdog/Kconfig
> @@ -1669,6 +1669,12 @@ config WATCHDOG_PRETIMEOUT_DEFAULT_GOV_DEVICE
>   	  Use device specific watchdog pretimeout event handler
>   	  by default.
>
> +config WATCHDOG_PRETIMEOUT_DEFAULT_GOV_PING
> +	bool "ping"
> +	select WATCHDOG_PRETIMEOUT_GOV_PING
> +	help
> +	  Use ping watchdog pretimeout event handler by default.
> +
>   endchoice
>
>   config WATCHDOG_PRETIMEOUT_GOV_NOOP
> @@ -1698,6 +1704,16 @@ config WATCHDOG_PRETIMEOUT_GOV_DEVICE
>   	  does nothing, if watchdog driver does not define a pretimeout
>   	  callback.
>
> +config WATCHDOG_PRETIMEOUT_GOV_PING
> +	tristate "Ping watchdog pretimeout governor"
> +	help
> +	  Ping watchdog pretimeout governor, on watchdog pretimeout
> +	  event ping watchdog device. Use this watchdog pretimeout
> +	  governor with caution, it may humiliate watchdog work,
> +	  however it may be helpful in some particular situations,
> +	  for instance if once started watchdog can not be stopped,
> +	  but reboot caused by the watchdog is undesired.
> +

This one will be unnecessary once the pending infrastructure enhancements
are accepted. The watchdog core will then handle the ping.
I don't think this should be left to user space, as it _would_ defeat
the watchdog's purpose if the watchdog application fails to ping
the watchdog.

Thanks,
Guenter


>   endif # WATCHDOG_PRETIMEOUT_GOV
>
>   endif # WATCHDOG
> diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
> index 0717b64..bbf6af8 100644
> --- a/drivers/watchdog/Makefile
> +++ b/drivers/watchdog/Makefile
> @@ -12,6 +12,7 @@ obj-$(CONFIG_WATCHDOG_PRETIMEOUT_GOV_NOOP)	+= pretimeout_noop.o
>   obj-$(CONFIG_WATCHDOG_PRETIMEOUT_GOV_PANIC)	+= pretimeout_panic.o
>   obj-$(CONFIG_WATCHDOG_PRETIMEOUT_GOV_USERSPACE)	+= pretimeout_userspace.o
>   obj-$(CONFIG_WATCHDOG_PRETIMEOUT_GOV_DEVICE)	+= pretimeout_device.o
> +obj-$(CONFIG_WATCHDOG_PRETIMEOUT_GOV_PING)	+= pretimeout_ping.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_ping.c b/drivers/watchdog/pretimeout_ping.c
> new file mode 100644
> index 0000000..0d55205
> --- /dev/null
> +++ b/drivers/watchdog/pretimeout_ping.c
> @@ -0,0 +1,48 @@
> +/*
> + * Copyright (C) 2015 Mentor Graphics
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation; either version 2 of the License, or
> + * (at your option) any later version.
> + *
> + */
> +
> +#include <linux/module.h>
> +#include <linux/watchdog.h>
> +
> +#include <watchdog_pretimeout.h>
> +
> +/**
> + * pretimeout_ping - Ping watchdog on pretimeout event
> + * @wdd - watchdog_device
> + *
> + */
> +static void pretimeout_ping(struct watchdog_device *wdd)
> +{
> +	wdd->ops->ping(wdd);
> +}
> +
> +static struct watchdog_governor watchdog_gov_ping = {
> +	.name		= "ping",
> +	.pretimeout	= pretimeout_ping,
> +#ifdef CONFIG_WATCHDOG_PRETIMEOUT_DEFAULT_GOV_PING
> +	.is_default	= true,
> +#endif
> +};
> +
> +static int __init watchdog_gov_ping_register(void)
> +{
> +	return watchdog_register_governor(&watchdog_gov_ping);
> +}
> +
> +static void __exit watchdog_gov_ping_unregister(void)
> +{
> +	watchdog_unregister_governor(&watchdog_gov_ping);
> +}
> +module_init(watchdog_gov_ping_register);
> +module_exit(watchdog_gov_ping_unregister);
> +
> +MODULE_AUTHOR("Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com>");
> +MODULE_DESCRIPTION("Ping watchdog pretimeout governor");
> +MODULE_LICENSE("GPL");
>

Re: [PATCH 6/6] watchdog: pretimeout: add ping watchdog pretimeout governor

[Vladimir Zapolskiy]

On 24.11.2015 08:36, Guenter Roeck wrote:
> On 11/20/2015 11:11 PM, Vladimir Zapolskiy wrote:
>> Ping watchdog pretimeout governor, on watchdog pretimeout event ping
>> watchdog device. Use this watchdog pretimeout governor with caution,
>> it may humiliate watchdog work, however it may be helpful in some
>> particular situations, for instance if once started watchdog can not
>> be stopped, but reboot caused by the watchdog is undesired.
>>
>> Signed-off-by: Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com>
>> ---
>>   drivers/watchdog/Kconfig           | 16 +++++++++++++
>>   drivers/watchdog/Makefile          |  1 +
>>   drivers/watchdog/pretimeout_ping.c | 48 ++++++++++++++++++++++++++++++++++++++
>>   3 files changed, 65 insertions(+)
>>   create mode 100644 drivers/watchdog/pretimeout_ping.c
>>
>> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
>> index 6c1f7e1..cb0885a 100644
>> --- a/drivers/watchdog/Kconfig
>> +++ b/drivers/watchdog/Kconfig
>> @@ -1669,6 +1669,12 @@ config WATCHDOG_PRETIMEOUT_DEFAULT_GOV_DEVICE
>>   	  Use device specific watchdog pretimeout event handler
>>   	  by default.
>>
>> +config WATCHDOG_PRETIMEOUT_DEFAULT_GOV_PING
>> +	bool "ping"
>> +	select WATCHDOG_PRETIMEOUT_GOV_PING
>> +	help
>> +	  Use ping watchdog pretimeout event handler by default.
>> +
>>   endchoice
>>
>>   config WATCHDOG_PRETIMEOUT_GOV_NOOP
>> @@ -1698,6 +1704,16 @@ config WATCHDOG_PRETIMEOUT_GOV_DEVICE
>>   	  does nothing, if watchdog driver does not define a pretimeout
>>   	  callback.
>>
>> +config WATCHDOG_PRETIMEOUT_GOV_PING
>> +	tristate "Ping watchdog pretimeout governor"
>> +	help
>> +	  Ping watchdog pretimeout governor, on watchdog pretimeout
>> +	  event ping watchdog device. Use this watchdog pretimeout
>> +	  governor with caution, it may humiliate watchdog work,
>> +	  however it may be helpful in some particular situations,
>> +	  for instance if once started watchdog can not be stopped,
>> +	  but reboot caused by the watchdog is undesired.
>> +
> 
> This one will be unnecessary once the pending infrastructure enhancements
> are accepted. The watchdog core will then handle the ping.
> I don't think this should be left to user space, as it _would_ defeat
> the watchdog's purpose if the watchdog application fails to ping
> the watchdog.
> 

Ok, I'll drop it.

--
With best wishes,
Vladimir

Re: [PATCH 0/6] watchdog: add watchdog pretimeout framework

[Guenter Roeck]

On 11/20/2015 11:11 PM, Vladimir Zapolskiy wrote:
> The change adds a simple watchdog pretimeout framework infrastructure,
> its purpose is to allow users to select a desired handling of watchdog
> pretimeout events, which may be generated by a watchdog driver.
>
> The idea of adding this kind of a framework appeared after reviewing
> several attempts to add hardcoded pretimeout event handling to some
> watchdog driver and after a discussion with Guenter, see
> https://lkml.org/lkml/2015/11/4/346
>
> By design every watchdog pretimeout governor may be compiled as a
> kernel module, a user selects a default watchdog pretimeout
> governor during compilation stage and can select another governor in
> runtime.
>
> Watchdogs with WDIOF_PRETIMEOUT capability now have two device
> attributes in sysfs: read/write pretimeout_governor attribute and read
> only pretimeout_available_governors attribute.
>
> To throw a pretimeout event for further processing a watchdog driver
> should call exported  watchdog_notify_pretimeout(wdd) interface.
>
> In addition to the framework a number of simple watchdog pretimeout
> governors are added for review.
>

Hi Vladimir,

Excellent idea. I would suggest to simplify it a bit, though.

Use only a single configuration flag, and bundle all governors together
with the framework. The governor code isn't that large that it warrants
separate modules, much less separate configuration flags. Keep in mind
that this will ultimately be used by distributions, and for those an
a-b-c choice is always bad. We'll have to find something else to specify
the default governor. Maybe make panic the primary default, and support
a module parameter to change it.

I don't think we should have per-watchdog sysfs attributes to change
the governor. A global set of attributes would make more sense. Maybe
this is possible through /proc/sys/, or just set it once with a
module parameter. If a watchdog driver actually supports pretimeout
is a different question. This should simplify the code a lot,
since there would always be a well known governor to execute on
a pretimeout.

If we have to use workqueues, it would have to run on the highest
possible priority. I think it would be better to determine on a
per-governor basis if a workqueue is needed (eg for userspace events).
We don't need one for panic, or for noop. Otherwise we run the risk
that the work never executes for the same reason that caused the
watchdog to expire in the first place.

Does this make sense ?

Thanks,
Guenter

> Vladimir Zapolskiy (6):
>    watchdog: add watchdog pretimeout framework
>    watchdog: pretimeout: add noop pretimeout governor
>    watchdog: pretimeout: add panic pretimeout governor
>    watchdog: pretimeout: add userspace notifier pretimeout governor
>    watchdog: pretimeout: add device specific notifier pretimeout governor
>    watchdog: pretimeout: add ping pretimeout governor
>
>   drivers/watchdog/Kconfig                |  91 +++++++++
>   drivers/watchdog/Makefile               |  10 +-
>   drivers/watchdog/pretimeout_device.c    |  49 +++++
>   drivers/watchdog/pretimeout_noop.c      |  49 +++++
>   drivers/watchdog/pretimeout_panic.c     |  49 +++++
>   drivers/watchdog/pretimeout_ping.c      |  48 +++++
>   drivers/watchdog/pretimeout_userspace.c |  49 +++++
>   drivers/watchdog/watchdog_core.c        |  14 +-
>   drivers/watchdog/watchdog_pretimeout.c  | 348 ++++++++++++++++++++++++++++++++
>   drivers/watchdog/watchdog_pretimeout.h  |  31 +++
>   include/linux/watchdog.h                |  12 ++
>   11 files changed, 747 insertions(+), 3 deletions(-)
>   create mode 100644 drivers/watchdog/pretimeout_device.c
>   create mode 100644 drivers/watchdog/pretimeout_noop.c
>   create mode 100644 drivers/watchdog/pretimeout_panic.c
>   create mode 100644 drivers/watchdog/pretimeout_ping.c
>   create mode 100644 drivers/watchdog/pretimeout_userspace.c
>   create mode 100644 drivers/watchdog/watchdog_pretimeout.c
>   create mode 100644 drivers/watchdog/watchdog_pretimeout.h
>

Re: [PATCH 0/6] watchdog: add watchdog pretimeout framework

[Vladimir Zapolskiy]

Hi Guenter,

On 21.11.2015 19:13, Guenter Roeck wrote:
> On 11/20/2015 11:11 PM, Vladimir Zapolskiy wrote:
>> The change adds a simple watchdog pretimeout framework infrastructure,
>> its purpose is to allow users to select a desired handling of watchdog
>> pretimeout events, which may be generated by a watchdog driver.
>>
>> The idea of adding this kind of a framework appeared after reviewing
>> several attempts to add hardcoded pretimeout event handling to some
>> watchdog driver and after a discussion with Guenter, see
>> https://lkml.org/lkml/2015/11/4/346
>>
>> By design every watchdog pretimeout governor may be compiled as a
>> kernel module, a user selects a default watchdog pretimeout
>> governor during compilation stage and can select another governor in
>> runtime.
>>
>> Watchdogs with WDIOF_PRETIMEOUT capability now have two device
>> attributes in sysfs: read/write pretimeout_governor attribute and read
>> only pretimeout_available_governors attribute.
>>
>> To throw a pretimeout event for further processing a watchdog driver
>> should call exported  watchdog_notify_pretimeout(wdd) interface.
>>
>> In addition to the framework a number of simple watchdog pretimeout
>> governors are added for review.
>>
> 
> Hi Vladimir,
> 
> Excellent idea. I would suggest to simplify it a bit, though.
> 
> Use only a single configuration flag, and bundle all governors together
> with the framework. 

the idea of having separated governors in kernel module format comes from a
need in one of my projects to create an own private kernel side governor,
bundling all of the governors together will noticeably complicate the
maintenance in my particular case.

Plus the proposed view on the framework actually repeats with minor
adjustments 3 existing governor frameworks created for cpufreq, devfreq and
thermal subsystems, please review them, if you find some time. Cpufreq and
devfreq governors can be compiled and deployed as kernel modules, thermal
governors are bound to thermal.ko, all of them are selected on kernel
compilation stage, all governors are chosen in runtime by means of sysfs
device attribute interface, still some of the governors in every of the
frameworks mentioned above are pretty small.

> The governor code isn't that large that it warrants
> separate modules, much less separate configuration flags. Keep in mind
> that this will ultimately be used by distributions, and for those an
> a-b-c choice is always bad. We'll have to find something else to specify
> the default governor. Maybe make panic the primary default, and support
> a module parameter to change it.

Here I also repeat cpufreq and thermal design (devfreq is a bit different),
please check that default governors for cpufreq and thermal are selected on
compilation stage.

Regarding the primary default governor itself, I don't have any specific
preference, *if* the default governor can be selected on compilation stage.
Panic is fine by default, but probably not for everyone.

I'm not closely involved in any Linux distribution development and so I'm
not familiar with any potential problems there, but why a-b-c choice can not
be always reduced to a-b (drop module tristate option)? And how do
distributions handle e.g. cpufreq governors at the moment?

> I don't think we should have per-watchdog sysfs attributes to change
> the governor. A global set of attributes would make more sense. Maybe
> this is possible through /proc/sys/, or just set it once with a
> module parameter.

I personally dislike the global setting in this particular case, /proc/sys/
is too way system wide (Greg probably will object this interface also),
module parameter setting seems to be more acceptable, but it might be less
straightforward to dynamically change the currently active governor.

Also because a system can have several independent watchdogs (my one have
three hardware watchdogs plus softdog, for example), potentially a user
wants to configure them separately, the limited functionality by means of a
global setting might be insufficient.

In my opinion watchdog pretimeout events should be coupled with the devices,
so sysfs device attribute interface is the most appropriate one among
possible interfaces.

As a side note, I anticipate development of watchdog sysfs device attributes
in the nearest future, I vaguely remember there were some requests to add
some attributes (set/get time left, get started/stopped status etc.). IMHO
further development of binary ioctl() interfaces to watchdogs is less user
friendly.

> If a watchdog driver actually supports pretimeout
> is a different question. This should simplify the code a lot,
> since there would always be a well known governor to execute on
> a pretimeout.

The answer depends on a design decision, should there be one pretimeout
handler for all watchdogs or separate attached handlers. As a user I vote
for improved flexibility.

> If we have to use workqueues, it would have to run on the highest
> possible priority. 

Right, we have to use a workqueue, due to my project demands a work done by
a governor can sleep.

> I think it would be better to determine on a
> per-governor basis if a workqueue is needed (eg for userspace events).
> We don't need one for panic, or for noop.

It makes sense, adding a .can_sleep flag like one defined by GPIO chips may
help.

Because it is an additional configuration option, I've tried to avoid it
right from the beginning, but in general I have no objections to add it.

> Otherwise we run the risk
> that the work never executes for the same reason that caused the
> watchdog to expire in the first place.

Certainly.

> Does this make sense ?

Sure, thank you so much for review. Please let me know your opinion on some
points discussed above, I'm ready to find some time in the nearest future
and update the proposed change, which after fossilization hopefully can be
added to v.4.5.

--
With best wishes,
Vladimir

Re: [PATCH 0/6] watchdog: add watchdog pretimeout framework

[Guenter Roeck]

Hi Vladimir,

On 11/22/2015 04:38 PM, Vladimir Zapolskiy wrote:
> Hi Guenter,
>
> On 21.11.2015 19:13, Guenter Roeck wrote:
>> On 11/20/2015 11:11 PM, Vladimir Zapolskiy wrote:
>>> The change adds a simple watchdog pretimeout framework infrastructure,
>>> its purpose is to allow users to select a desired handling of watchdog
>>> pretimeout events, which may be generated by a watchdog driver.
>>>
>>> The idea of adding this kind of a framework appeared after reviewing
>>> several attempts to add hardcoded pretimeout event handling to some
>>> watchdog driver and after a discussion with Guenter, see
>>> https://lkml.org/lkml/2015/11/4/346
>>>
>>> By design every watchdog pretimeout governor may be compiled as a
>>> kernel module, a user selects a default watchdog pretimeout
>>> governor during compilation stage and can select another governor in
>>> runtime.
>>>
>>> Watchdogs with WDIOF_PRETIMEOUT capability now have two device
>>> attributes in sysfs: read/write pretimeout_governor attribute and read
>>> only pretimeout_available_governors attribute.
>>>
>>> To throw a pretimeout event for further processing a watchdog driver
>>> should call exported  watchdog_notify_pretimeout(wdd) interface.
>>>
>>> In addition to the framework a number of simple watchdog pretimeout
>>> governors are added for review.
>>>
>>
>> Hi Vladimir,
>>
>> Excellent idea. I would suggest to simplify it a bit, though.
>>
>> Use only a single configuration flag, and bundle all governors together
>> with the framework.
>
> the idea of having separated governors in kernel module format comes from a
> need in one of my projects to create an own private kernel side governor,
> bundling all of the governors together will noticeably complicate the
> maintenance in my particular case.
>
> Plus the proposed view on the framework actually repeats with minor
> adjustments 3 existing governor frameworks created for cpufreq, devfreq and
> thermal subsystems, please review them, if you find some time. Cpufreq and
> devfreq governors can be compiled and deployed as kernel modules, thermal
> governors are bound to thermal.ko, all of them are selected on kernel
> compilation stage, all governors are chosen in runtime by means of sysfs
> device attribute interface, still some of the governors in every of the
> frameworks mentioned above are pretty small.
>

Hmm ... ok, I'll accept that. However, please do without the #ifdefs
in the code. Thermal manages to select the default governor in an include
file, and we should be able to do the same here as well. I prefer the
approach taken there, with a pointer to the default governor and no flag.

However, it should not be possible to unload a module if its governor
is in use. Instead of taking a governor away from a watchdog by unloading
its module, selecting a governor should increase the reference count
on a module, thus preventing it from being unloaded.

We might also want to consider loading the default governor early,
not as module. Not sure how messy that would be, though. I am a bit
concerned if a governor doesn't get to run because its module is not
loaded, even if it is the default (which is why I kind of dislike
using modules). Maybe we should force-load the default governor module
when the pretimeout code initializes, and prevent it from being unloaded.

>> The governor code isn't that large that it warrants
>> separate modules, much less separate configuration flags. Keep in mind
>> that this will ultimately be used by distributions, and for those an
>> a-b-c choice is always bad. We'll have to find something else to specify
>> the default governor. Maybe make panic the primary default, and support
>> a module parameter to change it.
>
> Here I also repeat cpufreq and thermal design (devfreq is a bit different),
> please check that default governors for cpufreq and thermal are selected on
> compilation stage.
>
> Regarding the primary default governor itself, I don't have any specific
> preference, *if* the default governor can be selected on compilation stage.
> Panic is fine by default, but probably not for everyone.
>
Ok.

> I'm not closely involved in any Linux distribution development and so I'm
> not familiar with any potential problems there, but why a-b-c choice can not
> be always reduced to a-b (drop module tristate option)? And how do
> distributions handle e.g. cpufreq governors at the moment?
>
>> I don't think we should have per-watchdog sysfs attributes to change
>> the governor. A global set of attributes would make more sense. Maybe
>> this is possible through /proc/sys/, or just set it once with a
>> module parameter.
>
> I personally dislike the global setting in this particular case, /proc/sys/
> is too way system wide (Greg probably will object this interface also),
> module parameter setting seems to be more acceptable, but it might be less
> straightforward to dynamically change the currently active governor.
>
> Also because a system can have several independent watchdogs (my one have
> three hardware watchdogs plus softdog, for example), potentially a user
> wants to configure them separately, the limited functionality by means of a
> global setting might be insufficient.
>
> In my opinion watchdog pretimeout events should be coupled with the devices,
> so sysfs device attribute interface is the most appropriate one among
> possible interfaces.
>
The problem here is that there would be one governor per watchdog. I don't think
any of the other subsystems has multiple default governors. This makes it very
hard for the user to configure the system. I really don't believe that there
is value in having multiple governors for different watchdogs.

Having said that, yes, you are right, all other governors do the same.
So much for overkill ;-). Meaning even though I don't think it provides
sufficient value and will make configuration more difficult than necessary,
I'll accept your point.

> As a side note, I anticipate development of watchdog sysfs device attributes
> in the nearest future, I vaguely remember there were some requests to add
> some attributes (set/get time left, get started/stopped status etc.). IMHO
> further development of binary ioctl() interfaces to watchdogs is less user
> friendly.
>

Yes, I already have those queued in my watchdog-next tree. I have no idea what
Wim thinks about it, though.

>> If a watchdog driver actually supports pretimeout
>> is a different question. This should simplify the code a lot,
>> since there would always be a well known governor to execute on
>> a pretimeout.
>
> The answer depends on a design decision, should there be one pretimeout
> handler for all watchdogs or separate attached handlers. As a user I vote
> for improved flexibility.
>
I prefer simplified configuration. It would be great to have some others
chime in with their opinion before we go too far along some route.

>> If we have to use workqueues, it would have to run on the highest
>> possible priority.
>
> Right, we have to use a workqueue, due to my project demands a work done by
> a governor can sleep.
>
>> I think it would be better to determine on a
>> per-governor basis if a workqueue is needed (eg for userspace events).
>> We don't need one for panic, or for noop.
>
> It makes sense, adding a .can_sleep flag like one defined by GPIO chips may
> help.
>
Either that, or the governor itself implements the workqueue if needed.
But a workqueue should not be mandatory if it is not needed. I can understand
that your project may need one, but that doesn't mean that we should
risk that the "panic" governor stalls because its workqueue never runs.

> Because it is an additional configuration option, I've tried to avoid it
> right from the beginning, but in general I have no objections to add it.
>

Why would this be a configuration option (instead of a flag determined
by the governor) ?

Thanks,
Guenter

Re: [PATCH 0/6] watchdog: add watchdog pretimeout framework

[Vladimir Zapolskiy]

Hi Guenter,

On 24.11.2015 08:47, Guenter Roeck wrote:
> Hi Vladimir,
> 
> On 11/22/2015 04:38 PM, Vladimir Zapolskiy wrote:
>> Hi Guenter,
>>
>> On 21.11.2015 19:13, Guenter Roeck wrote:
>>> On 11/20/2015 11:11 PM, Vladimir Zapolskiy wrote:
>>>> The change adds a simple watchdog pretimeout framework infrastructure,
>>>> its purpose is to allow users to select a desired handling of watchdog
>>>> pretimeout events, which may be generated by a watchdog driver.
>>>>
>>>> The idea of adding this kind of a framework appeared after reviewing
>>>> several attempts to add hardcoded pretimeout event handling to some
>>>> watchdog driver and after a discussion with Guenter, see
>>>> https://lkml.org/lkml/2015/11/4/346
>>>>
>>>> By design every watchdog pretimeout governor may be compiled as a
>>>> kernel module, a user selects a default watchdog pretimeout
>>>> governor during compilation stage and can select another governor in
>>>> runtime.
>>>>
>>>> Watchdogs with WDIOF_PRETIMEOUT capability now have two device
>>>> attributes in sysfs: read/write pretimeout_governor attribute and read
>>>> only pretimeout_available_governors attribute.
>>>>
>>>> To throw a pretimeout event for further processing a watchdog driver
>>>> should call exported  watchdog_notify_pretimeout(wdd) interface.
>>>>
>>>> In addition to the framework a number of simple watchdog pretimeout
>>>> governors are added for review.
>>>>
>>>
>>> Hi Vladimir,
>>>
>>> Excellent idea. I would suggest to simplify it a bit, though.
>>>
>>> Use only a single configuration flag, and bundle all governors together
>>> with the framework.
>>
>> the idea of having separated governors in kernel module format comes from a
>> need in one of my projects to create an own private kernel side governor,
>> bundling all of the governors together will noticeably complicate the
>> maintenance in my particular case.
>>
>> Plus the proposed view on the framework actually repeats with minor
>> adjustments 3 existing governor frameworks created for cpufreq, devfreq and
>> thermal subsystems, please review them, if you find some time. Cpufreq and
>> devfreq governors can be compiled and deployed as kernel modules, thermal
>> governors are bound to thermal.ko, all of them are selected on kernel
>> compilation stage, all governors are chosen in runtime by means of sysfs
>> device attribute interface, still some of the governors in every of the
>> frameworks mentioned above are pretty small.
>>
> 
> Hmm ... ok, I'll accept that. However, please do without the #ifdefs
> in the code. Thermal manages to select the default governor in an include
> file, and we should be able to do the same here as well. I prefer the
> approach taken there, with a pointer to the default governor and no flag.

Ok, if CONFIG_WATCHDOG_PRETIMEOUT_DEFAULT_GOV_* are moved from distributed
governor code to a centralized location, it might slightly complicate the
maintenance of private governors on my end, but I can cope with it, I believe.

> However, it should not be possible to unload a module if its governor
> is in use. Instead of taking a governor away from a watchdog by unloading
> its module, selecting a governor should increase the reference count
> on a module, thus preventing it from being unloaded.

Ok, this can be done, moreover it will simplify the design, because I had to
keep in mind a race, if pretimeout is reported, but governor is gone before
executing a workqueue task.

> We might also want to consider loading the default governor early,
> not as module. Not sure how messy that would be, though. I am a bit
> concerned if a governor doesn't get to run because its module is not
> loaded, even if it is the default (which is why I kind of dislike
> using modules). Maybe we should force-load the default governor module
> when the pretimeout code initializes, and prevent it from being unloaded.

Right in this series you may find from Kconfig that a default governor is
always compiled into the image (WATCHDOG_PRETIMEOUT_DEFAULT_GOV_* is bool).
I think it is aligned with your vision, so I'll keep this design decision,
if you don't mind.

Practically the published code can correctly handle a situation (i.e. no
oops or unexpected execution routes), if there is no assigned governor, for
example if a default governor is converted to a module by Kconfig change and
the module is unloaded. I have no objections to strictly proclaim that at
any time there should be one governor connected to a watchdog, this will
require some code removal, and I'll do it.

>>> The governor code isn't that large that it warrants
>>> separate modules, much less separate configuration flags. Keep in mind
>>> that this will ultimately be used by distributions, and for those an
>>> a-b-c choice is always bad. We'll have to find something else to specify
>>> the default governor. Maybe make panic the primary default, and support
>>> a module parameter to change it.
>>
>> Here I also repeat cpufreq and thermal design (devfreq is a bit different),
>> please check that default governors for cpufreq and thermal are selected on
>> compilation stage.
>>
>> Regarding the primary default governor itself, I don't have any specific
>> preference, *if* the default governor can be selected on compilation stage.
>> Panic is fine by default, but probably not for everyone.
>>
> Ok.
> 
>> I'm not closely involved in any Linux distribution development and so I'm
>> not familiar with any potential problems there, but why a-b-c choice can not
>> be always reduced to a-b (drop module tristate option)? And how do
>> distributions handle e.g. cpufreq governors at the moment?
>>
>>> I don't think we should have per-watchdog sysfs attributes to change
>>> the governor. A global set of attributes would make more sense. Maybe
>>> this is possible through /proc/sys/, or just set it once with a
>>> module parameter.
>>
>> I personally dislike the global setting in this particular case, /proc/sys/
>> is too way system wide (Greg probably will object this interface also),
>> module parameter setting seems to be more acceptable, but it might be less
>> straightforward to dynamically change the currently active governor.
>>
>> Also because a system can have several independent watchdogs (my one have
>> three hardware watchdogs plus softdog, for example), potentially a user
>> wants to configure them separately, the limited functionality by means of a
>> global setting might be insufficient.
>>
>> In my opinion watchdog pretimeout events should be coupled with the devices,
>> so sysfs device attribute interface is the most appropriate one among
>> possible interfaces.
>>
> The problem here is that there would be one governor per watchdog. I don't think
> any of the other subsystems has multiple default governors. This makes it very
> hard for the user to configure the system.

There is one default governor for all devices, but a user has a possibility
to select another one in runtime for a particular .

Talking about other subsystems, for example this is what I have on my laptop:

  # find /sys/ | grep governor
  /sys/devices/system/cpu/cpu0/cpufreq/scaling_governor
  /sys/devices/system/cpu/cpu0/cpufreq/scaling_available_governors
  /sys/devices/system/cpu/cpu1/cpufreq/scaling_governor
  /sys/devices/system/cpu/cpu1/cpufreq/scaling_available_governors
  /sys/devices/system/cpu/cpu2/cpufreq/scaling_governor
  /sys/devices/system/cpu/cpu2/cpufreq/scaling_available_governors
  /sys/devices/system/cpu/cpu3/cpufreq/scaling_governor
  /sys/devices/system/cpu/cpu3/cpufreq/scaling_available_governors

This watchdog pretimeout framework change does something similar, but you'll
note it only if you have 4 watchdogs.

> I really don't believe that there
> is value in having multiple governors for different watchdogs.

There is such value in my opinion, different watchdogs can play different
roles in system, can have different parameters and can be managed (started,
stopped, pinged) differently. I can imagine a situation, if 2-3 watchdogs
synchronously configured to reboot a system in 60 seconds, but pretimeouts
(not a panic one) are set to 30, 20, 10 seconds.

99.9% users have only one watchdog, so decisions to have one governor for
all watchdogs or one governor for one watchdog are equal for them, no
additional configuration overhead involved, but 0.1% users will suffer from
lack of flexibility. And this 0.1% users most probably come from
industrial/medical/automotive/aerospace and other areas of safety-critical
systems.

> Having said that, yes, you are right, all other governors do the same.
> So much for overkill ;-). Meaning even though I don't think it provides
> sufficient value and will make configuration more difficult than necessary,
> I'll accept your point.
> 
>> As a side note, I anticipate development of watchdog sysfs device attributes
>> in the nearest future, I vaguely remember there were some requests to add
>> some attributes (set/get time left, get started/stopped status etc.). IMHO
>> further development of binary ioctl() interfaces to watchdogs is less user
>> friendly.
>>
> 
> Yes, I already have those queued in my watchdog-next tree. I have no idea what
> Wim thinks about it, though.

I hope he approves.

>>> If a watchdog driver actually supports phttp://www.google.de/url?sa=t&rct=j&q=&esrc=s&source=web&cd=6&ved=0ahUKEwizlOLtkKnJAhWCSYgKHdCgCN8QFghDMAU&url=http%3A%2F%2Fwww.vectorcast.com%2Fblog%2F2014%2F01%2F6-industries-where-embedded-software-testing-mission-critical&usg=AFQjCNFgXGhKc4uGpMkGMTt6osGgZgNWaA&bvm=bv.108194040,d.cGUretimeout
>>> is a different question. This should simplify the code a lot,
>>> since there would always be a well known governor to execute on
>>> a pretimeout.
>>
>> The answer depends on a design decision, should there be one pretimeout
>> handler for all watchdogs or separate attached handlers. As a user I vote
>> for improved flexibility.
>>
> I prefer simplified configuration. It would be great to have some others
> chime in with their opinion before we go too far along some route.
>

I support it. Wim, do you have an opinion?

>>> If we have to use workqueues, it would have to run on the highest
>>> possible priority.
>>
>> Right, we have to use a workqueue, due to my project demands a work done by
>> a governor can sleep.
>>
>>> I think it would be better to determine on a
>>> per-governor basis if a workqueue is needed (eg for userspace events).
>>> We don't need one for panic, or for noop.
>>
>> It makes sense, adding a .can_sleep flag like one defined by GPIO chips may
>> help.
>>
> Either that, or the governor itself implements the workqueue if needed.

If governors implement own workqueues this will result in duplicated code
and more steadfast attention from maintainers. Passing a flag is simpler,
I'll do it.

> But a workqueue should not be mandatory if it is not needed. I can understand
> that your project may need one, but that doesn't mean that we should
> risk that the "panic" governor stalls because its workqueue never runs.

Sure, "panic" governor won't sleep.

>> Because it is an additional configuration option, I've tried to avoid it
>> right from the beginning, but in general I have no objections to add it.
>>
> 
> Why would this be a configuration option (instead of a flag determined
> by the governor) ?

I used an incorrect word, here I meant a fixed "struct watchdog_governor"
definition.

--
With best wishes,
Vladimir