[PATCH 0/11 v3] Generic Watchdog Timer Driver

[PATCH 0/11 v3] Generic Watchdog Timer Driver

[Wim Van Sebroeck]

Hi All,

To reduce copying the same code over and over in each watchdog device driver, Alan Cox and myself constructed a new framework/API that consolidates the common watchdog timer driver functions.
I incorporated the changes/feedback that I received from the second post.

This framework/API consists of the following patches:
part  1: Introduction of the WatchDog Timer Driver Core
part  2: Add the basic ioctl functionality
part  3: Add the WDIOC_KEEPALIVE ioctl
part  4: Add the WDIOC_SETOPTIONS ioctl
part  5: Add the WDIOC_SETTIMEOUT and WDIOC_GETTIMEOUT ioctl
part  6: Add the Magic Close feature
part  7: Add the nowayout feature
part  8: Add support for a miscdev parent device
part  9: Add support for extra ioctl calls
part 10: Add the minimum and maximum timeout parameters.
part 11: Remove llseek

The code will also be added to linux-2.6-watchdog-next.

Changes since V2:
* all "flags" are unsigned
* timeout values are also unsigned
* removed unnececessary debugging
* clean-up comments
* the core will be in drivers/watchdog/ and not in drivers/watchdog/core/

Kind regards,
Wim.

[PATCH 01/11] watchdog: WatchDog Timer Driver Core - Add basic framework

[Wim Van Sebroeck]

The WatchDog Timer Driver Core is a framework
that contains the common code for all watchdog-driver's.
It also introduces a watchdog device structure and the
operations that go with it.

This is the introduction of this framework. This part
supports the minimal watchdog userspace API (or with
other words: the functionality to use /dev/watchdog's
open, release and write functionality as defined in
the simplest watchdog API). Extra functionality will
follow in the next set of patches.

Signed-off-by: Alan Cox <alan@lxorguk.ukuu.org.uk>
Signed-off-by: Wim Van Sebroeck <wim@iguana.be>
---
 Documentation/watchdog/watchdog-kernel-api.txt |  103 ++++++++++
 drivers/watchdog/Kconfig                       |   11 +
 drivers/watchdog/Makefile                      |    4 +
 drivers/watchdog/watchdog_core.c               |  108 ++++++++++
 drivers/watchdog/watchdog_dev.c                |  261 ++++++++++++++++++++++++
 drivers/watchdog/watchdog_dev.h                |   33 +++
 include/linux/watchdog.h                       |   27 +++
 7 files changed, 547 insertions(+), 0 deletions(-)
 create mode 100644 Documentation/watchdog/watchdog-kernel-api.txt
 create mode 100644 drivers/watchdog/watchdog_core.c
 create mode 100644 drivers/watchdog/watchdog_dev.c
 create mode 100644 drivers/watchdog/watchdog_dev.h

