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

[PATCH v3 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

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: panic and noop.

Hopefully the change opens a possibility to complete development of
device specific watchdog pretimeout bits.

In comparison to v1 and v2 this version does not have quite many important
features, because now the goal is to initiate technical review of the
simplest possible core change, the fat tail is put aside at the moment.

These missing features from v1 and v2 are done, they'll be published
on top of the series after technical review completion of the core:
* compilation of several independent watchdog pretimeout governors
* watchdog pretimeout governors may be compiled as built-in or modules
* governors individually selectable per watchdog device over sysfs
* able to sleep watchdog pretimeout governors
* userspace notification over uevent governor

Note that the currently omitted design features of the watchdog pretimeout
framework are similar to cpufreq, devfreq and thermal governors, this
includes:
* option to compile governors as kernel modules,
* compile time selection of a default governor,
* similar per-device sysfs attributes and one governor to one device
  link model.

Changes from v2 to v3:
* from watchdog_pretimeout.c removed all features mentioned above
* rebased panic and noop governors on top of the simplified watchdog pretimeout
  framework
* added 2 rebased and cleaned up changes done by Robin Gong to the series,
  Robin's changes allow to test the series, if some individual watchdog driver
  adds WDIOF_PRETIMEOUT support and calls watchdog_notify_pretimeout(),
  for example Robin implemented the feature for imx2+ watchdog driver
* added pretimeout value display over sysfs for WDIOF_PRETIMEOUT capable
  watchdog devices
* moved sysfs device attributes to watchdog_dev.c, this required to add
  exported watchdog_pretimeout_governor_name() interface
* if pretimeout framework is not selected, then pretimeout event ends up
  in kernel panic -- this behaviour as a default one was asked by Guenter
* due to removal of support to sleeing governors removed userspace
  notification pretimeout governor from the series
* minor clean-ups and adjustments

Changes from v1 to v2, thanks to Guenter for review and discussion:
* removed re-ping pretimeout governor, the functionality is supposed
  to be covered by the pending infrastructure enhancements,
* removed watchdog driver specific pretimeout governor, at the moment
  it is not expected to add driver specific handlers,
* reordered governors, panic comes in the first place,
* removed framework private bits from struct watchdog_governor,
* centralized compile-time selection of a default governor in
  watchdog_pretimeout.h,
* added can_sleep option, now only sleeping governors (e.g. userspace)
  will be executed in a special workqueue,
* changed fallback logic, if a governor in use is removed, now this
  situation is not possible, because in use governors have non-zero
  module refcount,
* slightly improved description of the governors in Kconfig.

Robin Gong (2):
  watchdog: add set_pretimeout interface
  watchdog: add WDIOC_SETPRETIMEOUT and WDIOC_GETPRETIMEOUT

Vladimir Zapolskiy (4):
  watchdog: add pretimeout read-only device attribute to sysfs
  watchdog: add watchdog pretimeout framework
  watchdog: pretimeout: add panic pretimeout governor
  watchdog: pretimeout: add noop pretimeout governor

 drivers/watchdog/Kconfig               |  44 +++++++++++
 drivers/watchdog/Makefile              |   8 +-
 drivers/watchdog/pretimeout_noop.c     |  47 ++++++++++++
 drivers/watchdog/pretimeout_panic.c    |  47 ++++++++++++
 drivers/watchdog/watchdog_core.c       |   9 +++
 drivers/watchdog/watchdog_dev.c        |  60 +++++++++++++++
 drivers/watchdog/watchdog_pretimeout.c | 134 +++++++++++++++++++++++++++++++++
 drivers/watchdog/watchdog_pretimeout.h |  48 ++++++++++++
 include/linux/watchdog.h               |  23 ++++++
 9 files changed, 419 insertions(+), 1 deletion(-)
 create mode 100644 drivers/watchdog/pretimeout_noop.c
 create mode 100644 drivers/watchdog/pretimeout_panic.c
 create mode 100644 drivers/watchdog/watchdog_pretimeout.c
 create mode 100644 drivers/watchdog/watchdog_pretimeout.h

-- 
2.5.0

[PATCH v3 1/6] watchdog: add set_pretimeout interface

[Vladimir Zapolskiy]

From: Robin Gong <b38343@freescale.com>

Add set_pretimeout since our watchdog driver has those interfaces and
obviously, the new common watchdog framework didn't implement this
interface.

Signed-off-by: Robin Gong <b38343@freescale.com>
[vzapolskiy: rebased, added an inline comment to describe new interface]
Signed-off-by: Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com>
---
Changes from v2 to v3:
* none, new change

Changes from v1 to v2:
* none, new change

 include/linux/watchdog.h | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/include/linux/watchdog.h b/include/linux/watchdog.h
index 51732d6..e3d23d3 100644
--- a/include/linux/watchdog.h
+++ b/include/linux/watchdog.h
@@ -28,6 +28,7 @@ struct watchdog_core_data;
  * @ping:	The routine that sends a keepalive ping to the watchdog device.
  * @status:	The routine that shows the status of the watchdog device.
  * @set_timeout:The routine for setting the watchdog devices timeout value (in seconds).
+ * @set_pretimeout:The routine for setting the watchdog devices pretimeout (in seconds).
  * @get_timeleft:The routine that gets the time left before a reset (in seconds).
  * @restart:	The routine for restarting the machine.
  * @ioctl:	The routines that handles extra ioctl calls.
@@ -46,6 +47,7 @@ struct watchdog_ops {
 	int (*ping)(struct watchdog_device *);
 	unsigned int (*status)(struct watchdog_device *);
 	int (*set_timeout)(struct watchdog_device *, unsigned int);
+	int (*set_pretimeout)(struct watchdog_device *, unsigned int);
 	unsigned int (*get_timeleft)(struct watchdog_device *);
 	int (*restart)(struct watchdog_device *, unsigned long, void *);
 	long (*ioctl)(struct watchdog_device *, unsigned int, unsigned long);
@@ -95,6 +97,7 @@ struct watchdog_device {
 	const struct watchdog_ops *ops;
 	unsigned int bootstatus;
 	unsigned int timeout;
+	unsigned int pretimeout;
 	unsigned int min_timeout;
 	unsigned int max_timeout;
 	unsigned int min_hw_heartbeat_ms;
@@ -162,6 +165,13 @@ static inline bool watchdog_timeout_invalid(struct watchdog_device *wdd, unsigne
 		 t > wdd->max_timeout);
 }
 
+/* Use the following function to check if a pretimeout value is invalid */
+static inline bool watchdog_pretimeout_invalid(struct watchdog_device *wdd,
+					       unsigned int t)
+{
+	return wdd->timeout && t >= wdd->timeout;
+}
+
 /* Use the following functions to manipulate watchdog driver specific data */
 static inline void watchdog_set_drvdata(struct watchdog_device *wdd, void *data)
 {
-- 
2.5.0

[PATCH v3 2/6] watchdog: add WDIOC_SETPRETIMEOUT and WDIOC_GETPRETIMEOUT

[Vladimir Zapolskiy]

From: Robin Gong <b38343@freescale.com>

Since the watchdog common framework centrialize the IOCTL interfaces
of device driver now, the SETPRETIMEOUT and GETPRETIMEOUT need to be
added in common code.

Signed-off-by: Robin Gong <b38343@freescale.com>
[vzapolskiy: rebased, cleanups]
Signed-off-by: Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com>
---
Changes from v2 to v3:
* none, new change

Changes from v1 to v2:
* none, new change

 drivers/watchdog/watchdog_dev.c | 31 +++++++++++++++++++++++++++++++
 1 file changed, 31 insertions(+)

diff --git a/drivers/watchdog/watchdog_dev.c b/drivers/watchdog/watchdog_dev.c
index 3595cff..42ab86d 100644
--- a/drivers/watchdog/watchdog_dev.c
+++ b/drivers/watchdog/watchdog_dev.c
@@ -318,6 +318,27 @@ static int watchdog_set_timeout(struct watchdog_device *wdd,
 }
 
 /*
+ *	watchdog_set_pretimeout: set the watchdog timer pretimeout
+ *	@wdd: the watchdog device to set the timeout for
+ *	@timeout: pretimeout to set in seconds
+ *
+ *	The caller must hold wd_data->lock.
+ */
+
+static int watchdog_set_pretimeout(struct watchdog_device *wdd,
+				   unsigned int pretimeout)
+{
+	if (!wdd->ops->set_pretimeout ||
+	    !(wdd->info->options & WDIOF_PRETIMEOUT))
+		return -EOPNOTSUPP;
+
+	if (watchdog_pretimeout_invalid(wdd, pretimeout))
+		return -EINVAL;
+
+	return wdd->ops->set_pretimeout(wdd, pretimeout);
+}
+
+/*
  *	watchdog_get_timeleft: wrapper to get the time left before a reboot
  *	@wdd: the watchdog device to get the remaining time from
  *	@timeleft: the time that's left
@@ -620,6 +641,16 @@ static long watchdog_ioctl(struct file *file, unsigned int cmd,
 			break;
 		err = put_user(val, p);
 		break;
+	case WDIOC_SETPRETIMEOUT:
+		if (get_user(val, p)) {
+			err = -EFAULT;
+			break;
+		}
+		err = watchdog_set_pretimeout(wdd, val);
+		break;
+	case WDIOC_GETPRETIMEOUT:
+		err = put_user(wdd->pretimeout, p);
+		break;
 	default:
 		err = -ENOTTY;
 		break;
-- 
2.5.0

[PATCH v3 3/6] watchdog: add pretimeout read-only device attribute to sysfs

[Vladimir Zapolskiy]

The change allows for WDIOF_PRETIMEOUT capable watchdog devices to get
a configured pretimeout value over sysfs, the correspondent device
attribute name is "pretimeout".

Signed-off-by: Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com>
---
Changes from v2 to v3:
* none, new change

Changes from v1 to v2:
* none, new change

 drivers/watchdog/watchdog_dev.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

diff --git a/drivers/watchdog/watchdog_dev.c b/drivers/watchdog/watchdog_dev.c
index 42ab86d..87bbae7 100644
--- a/drivers/watchdog/watchdog_dev.c
+++ b/drivers/watchdog/watchdog_dev.c
@@ -443,6 +443,15 @@ static ssize_t state_show(struct device *dev, struct device_attribute *attr,
 }
 static DEVICE_ATTR_RO(state);
 
+static ssize_t pretimeout_show(struct device *dev,
+			       struct device_attribute *attr, char *buf)
+{
+	struct watchdog_device *wdd = dev_get_drvdata(dev);
+
+	return sprintf(buf, "%u\n", wdd->pretimeout);
+}
+static DEVICE_ATTR_RO(pretimeout);
+
 static umode_t wdt_is_visible(struct kobject *kobj, struct attribute *attr,
 				int n)
 {
@@ -454,6 +463,9 @@ static umode_t wdt_is_visible(struct kobject *kobj, struct attribute *attr,
 		mode = 0;
 	else if (attr == &dev_attr_timeleft.attr && !wdd->ops->get_timeleft)
 		mode = 0;
+	else if (attr == &dev_attr_pretimeout.attr &&
+		 !(wdd->info->options & WDIOF_PRETIMEOUT))
+		mode = 0;
 
 	return mode;
 }
@@ -465,6 +477,7 @@ static struct attribute *wdt_attrs[] = {
 	&dev_attr_bootstatus.attr,
 	&dev_attr_status.attr,
 	&dev_attr_nowayout.attr,
+	&dev_attr_pretimeout.attr,
 	NULL,
 };
 
-- 
2.5.0

[PATCH v3 4/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 some watchdog devices.

A user selects a default watchdog pretimeout governor during
compilation stage.

Watchdogs with WDIOF_PRETIMEOUT capability now have two device
attributes in sysfs: pretimeout to display currently set pretimeout
value and pretimeout_governor attribute to display the selected
watchdog pretimeout governor.

Watchdogs with no WDIOF_PRETIMEOUT capability has no changes in
sysfs, and such watchdog devices do not require the framework.

Signed-off-by: Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com>
---
Changes from v2 to v3:
* essentially simplified the implementation due to removal of runtime
  dynamic selection of watchdog pretimeout governors by a user, this
  feature is supposed to be added later on
* removed support of sleepable watchdog pretimeout governors
* moved sysfs device attributes to watchdog_dev.c, this required to
  add exported watchdog_pretimeout_governor_name() interface
* if pretimeout framework is not selected, then pretimeout event
  ends up in kernel panic -- this behaviour as a default one was asked
  by Guenter

Changes from v1 to v2:
* removed framework private bits from struct watchdog_governor,
* centralized compile-time selection of a default governor in
  watchdog_pretimeout.h,
* added can_sleep option, now only sleeping governors (e.g. userspace)
  will be executed in a special workqueue,
* changed fallback logic, if a governor in use is removed, now this
  situation is not possible, because in use governors have non-zero
  module refcount

 drivers/watchdog/Kconfig               |   8 ++
 drivers/watchdog/Makefile              |   5 +-
 drivers/watchdog/watchdog_core.c       |   9 +++
 drivers/watchdog/watchdog_dev.c        |  16 ++++
 drivers/watchdog/watchdog_pretimeout.c | 134 +++++++++++++++++++++++++++++++++
 drivers/watchdog/watchdog_pretimeout.h |  42 +++++++++++
 include/linux/watchdog.h               |  13 ++++
 7 files changed, 226 insertions(+), 1 deletion(-)
 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 b54f26c..354217e 100644
--- a/drivers/watchdog/Kconfig
+++ b/drivers/watchdog/Kconfig
@@ -1800,4 +1800,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 a46e7c1..cca47de 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 7c3ba58..ae6c23a 100644
--- a/drivers/watchdog/watchdog_core.c
+++ b/drivers/watchdog/watchdog_core.c
@@ -40,6 +40,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);
 
@@ -244,6 +245,13 @@ static int __watchdog_register_device(struct watchdog_device *wdd)
 		}
 	}
 
+	ret = watchdog_register_pretimeout(wdd);
+	if (ret) {
+		watchdog_dev_unregister(wdd);
+		ida_simple_remove(&watchdog_ida, wdd->id);
+		return ret;
+	}
+
 	if (test_bit(WDOG_STOP_ON_REBOOT, &wdd->status)) {
 		wdd->reboot_nb.notifier_call = watchdog_reboot_notifier;
 
@@ -251,6 +259,7 @@ static int __watchdog_register_device(struct watchdog_device *wdd)
 		if (ret) {
 			pr_err("watchdog%d: Cannot register reboot notifier (%d)\n",
 			       wdd->id, ret);
+			watchdog_unregister_pretimeout(wdd);
 			watchdog_dev_unregister(wdd);
 			ida_simple_remove(&watchdog_ida, wdd->id);
 			return ret;
diff --git a/drivers/watchdog/watchdog_dev.c b/drivers/watchdog/watchdog_dev.c
index 87bbae7..c0fd743 100644
--- a/drivers/watchdog/watchdog_dev.c
+++ b/drivers/watchdog/watchdog_dev.c
@@ -49,6 +49,7 @@
 #include <linux/uaccess.h>	/* For copy_to_user/put_user/... */
 
 #include "watchdog_core.h"
+#include "watchdog_pretimeout.h"
 
 /*
  * struct watchdog_core_data - watchdog core internal data
@@ -452,6 +453,16 @@ static ssize_t pretimeout_show(struct device *dev,
 }
 static DEVICE_ATTR_RO(pretimeout);
 
+static ssize_t pretimeout_governor_show(struct device *dev,
+					struct device_attribute *devattr,
+					char *buf)
+{
+	struct watchdog_device *wdd = dev_get_drvdata(dev);
+
+	return watchdog_pretimeout_governor_name(wdd, buf);
+}
+static DEVICE_ATTR_RO(pretimeout_governor);
+
 static umode_t wdt_is_visible(struct kobject *kobj, struct attribute *attr,
 				int n)
 {
@@ -466,6 +477,10 @@ static umode_t wdt_is_visible(struct kobject *kobj, struct attribute *attr,
 	else if (attr == &dev_attr_pretimeout.attr &&
 		 !(wdd->info->options & WDIOF_PRETIMEOUT))
 		mode = 0;
+	else if (attr == &dev_attr_pretimeout_governor.attr &&
+		 !((wdd->info->options & WDIOF_PRETIMEOUT) &&
+		   IS_ENABLED(CONFIG_WATCHDOG_PRETIMEOUT_GOV)))
+		mode = 0;
 
 	return mode;
 }
@@ -478,6 +493,7 @@ static struct attribute *wdt_attrs[] = {
 	&dev_attr_status.attr,
 	&dev_attr_nowayout.attr,
 	&dev_attr_pretimeout.attr,
+	&dev_attr_pretimeout_governor.attr,
 	NULL,
 };
 
diff --git a/drivers/watchdog/watchdog_pretimeout.c b/drivers/watchdog/watchdog_pretimeout.c
new file mode 100644
index 0000000..ebfc3d6
--- /dev/null
+++ b/drivers/watchdog/watchdog_pretimeout.c
@@ -0,0 +1,134 @@
+/*
+ * Copyright (C) 2015-2016 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/list.h>
+#include <linux/slab.h>
+#include <linux/spinlock.h>
+#include <linux/watchdog.h>
+
+#include "watchdog_pretimeout.h"
+
+/* Default watchdog pretimeout governor */
+static struct watchdog_governor *default_gov;
+
+/* 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);
+
+struct watchdog_pretimeout {
+	struct watchdog_device		*wdd;
+	struct list_head		entry;
+};
+
+int watchdog_pretimeout_governor_name(struct watchdog_device *wdd, char *buf)
+{
+	int count = 0;
+
+	spin_lock_irq(&pretimeout_lock);
+	if (wdd->gov)
+		count = sprintf(buf, "%s\n", wdd->gov->name);
+	else
+		count = sprintf(buf, "N/A\n");
+	spin_unlock_irq(&pretimeout_lock);
+
+	return count;
+}
+
+void watchdog_notify_pretimeout(struct watchdog_device *wdd)
+{
+	unsigned long flags;
+
+	if (!wdd)
+		return;
+
+	spin_lock_irqsave(&pretimeout_lock, flags);
+	if (!wdd->gov) {
+		spin_unlock_irqrestore(&pretimeout_lock, flags);
+		return;
+	}
+
+	wdd->gov->pretimeout(wdd);
+	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_MAXLEN)
+		return -EINVAL;
+
+	if (default_gov)
+		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);
+
+	default_gov = gov;
+
+	return 0;
+}
+
+void watchdog_unregister_governor(struct watchdog_governor *gov)
+{
+	if (!gov)
+		return;
+
+	if (default_gov == gov)
+		default_gov = NULL;
+}
+
+int watchdog_register_pretimeout(struct watchdog_device *wdd)
+{
+	struct watchdog_pretimeout *p;
+
+	if (!(wdd->info->options & WDIOF_PRETIMEOUT))
+		return 0;
+
+	p = kzalloc(sizeof(*p), GFP_KERNEL);
+	if (!p)
+		return -ENOMEM;
+
+	spin_lock_irq(&pretimeout_lock);
+	list_add(&p->entry, &pretimeout_list);
+	p->wdd = wdd;
+	wdd->gov = default_gov;
+	spin_unlock_irq(&pretimeout_lock);
+
+	return 0;
+}
+
+void watchdog_unregister_pretimeout(struct watchdog_device *wdd)
+{
+	struct watchdog_pretimeout *p;
+
+	if (!(wdd->info->options & WDIOF_PRETIMEOUT))
+		return;
+
+	spin_lock_irq(&pretimeout_lock);
+	wdd->gov = NULL;
+
+	list_for_each_entry(p, &pretimeout_list, entry) {
+		if (p->wdd == wdd) {
+			list_del(&p->entry);
+			break;
+		}
+	}
+	spin_unlock_irq(&pretimeout_lock);
+
+	kfree(p);
+}
diff --git a/drivers/watchdog/watchdog_pretimeout.h b/drivers/watchdog/watchdog_pretimeout.h
new file mode 100644
index 0000000..9e08d56
--- /dev/null
+++ b/drivers/watchdog/watchdog_pretimeout.h
@@ -0,0 +1,42 @@
+#ifndef __WATCHDOG_PRETIMEOUT_H
+#define __WATCHDOG_PRETIMEOUT_H
+
+#define WATCHDOG_GOV_NAME_MAXLEN	20
+
+struct watchdog_device;
+
+struct watchdog_governor {
+	const char		name[WATCHDOG_GOV_NAME_MAXLEN];
+	void			(*pretimeout)(struct watchdog_device *wdd);
+};
+
+#if IS_ENABLED(CONFIG_WATCHDOG_PRETIMEOUT_GOV)
+/* 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 */
+int watchdog_register_pretimeout(struct watchdog_device *wdd);
+void watchdog_unregister_pretimeout(struct watchdog_device *wdd);
+
+/* Interfaces to watchdog_dev.c */
+int watchdog_pretimeout_governor_name(struct watchdog_device *wdd, char *buf);
+
+#else
+static inline int watchdog_register_pretimeout(struct watchdog_device *wdd)
+{
+	return 0;
+}
+
+static inline void watchdog_unregister_pretimeout(struct watchdog_device *wdd)
+{
+}
+
+static inline int watchdog_pretimeout_governor_name(struct watchdog_device *wdd,
+						    char *buf)
+{
+	return 0;
+}
+#endif
+
+#endif
diff --git a/include/linux/watchdog.h b/include/linux/watchdog.h
index e3d23d3..0d18113 100644
--- a/include/linux/watchdog.h
+++ b/include/linux/watchdog.h
@@ -19,6 +19,7 @@
 struct watchdog_ops;
 struct watchdog_device;
 struct watchdog_core_data;
+struct watchdog_governor;
 
 /** struct watchdog_ops - The watchdog-devices operations
  *
@@ -61,6 +62,7 @@ struct watchdog_ops {
  *		watchdog 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).
@@ -95,6 +97,7 @@ struct watchdog_device {
 	const struct attribute_group **groups;
 	const struct watchdog_info *info;
 	const struct watchdog_ops *ops;
+	const struct watchdog_governor *gov;
 	unsigned int bootstatus;
 	unsigned int timeout;
 	unsigned int pretimeout;
@@ -183,6 +186,16 @@ static inline void *watchdog_get_drvdata(struct watchdog_device *wdd)
 	return wdd->driver_data;
 }
 
+/* Use the following functions to report watchdog pretimeout event */
+#if IS_ENABLED(CONFIG_WATCHDOG_PRETIMEOUT_GOV)
+void watchdog_notify_pretimeout(struct watchdog_device *wdd);
+#else
+static inline void watchdog_notify_pretimeout(struct watchdog_device *wdd)
+{
+	panic("watchdog pretimeout event\n");
+}
+#endif
+
 /* drivers/watchdog/watchdog_core.c */
 void watchdog_set_restart_priority(struct watchdog_device *wdd, int priority);
 extern int watchdog_init_timeout(struct watchdog_device *wdd,
-- 
2.5.0

[PATCH v3 5/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>
---
Changes from v2 to v3:
* temporarily removed tristate option, the governor can be built-in only

Changes from v1 to v2:
* removed #ifdef CONFIG_WATCHDOG_PRETIMEOUT_DEFAULT_GOV_PANIC,
  now this selection is done in a centralized manner,
* added module owner reference to count users of the governor,
* slightly improved description of the governors in Kconfig.

 drivers/watchdog/Kconfig               | 25 ++++++++++++++++++
 drivers/watchdog/Makefile              |  2 ++
 drivers/watchdog/pretimeout_panic.c    | 47 ++++++++++++++++++++++++++++++++++
 drivers/watchdog/watchdog_pretimeout.h |  4 +++
 4 files changed, 78 insertions(+)
 create mode 100644 drivers/watchdog/pretimeout_panic.c

diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
index 354217e..bf5938a 100644
--- a/drivers/watchdog/Kconfig
+++ b/drivers/watchdog/Kconfig
@@ -1808,4 +1808,29 @@ 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_PANIC
+	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_PANIC
+	bool "panic"
+	select WATCHDOG_PRETIMEOUT_GOV_PANIC
+	help
+	  Use panic watchdog pretimeout governor by default, if
+	  a watchdog pretimeout event happens, consider that
+	  a watchdog feeder is dead and reboot is unavoidable.
+
+endchoice
+
+config WATCHDOG_PRETIMEOUT_GOV_PANIC
+	bool
+
+endif # WATCHDOG_PRETIMEOUT_GOV
+
 endif # WATCHDOG
diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
index cca47de..12df9c7 100644
--- a/drivers/watchdog/Makefile
+++ b/drivers/watchdog/Makefile
@@ -9,6 +9,8 @@ watchdog-objs	+= watchdog_core.o watchdog_dev.o
 
 watchdog-$(CONFIG_WATCHDOG_PRETIMEOUT_GOV)	+= watchdog_pretimeout.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
 # drivers and then the architecture independent "softdog" driver.
diff --git a/drivers/watchdog/pretimeout_panic.c b/drivers/watchdog/pretimeout_panic.c
new file mode 100644
index 0000000..4d5338d
--- /dev/null
+++ b/drivers/watchdog/pretimeout_panic.c
@@ -0,0 +1,47 @@
+/*
+ * Copyright (C) 2015-2016 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/kernel.h>
+#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,
+};
+
+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");
diff --git a/drivers/watchdog/watchdog_pretimeout.h b/drivers/watchdog/watchdog_pretimeout.h
index 9e08d56..8f2565e 100644
--- a/drivers/watchdog/watchdog_pretimeout.h
+++ b/drivers/watchdog/watchdog_pretimeout.h
@@ -22,6 +22,10 @@ void watchdog_unregister_pretimeout(struct watchdog_device *wdd);
 /* Interfaces to watchdog_dev.c */
 int watchdog_pretimeout_governor_name(struct watchdog_device *wdd, char *buf);
 
+#if IS_ENABLED(CONFIG_WATCHDOG_PRETIMEOUT_DEFAULT_GOV_PANIC)
+#define WATCHDOG_PRETIMEOUT_DEFAULT_GOV		"panic"
+#endif
+
 #else
 static inline int watchdog_register_pretimeout(struct watchdog_device *wdd)
 {
-- 
2.5.0

[PATCH v3 6/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>
---
Changes from v2 to v3:
* temporarily removed tristate option, the governor can be built-in only

Changes from v1 to v2:
* removed #ifdef CONFIG_WATCHDOG_PRETIMEOUT_DEFAULT_GOV_NOOP,
  now this selection is done in a centralized manner,
* added module owner reference to count users of the governor,
* slightly improved description of the governors in Kconfig.

 drivers/watchdog/Kconfig               | 11 ++++++++
 drivers/watchdog/Makefile              |  1 +
 drivers/watchdog/pretimeout_noop.c     | 47 ++++++++++++++++++++++++++++++++++
 drivers/watchdog/watchdog_pretimeout.h |  2 ++
 4 files changed, 61 insertions(+)
 create mode 100644 drivers/watchdog/pretimeout_noop.c

diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
index bf5938a..aee2eca 100644
--- a/drivers/watchdog/Kconfig
+++ b/drivers/watchdog/Kconfig
@@ -1826,11 +1826,22 @@ config WATCHDOG_PRETIMEOUT_DEFAULT_GOV_PANIC
 	  a watchdog pretimeout event happens, consider that
 	  a watchdog feeder is dead and reboot is unavoidable.
 
+config WATCHDOG_PRETIMEOUT_DEFAULT_GOV_NOOP
+	bool "noop"
+	select WATCHDOG_PRETIMEOUT_GOV_NOOP
+	help
+	  Use noop watchdog pretimeout governor by default. If noop
+	  governor is selected by a user, write a short message to
+	  the kernel log buffer and don't do any system changes.
+
 endchoice
 
 config WATCHDOG_PRETIMEOUT_GOV_PANIC
 	bool
 
+config WATCHDOG_PRETIMEOUT_GOV_NOOP
+	bool
+
 endif # WATCHDOG_PRETIMEOUT_GOV
 
 endif # WATCHDOG
diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
index 12df9c7..712f5bd 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_PANIC)	+= pretimeout_panic.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..7ea8ed3
--- /dev/null
+++ b/drivers/watchdog/pretimeout_noop.c
@@ -0,0 +1,47 @@
+/*
+ * Copyright (C) 2015-2016 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/printk.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)
+{
+	pr_alert("watchdog pretimeout event\n");
+}
+
+static struct watchdog_governor watchdog_gov_noop = {
+	.name		= "noop",
+	.pretimeout	= pretimeout_noop,
+};
+
+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("Panic watchdog pretimeout governor");
+MODULE_LICENSE("GPL");
diff --git a/drivers/watchdog/watchdog_pretimeout.h b/drivers/watchdog/watchdog_pretimeout.h
index 8f2565e..86ddcbd 100644
--- a/drivers/watchdog/watchdog_pretimeout.h
+++ b/drivers/watchdog/watchdog_pretimeout.h
@@ -24,6 +24,8 @@ int watchdog_pretimeout_governor_name(struct watchdog_device *wdd, char *buf);
 
 #if IS_ENABLED(CONFIG_WATCHDOG_PRETIMEOUT_DEFAULT_GOV_PANIC)
 #define WATCHDOG_PRETIMEOUT_DEFAULT_GOV		"panic"
+#elif IS_ENABLED(CONFIG_WATCHDOG_PRETIMEOUT_DEFAULT_GOV_NOOP)
+#define WATCHDOG_PRETIMEOUT_DEFAULT_GOV		"noop"
 #endif
 
 #else
-- 
2.5.0

Re: [PATCH v3 1/6] watchdog: add set_pretimeout interface

[Guenter Roeck]

On Tue, Jun 07, 2016 at 08:38:42PM +0300, Vladimir Zapolskiy wrote:
> From: Robin Gong <b38343@freescale.com>
> 
> Add set_pretimeout since our watchdog driver has those interfaces and
> obviously, the new common watchdog framework didn't implement this
> interface.
> 
> Signed-off-by: Robin Gong <b38343@freescale.com>
> [vzapolskiy: rebased, added an inline comment to describe new interface]
> Signed-off-by: Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com>
> ---
> Changes from v2 to v3:
> * none, new change
> 
> Changes from v1 to v2:
> * none, new change
> 
>  include/linux/watchdog.h | 10 ++++++++++

Please also document the new API function and variable in
Documentation/watchdog/watchdog-kernel-api.txt.


>  1 file changed, 10 insertions(+)
> 
> diff --git a/include/linux/watchdog.h b/include/linux/watchdog.h
> index 51732d6..e3d23d3 100644
> --- a/include/linux/watchdog.h
> +++ b/include/linux/watchdog.h
> @@ -28,6 +28,7 @@ struct watchdog_core_data;
>   * @ping:	The routine that sends a keepalive ping to the watchdog device.
>   * @status:	The routine that shows the status of the watchdog device.
>   * @set_timeout:The routine for setting the watchdog devices timeout value (in seconds).
> + * @set_pretimeout:The routine for setting the watchdog devices pretimeout (in seconds).
>   * @get_timeleft:The routine that gets the time left before a reset (in seconds).
>   * @restart:	The routine for restarting the machine.
>   * @ioctl:	The routines that handles extra ioctl calls.
> @@ -46,6 +47,7 @@ struct watchdog_ops {
>  	int (*ping)(struct watchdog_device *);
>  	unsigned int (*status)(struct watchdog_device *);
>  	int (*set_timeout)(struct watchdog_device *, unsigned int);
> +	int (*set_pretimeout)(struct watchdog_device *, unsigned int);
>  	unsigned int (*get_timeleft)(struct watchdog_device *);
>  	int (*restart)(struct watchdog_device *, unsigned long, void *);
>  	long (*ioctl)(struct watchdog_device *, unsigned int, unsigned long);
> @@ -95,6 +97,7 @@ struct watchdog_device {
>  	const struct watchdog_ops *ops;
>  	unsigned int bootstatus;
>  	unsigned int timeout;
> +	unsigned int pretimeout;
>  	unsigned int min_timeout;
>  	unsigned int max_timeout;
>  	unsigned int min_hw_heartbeat_ms;
> @@ -162,6 +165,13 @@ static inline bool watchdog_timeout_invalid(struct watchdog_device *wdd, unsigne
>  		 t > wdd->max_timeout);
>  }
>  
> +/* Use the following function to check if a pretimeout value is invalid */
> +static inline bool watchdog_pretimeout_invalid(struct watchdog_device *wdd,
> +					       unsigned int t)
> +{
> +	return wdd->timeout && t >= wdd->timeout;
> +}

If no timeout is configured, this would happily accept timeout values larger
than the maximum supported timeout. Given that we can not evaluate true or false
if timeout is not set or not known, I think this will have to be something like
	return !wdd->timeout || t >= wdd->timeout;
(unless somone has a better idea).

I am missing a function to initialize the pretimeout from a driver, similar
to watchdog_init_timeout(). Is this an oversight or on purpose ?

Thanks,
Guenter

Re: [PATCH v3 4/6] watchdog: add watchdog pretimeout framework

[Guenter Roeck]

On Tue, Jun 07, 2016 at 08:38:45PM +0300, 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 some watchdog devices.
> 
> A user selects a default watchdog pretimeout governor during
> compilation stage.
> 
> Watchdogs with WDIOF_PRETIMEOUT capability now have two device
> attributes in sysfs: pretimeout to display currently set pretimeout
> value and pretimeout_governor attribute to display the selected
> watchdog pretimeout governor.
> 
> Watchdogs with no WDIOF_PRETIMEOUT capability has no changes in
> sysfs, and such watchdog devices do not require the framework.
> 

One might read "require" as saying that it is mandatory for drivers
with pretimeout support. That is not what you mean, presumably ?

> Signed-off-by: Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com>
> ---
> Changes from v2 to v3:
> * essentially simplified the implementation due to removal of runtime
>   dynamic selection of watchdog pretimeout governors by a user, this
>   feature is supposed to be added later on
> * removed support of sleepable watchdog pretimeout governors
> * moved sysfs device attributes to watchdog_dev.c, this required to
>   add exported watchdog_pretimeout_governor_name() interface
> * if pretimeout framework is not selected, then pretimeout event
>   ends up in kernel panic -- this behaviour as a default one was asked
>   by Guenter
> 
> Changes from v1 to v2:
> * removed framework private bits from struct watchdog_governor,
> * centralized compile-time selection of a default governor in
>   watchdog_pretimeout.h,
> * added can_sleep option, now only sleeping governors (e.g. userspace)
>   will be executed in a special workqueue,
> * changed fallback logic, if a governor in use is removed, now this
>   situation is not possible, because in use governors have non-zero
>   module refcount
> 
>  drivers/watchdog/Kconfig               |   8 ++
>  drivers/watchdog/Makefile              |   5 +-
>  drivers/watchdog/watchdog_core.c       |   9 +++
>  drivers/watchdog/watchdog_dev.c        |  16 ++++
>  drivers/watchdog/watchdog_pretimeout.c | 134 +++++++++++++++++++++++++++++++++
>  drivers/watchdog/watchdog_pretimeout.h |  42 +++++++++++
>  include/linux/watchdog.h               |  13 ++++
>  7 files changed, 226 insertions(+), 1 deletion(-)
>  create mode 100644 drivers/watchdog/watchdog_pretimeout.c
>  create mode 100644 drivers/watchdog/watchdog_pretimeout.h
> 
Please document the new API functions.

> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
> index b54f26c..354217e 100644
> --- a/drivers/watchdog/Kconfig
> +++ b/drivers/watchdog/Kconfig
> @@ -1800,4 +1800,12 @@ config USBPCWATCHDOG
>  
>  	  Most people will say N.
>  
> +comment "Watchdog Pretimeout Governors"
> +
> +config WATCHDOG_PRETIMEOUT_GOV
> +	bool "Enable watchdog pretimeout governors"
> +	default n

I don't think 'default n" is needed.

> +	help
> +	  The option allows to select watchdog pretimeout governors.
> +
>  endif # WATCHDOG
> diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
> index a46e7c1..cca47de 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 7c3ba58..ae6c23a 100644
> --- a/drivers/watchdog/watchdog_core.c
> +++ b/drivers/watchdog/watchdog_core.c
> @@ -40,6 +40,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);
>  
> @@ -244,6 +245,13 @@ static int __watchdog_register_device(struct watchdog_device *wdd)
>  		}
>  	}
>  
> +	ret = watchdog_register_pretimeout(wdd);
> +	if (ret) {
> +		watchdog_dev_unregister(wdd);
> +		ida_simple_remove(&watchdog_ida, wdd->id);
> +		return ret;
> +	}
> +
>  	if (test_bit(WDOG_STOP_ON_REBOOT, &wdd->status)) {
>  		wdd->reboot_nb.notifier_call = watchdog_reboot_notifier;
>  
> @@ -251,6 +259,7 @@ static int __watchdog_register_device(struct watchdog_device *wdd)
>  		if (ret) {
>  			pr_err("watchdog%d: Cannot register reboot notifier (%d)\n",
>  			       wdd->id, ret);
> +			watchdog_unregister_pretimeout(wdd);
>  			watchdog_dev_unregister(wdd);
>  			ida_simple_remove(&watchdog_ida, wdd->id);
>  			return ret;

A bit at loss here. Why is it not necessary to unregister the pretimeout
from __watchdog_unregister_device() ? Doesn't this result in a stale pointer
and a memory leak ?

> diff --git a/drivers/watchdog/watchdog_dev.c b/drivers/watchdog/watchdog_dev.c
> index 87bbae7..c0fd743 100644
> --- a/drivers/watchdog/watchdog_dev.c
> +++ b/drivers/watchdog/watchdog_dev.c
> @@ -49,6 +49,7 @@
>  #include <linux/uaccess.h>	/* For copy_to_user/put_user/... */
>  
>  #include "watchdog_core.h"
> +#include "watchdog_pretimeout.h"
>  
>  /*
>   * struct watchdog_core_data - watchdog core internal data
> @@ -452,6 +453,16 @@ static ssize_t pretimeout_show(struct device *dev,
>  }
>  static DEVICE_ATTR_RO(pretimeout);
>  
> +static ssize_t pretimeout_governor_show(struct device *dev,
> +					struct device_attribute *devattr,
> +					char *buf)
> +{
> +	struct watchdog_device *wdd = dev_get_drvdata(dev);
> +
> +	return watchdog_pretimeout_governor_name(wdd, buf);
> +}
> +static DEVICE_ATTR_RO(pretimeout_governor);
> +
>  static umode_t wdt_is_visible(struct kobject *kobj, struct attribute *attr,
>  				int n)
>  {
> @@ -466,6 +477,10 @@ static umode_t wdt_is_visible(struct kobject *kobj, struct attribute *attr,
>  	else if (attr == &dev_attr_pretimeout.attr &&
>  		 !(wdd->info->options & WDIOF_PRETIMEOUT))
>  		mode = 0;
> +	else if (attr == &dev_attr_pretimeout_governor.attr &&
> +		 !((wdd->info->options & WDIOF_PRETIMEOUT) &&
> +		   IS_ENABLED(CONFIG_WATCHDOG_PRETIMEOUT_GOV)))

This is confusing. How about the following ?
								&&
		(!(wdd->info->options & WDIOF_PRETIMEOUT) ||
		 !IS_ENABLED(CONFIG_WATCHDOG_PRETIMEOUT_GOV)))

Sure, it is mathematically the same, but it would be much easier
to understand.

> +		mode = 0;
>  
>  	return mode;
>  }
> @@ -478,6 +493,7 @@ static struct attribute *wdt_attrs[] = {
>  	&dev_attr_status.attr,
>  	&dev_attr_nowayout.attr,
>  	&dev_attr_pretimeout.attr,
> +	&dev_attr_pretimeout_governor.attr,
>  	NULL,
>  };
>  
> diff --git a/drivers/watchdog/watchdog_pretimeout.c b/drivers/watchdog/watchdog_pretimeout.c
> new file mode 100644
> index 0000000..ebfc3d6
> --- /dev/null
> +++ b/drivers/watchdog/watchdog_pretimeout.c
> @@ -0,0 +1,134 @@
> +/*
> + * Copyright (C) 2015-2016 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/list.h>
> +#include <linux/slab.h>
> +#include <linux/spinlock.h>
> +#include <linux/watchdog.h>
> +
> +#include "watchdog_pretimeout.h"
> +
> +/* Default watchdog pretimeout governor */
> +static struct watchdog_governor *default_gov;
> +
> +/* 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);
> +
> +struct watchdog_pretimeout {
> +	struct watchdog_device		*wdd;
> +	struct list_head		entry;
> +};
> +
> +int watchdog_pretimeout_governor_name(struct watchdog_device *wdd, char *buf)
> +{
> +	int count = 0;
> +
Initialization seems unnecessary.

> +	spin_lock_irq(&pretimeout_lock);
> +	if (wdd->gov)
> +		count = sprintf(buf, "%s\n", wdd->gov->name);
> +	else
> +		count = sprintf(buf, "N/A\n");

Why not just return an empty string ?

> +	spin_unlock_irq(&pretimeout_lock);
> +
> +	return count;
> +}
> +
> +void watchdog_notify_pretimeout(struct watchdog_device *wdd)
> +{
> +	unsigned long flags;
> +
> +	if (!wdd)
> +		return;
> +
Why would this function ever be called with NULL wdd ?

> +	spin_lock_irqsave(&pretimeout_lock, flags);
> +	if (!wdd->gov) {
> +		spin_unlock_irqrestore(&pretimeout_lock, flags);
> +		return;
> +	}
> +
> +	wdd->gov->pretimeout(wdd);
> +	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_MAXLEN)
> +		return -EINVAL;

We don't usually do API parameter validation like this. Callers
are expected to write correct code.

> +
> +	if (default_gov)
> +		return -EBUSY;
> +

What does this mean ? That only one governor will be accepted ?

Maybe an API documentation will help, but I don't really understand the logic
here. If only one governor can be registered anyway, why bother keeping more
than one around ?

> +	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);
> +
> +	default_gov = gov;
> +
> +	return 0;
> +}
> +
> +void watchdog_unregister_governor(struct watchdog_governor *gov)
> +{
> +	if (!gov)
> +		return;
> +
Under what circumstances is this expected to happen ?

> +	if (default_gov == gov)
> +		default_gov = NULL;

Why bother ? Registration code would not accept a second registration anyway.

Individual drivers may (and will) still reference default_gov.

With the governors all being boolean, the remove function is pretty much
useless anyway. 

I understand that you plan to add features later, and maybe there is a plan
to correctly handle more than one governor. But it doesn't make sense to start
with a broken framework; whatever is introduced should work by itself and not
rely on it being fixed later.

> +}
> +
> +int watchdog_register_pretimeout(struct watchdog_device *wdd)
> +{
> +	struct watchdog_pretimeout *p;
> +
> +	if (!(wdd->info->options & WDIOF_PRETIMEOUT))
> +		return 0;
> +
> +	p = kzalloc(sizeof(*p), GFP_KERNEL);
> +	if (!p)
> +		return -ENOMEM;
> +
> +	spin_lock_irq(&pretimeout_lock);
> +	list_add(&p->entry, &pretimeout_list);
> +	p->wdd = wdd;
> +	wdd->gov = default_gov;

Unless I am missing something, at least per this patch, there is only
one governor that is ever accepted, which is the first one registered.
With that, I don't really see the value of keeping a per-device governor
list around. 

After all my comments, I must admit that I am quite at loss, maybe due to
the lack of documentation. If so, please take my feedback as talking
points to add to the documentation, to help others understand how this
framework is supposed to work.

Thanks,
Guenter

Re: [PATCH v3 1/6] watchdog: add set_pretimeout interface

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 804 bytes --]

On Tue, Jun 07, 2016 at 08:38:42PM +0300, Vladimir Zapolskiy wrote:
> From: Robin Gong <b38343@freescale.com>
> 
> Add set_pretimeout since our watchdog driver has those interfaces and
> obviously, the new common watchdog framework didn't implement this
> interface.
> 
> Signed-off-by: Robin Gong <b38343@freescale.com>
> [vzapolskiy: rebased, added an inline comment to describe new interface]
> Signed-off-by: Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com>

Why didn't you just take my patch and worked on it? This would have
added the documentation Guenter explicitly requested when reviewing
Robin's patch. And it would have get rid of the bug you have in the
*_invalid function. I also don't think the split-up into three patches
is necessary here, but that might just be me.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH v3 4/6] watchdog: add watchdog pretimeout framework

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 2108 bytes --]

On Tue, Jun 07, 2016 at 08:38:45PM +0300, 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 some watchdog devices.
> 
> A user selects a default watchdog pretimeout governor during
> compilation stage.
> 
> Watchdogs with WDIOF_PRETIMEOUT capability now have two device
> attributes in sysfs: pretimeout to display currently set pretimeout
> value and pretimeout_governor attribute to display the selected
> watchdog pretimeout governor.
> 
> Watchdogs with no WDIOF_PRETIMEOUT capability has no changes in
> sysfs, and such watchdog devices do not require the framework.
> 
> Signed-off-by: Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com>
> ---
> Changes from v2 to v3:
> * essentially simplified the implementation due to removal of runtime
>   dynamic selection of watchdog pretimeout governors by a user, this
>   feature is supposed to be added later on

Hmm, your call, but I'm not sure this will make the reviewing process
easier...

> * removed support of sleepable watchdog pretimeout governors

This does.

> * moved sysfs device attributes to watchdog_dev.c, this required to
>   add exported watchdog_pretimeout_governor_name() interface

Why this move? Before, all the pretimeout stuff was nicely encapsulated
in its own file which could be compiled out. Now things are mixing. What
was wrong with the approach I took?`

> @@ -244,6 +245,13 @@ static int __watchdog_register_device(struct watchdog_device *wdd)
>  		}
>  	}
>  
> +	ret = watchdog_register_pretimeout(wdd);
> +	if (ret) {
> +		watchdog_dev_unregister(wdd);
> +		ida_simple_remove(&watchdog_ida, wdd->id);
> +		return ret;
> +	}
> +

What is the advantage of adding it here instead of adding it in
watchdog_dev.c? I mean the files to control govenors are tied to the
watchdog_device anyhow, so I'd think it's cleaner to move all that
action to watchdog_dev instead of having this stray one in the core.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH v3 3/6] watchdog: add pretimeout read-only device attribute to sysfs

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 138 bytes --]


> +	else if (attr == &dev_attr_pretimeout.attr &&
> +		 !(wdd->info->options & WDIOF_PRETIMEOUT))
> +		mode = 0;

Good catch, this one.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH v3 5/6] watchdog: pretimeout: add panic pretimeout governor

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 438 bytes --]


> +static void pretimeout_panic(struct watchdog_device *wdd)
> +{
> +	panic("panic on watchdog pretimeout event\n");
> +}

And here we have the same redundant message again ("panic on") :( Did
you look at my patches at all? To me, it looks like you didn't or you
are intentionally trying to leave my changes out. I don't think they
were all bad. In the previous patch in watchdog_notify_pretimeout() you
used the shortened message BTW.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH v3 6/6] watchdog: pretimeout: add noop pretimeout governor

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 187 bytes --]

> +static void pretimeout_noop(struct watchdog_device *wdd)
> +{
> +	pr_alert("watchdog pretimeout event\n");
> +}

My version said which watchdog caused the event, why not adding that?


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

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

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 539 bytes --]


> Changes from v2 to v3:

This series was odd to review. I am used to that we build stuff on top
of each other to strive for the best technical solution. I didn't expect
that you like all of my changes, but at least some of them were obviously
correct. But since even those were ignored, it really feels like a step
backwards and thus, the reviewing time a bit wasted :(

Stuff like 64-bit support and the softdog timer (so people can actually
test the framework) is completely missing, too. Why not adding those?
They are easy patches.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH v3 1/6] watchdog: add set_pretimeout interface

[Vladimir Zapolskiy]

Hi Wolfram,

On 08.06.2016 09:34, Wolfram Sang wrote:
> On Tue, Jun 07, 2016 at 08:38:42PM +0300, Vladimir Zapolskiy wrote:
>> From: Robin Gong <b38343@freescale.com>
>>
>> Add set_pretimeout since our watchdog driver has those interfaces and
>> obviously, the new common watchdog framework didn't implement this
>> interface.
>>
>> Signed-off-by: Robin Gong <b38343@freescale.com>
>> [vzapolskiy: rebased, added an inline comment to describe new interface]
>> Signed-off-by: Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com>
> 
> Why didn't you just take my patch and worked on it? This would have
> added the documentation Guenter explicitly requested when reviewing
> Robin's patch. And it would have get rid of the bug you have in the
> *_invalid function. I also don't think the split-up into three patches
> is necessary here, but that might just be me.
> 

I agree, your version is obviously more advanced, I will take it for v4.

Best wishes,
Vladimir

Re: [PATCH v3 4/6] watchdog: add watchdog pretimeout framework

[Vladimir Zapolskiy]

Hi Guenter,

On 08.06.2016 00:43, Guenter Roeck wrote:
> On Tue, Jun 07, 2016 at 08:38:45PM +0300, 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 some watchdog devices.
>>
>> A user selects a default watchdog pretimeout governor during
>> compilation stage.
>>
>> Watchdogs with WDIOF_PRETIMEOUT capability now have two device
>> attributes in sysfs: pretimeout to display currently set pretimeout
>> value and pretimeout_governor attribute to display the selected
>> watchdog pretimeout governor.
>>
>> Watchdogs with no WDIOF_PRETIMEOUT capability has no changes in
>> sysfs, and such watchdog devices do not require the framework.
>>
> 
> One might read "require" as saying that it is mandatory for drivers
> with pretimeout support. That is not what you mean, presumably ?
> 

Well, I expressed a negation, formally it does not contradict to the
fact that a WDIOF_PRETIMEOUT capable device/driver can exist without
this code.

If a board has watchdog devices/drivers without WDIOF_PRETIMEOUT
capability (at the moment only kempld_wdt.c explicitly sets it)
the whole framework is dead code, though "do not require" may be too
strict.

But I agree, there is a room for ambiguity, I'll reword the paragraph.

>> Signed-off-by: Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com>
>> ---
>> Changes from v2 to v3:
>> * essentially simplified the implementation due to removal of runtime
>>   dynamic selection of watchdog pretimeout governors by a user, this
>>   feature is supposed to be added later on
>> * removed support of sleepable watchdog pretimeout governors
>> * moved sysfs device attributes to watchdog_dev.c, this required to
>>   add exported watchdog_pretimeout_governor_name() interface
>> * if pretimeout framework is not selected, then pretimeout event
>>   ends up in kernel panic -- this behaviour as a default one was asked
>>   by Guenter
>>
>> Changes from v1 to v2:
>> * removed framework private bits from struct watchdog_governor,
>> * centralized compile-time selection of a default governor in
>>   watchdog_pretimeout.h,
>> * added can_sleep option, now only sleeping governors (e.g. userspace)
>>   will be executed in a special workqueue,
>> * changed fallback logic, if a governor in use is removed, now this
>>   situation is not possible, because in use governors have non-zero
>>   module refcount
>>
>>  drivers/watchdog/Kconfig               |   8 ++
>>  drivers/watchdog/Makefile              |   5 +-
>>  drivers/watchdog/watchdog_core.c       |   9 +++
>>  drivers/watchdog/watchdog_dev.c        |  16 ++++
>>  drivers/watchdog/watchdog_pretimeout.c | 134 +++++++++++++++++++++++++++++++++
>>  drivers/watchdog/watchdog_pretimeout.h |  42 +++++++++++
>>  include/linux/watchdog.h               |  13 ++++
>>  7 files changed, 226 insertions(+), 1 deletion(-)
>>  create mode 100644 drivers/watchdog/watchdog_pretimeout.c
>>  create mode 100644 drivers/watchdog/watchdog_pretimeout.h
>>
> Please document the new API functions.
> 

Ok.

>> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
>> index b54f26c..354217e 100644
>> --- a/drivers/watchdog/Kconfig
>> +++ b/drivers/watchdog/Kconfig
>> @@ -1800,4 +1800,12 @@ config USBPCWATCHDOG
>>  
>>  	  Most people will say N.
>>  
>> +comment "Watchdog Pretimeout Governors"
>> +
>> +config WATCHDOG_PRETIMEOUT_GOV
>> +	bool "Enable watchdog pretimeout governors"
>> +	default n
> 
> I don't think 'default n" is needed.
> 

No strict objections, but probably 'default n' may save quite many
lines in defconfigs.

>> +	help
>> +	  The option allows to select watchdog pretimeout governors.
>> +
>>  endif # WATCHDOG
>> diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
>> index a46e7c1..cca47de 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 7c3ba58..ae6c23a 100644
>> --- a/drivers/watchdog/watchdog_core.c
>> +++ b/drivers/watchdog/watchdog_core.c
>> @@ -40,6 +40,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);
>>  
>> @@ -244,6 +245,13 @@ static int __watchdog_register_device(struct watchdog_device *wdd)
>>  		}
>>  	}
>>  
>> +	ret = watchdog_register_pretimeout(wdd);
>> +	if (ret) {
>> +		watchdog_dev_unregister(wdd);
>> +		ida_simple_remove(&watchdog_ida, wdd->id);
>> +		return ret;
>> +	}
>> +
>>  	if (test_bit(WDOG_STOP_ON_REBOOT, &wdd->status)) {
>>  		wdd->reboot_nb.notifier_call = watchdog_reboot_notifier;
>>  
>> @@ -251,6 +259,7 @@ static int __watchdog_register_device(struct watchdog_device *wdd)
>>  		if (ret) {
>>  			pr_err("watchdog%d: Cannot register reboot notifier (%d)\n",
>>  			       wdd->id, ret);
>> +			watchdog_unregister_pretimeout(wdd);
>>  			watchdog_dev_unregister(wdd);
>>  			ida_simple_remove(&watchdog_ida, wdd->id);
>>  			return ret;
> 
> A bit at loss here. Why is it not necessary to unregister the pretimeout
> from __watchdog_unregister_device() ? Doesn't this result in a stale pointer
> and a memory leak ?

This is a bug you found, thank you.

>> diff --git a/drivers/watchdog/watchdog_dev.c b/drivers/watchdog/watchdog_dev.c
>> index 87bbae7..c0fd743 100644
>> --- a/drivers/watchdog/watchdog_dev.c
>> +++ b/drivers/watchdog/watchdog_dev.c
>> @@ -49,6 +49,7 @@
>>  #include <linux/uaccess.h>	/* For copy_to_user/put_user/... */
>>  
>>  #include "watchdog_core.h"
>> +#include "watchdog_pretimeout.h"
>>  
>>  /*
>>   * struct watchdog_core_data - watchdog core internal data
>> @@ -452,6 +453,16 @@ static ssize_t pretimeout_show(struct device *dev,
>>  }
>>  static DEVICE_ATTR_RO(pretimeout);
>>  
>> +static ssize_t pretimeout_governor_show(struct device *dev,
>> +					struct device_attribute *devattr,
>> +					char *buf)
>> +{
>> +	struct watchdog_device *wdd = dev_get_drvdata(dev);
>> +
>> +	return watchdog_pretimeout_governor_name(wdd, buf);
>> +}
>> +static DEVICE_ATTR_RO(pretimeout_governor);
>> +
>>  static umode_t wdt_is_visible(struct kobject *kobj, struct attribute *attr,
>>  				int n)
>>  {
>> @@ -466,6 +477,10 @@ static umode_t wdt_is_visible(struct kobject *kobj, struct attribute *attr,
>>  	else if (attr == &dev_attr_pretimeout.attr &&
>>  		 !(wdd->info->options & WDIOF_PRETIMEOUT))
>>  		mode = 0;
>> +	else if (attr == &dev_attr_pretimeout_governor.attr &&
>> +		 !((wdd->info->options & WDIOF_PRETIMEOUT) &&
>> +		   IS_ENABLED(CONFIG_WATCHDOG_PRETIMEOUT_GOV)))
> 
> This is confusing. How about the following ?
> 								&&
> 		(!(wdd->info->options & WDIOF_PRETIMEOUT) ||
> 		 !IS_ENABLED(CONFIG_WATCHDOG_PRETIMEOUT_GOV)))
> 
> Sure, it is mathematically the same, but it would be much easier
> to understand.

No objections, originally I intended to preserve the preexisting style of

   attr == &dev_attr_XXX.attr && !(some expression on wdd)

>> +		mode = 0;
>>  
>>  	return mode;
>>  }
>> @@ -478,6 +493,7 @@ static struct attribute *wdt_attrs[] = {
>>  	&dev_attr_status.attr,
>>  	&dev_attr_nowayout.attr,
>>  	&dev_attr_pretimeout.attr,
>> +	&dev_attr_pretimeout_governor.attr,
>>  	NULL,
>>  };
>>  
>> diff --git a/drivers/watchdog/watchdog_pretimeout.c b/drivers/watchdog/watchdog_pretimeout.c
>> new file mode 100644
>> index 0000000..ebfc3d6
>> --- /dev/null
>> +++ b/drivers/watchdog/watchdog_pretimeout.c
>> @@ -0,0 +1,134 @@
>> +/*
>> + * Copyright (C) 2015-2016 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/list.h>
>> +#include <linux/slab.h>
>> +#include <linux/spinlock.h>
>> +#include <linux/watchdog.h>
>> +
>> +#include "watchdog_pretimeout.h"
>> +
>> +/* Default watchdog pretimeout governor */
>> +static struct watchdog_governor *default_gov;
>> +
>> +/* 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);
>> +
>> +struct watchdog_pretimeout {
>> +	struct watchdog_device		*wdd;
>> +	struct list_head		entry;
>> +};
>> +
>> +int watchdog_pretimeout_governor_name(struct watchdog_device *wdd, char *buf)
>> +{
>> +	int count = 0;
>> +
> Initialization seems unnecessary.
> 

Correct, will remove it.

>> +	spin_lock_irq(&pretimeout_lock);
>> +	if (wdd->gov)
>> +		count = sprintf(buf, "%s\n", wdd->gov->name);
>> +	else
>> +		count = sprintf(buf, "N/A\n");
> 
> Why not just return an empty string ?
> 

This is a matter of interface, whatever clearer for a user is
preferred. I don't have objections against returning an empty string,
I will change it in v4.

>> +	spin_unlock_irq(&pretimeout_lock);
>> +
>> +	return count;
>> +}
>> +
>> +void watchdog_notify_pretimeout(struct watchdog_device *wdd)
>> +{
>> +	unsigned long flags;
>> +
>> +	if (!wdd)
>> +		return;
>> +
> Why would this function ever be called with NULL wdd ?
> 

No reason for that, I believe Postel's law is not applicable here...

>> +	spin_lock_irqsave(&pretimeout_lock, flags);
>> +	if (!wdd->gov) {
>> +		spin_unlock_irqrestore(&pretimeout_lock, flags);
>> +		return;
>> +	}
>> +
>> +	wdd->gov->pretimeout(wdd);
>> +	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_MAXLEN)
>> +		return -EINVAL;
> 
> We don't usually do API parameter validation like this. Callers
> are expected to write correct code.

Same as above, I will remove the checks.

>> +
>> +	if (default_gov)
>> +		return -EBUSY;
>> +
> 
> What does this mean ? That only one governor will be accepted ?
> 
> Maybe an API documentation will help, but I don't really understand the logic
> here. If only one governor can be registered anyway, why bother keeping more
> than one around ?

Same as above.

>> +	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);
>> +
>> +	default_gov = gov;
>> +
>> +	return 0;
>> +}
>> +
>> +void watchdog_unregister_governor(struct watchdog_governor *gov)
>> +{
>> +	if (!gov)
>> +		return;
>> +
> Under what circumstances is this expected to happen ?
> 

Same as above.

>> +	if (default_gov == gov)
>> +		default_gov = NULL;
> 
> Why bother ? Registration code would not accept a second registration anyway.
> 
> Individual drivers may (and will) still reference default_gov.
> 
> With the governors all being boolean, the remove function is pretty much
> useless anyway. 
> 
> I understand that you plan to add features later, and maybe there is a plan
> to correctly handle more than one governor. But it doesn't make sense to start
> with a broken framework; whatever is introduced should work by itself and not
> rely on it being fixed later.

Yes, that's perfectly correct, I totally agree.

>> +}
>> +
>> +int watchdog_register_pretimeout(struct watchdog_device *wdd)
>> +{
>> +	struct watchdog_pretimeout *p;
>> +
>> +	if (!(wdd->info->options & WDIOF_PRETIMEOUT))
>> +		return 0;
>> +
>> +	p = kzalloc(sizeof(*p), GFP_KERNEL);
>> +	if (!p)
>> +		return -ENOMEM;
>> +
>> +	spin_lock_irq(&pretimeout_lock);
>> +	list_add(&p->entry, &pretimeout_list);
>> +	p->wdd = wdd;
>> +	wdd->gov = default_gov;
> 
> Unless I am missing something, at least per this patch, there is only
> one governor that is ever accepted, which is the first one registered.
> With that, I don't really see the value of keeping a per-device governor
> list around. 

This is not a per-device governor list (which actually was present in v2
and removed in v3), the description of this list is found around its
declaration, this is a list of watchdog devices, which can produce
a pretimeout event. Oh, "pretimeout_list" is a confusing naming...

The list can not be safely removed due to the fact that there is no
synchronization between registration of governors and watchdog devices,
at boot time a governor driver may be registered after completed
registration of some watchdog drivers, still these early watchdogs should
get a link to the governor, when it is ready, the list intends to
accumulate all of these registered WDIOF_PRETIMEOUT capable watchdogs.

> After all my comments, I must admit that I am quite at loss, maybe due to
> the lack of documentation. If so, please take my feedback as talking
> points to add to the documentation, to help others understand how this
> framework is supposed to work.
> 

Sure, thank you so much for review.

--
Best wishes,
Vladimir

Re: [PATCH v3 4/6] watchdog: add watchdog pretimeout framework

[Guenter Roeck]

On 06/08/2016 06:37 AM, Vladimir Zapolskiy wrote:

>>>
>>> +comment "Watchdog Pretimeout Governors"
>>> +
>>> +config WATCHDOG_PRETIMEOUT_GOV
>>> +	bool "Enable watchdog pretimeout governors"
>>> +	default n
>>
>> I don't think 'default n" is needed.
>>
>
> No strict objections, but probably 'default n' may save quite many
> lines in defconfigs.
>

I always wondered why it would be necessary to say "default n".
What is the difference between "default n" and no explicit default ?

Guenter

Re: [PATCH v3 4/6] watchdog: add watchdog pretimeout framework

[Vladimir Zapolskiy]

Hi Wolfram,

On 08.06.2016 09:54, Wolfram Sang wrote:
> On Tue, Jun 07, 2016 at 08:38:45PM +0300, 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 some watchdog devices.
>>
>> A user selects a default watchdog pretimeout governor during
>> compilation stage.
>>
>> Watchdogs with WDIOF_PRETIMEOUT capability now have two device
>> attributes in sysfs: pretimeout to display currently set pretimeout
>> value and pretimeout_governor attribute to display the selected
>> watchdog pretimeout governor.
>>
>> Watchdogs with no WDIOF_PRETIMEOUT capability has no changes in
>> sysfs, and such watchdog devices do not require the framework.
>>
>> Signed-off-by: Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com>
>> ---
>> Changes from v2 to v3:
>> * essentially simplified the implementation due to removal of runtime
>>   dynamic selection of watchdog pretimeout governors by a user, this
>>   feature is supposed to be added later on
> 
> Hmm, your call, but I'm not sure this will make the reviewing process
> easier...

Looks like the series hits a relatively vague contradiction described
in Documentation//development-process/5.Posting :

1) logically independent change should be formatted as a separate patch
2) the changes need to be considered in their final form

While in v1/v2 I've selected point 2) and even tried to defend it
in conversation with Guenter, for v3 I primarily choose point 1),
because all other features can be added on top.

Guenter, do you have any judgment?

>> * removed support of sleepable watchdog pretimeout governors
> 
> This does.

Same as above.

>> * moved sysfs device attributes to watchdog_dev.c, this required to
>>   add exported watchdog_pretimeout_governor_name() interface
> 
> Why this move? Before, all the pretimeout stuff was nicely encapsulated
> in its own file which could be compiled out. Now things are mixing. What
> was wrong with the approach I took?`

Simplification of the "struct device" life time management?
A lot of time and efforts were spent to centralize it, while you know
that I took both approaches, I tend to keep it exclusively in
watchdog_dev.c , probably Guenter can express his point of view.

>> @@ -244,6 +245,13 @@ static int __watchdog_register_device(struct watchdog_device *wdd)
>>  		}
>>  	}
>>  
>> +	ret = watchdog_register_pretimeout(wdd);
>> +	if (ret) {
>> +		watchdog_dev_unregister(wdd);
>> +		ida_simple_remove(&watchdog_ida, wdd->id);
>> +		return ret;
>> +	}
>> +
> 
> What is the advantage of adding it here instead of adding it in
> watchdog_dev.c? I mean the files to control govenors are tied to the
> watchdog_device anyhow, so I'd think it's cleaner to move all that
> action to watchdog_dev instead of having this stray one in the core.
> 

This makes sense, I will move it to watchdog_dev.c.

Thank you for review.

--
Best wishes,
Vladimir

Re: [PATCH v3 5/6] watchdog: pretimeout: add panic pretimeout governor

[Vladimir Zapolskiy]

Hi Wolfram,

On 08.06.2016 10:08, Wolfram Sang wrote:
> 
>> +static void pretimeout_panic(struct watchdog_device *wdd)
>> +{
>> +	panic("panic on watchdog pretimeout event\n");
>> +}
> 
> And here we have the same redundant message again ("panic on") :( 

I will remove it then.

> Did you look at my patches at all? To me, it looks like you didn't or
> you are intentionally trying to leave my changes out. I don't think they
> were all bad. 

No, they are not, and I'll include some of the changes to v4, if you don't
mind.

> In the previous patch in watchdog_notify_pretimeout() you used 
> the shortened message BTW.

--
With best wishes,
Vladimir

Re: [PATCH v3 6/6] watchdog: pretimeout: add noop pretimeout governor

[Vladimir Zapolskiy]

On 08.06.2016 10:10, Wolfram Sang wrote:
>> +static void pretimeout_noop(struct watchdog_device *wdd)
>> +{
>> +	pr_alert("watchdog pretimeout event\n");
>> +}
> 
> My version said which watchdog caused the event, why not adding that?
> 

I will add it. It's a bit sad that a virtual volatile id is reported,
but it is better than nothing...

Best wishes,
Vladimir

Re: [PATCH v3 4/6] watchdog: add watchdog pretimeout framework

[Vladimir Zapolskiy]

On 08.06.2016 16:53, Guenter Roeck wrote:
> On 06/08/2016 06:37 AM, Vladimir Zapolskiy wrote:
> 
>>>>
>>>> +comment "Watchdog Pretimeout Governors"
>>>> +
>>>> +config WATCHDOG_PRETIMEOUT_GOV
>>>> +	bool "Enable watchdog pretimeout governors"
>>>> +	default n
>>>
>>> I don't think 'default n" is needed.
>>>
>>
>> No strict objections, but probably 'default n' may save quite many
>> lines in defconfigs.
>>
> 
> I always wondered why it would be necessary to say "default n".
> What is the difference between "default n" and no explicit default ?
> 

I pointed out that it may have impact on defconfig, but experimentally
it has no effect.

Users of "make oldconfig" get a prompt in both cases as well.

Also I haven't found any difference for silentoldconfig, olddefconfig
and alldefconfig, I assume explicit "default n" and "def_bool n"
can be safely dropped.

--
Best wishes,
Vladimir

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

[Vladimir Zapolskiy]

Hi Wolfram,

On 08.06.2016 10:56, Wolfram Sang wrote:
> 
>> Changes from v2 to v3:
> 
> This series was odd to review. I am used to that we build stuff on top
> of each other to strive for the best technical solution. I didn't expect
> that you like all of my changes, but at least some of them were obviously
> correct. But since even those were ignored, it really feels like a step
> backwards and thus, the reviewing time a bit wasted :(
> 
> Stuff like 64-bit support and the softdog timer (so people can actually
> test the framework) is completely missing, too. Why not adding those?
> They are easy patches.
> 

I don't object or ignore your work, I'm sorry if this series makes you
feel sad, I'll do all my best for you in v4. I'm sincerely happy that
I found one more independent user of the feature, and I appreciate your
done work and review comments, even downloading, applying and adjusting
the changes took your time, and because I'm pretty sure you don't have
much spare time I value it.

Quite many times when I sent long non-trivial series in the past they
were either deterrent for review and plainly ignored or expectedly
caused too many review comments at once, that's why here in the cover
letter I emphasized :

>> In comparison to v1 and v2 this version does not have quite many
>> important features, because now the goal is to initiate technical
>> review of the simplest possible core change, the fat tail is put
>> aside at the moment.

I hope I managed to collect enough review comments (if Guenter adds
a note to your/my comments to v3 4/6, that would be perfect), and I'll
add your new changes and my cut-off changes to v4 pile.

With best wishes,
Vladimir

kbuild: default n removals? (was: Re: [PATCH v3 4/6] watchdog: add watchdog pretimeout framework)

[Joe Perches]

(Adding Michal Marek and linux-kbuild)

On Wed, 2016-06-08 at 18:11 +0300, Vladimir Zapolskiy wrote:
> On 08.06.2016 16:53, Guenter Roeck wrote:
> > On 06/08/2016 06:37 AM, Vladimir Zapolskiy wrote:
> > > > > +comment "Watchdog Pretimeout Governors"
> > > > > +
> > > > > +config WATCHDOG_PRETIMEOUT_GOV
> > > > > +	bool "Enable watchdog pretimeout governors"
> > > > > +	default n
> > > > I don't think 'default n" is needed.
> > > > 
> > > No strict objections, but probably 'default n' may save quite many
> > > lines in defconfigs.
> > > 
> > I always wondered why it would be necessary to say "default n".
> > What is the difference between "default n" and no explicit default ?
> > 
> I pointed out that it may have impact on defconfig, but experimentally
> it has no effect.
> 
> Users of "make oldconfig" get a prompt in both cases as well.
> 
> Also I haven't found any difference for silentoldconfig, olddefconfig
> and alldefconfig, I assume explicit "default n" and "def_bool n"
> can be safely dropped.

It's not completely clear removals are always appropriate.

from: Documentation/kbuild/kconfig-language.txt:
------------------------------------------------------------------
- default value: "default" <expr> ["if" <expr>]
  A config option can have any number of default values. If multiple
  default values are visible, only the first defined one is active.
  Default values are not limited to the menu entry where they are
  defined. This means the default can be defined somewhere else or be
  overridden by an earlier definition.
  The default value is only assigned to the config symbol if no other
  value was set by the user (via the input prompt above). If an input
  prompt is visible the default value is presented to the user and can
  be overridden by him.
  Optionally, dependencies only for this default value can be added with
  "if".
------------------------------------------------------------------

Michal?  Do you have an opinion or clarification?

Re: kbuild: default n removals? (was: Re: [PATCH v3 4/6] watchdog: add watchdog pretimeout framework)

[Guenter Roeck]

On Wed, Jun 08, 2016 at 08:38:52AM -0700, Joe Perches wrote:
> (Adding Michal Marek and linux-kbuild)
> 
> On Wed, 2016-06-08 at 18:11 +0300, Vladimir Zapolskiy wrote:
> > On 08.06.2016 16:53, Guenter Roeck wrote:
> > > On 06/08/2016 06:37 AM, Vladimir Zapolskiy wrote:
> > > > > > +comment "Watchdog Pretimeout Governors"
> > > > > > +
> > > > > > +config WATCHDOG_PRETIMEOUT_GOV
> > > > > > +	bool "Enable watchdog pretimeout governors"
> > > > > > +	default n
> > > > > I don't think 'default n" is needed.
> > > > > 
> > > > No strict objections, but probably 'default n' may save quite many
> > > > lines in defconfigs.
> > > > 
> > > I always wondered why it would be necessary to say "default n".
> > > What is the difference between "default n" and no explicit default ?
> > > 
> > I pointed out that it may have impact on defconfig, but experimentally
> > it has no effect.
> > 
> > Users of "make oldconfig" get a prompt in both cases as well.
> > 
> > Also I haven't found any difference for silentoldconfig, olddefconfig
> > and alldefconfig, I assume explicit "default n" and "def_bool n"
> > can be safely dropped.
> 
> It's not completely clear removals are always appropriate.
> 
> from: Documentation/kbuild/kconfig-language.txt:
> ------------------------------------------------------------------
> - default value: "default" <expr> ["if" <expr>]
>   A config option can have any number of default values. If multiple
>   default values are visible, only the first defined one is active.
>   Default values are not limited to the menu entry where they are
>   defined. This means the default can be defined somewhere else or be
>   overridden by an earlier definition.
>   The default value is only assigned to the config symbol if no other
>   value was set by the user (via the input prompt above). If an input
>   prompt is visible the default value is presented to the user and can
>   be overridden by him.
>   Optionally, dependencies only for this default value can be added with
>   "if".

This describes default settings such as
	default n if <expr>
	default y

which would set the default to y unless <expr> is true.

Question here was about the stand-alone "default n" which always
perplexed me.

Guenter

Re: [PATCH v3 4/6] watchdog: add watchdog pretimeout framework

[Guenter Roeck]

On Wed, Jun 08, 2016 at 05:08:09PM +0300, Vladimir Zapolskiy wrote:
> Hi Wolfram,
> 
> On 08.06.2016 09:54, Wolfram Sang wrote:
> > On Tue, Jun 07, 2016 at 08:38:45PM +0300, 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 some watchdog devices.
> >>
> >> A user selects a default watchdog pretimeout governor during
> >> compilation stage.
> >>
> >> Watchdogs with WDIOF_PRETIMEOUT capability now have two device
> >> attributes in sysfs: pretimeout to display currently set pretimeout
> >> value and pretimeout_governor attribute to display the selected
> >> watchdog pretimeout governor.
> >>
> >> Watchdogs with no WDIOF_PRETIMEOUT capability has no changes in
> >> sysfs, and such watchdog devices do not require the framework.
> >>
> >> Signed-off-by: Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com>
> >> ---
> >> Changes from v2 to v3:
> >> * essentially simplified the implementation due to removal of runtime
> >>   dynamic selection of watchdog pretimeout governors by a user, this
> >>   feature is supposed to be added later on
> > 
> > Hmm, your call, but I'm not sure this will make the reviewing process
> > easier...
> 
> Looks like the series hits a relatively vague contradiction described
> in Documentation//development-process/5.Posting :
> 
> 1) logically independent change should be formatted as a separate patch
> 2) the changes need to be considered in their final form
> 
> While in v1/v2 I've selected point 2) and even tried to defend it
> in conversation with Guenter, for v3 I primarily choose point 1),
> because all other features can be added on top.
> 
> Guenter, do you have any judgment?
> 
Kind of mixed feelings. I prefer a sequence of simple patches,
but most important is that patches are, at the end, complete.
I got the impression that your patch series wasn't complete.
That a series is incomplete is easier to hide if patches are
split into multiple parts.

Simple patches are easy to review, complex patches take more time.
But that doesn't help if the simple patches are incomplete, because
it means that the reviewer has to spend a lot of time trying to find
the missing pieces. Finding one missing piece suggests that there can
and will be more, meaning even more time needs to be spent. Since time
is always short at hand, a single missing piece may result in the entire
series to be put on the back-burner.

It can be difficult to see the forest because of all the trees,
and it can be difficult to see the individual trees in a forest.
It is all about trade-offs; nothing is black and white.

> >> * removed support of sleepable watchdog pretimeout governors
> > 
> > This does.
> 
> Same as above.
> 
> >> * moved sysfs device attributes to watchdog_dev.c, this required to
> >>   add exported watchdog_pretimeout_governor_name() interface
> > 
> > Why this move? Before, all the pretimeout stuff was nicely encapsulated
> > in its own file which could be compiled out. Now things are mixing. What
> > was wrong with the approach I took?`
> 
> Simplification of the "struct device" life time management?
> A lot of time and efforts were spent to centralize it, while you know
> that I took both approaches, I tend to keep it exclusively in
> watchdog_dev.c , probably Guenter can express his point of view.
> 

Yes, I am very much concerned that I'll have to go back and look into
all the lifetime issues again which we just managed to resolve.
Not looking forward to it :-(.

Guenter

Re: [PATCH v3 1/6] watchdog: add set_pretimeout interface

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 86 bytes --]


> I agree, your version is obviously more advanced, I will take it for v4.

Thanks!


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH v3 4/6] watchdog: add watchdog pretimeout framework

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 277 bytes --]


> >> * removed support of sleepable watchdog pretimeout governors
> > 
> > This does.
> 
> Same as above.

Just to make sure: I meant this does make reviewing easier since the
bottom half handling is the biggest thing to discuss and it is good to
seperate it out.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH v3 4/6] watchdog: add watchdog pretimeout framework

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 1029 bytes --]


> > >> * moved sysfs device attributes to watchdog_dev.c, this required to
> > >>   add exported watchdog_pretimeout_governor_name() interface
> > > 
> > > Why this move? Before, all the pretimeout stuff was nicely encapsulated
> > > in its own file which could be compiled out. Now things are mixing. What
> > > was wrong with the approach I took?`
> > 
> > Simplification of the "struct device" life time management?
> > A lot of time and efforts were spent to centralize it, while you know
> > that I took both approaches, I tend to keep it exclusively in
> > watchdog_dev.c , probably Guenter can express his point of view.
> > 
> 
> Yes, I am very much concerned that I'll have to go back and look into
> all the lifetime issues again which we just managed to resolve.
> Not looking forward to it :-(.

I will have a look at it again, too... but I still wonder if this is an
issue if the whole pretimeout governor framework is spawned from
watchdog_dev.c. And I understand things can be subtle here.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH v3 5/6] watchdog: pretimeout: add panic pretimeout governor

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 307 bytes --]


> > Did you look at my patches at all? To me, it looks like you didn't or
> > you are intentionally trying to leave my changes out. I don't think they
> > were all bad. 
> 
> No, they are not, and I'll include some of the changes to v4, if you don't
> mind.

Quite the contrary, I'd be happy :)


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

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

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 321 bytes --]


> I hope I managed to collect enough review comments (if Guenter adds
> a note to your/my comments to v3 4/6, that would be perfect), and I'll
> add your new changes and my cut-off changes to v4 pile.

Sounds great! Thanks for your understanding and sorry for my grumpy mood
yesterday.

Let's get this upstream then :)


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: kbuild: default n removals?

[Michal Marek]

On 2016-06-08 17:38, Joe Perches wrote:
> (Adding Michal Marek and linux-kbuild)
> 
> On Wed, 2016-06-08 at 18:11 +0300, Vladimir Zapolskiy wrote:
>> On 08.06.2016 16:53, Guenter Roeck wrote:
>>> On 06/08/2016 06:37 AM, Vladimir Zapolskiy wrote:
>>>>>> +comment "Watchdog Pretimeout Governors"
>>>>>> +
>>>>>> +config WATCHDOG_PRETIMEOUT_GOV
>>>>>> +	bool "Enable watchdog pretimeout governors"
>>>>>> +	default n
>>>>> I don't think 'default n" is needed.
>>>>>
>>>> No strict objections, but probably 'default n' may save quite many
>>>> lines in defconfigs.
>>>>
>>> I always wondered why it would be necessary to say "default n".
>>> What is the difference between "default n" and no explicit default ?
>>>
>> I pointed out that it may have impact on defconfig, but experimentally
>> it has no effect.
>>
>> Users of "make oldconfig" get a prompt in both cases as well.
>>
>> Also I haven't found any difference for silentoldconfig, olddefconfig
>> and alldefconfig, I assume explicit "default n" and "def_bool n"
>> can be safely dropped.

Yes, 'default n' is a noop.


> It's not completely clear removals are always appropriate.
> 
> from: Documentation/kbuild/kconfig-language.txt:
> ------------------------------------------------------------------
> - default value: "default" <expr> ["if" <expr>]
>   A config option can have any number of default values. If multiple
>   default values are visible, only the first defined one is active.
>   Default values are not limited to the menu entry where they are
>   defined. This means the default can be defined somewhere else or be
>   overridden by an earlier definition.
>   The default value is only assigned to the config symbol if no other
>   value was set by the user (via the input prompt above). If an input
>   prompt is visible the default value is presented to the user and can
>   be overridden by him.
>   Optionally, dependencies only for this default value can be added with
>   "if".
> ------------------------------------------------------------------
> 
> Michal?  Do you have an opinion or clarification?

As Guenter explained, there can be multiple default statements with
different if conditions. The first statement to match applies.

Michal

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

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 361 bytes --]

On Tue, Jun 07, 2016 at 08:38:41PM +0300, 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.

Any news? We'd really like to have it in 4.8, at least the non-sleeping
stuff.


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

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

[Guenter Roeck]

On 06/24/2016 02:46 AM, Wolfram Sang wrote:
> On Tue, Jun 07, 2016 at 08:38:41PM +0300, 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.
>
> Any news? We'd really like to have it in 4.8, at least the non-sleeping
> stuff.
>

Kind of cutting it close. v4 would have to be quite clean for this to happen,
and arrive very soon. I don't think it would be a good idea to rush this in.

Guenter

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

[Vladimir Zapolskiy]

Hi Wolfram, Guenter,

On 24.06.2016 16:45, Guenter Roeck wrote:
> On 06/24/2016 02:46 AM, Wolfram Sang wrote:
>> On Tue, Jun 07, 2016 at 08:38:41PM +0300, 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.
>>
>> Any news? We'd really like to have it in 4.8, at least the non-sleeping
>> stuff.
>>

I continued to cut the change into smaller pieces, but then in a series
a lot of code copy-paste cases between files appear, so eventually I think
that the essential part should be submitted undivided.

> Kind of cutting it close. v4 would have to be quite clean for this to happen,
> and arrive very soon. I don't think it would be a good idea to rush this in.
> 

Unfortunately the task didn't get enough priority on my end due to
something else, anyway it is not abandoned.

Guenter, some relatively small preliminary parts from Wolfram based
on Robin's work can be included into v4.8 IMHO.

--
With best wishes,
Vladimir