Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1416457 > unrolled thread

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

Back to article view | Back to linux.kernel

Contents

  [PATCH v3 0/6] watchdog: add watchdog pretimeout framework Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com> - 2016-06-07 19:50 +0200
    [PATCH v3 4/6] watchdog: add watchdog pretimeout framework Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com> - 2016-06-07 19:50 +0200
      Re: [PATCH v3 4/6] watchdog: add watchdog pretimeout framework Guenter Roeck <linux@roeck-us.net> - 2016-06-07 23:50 +0200
        Re: [PATCH v3 4/6] watchdog: add watchdog pretimeout framework Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com> - 2016-06-08 15:40 +0200
          Re: [PATCH v3 4/6] watchdog: add watchdog pretimeout framework Guenter Roeck <linux@roeck-us.net> - 2016-06-08 16:00 +0200
            Re: [PATCH v3 4/6] watchdog: add watchdog pretimeout framework Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com> - 2016-06-08 17:20 +0200
              kbuild: default n removals? (was: Re: [PATCH v3 4/6] watchdog: add  watchdog pretimeout framework) Joe Perches <joe@perches.com> - 2016-06-08 17:40 +0200
                Re: kbuild: default n removals? (was: Re: [PATCH v3 4/6] watchdog:  add watchdog pretimeout framework) Guenter Roeck <linux@roeck-us.net> - 2016-06-08 20:10 +0200
      Re: [PATCH v3 4/6] watchdog: add watchdog pretimeout framework Wolfram Sang <wsa@the-dreams.de> - 2016-06-08 09:00 +0200
        Re: [PATCH v3 4/6] watchdog: add watchdog pretimeout framework Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com> - 2016-06-08 16:10 +0200
          Re: [PATCH v3 4/6] watchdog: add watchdog pretimeout framework Guenter Roeck <linux@roeck-us.net> - 2016-06-08 20:30 +0200
            Re: [PATCH v3 4/6] watchdog: add watchdog pretimeout framework Wolfram Sang <wsa@the-dreams.de> - 2016-06-09 23:30 +0200
          Re: [PATCH v3 4/6] watchdog: add watchdog pretimeout framework Wolfram Sang <wsa@the-dreams.de> - 2016-06-09 23:20 +0200
    [PATCH v3 5/6] watchdog: pretimeout: add panic pretimeout governor Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com> - 2016-06-07 19:50 +0200
      Re: [PATCH v3 5/6] watchdog: pretimeout: add panic pretimeout  governor Wolfram Sang <wsa@the-dreams.de> - 2016-06-08 09:10 +0200
        Re: [PATCH v3 5/6] watchdog: pretimeout: add panic pretimeout  governor Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com> - 2016-06-08 16:30 +0200
          Re: [PATCH v3 5/6] watchdog: pretimeout: add panic pretimeout  governor Wolfram Sang <wsa@the-dreams.de> - 2016-06-09 23:30 +0200
    [PATCH v3 2/6] watchdog: add WDIOC_SETPRETIMEOUT and WDIOC_GETPRETIMEOUT Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com> - 2016-06-07 19:50 +0200
    [PATCH v3 3/6] watchdog: add pretimeout read-only device attribute to sysfs Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com> - 2016-06-07 19:50 +0200
      Re: [PATCH v3 3/6] watchdog: add pretimeout read-only device  attribute to sysfs Wolfram Sang <wsa@the-dreams.de> - 2016-06-08 09:00 +0200
    Re: [PATCH v3 0/6] watchdog: add watchdog pretimeout framework Wolfram Sang <wsa@the-dreams.de> - 2016-06-08 10:00 +0200
      Re: [PATCH v3 0/6] watchdog: add watchdog pretimeout framework Vladimir Zapolskiy <vladimir_zapolskiy@mentor.com> - 2016-06-08 17:40 +0200
        Re: [PATCH v3 0/6] watchdog: add watchdog pretimeout framework Wolfram Sang <wsa@the-dreams.de> - 2016-06-09 23:40 +0200

Page 1 of 2  [1] 2  Next page →

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

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

[toc] | [next] | [standalone]

#1416459 — [PATCH v3 4/6] watchdog: add watchdog pretimeout framework

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

(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?

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

[Multipart message — attachments visible in raw view] — view raw

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.

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

[Multipart message — attachments visible in raw view] — view raw

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

[toc] | [prev] | [next] | [standalone]

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

[Multipart message — attachments visible in raw view] — view raw

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

[toc] | [prev] | [next] | [standalone]

#1416462 — [PATCH v3 5/6] watchdog: pretimeout: add panic pretimeout governor

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

[toc] | [prev] | [next] | [standalone]

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

[Multipart message — attachments visible in raw view] — view raw

> +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.

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

[Multipart message — attachments visible in raw view] — view raw

> > 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 :)

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

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

[toc] | [prev] | [next] | [standalone]

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

[Multipart message — attachments visible in raw view] — view raw

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

Good catch, this one.

[toc] | [prev] | [next] | [standalone]

Page 1 of 2  [1] 2  Next page →

Back to top | Article view | linux.kernel

csiph-web