diff --git a/Documentation/watchdog/watchdog-kernel-api.txt b/Documentation/watchdog/watchdog-kernel-api.txt
new file mode 100644
index 0000000..03c1066
--- /dev/null
+++ b/Documentation/watchdog/watchdog-kernel-api.txt
@@ -0,0 +1,103 @@
+The Linux WatchDog Timer Driver Core kernel API.
+===============================================
+Last reviewed: 09-Jul-2011
+
+Wim Van Sebroeck <wim@iguana.be>
+
+Introduction
+------------
+This document does not describe what a WatchDog Timer (WDT) Driver or Device is.
+It also does not describe the API which can be used by user space to communicate
+with a WatchDog Timer. If you want to know this then please read the following
+file: Documentation/watchdog/watchdog-api.txt .
+
+So what does this document describe? It describes the API that can be used by
+WatchDog Timer Drivers that want to use the WatchDog Timer Driver Core
+Framework. This framework provides all interfacing towards user space so that
+the same code does not have to be reproduced each time. This also means that
+a watchdog timer driver then only needs to provide the different routines
+(operations) that control the watchdog timer (WDT).
+
+The API
+-------
+Each watchdog timer driver that wants to use the WatchDog Timer Driver Core
+must #include <linux/watchdog.h> (you would have to do this anyway when
+writing a watchdog device driver). This include file contains following
+register/unregister routines:
+
+extern int watchdog_register_device(struct watchdog_device *);
+extern void watchdog_unregister_device(struct watchdog_device *);
+
+The watchdog_register_device routine registers a watchdog timer device.
+The parameter of this routine is a pointer to a watchdog_device structure.
+This routine returns zero on success and a negative errno code for failure.
+
+The watchdog_unregister_device routine deregisters a registered watchdog timer
+device. The parameter of this routine is the pointer to the registered
+watchdog_device structure.
+
+The watchdog device structure looks like this:
+
+struct watchdog_device {
+	const struct watchdog_info *info;
+	const struct watchdog_ops *ops;
+	void *priv;
+	unsigned long status;
+};
+
+It contains following fields:
+* info: a pointer to a watchdog_info structure. This structure gives some
+  additional information about the watchdog timer itself. (Like it's unique name)
+* ops: a pointer to the list of watchdog operations that the watchdog supports.
+* priv: a pointer to the private data of a watchdog device
+* status: this field contains a number of status bits that give extra
+  information about the status of the device (Like: is the device opened via
+  the /dev/watchdog interface or not, ...)
+
+The list of watchdog operations is defined as:
+
+struct watchdog_ops {
+	struct module *owner;
+	/* mandatory operations */
+	int (*start)(struct watchdog_device *);
+	int (*stop)(struct watchdog_device *);
+	/* optional operations */
+	int (*ping)(struct watchdog_device *);
+};
+
+It is important that you first define the module owner of the watchdog timer
+driver's operations. This module owner will be used to lock the module when
+the watchdog is active. (This to avoid a system crash when you unload the
+module and /dev/watchdog is still open).
+Some operations are mandatory and some are optional. The mandatory operations
+are:
+* start: this is a pointer to the routine that starts the watchdog timer
+  device.
+  The routine needs a pointer to the watchdog timer device structure as a
+  parameter. It returns zero on success or a negative errno code for failure.
+* stop: with this routine the watchdog timer device is being stopped.
+  The routine needs a pointer to the watchdog timer device structure as a
+  parameter. It returns zero on success or a negative errno code for failure.
+  Some watchdog timer hardware can only be started and not be stopped. The
+  driver supporting this hardware needs to make sure that a start and stop
+  routine is being provided. This can be done by using a timer in the driver
+  that regularly sends a keepalive ping to the watchdog timer hardware.
+
+Not all watchdog timer hardware supports the same functionality. That's why
+all other routines/operations are optional. They only need to be provided if
+they are supported. These optional routines/operations are:
+* ping: this is the routine that sends a keepalive ping to the watchdog timer
+  hardware.
+  The routine needs a pointer to the watchdog timer device structure as a
+  parameter. It returns zero on success or a negative errno code for failure.
+  Most hardware that does not support this as a separate function uses the
+  start function to restart the watchdog timer hardware. And that's also what
+  the watchdog timer driver core does: to send a keepalive ping to the watchdog
+  timer hardware it will either use the ping operation (when available) or the
+  start operation (when the ping operation is not available).
+
+The status bits should (preferably) be set with the set_bit and clear_bit alike
+bit-operations. The status bits that are defined are:
+* WDOG_DEV_OPEN: this status bit shows whether or not the watchdog device
+  was opened via /dev/watchdog.
+  (This bit should only be used by the WatchDog Timer Driver Core).
diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
index 372bc64..edb0f1b 100644
--- a/drivers/watchdog/Kconfig
+++ b/drivers/watchdog/Kconfig
@@ -28,6 +28,17 @@ menuconfig WATCHDOG
 
 if WATCHDOG
 
+config WATCHDOG_CORE
+	tristate "WatchDog Timer Driver Core"
+	---help---
+	  Say Y here if you want to use the new watchdog timer driver core.
+	  This driver provides a framework for all watchdog timer drivers
+	  and gives them the /dev/watchdog interface (and later also the
+	  sysfs interface).
+
+	  To compile this driver as a module, choose M here: the module will
+	  be called watchdog.
+
 config WATCHDOG_NOWAYOUT
 	bool "Disable watchdog shutdown on close"
 	help
diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
index b2ddff7..7b26484 100644
--- a/drivers/watchdog/Makefile
+++ b/drivers/watchdog/Makefile
@@ -2,6 +2,10 @@
 # Makefile for the WatchDog device drivers.
 #
 
+# The WatchDog Timer Driver Core.
+watchdog-objs	+= watchdog_core.o watchdog_dev.o
+obj-$(CONFIG_WATCHDOG_CORE)	+= watchdog.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
new file mode 100644
index 0000000..79684ad
--- /dev/null
+++ b/drivers/watchdog/watchdog_core.c
@@ -0,0 +1,108 @@
+/*
+ *	watchdog_core.c
+ *
+ *	(c) Copyright 2008-2011 Alan Cox <alan@lxorguk.ukuu.org.uk>,
+ *						All Rights Reserved.
+ *
+ *	(c) Copyright 2008-2011 Wim Van Sebroeck <wim@iguana.be>.
+ *
+ *	This source code is part of the generic code that can be used
+ *	by all the watchdog timer drivers.
+ *
+ *	Based on source code of the following authors:
+ *	  Matt Domsch <Matt_Domsch@dell.com>,
+ *	  Rob Radez <rob@osinvestor.com>,
+ *	  Rusty Lynch <rusty@linux.co.intel.com>
+ *	  Satyam Sharma <satyam@infradead.org>
+ *	  Randy Dunlap <randy.dunlap@oracle.com>
+ *
+ *	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.
+ *
+ *	Neither Alan Cox, CymruNet Ltd., Wim Van Sebroeck nor Iguana vzw.
+ *	admit liability nor provide warranty for any of this software.
+ *	This material is provided "AS-IS" and at no charge.
+ */
+
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
+/*
+ *	Include files
+ */
+#include <linux/module.h>	/* For EXPORT_SYMBOL/module stuff/... */
+#include <linux/types.h>	/* For standard types */
+#include <linux/errno.h>	/* For the -ENODEV/... values */
+#include <linux/kernel.h>	/* For printk/panic/... */
+#include <linux/watchdog.h>	/* For watchdog specific items */
+#include <linux/init.h>		/* For __init/__exit/... */
+
+#include "watchdog_dev.h"	/* For watchdog_dev_register/... */
+
+/**
+ *	watchdog_register_device	-	register a watchdog device
+ *	@wdd: watchdog device
+ *
+ *	Register a watchdog device with the kernel so that the
+ *	watchdog timer can be accessed from userspace.
+ *
+ *	A zero is returned on success and a negative errno code for
+ *	failure.
+ */
+int watchdog_register_device(struct watchdog_device *wdd)
+{
+	int ret;
+
+	/* Make sure we have a valid watchdog_device structure */
+	if (wdd == NULL || wdd->info == NULL || wdd->ops == NULL)
+		return -EINVAL;
+
+	/* Make sure that the mandatory operations are supported */
+	if (wdd->ops->start == NULL || wdd->ops->stop == NULL)
+		return -EINVAL;
+
+	/*
+	 * Note: now that all watchdog_device data has been verified, we
+	 * will not check this anymore in other functions. If data gets
+	 * corrupted in a later stage then we expect a kernel panic!
+	 */
+
+	/* We only support 1 watchdog device via the /dev/watchdog interface */
+	ret = watchdog_dev_register(wdd);
+	if (ret) {
+		pr_err("error registering /dev/watchdog (err=%d).\n", ret);
+		return ret;
+	}
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(watchdog_register_device);
+
+/**
+ *	watchdog_unregister_device	-	unregister a watchdog device
+ *	@wdd: watchdog device to unregister
+ *
+ *	Unregister a watchdog device that was previously successfully
+ *	registered with watchdog_register_device().
+ */
+void watchdog_unregister_device(struct watchdog_device *wdd)
+{
+	int ret;
+
+	/* Make sure we have a valid watchdog_device structure */
+	if (wdd == NULL)
+		return;
+
+	/* remove the /dev/watchdog interface */
+	ret = watchdog_dev_unregister(wdd);
+	if (ret)
+		pr_err("error unregistering /dev/watchdog (err=%d).\n", ret);
+}
+EXPORT_SYMBOL_GPL(watchdog_unregister_device);
+
+MODULE_AUTHOR("Alan Cox <alan@lxorguk.ukuu.org.uk>");
+MODULE_AUTHOR("Wim Van Sebroeck <wim@iguana.be>");
+MODULE_DESCRIPTION("WatchDog Timer Driver Core");
+MODULE_LICENSE("GPL");
+MODULE_SUPPORTED_DEVICE("watchdog");
diff --git a/drivers/watchdog/watchdog_dev.c b/drivers/watchdog/watchdog_dev.c
new file mode 100644
index 0000000..c47154c
--- /dev/null
+++ b/drivers/watchdog/watchdog_dev.c
@@ -0,0 +1,261 @@
+/*
+ *	watchdog_dev.c
+ *
+ *	(c) Copyright 2008-2011 Alan Cox <alan@lxorguk.ukuu.org.uk>,
+ *						All Rights Reserved.
+ *
+ *	(c) Copyright 2008-2011 Wim Van Sebroeck <wim@iguana.be>.
+ *
+ *
+ *	This source code is part of the generic code that can be used
+ *	by all the watchdog timer drivers.
+ *
+ *	This part of the generic code takes care of the following
+ *	misc device: /dev/watchdog.
+ *
+ *	Based on source code of the following authors:
+ *	  Matt Domsch <Matt_Domsch@dell.com>,
+ *	  Rob Radez <rob@osinvestor.com>,
+ *	  Rusty Lynch <rusty@linux.co.intel.com>
+ *	  Satyam Sharma <satyam@infradead.org>
+ *	  Randy Dunlap <randy.dunlap@oracle.com>
+ *
+ *	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.
+ *
+ *	Neither Alan Cox, CymruNet Ltd., Wim Van Sebroeck nor Iguana vzw.
+ *	admit liability nor provide warranty for any of this software.
+ *	This material is provided "AS-IS" and at no charge.
+ */
+
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
+/*
+ *	Include files
+ */
+#include <linux/module.h>	/* For module stuff/... */
+#include <linux/types.h>	/* For standard types (like size_t) */
+#include <linux/errno.h>	/* For the -ENODEV/... values */
+#include <linux/kernel.h>	/* For printk/panic/... */
+#include <linux/fs.h>		/* For file operations */
+#include <linux/watchdog.h>	/* For watchdog specific items */
+#include <linux/miscdevice.h>	/* For handling misc devices */
+#include <linux/init.h>		/* For __init/__exit/... */
+#include <linux/uaccess.h>	/* For copy_to_user/put_user/... */
+
+/*
+ *	Locally used variables
+ */
+
+/* make sure we only register one /dev/watchdog device */
+static unsigned long watchdog_dev_busy;
+/* the watchdog device behind /dev/watchdog */
+static struct watchdog_device *wdd;
+
+/*
+ *	/dev/watchdog operations
+ */
+
+/*
+ *	watchdog_ping: ping the watchdog.
+ *	@wddev: the watchdog device to ping
+ *
+ *	If the watchdog has no own ping operation then it needs to be
+ *	restarted via the start operation. This wrapper function does
+ *	exactly that.
+ */
+
+static int watchdog_ping(struct watchdog_device *wddev)
+{
+	if (wddev->ops->ping)
+		return wddev->ops->ping(wddev);  /* ping the watchdog */
+	else
+		return wddev->ops->start(wddev); /* restart the watchdog */
+}
+
+/*
+ *	watchdog_write: writes to the watchdog.
+ *	@file: file from VFS
+ *	@data: user address of data
+ *	@len: length of data
+ *	@ppos: pointer to the file offset
+ *
+ *	A write to a watchdog device is defined as a keepalive ping.
+ */
+
+static ssize_t watchdog_write(struct file *file, const char __user *data,
+						size_t len, loff_t *ppos)
+{
+	size_t i;
+	char c;
+
+	if (len == 0)
+		return 0;
+
+	for (i = 0; i != len; i++) {
+		if (get_user(c, data + i))
+			return -EFAULT;
+	}
+
+	/* someone wrote to us, so we send the watchdog a keepalive ping */
+	watchdog_ping(wdd);
+
+	return len;
+}
+
+/*
+ *	watchdog_open: open the /dev/watchdog device.
+ *	@inode: inode of device
+ *	@file: file handle to device
+ *
+ *	When the /dev/watchdog device gets opened, we start the watchdog.
+ *	Watch out: the /dev/watchdog device is single open, so we make sure
+ *	it can only be opened once.
+ */
+
+static int watchdog_open(struct inode *inode, struct file *file)
+{
+	int err = -EBUSY;
+
+	/* the watchdog is single open! */
+	if (test_and_set_bit(WDOG_DEV_OPEN, &wdd->status))
+		return -EBUSY;
+
+	/*
+	 * If the /dev/watchdog device is open, we don't want the module
+	 * to be unloaded.
+	 */
+	if (!try_module_get(wdd->ops->owner))
+		goto out;
+
+	/* start the watchdog */
+	err = wdd->ops->start(wdd);
+	if (err < 0)
+		goto out_mod;
+
+	/* dev/watchdog is a virtual (and thus non-seekable) filesystem */
+	return nonseekable_open(inode, file);
+
+out_mod:
+	module_put(wdd->ops->owner);
+out:
+	clear_bit(WDOG_DEV_OPEN, &wdd->status);
+	return err;
+}
+
+/*
+ *      watchdog_release: release the /dev/watchdog device.
+ *      @inode: inode of device
+ *      @file: file handle to device
+ *
+ *	This is the code for when /dev/watchdog gets closed.
+ */
+
+static int watchdog_release(struct inode *inode, struct file *file)
+{
+	int err;
+
+	/* stop the watchdog */
+	err = wdd->ops->stop(wdd);
+	if (err != 0) {
+		pr_crit("%s: watchdog did not stop!\n", wdd->info->identity);
+		watchdog_ping(wdd);
+	}
+
+	/* Allow the owner module to be unloaded again */
+	module_put(wdd->ops->owner);
+
+	/* make sure that /dev/watchdog can be re-opened */
+	clear_bit(WDOG_DEV_OPEN, &wdd->status);
+
+	return 0;
+}
+
+/*
+ *	/dev/watchdog kernel interfaces
+ */
+
+static const struct file_operations watchdog_fops = {
+	.owner		= THIS_MODULE,
+	.llseek		= no_llseek,
+	.write		= watchdog_write,
+	.open		= watchdog_open,
+	.release	= watchdog_release,
+};
+
+static struct miscdevice watchdog_miscdev = {
+	.minor		= WATCHDOG_MINOR,
+	.name		= "watchdog",
+	.fops		= &watchdog_fops,
+};
+
+/*
+ *	/dev/watchdog register and unregister functions
+ */
+
+/*
+ *	watchdog_dev_register:
+ *	@watchdog: watchdog device
+ *
+ *	Register a watchdog device as /dev/watchdog. /dev/watchdog
+ *	is actually a miscdevice and thus we set it up like that.
+ */
+
+int watchdog_dev_register(struct watchdog_device *watchdog)
+{
+	int err;
+
+	/* Only one device can register for /dev/watchdog */
+	if (test_and_set_bit(0, &watchdog_dev_busy)) {
+		pr_err("only one watchdog can use /dev/watchdog.\n");
+		return -EBUSY;
+	}
+
+	wdd = watchdog;
+
+	/* Register the miscdevice */
+	err = misc_register(&watchdog_miscdev);
+	if (err != 0) {
+		pr_err("%s: cannot register miscdev on minor=%d (err=%d).\n",
+			watchdog->info->identity, WATCHDOG_MINOR, err);
+		goto out;
+	}
+
+	return 0;
+
+out:
+	wdd = NULL;
+	clear_bit(0, &watchdog_dev_busy);
+	return err;
+}
+
+/*
+ *	watchdog_dev_unregister:
+ *	@watchdog: watchdog device
+ *
+ *	Deregister the /dev/watchdog device.
+ */
+
+int watchdog_dev_unregister(struct watchdog_device *watchdog)
+{
+	/* Check that a watchdog device was registered in the past */
+	if (!test_bit(0, &watchdog_dev_busy) || !wdd)
+		return -ENODEV;
+
+	/* We can only unregister the watchdog device that was registered */
+	if (watchdog != wdd) {
+		pr_err("%s: watchdog was not registered as /dev/watchdog.\n",
+			watchdog->info->identity);
+		return -ENODEV;
+	}
+
+	/* Unregister the miscdevice */
+	misc_deregister(&watchdog_miscdev);
+	wdd = NULL;
+	clear_bit(0, &watchdog_dev_busy);
+	return 0;
+}
+
+MODULE_ALIAS_MISCDEV(WATCHDOG_MINOR);
diff --git a/drivers/watchdog/watchdog_dev.h b/drivers/watchdog/watchdog_dev.h
new file mode 100644
index 0000000..bc7612b
--- /dev/null
+++ b/drivers/watchdog/watchdog_dev.h
@@ -0,0 +1,33 @@
+/*
+ *	watchdog_core.h
+ *
+ *	(c) Copyright 2008-2011 Alan Cox <alan@lxorguk.ukuu.org.uk>,
+ *						All Rights Reserved.
+ *
+ *	(c) Copyright 2008-2011 Wim Van Sebroeck <wim@iguana.be>.
+ *
+ *	This source code is part of the generic code that can be used
+ *	by all the watchdog timer drivers.
+ *
+ *	Based on source code of the following authors:
+ *	  Matt Domsch <Matt_Domsch@dell.com>,
+ *	  Rob Radez <rob@osinvestor.com>,
+ *	  Rusty Lynch <rusty@linux.co.intel.com>
+ *	  Satyam Sharma <satyam@infradead.org>
+ *	  Randy Dunlap <randy.dunlap@oracle.com>
+ *
+ *	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.
+ *
+ *	Neither Alan Cox, CymruNet Ltd., Wim Van Sebroeck nor Iguana vzw.
+ *	admit liability nor provide warranty for any of this software.
+ *	This material is provided "AS-IS" and at no charge.
+ */
+
+/*
+ *	Functions/procedures to be called by the core
+ */
+int watchdog_dev_register(struct watchdog_device *);
+int watchdog_dev_unregister(struct watchdog_device *);
diff --git a/include/linux/watchdog.h b/include/linux/watchdog.h
index 011bcfe..40333ff 100644
--- a/include/linux/watchdog.h
+++ b/include/linux/watchdog.h
@@ -59,6 +59,33 @@ struct watchdog_info {
 #define WATCHDOG_NOWAYOUT	0
 #endif
 
+struct watchdog_ops;
+struct watchdog_device;
+
+/* The watchdog-devices operations */
+struct watchdog_ops {
+	struct module *owner;
+	/* mandatory operations */
+	int (*start)(struct watchdog_device *);
+	int (*stop)(struct watchdog_device *);
+	/* optional operations */
+	int (*ping)(struct watchdog_device *);
+};
+
+/* The structure that defines a watchdog device */
+struct watchdog_device {
+	const struct watchdog_info *info;
+	const struct watchdog_ops *ops;
+	void *priv;
+	unsigned long status;
+/* Bit numbers for status flags */
+#define WDOG_DEV_OPEN		1	/* Opened via /dev/watchdog ? */
+};
+
+/* drivers/watchdog/core/watchdog_core.c */
+extern int watchdog_register_device(struct watchdog_device *);
+extern void watchdog_unregister_device(struct watchdog_device *);
+
 #endif	/* __KERNEL__ */
 
 #endif  /* ifndef _LINUX_WATCHDOG_H */
-- 
1.7.6

[PATCH 02/11] watchdog: WatchDog Timer Driver Core - Add basic ioctl functionality

[Wim Van Sebroeck]

This part add's the basic ioctl functionality to the
WatchDog Timer Driver Core framework. The supported
ioctl call's are:
	WDIOC_GETSUPPORT
	WDIOC_GETSTATUS
	WDIOC_GETBOOTSTATUS

Signed-off-by: Alan Cox <alan@lxorguk.ukuu.org.uk>
Signed-off-by: Wim Van Sebroeck <wim@iguana.be>
---
 Documentation/watchdog/watchdog-kernel-api.txt |    6 ++++
 drivers/watchdog/watchdog_dev.c                |   33 ++++++++++++++++++++++++
 include/linux/watchdog.h                       |    2 +
 3 files changed, 41 insertions(+), 0 deletions(-)

diff --git a/Documentation/watchdog/watchdog-kernel-api.txt b/Documentation/watchdog/watchdog-kernel-api.txt
index 03c1066..544eb19 100644
--- a/Documentation/watchdog/watchdog-kernel-api.txt
+++ b/Documentation/watchdog/watchdog-kernel-api.txt
@@ -41,6 +41,7 @@ The watchdog device structure looks like this:
 struct watchdog_device {
 	const struct watchdog_info *info;
 	const struct watchdog_ops *ops;
+	unsigned int bootstatus;
 	void *priv;
 	unsigned long status;
 };
@@ -49,6 +50,8 @@ It contains following fields:
 * info: a pointer to a watchdog_info structure. This structure gives some
   additional information about the watchdog timer itself. (Like it's unique name)
 * ops: a pointer to the list of watchdog operations that the watchdog supports.
+* bootstatus: status of the device after booting (reported with watchdog
+  WDIOF_* status bits).
 * priv: a pointer to the private data of a watchdog device
 * status: this field contains a number of status bits that give extra
   information about the status of the device (Like: is the device opened via
@@ -63,6 +66,7 @@ struct watchdog_ops {
 	int (*stop)(struct watchdog_device *);
 	/* optional operations */
 	int (*ping)(struct watchdog_device *);
+	unsigned int (*status)(struct watchdog_device *);
 };
 
 It is important that you first define the module owner of the watchdog timer
@@ -95,6 +99,8 @@ they are supported. These optional routines/operations are:
   the watchdog timer driver core does: to send a keepalive ping to the watchdog
   timer hardware it will either use the ping operation (when available) or the
   start operation (when the ping operation is not available).
+* status: this routine checks the status of the watchdog timer device. The
+  status of the device is reported with watchdog WDIOF_* status flags/bits.
 
 The status bits should (preferably) be set with the set_bit and clear_bit alike
 bit-operations. The status bits that are defined are:
diff --git a/drivers/watchdog/watchdog_dev.c b/drivers/watchdog/watchdog_dev.c
index c47154c..bebea38 100644
--- a/drivers/watchdog/watchdog_dev.c
+++ b/drivers/watchdog/watchdog_dev.c
@@ -106,6 +106,38 @@ static ssize_t watchdog_write(struct file *file, const char __user *data,
 }
 
 /*
+ *	watchdog_ioctl: handle the different ioctl's for the watchdog device.
+ *	@file: file handle to the device
+ *	@cmd: watchdog command
+ *	@arg: argument pointer
+ *
+ *	The watchdog API defines a common set of functions for all watchdogs
+ *	according to their available features.
+ */
+
+static long watchdog_ioctl(struct file *file, unsigned int cmd,
+							unsigned long arg)
+{
+	void __user *argp = (void __user *)arg;
+	int __user *p = argp;
+	unsigned int val;
+
+	switch (cmd) {
+	case WDIOC_GETSUPPORT:
+		return copy_to_user(argp, wdd->info,
+			sizeof(struct watchdog_info)) ? -EFAULT : 0;
+	case WDIOC_GETSTATUS:
+		val = wdd->ops->status ? wdd->ops->status(wdd) : 0;
+		return put_user(val, p);
+	case WDIOC_GETBOOTSTATUS:
+		return put_user(wdd->bootstatus, p);
+	default:
+		return -ENOTTY;
+	}
+	return -ENOTTY;
+}
+
+/*
  *	watchdog_open: open the /dev/watchdog device.
  *	@inode: inode of device
  *	@file: file handle to device
@@ -181,6 +213,7 @@ static const struct file_operations watchdog_fops = {
 	.owner		= THIS_MODULE,
 	.llseek		= no_llseek,
 	.write		= watchdog_write,
+	.unlocked_ioctl	= watchdog_ioctl,
 	.open		= watchdog_open,
 	.release	= watchdog_release,
 };
diff --git a/include/linux/watchdog.h b/include/linux/watchdog.h
index 40333ff..a2b639b 100644
--- a/include/linux/watchdog.h
+++ b/include/linux/watchdog.h
@@ -70,12 +70,14 @@ struct watchdog_ops {
 	int (*stop)(struct watchdog_device *);
 	/* optional operations */
 	int (*ping)(struct watchdog_device *);
+	unsigned int (*status)(struct watchdog_device *);
 };
 
 /* The structure that defines a watchdog device */
 struct watchdog_device {
 	const struct watchdog_info *info;
 	const struct watchdog_ops *ops;
+	unsigned int bootstatus;
 	void *priv;
 	unsigned long status;
 /* Bit numbers for status flags */
-- 
1.7.6

Re: [PATCH 02/11] watchdog: WatchDog Timer Driver Core - Add basic ioctl functionality

[Wolfram Sang]

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

> +static long watchdog_ioctl(struct file *file, unsigned int cmd,
> +							unsigned long arg)
> +{
> +	void __user *argp = (void __user *)arg;
> +	int __user *p = argp;
> +	unsigned int val;
> +
> +	switch (cmd) {
> +	case WDIOC_GETSUPPORT:
> +		return copy_to_user(argp, wdd->info,
> +			sizeof(struct watchdog_info)) ? -EFAULT : 0;
> +	case WDIOC_GETSTATUS:
> +		val = wdd->ops->status ? wdd->ops->status(wdd) : 0;
> +		return put_user(val, p);
> +	case WDIOC_GETBOOTSTATUS:
> +		return put_user(wdd->bootstatus, p);
> +	default:
> +		return -ENOTTY;
> +	}
> +	return -ENOTTY;

I think one of the -ENOTTY could be removed?

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

[PATCH 03/11] watchdog: WatchDog Timer Driver Core - Add WDIOC_KEEPALIVE ioctl

[Wim Van Sebroeck]

This part add's the WDIOC_KEEPALIVE ioctl functionality to the
WatchDog Timer Driver Core framework. Please note that the
WDIOF_KEEPALIVEPING bit has to be set in the watchdog_info
options field.

Signed-off-by: Alan Cox <alan@lxorguk.ukuu.org.uk>
Signed-off-by: Wim Van Sebroeck <wim@iguana.be>
---
 Documentation/watchdog/watchdog-kernel-api.txt |    3 +++
 drivers/watchdog/watchdog_dev.c                |    5 +++++
 2 files changed, 8 insertions(+), 0 deletions(-)

diff --git a/Documentation/watchdog/watchdog-kernel-api.txt b/Documentation/watchdog/watchdog-kernel-api.txt
index 544eb19..8ca0571 100644
--- a/Documentation/watchdog/watchdog-kernel-api.txt
+++ b/Documentation/watchdog/watchdog-kernel-api.txt
@@ -99,6 +99,9 @@ they are supported. These optional routines/operations are:
   the watchdog timer driver core does: to send a keepalive ping to the watchdog
   timer hardware it will either use the ping operation (when available) or the
   start operation (when the ping operation is not available).
+  (Note: the WDIOC_KEEPALIVE ioctl call will only be active when the
+  WDIOF_KEEPALIVEPING bit has been set in the option field on the watchdog's
+  info structure).
 * status: this routine checks the status of the watchdog timer device. The
   status of the device is reported with watchdog WDIOF_* status flags/bits.
 
diff --git a/drivers/watchdog/watchdog_dev.c b/drivers/watchdog/watchdog_dev.c
index bebea38..cc361e1 100644
--- a/drivers/watchdog/watchdog_dev.c
+++ b/drivers/watchdog/watchdog_dev.c
@@ -131,6 +131,11 @@ static long watchdog_ioctl(struct file *file, unsigned int cmd,
 		return put_user(val, p);
 	case WDIOC_GETBOOTSTATUS:
 		return put_user(wdd->bootstatus, p);
+	case WDIOC_KEEPALIVE:
+		if (!(wdd->info->options & WDIOF_KEEPALIVEPING))
+			return -EOPNOTSUPP;
+		watchdog_ping(wdd);
+		return 0;
 	default:
 		return -ENOTTY;
 	}
-- 
1.7.6

[PATCH 04/11] watchdog: WatchDog Timer Driver Core - Add WDIOC_SETOPTIONS ioctl

[Wim Van Sebroeck]

This part add's the WDIOC_SETOPTIONS ioctl functionality
to the WatchDog Timer Driver Core framework.

Signed-off-by: Alan Cox <alan@lxorguk.ukuu.org.uk>
Signed-off-by: Wim Van Sebroeck <wim@iguana.be>
---
 Documentation/watchdog/watchdog-kernel-api.txt |    9 ++-
 drivers/watchdog/watchdog_dev.c                |   79 +++++++++++++++++++++--
 include/linux/watchdog.h                       |    1 +
 3 files changed, 80 insertions(+), 9 deletions(-)

diff --git a/Documentation/watchdog/watchdog-kernel-api.txt b/Documentation/watchdog/watchdog-kernel-api.txt
index 8ca0571..6df1ade 100644
--- a/Documentation/watchdog/watchdog-kernel-api.txt
+++ b/Documentation/watchdog/watchdog-kernel-api.txt
@@ -54,8 +54,9 @@ It contains following fields:
   WDIOF_* status bits).
 * priv: a pointer to the private data of a watchdog device
 * status: this field contains a number of status bits that give extra
-  information about the status of the device (Like: is the device opened via
-  the /dev/watchdog interface or not, ...)
+  information about the status of the device (Like: is the watchdog timer
+  running/active, is the device opened via the /dev/watchdog interface or not,
+  ...)
 
 The list of watchdog operations is defined as:
 
@@ -107,6 +108,10 @@ they are supported. These optional routines/operations are:
 
 The status bits should (preferably) be set with the set_bit and clear_bit alike
 bit-operations. The status bits that are defined are:
+* WDOG_ACTIVE: this status bit indicates whether or not a watchdog timer device
+  is active or not. When the watchdog is active after booting, then you should
+  set this status bit (Note: when you register the watchdog timer device with
+  this bit set, then opening /dev/watchdog will skip the start operation)
 * WDOG_DEV_OPEN: this status bit shows whether or not the watchdog device
   was opened via /dev/watchdog.
   (This bit should only be used by the WatchDog Timer Driver Core).
diff --git a/drivers/watchdog/watchdog_dev.c b/drivers/watchdog/watchdog_dev.c
index cc361e1..530003b 100644
--- a/drivers/watchdog/watchdog_dev.c
+++ b/drivers/watchdog/watchdog_dev.c
@@ -65,14 +65,64 @@ static struct watchdog_device *wdd;
  *	If the watchdog has no own ping operation then it needs to be
  *	restarted via the start operation. This wrapper function does
  *	exactly that.
+ *	We only ping when the watchdog device is running.
  */
 
 static int watchdog_ping(struct watchdog_device *wddev)
 {
-	if (wddev->ops->ping)
-		return wddev->ops->ping(wddev);  /* ping the watchdog */
-	else
-		return wddev->ops->start(wddev); /* restart the watchdog */
+	if (test_bit(WDOG_ACTIVE, &wdd->status)) {
+		if (wddev->ops->ping)
+			return wddev->ops->ping(wddev);  /* ping the watchdog */
+		else
+			return wddev->ops->start(wddev); /* restart watchdog */
+	}
+	return 0;
+}
+
+/*
+ *	watchdog_start: wrapper to start the watchdog.
+ *	@wddev: the watchdog device to start
+ *
+ *	Start the watchdog if it is not active and mark it active.
+ *	This function returns zero on success or a negative errno code for
+ *	failure.
+ */
+
+static int watchdog_start(struct watchdog_device *wddev)
+{
+	int err;
+
+	if (!test_bit(WDOG_ACTIVE, &wdd->status)) {
+		err = wddev->ops->start(wddev);
+		if (err < 0)
+			return err;
+
+		set_bit(WDOG_ACTIVE, &wdd->status);
+	}
+	return 0;
+}
+
+/*
+ *	watchdog_stop: wrapper to stop the watchdog.
+ *	@wddev: the watchdog device to stop
+ *
+ *	Stop the watchdog if it is still active and unmark it active.
+ *	This function returns zero on success or a negative errno code for
+ *	failure.
+ */
+
+static int watchdog_stop(struct watchdog_device *wddev)
+{
+	int err;
+
+	if (test_bit(WDOG_ACTIVE, &wdd->status)) {
+		err = wddev->ops->stop(wddev);
+		if (err < 0)
+			return err;
+
+		clear_bit(WDOG_ACTIVE, &wdd->status);
+	}
+	return 0;
 }
 
 /*
@@ -121,6 +171,7 @@ static long watchdog_ioctl(struct file *file, unsigned int cmd,
 	void __user *argp = (void __user *)arg;
 	int __user *p = argp;
 	unsigned int val;
+	int err;
 
 	switch (cmd) {
 	case WDIOC_GETSUPPORT:
@@ -131,6 +182,20 @@ static long watchdog_ioctl(struct file *file, unsigned int cmd,
 		return put_user(val, p);
 	case WDIOC_GETBOOTSTATUS:
 		return put_user(wdd->bootstatus, p);
+	case WDIOC_SETOPTIONS:
+		if (get_user(val, p))
+			return -EFAULT;
+		if (val & WDIOS_DISABLECARD) {
+			err = watchdog_stop(wdd);
+			if (err < 0)
+				return err;
+		}
+		if (val & WDIOS_ENABLECARD) {
+			err = watchdog_start(wdd);
+			if (err < 0)
+				return err;
+		}
+		return 0;
 	case WDIOC_KEEPALIVE:
 		if (!(wdd->info->options & WDIOF_KEEPALIVEPING))
 			return -EOPNOTSUPP;
@@ -168,7 +233,7 @@ static int watchdog_open(struct inode *inode, struct file *file)
 		goto out;
 
 	/* start the watchdog */
-	err = wdd->ops->start(wdd);
+	err = watchdog_start(wdd);
 	if (err < 0)
 		goto out_mod;
 
@@ -195,8 +260,8 @@ static int watchdog_release(struct inode *inode, struct file *file)
 	int err;
 
 	/* stop the watchdog */
-	err = wdd->ops->stop(wdd);
-	if (err != 0) {
+	err = watchdog_stop(wdd);
+	if (err < 0) {
 		pr_crit("%s: watchdog did not stop!\n", wdd->info->identity);
 		watchdog_ping(wdd);
 	}
diff --git a/include/linux/watchdog.h b/include/linux/watchdog.h
index a2b639b..853387f 100644
--- a/include/linux/watchdog.h
+++ b/include/linux/watchdog.h
@@ -81,6 +81,7 @@ struct watchdog_device {
 	void *priv;
 	unsigned long status;
 /* Bit numbers for status flags */
+#define WDOG_ACTIVE		0	/* Is the watchdog running/active */
 #define WDOG_DEV_OPEN		1	/* Opened via /dev/watchdog ? */
 };
 
-- 
1.7.6

Re: [PATCH 04/11] watchdog: WatchDog Timer Driver Core - Add WDIOC_SETOPTIONS ioctl

[Wolfram Sang]

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


> @@ -121,6 +171,7 @@ static long watchdog_ioctl(struct file *file, unsigned int cmd,
>  	void __user *argp = (void __user *)arg;
>  	int __user *p = argp;
>  	unsigned int val;
> +	int err;
>  
>  	switch (cmd) {
>  	case WDIOC_GETSUPPORT:
> @@ -131,6 +182,20 @@ static long watchdog_ioctl(struct file *file, unsigned int cmd,
>  		return put_user(val, p);
>  	case WDIOC_GETBOOTSTATUS:
>  		return put_user(wdd->bootstatus, p);
> +	case WDIOC_SETOPTIONS:
> +		if (get_user(val, p))
> +			return -EFAULT;
> +		if (val & WDIOS_DISABLECARD) {
> +			err = watchdog_stop(wdd);
> +			if (err < 0)
> +				return err;
> +		}
> +		if (val & WDIOS_ENABLECARD) {
> +			err = watchdog_start(wdd);
> +			if (err < 0)
> +				return err;
> +		}
> +		return 0;
>  	case WDIOC_KEEPALIVE:
>  		if (!(wdd->info->options & WDIOF_KEEPALIVEPING))
>  			return -EOPNOTSUPP;
> @@ -168,7 +233,7 @@ static int watchdog_open(struct inode *inode, struct file *file)
>  		goto out;
>  
>  	/* start the watchdog */
> -	err = wdd->ops->start(wdd);
> +	err = watchdog_start(wdd);

I think we still might need open- and close-callbacks here. Some drivers turn
on/off clocks on these operations and we can't simply move them to the
start/stop callbacks, because the clock might be turned off via DISABLECARD
which calls stop. And without the clock, a simple register access via
set_timeout might lock up the machine.

>  	if (err < 0)
>  		goto out_mod;
>  
> @@ -195,8 +260,8 @@ static int watchdog_release(struct inode *inode, struct file *file)
>  	int err;
>  
>  	/* stop the watchdog */
> -	err = wdd->ops->stop(wdd);
> -	if (err != 0) {
> +	err = watchdog_stop(wdd);
> +	if (err < 0) {
>  		pr_crit("%s: watchdog did not stop!\n", wdd->info->identity);
>  		watchdog_ping(wdd);
>  	}
> diff --git a/include/linux/watchdog.h b/include/linux/watchdog.h
> index a2b639b..853387f 100644
> --- a/include/linux/watchdog.h
> +++ b/include/linux/watchdog.h
> @@ -81,6 +81,7 @@ struct watchdog_device {
>  	void *priv;
>  	unsigned long status;
>  /* Bit numbers for status flags */
> +#define WDOG_ACTIVE		0	/* Is the watchdog running/active */
>  #define WDOG_DEV_OPEN		1	/* Opened via /dev/watchdog ? */
>  };
>  
> -- 
> 1.7.6
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH 04/11] watchdog: WatchDog Timer Driver Core - Add WDIOC_SETOPTIONS ioctl

[Wim Van Sebroeck]

Hi Wolfram,

> > @@ -168,7 +233,7 @@ static int watchdog_open(struct inode *inode, struct file *file)
> >  		goto out;
> >  
> >  	/* start the watchdog */
> > -	err = wdd->ops->start(wdd);
> > +	err = watchdog_start(wdd);
> 
> I think we still might need open- and close-callbacks here. Some drivers turn
> on/off clocks on these operations and we can't simply move them to the
> start/stop callbacks, because the clock might be turned off via DISABLECARD
> which calls stop. And without the clock, a simple register access via
> set_timeout might lock up the machine.

I consider that an extension on the basic framework.
After these patches are accepted we can work on this.

Kind regards,
Wim.

[PATCH 05/11] watchdog: WatchDog Timer Driver Core - Add WDIOC_SETTIMEOUT and WDIOC_GETTIMEOUT ioctl

[Wim Van Sebroeck]

This part add's the WDIOC_SETTIMEOUT and WDIOC_GETTIMEOUT ioctl
functionality to the WatchDog Timer Driver Core framework.

Signed-off-by: Alan Cox <alan@lxorguk.ukuu.org.uk>
Signed-off-by: Wim Van Sebroeck <wim@iguana.be>
---
 Documentation/watchdog/watchdog-kernel-api.txt |   10 ++++++++++
 drivers/watchdog/watchdog_dev.c                |   20 ++++++++++++++++++++
 include/linux/watchdog.h                       |    2 ++
 3 files changed, 32 insertions(+), 0 deletions(-)

diff --git a/Documentation/watchdog/watchdog-kernel-api.txt b/Documentation/watchdog/watchdog-kernel-api.txt
index 6df1ade..3085c5c 100644
--- a/Documentation/watchdog/watchdog-kernel-api.txt
+++ b/Documentation/watchdog/watchdog-kernel-api.txt
@@ -42,6 +42,7 @@ struct watchdog_device {
 	const struct watchdog_info *info;
 	const struct watchdog_ops *ops;
 	unsigned int bootstatus;
+	unsigned int timeout;
 	void *priv;
 	unsigned long status;
 };
@@ -50,6 +51,7 @@ It contains following fields:
 * info: a pointer to a watchdog_info structure. This structure gives some
   additional information about the watchdog timer itself. (Like it's unique name)
 * ops: a pointer to the list of watchdog operations that the watchdog supports.
+* timeout: the watchdog timer's timeout value (in seconds).
 * bootstatus: status of the device after booting (reported with watchdog
   WDIOF_* status bits).
 * priv: a pointer to the private data of a watchdog device
@@ -68,6 +70,7 @@ struct watchdog_ops {
 	/* optional operations */
 	int (*ping)(struct watchdog_device *);
 	unsigned int (*status)(struct watchdog_device *);
+	int (*set_timeout)(struct watchdog_device *, unsigned int);
 };
 
 It is important that you first define the module owner of the watchdog timer
@@ -105,6 +108,13 @@ they are supported. These optional routines/operations are:
   info structure).
 * status: this routine checks the status of the watchdog timer device. The
   status of the device is reported with watchdog WDIOF_* status flags/bits.
+* set_timeout: this routine checks and changes the timeout of the watchdog
+  timer device. It returns 0 on success, -EINVAL for "parameter out of range"
+  and -EIO for "could not write value to the watchdog". On success the timeout
+  value of the watchdog_device will be changed to the value that was just used
+  to re-program the watchdog timer device.
+  (Note: the WDIOF_SETTIMEOUT needs to be set in the options field of the
+  watchdog's info structure).
 
 The status bits should (preferably) be set with the set_bit and clear_bit alike
 bit-operations. The status bits that are defined are:
diff --git a/drivers/watchdog/watchdog_dev.c b/drivers/watchdog/watchdog_dev.c
index 530003b..0d4b16d 100644
--- a/drivers/watchdog/watchdog_dev.c
+++ b/drivers/watchdog/watchdog_dev.c
@@ -201,6 +201,26 @@ static long watchdog_ioctl(struct file *file, unsigned int cmd,
 			return -EOPNOTSUPP;
 		watchdog_ping(wdd);
 		return 0;
+	case WDIOC_SETTIMEOUT:
+		if ((wdd->ops->set_timeout == NULL) ||
+		    !(wdd->info->options & WDIOF_SETTIMEOUT))
+			return -EOPNOTSUPP;
+		if (get_user(val, p))
+			return -EFAULT;
+		err = wdd->ops->set_timeout(wdd, val);
+		if (err < 0)
+			return err;
+		wdd->timeout = val;
+		/* If the watchdog is active then we sent a keepalive ping
+		 * to make sure that the watchdog keep's running (and if
+		 * possible that it takes the new timeout) */
+		watchdog_ping(wdd);
+		/* Fall */
+	case WDIOC_GETTIMEOUT:
+		/* timeout == 0 means that we don't know the timeout */
+		if (wdd->timeout)
+			return put_user(wdd->timeout, p);
+		return -EOPNOTSUPP;
 	default:
 		return -ENOTTY;
 	}
diff --git a/include/linux/watchdog.h b/include/linux/watchdog.h
index 853387f..7e37fdf 100644
--- a/include/linux/watchdog.h
+++ b/include/linux/watchdog.h
@@ -71,6 +71,7 @@ struct watchdog_ops {
 	/* optional operations */
 	int (*ping)(struct watchdog_device *);
 	unsigned int (*status)(struct watchdog_device *);
+	int (*set_timeout)(struct watchdog_device *, unsigned int);
 };
 
 /* The structure that defines a watchdog device */
@@ -78,6 +79,7 @@ struct watchdog_device {
 	const struct watchdog_info *info;
 	const struct watchdog_ops *ops;
 	unsigned int bootstatus;
+	unsigned int timeout;
 	void *priv;
 	unsigned long status;
 /* Bit numbers for status flags */
-- 
1.7.6

Re: [PATCH 05/11] watchdog: WatchDog Timer Driver Core - Add WDIOC_SETTIMEOUT and WDIOC_GETTIMEOUT ioctl

[Wolfram Sang]

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

> diff --git a/drivers/watchdog/watchdog_dev.c b/drivers/watchdog/watchdog_dev.c
> index 530003b..0d4b16d 100644
> --- a/drivers/watchdog/watchdog_dev.c
> +++ b/drivers/watchdog/watchdog_dev.c
> @@ -201,6 +201,26 @@ static long watchdog_ioctl(struct file *file, unsigned int cmd,
>  			return -EOPNOTSUPP;
>  		watchdog_ping(wdd);
>  		return 0;
> +	case WDIOC_SETTIMEOUT:
> +		if ((wdd->ops->set_timeout == NULL) ||
> +		    !(wdd->info->options & WDIOF_SETTIMEOUT))
> +			return -EOPNOTSUPP;
> +		if (get_user(val, p))
> +			return -EFAULT;
> +		err = wdd->ops->set_timeout(wdd, val);
> +		if (err < 0)
> +			return err;
> +		wdd->timeout = val;
> +		/* If the watchdog is active then we sent a keepalive ping

s/sent/send/

> +		 * to make sure that the watchdog keep's running (and if
> +		 * possible that it takes the new timeout) */
> +		watchdog_ping(wdd);
> +		/* Fall */
> +	case WDIOC_GETTIMEOUT:
> +		/* timeout == 0 means that we don't know the timeout */
> +		if (wdd->timeout)
> +			return put_user(wdd->timeout, p);
> +		return -EOPNOTSUPP;

I'd suggest to swap the logic here (branch taken on error): 
 
        if (wdd->timeout == 0) 
                return -EOPNOTSUPP; 
        return put_user(); 

>  	default:
>  		return -ENOTTY;
>  	}

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

[PATCH 06/11] watchdog: WatchDog Timer Driver Core - Add Magic Close feature

[Wim Van Sebroeck]

Add support for the Magic Close feature to the
WatchDog Timer Driver Core framework.

Signed-off-by: Alan Cox <alan@lxorguk.ukuu.org.uk>
Signed-off-by: Wim Van Sebroeck <wim@iguana.be>
---
 Documentation/watchdog/watchdog-kernel-api.txt |    7 ++++++
 drivers/watchdog/watchdog_dev.c                |   28 ++++++++++++++++++++---
 include/linux/watchdog.h                       |    1 +
 3 files changed, 32 insertions(+), 4 deletions(-)

diff --git a/Documentation/watchdog/watchdog-kernel-api.txt b/Documentation/watchdog/watchdog-kernel-api.txt
index 3085c5c..8e5867c 100644
--- a/Documentation/watchdog/watchdog-kernel-api.txt
+++ b/Documentation/watchdog/watchdog-kernel-api.txt
@@ -125,3 +125,10 @@ bit-operations. The status bits that are defined are:
 * WDOG_DEV_OPEN: this status bit shows whether or not the watchdog device
   was opened via /dev/watchdog.
   (This bit should only be used by the WatchDog Timer Driver Core).
+* WDOG_ALLOW_RELEASE: this bit stores whether or not the magic close character
+  has been sent (so that we can support the magic close feature).
+  (This bit should only be used by the WatchDog Timer Driver Core).
+
+Note: The WatchDog Timer Driver Core supports the magic close feature. To use
+the magic close feature you must set the WDIOF_MAGICCLOSE bit in the options
+field of the watchdog's info structure.
diff --git a/drivers/watchdog/watchdog_dev.c b/drivers/watchdog/watchdog_dev.c
index 0d4b16d..c9ab89e 100644
--- a/drivers/watchdog/watchdog_dev.c
+++ b/drivers/watchdog/watchdog_dev.c
@@ -133,6 +133,8 @@ static int watchdog_stop(struct watchdog_device *wddev)
  *	@ppos: pointer to the file offset
  *
  *	A write to a watchdog device is defined as a keepalive ping.
+ *	Writing the magic 'V' sequence allows the next close to turn
+ *	off the watchdog.
  */
 
 static ssize_t watchdog_write(struct file *file, const char __user *data,
@@ -144,9 +146,18 @@ static ssize_t watchdog_write(struct file *file, const char __user *data,
 	if (len == 0)
 		return 0;
 
+	/*
+	 * Note: just in case someone wrote the magic character
+	 * five months ago...
+	 */
+	clear_bit(WDOG_ALLOW_RELEASE, &wdd->status);
+
+	/* scan to see whether or not we got the magic character */
 	for (i = 0; i != len; i++) {
 		if (get_user(c, data + i))
 			return -EFAULT;
+		if (c == 'V')
+			set_bit(WDOG_ALLOW_RELEASE, &wdd->status);
 	}
 
 	/* someone wrote to us, so we send the watchdog a keepalive ping */
@@ -272,15 +283,24 @@ out:
  *      @inode: inode of device
  *      @file: file handle to device
  *
- *	This is the code for when /dev/watchdog gets closed.
+ *	This is the code for when /dev/watchdog gets closed. We will only
+ *	stop the watchdog when we have received the magic char, else the
+ *	watchdog will keep running.
  */
 
 static int watchdog_release(struct inode *inode, struct file *file)
 {
-	int err;
+	int err = -EBUSY;
+
+	/*
+	 * We only stop the watchdog if we received the magic character
+	 * or if WDIOF_MAGICCLOSE is not set
+	 */
+	if (test_and_clear_bit(WDOG_ALLOW_RELEASE, &wdd->status) ||
+	    !(wdd->info->options & WDIOF_MAGICCLOSE))
+		err = watchdog_stop(wdd);
 
-	/* stop the watchdog */
-	err = watchdog_stop(wdd);
+	/* If the watchdog was not stopped, sent a keepalive ping */
 	if (err < 0) {
 		pr_crit("%s: watchdog did not stop!\n", wdd->info->identity);
 		watchdog_ping(wdd);
diff --git a/include/linux/watchdog.h b/include/linux/watchdog.h
index 7e37fdf..8406ed5 100644
--- a/include/linux/watchdog.h
+++ b/include/linux/watchdog.h
@@ -85,6 +85,7 @@ struct watchdog_device {
 /* Bit numbers for status flags */
 #define WDOG_ACTIVE		0	/* Is the watchdog running/active */
 #define WDOG_DEV_OPEN		1	/* Opened via /dev/watchdog ? */
+#define WDOG_ALLOW_RELEASE	2	/* Did we receive the magic char ? */
 };
 
 /* drivers/watchdog/core/watchdog_core.c */
-- 
1.7.6

Re: [PATCH 06/11] watchdog: WatchDog Timer Driver Core - Add Magic Close feature

[Wolfram Sang]

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

>  static int watchdog_release(struct inode *inode, struct file *file)
>  {
> -	int err;
> +	int err = -EBUSY;
> +
> +	/*
> +	 * We only stop the watchdog if we received the magic character
> +	 * or if WDIOF_MAGICCLOSE is not set
> +	 */
> +	if (test_and_clear_bit(WDOG_ALLOW_RELEASE, &wdd->status) ||
> +	    !(wdd->info->options & WDIOF_MAGICCLOSE))
> +		err = watchdog_stop(wdd);
>  
> -	/* stop the watchdog */
> -	err = watchdog_stop(wdd);
> +	/* If the watchdog was not stopped, sent a keepalive ping */

s/sent/send/

>  	if (err < 0) {
>  		pr_crit("%s: watchdog did not stop!\n", wdd->info->identity);
>  		watchdog_ping(wdd);

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

[PATCH 07/11] watchdog: WatchDog Timer Driver Core - Add nowayout feature

[Wim Van Sebroeck]

Add support for the nowayout feature to the
WatchDog Timer Driver Core framework.
This feature prevents the watchdog timer from being
stopped.

Signed-off-by: Alan Cox <alan@lxorguk.ukuu.org.uk>
Signed-off-by: Wim Van Sebroeck <wim@iguana.be>
---
 Documentation/watchdog/watchdog-kernel-api.txt |   13 ++++++++-----
 drivers/watchdog/watchdog_dev.c                |   18 +++++++++++++-----
 include/linux/watchdog.h                       |    1 +
 3 files changed, 22 insertions(+), 10 deletions(-)

diff --git a/Documentation/watchdog/watchdog-kernel-api.txt b/Documentation/watchdog/watchdog-kernel-api.txt
index 8e5867c..fb53001 100644
--- a/Documentation/watchdog/watchdog-kernel-api.txt
+++ b/Documentation/watchdog/watchdog-kernel-api.txt
@@ -57,8 +57,8 @@ It contains following fields:
 * priv: a pointer to the private data of a watchdog device
 * status: this field contains a number of status bits that give extra
   information about the status of the device (Like: is the watchdog timer
-  running/active, is the device opened via the /dev/watchdog interface or not,
-  ...)
+  running/active, is the nowayout bit set, is the device opened via
+  the /dev/watchdog interface or not, ...)
 
 The list of watchdog operations is defined as:
 
@@ -128,7 +128,10 @@ bit-operations. The status bits that are defined are:
 * WDOG_ALLOW_RELEASE: this bit stores whether or not the magic close character
   has been sent (so that we can support the magic close feature).
   (This bit should only be used by the WatchDog Timer Driver Core).
+* WDOG_NO_WAY_OUT: this bit stores the nowayout setting for the watchdog.
+  If this bit is set then the watchdog timer will not be able to stop.
 
-Note: The WatchDog Timer Driver Core supports the magic close feature. To use
-the magic close feature you must set the WDIOF_MAGICCLOSE bit in the options
-field of the watchdog's info structure.
+Note: The WatchDog Timer Driver Core supports the magic close feature and
+the nowayout feature. To use the magic close feature you must set the
+WDIOF_MAGICCLOSE bit in the options field of the watchdog's info structure.
+The nowayout feature will overrule the magic close feature.
diff --git a/drivers/watchdog/watchdog_dev.c b/drivers/watchdog/watchdog_dev.c
index c9ab89e..ca60bea 100644
--- a/drivers/watchdog/watchdog_dev.c
+++ b/drivers/watchdog/watchdog_dev.c
@@ -109,11 +109,18 @@ static int watchdog_start(struct watchdog_device *wddev)
  *	Stop the watchdog if it is still active and unmark it active.
  *	This function returns zero on success or a negative errno code for
  *	failure.
+ *	If the 'nowayout' feature was set, the watchdog cannot be stopped.
  */
 
 static int watchdog_stop(struct watchdog_device *wddev)
 {
-	int err;
+	int err = -EBUSY;
+
+	if (test_bit(WDOG_NO_WAY_OUT, &wdd->status)) {
+		pr_info("%s: nowayout prevents watchdog to be stopped!\n",
+							wdd->info->identity);
+		return err;
+	}
 
 	if (test_bit(WDOG_ACTIVE, &wdd->status)) {
 		err = wddev->ops->stop(wddev);
@@ -134,7 +141,7 @@ static int watchdog_stop(struct watchdog_device *wddev)
  *
  *	A write to a watchdog device is defined as a keepalive ping.
  *	Writing the magic 'V' sequence allows the next close to turn
- *	off the watchdog.
+ *	off the watchdog (if 'nowayout' is not set).
  */
 
 static ssize_t watchdog_write(struct file *file, const char __user *data,
@@ -284,8 +291,8 @@ out:
  *      @file: file handle to device
  *
  *	This is the code for when /dev/watchdog gets closed. We will only
- *	stop the watchdog when we have received the magic char, else the
- *	watchdog will keep running.
+ *	stop the watchdog when we have received the magic char (and nowayout
+ *	was not set), else the watchdog will keep running.
  */
 
 static int watchdog_release(struct inode *inode, struct file *file)
@@ -294,7 +301,8 @@ static int watchdog_release(struct inode *inode, struct file *file)
 
 	/*
 	 * We only stop the watchdog if we received the magic character
-	 * or if WDIOF_MAGICCLOSE is not set
+	 * or if WDIOF_MAGICCLOSE is not set. If nowayout was set then
+	 * watchdog_stop will fail.
 	 */
 	if (test_and_clear_bit(WDOG_ALLOW_RELEASE, &wdd->status) ||
 	    !(wdd->info->options & WDIOF_MAGICCLOSE))
diff --git a/include/linux/watchdog.h b/include/linux/watchdog.h
index 8406ed5..aa54fb1 100644
--- a/include/linux/watchdog.h
+++ b/include/linux/watchdog.h
@@ -86,6 +86,7 @@ struct watchdog_device {
 #define WDOG_ACTIVE		0	/* Is the watchdog running/active */
 #define WDOG_DEV_OPEN		1	/* Opened via /dev/watchdog ? */
 #define WDOG_ALLOW_RELEASE	2	/* Did we receive the magic char ? */
+#define WDOG_NO_WAY_OUT		3	/* Is 'nowayout' feature set ? */
 };
 
 /* drivers/watchdog/core/watchdog_core.c */
-- 
1.7.6

[PATCH 09/11] watchdog: WatchDog Timer Driver Core - Add ioctl call

[Wim Van Sebroeck]

Add support for extra ioctl calls by adding a
ioctl watchdog operation. This operation will be
called before we do our own handling of ioctl
commands. This way we can override the internal
ioctl command handling and we can also add
extra ioctl commands. The ioctl watchdog operation
should return the appropriate error codes or
-ENOIOCTLCMD if the ioctl command should be handled
through the internal ioctl handling of the framework.

Signed-off-by: Alan Cox <alan@lxorguk.ukuu.org.uk>
Signed-off-by: Wim Van Sebroeck <wim@iguana.be>
---
 Documentation/watchdog/watchdog-kernel-api.txt |    5 +++++
 drivers/watchdog/watchdog_dev.c                |    6 ++++++
 include/linux/watchdog.h                       |    1 +
 3 files changed, 12 insertions(+), 0 deletions(-)

diff --git a/Documentation/watchdog/watchdog-kernel-api.txt b/Documentation/watchdog/watchdog-kernel-api.txt
index 79f6edf..377be98 100644
--- a/Documentation/watchdog/watchdog-kernel-api.txt
+++ b/Documentation/watchdog/watchdog-kernel-api.txt
@@ -75,6 +75,7 @@ struct watchdog_ops {
 	int (*ping)(struct watchdog_device *);
 	unsigned int (*status)(struct watchdog_device *);
 	int (*set_timeout)(struct watchdog_device *, unsigned int);
+	long (*ioctl)(struct watchdog_device *, unsigned int, unsigned long);
 };
 
 It is important that you first define the module owner of the watchdog timer
@@ -119,6 +120,10 @@ they are supported. These optional routines/operations are:
   to re-program the watchdog timer device.
   (Note: the WDIOF_SETTIMEOUT needs to be set in the options field of the
   watchdog's info structure).
+* ioctl: if this routine is present then it will be called first before we do
+  our own internal ioctl call handling. This routine should return -ENOIOCTLCMD
+  if a command is not supported. The parameters that are passed to the ioctl
+  call are: watchdog_device, cmd and arg.
 
 The status bits should (preferably) be set with the set_bit and clear_bit alike
 bit-operations. The status bits that are defined are:
diff --git a/drivers/watchdog/watchdog_dev.c b/drivers/watchdog/watchdog_dev.c
index c66bb6d..83c08a8 100644
--- a/drivers/watchdog/watchdog_dev.c
+++ b/drivers/watchdog/watchdog_dev.c
@@ -191,6 +191,12 @@ static long watchdog_ioctl(struct file *file, unsigned int cmd,
 	unsigned int val;
 	int err;
 
+	if (wdd->ops->ioctl) {
+		err = wdd->ops->ioctl(wdd, cmd, arg);
+		if (err != -ENOIOCTLCMD)
+			return err;
+	}
+
 	switch (cmd) {
 	case WDIOC_GETSUPPORT:
 		return copy_to_user(argp, wdd->info,
diff --git a/include/linux/watchdog.h b/include/linux/watchdog.h
index e2c7337..fa4a46b 100644
--- a/include/linux/watchdog.h
+++ b/include/linux/watchdog.h
@@ -72,6 +72,7 @@ struct watchdog_ops {
 	int (*ping)(struct watchdog_device *);
 	unsigned int (*status)(struct watchdog_device *);
 	int (*set_timeout)(struct watchdog_device *, unsigned int);
+	long (*ioctl)(struct watchdog_device *, unsigned int, unsigned long);
 };
 
 /* The structure that defines a watchdog device */
-- 
1.7.6

[PATCH 08/11] watchdog: WatchDog Timer Driver Core - Add parent device

[Wim Van Sebroeck]

Add support for a parent device so that it can
be set as the parent of the misc_device.

Signed-off-by: Alan Cox <alan@lxorguk.ukuu.org.uk>
Signed-off-by: Wim Van Sebroeck <wim@iguana.be>
---
 Documentation/watchdog/watchdog-kernel-api.txt |    4 ++++
 drivers/watchdog/watchdog_dev.c                |    1 +
 include/linux/watchdog.h                       |    1 +
 3 files changed, 6 insertions(+), 0 deletions(-)

diff --git a/Documentation/watchdog/watchdog-kernel-api.txt b/Documentation/watchdog/watchdog-kernel-api.txt
index fb53001..79f6edf 100644
--- a/Documentation/watchdog/watchdog-kernel-api.txt
+++ b/Documentation/watchdog/watchdog-kernel-api.txt
@@ -41,6 +41,7 @@ The watchdog device structure looks like this:
 struct watchdog_device {
 	const struct watchdog_info *info;
 	const struct watchdog_ops *ops;
+	struct device *parent;
 	unsigned int bootstatus;
 	unsigned int timeout;
 	void *priv;
@@ -51,6 +52,9 @@ It contains following fields:
 * info: a pointer to a watchdog_info structure. This structure gives some
   additional information about the watchdog timer itself. (Like it's unique name)
 * ops: a pointer to the list of watchdog operations that the watchdog supports.
+* parent: a pointer to the parent device of the watchdog. This will be set
+  as the parent of the misc device to improve dependencies in driver model
+  (e.g. sysfs).
 * timeout: the watchdog timer's timeout value (in seconds).
 * bootstatus: status of the device after booting (reported with watchdog
   WDIOF_* status bits).
diff --git a/drivers/watchdog/watchdog_dev.c b/drivers/watchdog/watchdog_dev.c
index ca60bea..c66bb6d 100644
--- a/drivers/watchdog/watchdog_dev.c
+++ b/drivers/watchdog/watchdog_dev.c
@@ -367,6 +367,7 @@ int watchdog_dev_register(struct watchdog_device *watchdog)
 	wdd = watchdog;
 
 	/* Register the miscdevice */
+	watchdog_miscdev.parent = watchdog->parent;
 	err = misc_register(&watchdog_miscdev);
 	if (err != 0) {
 		pr_err("%s: cannot register miscdev on minor=%d (err=%d).\n",
diff --git a/include/linux/watchdog.h b/include/linux/watchdog.h
index aa54fb1..e2c7337 100644
--- a/include/linux/watchdog.h
+++ b/include/linux/watchdog.h
@@ -78,6 +78,7 @@ struct watchdog_ops {
 struct watchdog_device {
 	const struct watchdog_info *info;
 	const struct watchdog_ops *ops;
+	struct device *parent;
 	unsigned int bootstatus;
 	unsigned int timeout;
 	void *priv;
-- 
1.7.6

[PATCH 10/11] watchdog: WatchDog Timer Driver Core - Add minimum and max timeout

[Wim Van Sebroeck]

Add min_timeout (minimum timeout) and max_timeout
values so that the framework can check if the new
timeout value is between the minimum and maximum
timeout values. If both values are 0, then the
framework will leave the check for the watchdog
device driver itself.

Signed-off-by: Alan Cox <alan@lxorguk.ukuu.org.uk>
Signed-off-by: Wim Van Sebroeck <wim@iguana.be>
---
 Documentation/watchdog/watchdog-kernel-api.txt |    4 ++++
 drivers/watchdog/watchdog_core.c               |   10 ++++++++++
 drivers/watchdog/watchdog_dev.c                |    3 +++
 include/linux/watchdog.h                       |    2 ++
 4 files changed, 19 insertions(+), 0 deletions(-)

diff --git a/Documentation/watchdog/watchdog-kernel-api.txt b/Documentation/watchdog/watchdog-kernel-api.txt
index 377be98..7c0ccb1 100644
--- a/Documentation/watchdog/watchdog-kernel-api.txt
+++ b/Documentation/watchdog/watchdog-kernel-api.txt
@@ -44,6 +44,8 @@ struct watchdog_device {
 	struct device *parent;
 	unsigned int bootstatus;
 	unsigned int timeout;
+	unsigned int min_timeout;
+	unsigned int max_timeout;
 	void *priv;
 	unsigned long status;
 };
@@ -56,6 +58,8 @@ It contains following fields:
   as the parent of the misc device to improve dependencies in driver model
   (e.g. sysfs).
 * timeout: the watchdog timer's timeout value (in seconds).
+* min_timeout: the watchdog timer's minimum timeout value (in seconds).
+* max_timeout: the watchdog timer's maximum timeout value (in seconds).
 * bootstatus: status of the device after booting (reported with watchdog
   WDIOF_* status bits).
 * priv: a pointer to the private data of a watchdog device
diff --git a/drivers/watchdog/watchdog_core.c b/drivers/watchdog/watchdog_core.c
index 79684ad..85fc787 100644
--- a/drivers/watchdog/watchdog_core.c
+++ b/drivers/watchdog/watchdog_core.c
@@ -63,6 +63,16 @@ int watchdog_register_device(struct watchdog_device *wdd)
 		return -EINVAL;
 
 	/*
+	 * Check that we have valid min and max timeout values, if
+	 * not reset them both to 0 (=not used or unknown)
+	 */
+	if (wdd->min_timeout > wdd->max_timeout) {
+		pr_info("Invalid min and max timeout values, resetting to 0!\n");
+		wdd->min_timeout = 0;
+		wdd->max_timeout = 0;
+	}
+
+	/*
 	 * Note: now that all watchdog_device data has been verified, we
 	 * will not check this anymore in other functions. If data gets
 	 * corrupted in a later stage then we expect a kernel panic!
diff --git a/drivers/watchdog/watchdog_dev.c b/drivers/watchdog/watchdog_dev.c
index 83c08a8..f6aa4b5 100644
--- a/drivers/watchdog/watchdog_dev.c
+++ b/drivers/watchdog/watchdog_dev.c
@@ -231,6 +231,9 @@ static long watchdog_ioctl(struct file *file, unsigned int cmd,
 			return -EOPNOTSUPP;
 		if (get_user(val, p))
 			return -EFAULT;
+		if ((wdd->max_timeout != 0) &&
+		    (val < wdd->min_timeout || val > wdd->max_timeout))
+				return -EINVAL;
 		err = wdd->ops->set_timeout(wdd, val);
 		if (err < 0)
 			return err;
diff --git a/include/linux/watchdog.h b/include/linux/watchdog.h
index fa4a46b..444b506 100644
--- a/include/linux/watchdog.h
+++ b/include/linux/watchdog.h
@@ -82,6 +82,8 @@ struct watchdog_device {
 	struct device *parent;
 	unsigned int bootstatus;
 	unsigned int timeout;
+	unsigned int min_timeout;
+	unsigned int max_timeout;
 	void *priv;
 	unsigned long status;
 /* Bit numbers for status flags */
-- 
1.7.6

[PATCH 11/11] watchdog: WatchDog Timer Driver Core - Remove llseek

[Wim Van Sebroeck]

No need to set no_llseek any more since it's the default now.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>
Signed-off-by: Wim Van Sebroeck <wim@iguana.be>
---
 drivers/watchdog/watchdog_dev.c |    1 -
 1 files changed, 0 insertions(+), 1 deletions(-)

diff --git a/drivers/watchdog/watchdog_dev.c b/drivers/watchdog/watchdog_dev.c
index f6aa4b5..30a1d36 100644
--- a/drivers/watchdog/watchdog_dev.c
+++ b/drivers/watchdog/watchdog_dev.c
@@ -338,7 +338,6 @@ static int watchdog_release(struct inode *inode, struct file *file)
 
 static const struct file_operations watchdog_fops = {
 	.owner		= THIS_MODULE,
-	.llseek		= no_llseek,
 	.write		= watchdog_write,
 	.unlocked_ioctl	= watchdog_ioctl,
 	.open		= watchdog_open,
-- 
1.7.6

Re: [PATCH 11/11] watchdog: WatchDog Timer Driver Core - Remove llseek

[Wolfram Sang]

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

On Mon, Jul 11, 2011 at 04:24:39PM +0200, Wim Van Sebroeck wrote:
> No need to set no_llseek any more since it's the default now.
> 
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> Signed-off-by: Wim Van Sebroeck <wim@iguana.be>

I'd think we can squash this into patch 1? Arnd?

> ---
>  drivers/watchdog/watchdog_dev.c |    1 -
>  1 files changed, 0 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/watchdog/watchdog_dev.c b/drivers/watchdog/watchdog_dev.c
> index f6aa4b5..30a1d36 100644
> --- a/drivers/watchdog/watchdog_dev.c
> +++ b/drivers/watchdog/watchdog_dev.c
> @@ -338,7 +338,6 @@ static int watchdog_release(struct inode *inode, struct file *file)
>  
>  static const struct file_operations watchdog_fops = {
>  	.owner		= THIS_MODULE,
> -	.llseek		= no_llseek,
>  	.write		= watchdog_write,
>  	.unlocked_ioctl	= watchdog_ioctl,
>  	.open		= watchdog_open,
> -- 
> 1.7.6
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH 11/11] watchdog: WatchDog Timer Driver Core - Remove llseek

[Arnd Bergmann]

On Monday 11 July 2011, Wolfram Sang wrote:
> On Mon, Jul 11, 2011 at 04:24:39PM +0200, Wim Van Sebroeck wrote:
> > No need to set no_llseek any more since it's the default now.
> > 
> > Signed-off-by: Arnd Bergmann <arnd@arndb.de>
> > Signed-off-by: Wim Van Sebroeck <wim@iguana.be>
> 
> I'd think we can squash this into patch 1? Arnd?

Yes, certainly if you like.

	Arnd

Re: [PATCH 01/11] watchdog: WatchDog Timer Driver Core - Add basic framework

[Wolfram Sang]

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


I put some "Overdocumented" in here. Maybe our mileage just varies :)

On Mon, Jul 11, 2011 at 04:19:48PM +0200, Wim Van Sebroeck wrote:
> The WatchDog Timer Driver Core is a framework
> that contains the common code for all watchdog-driver's.
> It also introduces a watchdog device structure and the
> operations that go with it.
> 
> This is the introduction of this framework. This part
> supports the minimal watchdog userspace API (or with
> other words: the functionality to use /dev/watchdog's
> open, release and write functionality as defined in
> the simplest watchdog API). Extra functionality will
> follow in the next set of patches.
> 
> Signed-off-by: Alan Cox <alan@lxorguk.ukuu.org.uk>
> Signed-off-by: Wim Van Sebroeck <wim@iguana.be>
> ---
>  Documentation/watchdog/watchdog-kernel-api.txt |  103 ++++++++++
>  drivers/watchdog/Kconfig                       |   11 +
>  drivers/watchdog/Makefile                      |    4 +
>  drivers/watchdog/watchdog_core.c               |  108 ++++++++++
>  drivers/watchdog/watchdog_dev.c                |  261 ++++++++++++++++++++++++
>  drivers/watchdog/watchdog_dev.h                |   33 +++
>  include/linux/watchdog.h                       |   27 +++
>  7 files changed, 547 insertions(+), 0 deletions(-)
>  create mode 100644 Documentation/watchdog/watchdog-kernel-api.txt
>  create mode 100644 drivers/watchdog/watchdog_core.c
>  create mode 100644 drivers/watchdog/watchdog_dev.c
>  create mode 100644 drivers/watchdog/watchdog_dev.h
> 
> diff --git a/Documentation/watchdog/watchdog-kernel-api.txt b/Documentation/watchdog/watchdog-kernel-api.txt
> new file mode 100644
> index 0000000..03c1066
> --- /dev/null
> +++ b/Documentation/watchdog/watchdog-kernel-api.txt
> @@ -0,0 +1,103 @@
> +The Linux WatchDog Timer Driver Core kernel API.
> +===============================================
> +Last reviewed: 09-Jul-2011
> +
> +Wim Van Sebroeck <wim@iguana.be>
> +
> +Introduction
> +------------
> +This document does not describe what a WatchDog Timer (WDT) Driver or Device is.
> +It also does not describe the API which can be used by user space to communicate
> +with a WatchDog Timer. If you want to know this then please read the following
> +file: Documentation/watchdog/watchdog-api.txt .
> +
> +So what does this document describe? It describes the API that can be used by
> +WatchDog Timer Drivers that want to use the WatchDog Timer Driver Core
> +Framework. This framework provides all interfacing towards user space so that
> +the same code does not have to be reproduced each time. This also means that
> +a watchdog timer driver then only needs to provide the different routines
> +(operations) that control the watchdog timer (WDT).
> +
> +The API
> +-------
> +Each watchdog timer driver that wants to use the WatchDog Timer Driver Core
> +must #include <linux/watchdog.h> (you would have to do this anyway when
> +writing a watchdog device driver). This include file contains following
> +register/unregister routines:
> +
> +extern int watchdog_register_device(struct watchdog_device *);
> +extern void watchdog_unregister_device(struct watchdog_device *);
> +
> +The watchdog_register_device routine registers a watchdog timer device.
> +The parameter of this routine is a pointer to a watchdog_device structure.
> +This routine returns zero on success and a negative errno code for failure.
> +
> +The watchdog_unregister_device routine deregisters a registered watchdog timer
> +device. The parameter of this routine is the pointer to the registered
> +watchdog_device structure.
> +
> +The watchdog device structure looks like this:
> +
> +struct watchdog_device {
> +	const struct watchdog_info *info;
> +	const struct watchdog_ops *ops;
> +	void *priv;
> +	unsigned long status;
> +};
> +
> +It contains following fields:
> +* info: a pointer to a watchdog_info structure. This structure gives some
> +  additional information about the watchdog timer itself. (Like it's unique name)
> +* ops: a pointer to the list of watchdog operations that the watchdog supports.
> +* priv: a pointer to the private data of a watchdog device
> +* status: this field contains a number of status bits that give extra
> +  information about the status of the device (Like: is the device opened via
> +  the /dev/watchdog interface or not, ...)
> +
> +The list of watchdog operations is defined as:
> +
> +struct watchdog_ops {
> +	struct module *owner;
> +	/* mandatory operations */
> +	int (*start)(struct watchdog_device *);
> +	int (*stop)(struct watchdog_device *);
> +	/* optional operations */
> +	int (*ping)(struct watchdog_device *);
> +};
> +
> +It is important that you first define the module owner of the watchdog timer
> +driver's operations. This module owner will be used to lock the module when
> +the watchdog is active. (This to avoid a system crash when you unload the
> +module and /dev/watchdog is still open).
> +Some operations are mandatory and some are optional. The mandatory operations
> +are:
> +* start: this is a pointer to the routine that starts the watchdog timer
> +  device.
> +  The routine needs a pointer to the watchdog timer device structure as a
> +  parameter. It returns zero on success or a negative errno code for failure.
> +* stop: with this routine the watchdog timer device is being stopped.
> +  The routine needs a pointer to the watchdog timer device structure as a
> +  parameter. It returns zero on success or a negative errno code for failure.
> +  Some watchdog timer hardware can only be started and not be stopped. The
> +  driver supporting this hardware needs to make sure that a start and stop
> +  routine is being provided. This can be done by using a timer in the driver
> +  that regularly sends a keepalive ping to the watchdog timer hardware.
> +
> +Not all watchdog timer hardware supports the same functionality. That's why
> +all other routines/operations are optional. They only need to be provided if
> +they are supported. These optional routines/operations are:
> +* ping: this is the routine that sends a keepalive ping to the watchdog timer
> +  hardware.
> +  The routine needs a pointer to the watchdog timer device structure as a
> +  parameter. It returns zero on success or a negative errno code for failure.
> +  Most hardware that does not support this as a separate function uses the
> +  start function to restart the watchdog timer hardware. And that's also what
> +  the watchdog timer driver core does: to send a keepalive ping to the watchdog
> +  timer hardware it will either use the ping operation (when available) or the
> +  start operation (when the ping operation is not available).
> +
> +The status bits should (preferably) be set with the set_bit and clear_bit alike
> +bit-operations. The status bits that are defined are:
> +* WDOG_DEV_OPEN: this status bit shows whether or not the watchdog device
> +  was opened via /dev/watchdog.
> +  (This bit should only be used by the WatchDog Timer Driver Core).
> diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
> index 372bc64..edb0f1b 100644
> --- a/drivers/watchdog/Kconfig
> +++ b/drivers/watchdog/Kconfig
> @@ -28,6 +28,17 @@ menuconfig WATCHDOG
>  
>  if WATCHDOG
>  
> +config WATCHDOG_CORE
> +	tristate "WatchDog Timer Driver Core"

What about the idea of making this bool and let it always be compiled as soon
as WATCHDOG is selected?

> +	---help---
> +	  Say Y here if you want to use the new watchdog timer driver core.
> +	  This driver provides a framework for all watchdog timer drivers
> +	  and gives them the /dev/watchdog interface (and later also the
> +	  sysfs interface).
> +
> +	  To compile this driver as a module, choose M here: the module will
> +	  be called watchdog.
> +
>  config WATCHDOG_NOWAYOUT
>  	bool "Disable watchdog shutdown on close"
>  	help
> diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
> index b2ddff7..7b26484 100644
> --- a/drivers/watchdog/Makefile
> +++ b/drivers/watchdog/Makefile
> @@ -2,6 +2,10 @@
>  # Makefile for the WatchDog device drivers.
>  #
>  
> +# The WatchDog Timer Driver Core.
> +watchdog-objs	+= watchdog_core.o watchdog_dev.o
> +obj-$(CONFIG_WATCHDOG_CORE)	+= watchdog.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
> new file mode 100644
> index 0000000..79684ad
> --- /dev/null
> +++ b/drivers/watchdog/watchdog_core.c
> @@ -0,0 +1,108 @@
> +/*
> + *	watchdog_core.c
> + *
> + *	(c) Copyright 2008-2011 Alan Cox <alan@lxorguk.ukuu.org.uk>,
> + *						All Rights Reserved.
> + *
> + *	(c) Copyright 2008-2011 Wim Van Sebroeck <wim@iguana.be>.
> + *
> + *	This source code is part of the generic code that can be used
> + *	by all the watchdog timer drivers.
> + *
> + *	Based on source code of the following authors:
> + *	  Matt Domsch <Matt_Domsch@dell.com>,
> + *	  Rob Radez <rob@osinvestor.com>,
> + *	  Rusty Lynch <rusty@linux.co.intel.com>
> + *	  Satyam Sharma <satyam@infradead.org>
> + *	  Randy Dunlap <randy.dunlap@oracle.com>
> + *
> + *	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.
> + *
> + *	Neither Alan Cox, CymruNet Ltd., Wim Van Sebroeck nor Iguana vzw.
> + *	admit liability nor provide warranty for any of this software.
> + *	This material is provided "AS-IS" and at no charge.
> + */
> +
> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> +
> +/*
> + *	Include files
> + */
> +#include <linux/module.h>	/* For EXPORT_SYMBOL/module stuff/... */
> +#include <linux/types.h>	/* For standard types */
> +#include <linux/errno.h>	/* For the -ENODEV/... values */
> +#include <linux/kernel.h>	/* For printk/panic/... */
> +#include <linux/watchdog.h>	/* For watchdog specific items */
> +#include <linux/init.h>		/* For __init/__exit/... */
> +
> +#include "watchdog_dev.h"	/* For watchdog_dev_register/... */
> +
> +/**
> + *	watchdog_register_device	-	register a watchdog device
> + *	@wdd: watchdog device
> + *
> + *	Register a watchdog device with the kernel so that the
> + *	watchdog timer can be accessed from userspace.
> + *
> + *	A zero is returned on success and a negative errno code for
> + *	failure.
> + */
> +int watchdog_register_device(struct watchdog_device *wdd)
> +{
> +	int ret;
> +
> +	/* Make sure we have a valid watchdog_device structure */
> +	if (wdd == NULL || wdd->info == NULL || wdd->ops == NULL)
> +		return -EINVAL;
> +
> +	/* Make sure that the mandatory operations are supported */
> +	if (wdd->ops->start == NULL || wdd->ops->stop == NULL)
> +		return -EINVAL;

Check also for wdd->ops->owner? Should we mark it as mandatory?

> +
> +	/*
> +	 * Note: now that all watchdog_device data has been verified, we
> +	 * will not check this anymore in other functions. If data gets
> +	 * corrupted in a later stage then we expect a kernel panic!
> +	 */
> +
> +	/* We only support 1 watchdog device via the /dev/watchdog interface */
> +	ret = watchdog_dev_register(wdd);
> +	if (ret) {
> +		pr_err("error registering /dev/watchdog (err=%d).\n", ret);
> +		return ret;
> +	}
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(watchdog_register_device);
> +
> +/**
> + *	watchdog_unregister_device	-	unregister a watchdog device
> + *	@wdd: watchdog device to unregister
> + *
> + *	Unregister a watchdog device that was previously successfully
> + *	registered with watchdog_register_device().
> + */
> +void watchdog_unregister_device(struct watchdog_device *wdd)
> +{
> +	int ret;
> +
> +	/* Make sure we have a valid watchdog_device structure */

Overdocumented?

> +	if (wdd == NULL)
> +		return;
> +
> +	/* remove the /dev/watchdog interface */

Overdocumented?

> +	ret = watchdog_dev_unregister(wdd);
> +	if (ret)
> +		pr_err("error unregistering /dev/watchdog (err=%d).\n", ret);
> +}
> +EXPORT_SYMBOL_GPL(watchdog_unregister_device);
> +
> +MODULE_AUTHOR("Alan Cox <alan@lxorguk.ukuu.org.uk>");
> +MODULE_AUTHOR("Wim Van Sebroeck <wim@iguana.be>");
> +MODULE_DESCRIPTION("WatchDog Timer Driver Core");
> +MODULE_LICENSE("GPL");
> +MODULE_SUPPORTED_DEVICE("watchdog");

That one should go? There isn't even a init_function here, so nothing will
happen when the module gets loaded. (and if we pickup the idea of this being
always compiled when WATCHDOG is selected, it won't even be a module).

> diff --git a/drivers/watchdog/watchdog_dev.c b/drivers/watchdog/watchdog_dev.c
> new file mode 100644
> index 0000000..c47154c
> --- /dev/null
> +++ b/drivers/watchdog/watchdog_dev.c
> @@ -0,0 +1,261 @@
> +/*
> + *	watchdog_dev.c
> + *
> + *	(c) Copyright 2008-2011 Alan Cox <alan@lxorguk.ukuu.org.uk>,
> + *						All Rights Reserved.
> + *
> + *	(c) Copyright 2008-2011 Wim Van Sebroeck <wim@iguana.be>.
> + *
> + *
> + *	This source code is part of the generic code that can be used
> + *	by all the watchdog timer drivers.
> + *
> + *	This part of the generic code takes care of the following
> + *	misc device: /dev/watchdog.
> + *
> + *	Based on source code of the following authors:
> + *	  Matt Domsch <Matt_Domsch@dell.com>,
> + *	  Rob Radez <rob@osinvestor.com>,
> + *	  Rusty Lynch <rusty@linux.co.intel.com>
> + *	  Satyam Sharma <satyam@infradead.org>
> + *	  Randy Dunlap <randy.dunlap@oracle.com>
> + *
> + *	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.
> + *
> + *	Neither Alan Cox, CymruNet Ltd., Wim Van Sebroeck nor Iguana vzw.
> + *	admit liability nor provide warranty for any of this software.
> + *	This material is provided "AS-IS" and at no charge.
> + */
> +
> +#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
> +
> +/*
> + *	Include files
> + */

Overdocumented?

> +#include <linux/module.h>	/* For module stuff/... */
> +#include <linux/types.h>	/* For standard types (like size_t) */
> +#include <linux/errno.h>	/* For the -ENODEV/... values */
> +#include <linux/kernel.h>	/* For printk/panic/... */
> +#include <linux/fs.h>		/* For file operations */
> +#include <linux/watchdog.h>	/* For watchdog specific items */
> +#include <linux/miscdevice.h>	/* For handling misc devices */
> +#include <linux/init.h>		/* For __init/__exit/... */
> +#include <linux/uaccess.h>	/* For copy_to_user/put_user/... */
> +
> +/*
> + *	Locally used variables
> + */

Overdocumented?

> +
> +/* make sure we only register one /dev/watchdog device */
> +static unsigned long watchdog_dev_busy;
> +/* the watchdog device behind /dev/watchdog */
> +static struct watchdog_device *wdd;
> +
> +/*
> + *	/dev/watchdog operations
> + */
> +
> +/*
> + *	watchdog_ping: ping the watchdog.
> + *	@wddev: the watchdog device to ping
> + *
> + *	If the watchdog has no own ping operation then it needs to be
> + *	restarted via the start operation. This wrapper function does
> + *	exactly that.
> + */
> +
> +static int watchdog_ping(struct watchdog_device *wddev)
> +{
> +	if (wddev->ops->ping)
> +		return wddev->ops->ping(wddev);  /* ping the watchdog */
> +	else
> +		return wddev->ops->start(wddev); /* restart the watchdog */
> +}
> +
> +/*
> + *	watchdog_write: writes to the watchdog.
> + *	@file: file from VFS
> + *	@data: user address of data
> + *	@len: length of data
> + *	@ppos: pointer to the file offset
> + *
> + *	A write to a watchdog device is defined as a keepalive ping.
> + */
> +
> +static ssize_t watchdog_write(struct file *file, const char __user *data,
> +						size_t len, loff_t *ppos)
> +{
> +	size_t i;
> +	char c;
> +
> +	if (len == 0)
> +		return 0;
> +
> +	for (i = 0; i != len; i++) {
> +		if (get_user(c, data + i))
> +			return -EFAULT;
> +	}
> +
> +	/* someone wrote to us, so we send the watchdog a keepalive ping */
> +	watchdog_ping(wdd);
> +
> +	return len;
> +}
> +
> +/*
> + *	watchdog_open: open the /dev/watchdog device.
> + *	@inode: inode of device
> + *	@file: file handle to device
> + *
> + *	When the /dev/watchdog device gets opened, we start the watchdog.
> + *	Watch out: the /dev/watchdog device is single open, so we make sure
> + *	it can only be opened once.
> + */
> +
> +static int watchdog_open(struct inode *inode, struct file *file)
> +{
> +	int err = -EBUSY;
> +
> +	/* the watchdog is single open! */
> +	if (test_and_set_bit(WDOG_DEV_OPEN, &wdd->status))
> +		return -EBUSY;
> +
> +	/*
> +	 * If the /dev/watchdog device is open, we don't want the module
> +	 * to be unloaded.
> +	 */
> +	if (!try_module_get(wdd->ops->owner))
> +		goto out;
> +
> +	/* start the watchdog */

Overdocumented?

> +	err = wdd->ops->start(wdd);
> +	if (err < 0)
> +		goto out_mod;
> +
> +	/* dev/watchdog is a virtual (and thus non-seekable) filesystem */
> +	return nonseekable_open(inode, file);
> +
> +out_mod:
> +	module_put(wdd->ops->owner);
> +out:
> +	clear_bit(WDOG_DEV_OPEN, &wdd->status);
> +	return err;
> +}
> +
> +/*
> + *      watchdog_release: release the /dev/watchdog device.
> + *      @inode: inode of device
> + *      @file: file handle to device
> + *
> + *	This is the code for when /dev/watchdog gets closed.
> + */
> +
> +static int watchdog_release(struct inode *inode, struct file *file)
> +{
> +	int err;
> +
> +	/* stop the watchdog */

Overdocumented?

> +	err = wdd->ops->stop(wdd);
> +	if (err != 0) {
> +		pr_crit("%s: watchdog did not stop!\n", wdd->info->identity);
> +		watchdog_ping(wdd);
> +	}
> +
> +	/* Allow the owner module to be unloaded again */
> +	module_put(wdd->ops->owner);
> +
> +	/* make sure that /dev/watchdog can be re-opened */
> +	clear_bit(WDOG_DEV_OPEN, &wdd->status);
> +
> +	return 0;
> +}
> +
> +/*
> + *	/dev/watchdog kernel interfaces
> + */

Overdocumented?

> +
> +static const struct file_operations watchdog_fops = {
> +	.owner		= THIS_MODULE,
> +	.llseek		= no_llseek,
> +	.write		= watchdog_write,
> +	.open		= watchdog_open,
> +	.release	= watchdog_release,
> +};
> +
> +static struct miscdevice watchdog_miscdev = {
> +	.minor		= WATCHDOG_MINOR,
> +	.name		= "watchdog",
> +	.fops		= &watchdog_fops,
> +};
> +
> +/*
> + *	/dev/watchdog register and unregister functions
> + */

Overdocumented?

> +
> +/*
> + *	watchdog_dev_register:
> + *	@watchdog: watchdog device
> + *
> + *	Register a watchdog device as /dev/watchdog. /dev/watchdog
> + *	is actually a miscdevice and thus we set it up like that.
> + */
> +
> +int watchdog_dev_register(struct watchdog_device *watchdog)
> +{
> +	int err;
> +
> +	/* Only one device can register for /dev/watchdog */
> +	if (test_and_set_bit(0, &watchdog_dev_busy)) {
> +		pr_err("only one watchdog can use /dev/watchdog.\n");
> +		return -EBUSY;
> +	}
> +
> +	wdd = watchdog;
> +
> +	/* Register the miscdevice */

Overdocumented?

> +	err = misc_register(&watchdog_miscdev);
> +	if (err != 0) {
> +		pr_err("%s: cannot register miscdev on minor=%d (err=%d).\n",
> +			watchdog->info->identity, WATCHDOG_MINOR, err);
> +		goto out;
> +	}
> +
> +	return 0;
> +
> +out:
> +	wdd = NULL;
> +	clear_bit(0, &watchdog_dev_busy);
> +	return err;
> +}
> +
> +/*
> + *	watchdog_dev_unregister:
> + *	@watchdog: watchdog device
> + *
> + *	Deregister the /dev/watchdog device.
> + */
> +
> +int watchdog_dev_unregister(struct watchdog_device *watchdog)
> +{
> +	/* Check that a watchdog device was registered in the past */
> +	if (!test_bit(0, &watchdog_dev_busy) || !wdd)
> +		return -ENODEV;
> +
> +	/* We can only unregister the watchdog device that was registered */
> +	if (watchdog != wdd) {
> +		pr_err("%s: watchdog was not registered as /dev/watchdog.\n",
> +			watchdog->info->identity);
> +		return -ENODEV;
> +	}
> +
> +	/* Unregister the miscdevice */

Overdocumented?

> +	misc_deregister(&watchdog_miscdev);
> +	wdd = NULL;
> +	clear_bit(0, &watchdog_dev_busy);
> +	return 0;
> +}
> +
> +MODULE_ALIAS_MISCDEV(WATCHDOG_MINOR);

Same as for MODULE_SUPPORTED_DEVICE?

> diff --git a/drivers/watchdog/watchdog_dev.h b/drivers/watchdog/watchdog_dev.h
> new file mode 100644
> index 0000000..bc7612b
> --- /dev/null
> +++ b/drivers/watchdog/watchdog_dev.h
> @@ -0,0 +1,33 @@
> +/*
> + *	watchdog_core.h
> + *
> + *	(c) Copyright 2008-2011 Alan Cox <alan@lxorguk.ukuu.org.uk>,
> + *						All Rights Reserved.
> + *
> + *	(c) Copyright 2008-2011 Wim Van Sebroeck <wim@iguana.be>.
> + *
> + *	This source code is part of the generic code that can be used
> + *	by all the watchdog timer drivers.
> + *
> + *	Based on source code of the following authors:
> + *	  Matt Domsch <Matt_Domsch@dell.com>,
> + *	  Rob Radez <rob@osinvestor.com>,
> + *	  Rusty Lynch <rusty@linux.co.intel.com>
> + *	  Satyam Sharma <satyam@infradead.org>
> + *	  Randy Dunlap <randy.dunlap@oracle.com>
> + *
> + *	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.
> + *
> + *	Neither Alan Cox, CymruNet Ltd., Wim Van Sebroeck nor Iguana vzw.
> + *	admit liability nor provide warranty for any of this software.
> + *	This material is provided "AS-IS" and at no charge.
> + */
> +
> +/*
> + *	Functions/procedures to be called by the core
> + */
> +int watchdog_dev_register(struct watchdog_device *);
> +int watchdog_dev_unregister(struct watchdog_device *);
> diff --git a/include/linux/watchdog.h b/include/linux/watchdog.h
> index 011bcfe..40333ff 100644
> --- a/include/linux/watchdog.h
> +++ b/include/linux/watchdog.h
> @@ -59,6 +59,33 @@ struct watchdog_info {
>  #define WATCHDOG_NOWAYOUT	0
>  #endif
>  
> +struct watchdog_ops;
> +struct watchdog_device;
> +
> +/* The watchdog-devices operations */
> +struct watchdog_ops {
> +	struct module *owner;
> +	/* mandatory operations */
> +	int (*start)(struct watchdog_device *);
> +	int (*stop)(struct watchdog_device *);
> +	/* optional operations */
> +	int (*ping)(struct watchdog_device *);
> +};
> +
> +/* The structure that defines a watchdog device */
> +struct watchdog_device {
> +	const struct watchdog_info *info;
> +	const struct watchdog_ops *ops;
> +	void *priv;
> +	unsigned long status;
> +/* Bit numbers for status flags */
> +#define WDOG_DEV_OPEN		1	/* Opened via /dev/watchdog ? */
> +};
> +
> +/* drivers/watchdog/core/watchdog_core.c */
> +extern int watchdog_register_device(struct watchdog_device *);
> +extern void watchdog_unregister_device(struct watchdog_device *);
> +
>  #endif	/* __KERNEL__ */
>  
>  #endif  /* ifndef _LINUX_WATCHDOG_H */
> -- 
> 1.7.6
> 
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH 01/11] watchdog: WatchDog Timer Driver Core - Add basic framework

[Lars-Peter Clausen]

On 07/11/2011 11:32 PM, Wolfram Sang wrote:

>> +int watchdog_register_device(struct watchdog_device *wdd)
>> +{
>> +	int ret;
>> +
>> +	/* Make sure we have a valid watchdog_device structure */
>> +	if (wdd == NULL || wdd->info == NULL || wdd->ops == NULL)
>> +		return -EINVAL;
>> +
>> +	/* Make sure that the mandatory operations are supported */
>> +	if (wdd->ops->start == NULL || wdd->ops->stop == NULL)
>> +		return -EINVAL;
> 
> Check also for wdd->ops->owner? Should we mark it as mandatory?
> 

If the driver is built-in owner will be NULL.

Re: [PATCH 01/11] watchdog: WatchDog Timer Driver Core - Add basic framework

[Wim Van Sebroeck]

Hi Wolfram,

> > diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
> > index 372bc64..edb0f1b 100644
> > --- a/drivers/watchdog/Kconfig
> > +++ b/drivers/watchdog/Kconfig
> > @@ -28,6 +28,17 @@ menuconfig WATCHDOG
> >  
> >  if WATCHDOG
> >  
> > +config WATCHDOG_CORE
> > +	tristate "WatchDog Timer Driver Core"
> 
> What about the idea of making this bool and let it always be compiled as soon
> as WATCHDOG is selected?

This should indeed have been bool. For this merge window it will stay as WATCHDOG_CORE.
In a futue release WATCHDOG_CORE will disappear and the watchdog core will be dependant
on CONFIG_WATCHDOG. Goal is that we only have 1 lower-level API and that all other drivers
use that.

> > +int watchdog_register_device(struct watchdog_device *wdd)
> > +{
> > +	int ret;
> > +
> > +	/* Make sure we have a valid watchdog_device structure */
> > +	if (wdd == NULL || wdd->info == NULL || wdd->ops == NULL)
> > +		return -EINVAL;
> > +
> > +	/* Make sure that the mandatory operations are supported */
> > +	if (wdd->ops->start == NULL || wdd->ops->stop == NULL)
> > +		return -EINVAL;
> 
> Check also for wdd->ops->owner? Should we mark it as mandatory?

No, see other comments also (if built-in owner is NULL).

All other comments have been incorporated.
It's allready in the linux-2.6-watchdog-next tree. Will try to sent out the new series as soon as possible.

Kind regards,
Wim.

Re: [PATCH 01/11] watchdog: WatchDog Timer Driver Core - Add basic framework

[Lars-Peter Clausen]

On 07/11/2011 04:19 PM, Wim Van Sebroeck wrote:
> [...]
> +
> +
> +static int watchdog_release(struct inode *inode, struct file *file)
> +{
> +	int err;
> +
> +	/* stop the watchdog */
> +	err = wdd->ops->stop(wdd);
Does it really make sense to allow stop() to fail? Will this ever happen, and
if yes do we gain anything by sending a additional ping?

> +	if (err != 0) {
> +		pr_crit("%s: watchdog did not stop!\n", wdd->info->identity);
> +		watchdog_ping(wdd);
> +	}
> +
> +	/* Allow the owner module to be unloaded again */
> +	module_put(wdd->ops->owner);
> +
> +	/* make sure that /dev/watchdog can be re-opened */
> +	clear_bit(WDOG_DEV_OPEN, &wdd->status);
> +
> +	return 0;
> +}
> +
> [...]
> +
> +int watchdog_dev_unregister(struct watchdog_device *watchdog)
> +{
> +	/* Check that a watchdog device was registered in the past */
> +	if (!test_bit(0, &watchdog_dev_busy) || !wdd)
> +		return -ENODEV;
> +
> +	/* We can only unregister the watchdog device that was registered */
> +	if (watchdog != wdd) {
> +		pr_err("%s: watchdog was not registered as /dev/watchdog.\n",
> +			watchdog->info->identity);
> +		return -ENODEV;
> +	}
> +
> +	/* Unregister the miscdevice */
> +	misc_deregister(&watchdog_miscdev);
> +	wdd = NULL;
> +	clear_bit(0, &watchdog_dev_busy);
> +	return 0;
> +}

What happens if the watchdog gets unregistered if the device is still opened?
Even though if you'd check wdd for not being NULL in the file callbacks there
is still a chance for races if the devices is unregistered at the same time as
the callback is running. You'd either need a big lock to protect from having a
file callback and unregister running concurrently or add ref-counting to the
watchdog_device, the later best done by embedding a struct device and using the
device driver model.

> +
> +MODULE_ALIAS_MISCDEV(WATCHDOG_MINOR);
> diff --git a/drivers/watchdog/watchdog_dev.h b/drivers/watchdog/watchdog_dev.h
> new file mode 100644
> index 0000000..bc7612b
> --- /dev/null
> +++ b/drivers/watchdog/watchdog_dev.h
> @@ -0,0 +1,33 @@
> [...]
>  
> +struct watchdog_ops;
> +struct watchdog_device;
> +
> +/* The watchdog-devices operations */
> +struct watchdog_ops {
> +	struct module *owner;
> +	/* mandatory operations */
> +	int (*start)(struct watchdog_device *);
> +	int (*stop)(struct watchdog_device *);
> +	/* optional operations */
> +	int (*ping)(struct watchdog_device *);
> +};
> +
> +/* The structure that defines a watchdog device */
> +struct watchdog_device {
> +	const struct watchdog_info *info;
> +	const struct watchdog_ops *ops;
> +	void *priv;

You should provide getter/setter methods for priv, so that when the watchdog
API is changed to make use of the device driver model it becomes easier to get
rid of the priv field and use dev_{get,set}_drvdata instead.

> +	unsigned long status;
> +/* Bit numbers for status flags */
> +#define WDOG_DEV_OPEN		1	/* Opened via /dev/watchdog ? */
> +};

Kernel-doc style documentation for the structs would be nice.

> +
> +/* drivers/watchdog/core/watchdog_core.c */
> +extern int watchdog_register_device(struct watchdog_device *);
> +extern void watchdog_unregister_device(struct watchdog_device *);
> +
>  #endif	/* __KERNEL__ */
>  
>  #endif  /* ifndef _LINUX_WATCHDOG_H */

Re: [PATCH 01/11] watchdog: WatchDog Timer Driver Core - Add basic framework

[Mark Brown]

On Tue, Jul 12, 2011 at 01:00:25AM +0200, Lars-Peter Clausen wrote:
> On 07/11/2011 04:19 PM, Wim Van Sebroeck wrote:

> > +	/* stop the watchdog */
> > +	err = wdd->ops->stop(wdd);
> Does it really make sense to allow stop() to fail? Will this ever happen, and

Some hardware can't be stopped, sometimes depending on circumstance.

> if yes do we gain anything by sending a additional ping?

It would increase the chances that someone sees the error message, or
that it gets written to disk or whatever (especially if the watchdog
time is short).

Re: [PATCH 01/11] watchdog: WatchDog Timer Driver Core - Add basic framework

[Alan Cox]

> > +	/* stop the watchdog */
> > +	err = wdd->ops->stop(wdd);
> Does it really make sense to allow stop() to fail? Will this ever happen, and
> if yes do we gain anything by sending a additional ping?

It gives user space more time to react - or would do if the err was
propogated.

Re: [PATCH 01/11] watchdog: WatchDog Timer Driver Core - Add basic framework

[Wim Van Sebroeck]

Hi Lars-Peter,

> On 07/11/2011 04:19 PM, Wim Van Sebroeck wrote:
> > [...]
> > +
> > +
> > +static int watchdog_release(struct inode *inode, struct file *file)
> > +{
> > +	int err;
> > +
> > +	/* stop the watchdog */
> > +	err = wdd->ops->stop(wdd);
> Does it really make sense to allow stop() to fail? Will this ever happen, and
> if yes do we gain anything by sending a additional ping?

It buys user-space extra time to recover if necessary.

> > [...]
> > +
> > +int watchdog_dev_unregister(struct watchdog_device *watchdog)
> > +{
> > +	/* Check that a watchdog device was registered in the past */
> > +	if (!test_bit(0, &watchdog_dev_busy) || !wdd)
> > +		return -ENODEV;
> > +
> > +	/* We can only unregister the watchdog device that was registered */
> > +	if (watchdog != wdd) {
> > +		pr_err("%s: watchdog was not registered as /dev/watchdog.\n",
> > +			watchdog->info->identity);
> > +		return -ENODEV;
> > +	}
> > +
> > +	/* Unregister the miscdevice */
> > +	misc_deregister(&watchdog_miscdev);
> > +	wdd = NULL;
> > +	clear_bit(0, &watchdog_dev_busy);
> > +	return 0;
> > +}
> 
> What happens if the watchdog gets unregistered if the device is still opened?
> Even though if you'd check wdd for not being NULL in the file callbacks there
> is still a chance for races if the devices is unregistered at the same time as
> the callback is running. You'd either need a big lock to protect from having a
> file callback and unregister running concurrently or add ref-counting to the
> watchdog_device, the later best done by embedding a struct device and using the
> device driver model.

You cannot unload the watchdog-drivers module if /dev/watchdog is still open.
So if the watchdog_unregister function is in the exit function of the module
then we are safe. But I think you have a point if that is not the case.
Solution would be to return an error when the watchdog_unregister_device routine
is called and the WDOG_DEV_OPEN bit is set. Will create an extra patch for that.

> > [...]
> >  
> > +struct watchdog_ops;
> > +struct watchdog_device;
> > +
> > +/* The watchdog-devices operations */
> > +struct watchdog_ops {
> > +	struct module *owner;
> > +	/* mandatory operations */
> > +	int (*start)(struct watchdog_device *);
> > +	int (*stop)(struct watchdog_device *);
> > +	/* optional operations */
> > +	int (*ping)(struct watchdog_device *);
> > +};
> > +
> > +/* The structure that defines a watchdog device */
> > +struct watchdog_device {
> > +	const struct watchdog_info *info;
> > +	const struct watchdog_ops *ops;
> > +	void *priv;
> 
> You should provide getter/setter methods for priv, so that when the watchdog
> API is changed to make use of the device driver model it becomes easier to get
> rid of the priv field and use dev_{get,set}_drvdata instead.

Added.

> > +	unsigned long status;
> > +/* Bit numbers for status flags */
> > +#define WDOG_DEV_OPEN		1	/* Opened via /dev/watchdog ? */
> > +};
> 
> Kernel-doc style documentation for the structs would be nice.

Added.

Kind regards,
Wim.

Re: [PATCH 01/11] watchdog: WatchDog Timer Driver Core - Add basic framework

[Lars-Peter Clausen]

On 07/22/2011 09:32 PM, Wim Van Sebroeck wrote:
> Hi Lars-Peter,
> 
>> On 07/11/2011 04:19 PM, Wim Van Sebroeck wrote:
>>> +
>>> +int watchdog_dev_unregister(struct watchdog_device *watchdog)
>>> +{
>>> +	/* Check that a watchdog device was registered in the past */
>>> +	if (!test_bit(0, &watchdog_dev_busy) || !wdd)
>>> +		return -ENODEV;
>>> +
>>> +	/* We can only unregister the watchdog device that was registered */
>>> +	if (watchdog != wdd) {
>>> +		pr_err("%s: watchdog was not registered as /dev/watchdog.\n",
>>> +			watchdog->info->identity);
>>> +		return -ENODEV;
>>> +	}
>>> +
>>> +	/* Unregister the miscdevice */
>>> +	misc_deregister(&watchdog_miscdev);
>>> +	wdd = NULL;
>>> +	clear_bit(0, &watchdog_dev_busy);
>>> +	return 0;
>>> +}
>>
>> What happens if the watchdog gets unregistered if the device is still opened?
>> Even though if you'd check wdd for not being NULL in the file callbacks there
>> is still a chance for races if the devices is unregistered at the same time as
>> the callback is running. You'd either need a big lock to protect from having a
>> file callback and unregister running concurrently or add ref-counting to the
>> watchdog_device, the later best done by embedding a struct device and using the
>> device driver model.
> 
> You cannot unload the watchdog-drivers module if /dev/watchdog is still open.
> So if the watchdog_unregister function is in the exit function of the module
> then we are safe. But I think you have a point if that is not the case.
> Solution would be to return an error when the watchdog_unregister_device routine
> is called and the WDOG_DEV_OPEN bit is set. Will create an extra patch for that.
> 

The problem is, that this doesn't fit nicely into the linux device driver
model, because it doesn't allow the removal of a device to fail. So you'll
still end of with undefined behavior.

- Lars

Re: [PATCH 01/11] watchdog: WatchDog Timer Driver Core - Add basic framework

[Wim Van Sebroeck]

Hi Lars-Peter,

> >>> +int watchdog_dev_unregister(struct watchdog_device *watchdog)
> >>> +{
> >>> +	/* Check that a watchdog device was registered in the past */
> >>> +	if (!test_bit(0, &watchdog_dev_busy) || !wdd)
> >>> +		return -ENODEV;
> >>> +
> >>> +	/* We can only unregister the watchdog device that was registered */
> >>> +	if (watchdog != wdd) {
> >>> +		pr_err("%s: watchdog was not registered as /dev/watchdog.\n",
> >>> +			watchdog->info->identity);
> >>> +		return -ENODEV;
> >>> +	}
> >>> +
> >>> +	/* Unregister the miscdevice */
> >>> +	misc_deregister(&watchdog_miscdev);
> >>> +	wdd = NULL;
> >>> +	clear_bit(0, &watchdog_dev_busy);
> >>> +	return 0;
> >>> +}
> >>
> >> What happens if the watchdog gets unregistered if the device is still opened?
> >> Even though if you'd check wdd for not being NULL in the file callbacks there
> >> is still a chance for races if the devices is unregistered at the same time as
> >> the callback is running. You'd either need a big lock to protect from having a
> >> file callback and unregister running concurrently or add ref-counting to the
> >> watchdog_device, the later best done by embedding a struct device and using the
> >> device driver model.
> > 
> > You cannot unload the watchdog-drivers module if /dev/watchdog is still open.
> > So if the watchdog_unregister function is in the exit function of the module
> > then we are safe. But I think you have a point if that is not the case.
> > Solution would be to return an error when the watchdog_unregister_device routine
> > is called and the WDOG_DEV_OPEN bit is set. Will create an extra patch for that.
> > 
> 
> The problem is, that this doesn't fit nicely into the linux device driver
> model, because it doesn't allow the removal of a device to fail. So you'll
> still end of with undefined behavior.

It's not an issue now. But we will indeed have to tackle it, when we start using
the device driver model.

Kind regards,
Wim.

Re: [PATCH 01/11] watchdog: WatchDog Timer Driver Core - Add basic framework

[Lars-Peter Clausen]

On 07/27/2011 10:24 PM, Wim Van Sebroeck wrote:
> Hi Lars-Peter,
> 
>>>>> +int watchdog_dev_unregister(struct watchdog_device *watchdog)
>>>>> +{
>>>>> +	/* Check that a watchdog device was registered in the past */
>>>>> +	if (!test_bit(0, &watchdog_dev_busy) || !wdd)
>>>>> +		return -ENODEV;
>>>>> +
>>>>> +	/* We can only unregister the watchdog device that was registered */
>>>>> +	if (watchdog != wdd) {
>>>>> +		pr_err("%s: watchdog was not registered as /dev/watchdog.\n",
>>>>> +			watchdog->info->identity);
>>>>> +		return -ENODEV;
>>>>> +	}
>>>>> +
>>>>> +	/* Unregister the miscdevice */
>>>>> +	misc_deregister(&watchdog_miscdev);
>>>>> +	wdd = NULL;
>>>>> +	clear_bit(0, &watchdog_dev_busy);
>>>>> +	return 0;
>>>>> +}
>>>>
>>>> What happens if the watchdog gets unregistered if the device is still opened?
>>>> Even though if you'd check wdd for not being NULL in the file callbacks there
>>>> is still a chance for races if the devices is unregistered at the same time as
>>>> the callback is running. You'd either need a big lock to protect from having a
>>>> file callback and unregister running concurrently or add ref-counting to the
>>>> watchdog_device, the later best done by embedding a struct device and using the
>>>> device driver model.
>>>
>>> You cannot unload the watchdog-drivers module if /dev/watchdog is still open.
>>> So if the watchdog_unregister function is in the exit function of the module
>>> then we are safe. But I think you have a point if that is not the case.
>>> Solution would be to return an error when the watchdog_unregister_device routine
>>> is called and the WDOG_DEV_OPEN bit is set. Will create an extra patch for that.
>>>
>>
>> The problem is, that this doesn't fit nicely into the linux device driver
>> model, because it doesn't allow the removal of a device to fail. So you'll
>> still end of with undefined behavior.
> 
> It's not an issue now. But we will indeed have to tackle it, when we start using
> the device driver model.
> 

Well, the framework itself might not be using the device driver model yet, but
drivers using the framework do. For example for a driver using a platform
device there would be no correct solution for handling an error from
watchdog_unregister in the drivers remove callback.

- Lars

Re: [PATCH 01/11] watchdog: WatchDog Timer Driver Core - Add basic framework

[Wim Van Sebroeck]

Hi Lars,

> >>>>> +int watchdog_dev_unregister(struct watchdog_device *watchdog)
> >>>>> +{
> >>>>> +	/* Check that a watchdog device was registered in the past */
> >>>>> +	if (!test_bit(0, &watchdog_dev_busy) || !wdd)
> >>>>> +		return -ENODEV;
> >>>>> +
> >>>>> +	/* We can only unregister the watchdog device that was registered */
> >>>>> +	if (watchdog != wdd) {
> >>>>> +		pr_err("%s: watchdog was not registered as /dev/watchdog.\n",
> >>>>> +			watchdog->info->identity);
> >>>>> +		return -ENODEV;
> >>>>> +	}
> >>>>> +
> >>>>> +	/* Unregister the miscdevice */
> >>>>> +	misc_deregister(&watchdog_miscdev);
> >>>>> +	wdd = NULL;
> >>>>> +	clear_bit(0, &watchdog_dev_busy);
> >>>>> +	return 0;
> >>>>> +}
> >>>>
> >>>> What happens if the watchdog gets unregistered if the device is still opened?
> >>>> Even though if you'd check wdd for not being NULL in the file callbacks there
> >>>> is still a chance for races if the devices is unregistered at the same time as
> >>>> the callback is running. You'd either need a big lock to protect from having a
> >>>> file callback and unregister running concurrently or add ref-counting to the
> >>>> watchdog_device, the later best done by embedding a struct device and using the
> >>>> device driver model.
> >>>
> >>> You cannot unload the watchdog-drivers module if /dev/watchdog is still open.
> >>> So if the watchdog_unregister function is in the exit function of the module
> >>> then we are safe. But I think you have a point if that is not the case.
> >>> Solution would be to return an error when the watchdog_unregister_device routine
> >>> is called and the WDOG_DEV_OPEN bit is set. Will create an extra patch for that.
> >>>
> >>
> >> The problem is, that this doesn't fit nicely into the linux device driver
> >> model, because it doesn't allow the removal of a device to fail. So you'll
> >> still end of with undefined behavior.
> > 
> > It's not an issue now. But we will indeed have to tackle it, when we start using
> > the device driver model.
> > 
> 
> Well, the framework itself might not be using the device driver model yet, but
> drivers using the framework do. For example for a driver using a platform
> device there would be no correct solution for handling an error from
> watchdog_unregister in the drivers remove callback.

That is correct. we have nothing for handling the error. So I didn't introduce
a fix for it neither. What I wanted to tell was:
* a driver now registers a miscdevice in its init function and deregisters it
during the exit function. You cannot unload the module as long as the miscdevice
for the watchdog is open.
* a driver that uses the core will register the watchdog device during init and
will only unregister it during exit. Because we prevent the driver of being
unloaded (via try_module_get and module_put), you cannot unload the module as
long as /dev/watchdog is open. So the unregister will not take place.

So in it's current form, we still have no issue.

Kind regards,
Wim.

Re: [PATCH 0/11 v3] Generic Watchdog Timer Driver

[Wolfram Sang]

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


> Changes since V2:
> * all "flags" are unsigned
> * timeout values are also unsigned
> * removed unnececessary debugging
> * clean-up comments
> * the core will be in drivers/watchdog/ and not in drivers/watchdog/core/

Also (just in case someone started converting drivers):

* some error-codes changed
* .name has been removed from watchdog_device in favor of watchdog_info->identity

I already started converting a few drivers and will post the patches as RFC
tomorrow. In general, converting went quite well and despite a few comments
(see later mails), I think this could go into 3.1, so that converted drivers
can go into 3.2 together with (probably) a few updates to the framework. There
is potential for a bit more consolidation, yet I think the basic stuff could
go in now.

Thanks to Wim and Alan for their work and CELF/LF for supporting the final
steps :)

Regards,

   Wolfram

-- 
Pengutronix e.K.                           | Wolfram Sang                |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH 0/11 v3] Generic Watchdog Timer Driver

[Wim Van Sebroeck]

Hi Wolfram,

> > Changes since V2:
> > * all "flags" are unsigned
> > * timeout values are also unsigned
> > * removed unnececessary debugging
> > * clean-up comments
> > * the core will be in drivers/watchdog/ and not in drivers/watchdog/core/
> 
> Also (just in case someone started converting drivers):
> 
> * some error-codes changed
> * .name has been removed from watchdog_device in favor of watchdog_info->identity

Yup, I forgot these. Thanks.

Status update: I incorporated your comments and Lars-Peter Clausen's comments and
created V4.  The changes since V3 are:
* removed overdocumentation
* removed MODULE_SUPPORTED_DEVICE and +MODULE_ALIAS_MISCDEV
* removed return -ENOTTY after switch (cmd) because we have the default allready
* s/sent/send/
* I'd suggest to swap the logic here (branch taken on error)
* no_llseek removal squashed into patch 1
* provide getter/setter methods for driver specific data so that we can easily do the conversion towards the device driver model
* removed parent because this would change when we will start using the device driver model


> I already started converting a few drivers and will post the patches as RFC
> tomorrow. In general, converting went quite well and despite a few comments
> (see later mails), I think this could go into 3.1, so that converted drivers
> can go into 3.2 together with (probably) a few updates to the framework. There
> is potential for a bit more consolidation, yet I think the basic stuff could
> go in now.
> 
> Thanks to Wim and Alan for their work and CELF/LF for supporting the final
> steps :)

And for your assistance and the feedback from others also.

Kind regards,
Wim.

Re: [PATCH 0/11 v3] Generic Watchdog Timer Driver

[Wim Van Sebroeck]

Hi Wolfram,

> Status update: I incorporated your comments and Lars-Peter Clausen's comments and
> created V4.  The changes since V3 are:
> * removed overdocumentation
> * removed MODULE_SUPPORTED_DEVICE and +MODULE_ALIAS_MISCDEV
> * removed return -ENOTTY after switch (cmd) because we have the default allready
> * s/sent/send/
> * I'd suggest to swap the logic here (branch taken on error)
> * no_llseek removal squashed into patch 1
> * provide getter/setter methods for driver specific data so that we can easily do the conversion towards the device driver model
> * removed parent because this would change when we will start using the device driver model

And:
* Changed CONFIG_WATCHDOG_CORE from tristate to bool
* Added Docbook documentation for include:linuw/watchdog.h

Todo:
* Fix watchdog_unregister_device so that it returns an error if /dev/watchdog is still open.

Kind regards,
Wim.

[PATCH 0/9 v4] Generic Watchdog Timer Driver

[Wim Van Sebroeck]

Hi All,

To reduce copying the same code over and over in each watchdog device driver, Alan Cox and myself constructed a new framework/API that consolidates the common
watchdog timer driver functions. The changes from the third post have been incorporated.

The code is ready to be sent upstream, but just to be sure I sent out this last version anyway.

This framework/API consists of the following patches:
part 1: Introduction of the WatchDog Timer Driver Core
part 2: Add the basic ioctl functionality
part 3: Add the WDIOC_KEEPALIVE ioctl
part 4: Add the WDIOC_SETOPTIONS ioctl
part 5: Add the WDIOC_SETTIMEOUT and WDIOC_GETTIMEOUT ioctl
part 6: Add the Magic Close feature
part 7: Add the nowayout feature
part 8: Add support for extra ioctl calls
part 9: Add the minimum and maximum timeout parameters.

The code is added to linux-2.6-watchdog-next since this weekend.

Changes since V3:
* removed overdocumentation
* removed MODULE_SUPPORTED_DEVICE and MODULE_ALIAS_MISCDEV
* removed return -ENOTTY after switch (cmd) because we have the default allready
* s/sent/send/
* I'd suggest to swap the logic here (branch taken on error)
* no_llseek removal squashed into patch 1
* provide getter/setter methods for driver specific data so that we can easily do the conversion towards the device driver model
* removed parent because this would change when we will start using the device driver model
* Changed CONFIG_WATCHDOG_CORE from tristate to bool
* Added Docbook documentation for include/linux/watchdog.h

Kind regards,
Wim.

[PATCH 1/9] watchdog: WatchDog Timer Driver Core - Add basic framework

[Wim Van Sebroeck]

The WatchDog Timer Driver Core is a framework
that contains the common code for all watchdog-driver's.
It also introduces a watchdog device structure and the
operations that go with it.

This is the introduction of this framework. This part
supports the minimal watchdog userspace API (or with
other words: the functionality to use /dev/watchdog's
open, release and write functionality as defined in
the simplest watchdog API). Extra functionality will
follow in the next set of patches.

Signed-off-by: Alan Cox <alan@lxorguk.ukuu.org.uk>
Signed-off-by: Wim Van Sebroeck <wim@iguana.be>
Acked-by: Arnd Bergmann <arnd@arndb.de>
Acked-by: Wolfram Sang <w.sang@pengutronix.de>
---
 Documentation/watchdog/00-INDEX                |    2 +
 Documentation/watchdog/watchdog-kernel-api.txt |  119 ++++++++++++
 drivers/watchdog/Kconfig                       |   11 +
 drivers/watchdog/Makefile                      |    4 +
 drivers/watchdog/watchdog_core.c               |  101 ++++++++++
 drivers/watchdog/watchdog_dev.c                |  235 ++++++++++++++++++++++++
 drivers/watchdog/watchdog_dev.h                |   33 ++++
 include/linux/watchdog.h                       |   61 ++++++
 8 files changed, 566 insertions(+), 0 deletions(-)
 create mode 100644 Documentation/watchdog/watchdog-kernel-api.txt
 create mode 100644 drivers/watchdog/watchdog_core.c
 create mode 100644 drivers/watchdog/watchdog_dev.c
 create mode 100644 drivers/watchdog/watchdog_dev.h

diff --git a/Documentation/watchdog/00-INDEX b/Documentation/watchdog/00-INDEX
index ee99451..fc51128 100644
--- a/Documentation/watchdog/00-INDEX
+++ b/Documentation/watchdog/00-INDEX
@@ -8,6 +8,8 @@ src/
 	- directory holding watchdog related example programs.
 watchdog-api.txt
 	- description of the Linux Watchdog driver API.
+watchdog-kernel-api.txt
+	- description of the Linux WatchDog Timer Driver Core kernel API.
 watchdog-parameters.txt
 	- information on driver parameters (for drivers other than
 	  the ones that have driver-specific files here)
diff --git a/Documentation/watchdog/watchdog-kernel-api.txt b/Documentation/watchdog/watchdog-kernel-api.txt
new file mode 100644
index 0000000..3db67e7
--- /dev/null
+++ b/Documentation/watchdog/watchdog-kernel-api.txt
@@ -0,0 +1,119 @@
+The Linux WatchDog Timer Driver Core kernel API.
+===============================================
+Last reviewed: 22-Jul-2011
+
+Wim Van Sebroeck <wim@iguana.be>
+
+Introduction
+------------
+This document does not describe what a WatchDog Timer (WDT) Driver or Device is.
+It also does not describe the API which can be used by user space to communicate
+with a WatchDog Timer. If you want to know this then please read the following
+file: Documentation/watchdog/watchdog-api.txt .
+
+So what does this document describe? It describes the API that can be used by
+WatchDog Timer Drivers that want to use the WatchDog Timer Driver Core
+Framework. This framework provides all interfacing towards user space so that
+the same code does not have to be reproduced each time. This also means that
+a watchdog timer driver then only needs to provide the different routines
+(operations) that control the watchdog timer (WDT).
+
+The API
+-------
+Each watchdog timer driver that wants to use the WatchDog Timer Driver Core
+must #include <linux/watchdog.h> (you would have to do this anyway when
+writing a watchdog device driver). This include file contains following
+register/unregister routines:
+
+extern int watchdog_register_device(struct watchdog_device *);
+extern void watchdog_unregister_device(struct watchdog_device *);
+
+The watchdog_register_device routine registers a watchdog timer device.
+The parameter of this routine is a pointer to a watchdog_device structure.
+This routine returns zero on success and a negative errno code for failure.
+
+The watchdog_unregister_device routine deregisters a registered watchdog timer
+device. The parameter of this routine is the pointer to the registered
+watchdog_device structure.
+
+The watchdog device structure looks like this:
+
+struct watchdog_device {
+	const struct watchdog_info *info;
+	const struct watchdog_ops *ops;
+	void *driver_data;
+	unsigned long status;
+};
+
+It contains following fields:
+* info: a pointer to a watchdog_info structure. This structure gives some
+  additional information about the watchdog timer itself. (Like it's unique name)
+* ops: a pointer to the list of watchdog operations that the watchdog supports.
+* driver_data: a pointer to the drivers private data of a watchdog device.
+  This data should only be accessed via the watchdog_set_drvadata and
+  watchdog_get_drvdata routines.
+* status: this field contains a number of status bits that give extra
+  information about the status of the device (Like: is the device opened via
+  the /dev/watchdog interface or not, ...).
+
+The list of watchdog operations is defined as:
+
+struct watchdog_ops {
+	struct module *owner;
+	/* mandatory operations */
+	int (*start)(struct watchdog_device *);
+	int (*stop)(struct watchdog_device *);
+	/* optional operations */
+	int (*ping)(struct watchdog_device *);
+};
+
+It is important that you first define the module owner of the watchdog timer
+driver's operations. This module owner will be used to lock the module when
+the watchdog is active. (This to avoid a system crash when you unload the
+module and /dev/watchdog is still open).
+Some operations are mandatory and some are optional. The mandatory operations
+are:
+* start: this is a pointer to the routine that starts the watchdog timer
+  device.
+  The routine needs a pointer to the watchdog timer device structure as a
+  parameter. It returns zero on success or a negative errno code for failure.
+* stop: with this routine the watchdog timer device is being stopped.
+  The routine needs a pointer to the watchdog timer device structure as a
+  parameter. It returns zero on success or a negative errno code for failure.
+  Some watchdog timer hardware can only be started and not be stopped. The
+  driver supporting this hardware needs to make sure that a start and stop
+  routine is being provided. This can be done by using a timer in the driver
+  that regularly sends a keepalive ping to the watchdog timer hardware.
+
+Not all watchdog timer hardware supports the same functionality. That's why
+all other routines/operations are optional. They only need to be provided if
+they are supported. These optional routines/operations are:
+* ping: this is the routine that sends a keepalive ping to the watchdog timer
+  hardware.
+  The routine needs a pointer to the watchdog timer device structure as a
+  parameter. It returns zero on success or a negative errno code for failure.
+  Most hardware that does not support this as a separate function uses the
+  start function to restart the watchdog timer hardware. And that's also what
+  the watchdog timer driver core does: to send a keepalive ping to the watchdog
+  timer hardware it will either use the ping operation (when available) or the
+  start operation (when the ping operation is not available).
+
+The status bits should (preferably) be set with the set_bit and clear_bit alike
+bit-operations. The status bits that are defined are:
+* WDOG_DEV_OPEN: this status bit shows whether or not the watchdog device
+  was opened via /dev/watchdog.
+  (This bit should only be used by the WatchDog Timer Driver Core).
+
+To get or set driver specific data the following two helper functions should be
+used:
+
+static inline void watchdog_set_drvdata(struct watchdog_device *wdd, void *data)
+static inline void *watchdog_get_drvdata(struct watchdog_device *wdd)
+
+The watchdog_set_drvdata function allows you to add driver specific data. The
+arguments of this function are the watchdog device where you want to add the
+driver specific data to and a pointer to the data itself.
+
+The watchdog_get_drvdata function allows you to retrieve driver specific data.
+The argument of this function is the watchdog device where you want to retrieve
+data from. The function retruns the pointer to the driver specific data.
diff --git a/drivers/watchdog/Kconfig b/drivers/watchdog/Kconfig
index 0635e72..f441726 100644
--- a/drivers/watchdog/Kconfig
+++ b/drivers/watchdog/Kconfig
@@ -28,6 +28,17 @@ menuconfig WATCHDOG
 
 if WATCHDOG
 
+config WATCHDOG_CORE
+	bool "WatchDog Timer Driver Core"
+	---help---
+	  Say Y here if you want to use the new watchdog timer driver core.
+	  This driver provides a framework for all watchdog timer drivers
+	  and gives them the /dev/watchdog interface (and later also the
+	  sysfs interface).
+
+	  To compile this driver as a module, choose M here: the module will
+	  be called watchdog.
+
 config WATCHDOG_NOWAYOUT
 	bool "Disable watchdog shutdown on close"
 	help
diff --git a/drivers/watchdog/Makefile b/drivers/watchdog/Makefile
index 9eaa212..55bd574 100644
--- a/drivers/watchdog/Makefile
+++ b/drivers/watchdog/Makefile
@@ -2,6 +2,10 @@
 # Makefile for the WatchDog device drivers.
 #
 
+# The WatchDog Timer Driver Core.
+watchdog-objs	+= watchdog_core.o watchdog_dev.o
+obj-$(CONFIG_WATCHDOG_CORE)	+= watchdog.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
new file mode 100644
index 0000000..47fc126
--- /dev/null
+++ b/drivers/watchdog/watchdog_core.c
@@ -0,0 +1,101 @@
+/*
+ *	watchdog_core.c
+ *
+ *	(c) Copyright 2008-2011 Alan Cox <alan@lxorguk.ukuu.org.uk>,
+ *						All Rights Reserved.
+ *
+ *	(c) Copyright 2008-2011 Wim Van Sebroeck <wim@iguana.be>.
+ *
+ *	This source code is part of the generic code that can be used
+ *	by all the watchdog timer drivers.
+ *
+ *	Based on source code of the following authors:
+ *	  Matt Domsch <Matt_Domsch@dell.com>,
+ *	  Rob Radez <rob@osinvestor.com>,
+ *	  Rusty Lynch <rusty@linux.co.intel.com>
+ *	  Satyam Sharma <satyam@infradead.org>
+ *	  Randy Dunlap <randy.dunlap@oracle.com>
+ *
+ *	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.
+ *
+ *	Neither Alan Cox, CymruNet Ltd., Wim Van Sebroeck nor Iguana vzw.
+ *	admit liability nor provide warranty for any of this software.
+ *	This material is provided "AS-IS" and at no charge.
+ */
+
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
+#include <linux/module.h>	/* For EXPORT_SYMBOL/module stuff/... */
+#include <linux/types.h>	/* For standard types */
+#include <linux/errno.h>	/* For the -ENODEV/... values */
+#include <linux/kernel.h>	/* For printk/panic/... */
+#include <linux/watchdog.h>	/* For watchdog specific items */
+#include <linux/init.h>		/* For __init/__exit/... */
+
+#include "watchdog_dev.h"	/* For watchdog_dev_register/... */
+
+/**
+ * watchdog_register_device() - register a watchdog device
+ * @wdd: watchdog device
+ *
+ * Register a watchdog device with the kernel so that the
+ * watchdog timer can be accessed from userspace.
+ *
+ * A zero is returned on success and a negative errno code for
+ * failure.
+ */
+int watchdog_register_device(struct watchdog_device *wdd)
+{
+	int ret;
+
+	if (wdd == NULL || wdd->info == NULL || wdd->ops == NULL)
+		return -EINVAL;
+
+	/* Mandatory operations need to be supported */
+	if (wdd->ops->start == NULL || wdd->ops->stop == NULL)
+		return -EINVAL;
+
+	/*
+	 * Note: now that all watchdog_device data has been verified, we
+	 * will not check this anymore in other functions. If data gets
+	 * corrupted in a later stage then we expect a kernel panic!
+	 */
+
+	/* We only support 1 watchdog device via the /dev/watchdog interface */
+	ret = watchdog_dev_register(wdd);
+	if (ret) {
+		pr_err("error registering /dev/watchdog (err=%d).\n", ret);
+		return ret;
+	}
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(watchdog_register_device);
+
+/**
+ * watchdog_unregister_device() - unregister a watchdog device
+ * @wdd: watchdog device to unregister
+ *
+ * Unregister a watchdog device that was previously successfully
+ * registered with watchdog_register_device().
+ */
+void watchdog_unregister_device(struct watchdog_device *wdd)
+{
+	int ret;
+
+	if (wdd == NULL)
+		return;
+
+	ret = watchdog_dev_unregister(wdd);
+	if (ret)
+		pr_err("error unregistering /dev/watchdog (err=%d).\n", ret);
+}
+EXPORT_SYMBOL_GPL(watchdog_unregister_device);
+
+MODULE_AUTHOR("Alan Cox <alan@lxorguk.ukuu.org.uk>");
+MODULE_AUTHOR("Wim Van Sebroeck <wim@iguana.be>");
+MODULE_DESCRIPTION("WatchDog Timer Driver Core");
+MODULE_LICENSE("GPL");
diff --git a/drivers/watchdog/watchdog_dev.c b/drivers/watchdog/watchdog_dev.c
new file mode 100644
index 0000000..366f49c
--- /dev/null
+++ b/drivers/watchdog/watchdog_dev.c
@@ -0,0 +1,235 @@
+/*
+ *	watchdog_dev.c
+ *
+ *	(c) Copyright 2008-2011 Alan Cox <alan@lxorguk.ukuu.org.uk>,
+ *						All Rights Reserved.
+ *
+ *	(c) Copyright 2008-2011 Wim Van Sebroeck <wim@iguana.be>.
+ *
+ *
+ *	This source code is part of the generic code that can be used
+ *	by all the watchdog timer drivers.
+ *
+ *	This part of the generic code takes care of the following
+ *	misc device: /dev/watchdog.
+ *
+ *	Based on source code of the following authors:
+ *	  Matt Domsch <Matt_Domsch@dell.com>,
+ *	  Rob Radez <rob@osinvestor.com>,
+ *	  Rusty Lynch <rusty@linux.co.intel.com>
+ *	  Satyam Sharma <satyam@infradead.org>
+ *	  Randy Dunlap <randy.dunlap@oracle.com>
+ *
+ *	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.
+ *
+ *	Neither Alan Cox, CymruNet Ltd., Wim Van Sebroeck nor Iguana vzw.
+ *	admit liability nor provide warranty for any of this software.
+ *	This material is provided "AS-IS" and at no charge.
+ */
+
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
+#include <linux/module.h>	/* For module stuff/... */
+#include <linux/types.h>	/* For standard types (like size_t) */
+#include <linux/errno.h>	/* For the -ENODEV/... values */
+#include <linux/kernel.h>	/* For printk/panic/... */
+#include <linux/fs.h>		/* For file operations */
+#include <linux/watchdog.h>	/* For watchdog specific items */
+#include <linux/miscdevice.h>	/* For handling misc devices */
+#include <linux/init.h>		/* For __init/__exit/... */
+#include <linux/uaccess.h>	/* For copy_to_user/put_user/... */
+
+/* make sure we only register one /dev/watchdog device */
+static unsigned long watchdog_dev_busy;
+/* the watchdog device behind /dev/watchdog */
+static struct watchdog_device *wdd;
+
+/*
+ *	watchdog_ping: ping the watchdog.
+ *	@wddev: the watchdog device to ping
+ *
+ *	If the watchdog has no own ping operation then it needs to be
+ *	restarted via the start operation. This wrapper function does
+ *	exactly that.
+ */
+
+static int watchdog_ping(struct watchdog_device *wddev)
+{
+	if (wddev->ops->ping)
+		return wddev->ops->ping(wddev);  /* ping the watchdog */
+	else
+		return wddev->ops->start(wddev); /* restart the watchdog */
+}
+
+/*
+ *	watchdog_write: writes to the watchdog.
+ *	@file: file from VFS
+ *	@data: user address of data
+ *	@len: length of data
+ *	@ppos: pointer to the file offset
+ *
+ *	A write to a watchdog device is defined as a keepalive ping.
+ */
+
+static ssize_t watchdog_write(struct file *file, const char __user *data,
+						size_t len, loff_t *ppos)
+{
+	size_t i;
+	char c;
+
+	if (len == 0)
+		return 0;
+
+	for (i = 0; i != len; i++) {
+		if (get_user(c, data + i))
+			return -EFAULT;
+	}
+
+	/* someone wrote to us, so we send the watchdog a keepalive ping */
+	watchdog_ping(wdd);
+
+	return len;
+}
+
+/*
+ *	watchdog_open: open the /dev/watchdog device.
+ *	@inode: inode of device
+ *	@file: file handle to device
+ *
+ *	When the /dev/watchdog device gets opened, we start the watchdog.
+ *	Watch out: the /dev/watchdog device is single open, so we make sure
+ *	it can only be opened once.
+ */
+
+static int watchdog_open(struct inode *inode, struct file *file)
+{
+	int err = -EBUSY;
+
+	/* the watchdog is single open! */
+	if (test_and_set_bit(WDOG_DEV_OPEN, &wdd->status))
+		return -EBUSY;
+
+	/*
+	 * If the /dev/watchdog device is open, we don't want the module
+	 * to be unloaded.
+	 */
+	if (!try_module_get(wdd->ops->owner))
+		goto out;
+
+	err = wdd->ops->start(wdd);
+	if (err < 0)
+		goto out_mod;
+
+	/* dev/watchdog is a virtual (and thus non-seekable) filesystem */
+	return nonseekable_open(inode, file);
+
+out_mod:
+	module_put(wdd->ops->owner);
+out:
+	clear_bit(WDOG_DEV_OPEN, &wdd->status);
+	return err;
+}
+
+/*
+ *      watchdog_release: release the /dev/watchdog device.
+ *      @inode: inode of device
+ *      @file: file handle to device
+ *
+ *	This is the code for when /dev/watchdog gets closed.
+ */
+
+static int watchdog_release(struct inode *inode, struct file *file)
+{
+	int err;
+
+	err = wdd->ops->stop(wdd);
+	if (err != 0) {
+		pr_crit("%s: watchdog did not stop!\n", wdd->info->identity);
+		watchdog_ping(wdd);
+	}
+
+	/* Allow the owner module to be unloaded again */
+	module_put(wdd->ops->owner);
+
+	/* make sure that /dev/watchdog can be re-opened */
+	clear_bit(WDOG_DEV_OPEN, &wdd->status);
+
+	return 0;
+}
+
+static const struct file_operations watchdog_fops = {
+	.owner		= THIS_MODULE,
+	.write		= watchdog_write,
+	.open		= watchdog_open,
+	.release	= watchdog_release,
+};
+
+static struct miscdevice watchdog_miscdev = {
+	.minor		= WATCHDOG_MINOR,
+	.name		= "watchdog",
+	.fops		= &watchdog_fops,
+};
+
+/*
+ *	watchdog_dev_register:
+ *	@watchdog: watchdog device
+ *
+ *	Register a watchdog device as /dev/watchdog. /dev/watchdog
+ *	is actually a miscdevice and thus we set it up like that.
+ */
+
+int watchdog_dev_register(struct watchdog_device *watchdog)
+{
+	int err;
+
+	/* Only one device can register for /dev/watchdog */
+	if (test_and_set_bit(0, &watchdog_dev_busy)) {
+		pr_err("only one watchdog can use /dev/watchdog.\n");
+		return -EBUSY;
+	}
+
+	wdd = watchdog;
+
+	err = misc_register(&watchdog_miscdev);
+	if (err != 0) {
+		pr_err("%s: cannot register miscdev on minor=%d (err=%d).\n",
+			watchdog->info->identity, WATCHDOG_MINOR, err);
+		goto out;
+	}
+
+	return 0;
+
+out:
+	wdd = NULL;
+	clear_bit(0, &watchdog_dev_busy);
+	return err;
+}
+
+/*
+ *	watchdog_dev_unregister:
+ *	@watchdog: watchdog device
+ *
+ *	Deregister the /dev/watchdog device.
+ */
+
+int watchdog_dev_unregister(struct watchdog_device *watchdog)
+{
+	/* Check that a watchdog device was registered in the past */
+	if (!test_bit(0, &watchdog_dev_busy) || !wdd)
+		return -ENODEV;
+
+	/* We can only unregister the watchdog device that was registered */
+	if (watchdog != wdd) {
+		pr_err("%s: watchdog was not registered as /dev/watchdog.\n",
+			watchdog->info->identity);
+		return -ENODEV;
+	}
+
+	misc_deregister(&watchdog_miscdev);
+	wdd = NULL;
+	clear_bit(0, &watchdog_dev_busy);
+	return 0;
+}
diff --git a/drivers/watchdog/watchdog_dev.h b/drivers/watchdog/watchdog_dev.h
new file mode 100644
index 0000000..bc7612b
--- /dev/null
+++ b/drivers/watchdog/watchdog_dev.h
@@ -0,0 +1,33 @@
+/*
+ *	watchdog_core.h
+ *
+ *	(c) Copyright 2008-2011 Alan Cox <alan@lxorguk.ukuu.org.uk>,
+ *						All Rights Reserved.
+ *
+ *	(c) Copyright 2008-2011 Wim Van Sebroeck <wim@iguana.be>.
+ *
+ *	This source code is part of the generic code that can be used
+ *	by all the watchdog timer drivers.
+ *
+ *	Based on source code of the following authors:
+ *	  Matt Domsch <Matt_Domsch@dell.com>,
+ *	  Rob Radez <rob@osinvestor.com>,
+ *	  Rusty Lynch <rusty@linux.co.intel.com>
+ *	  Satyam Sharma <satyam@infradead.org>
+ *	  Randy Dunlap <randy.dunlap@oracle.com>
+ *
+ *	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.
+ *
+ *	Neither Alan Cox, CymruNet Ltd., Wim Van Sebroeck nor Iguana vzw.
+ *	admit liability nor provide warranty for any of this software.
+ *	This material is provided "AS-IS" and at no charge.
+ */
+
+/*
+ *	Functions/procedures to be called by the core
+ */
+int watchdog_dev_register(struct watchdog_device *);
+int watchdog_dev_unregister(struct watchdog_device *);
diff --git a/include/linux/watchdog.h b/include/linux/watchdog.h
index 011bcfe..5ab31bf 100644
--- a/include/linux/watchdog.h
+++ b/include/linux/watchdog.h
@@ -59,6 +59,67 @@ struct watchdog_info {
 #define WATCHDOG_NOWAYOUT	0
 #endif
 
+struct watchdog_ops;
+struct watchdog_device;
+
+/** struct watchdog_ops - The watchdog-devices operations
+ *
+ * @owner:	The module owner.
+ * @start:	The routine for starting the watchdog device.
+ * @stop:	The routine for stopping the watchdog device.
+ * @ping:	The routine that sends a keepalive ping to the watchdog device.
+ *
+ * The watchdog_ops structure contains a list of low-level operations
+ * that control a watchdog device. It also contains the module that owns
+ * these operations. The start and stop function are mandatory, all other
+ * functions are optonal.
+ */
+struct watchdog_ops {
+	struct module *owner;
+	/* mandatory operations */
+	int (*start)(struct watchdog_device *);
+	int (*stop)(struct watchdog_device *);
+	/* optional operations */
+	int (*ping)(struct watchdog_device *);
+};
+
+/** struct watchdog_device - The structure that defines a watchdog device
+ *
+ * @info:	Pointer to a watchdog_info structure.
+ * @ops:	Pointer to the list of watchdog operations.
+ * @driver-data:Pointer to the drivers private data.
+ * @status:	Field that contains the devices internal status bits.
+ *
+ * The watchdog_device structure contains all information about a
+ * watchdog timer device.
+ *
+ * The driver-data field may not be accessed directly. It must be accessed
+ * via the watchdog_set_drvdata and watchdog_get_drvdata helpers.
+ */
+struct watchdog_device {
+	const struct watchdog_info *info;
+	const struct watchdog_ops *ops;
+	void *driver_data;
+	unsigned long status;
+/* Bit numbers for status flags */
+#define WDOG_DEV_OPEN		1	/* Opened via /dev/watchdog ? */
+};
+
+/* Use the following functions to manipulate watchdog driver specific data */
+static inline void watchdog_set_drvdata(struct watchdog_device *wdd, void *data)
+{
+	wdd->driver_data = data;
+}
+
+static inline void *watchdog_get_drvdata(struct watchdog_device *wdd)
+{
+	return wdd->driver_data;
+}
+
+/* drivers/watchdog/core/watchdog_core.c */
+extern int watchdog_register_device(struct watchdog_device *);
+extern void watchdog_unregister_device(struct watchdog_device *);
+
 #endif	/* __KERNEL__ */
 
 #endif  /* ifndef _LINUX_WATCHDOG_H */
-- 
1.7.6

[PATCH 2/9] watchdog: WatchDog Timer Driver Core - Add basic ioctl functionality

[Wim Van Sebroeck]

This part add's the basic ioctl functionality to the
WatchDog Timer Driver Core framework. The supported
ioctl call's are:
	WDIOC_GETSUPPORT
	WDIOC_GETSTATUS
	WDIOC_GETBOOTSTATUS

Signed-off-by: Alan Cox <alan@lxorguk.ukuu.org.uk>
Signed-off-by: Wim Van Sebroeck <wim@iguana.be>
Acked-by: Arnd Bergmann <arnd@arndb.de>
Acked-by: Wolfram Sang <w.sang@pengutronix.de>
---
 Documentation/watchdog/watchdog-kernel-api.txt |    6 ++++
 drivers/watchdog/watchdog_dev.c                |   32 ++++++++++++++++++++++++
 include/linux/watchdog.h                       |    4 +++
 3 files changed, 42 insertions(+), 0 deletions(-)

diff --git a/Documentation/watchdog/watchdog-kernel-api.txt b/Documentation/watchdog/watchdog-kernel-api.txt
index 3db67e7..2bdc6dc 100644
--- a/Documentation/watchdog/watchdog-kernel-api.txt
+++ b/Documentation/watchdog/watchdog-kernel-api.txt
@@ -41,6 +41,7 @@ The watchdog device structure looks like this:
 struct watchdog_device {
 	const struct watchdog_info *info;
 	const struct watchdog_ops *ops;
+	unsigned int bootstatus;
 	void *driver_data;
 	unsigned long status;
 };
@@ -49,6 +50,8 @@ It contains following fields:
 * info: a pointer to a watchdog_info structure. This structure gives some
   additional information about the watchdog timer itself. (Like it's unique name)
 * ops: a pointer to the list of watchdog operations that the watchdog supports.
+* bootstatus: status of the device after booting (reported with watchdog
+  WDIOF_* status bits).
 * driver_data: a pointer to the drivers private data of a watchdog device.
   This data should only be accessed via the watchdog_set_drvadata and
   watchdog_get_drvdata routines.
@@ -65,6 +68,7 @@ struct watchdog_ops {
 	int (*stop)(struct watchdog_device *);
 	/* optional operations */
 	int (*ping)(struct watchdog_device *);
+	unsigned int (*status)(struct watchdog_device *);
 };
 
 It is important that you first define the module owner of the watchdog timer
@@ -97,6 +101,8 @@ they are supported. These optional routines/operations are:
   the watchdog timer driver core does: to send a keepalive ping to the watchdog
   timer hardware it will either use the ping operation (when available) or the
   start operation (when the ping operation is not available).
+* status: this routine checks the status of the watchdog timer device. The
+  status of the device is reported with watchdog WDIOF_* status flags/bits.
 
 The status bits should (preferably) be set with the set_bit and clear_bit alike
 bit-operations. The status bits that are defined are:
diff --git a/drivers/watchdog/watchdog_dev.c b/drivers/watchdog/watchdog_dev.c
index 366f49c..00a6112 100644
--- a/drivers/watchdog/watchdog_dev.c
+++ b/drivers/watchdog/watchdog_dev.c
@@ -95,6 +95,37 @@ static ssize_t watchdog_write(struct file *file, const char __user *data,
 }
 
 /*
+ *	watchdog_ioctl: handle the different ioctl's for the watchdog device.
+ *	@file: file handle to the device
+ *	@cmd: watchdog command
+ *	@arg: argument pointer
+ *
+ *	The watchdog API defines a common set of functions for all watchdogs
+ *	according to their available features.
+ */
+
+static long watchdog_ioctl(struct file *file, unsigned int cmd,
+							unsigned long arg)
+{
+	void __user *argp = (void __user *)arg;
+	int __user *p = argp;
+	unsigned int val;
+
+	switch (cmd) {
+	case WDIOC_GETSUPPORT:
+		return copy_to_user(argp, wdd->info,
+			sizeof(struct watchdog_info)) ? -EFAULT : 0;
+	case WDIOC_GETSTATUS:
+		val = wdd->ops->status ? wdd->ops->status(wdd) : 0;
+		return put_user(val, p);
+	case WDIOC_GETBOOTSTATUS:
+		return put_user(wdd->bootstatus, p);
+	default:
+		return -ENOTTY;
+	}
+}
+
+/*
  *	watchdog_open: open the /dev/watchdog device.
  *	@inode: inode of device
  *	@file: file handle to device
@@ -163,6 +194,7 @@ static int watchdog_release(struct inode *inode, struct file *file)
 static const struct file_operations watchdog_fops = {
 	.owner		= THIS_MODULE,
 	.write		= watchdog_write,
+	.unlocked_ioctl	= watchdog_ioctl,
 	.open		= watchdog_open,
 	.release	= watchdog_release,
 };
diff --git a/include/linux/watchdog.h b/include/linux/watchdog.h
index 5ab31bf..29ff808 100644
--- a/include/linux/watchdog.h
+++ b/include/linux/watchdog.h
@@ -68,6 +68,7 @@ struct watchdog_device;
  * @start:	The routine for starting the watchdog device.
  * @stop:	The routine for stopping the watchdog device.
  * @ping:	The routine that sends a keepalive ping to the watchdog device.
+ * @status:	The routine that shows the status of the watchdog device.
  *
  * The watchdog_ops structure contains a list of low-level operations
  * that control a watchdog device. It also contains the module that owns
@@ -81,12 +82,14 @@ struct watchdog_ops {
 	int (*stop)(struct watchdog_device *);
 	/* optional operations */
 	int (*ping)(struct watchdog_device *);
+	unsigned int (*status)(struct watchdog_device *);
 };
 
 /** struct watchdog_device - The structure that defines a watchdog device
  *
  * @info:	Pointer to a watchdog_info structure.
  * @ops:	Pointer to the list of watchdog operations.
+ * @bootstatus:	Status of the watchdog device at boot.
  * @driver-data:Pointer to the drivers private data.
  * @status:	Field that contains the devices internal status bits.
  *
@@ -99,6 +102,7 @@ struct watchdog_ops {
 struct watchdog_device {
 	const struct watchdog_info *info;
 	const struct watchdog_ops *ops;
+	unsigned int bootstatus;
 	void *driver_data;
 	unsigned long status;
 /* Bit numbers for status flags */
-- 
1.7.6

[PATCH 3/9] watchdog: WatchDog Timer Driver Core - Add WDIOC_KEEPALIVE ioctl

[Wim Van Sebroeck]

This part add's the WDIOC_KEEPALIVE ioctl functionality to the
WatchDog Timer Driver Core framework. Please note that the
WDIOF_KEEPALIVEPING bit has to be set in the watchdog_info
options field.

Signed-off-by: Alan Cox <alan@lxorguk.ukuu.org.uk>
Signed-off-by: Wim Van Sebroeck <wim@iguana.be>
Acked-by: Arnd Bergmann <arnd@arndb.de>
Acked-by: Wolfram Sang <w.sang@pengutronix.de>
---
 Documentation/watchdog/watchdog-kernel-api.txt |    3 +++
 drivers/watchdog/watchdog_dev.c                |    5 +++++
 2 files changed, 8 insertions(+), 0 deletions(-)

diff --git a/Documentation/watchdog/watchdog-kernel-api.txt b/Documentation/watchdog/watchdog-kernel-api.txt
index 2bdc6dc..abbcf2c 100644
--- a/Documentation/watchdog/watchdog-kernel-api.txt
+++ b/Documentation/watchdog/watchdog-kernel-api.txt
@@ -101,6 +101,9 @@ they are supported. These optional routines/operations are:
   the watchdog timer driver core does: to send a keepalive ping to the watchdog
   timer hardware it will either use the ping operation (when available) or the
   start operation (when the ping operation is not available).
+  (Note: the WDIOC_KEEPALIVE ioctl call will only be active when the
+  WDIOF_KEEPALIVEPING bit has been set in the option field on the watchdog's
+  info structure).
 * status: this routine checks the status of the watchdog timer device. The
   status of the device is reported with watchdog WDIOF_* status flags/bits.
 
diff --git a/drivers/watchdog/watchdog_dev.c b/drivers/watchdog/watchdog_dev.c
index 00a6112..2fb4cec 100644
--- a/drivers/watchdog/watchdog_dev.c
+++ b/drivers/watchdog/watchdog_dev.c
@@ -120,6 +120,11 @@ static long watchdog_ioctl(struct file *file, unsigned int cmd,
 		return put_user(val, p);
 	case WDIOC_GETBOOTSTATUS:
 		return put_user(wdd->bootstatus, p);
+	case WDIOC_KEEPALIVE:
+		if (!(wdd->info->options & WDIOF_KEEPALIVEPING))
+			return -EOPNOTSUPP;
+		watchdog_ping(wdd);
+		return 0;
 	default:
 		return -ENOTTY;
 	}
-- 
1.7.6

[PATCH 4/9] watchdog: WatchDog Timer Driver Core - Add WDIOC_SETOPTIONS ioctl

[Wim Van Sebroeck]

This part add's the WDIOC_SETOPTIONS ioctl functionality
to the WatchDog Timer Driver Core framework.

Signed-off-by: Alan Cox <alan@lxorguk.ukuu.org.uk>
Signed-off-by: Wim Van Sebroeck <wim@iguana.be>
Acked-by: Arnd Bergmann <arnd@arndb.de>
Acked-by: Wolfram Sang <w.sang@pengutronix.de>
---
 Documentation/watchdog/watchdog-kernel-api.txt |    9 ++-
 drivers/watchdog/watchdog_dev.c                |   79 +++++++++++++++++++++--
 include/linux/watchdog.h                       |    1 +
 3 files changed, 80 insertions(+), 9 deletions(-)

diff --git a/Documentation/watchdog/watchdog-kernel-api.txt b/Documentation/watchdog/watchdog-kernel-api.txt
index abbcf2c..429f81b 100644
--- a/Documentation/watchdog/watchdog-kernel-api.txt
+++ b/Documentation/watchdog/watchdog-kernel-api.txt
@@ -56,8 +56,9 @@ It contains following fields:
   This data should only be accessed via the watchdog_set_drvadata and
   watchdog_get_drvdata routines.
 * status: this field contains a number of status bits that give extra
-  information about the status of the device (Like: is the device opened via
-  the /dev/watchdog interface or not, ...).
+  information about the status of the device (Like: is the watchdog timer
+  running/active, is the device opened via the /dev/watchdog interface or not,
+  ...).
 
 The list of watchdog operations is defined as:
 
@@ -109,6 +110,10 @@ they are supported. These optional routines/operations are:
 
 The status bits should (preferably) be set with the set_bit and clear_bit alike
 bit-operations. The status bits that are defined are:
+* WDOG_ACTIVE: this status bit indicates whether or not a watchdog timer device
+  is active or not. When the watchdog is active after booting, then you should
+  set this status bit (Note: when you register the watchdog timer device with
+  this bit set, then opening /dev/watchdog will skip the start operation)
 * WDOG_DEV_OPEN: this status bit shows whether or not the watchdog device
   was opened via /dev/watchdog.
   (This bit should only be used by the WatchDog Timer Driver Core).
diff --git a/drivers/watchdog/watchdog_dev.c b/drivers/watchdog/watchdog_dev.c
index 2fb4cec..9f5550e 100644
--- a/drivers/watchdog/watchdog_dev.c
+++ b/drivers/watchdog/watchdog_dev.c
@@ -54,14 +54,64 @@ static struct watchdog_device *wdd;
  *	If the watchdog has no own ping operation then it needs to be
  *	restarted via the start operation. This wrapper function does
  *	exactly that.
+ *	We only ping when the watchdog device is running.
  */
 
 static int watchdog_ping(struct watchdog_device *wddev)
 {
-	if (wddev->ops->ping)
-		return wddev->ops->ping(wddev);  /* ping the watchdog */
-	else
-		return wddev->ops->start(wddev); /* restart the watchdog */
+	if (test_bit(WDOG_ACTIVE, &wdd->status)) {
+		if (wddev->ops->ping)
+			return wddev->ops->ping(wddev);  /* ping the watchdog */
+		else
+			return wddev->ops->start(wddev); /* restart watchdog */
+	}
+	return 0;
+}
+
+/*
+ *	watchdog_start: wrapper to start the watchdog.
+ *	@wddev: the watchdog device to start
+ *
+ *	Start the watchdog if it is not active and mark it active.
+ *	This function returns zero on success or a negative errno code for
+ *	failure.
+ */
+
+static int watchdog_start(struct watchdog_device *wddev)
+{
+	int err;
+
+	if (!test_bit(WDOG_ACTIVE, &wdd->status)) {
+		err = wddev->ops->start(wddev);
+		if (err < 0)
+			return err;
+
+		set_bit(WDOG_ACTIVE, &wdd->status);
+	}
+	return 0;
+}
+
+/*
+ *	watchdog_stop: wrapper to stop the watchdog.
+ *	@wddev: the watchdog device to stop
+ *
+ *	Stop the watchdog if it is still active and unmark it active.
+ *	This function returns zero on success or a negative errno code for
+ *	failure.
+ */
+
+static int watchdog_stop(struct watchdog_device *wddev)
+{
+	int err;
+
+	if (test_bit(WDOG_ACTIVE, &wdd->status)) {
+		err = wddev->ops->stop(wddev);
+		if (err < 0)
+			return err;
+
+		clear_bit(WDOG_ACTIVE, &wdd->status);
+	}
+	return 0;
 }
 
 /*
@@ -110,6 +160,7 @@ static long watchdog_ioctl(struct file *file, unsigned int cmd,
 	void __user *argp = (void __user *)arg;
 	int __user *p = argp;
 	unsigned int val;
+	int err;
 
 	switch (cmd) {
 	case WDIOC_GETSUPPORT:
@@ -120,6 +171,20 @@ static long watchdog_ioctl(struct file *file, unsigned int cmd,
 		return put_user(val, p);
 	case WDIOC_GETBOOTSTATUS:
 		return put_user(wdd->bootstatus, p);
+	case WDIOC_SETOPTIONS:
+		if (get_user(val, p))
+			return -EFAULT;
+		if (val & WDIOS_DISABLECARD) {
+			err = watchdog_stop(wdd);
+			if (err < 0)
+				return err;
+		}
+		if (val & WDIOS_ENABLECARD) {
+			err = watchdog_start(wdd);
+			if (err < 0)
+				return err;
+		}
+		return 0;
 	case WDIOC_KEEPALIVE:
 		if (!(wdd->info->options & WDIOF_KEEPALIVEPING))
 			return -EOPNOTSUPP;
@@ -155,7 +220,7 @@ static int watchdog_open(struct inode *inode, struct file *file)
 	if (!try_module_get(wdd->ops->owner))
 		goto out;
 
-	err = wdd->ops->start(wdd);
+	err = watchdog_start(wdd);
 	if (err < 0)
 		goto out_mod;
 
@@ -181,8 +246,8 @@ static int watchdog_release(struct inode *inode, struct file *file)
 {
 	int err;
 
-	err = wdd->ops->stop(wdd);
-	if (err != 0) {
+	err = watchdog_stop(wdd);
+	if (err < 0) {
 		pr_crit("%s: watchdog did not stop!\n", wdd->info->identity);
 		watchdog_ping(wdd);
 	}
diff --git a/include/linux/watchdog.h b/include/linux/watchdog.h
index 29ff808..db46fe8 100644
--- a/include/linux/watchdog.h
+++ b/include/linux/watchdog.h
@@ -106,6 +106,7 @@ struct watchdog_device {
 	void *driver_data;
 	unsigned long status;
 /* Bit numbers for status flags */
+#define WDOG_ACTIVE		0	/* Is the watchdog running/active */
 #define WDOG_DEV_OPEN		1	/* Opened via /dev/watchdog ? */
 };
 
-- 
1.7.6

[PATCH 5/9] watchdog: WatchDog Timer Driver Core - Add WDIOC_SETTIMEOUT and WDIOC_GETTIMEOUT ioctl

[Wim Van Sebroeck]

This part add's the WDIOC_SETTIMEOUT and WDIOC_GETTIMEOUT ioctl
functionality to the WatchDog Timer Driver Core framework.

Signed-off-by: Alan Cox <alan@lxorguk.ukuu.org.uk>
Signed-off-by: Wim Van Sebroeck <wim@iguana.be>
Acked-by: Arnd Bergmann <arnd@arndb.de>
Acked-by: Wolfram Sang <w.sang@pengutronix.de>
---
 Documentation/watchdog/watchdog-kernel-api.txt |   10 ++++++++++
 drivers/watchdog/watchdog_dev.c                |   20 ++++++++++++++++++++
 include/linux/watchdog.h                       |    4 ++++
 3 files changed, 34 insertions(+), 0 deletions(-)

diff --git a/Documentation/watchdog/watchdog-kernel-api.txt b/Documentation/watchdog/watchdog-kernel-api.txt
index 429f81b..acdee39 100644
--- a/Documentation/watchdog/watchdog-kernel-api.txt
+++ b/Documentation/watchdog/watchdog-kernel-api.txt
@@ -42,6 +42,7 @@ struct watchdog_device {
 	const struct watchdog_info *info;
 	const struct watchdog_ops *ops;
 	unsigned int bootstatus;
+	unsigned int timeout;
 	void *driver_data;
 	unsigned long status;
 };
@@ -50,6 +51,7 @@ It contains following fields:
 * info: a pointer to a watchdog_info structure. This structure gives some
   additional information about the watchdog timer itself. (Like it's unique name)
 * ops: a pointer to the list of watchdog operations that the watchdog supports.
+* timeout: the watchdog timer's timeout value (in seconds).
 * bootstatus: status of the device after booting (reported with watchdog
   WDIOF_* status bits).
 * driver_data: a pointer to the drivers private data of a watchdog device.
@@ -70,6 +72,7 @@ struct watchdog_ops {
 	/* optional operations */
 	int (*ping)(struct watchdog_device *);
 	unsigned int (*status)(struct watchdog_device *);
+	int (*set_timeout)(struct watchdog_device *, unsigned int);
 };
 
 It is important that you first define the module owner of the watchdog timer
@@ -107,6 +110,13 @@ they are supported. These optional routines/operations are:
   info structure).
 * status: this routine checks the status of the watchdog timer device. The
   status of the device is reported with watchdog WDIOF_* status flags/bits.
+* set_timeout: this routine checks and changes the timeout of the watchdog
+  timer device. It returns 0 on success, -EINVAL for "parameter out of range"
+  and -EIO for "could not write value to the watchdog". On success the timeout
+  value of the watchdog_device will be changed to the value that was just used
+  to re-program the watchdog timer device.
+  (Note: the WDIOF_SETTIMEOUT needs to be set in the options field of the
+  watchdog's info structure).
 
 The status bits should (preferably) be set with the set_bit and clear_bit alike
 bit-operations. The status bits that are defined are:
diff --git a/drivers/watchdog/watchdog_dev.c b/drivers/watchdog/watchdog_dev.c
index 9f5550e..2c0289d 100644
--- a/drivers/watchdog/watchdog_dev.c
+++ b/drivers/watchdog/watchdog_dev.c
@@ -190,6 +190,26 @@ static long watchdog_ioctl(struct file *file, unsigned int cmd,
 			return -EOPNOTSUPP;
 		watchdog_ping(wdd);
 		return 0;
+	case WDIOC_SETTIMEOUT:
+		if ((wdd->ops->set_timeout == NULL) ||
+		    !(wdd->info->options & WDIOF_SETTIMEOUT))
+			return -EOPNOTSUPP;
+		if (get_user(val, p))
+			return -EFAULT;
+		err = wdd->ops->set_timeout(wdd, val);
+		if (err < 0)
+			return err;
+		wdd->timeout = val;
+		/* If the watchdog is active then we send a keepalive ping
+		 * to make sure that the watchdog keep's running (and if
+		 * possible that it takes the new timeout) */
+		watchdog_ping(wdd);
+		/* Fall */
+	case WDIOC_GETTIMEOUT:
+		/* timeout == 0 means that we don't know the timeout */
+		if (wdd->timeout == 0)
+			return -EOPNOTSUPP;
+		return put_user(wdd->timeout, p);
 	default:
 		return -ENOTTY;
 	}
diff --git a/include/linux/watchdog.h b/include/linux/watchdog.h
index db46fe8..9f33efe 100644
--- a/include/linux/watchdog.h
+++ b/include/linux/watchdog.h
@@ -69,6 +69,7 @@ struct watchdog_device;
  * @stop:	The routine for stopping the watchdog device.
  * @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.
  *
  * The watchdog_ops structure contains a list of low-level operations
  * that control a watchdog device. It also contains the module that owns
@@ -83,6 +84,7 @@ struct watchdog_ops {
 	/* optional operations */
 	int (*ping)(struct watchdog_device *);
 	unsigned int (*status)(struct watchdog_device *);
+	int (*set_timeout)(struct watchdog_device *, unsigned int);
 };
 
 /** struct watchdog_device - The structure that defines a watchdog device
@@ -90,6 +92,7 @@ struct watchdog_ops {
  * @info:	Pointer to a watchdog_info structure.
  * @ops:	Pointer to the list of watchdog operations.
  * @bootstatus:	Status of the watchdog device at boot.
+ * @timeout:	The watchdog devices timeout value.
  * @driver-data:Pointer to the drivers private data.
  * @status:	Field that contains the devices internal status bits.
  *
@@ -103,6 +106,7 @@ struct watchdog_device {
 	const struct watchdog_info *info;
 	const struct watchdog_ops *ops;
 	unsigned int bootstatus;
+	unsigned int timeout;
 	void *driver_data;
 	unsigned long status;
 /* Bit numbers for status flags */
-- 
1.7.6

[PATCH 6/9] watchdog: WatchDog Timer Driver Core - Add Magic Close feature

[Wim Van Sebroeck]

Add support for the Magic Close feature to the
WatchDog Timer Driver Core framework.

Signed-off-by: Alan Cox <alan@lxorguk.ukuu.org.uk>
Signed-off-by: Wim Van Sebroeck <wim@iguana.be>
Acked-by: Arnd Bergmann <arnd@arndb.de>
Acked-by: Wolfram Sang <w.sang@pengutronix.de>
---
 Documentation/watchdog/watchdog-kernel-api.txt |    7 ++++++
 drivers/watchdog/watchdog_dev.c                |   27 +++++++++++++++++++++--
 include/linux/watchdog.h                       |    1 +
 3 files changed, 32 insertions(+), 3 deletions(-)

diff --git a/Documentation/watchdog/watchdog-kernel-api.txt b/Documentation/watchdog/watchdog-kernel-api.txt
index acdee39..41d5526 100644
--- a/Documentation/watchdog/watchdog-kernel-api.txt
+++ b/Documentation/watchdog/watchdog-kernel-api.txt
@@ -127,6 +127,13 @@ bit-operations. The status bits that are defined are:
 * WDOG_DEV_OPEN: this status bit shows whether or not the watchdog device
   was opened via /dev/watchdog.
   (This bit should only be used by the WatchDog Timer Driver Core).
+* WDOG_ALLOW_RELEASE: this bit stores whether or not the magic close character
+  has been sent (so that we can support the magic close feature).
+  (This bit should only be used by the WatchDog Timer Driver Core).
+
+Note: The WatchDog Timer Driver Core supports the magic close feature. To use
+the magic close feature you must set the WDIOF_MAGICCLOSE bit in the options
+field of the watchdog's info structure.
 
 To get or set driver specific data the following two helper functions should be
 used:
diff --git a/drivers/watchdog/watchdog_dev.c b/drivers/watchdog/watchdog_dev.c
index 2c0289d..db40c6c 100644
--- a/drivers/watchdog/watchdog_dev.c
+++ b/drivers/watchdog/watchdog_dev.c
@@ -122,6 +122,8 @@ static int watchdog_stop(struct watchdog_device *wddev)
  *	@ppos: pointer to the file offset
  *
  *	A write to a watchdog device is defined as a keepalive ping.
+ *	Writing the magic 'V' sequence allows the next close to turn
+ *	off the watchdog.
  */
 
 static ssize_t watchdog_write(struct file *file, const char __user *data,
@@ -133,9 +135,18 @@ static ssize_t watchdog_write(struct file *file, const char __user *data,
 	if (len == 0)
 		return 0;
 
+	/*
+	 * Note: just in case someone wrote the magic character
+	 * five months ago...
+	 */
+	clear_bit(WDOG_ALLOW_RELEASE, &wdd->status);
+
+	/* scan to see whether or not we got the magic character */
 	for (i = 0; i != len; i++) {
 		if (get_user(c, data + i))
 			return -EFAULT;
+		if (c == 'V')
+			set_bit(WDOG_ALLOW_RELEASE, &wdd->status);
 	}
 
 	/* someone wrote to us, so we send the watchdog a keepalive ping */
@@ -259,14 +270,24 @@ out:
  *      @inode: inode of device
  *      @file: file handle to device
  *
- *	This is the code for when /dev/watchdog gets closed.
+ *	This is the code for when /dev/watchdog gets closed. We will only
+ *	stop the watchdog when we have received the magic char, else the
+ *	watchdog will keep running.
  */
 
 static int watchdog_release(struct inode *inode, struct file *file)
 {
-	int err;
+	int err = -EBUSY;
+
+	/*
+	 * We only stop the watchdog if we received the magic character
+	 * or if WDIOF_MAGICCLOSE is not set
+	 */
+	if (test_and_clear_bit(WDOG_ALLOW_RELEASE, &wdd->status) ||
+	    !(wdd->info->options & WDIOF_MAGICCLOSE))
+		err = watchdog_stop(wdd);
 
-	err = watchdog_stop(wdd);
+	/* If the watchdog was not stopped, send a keepalive ping */
 	if (err < 0) {
 		pr_crit("%s: watchdog did not stop!\n", wdd->info->identity);
 		watchdog_ping(wdd);
diff --git a/include/linux/watchdog.h b/include/linux/watchdog.h
index 9f33efe..e9881ca 100644
--- a/include/linux/watchdog.h
+++ b/include/linux/watchdog.h
@@ -112,6 +112,7 @@ struct watchdog_device {
 /* Bit numbers for status flags */
 #define WDOG_ACTIVE		0	/* Is the watchdog running/active */
 #define WDOG_DEV_OPEN		1	/* Opened via /dev/watchdog ? */
+#define WDOG_ALLOW_RELEASE	2	/* Did we receive the magic char ? */
 };
 
 /* Use the following functions to manipulate watchdog driver specific data */
-- 
1.7.6

[PATCH 7/9] watchdog: WatchDog Timer Driver Core - Add nowayout feature

[Wim Van Sebroeck]

Add support for the nowayout feature to the
WatchDog Timer Driver Core framework.
This feature prevents the watchdog timer from being
stopped.

Signed-off-by: Alan Cox <alan@lxorguk.ukuu.org.uk>
Signed-off-by: Wim Van Sebroeck <wim@iguana.be>
Acked-by: Arnd Bergmann <arnd@arndb.de>
Acked-by: Wolfram Sang <w.sang@pengutronix.de>
---
 Documentation/watchdog/watchdog-kernel-api.txt |   13 ++++++++-----
 drivers/watchdog/watchdog_dev.c                |   18 +++++++++++++-----
 include/linux/watchdog.h                       |    1 +
 3 files changed, 22 insertions(+), 10 deletions(-)

diff --git a/Documentation/watchdog/watchdog-kernel-api.txt b/Documentation/watchdog/watchdog-kernel-api.txt
index 41d5526..785fa0c 100644
--- a/Documentation/watchdog/watchdog-kernel-api.txt
+++ b/Documentation/watchdog/watchdog-kernel-api.txt
@@ -59,8 +59,8 @@ It contains following fields:
   watchdog_get_drvdata routines.
 * status: this field contains a number of status bits that give extra
   information about the status of the device (Like: is the watchdog timer
-  running/active, is the device opened via the /dev/watchdog interface or not,
-  ...).
+  running/active, is the nowayout bit set, is the device opened via
+  the /dev/watchdog interface or not, ...).
 
 The list of watchdog operations is defined as:
 
@@ -130,10 +130,13 @@ bit-operations. The status bits that are defined are:
 * WDOG_ALLOW_RELEASE: this bit stores whether or not the magic close character
   has been sent (so that we can support the magic close feature).
   (This bit should only be used by the WatchDog Timer Driver Core).
+* WDOG_NO_WAY_OUT: this bit stores the nowayout setting for the watchdog.
+  If this bit is set then the watchdog timer will not be able to stop.
 
-Note: The WatchDog Timer Driver Core supports the magic close feature. To use
-the magic close feature you must set the WDIOF_MAGICCLOSE bit in the options
-field of the watchdog's info structure.
+Note: The WatchDog Timer Driver Core supports the magic close feature and
+the nowayout feature. To use the magic close feature you must set the
+WDIOF_MAGICCLOSE bit in the options field of the watchdog's info structure.
+The nowayout feature will overrule the magic close feature.
 
 To get or set driver specific data the following two helper functions should be
 used:
diff --git a/drivers/watchdog/watchdog_dev.c b/drivers/watchdog/watchdog_dev.c
index db40c6c..ac20f92 100644
--- a/drivers/watchdog/watchdog_dev.c
+++ b/drivers/watchdog/watchdog_dev.c
@@ -98,11 +98,18 @@ static int watchdog_start(struct watchdog_device *wddev)
  *	Stop the watchdog if it is still active and unmark it active.
  *	This function returns zero on success or a negative errno code for
  *	failure.
+ *	If the 'nowayout' feature was set, the watchdog cannot be stopped.
  */
 
 static int watchdog_stop(struct watchdog_device *wddev)
 {
-	int err;
+	int err = -EBUSY;
+
+	if (test_bit(WDOG_NO_WAY_OUT, &wdd->status)) {
+		pr_info("%s: nowayout prevents watchdog to be stopped!\n",
+							wdd->info->identity);
+		return err;
+	}
 
 	if (test_bit(WDOG_ACTIVE, &wdd->status)) {
 		err = wddev->ops->stop(wddev);
@@ -123,7 +130,7 @@ static int watchdog_stop(struct watchdog_device *wddev)
  *
  *	A write to a watchdog device is defined as a keepalive ping.
  *	Writing the magic 'V' sequence allows the next close to turn
- *	off the watchdog.
+ *	off the watchdog (if 'nowayout' is not set).
  */
 
 static ssize_t watchdog_write(struct file *file, const char __user *data,
@@ -271,8 +278,8 @@ out:
  *      @file: file handle to device
  *
  *	This is the code for when /dev/watchdog gets closed. We will only
- *	stop the watchdog when we have received the magic char, else the
- *	watchdog will keep running.
+ *	stop the watchdog when we have received the magic char (and nowayout
+ *	was not set), else the watchdog will keep running.
  */
 
 static int watchdog_release(struct inode *inode, struct file *file)
@@ -281,7 +288,8 @@ static int watchdog_release(struct inode *inode, struct file *file)
 
 	/*
 	 * We only stop the watchdog if we received the magic character
-	 * or if WDIOF_MAGICCLOSE is not set
+	 * or if WDIOF_MAGICCLOSE is not set. If nowayout was set then
+	 * watchdog_stop will fail.
 	 */
 	if (test_and_clear_bit(WDOG_ALLOW_RELEASE, &wdd->status) ||
 	    !(wdd->info->options & WDIOF_MAGICCLOSE))
diff --git a/include/linux/watchdog.h b/include/linux/watchdog.h
index e9881ca..f719883 100644
--- a/include/linux/watchdog.h
+++ b/include/linux/watchdog.h
@@ -113,6 +113,7 @@ struct watchdog_device {
 #define WDOG_ACTIVE		0	/* Is the watchdog running/active */
 #define WDOG_DEV_OPEN		1	/* Opened via /dev/watchdog ? */
 #define WDOG_ALLOW_RELEASE	2	/* Did we receive the magic char ? */
+#define WDOG_NO_WAY_OUT		3	/* Is 'nowayout' feature set ? */
 };
 
 /* Use the following functions to manipulate watchdog driver specific data */
-- 
1.7.6

[PATCH 8/9] watchdog: WatchDog Timer Driver Core - Add ioctl call

[Wim Van Sebroeck]

Add support for extra ioctl calls by adding a
ioctl watchdog operation. This operation will be
called before we do our own handling of ioctl
commands. This way we can override the internal
ioctl command handling and we can also add
extra ioctl commands. The ioctl watchdog operation
should return the appropriate error codes or
-ENOIOCTLCMD if the ioctl command should be handled
through the internal ioctl handling of the framework.

Signed-off-by: Alan Cox <alan@lxorguk.ukuu.org.uk>
Signed-off-by: Wim Van Sebroeck <wim@iguana.be>
Acked-by: Arnd Bergmann <arnd@arndb.de>
Acked-by: Wolfram Sang <w.sang@pengutronix.de>
---
 Documentation/watchdog/watchdog-kernel-api.txt |    5 +++++
 drivers/watchdog/watchdog_dev.c                |    6 ++++++
 include/linux/watchdog.h                       |    2 ++
 3 files changed, 13 insertions(+), 0 deletions(-)

diff --git a/Documentation/watchdog/watchdog-kernel-api.txt b/Documentation/watchdog/watchdog-kernel-api.txt
index 785fa0c..829955b 100644
--- a/Documentation/watchdog/watchdog-kernel-api.txt
+++ b/Documentation/watchdog/watchdog-kernel-api.txt
@@ -73,6 +73,7 @@ struct watchdog_ops {
 	int (*ping)(struct watchdog_device *);
 	unsigned int (*status)(struct watchdog_device *);
 	int (*set_timeout)(struct watchdog_device *, unsigned int);
+	long (*ioctl)(struct watchdog_device *, unsigned int, unsigned long);
 };
 
 It is important that you first define the module owner of the watchdog timer
@@ -117,6 +118,10 @@ they are supported. These optional routines/operations are:
   to re-program the watchdog timer device.
   (Note: the WDIOF_SETTIMEOUT needs to be set in the options field of the
   watchdog's info structure).
+* ioctl: if this routine is present then it will be called first before we do
+  our own internal ioctl call handling. This routine should return -ENOIOCTLCMD
+  if a command is not supported. The parameters that are passed to the ioctl
+  call are: watchdog_device, cmd and arg.
 
 The status bits should (preferably) be set with the set_bit and clear_bit alike
 bit-operations. The status bits that are defined are:
diff --git a/drivers/watchdog/watchdog_dev.c b/drivers/watchdog/watchdog_dev.c
index ac20f92..e7134a5 100644
--- a/drivers/watchdog/watchdog_dev.c
+++ b/drivers/watchdog/watchdog_dev.c
@@ -180,6 +180,12 @@ static long watchdog_ioctl(struct file *file, unsigned int cmd,
 	unsigned int val;
 	int err;
 
+	if (wdd->ops->ioctl) {
+		err = wdd->ops->ioctl(wdd, cmd, arg);
+		if (err != -ENOIOCTLCMD)
+			return err;
+	}
+
 	switch (cmd) {
 	case WDIOC_GETSUPPORT:
 		return copy_to_user(argp, wdd->info,
diff --git a/include/linux/watchdog.h b/include/linux/watchdog.h
index f719883..325d90b 100644
--- a/include/linux/watchdog.h
+++ b/include/linux/watchdog.h
@@ -70,6 +70,7 @@ struct watchdog_device;
  * @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.
+ * @ioctl:	The routines that handles extra ioctl calls.
  *
  * The watchdog_ops structure contains a list of low-level operations
  * that control a watchdog device. It also contains the module that owns
@@ -85,6 +86,7 @@ struct watchdog_ops {
 	int (*ping)(struct watchdog_device *);
 	unsigned int (*status)(struct watchdog_device *);
 	int (*set_timeout)(struct watchdog_device *, unsigned int);
+	long (*ioctl)(struct watchdog_device *, unsigned int, unsigned long);
 };
 
 /** struct watchdog_device - The structure that defines a watchdog device
-- 
1.7.6

[PATCH 9/9] watchdog: WatchDog Timer Driver Core - Add minimum and max timeout

[Wim Van Sebroeck]

Add min_timeout (minimum timeout) and max_timeout
values so that the framework can check if the new
timeout value is between the minimum and maximum
timeout values. If both values are 0, then the
framework will leave the check for the watchdog
device driver itself.

Signed-off-by: Alan Cox <alan@lxorguk.ukuu.org.uk>
Signed-off-by: Wim Van Sebroeck <wim@iguana.be>
Acked-by: Arnd Bergmann <arnd@arndb.de>
Acked-by: Wolfram Sang <w.sang@pengutronix.de>
---
 Documentation/watchdog/watchdog-kernel-api.txt |    4 ++++
 drivers/watchdog/watchdog_core.c               |   10 ++++++++++
 drivers/watchdog/watchdog_dev.c                |    3 +++
 include/linux/watchdog.h                       |    4 ++++
 4 files changed, 21 insertions(+), 0 deletions(-)

diff --git a/Documentation/watchdog/watchdog-kernel-api.txt b/Documentation/watchdog/watchdog-kernel-api.txt
index 829955b..4f7c894 100644
--- a/Documentation/watchdog/watchdog-kernel-api.txt
+++ b/Documentation/watchdog/watchdog-kernel-api.txt
@@ -43,6 +43,8 @@ struct watchdog_device {
 	const struct watchdog_ops *ops;
 	unsigned int bootstatus;
 	unsigned int timeout;
+	unsigned int min_timeout;
+	unsigned int max_timeout;
 	void *driver_data;
 	unsigned long status;
 };
@@ -52,6 +54,8 @@ It contains following fields:
   additional information about the watchdog timer itself. (Like it's unique name)
 * ops: a pointer to the list of watchdog operations that the watchdog supports.
 * timeout: the watchdog timer's timeout value (in seconds).
+* min_timeout: the watchdog timer's minimum timeout value (in seconds).
+* max_timeout: the watchdog timer's maximum timeout value (in seconds).
 * bootstatus: status of the device after booting (reported with watchdog
   WDIOF_* status bits).
 * driver_data: a pointer to the drivers private data of a watchdog device.
diff --git a/drivers/watchdog/watchdog_core.c b/drivers/watchdog/watchdog_core.c
index 47fc126..cfa1a15 100644
--- a/drivers/watchdog/watchdog_core.c
+++ b/drivers/watchdog/watchdog_core.c
@@ -59,6 +59,16 @@ int watchdog_register_device(struct watchdog_device *wdd)
 		return -EINVAL;
 
 	/*
+	 * Check that we have valid min and max timeout values, if
+	 * not reset them both to 0 (=not used or unknown)
+	 */
+	if (wdd->min_timeout > wdd->max_timeout) {
+		pr_info("Invalid min and max timeout values, resetting to 0!\n");
+		wdd->min_timeout = 0;
+		wdd->max_timeout = 0;
+	}
+
+	/*
 	 * Note: now that all watchdog_device data has been verified, we
 	 * will not check this anymore in other functions. If data gets
 	 * corrupted in a later stage then we expect a kernel panic!
diff --git a/drivers/watchdog/watchdog_dev.c b/drivers/watchdog/watchdog_dev.c
index e7134a5..d33520d 100644
--- a/drivers/watchdog/watchdog_dev.c
+++ b/drivers/watchdog/watchdog_dev.c
@@ -220,6 +220,9 @@ static long watchdog_ioctl(struct file *file, unsigned int cmd,
 			return -EOPNOTSUPP;
 		if (get_user(val, p))
 			return -EFAULT;
+		if ((wdd->max_timeout != 0) &&
+		    (val < wdd->min_timeout || val > wdd->max_timeout))
+				return -EINVAL;
 		err = wdd->ops->set_timeout(wdd, val);
 		if (err < 0)
 			return err;
diff --git a/include/linux/watchdog.h b/include/linux/watchdog.h
index 325d90b..111843f 100644
--- a/include/linux/watchdog.h
+++ b/include/linux/watchdog.h
@@ -95,6 +95,8 @@ struct watchdog_ops {
  * @ops:	Pointer to the list of watchdog operations.
  * @bootstatus:	Status of the watchdog device at boot.
  * @timeout:	The watchdog devices timeout value.
+ * @min_timeout:The watchdog devices minimum timeout value.
+ * @max_timeout:The watchdog devices maximum timeout value.
  * @driver-data:Pointer to the drivers private data.
  * @status:	Field that contains the devices internal status bits.
  *
@@ -109,6 +111,8 @@ struct watchdog_device {
 	const struct watchdog_ops *ops;
 	unsigned int bootstatus;
 	unsigned int timeout;
+	unsigned int min_timeout;
+	unsigned int max_timeout;
 	void *driver_data;
 	unsigned long status;
 /* Bit numbers for status flags */
-- 
1.7.6

Re: [PATCH 1/9] watchdog: WatchDog Timer Driver Core - Add basic framework

[Joe Perches]

On Wed, 2011-07-27 at 20:16 +0000, Wim Van Sebroeck wrote:
> The WatchDog Timer Driver Core is a framework
> that contains the common code for all watchdog-driver's.
> It also introduces a watchdog device structure and the
> operations that go with it.
> 
> This is the introduction of this framework. This part
> supports the minimal watchdog userspace API (or with
> other words: the functionality to use /dev/watchdog's
> open, release and write functionality as defined in
> the simplest watchdog API). Extra functionality will
> follow in the next set of patches.

Perhaps add some logging helpers to the .h?

#define wdd_emerg(wd, fmt, ...)					\
	pr_emerg("%s: " fmt, (wd)->info->identity, ##__VA_ARGS__)
#define wdd_crit(wd, fmt, ...)					\
	pr_crit("%s: " fmt, (wd)->info->identity, ##__VA_ARGS__)
#define wdd_alert(wd, fmt, ...)					\
	pr_alert("%s: " fmt, (wd)->info->identity, ##__VA_ARGS__)
#define wdd_err(wd, fmt, ...)					\
	pr_err("%s: " fmt, (wd)->info->identity, ##__VA_ARGS__)
#define wdd_notice(wd, fmt, ...)				\
	pr_notice("%s: " fmt, (wd)->info->identity, ##__VA_ARGS__)
#define wdd_warn(wd, fmt, ...)					\
	pr_warn("%s: " fmt, (wd)->info->identity, ##__VA_ARGS__)
#define wdd_info(wd, fmt, ...)					\
	pr_info("%s: " fmt, (wd)->info->identity, ##__VA_ARGS__)
#define wdd_dbg(wd, fmt, ...)					\
	pr_debug("%s: " fmt, (wd)->info->identity, ##__VA_ARGS__)

> +	err = misc_register(&watchdog_miscdev);
> +	if (err != 0) {
> +		pr_err("%s: cannot register miscdev on minor=%d (err=%d).\n",
> +			watchdog->info->identity, WATCHDOG_MINOR, err);

		wdd_err(watchdog, "cannot register watchdog on minor %d, err: %d\n",
			WATCHDOG_MINOR, err);

etc...

Re: [PATCH 1/9] watchdog: WatchDog Timer Driver Core - Add basic framework

[Arnd Bergmann]

On Thursday 28 July 2011, Joe Perches wrote:
> Perhaps add some logging helpers to the .h?
> 
> #define wdd_emerg(wd, fmt, ...)                                 \
>         pr_emerg("%s: " fmt, (wd)->info->identity, ##__VA_ARGS__)
> #define wdd_crit(wd, fmt, ...)                                  \
>         pr_crit("%s: " fmt, (wd)->info->identity, ##__VA_ARGS__)
> #define wdd_alert(wd, fmt, ...)                                 \
>         pr_alert("%s: " fmt, (wd)->info->identity, ##__VA_ARGS__)
> #define wdd_err(wd, fmt, ...)                                   \
>         pr_err("%s: " fmt, (wd)->info->identity, ##__VA_ARGS__)
> #define wdd_notice(wd, fmt, ...)                                \
>         pr_notice("%s: " fmt, (wd)->info->identity, ##__VA_ARGS__)
> #define wdd_warn(wd, fmt, ...)                                  \
>         pr_warn("%s: " fmt, (wd)->info->identity, ##__VA_ARGS__)
> #define wdd_info(wd, fmt, ...)                                  \
>         pr_info("%s: " fmt, (wd)->info->identity, ##__VA_ARGS__)
> #define wdd_dbg(wd, fmt, ...)                                   \
>         pr_debug("%s: " fmt, (wd)->info->identity, ##__VA_ARGS__)

No, please don't. Adding driver-specific debugging macros tends to confuse
more than it helps, IMHO.

> > +     err = misc_register(&watchdog_miscdev);
> > +     if (err != 0) {
> > +             pr_err("%s: cannot register miscdev on minor=%d (err=%d).\n",
> > +                     watchdog->info->identity, WATCHDOG_MINOR, err);
> 
>                 wdd_err(watchdog, "cannot register watchdog on minor %d, err: %d\n",
>                         WATCHDOG_MINOR, err);

I think changing them could be useful, but I'd rather change them to
dev_err etc and pass the underlying device.

	Arnd

Re: [PATCH 0/11 v3] Generic Watchdog Timer Driver

[Arnd Bergmann]

On Monday 11 July 2011 15:50:56 Wim Van Sebroeck wrote:
> To reduce copying the same code over and over in each watchdog device driver,
> Alan Cox and myself constructed a new framework/API that consolidates the common watchdog timer driver functions.
> I incorporated the changes/feedback that I received from the second post.
> 
> This framework/API consists of the following patches:
> part  1: Introduction of the WatchDog Timer Driver Core
> part  2: Add the basic ioctl functionality
> part  3: Add the WDIOC_KEEPALIVE ioctl
> part  4: Add the WDIOC_SETOPTIONS ioctl
> part  5: Add the WDIOC_SETTIMEOUT and WDIOC_GETTIMEOUT ioctl
> part  6: Add the Magic Close feature
> part  7: Add the nowayout feature
> part  8: Add support for a miscdev parent device
> part  9: Add support for extra ioctl calls
> part 10: Add the minimum and maximum timeout parameters.
> part 11: Remove llseek

Acked-by: Arnd Bergmann <arnd@arndb.de>, the whole series.

As far as I can tell, all the issues I've raised the last time have been
resolved, in a lot of cases by convincing me that your solution was
ok to start with ;-)

	Arnd

Re: [PATCH 0/11 v3] Generic Watchdog Timer Driver

[Arnaud Lacombe]

Hi,

On Mon, Jul 11, 2011 at 9:50 AM, Wim Van Sebroeck <wim@iguana.be> wrote:
> Hi All,
>
> To reduce copying the same code over and over in each watchdog device driver, Alan Cox and myself constructed a new framework/API that consolidates the common watchdog timer driver functions.
> I incorporated the changes/feedback that I received from the second post.
>
> This framework/API consists of the following patches:
> part  1: Introduction of the WatchDog Timer Driver Core
> part  2: Add the basic ioctl functionality
> part  3: Add the WDIOC_KEEPALIVE ioctl
> part  4: Add the WDIOC_SETOPTIONS ioctl
> part  5: Add the WDIOC_SETTIMEOUT and WDIOC_GETTIMEOUT ioctl
> part  6: Add the Magic Close feature
> part  7: Add the nowayout feature
> part  8: Add support for a miscdev parent device
> part  9: Add support for extra ioctl calls
> part 10: Add the minimum and maximum timeout parameters.
> part 11: Remove llseek
>
> The code will also be added to linux-2.6-watchdog-next.
>
> Changes since V2:
> * all "flags" are unsigned
> * timeout values are also unsigned
> * removed unnececessary debugging
> * clean-up comments
> * the core will be in drivers/watchdog/ and not in drivers/watchdog/core/
>
I had the occasion to have a look to the code in -next this week, so
my comment will be based on this.

One thing which looked too constrained to me, is that the framework
only supports a for a single watchdog. While this is fine for most
board, I have an x86 based device which has 2 watchdog. This might be
silly, but that's what the hardware provide. The current wartchdog
framework would fail to adapt to this board. Moreover, one might think
to provide interface to fail-to-wire NICs device on top of the
watchdog framework. So at the end, it might be needed to be able to
register an infinite amount of watchdog. Though, I am not sure how it
would presented to userland, maybe something ala /dev/watchdog/[0-N].

Regards,
 - Arnaud

> Kind regards,
> Wim.
>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> Please read the FAQ at  http://www.tux.org/lkml/
>

Re: [PATCH 0/11 v3] Generic Watchdog Timer Driver

[Wim Van Sebroeck]

Hi Arnaud,

> I had the occasion to have a look to the code in -next this week, so
> my comment will be based on this.
> 
> One thing which looked too constrained to me, is that the framework
> only supports a for a single watchdog. While this is fine for most
> board, I have an x86 based device which has 2 watchdog. This might be
> silly, but that's what the hardware provide. The current wartchdog
> framework would fail to adapt to this board. Moreover, one might think
> to provide interface to fail-to-wire NICs device on top of the
> watchdog framework. So at the end, it might be needed to be able to
> register an infinite amount of watchdog. Though, I am not sure how it
> would presented to userland, maybe something ala /dev/watchdog/[0-N].

This is what I said allready in the past:
1) we need the framework to get rid of the existing common code first.
2) next step is to extend the framework to support multiple watchdogs.
(via a sysfs like interface).

And we are seeing other interesting functionality that might also be added
to the framework. So still a lot of work to do :-).

PS: this comment was removed as a cleanup from Beta code to code that
is ready to be included in mainline:
+       /* The future version will have to manage a list of all
+        * registered watchdog devices. To start we will only
+        * support 1 watchdog device via the /dev/watchdog interface */

Kind regards,
Wim.