* [PATCH v3 1/6] watchdog: add set_pretimeout interface
2016-06-07 17:38 [PATCH v3 0/6] watchdog: add watchdog pretimeout framework Vladimir Zapolskiy
@ 2016-06-07 17:38 ` Vladimir Zapolskiy
2016-06-07 20:32 ` Guenter Roeck
2016-06-08 6:34 ` Wolfram Sang
2016-06-07 17:38 ` [PATCH v3 2/6] watchdog: add WDIOC_SETPRETIMEOUT and WDIOC_GETPRETIMEOUT Vladimir Zapolskiy
` (6 subsequent siblings)
7 siblings, 2 replies; 35+ messages in thread
From: Vladimir Zapolskiy @ 2016-06-07 17:38 UTC (permalink / raw)
To: Wim Van Sebroeck, Guenter Roeck
Cc: Wolfram Sang, Robin Gong, linux-watchdog, linux-kernel
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
^ permalink raw reply related [flat|nested] 35+ messages in thread* Re: [PATCH v3 1/6] watchdog: add set_pretimeout interface
2016-06-07 17:38 ` [PATCH v3 1/6] watchdog: add set_pretimeout interface Vladimir Zapolskiy
@ 2016-06-07 20:32 ` Guenter Roeck
2016-06-08 6:34 ` Wolfram Sang
1 sibling, 0 replies; 35+ messages in thread
From: Guenter Roeck @ 2016-06-07 20:32 UTC (permalink / raw)
To: Vladimir Zapolskiy
Cc: Wim Van Sebroeck, Wolfram Sang, Robin Gong, linux-watchdog, linux-kernel
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
^ permalink raw reply [flat|nested] 35+ messages in thread* Re: [PATCH v3 1/6] watchdog: add set_pretimeout interface
2016-06-07 17:38 ` [PATCH v3 1/6] watchdog: add set_pretimeout interface Vladimir Zapolskiy
2016-06-07 20:32 ` Guenter Roeck
@ 2016-06-08 6:34 ` Wolfram Sang
2016-06-08 12:58 ` Vladimir Zapolskiy
1 sibling, 1 reply; 35+ messages in thread
From: Wolfram Sang @ 2016-06-08 6:34 UTC (permalink / raw)
To: Vladimir Zapolskiy
Cc: Wim Van Sebroeck, Guenter Roeck, Robin Gong, linux-watchdog,
linux-kernel
[-- 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 --]
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH v3 1/6] watchdog: add set_pretimeout interface
2016-06-08 6:34 ` Wolfram Sang
@ 2016-06-08 12:58 ` Vladimir Zapolskiy
2016-06-09 21:12 ` Wolfram Sang
0 siblings, 1 reply; 35+ messages in thread
From: Vladimir Zapolskiy @ 2016-06-08 12:58 UTC (permalink / raw)
To: Wolfram Sang
Cc: Wim Van Sebroeck, Guenter Roeck, Robin Gong, linux-watchdog,
linux-kernel
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
^ permalink raw reply [flat|nested] 35+ messages in thread
* [PATCH v3 2/6] watchdog: add WDIOC_SETPRETIMEOUT and WDIOC_GETPRETIMEOUT
2016-06-07 17:38 [PATCH v3 0/6] watchdog: add watchdog pretimeout framework Vladimir Zapolskiy
2016-06-07 17:38 ` [PATCH v3 1/6] watchdog: add set_pretimeout interface Vladimir Zapolskiy
@ 2016-06-07 17:38 ` Vladimir Zapolskiy
2016-06-07 17:38 ` [PATCH v3 3/6] watchdog: add pretimeout read-only device attribute to sysfs Vladimir Zapolskiy
` (5 subsequent siblings)
7 siblings, 0 replies; 35+ messages in thread
From: Vladimir Zapolskiy @ 2016-06-07 17:38 UTC (permalink / raw)
To: Wim Van Sebroeck, Guenter Roeck
Cc: Wolfram Sang, Robin Gong, linux-watchdog, linux-kernel
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
^ permalink raw reply related [flat|nested] 35+ messages in thread* [PATCH v3 3/6] watchdog: add pretimeout read-only device attribute to sysfs
2016-06-07 17:38 [PATCH v3 0/6] watchdog: add watchdog pretimeout framework Vladimir Zapolskiy
2016-06-07 17:38 ` [PATCH v3 1/6] watchdog: add set_pretimeout interface Vladimir Zapolskiy
2016-06-07 17:38 ` [PATCH v3 2/6] watchdog: add WDIOC_SETPRETIMEOUT and WDIOC_GETPRETIMEOUT Vladimir Zapolskiy
@ 2016-06-07 17:38 ` Vladimir Zapolskiy
2016-06-08 6:57 ` Wolfram Sang
2016-06-07 17:38 ` [PATCH v3 4/6] watchdog: add watchdog pretimeout framework Vladimir Zapolskiy
` (4 subsequent siblings)
7 siblings, 1 reply; 35+ messages in thread
From: Vladimir Zapolskiy @ 2016-06-07 17:38 UTC (permalink / raw)
To: Wim Van Sebroeck, Guenter Roeck
Cc: Wolfram Sang, Robin Gong, linux-watchdog, linux-kernel
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
^ permalink raw reply related [flat|nested] 35+ messages in thread* [PATCH v3 4/6] watchdog: add watchdog pretimeout framework
2016-06-07 17:38 [PATCH v3 0/6] watchdog: add watchdog pretimeout framework Vladimir Zapolskiy
` (2 preceding siblings ...)
2016-06-07 17:38 ` [PATCH v3 3/6] watchdog: add pretimeout read-only device attribute to sysfs Vladimir Zapolskiy
@ 2016-06-07 17:38 ` Vladimir Zapolskiy
2016-06-07 21:43 ` Guenter Roeck
2016-06-08 6:54 ` [PATCH v3 4/6] watchdog: add watchdog pretimeout framework Wolfram Sang
2016-06-07 17:38 ` [PATCH v3 5/6] watchdog: pretimeout: add panic pretimeout governor Vladimir Zapolskiy
` (3 subsequent siblings)
7 siblings, 2 replies; 35+ messages in thread
From: Vladimir Zapolskiy @ 2016-06-07 17:38 UTC (permalink / raw)
To: Wim Van Sebroeck, Guenter Roeck
Cc: Wolfram Sang, Robin Gong, linux-watchdog, linux-kernel
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
^ permalink raw reply related [flat|nested] 35+ messages in thread* Re: [PATCH v3 4/6] watchdog: add watchdog pretimeout framework
2016-06-07 17:38 ` [PATCH v3 4/6] watchdog: add watchdog pretimeout framework Vladimir Zapolskiy
@ 2016-06-07 21:43 ` Guenter Roeck
2016-06-08 13:37 ` Vladimir Zapolskiy
2016-06-08 6:54 ` [PATCH v3 4/6] watchdog: add watchdog pretimeout framework Wolfram Sang
1 sibling, 1 reply; 35+ messages in thread
From: Guenter Roeck @ 2016-06-07 21:43 UTC (permalink / raw)
To: Vladimir Zapolskiy
Cc: Wim Van Sebroeck, Wolfram Sang, Robin Gong, linux-watchdog, linux-kernel
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
^ permalink raw reply [flat|nested] 35+ messages in thread* Re: [PATCH v3 4/6] watchdog: add watchdog pretimeout framework
2016-06-07 21:43 ` Guenter Roeck
@ 2016-06-08 13:37 ` Vladimir Zapolskiy
2016-06-08 13:53 ` Guenter Roeck
0 siblings, 1 reply; 35+ messages in thread
From: Vladimir Zapolskiy @ 2016-06-08 13:37 UTC (permalink / raw)
To: Guenter Roeck
Cc: Wim Van Sebroeck, Wolfram Sang, Robin Gong, linux-watchdog, linux-kernel
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
^ permalink raw reply [flat|nested] 35+ messages in thread* Re: [PATCH v3 4/6] watchdog: add watchdog pretimeout framework
2016-06-08 13:37 ` Vladimir Zapolskiy
@ 2016-06-08 13:53 ` Guenter Roeck
2016-06-08 15:11 ` Vladimir Zapolskiy
0 siblings, 1 reply; 35+ messages in thread
From: Guenter Roeck @ 2016-06-08 13:53 UTC (permalink / raw)
To: Vladimir Zapolskiy
Cc: Wim Van Sebroeck, Wolfram Sang, Robin Gong, linux-watchdog, linux-kernel
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
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH v3 4/6] watchdog: add watchdog pretimeout framework
2016-06-08 13:53 ` Guenter Roeck
@ 2016-06-08 15:11 ` Vladimir Zapolskiy
2016-06-08 15:38 ` kbuild: default n removals? (was: Re: [PATCH v3 4/6] watchdog: add watchdog pretimeout framework) Joe Perches
0 siblings, 1 reply; 35+ messages in thread
From: Vladimir Zapolskiy @ 2016-06-08 15:11 UTC (permalink / raw)
To: Guenter Roeck
Cc: Wim Van Sebroeck, Wolfram Sang, Robin Gong, linux-watchdog, linux-kernel
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
^ permalink raw reply [flat|nested] 35+ messages in thread
* kbuild: default n removals? (was: Re: [PATCH v3 4/6] watchdog: add watchdog pretimeout framework)
2016-06-08 15:11 ` Vladimir Zapolskiy
@ 2016-06-08 15:38 ` Joe Perches
2016-06-08 18:05 ` Guenter Roeck
2016-06-15 10:02 ` kbuild: default n removals? Michal Marek
0 siblings, 2 replies; 35+ messages in thread
From: Joe Perches @ 2016-06-08 15:38 UTC (permalink / raw)
To: Vladimir Zapolskiy, Guenter Roeck, Michal Marek
Cc: Wim Van Sebroeck, Wolfram Sang, Robin Gong, linux-watchdog,
linux-kernel, linux-kbuild
(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?
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: kbuild: default n removals? (was: Re: [PATCH v3 4/6] watchdog: add watchdog pretimeout framework)
2016-06-08 15:38 ` kbuild: default n removals? (was: Re: [PATCH v3 4/6] watchdog: add watchdog pretimeout framework) Joe Perches
@ 2016-06-08 18:05 ` Guenter Roeck
2016-06-15 10:02 ` kbuild: default n removals? Michal Marek
1 sibling, 0 replies; 35+ messages in thread
From: Guenter Roeck @ 2016-06-08 18:05 UTC (permalink / raw)
To: Joe Perches
Cc: Vladimir Zapolskiy, Michal Marek, Wim Van Sebroeck, Wolfram Sang,
Robin Gong, linux-watchdog, linux-kernel, linux-kbuild
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
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: kbuild: default n removals?
2016-06-08 15:38 ` kbuild: default n removals? (was: Re: [PATCH v3 4/6] watchdog: add watchdog pretimeout framework) Joe Perches
2016-06-08 18:05 ` Guenter Roeck
@ 2016-06-15 10:02 ` Michal Marek
1 sibling, 0 replies; 35+ messages in thread
From: Michal Marek @ 2016-06-15 10:02 UTC (permalink / raw)
To: Joe Perches
Cc: Vladimir Zapolskiy, Guenter Roeck, Wim Van Sebroeck,
Wolfram Sang, Robin Gong, linux-watchdog, linux-kernel,
linux-kbuild
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
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH v3 4/6] watchdog: add watchdog pretimeout framework
2016-06-07 17:38 ` [PATCH v3 4/6] watchdog: add watchdog pretimeout framework Vladimir Zapolskiy
2016-06-07 21:43 ` Guenter Roeck
@ 2016-06-08 6:54 ` Wolfram Sang
2016-06-08 14:08 ` Vladimir Zapolskiy
1 sibling, 1 reply; 35+ messages in thread
From: Wolfram Sang @ 2016-06-08 6:54 UTC (permalink / raw)
To: Vladimir Zapolskiy
Cc: Wim Van Sebroeck, Guenter Roeck, Robin Gong, linux-watchdog,
linux-kernel
[-- 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 --]
^ permalink raw reply [flat|nested] 35+ messages in thread* Re: [PATCH v3 4/6] watchdog: add watchdog pretimeout framework
2016-06-08 6:54 ` [PATCH v3 4/6] watchdog: add watchdog pretimeout framework Wolfram Sang
@ 2016-06-08 14:08 ` Vladimir Zapolskiy
2016-06-08 18:20 ` Guenter Roeck
2016-06-09 21:14 ` Wolfram Sang
0 siblings, 2 replies; 35+ messages in thread
From: Vladimir Zapolskiy @ 2016-06-08 14:08 UTC (permalink / raw)
To: Wolfram Sang
Cc: Wim Van Sebroeck, Guenter Roeck, Robin Gong, linux-watchdog,
linux-kernel
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
^ permalink raw reply [flat|nested] 35+ messages in thread* Re: [PATCH v3 4/6] watchdog: add watchdog pretimeout framework
2016-06-08 14:08 ` Vladimir Zapolskiy
@ 2016-06-08 18:20 ` Guenter Roeck
2016-06-09 21:22 ` Wolfram Sang
2016-06-09 21:14 ` Wolfram Sang
1 sibling, 1 reply; 35+ messages in thread
From: Guenter Roeck @ 2016-06-08 18:20 UTC (permalink / raw)
To: Vladimir Zapolskiy
Cc: Wolfram Sang, Wim Van Sebroeck, Robin Gong, linux-watchdog, linux-kernel
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
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH v3 4/6] watchdog: add watchdog pretimeout framework
2016-06-08 18:20 ` Guenter Roeck
@ 2016-06-09 21:22 ` Wolfram Sang
0 siblings, 0 replies; 35+ messages in thread
From: Wolfram Sang @ 2016-06-09 21:22 UTC (permalink / raw)
To: Guenter Roeck
Cc: Vladimir Zapolskiy, Wim Van Sebroeck, Robin Gong, linux-watchdog,
linux-kernel
[-- 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 --]
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH v3 4/6] watchdog: add watchdog pretimeout framework
2016-06-08 14:08 ` Vladimir Zapolskiy
2016-06-08 18:20 ` Guenter Roeck
@ 2016-06-09 21:14 ` Wolfram Sang
1 sibling, 0 replies; 35+ messages in thread
From: Wolfram Sang @ 2016-06-09 21:14 UTC (permalink / raw)
To: Vladimir Zapolskiy
Cc: Wim Van Sebroeck, Guenter Roeck, Robin Gong, linux-watchdog,
linux-kernel
[-- 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 --]
^ permalink raw reply [flat|nested] 35+ messages in thread
* [PATCH v3 5/6] watchdog: pretimeout: add panic pretimeout governor
2016-06-07 17:38 [PATCH v3 0/6] watchdog: add watchdog pretimeout framework Vladimir Zapolskiy
` (3 preceding siblings ...)
2016-06-07 17:38 ` [PATCH v3 4/6] watchdog: add watchdog pretimeout framework Vladimir Zapolskiy
@ 2016-06-07 17:38 ` Vladimir Zapolskiy
2016-06-08 7:08 ` Wolfram Sang
2016-06-07 17:38 ` [PATCH v3 6/6] watchdog: pretimeout: add noop " Vladimir Zapolskiy
` (2 subsequent siblings)
7 siblings, 1 reply; 35+ messages in thread
From: Vladimir Zapolskiy @ 2016-06-07 17:38 UTC (permalink / raw)
To: Wim Van Sebroeck, Guenter Roeck
Cc: Wolfram Sang, Robin Gong, linux-watchdog, linux-kernel
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
^ permalink raw reply related [flat|nested] 35+ messages in thread* Re: [PATCH v3 5/6] watchdog: pretimeout: add panic pretimeout governor
2016-06-07 17:38 ` [PATCH v3 5/6] watchdog: pretimeout: add panic pretimeout governor Vladimir Zapolskiy
@ 2016-06-08 7:08 ` Wolfram Sang
2016-06-08 14:28 ` Vladimir Zapolskiy
0 siblings, 1 reply; 35+ messages in thread
From: Wolfram Sang @ 2016-06-08 7:08 UTC (permalink / raw)
To: Vladimir Zapolskiy
Cc: Wim Van Sebroeck, Guenter Roeck, Robin Gong, linux-watchdog,
linux-kernel
[-- 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 --]
^ permalink raw reply [flat|nested] 35+ messages in thread* Re: [PATCH v3 5/6] watchdog: pretimeout: add panic pretimeout governor
2016-06-08 7:08 ` Wolfram Sang
@ 2016-06-08 14:28 ` Vladimir Zapolskiy
2016-06-09 21:23 ` Wolfram Sang
0 siblings, 1 reply; 35+ messages in thread
From: Vladimir Zapolskiy @ 2016-06-08 14:28 UTC (permalink / raw)
To: Wolfram Sang
Cc: Wim Van Sebroeck, Guenter Roeck, Robin Gong, linux-watchdog,
linux-kernel
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
^ permalink raw reply [flat|nested] 35+ messages in thread* Re: [PATCH v3 5/6] watchdog: pretimeout: add panic pretimeout governor
2016-06-08 14:28 ` Vladimir Zapolskiy
@ 2016-06-09 21:23 ` Wolfram Sang
0 siblings, 0 replies; 35+ messages in thread
From: Wolfram Sang @ 2016-06-09 21:23 UTC (permalink / raw)
To: Vladimir Zapolskiy
Cc: Wim Van Sebroeck, Guenter Roeck, Robin Gong, linux-watchdog,
linux-kernel
[-- 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 --]
^ permalink raw reply [flat|nested] 35+ messages in thread
* [PATCH v3 6/6] watchdog: pretimeout: add noop pretimeout governor
2016-06-07 17:38 [PATCH v3 0/6] watchdog: add watchdog pretimeout framework Vladimir Zapolskiy
` (4 preceding siblings ...)
2016-06-07 17:38 ` [PATCH v3 5/6] watchdog: pretimeout: add panic pretimeout governor Vladimir Zapolskiy
@ 2016-06-07 17:38 ` Vladimir Zapolskiy
2016-06-08 7:10 ` Wolfram Sang
2016-06-08 7:56 ` [PATCH v3 0/6] watchdog: add watchdog pretimeout framework Wolfram Sang
2016-06-24 9:46 ` Wolfram Sang
7 siblings, 1 reply; 35+ messages in thread
From: Vladimir Zapolskiy @ 2016-06-07 17:38 UTC (permalink / raw)
To: Wim Van Sebroeck, Guenter Roeck
Cc: Wolfram Sang, Robin Gong, linux-watchdog, linux-kernel
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
^ permalink raw reply related [flat|nested] 35+ messages in thread* Re: [PATCH v3 6/6] watchdog: pretimeout: add noop pretimeout governor
2016-06-07 17:38 ` [PATCH v3 6/6] watchdog: pretimeout: add noop " Vladimir Zapolskiy
@ 2016-06-08 7:10 ` Wolfram Sang
2016-06-08 14:32 ` Vladimir Zapolskiy
0 siblings, 1 reply; 35+ messages in thread
From: Wolfram Sang @ 2016-06-08 7:10 UTC (permalink / raw)
To: Vladimir Zapolskiy
Cc: Wim Van Sebroeck, Guenter Roeck, Robin Gong, linux-watchdog,
linux-kernel
[-- 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 --]
^ permalink raw reply [flat|nested] 35+ messages in thread* Re: [PATCH v3 6/6] watchdog: pretimeout: add noop pretimeout governor
2016-06-08 7:10 ` Wolfram Sang
@ 2016-06-08 14:32 ` Vladimir Zapolskiy
0 siblings, 0 replies; 35+ messages in thread
From: Vladimir Zapolskiy @ 2016-06-08 14:32 UTC (permalink / raw)
To: Wolfram Sang
Cc: Wim Van Sebroeck, Guenter Roeck, Robin Gong, linux-watchdog,
linux-kernel
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
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH v3 0/6] watchdog: add watchdog pretimeout framework
2016-06-07 17:38 [PATCH v3 0/6] watchdog: add watchdog pretimeout framework Vladimir Zapolskiy
` (5 preceding siblings ...)
2016-06-07 17:38 ` [PATCH v3 6/6] watchdog: pretimeout: add noop " Vladimir Zapolskiy
@ 2016-06-08 7:56 ` Wolfram Sang
2016-06-08 15:35 ` Vladimir Zapolskiy
2016-06-24 9:46 ` Wolfram Sang
7 siblings, 1 reply; 35+ messages in thread
From: Wolfram Sang @ 2016-06-08 7:56 UTC (permalink / raw)
To: Vladimir Zapolskiy
Cc: Wim Van Sebroeck, Guenter Roeck, Robin Gong, linux-watchdog,
linux-kernel
[-- 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 --]
^ permalink raw reply [flat|nested] 35+ messages in thread* Re: [PATCH v3 0/6] watchdog: add watchdog pretimeout framework
2016-06-08 7:56 ` [PATCH v3 0/6] watchdog: add watchdog pretimeout framework Wolfram Sang
@ 2016-06-08 15:35 ` Vladimir Zapolskiy
2016-06-09 21:30 ` Wolfram Sang
0 siblings, 1 reply; 35+ messages in thread
From: Vladimir Zapolskiy @ 2016-06-08 15:35 UTC (permalink / raw)
To: Wolfram Sang, Guenter Roeck
Cc: Wim Van Sebroeck, Robin Gong, linux-watchdog, linux-kernel
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
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH v3 0/6] watchdog: add watchdog pretimeout framework
2016-06-07 17:38 [PATCH v3 0/6] watchdog: add watchdog pretimeout framework Vladimir Zapolskiy
` (6 preceding siblings ...)
2016-06-08 7:56 ` [PATCH v3 0/6] watchdog: add watchdog pretimeout framework Wolfram Sang
@ 2016-06-24 9:46 ` Wolfram Sang
2016-06-24 13:45 ` Guenter Roeck
7 siblings, 1 reply; 35+ messages in thread
From: Wolfram Sang @ 2016-06-24 9:46 UTC (permalink / raw)
To: Vladimir Zapolskiy
Cc: Wim Van Sebroeck, Guenter Roeck, Robin Gong, linux-watchdog,
linux-kernel
[-- 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 --]
^ permalink raw reply [flat|nested] 35+ messages in thread* Re: [PATCH v3 0/6] watchdog: add watchdog pretimeout framework
2016-06-24 9:46 ` Wolfram Sang
@ 2016-06-24 13:45 ` Guenter Roeck
2016-06-24 22:13 ` Vladimir Zapolskiy
0 siblings, 1 reply; 35+ messages in thread
From: Guenter Roeck @ 2016-06-24 13:45 UTC (permalink / raw)
To: Wolfram Sang, Vladimir Zapolskiy
Cc: Wim Van Sebroeck, Robin Gong, linux-watchdog, linux-kernel
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
^ permalink raw reply [flat|nested] 35+ messages in thread
* Re: [PATCH v3 0/6] watchdog: add watchdog pretimeout framework
2016-06-24 13:45 ` Guenter Roeck
@ 2016-06-24 22:13 ` Vladimir Zapolskiy
0 siblings, 0 replies; 35+ messages in thread
From: Vladimir Zapolskiy @ 2016-06-24 22:13 UTC (permalink / raw)
To: Guenter Roeck, Wolfram Sang
Cc: Wim Van Sebroeck, Robin Gong, linux-watchdog, linux-kernel
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
^ permalink raw reply [flat|nested] 35+ messages in thread