[PATCH 1/4] watchdog: Add multiple device support

[PATCH 1/4] watchdog: Add multiple device support

[Alan Cox]

From: Alan Cox <alan@linux.intel.com>

We keep the old /dev/watchdog interface file for the first watchdog via
miscdev. This is basically a cut and paste of the relevant interface code
from the rtc driver layer tweaked for watchdog.

Revised to fix problems noted by Hans de Goede

Signed-off-by: Alan Cox <alan@linux.intel.com>
---

 drivers/watchdog/watchdog_dev.c |  231 ++++++++++++++++++++++++++++++++-------
 include/linux/watchdog.h        |    6 +
 2 files changed, 193 insertions(+), 44 deletions(-)


diff --git a/drivers/watchdog/watchdog_dev.c b/drivers/watchdog/watchdog_dev.c
index c6e1b8d..40c557b 100644
--- a/drivers/watchdog/watchdog_dev.c
+++ b/drivers/watchdog/watchdog_dev.c
@@ -42,10 +42,12 @@
 #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;
+static dev_t dog_devt;		/* Watchdog device range */
+static unsigned long dog_mask;	/* Watchdogs that are in use */
 /* the watchdog device behind /dev/watchdog */
-static struct watchdog_device *wdd;
+static struct watchdog_device *old_wdd;
+
+#define DOG_MAX		32	/* assign_id must be changed to boost this */
 
 /*
  *	watchdog_ping: ping the watchdog.
@@ -136,6 +138,7 @@ static int watchdog_stop(struct watchdog_device *wddev)
 static ssize_t watchdog_write(struct file *file, const char __user *data,
 						size_t len, loff_t *ppos)
 {
+	struct watchdog_device *wdd = file->private_data;
 	size_t i;
 	char c;
 
@@ -175,6 +178,7 @@ static ssize_t watchdog_write(struct file *file, const char __user *data,
 static long watchdog_ioctl(struct file *file, unsigned int cmd,
 							unsigned long arg)
 {
+	struct watchdog_device *wdd = file->private_data;
 	void __user *argp = (void __user *)arg;
 	int __user *p = argp;
 	unsigned int val;
@@ -251,7 +255,8 @@ static long watchdog_ioctl(struct file *file, unsigned int cmd,
  *	it can only be opened once.
  */
 
-static int watchdog_open(struct inode *inode, struct file *file)
+static int watchdog_do_open(struct watchdog_device *wdd,
+				struct inode *inode, struct file *file)
 {
 	int err = -EBUSY;
 
@@ -270,6 +275,8 @@ static int watchdog_open(struct inode *inode, struct file *file)
 	if (err < 0)
 		goto out_mod;
 
+	file->private_data = wdd;
+
 	/* dev/watchdog is a virtual (and thus non-seekable) filesystem */
 	return nonseekable_open(inode, file);
 
@@ -281,7 +288,40 @@ out:
 }
 
 /*
- *      watchdog_release: release the /dev/watchdog device.
+ *	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)
+{
+	return watchdog_do_open(old_wdd, inode, file);
+}
+
+/*
+ *	watchdog_mopen: open the newer watchdog devices.
+ *	@inode: inode of device
+ *	@file: file handle to device
+ *
+ *	When the 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_mopen(struct inode *inode, struct file *file)
+{
+	struct watchdog_device *wdd = container_of(inode->i_cdev,
+		struct watchdog_device, cdev);
+
+	return watchdog_do_open(wdd, inode, file);
+}
+
+/*
+ *      watchdog_release: release the watchdog device.
  *      @inode: inode of device
  *      @file: file handle to device
  *
@@ -292,6 +332,7 @@ out:
 
 static int watchdog_release(struct inode *inode, struct file *file)
 {
+	struct watchdog_device *wdd = file->private_data;
 	int err = -EBUSY;
 
 	/*
@@ -326,69 +367,171 @@ static const struct file_operations watchdog_fops = {
 	.release	= watchdog_release,
 };
 
+static const struct file_operations watchdog_multi_fops = {
+	.owner		= THIS_MODULE,
+	.write		= watchdog_write,
+	.unlocked_ioctl	= watchdog_ioctl,
+	.open		= watchdog_mopen,
+	.release	= watchdog_release,
+};
+
 static struct miscdevice watchdog_miscdev = {
 	.minor		= WATCHDOG_MINOR,
 	.name		= "watchdog",
 	.fops		= &watchdog_fops,
 };
 
-/*
- *	watchdog_dev_register:
- *	@watchdog: watchdog device
+/**
+ *	watchdog_assign_id	-	pick a free watchdog ident
+ *	@watchdog: the watchdog device
+ *	@first: number to scan from
  *
- *	Register a watchdog device as /dev/watchdog. /dev/watchdog
- *	is actually a miscdevice and thus we set it up like that.
+ *	Assign a watchdog number to the device. For the moment we support
+ *	a starting offset so that a legacy watchdog can be worked around
  */
+static int watchdog_assign_id(struct watchdog_device *watchdog, int first)
+{
+	int i;
+	for (i = first; i < DOG_MAX; i ++) {
+		if (test_and_set_bit(i, &dog_mask) == 0) {
+			watchdog->id = i;
+			return i;
+		}
+	}
+	pr_err("no free watchdog slots.\n");
+	return -EBUSY;
+}
 
-int watchdog_dev_register(struct watchdog_device *watchdog)
+/**
+ *	watchdog_release_id	-	release a watchdg id
+ *	@watchdog: the watchdog device
+ *
+ *	Release the identifier associated with this watchdog. All cdev
+ *	resources must have been released first.
+ */
+static void watchdog_release_id(struct watchdog_device *watchdog)
+{
+	clear_bit(watchdog->id, &dog_mask);
+}
+
+/**
+ *	watchdog_add_device	-	add a watchdog device
+ *	@watchdog: the watchdog device
+ *
+ *	Add a watchdog device node to the system and set up all the
+ *	needed structures.
+ */
+static int watchdog_add_device(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;
-	}
+	/* Fill in the data structures */
+	watchdog->devt = MKDEV(MAJOR(dog_devt), watchdog->id);
+	watchdog->cdev.owner = watchdog->ops->owner;
+	cdev_init(&watchdog->cdev, &watchdog_multi_fops);
 
-	wdd = watchdog;
+	/* Add the device */
+	err  = cdev_add(&watchdog->cdev, watchdog->devt, 1);
+	if (err)
+		pr_err("watchdog%d unable to add device %d:%d\n",
+			watchdog->id,  MAJOR(dog_devt), watchdog->id);
+	return err;
+}
 
-	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;
-	}
+/**
+ *	watchdog_del_device	-	delete a watchdog device
+ *	@watchdog: the watchdog device
+ *
+ *	Delete a watchdog device cdev object
+ */
+static void watchdog_del_device(struct watchdog_device *watchdog)
+{
+	cdev_del(&watchdog->cdev);
+}	
 
-	return 0;
+/**
+ *	watchdog_dev_register 	-	register a watchdog
+ *	@watchdog: watchdog device
+ *
+ *	Register a watchdog device including handling the legacy
+ *	/dev/watchdog node. /dev/watchdog is actually a miscdevice and
+ *	thus we set it up like that.
+ */
+int watchdog_dev_register(struct watchdog_device *watchdog)
+{
+	int err;
 
-out:
-	wdd = NULL;
-	clear_bit(0, &watchdog_dev_busy);
+	err = watchdog_assign_id(watchdog, 0);
+	if (err < 0)
+		return err;
+	if (err == 0) {
+		err = misc_register(&watchdog_miscdev);
+		if (err == 0)
+			old_wdd = watchdog;
+		else {
+			pr_err("%s: cannot register miscdev on minor=%d (err=%d).\n",
+				watchdog->info->identity, WATCHDOG_MINOR, err);
+			if (err == -EBUSY) {
+				pr_err("%s: probably a legacy watchdog module is present.\n",
+					watchdog->info->identity);
+				watchdog_release_id(watchdog);
+				err = watchdog_assign_id(watchdog, 1);
+			}
+			if (err < 0)
+				return err;
+		}
+	}
+	err = watchdog_add_device(watchdog);
+	if (err < 0) {
+		if (watchdog->id == 0) {
+			misc_deregister(&watchdog_miscdev);
+			old_wdd = NULL;
+		}
+		watchdog_release_id(watchdog);
+	}
 	return err;
 }
 
-/*
- *	watchdog_dev_unregister:
+/**
+ *	watchdog_dev_unregister		-	unregister a watchdog
  *	@watchdog: watchdog device
  *
- *	Deregister the /dev/watchdog device.
+ *	Unregister the watchdog and if needed the legacy /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;
+	watchdog_del_device(watchdog);
+	if (watchdog->id == 0) {
+		misc_deregister(&watchdog_miscdev);
+		old_wdd = NULL;
 	}
-
-	misc_deregister(&watchdog_miscdev);
-	wdd = NULL;
-	clear_bit(0, &watchdog_dev_busy);
+	watchdog_release_id(watchdog);
 	return 0;
 }
+
+/**
+ *	watchdog_init	-	module init call
+ *
+ *	Allocate a range of chardev nodes to use for watchdog devices
+ */
+int __init watchdog_init(void)
+{
+	int err = alloc_chrdev_region(&dog_devt, 0, DOG_MAX, "watchdog");
+	if (err < 0)
+		pr_err("watchdog: unable to allocate char dev region\n");
+	return err;
+}
+
+/**
+ *	watchdog_exit	-	module exit
+ *
+ *	Release the range of chardev nodes used for watchdog devices
+ */
+void __exit watchdog_exit(void)
+{
+	unregister_chrdev_region(dog_devt, DOG_MAX);
+}
+
+module_init(watchdog_init);
+module_exit(watchdog_exit);
+
diff --git a/include/linux/watchdog.h b/include/linux/watchdog.h
index de75167..0d5c3af 100644
--- a/include/linux/watchdog.h
+++ b/include/linux/watchdog.h
@@ -54,6 +54,8 @@ struct watchdog_info {
 #ifdef __KERNEL__
 
 #include <linux/bitops.h>
+#include <linux/device.h>
+#include <linux/cdev.h>
 
 struct watchdog_ops;
 struct watchdog_device;
@@ -103,6 +105,10 @@ struct watchdog_ops {
  * via the watchdog_set_drvdata and watchdog_get_drvdata helpers.
  */
 struct watchdog_device {
+	struct cdev cdev;		/* Character device */
+	dev_t devt;			/* Our device */
+	int id;				/* Watchdog id */
+
 	const struct watchdog_info *info;
 	const struct watchdog_ops *ops;
 	unsigned int bootstatus;

[PATCH 2/4] watchdog: Add a flag to indicate the watchdog doesn't reboot things

[Alan Cox]

From: Alan Cox <alan@linux.intel.com>

Some watchdogs merely trigger external alarms and controls. In a managed
environment this is very useful but we want drivers to be able to figure
out which is which now multiple dogs can be loaded. Thus add an ALARMONLY
feature flag.

Signed-off-by: Alan Cox <alan@linux.intel.com>
---

 include/linux/watchdog.h |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)


diff --git a/include/linux/watchdog.h b/include/linux/watchdog.h
index 0d5c3af..fdd5dbb 100644
--- a/include/linux/watchdog.h
+++ b/include/linux/watchdog.h
@@ -45,6 +45,8 @@ struct watchdog_info {
 #define	WDIOF_SETTIMEOUT	0x0080  /* Set timeout (in seconds) */
 #define	WDIOF_MAGICCLOSE	0x0100	/* Supports magic close char */
 #define	WDIOF_PRETIMEOUT	0x0200  /* Pretimeout (in seconds), get/set */
+#define WDIOF_ALARMONLY		0x0400	/* Watchdog triggers a management or
+					   other external alarm not a reboot */
 #define	WDIOF_KEEPALIVEPING	0x8000	/* Keep alive ping reply */
 
 #define	WDIOS_DISABLECARD	0x0001	/* Turn off the watchdog timer */

[PATCH 3/4] watchdog: create all the proper device files

[Alan Cox]

From: Alan Cox <alan@linux.intel.com>

Signed-off-by: Alan Cox <alan@linux.intel.com>
---

 drivers/watchdog/watchdog_dev.c |   29 ++++++++++++++++++++++++-----
 include/linux/watchdog.h        |    5 ++++-
 2 files changed, 28 insertions(+), 6 deletions(-)


diff --git a/drivers/watchdog/watchdog_dev.c b/drivers/watchdog/watchdog_dev.c
index 40c557b..abefa56 100644
--- a/drivers/watchdog/watchdog_dev.c
+++ b/drivers/watchdog/watchdog_dev.c
@@ -42,6 +42,8 @@
 #include <linux/init.h>		/* For __init/__exit/... */
 #include <linux/uaccess.h>	/* For copy_to_user/put_user/... */
 
+static struct class *watchdog_class;	/* So we have someone to create
+					   our devices */
 static dev_t dog_devt;		/* Watchdog device range */
 static unsigned long dog_mask;	/* Watchodgs that are in use */
 /* the watchdog device behind /dev/watchdog */
@@ -429,13 +431,21 @@ static int watchdog_add_device(struct watchdog_device *watchdog)
 	watchdog->devt = MKDEV(MAJOR(dog_devt), watchdog->id);
 	watchdog->cdev.owner = watchdog->ops->owner;
 	cdev_init(&watchdog->cdev, &watchdog_multi_fops);
-
 	/* Add the device */
 	err  = cdev_add(&watchdog->cdev, watchdog->devt, 1);
-	if (err)
+	if (err) {
 		pr_err("watchdog%d unable to add device %d:%d\n",
 			watchdog->id,  MAJOR(dog_devt), watchdog->id);
-	return err;
+		return err;
+	}
+	watchdog->dev = device_create(watchdog_class, watchdog->busdev,
+					MKDEV(MAJOR(dog_devt), watchdog->id),
+					NULL, "watchdog%d", watchdog->id);
+	if (IS_ERR(watchdog->dev)) {
+		cdev_del(&watchdog->cdev);
+		return PTR_ERR(watchdog->dev);
+	}
+	return 0;
 }
 
 /**
@@ -447,6 +457,7 @@ static int watchdog_add_device(struct watchdog_device *watchdog)
 static void watchdog_del_device(struct watchdog_device *watchdog)
 {
 	cdev_del(&watchdog->cdev);
+	device_destroy(watchdog_class, MKDEV(MAJOR(dog_devt), watchdog->id));
 }	
 
 /**
@@ -517,9 +528,16 @@ int watchdog_dev_unregister(struct watchdog_device *watchdog)
 int __init watchdog_init(void)
 {
 	int err = alloc_chrdev_region(&dog_devt, 0, DOG_MAX, "watchdog");
-	if (err < 0)
+	if (err < 0) {
 		pr_err("watchdog: unable to allocate char dev region\n");
-	return err;
+		return err;
+	}
+	watchdog_class = class_create(THIS_MODULE, "watchdog");
+	if (IS_ERR(watchdog_class)) {
+		unregister_chrdev_region(dog_devt, DOG_MAX);
+		return PTR_ERR(watchdog_class);
+	}
+	return 0;
 }
 
 /**
@@ -530,6 +548,7 @@ int __init watchdog_init(void)
 void __exit watchdog_exit(void)
 {
 	unregister_chrdev_region(dog_devt, DOG_MAX);
+	class_destroy(watchdog_class);
 }
 
 module_init(watchdog_init);
diff --git a/include/linux/watchdog.h b/include/linux/watchdog.h
index fdd5dbb..b7a3359 100644
--- a/include/linux/watchdog.h
+++ b/include/linux/watchdog.h
@@ -108,7 +108,10 @@ struct watchdog_ops {
  */
 struct watchdog_device {
 	struct cdev cdev;		/* Character device */
-	dev_t devt;			/* Our device */
+	struct device *dev;		/* Device for our node */
+	struct device *busdev;		/* Parent bus device (set by caller
+							if used) */
+	dev_t devt;			/* Our device devt */
 	int id;				/* Watchdog id */
 
 	const struct watchdog_info *info;

[PATCH 4/4] watchdog: use dev_ functions

[Alan Cox]

From: Alan Cox <alan@linux.intel.com>

While they are registered all our watchdogs now have a valid device object
so we can in turn use that to report problems nicely.

Signed-off-by: Alan Cox <alan@linux.intel.com>
---

 drivers/watchdog/watchdog_dev.c |    5 ++---
 1 files changed, 2 insertions(+), 3 deletions(-)


diff --git a/drivers/watchdog/watchdog_dev.c b/drivers/watchdog/watchdog_dev.c
index abefa56..902a3af 100644
--- a/drivers/watchdog/watchdog_dev.c
+++ b/drivers/watchdog/watchdog_dev.c
@@ -110,8 +110,7 @@ static int watchdog_stop(struct watchdog_device *wddev)
 	int err = -EBUSY;
 
 	if (test_bit(WDOG_NO_WAY_OUT, &wddev->status)) {
-		pr_info("%s: nowayout prevents watchdog to be stopped!\n",
-							wddev->info->identity);
+		dev_info(wddev->dev, "nowayout prevents watchdog being stopped!\n");
 		return err;
 	}
 
@@ -348,7 +347,7 @@ static int watchdog_release(struct inode *inode, struct file *file)
 
 	/* If the watchdog was not stopped, send a keepalive ping */
 	if (err < 0) {
-		pr_crit("%s: watchdog did not stop!\n", wdd->info->identity);
+		dev_crit(wdd->dev, "watchdog did not stop!\n");
 		watchdog_ping(wdd);
 	}
 

Re: [PATCH 1/4] watchdog: Add multiple device support

[Hans de Goede]

Hi,

I've reviewed & tested this, it looks good and works as advertised.

But I'm afraid that I missed one item during my previous review,
this patch should also update:
Documentation/watchdog/watchdog-kernel-api.txt and the comment
above struct watchdog_device in include/linux/watchdog.h,
WRT the new members added to struct watchdog_device.

Besides that there are also some minor whitespace issues
noted by checkpatch / git am:

ERROR: space prohibited before that '++' (ctx:WxB)
#193: FILE: drivers/watchdog/watchdog_dev.c:395:
+       for (i = first; i < DOG_MAX; i ++) {
                                        ^
ERROR: trailing whitespace
#261: FILE: drivers/watchdog/watchdog_dev.c:450:
+}^I$

WARNING: please, no space before tabs
#265: FILE: drivers/watchdog/watchdog_dev.c:453:
+ *^Iwatchdog_dev_register ^I-^Iregister a watchdog$

/home/hans/projects/linux/.git/rebase-apply/patch:323: new blank line at EOF.

With these issues fixed, this patch is:
Acked-by: Hans de Goede <hdegoede@redhat.com>

(and/or Reviewed-by / Tested-by whichever you prefer :)

Regards,

Hans


On 03/21/2012 04:24 PM, Alan Cox wrote:
> From: Alan Cox<alan@linux.intel.com>
>
> We keep the old /dev/watchdog interface file for the first watchdog via
> miscdev. This is basically a cut and paste of the relevant interface code
> from the rtc driver layer tweaked for watchdog.
>
> Revised to fix problems noted by Hans de Goede
>
> Signed-off-by: Alan Cox<alan@linux.intel.com>
> ---
>
>   drivers/watchdog/watchdog_dev.c |  231 ++++++++++++++++++++++++++++++++-------
>   include/linux/watchdog.h        |    6 +
>   2 files changed, 193 insertions(+), 44 deletions(-)
>
>
> diff --git a/drivers/watchdog/watchdog_dev.c b/drivers/watchdog/watchdog_dev.c
> index c6e1b8d..40c557b 100644
> --- a/drivers/watchdog/watchdog_dev.c
> +++ b/drivers/watchdog/watchdog_dev.c
> @@ -42,10 +42,12 @@
>   #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;
> +static dev_t dog_devt;		/* Watchdog device range */
> +static unsigned long dog_mask;	/* Watchdogs that are in use */
>   /* the watchdog device behind /dev/watchdog */
> -static struct watchdog_device *wdd;
> +static struct watchdog_device *old_wdd;
> +
> +#define DOG_MAX		32	/* assign_id must be changed to boost this */
>
>   /*
>    *	watchdog_ping: ping the watchdog.
> @@ -136,6 +138,7 @@ static int watchdog_stop(struct watchdog_device *wddev)
>   static ssize_t watchdog_write(struct file *file, const char __user *data,
>   						size_t len, loff_t *ppos)
>   {
> +	struct watchdog_device *wdd = file->private_data;
>   	size_t i;
>   	char c;
>
> @@ -175,6 +178,7 @@ static ssize_t watchdog_write(struct file *file, const char __user *data,
>   static long watchdog_ioctl(struct file *file, unsigned int cmd,
>   							unsigned long arg)
>   {
> +	struct watchdog_device *wdd = file->private_data;
>   	void __user *argp = (void __user *)arg;
>   	int __user *p = argp;
>   	unsigned int val;
> @@ -251,7 +255,8 @@ static long watchdog_ioctl(struct file *file, unsigned int cmd,
>    *	it can only be opened once.
>    */
>
> -static int watchdog_open(struct inode *inode, struct file *file)
> +static int watchdog_do_open(struct watchdog_device *wdd,
> +				struct inode *inode, struct file *file)
>   {
>   	int err = -EBUSY;
>
> @@ -270,6 +275,8 @@ static int watchdog_open(struct inode *inode, struct file *file)
>   	if (err<  0)
>   		goto out_mod;
>
> +	file->private_data = wdd;
> +
>   	/* dev/watchdog is a virtual (and thus non-seekable) filesystem */
>   	return nonseekable_open(inode, file);
>
> @@ -281,7 +288,40 @@ out:
>   }
>
>   /*
> - *      watchdog_release: release the /dev/watchdog device.
> + *	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)
> +{
> +	return watchdog_do_open(old_wdd, inode, file);
> +}
> +
> +/*
> + *	watchdog_mopen: open the newer watchdog devices.
> + *	@inode: inode of device
> + *	@file: file handle to device
> + *
> + *	When the 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_mopen(struct inode *inode, struct file *file)
> +{
> +	struct watchdog_device *wdd = container_of(inode->i_cdev,
> +		struct watchdog_device, cdev);
> +
> +	return watchdog_do_open(wdd, inode, file);
> +}
> +
> +/*
> + *      watchdog_release: release the watchdog device.
>    *      @inode: inode of device
>    *      @file: file handle to device
>    *
> @@ -292,6 +332,7 @@ out:
>
>   static int watchdog_release(struct inode *inode, struct file *file)
>   {
> +	struct watchdog_device *wdd = file->private_data;
>   	int err = -EBUSY;
>
>   	/*
> @@ -326,69 +367,171 @@ static const struct file_operations watchdog_fops = {
>   	.release	= watchdog_release,
>   };
>
> +static const struct file_operations watchdog_multi_fops = {
> +	.owner		= THIS_MODULE,
> +	.write		= watchdog_write,
> +	.unlocked_ioctl	= watchdog_ioctl,
> +	.open		= watchdog_mopen,
> +	.release	= watchdog_release,
> +};
> +
>   static struct miscdevice watchdog_miscdev = {
>   	.minor		= WATCHDOG_MINOR,
>   	.name		= "watchdog",
>   	.fops		=&watchdog_fops,
>   };
>
> -/*
> - *	watchdog_dev_register:
> - *	@watchdog: watchdog device
> +/**
> + *	watchdog_assign_id	-	pick a free watchdog ident
> + *	@watchdog: the watchdog device
> + *	@first: number to scan from
>    *
> - *	Register a watchdog device as /dev/watchdog. /dev/watchdog
> - *	is actually a miscdevice and thus we set it up like that.
> + *	Assign a watchdog number to the device. For the moment we support
> + *	a starting offset so that a legacy watchdog can be worked around
>    */
> +static int watchdog_assign_id(struct watchdog_device *watchdog, int first)
> +{
> +	int i;
> +	for (i = first; i<  DOG_MAX; i ++) {
> +		if (test_and_set_bit(i,&dog_mask) == 0) {
> +			watchdog->id = i;
> +			return i;
> +		}
> +	}
> +	pr_err("no free watchdog slots.\n");
> +	return -EBUSY;
> +}
>
> -int watchdog_dev_register(struct watchdog_device *watchdog)
> +/**
> + *	watchdog_release_id	-	release a watchdg id
> + *	@watchdog: the watchdog device
> + *
> + *	Release the identifier associated with this watchdog. All cdev
> + *	resources must have been released first.
> + */
> +static void watchdog_release_id(struct watchdog_device *watchdog)
> +{
> +	clear_bit(watchdog->id,&dog_mask);
> +}
> +
> +/**
> + *	watchdog_add_device	-	add a watchdog device
> + *	@watchdog: the watchdog device
> + *
> + *	Add a watchdog device node to the system and set up all the
> + *	needed structures.
> + */
> +static int watchdog_add_device(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;
> -	}
> +	/* Fill in the data structures */
> +	watchdog->devt = MKDEV(MAJOR(dog_devt), watchdog->id);
> +	watchdog->cdev.owner = watchdog->ops->owner;
> +	cdev_init(&watchdog->cdev,&watchdog_multi_fops);
>
> -	wdd = watchdog;
> +	/* Add the device */
> +	err  = cdev_add(&watchdog->cdev, watchdog->devt, 1);
> +	if (err)
> +		pr_err("watchdog%d unable to add device %d:%d\n",
> +			watchdog->id,  MAJOR(dog_devt), watchdog->id);
> +	return err;
> +}
>
> -	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;
> -	}
> +/**
> + *	watchdog_del_device	-	delete a watchdog device
> + *	@watchdog: the watchdog device
> + *
> + *	Delete a watchdog device cdev object
> + */
> +static void watchdog_del_device(struct watchdog_device *watchdog)
> +{
> +	cdev_del(&watchdog->cdev);
> +}	
>
> -	return 0;
> +/**
> + *	watchdog_dev_register 	-	register a watchdog
> + *	@watchdog: watchdog device
> + *
> + *	Register a watchdog device including handling the legacy
> + *	/dev/watchdog node. /dev/watchdog is actually a miscdevice and
> + *	thus we set it up like that.
> + */
> +int watchdog_dev_register(struct watchdog_device *watchdog)
> +{
> +	int err;
>
> -out:
> -	wdd = NULL;
> -	clear_bit(0,&watchdog_dev_busy);
> +	err = watchdog_assign_id(watchdog, 0);
> +	if (err<  0)
> +		return err;
> +	if (err == 0) {
> +		err = misc_register(&watchdog_miscdev);
> +		if (err == 0)
> +			old_wdd = watchdog;
> +		else {
> +			pr_err("%s: cannot register miscdev on minor=%d (err=%d).\n",
> +				watchdog->info->identity, WATCHDOG_MINOR, err);
> +			if (err == -EBUSY) {
> +				pr_err("%s: probably a legacy watchdog module is present.\n",
> +					watchdog->info->identity);
> +				watchdog_release_id(watchdog);
> +				err = watchdog_assign_id(watchdog, 1);
> +			}
> +			if (err<  0)
> +				return err;
> +		}
> +	}
> +	err = watchdog_add_device(watchdog);
> +	if (err<  0) {
> +		if (watchdog->id == 0) {
> +			misc_deregister(&watchdog_miscdev);
> +			old_wdd = NULL;
> +		}
> +		watchdog_release_id(watchdog);
> +	}
>   	return err;
>   }
>
> -/*
> - *	watchdog_dev_unregister:
> +/**
> + *	watchdog_dev_unregister		-	unregister a watchdog
>    *	@watchdog: watchdog device
>    *
> - *	Deregister the /dev/watchdog device.
> + *	Unregister the watchdog and if needed the legacy /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;
> +	watchdog_del_device(watchdog);
> +	if (watchdog->id == 0) {
> +		misc_deregister(&watchdog_miscdev);
> +		old_wdd = NULL;
>   	}
> -
> -	misc_deregister(&watchdog_miscdev);
> -	wdd = NULL;
> -	clear_bit(0,&watchdog_dev_busy);
> +	watchdog_release_id(watchdog);
>   	return 0;
>   }
> +
> +/**
> + *	watchdog_init	-	module init call
> + *
> + *	Allocate a range of chardev nodes to use for watchdog devices
> + */
> +int __init watchdog_init(void)
> +{
> +	int err = alloc_chrdev_region(&dog_devt, 0, DOG_MAX, "watchdog");
> +	if (err<  0)
> +		pr_err("watchdog: unable to allocate char dev region\n");
> +	return err;
> +}
> +
> +/**
> + *	watchdog_exit	-	module exit
> + *
> + *	Release the range of chardev nodes used for watchdog devices
> + */
> +void __exit watchdog_exit(void)
> +{
> +	unregister_chrdev_region(dog_devt, DOG_MAX);
> +}
> +
> +module_init(watchdog_init);
> +module_exit(watchdog_exit);
> +
> diff --git a/include/linux/watchdog.h b/include/linux/watchdog.h
> index de75167..0d5c3af 100644
> --- a/include/linux/watchdog.h
> +++ b/include/linux/watchdog.h
> @@ -54,6 +54,8 @@ struct watchdog_info {
>   #ifdef __KERNEL__
>
>   #include<linux/bitops.h>
> +#include<linux/device.h>
> +#include<linux/cdev.h>
>
>   struct watchdog_ops;
>   struct watchdog_device;
> @@ -103,6 +105,10 @@ struct watchdog_ops {
>    * via the watchdog_set_drvdata and watchdog_get_drvdata helpers.
>    */
>   struct watchdog_device {
> +	struct cdev cdev;		/* Character device */
> +	dev_t devt;			/* Our device */
> +	int id;				/* Watchdog id */
> +
>   	const struct watchdog_info *info;
>   	const struct watchdog_ops *ops;
>   	unsigned int bootstatus;
>

Re: [PATCH 2/4] watchdog: Add a flag to indicate the watchdog doesn't reboot things

[Hans de Goede]

Looks good, ack.

Acked-by: Hans de Goede <hdegoede@redhat.com>

On 03/21/2012 04:25 PM, Alan Cox wrote:
> From: Alan Cox<alan@linux.intel.com>
>
> Some watchdogs merely trigger external alarms and controls. In a managed
> environment this is very useful but we want drivers to be able to figure
> out which is which now multiple dogs can be loaded. Thus add an ALARMONLY
> feature flag.
>
> Signed-off-by: Alan Cox<alan@linux.intel.com>
> ---
>
>   include/linux/watchdog.h |    2 ++
>   1 files changed, 2 insertions(+), 0 deletions(-)
>
>
> diff --git a/include/linux/watchdog.h b/include/linux/watchdog.h
> index 0d5c3af..fdd5dbb 100644
> --- a/include/linux/watchdog.h
> +++ b/include/linux/watchdog.h
> @@ -45,6 +45,8 @@ struct watchdog_info {
>   #define	WDIOF_SETTIMEOUT	0x0080  /* Set timeout (in seconds) */
>   #define	WDIOF_MAGICCLOSE	0x0100	/* Supports magic close char */
>   #define	WDIOF_PRETIMEOUT	0x0200  /* Pretimeout (in seconds), get/set */
> +#define WDIOF_ALARMONLY		0x0400	/* Watchdog triggers a management or
> +					   other external alarm not a reboot */
>   #define	WDIOF_KEEPALIVEPING	0x8000	/* Keep alive ping reply */
>
>   #define	WDIOS_DISABLECARD	0x0001	/* Turn off the watchdog timer */
>

Re: [PATCH 3/4] watchdog: create all the proper device files

[Hans de Goede]

Hi,

One small remark below (inline), with that fixed this patch is:
Acked-by: Hans de Goede <hdegoede@redhat.com>

Regards,

Hans

On 03/21/2012 04:25 PM, Alan Cox wrote:
> From: Alan Cox<alan@linux.intel.com>
>
> Signed-off-by: Alan Cox<alan@linux.intel.com>
> ---
>
>   drivers/watchdog/watchdog_dev.c |   29 ++++++++++++++++++++++++-----
>   include/linux/watchdog.h        |    5 ++++-
>   2 files changed, 28 insertions(+), 6 deletions(-)
>
>
> diff --git a/drivers/watchdog/watchdog_dev.c b/drivers/watchdog/watchdog_dev.c
> index 40c557b..abefa56 100644
> --- a/drivers/watchdog/watchdog_dev.c
> +++ b/drivers/watchdog/watchdog_dev.c
> @@ -42,6 +42,8 @@
>   #include<linux/init.h>		/* For __init/__exit/... */
>   #include<linux/uaccess.h>	/* For copy_to_user/put_user/... */
>
> +static struct class *watchdog_class;	/* So we have someone to create
> +					   our devices */
>   static dev_t dog_devt;		/* Watchdog device range */
>   static unsigned long dog_mask;	/* Watchodgs that are in use */
>   /* the watchdog device behind /dev/watchdog */
> @@ -429,13 +431,21 @@ static int watchdog_add_device(struct watchdog_device *watchdog)
>   	watchdog->devt = MKDEV(MAJOR(dog_devt), watchdog->id);
>   	watchdog->cdev.owner = watchdog->ops->owner;
>   	cdev_init(&watchdog->cdev,&watchdog_multi_fops);
> -
>   	/* Add the device */
>   	err  = cdev_add(&watchdog->cdev, watchdog->devt, 1);
> -	if (err)
> +	if (err) {
>   		pr_err("watchdog%d unable to add device %d:%d\n",
>   			watchdog->id,  MAJOR(dog_devt), watchdog->id);
> -	return err;
> +		return err;
> +	}
> +	watchdog->dev = device_create(watchdog_class, watchdog->busdev,
> +					MKDEV(MAJOR(dog_devt), watchdog->id),
> +					NULL, "watchdog%d", watchdog->id);

It is probably cleaner to pass watchdog->devt as 3th parameter here, rather then
MKDEV(MAJOR(dog_devt), watchdog->id).

> +	if (IS_ERR(watchdog->dev)) {
> +		cdev_del(&watchdog->cdev);
> +		return PTR_ERR(watchdog->dev);
> +	}
> +	return 0;
>   }
>
>   /**
> @@ -447,6 +457,7 @@ static int watchdog_add_device(struct watchdog_device *watchdog)
>   static void watchdog_del_device(struct watchdog_device *watchdog)
>   {
>   	cdev_del(&watchdog->cdev);
> +	device_destroy(watchdog_class, MKDEV(MAJOR(dog_devt), watchdog->id));
>   }	
>
>   /**
> @@ -517,9 +528,16 @@ int watchdog_dev_unregister(struct watchdog_device *watchdog)
>   int __init watchdog_init(void)
>   {
>   	int err = alloc_chrdev_region(&dog_devt, 0, DOG_MAX, "watchdog");
> -	if (err<  0)
> +	if (err<  0) {
>   		pr_err("watchdog: unable to allocate char dev region\n");
> -	return err;
> +		return err;
> +	}
> +	watchdog_class = class_create(THIS_MODULE, "watchdog");
> +	if (IS_ERR(watchdog_class)) {
> +		unregister_chrdev_region(dog_devt, DOG_MAX);
> +		return PTR_ERR(watchdog_class);
> +	}
> +	return 0;
>   }
>
>   /**
> @@ -530,6 +548,7 @@ int __init watchdog_init(void)
>   void __exit watchdog_exit(void)
>   {
>   	unregister_chrdev_region(dog_devt, DOG_MAX);
> +	class_destroy(watchdog_class);
>   }
>
>   module_init(watchdog_init);
> diff --git a/include/linux/watchdog.h b/include/linux/watchdog.h
> index fdd5dbb..b7a3359 100644
> --- a/include/linux/watchdog.h
> +++ b/include/linux/watchdog.h
> @@ -108,7 +108,10 @@ struct watchdog_ops {
>    */
>   struct watchdog_device {
>   	struct cdev cdev;		/* Character device */
> -	dev_t devt;			/* Our device */
> +	struct device *dev;		/* Device for our node */
> +	struct device *busdev;		/* Parent bus device (set by caller
> +							if used) */
> +	dev_t devt;			/* Our device devt */
>   	int id;				/* Watchdog id */
>
>   	const struct watchdog_info *info;
>

Re: [PATCH 4/4] watchdog: use dev_ functions

[Hans de Goede]

Looks good, ack.

Acked-by: Hans de Goede <hdegoede@redhat.com>

On 03/21/2012 04:25 PM, Alan Cox wrote:
> From: Alan Cox<alan@linux.intel.com>
>
> While they are registered all our watchdogs now have a valid device object
> so we can in turn use that to report problems nicely.
>
> Signed-off-by: Alan Cox<alan@linux.intel.com>
> ---
>
>   drivers/watchdog/watchdog_dev.c |    5 ++---
>   1 files changed, 2 insertions(+), 3 deletions(-)
>
>
> diff --git a/drivers/watchdog/watchdog_dev.c b/drivers/watchdog/watchdog_dev.c
> index abefa56..902a3af 100644
> --- a/drivers/watchdog/watchdog_dev.c
> +++ b/drivers/watchdog/watchdog_dev.c
> @@ -110,8 +110,7 @@ static int watchdog_stop(struct watchdog_device *wddev)
>   	int err = -EBUSY;
>
>   	if (test_bit(WDOG_NO_WAY_OUT,&wddev->status)) {
> -		pr_info("%s: nowayout prevents watchdog to be stopped!\n",
> -							wddev->info->identity);
> +		dev_info(wddev->dev, "nowayout prevents watchdog being stopped!\n");
>   		return err;
>   	}
>
> @@ -348,7 +347,7 @@ static int watchdog_release(struct inode *inode, struct file *file)
>
>   	/* If the watchdog was not stopped, send a keepalive ping */
>   	if (err<  0) {
> -		pr_crit("%s: watchdog did not stop!\n", wdd->info->identity);
> +		dev_crit(wdd->dev, "watchdog did not stop!\n");
>   		watchdog_ping(wdd);
>   	}
>
>

Re: [PATCH 2/4] watchdog: Add a flag to indicate the watchdog doesn't reboot things

[Wim Van Sebroeck]

Hi All,

> Looks good, ack.
> 
> Acked-by: Hans de Goede <hdegoede@redhat.com>

Will have a look at Alan's and Hans patches next week.

Kind regards,
Wim.

Re: [PATCH 1/4] watchdog: Add multiple device support

[Wim Van Sebroeck]

Hi Alan,

> We keep the old /dev/watchdog interface file for the first watchdog via
> miscdev. This is basically a cut and paste of the relevant interface code
> from the rtc driver layer tweaked for watchdog.
> 
> Revised to fix problems noted by Hans de Goede
> 
> Signed-off-by: Alan Cox <alan@linux.intel.com>

I'm ok with the principle. Need some more thoughts about the code (meaning: that the ID assignment should imho be in the watchdog_core.c file).

I do have one question though: why did you use your own assign_id routines and not ida_simple_get/ida_simple_remove for instance?

Thanks,
Wim.

Re: [PATCH 1/4] watchdog: Add multiple device support

[Wim Van Sebroeck]

Hi Alan, Hans,

> > We keep the old /dev/watchdog interface file for the first watchdog via
> > miscdev. This is basically a cut and paste of the relevant interface code
> > from the rtc driver layer tweaked for watchdog.
> > 
> > Revised to fix problems noted by Hans de Goede
> > 
> > Signed-off-by: Alan Cox <alan@linux.intel.com>
> 
> I'm ok with the principle. Need some more thoughts about the code (meaning: that the ID assignment should imho be in the watchdog_core.c file).
> 
> I do have one question though: why did you use your own assign_id routines and not ida_simple_get/ida_simple_remove for instance?

I re-tweaked it to fit the clean split between core and dev related stuff. I also used ida instead of the watchdog_assign/release_id code.
For the rest it's basically the code that Alan produced with some minor changes (and also with the comments that Hans made).

See attached the diff (I will need to add the kernel-api documentation update still).
Please comment and test.

Thanks in advance,
Wim.

---
diff --git a/drivers/watchdog/watchdog_core.c b/drivers/watchdog/watchdog_core.c
index 14d768b..9a10bd4 100644
--- a/drivers/watchdog/watchdog_core.c
+++ b/drivers/watchdog/watchdog_core.c
@@ -34,9 +34,12 @@
 #include <linux/kernel.h>	/* For printk/panic/... */
 #include <linux/watchdog.h>	/* For watchdog specific items */
 #include <linux/init.h>		/* For __init/__exit/... */
+#include <linux/idr.h>		/* For ida_* macros */
 
 #include "watchdog_dev.h"	/* For watchdog_dev_register/... */
 
+static DEFINE_IDA(watchdog_ida);
+
 /**
  * watchdog_register_device() - register a watchdog device
  * @wdd: watchdog device
@@ -49,7 +52,7 @@
  */
 int watchdog_register_device(struct watchdog_device *wdd)
 {
-	int ret;
+	int ret, id;
 
 	if (wdd == NULL || wdd->info == NULL || wdd->ops == NULL)
 		return -EINVAL;
@@ -74,11 +77,28 @@ int watchdog_register_device(struct watchdog_device *wdd)
 	 * corrupted in a later stage then we expect a kernel panic!
 	 */
 
-	/* We only support 1 watchdog device via the /dev/watchdog interface */
+	id = ida_simple_get(&watchdog_ida, 0, MAX_DOGS, GFP_KERNEL);
+	if (id < 0)
+		return id;
+	wdd->id = id;
+
 	ret = watchdog_dev_register(wdd);
 	if (ret) {
-		pr_err("error registering /dev/watchdog (err=%d)\n", ret);
-		return ret;
+		ida_simple_remove(&watchdog_ida, id);
+		if (id != 0)
+			return ret;
+
+		/* Retry in case a legacy watchdog module exists */
+		id = ida_simple_get(&watchdog_ida, 1, MAX_DOGS, GFP_KERNEL);
+		if (id < 0)
+			return id;
+		wdd->id = id;
+
+		ret = watchdog_dev_register(wdd);
+		if (ret) {
+			ida_simple_remove(&watchdog_ida, id);
+			return ret;
+		}
 	}
 
 	return 0;
@@ -102,6 +122,7 @@ void watchdog_unregister_device(struct watchdog_device *wdd)
 	ret = watchdog_dev_unregister(wdd);
 	if (ret)
 		pr_err("error unregistering /dev/watchdog (err=%d)\n", ret);
+	ida_simple_remove(&watchdog_ida, wdd->id);
 }
 EXPORT_SYMBOL_GPL(watchdog_unregister_device);
 
diff --git a/drivers/watchdog/watchdog_dev.c b/drivers/watchdog/watchdog_dev.c
index 930cc7c..b6666ab 100644
--- a/drivers/watchdog/watchdog_dev.c
+++ b/drivers/watchdog/watchdog_dev.c
@@ -44,10 +44,10 @@
 
 #include "watchdog_dev.h"
 
-/* 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;
+static struct watchdog_device *old_wdd;
+/* the dev_t structure to store the dynamically allocated watchdog devices */
+static dev_t watchdog_devt;
 
 /*
  *	watchdog_ping: ping the watchdog.
@@ -138,6 +138,7 @@ static int watchdog_stop(struct watchdog_device *wddev)
 static ssize_t watchdog_write(struct file *file, const char __user *data,
 						size_t len, loff_t *ppos)
 {
+	struct watchdog_device *wdd = file->private_data;
 	size_t i;
 	char c;
 
@@ -177,6 +178,7 @@ static ssize_t watchdog_write(struct file *file, const char __user *data,
 static long watchdog_ioctl(struct file *file, unsigned int cmd,
 							unsigned long arg)
 {
+	struct watchdog_device *wdd = file->private_data;
 	void __user *argp = (void __user *)arg;
 	int __user *p = argp;
 	unsigned int val;
@@ -249,11 +251,11 @@ static long watchdog_ioctl(struct file *file, unsigned int cmd,
 }
 
 /*
- *	watchdog_open: open the /dev/watchdog device.
+ *	watchdog_open: open the /dev/watchdog* devices.
  *	@inode: inode of device
  *	@file: file handle to device
  *
- *	When the /dev/watchdog device gets opened, we start the watchdog.
+ *	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.
  */
@@ -261,6 +263,12 @@ static long watchdog_ioctl(struct file *file, unsigned int cmd,
 static int watchdog_open(struct inode *inode, struct file *file)
 {
 	int err = -EBUSY;
+	struct watchdog_device *wdd;
+
+	/* Get the corresponding watchdog device */
+	if (imajor(inode) == MISC_MAJOR)
+		wdd = old_wdd;
+	else wdd = container_of(inode->i_cdev, struct watchdog_device, cdev);
 
 	/* the watchdog is single open! */
 	if (test_and_set_bit(WDOG_DEV_OPEN, &wdd->status))
@@ -277,6 +285,8 @@ static int watchdog_open(struct inode *inode, struct file *file)
 	if (err < 0)
 		goto out_mod;
 
+	file->private_data = wdd;
+
 	/* dev/watchdog is a virtual (and thus non-seekable) filesystem */
 	return nonseekable_open(inode, file);
 
@@ -288,9 +298,9 @@ out:
 }
 
 /*
- *      watchdog_release: release the /dev/watchdog device.
- *      @inode: inode of device
- *      @file: file handle to device
+ *	watchdog_release: release the watchdog device.
+ *	@inode: inode of device
+ *	@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 (and nowayout
@@ -299,6 +309,7 @@ out:
 
 static int watchdog_release(struct inode *inode, struct file *file)
 {
+	struct watchdog_device *wdd = file->private_data;
 	int err = -EBUSY;
 
 	/*
@@ -340,62 +351,90 @@ static struct miscdevice watchdog_miscdev = {
 };
 
 /*
- *	watchdog_dev_register:
+ *	watchdog_dev_register: register a watchdog device
  *	@watchdog: watchdog device
  *
- *	Register a watchdog device as /dev/watchdog. /dev/watchdog
- *	is actually a miscdevice and thus we set it up like that.
+ *	Register a watchdog device including handling the legacy
+ *	/dev/watchdog node. /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;
+	int err, devno;
+
+	if (watchdog->id == 0) {
+		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);
+			if (err == -EBUSY)
+				pr_err("%s: a legacy watchdog module is probably present.\n",
+					watchdog->info->identity);
+			return err;
+		}
+		old_wdd = watchdog;
 	}
 
-	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;
+	/* Fill in the data structures */
+	devno = MKDEV(MAJOR(watchdog_devt), watchdog->id);
+	cdev_init(&watchdog->cdev, &watchdog_fops);
+	watchdog->cdev.owner = watchdog->ops->owner;
+
+	/* Add the device */
+	err  = cdev_add(&watchdog->cdev, devno, 1);
+	if (err) {
+		pr_err("watchdog%d unable to add device %d:%d\n",
+			watchdog->id,  MAJOR(watchdog_devt), watchdog->id);
+		if (watchdog->id == 0) {
+			misc_deregister(&watchdog_miscdev);
+			old_wdd = NULL;
+		}
 	}
-
-	return 0;
-
-out:
-	wdd = NULL;
-	clear_bit(0, &watchdog_dev_busy);
 	return err;
 }
 
 /*
- *	watchdog_dev_unregister:
+ *	watchdog_dev_unregister: unregister a watchdog device
  *	@watchdog: watchdog device
  *
- *	Deregister the /dev/watchdog device.
+ *	Unregister the watchdog and if needed the legacy /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;
+	cdev_del(&watchdog->cdev);
+	if (watchdog->id == 0) {
+		misc_deregister(&watchdog_miscdev);
+		old_wdd = NULL;
 	}
-
-	misc_deregister(&watchdog_miscdev);
-	wdd = NULL;
-	clear_bit(0, &watchdog_dev_busy);
 	return 0;
 }
+
+/*
+ *	watchdog_init: module init call
+ *
+ *	Allocate a range of chardev nodes to use for watchdog devices
+ */
+
+int __init watchdog_init(void)
+{
+	int err = alloc_chrdev_region(&watchdog_devt, 0, MAX_DOGS, "watchdog");
+	if (err < 0)
+		pr_err("watchdog: unable to allocate char dev region\n");
+	return err;
+}
+
+/*
+ *	watchdog_exit: module exit
+ *
+ *	Release the range of chardev nodes used for watchdog devices
+ */
+
+void __exit watchdog_exit(void)
+{
+	unregister_chrdev_region(watchdog_devt, MAX_DOGS);
+}
+
+module_init(watchdog_init);
+module_exit(watchdog_exit);
diff --git a/drivers/watchdog/watchdog_dev.h b/drivers/watchdog/watchdog_dev.h
index bc7612b..0eb8f6f 100644
--- a/drivers/watchdog/watchdog_dev.h
+++ b/drivers/watchdog/watchdog_dev.h
@@ -26,6 +26,8 @@
  *	This material is provided "AS-IS" and at no charge.
  */
 
+#define MAX_DOGS	32	/* Maximum number of watchdog devices */
+
 /*
  *	Functions/procedures to be called by the core
  */
diff --git a/include/linux/watchdog.h b/include/linux/watchdog.h
index 1984ea6..fd648ea 100644
--- a/include/linux/watchdog.h
+++ b/include/linux/watchdog.h
@@ -54,6 +54,8 @@ struct watchdog_info {
 #ifdef __KERNEL__
 
 #include <linux/bitops.h>
+#include <linux/device.h>
+#include <linux/cdev.h>
 
 struct watchdog_ops;
 struct watchdog_device;
@@ -96,6 +98,8 @@ struct watchdog_ops {
  * @min_timeout:The watchdog devices minimum timeout value.
  * @max_timeout:The watchdog devices maximum timeout value.
  * @driver-data:Pointer to the drivers private data.
+ * @id:		The watchdog's ID.
+ * @cdev:	The watchdog's Character device.
  * @status:	Field that contains the devices internal status bits.
  *
  * The watchdog_device structure contains all information about a
@@ -112,6 +116,8 @@ struct watchdog_device {
 	unsigned int min_timeout;
 	unsigned int max_timeout;
 	void *driver_data;
+	int id;
+	struct cdev cdev;
 	unsigned long status;
 /* Bit numbers for status flags */
 #define WDOG_ACTIVE		0	/* Is the watchdog running/active */

Re: [PATCH 1/4] watchdog: Add multiple device support

[Hans de Goede]

Hi,

On 05/10/2012 09:20 PM, Wim Van Sebroeck wrote:
> Hi Alan, Hans,
>
>>> We keep the old /dev/watchdog interface file for the first watchdog via
>>> miscdev. This is basically a cut and paste of the relevant interface code
>>> from the rtc driver layer tweaked for watchdog.
>>>
>>> Revised to fix problems noted by Hans de Goede
>>>
>>> Signed-off-by: Alan Cox<alan@linux.intel.com>
>>
>> I'm ok with the principle. Need some more thoughts about the code (meaning: that the ID assignment should imho be in the watchdog_core.c file).
>>
>> I do have one question though: why did you use your own assign_id routines and not ida_simple_get/ida_simple_remove for instance?
>
> I re-tweaked it to fit the clean split between core and dev related stuff. I also used ida instead of the watchdog_assign/release_id code.
> For the rest it's basically the code that Alan produced with some minor changes (and also with the comments that Hans made).
>
> See attached the diff (I will need to add the kernel-api documentation update still).
> Please comment and test.

Thanks for working on this! I've 2 comments inline below, and I'll try to test this
today!

> Thanks in advance,
> Wim.
>
> ---
> diff --git a/drivers/watchdog/watchdog_core.c b/drivers/watchdog/watchdog_core.c
> index 14d768b..9a10bd4 100644
> --- a/drivers/watchdog/watchdog_core.c
> +++ b/drivers/watchdog/watchdog_core.c
> @@ -34,9 +34,12 @@
>   #include<linux/kernel.h>	/* For printk/panic/... */
>   #include<linux/watchdog.h>	/* For watchdog specific items */
>   #include<linux/init.h>		/* For __init/__exit/... */
> +#include<linux/idr.h>		/* For ida_* macros */
>
>   #include "watchdog_dev.h"	/* For watchdog_dev_register/... */
>
> +static DEFINE_IDA(watchdog_ida);
> +
>   /**
>    * watchdog_register_device() - register a watchdog device
>    * @wdd: watchdog device
> @@ -49,7 +52,7 @@
>    */
>   int watchdog_register_device(struct watchdog_device *wdd)
>   {
> -	int ret;
> +	int ret, id;
>
>   	if (wdd == NULL || wdd->info == NULL || wdd->ops == NULL)
>   		return -EINVAL;
> @@ -74,11 +77,28 @@ int watchdog_register_device(struct watchdog_device *wdd)
>   	 * corrupted in a later stage then we expect a kernel panic!
>   	 */
>
> -	/* We only support 1 watchdog device via the /dev/watchdog interface */
> +	id = ida_simple_get(&watchdog_ida, 0, MAX_DOGS, GFP_KERNEL);
> +	if (id<  0)
> +		return id;
> +	wdd->id = id;
> +
>   	ret = watchdog_dev_register(wdd);
>   	if (ret) {
> -		pr_err("error registering /dev/watchdog (err=%d)\n", ret);
> -		return ret;
> +		ida_simple_remove(&watchdog_ida, id);
> +		if (id != 0)
> +			return ret;


I believe the check above should be:
		if (id != 0 || ret != -EBUSY)
			return ret;

IOW we should not retry the registration if it failed for another reason
then the id already being taken by a legacy driver.

> +		/* Retry in case a legacy watchdog module exists */
> +		id = ida_simple_get(&watchdog_ida, 1, MAX_DOGS, GFP_KERNEL);
> +		if (id<  0)
> +			return id;
> +		wdd->id = id;
> +
> +		ret = watchdog_dev_register(wdd);
> +		if (ret) {
> +			ida_simple_remove(&watchdog_ida, id);
> +			return ret;
> +		}
>   	}
>
>   	return 0;
> @@ -102,6 +122,7 @@ void watchdog_unregister_device(struct watchdog_device *wdd)
>   	ret = watchdog_dev_unregister(wdd);
>   	if (ret)
>   		pr_err("error unregistering /dev/watchdog (err=%d)\n", ret);
> +	ida_simple_remove(&watchdog_ida, wdd->id);
>   }
>   EXPORT_SYMBOL_GPL(watchdog_unregister_device);
>

A note to myself :)  : This is going to become a problem once we support
dynamic allocated watchdog structs, as the unregister will unref the struct,
so it may be gone after the unregister, solution: store id in a local var
before calling unregister.


> diff --git a/drivers/watchdog/watchdog_dev.c b/drivers/watchdog/watchdog_dev.c
> index 930cc7c..b6666ab 100644
> --- a/drivers/watchdog/watchdog_dev.c
> +++ b/drivers/watchdog/watchdog_dev.c
> @@ -44,10 +44,10 @@
>
>   #include "watchdog_dev.h"
>
> -/* 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;
> +static struct watchdog_device *old_wdd;
> +/* the dev_t structure to store the dynamically allocated watchdog devices */
> +static dev_t watchdog_devt;
>
>   /*
>    *	watchdog_ping: ping the watchdog.
> @@ -138,6 +138,7 @@ static int watchdog_stop(struct watchdog_device *wddev)
>   static ssize_t watchdog_write(struct file *file, const char __user *data,
>   						size_t len, loff_t *ppos)
>   {
> +	struct watchdog_device *wdd = file->private_data;
>   	size_t i;
>   	char c;
>
> @@ -177,6 +178,7 @@ static ssize_t watchdog_write(struct file *file, const char __user *data,
>   static long watchdog_ioctl(struct file *file, unsigned int cmd,
>   							unsigned long arg)
>   {
> +	struct watchdog_device *wdd = file->private_data;
>   	void __user *argp = (void __user *)arg;
>   	int __user *p = argp;
>   	unsigned int val;
> @@ -249,11 +251,11 @@ static long watchdog_ioctl(struct file *file, unsigned int cmd,
>   }
>
>   /*
> - *	watchdog_open: open the /dev/watchdog device.
> + *	watchdog_open: open the /dev/watchdog* devices.
>    *	@inode: inode of device
>    *	@file: file handle to device
>    *
> - *	When the /dev/watchdog device gets opened, we start the watchdog.
> + *	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.
>    */
> @@ -261,6 +263,12 @@ static long watchdog_ioctl(struct file *file, unsigned int cmd,
>   static int watchdog_open(struct inode *inode, struct file *file)
>   {
>   	int err = -EBUSY;
> +	struct watchdog_device *wdd;
> +
> +	/* Get the corresponding watchdog device */
> +	if (imajor(inode) == MISC_MAJOR)
> +		wdd = old_wdd;
> +	else wdd = container_of(inode->i_cdev, struct watchdog_device, cdev);
>
>   	/* the watchdog is single open! */
>   	if (test_and_set_bit(WDOG_DEV_OPEN,&wdd->status))
> @@ -277,6 +285,8 @@ static int watchdog_open(struct inode *inode, struct file *file)
>   	if (err<  0)
>   		goto out_mod;
>
> +	file->private_data = wdd;
> +
>   	/* dev/watchdog is a virtual (and thus non-seekable) filesystem */
>   	return nonseekable_open(inode, file);
>
> @@ -288,9 +298,9 @@ out:
>   }
>
>   /*
> - *      watchdog_release: release the /dev/watchdog device.
> - *      @inode: inode of device
> - *      @file: file handle to device
> + *	watchdog_release: release the watchdog device.
> + *	@inode: inode of device
> + *	@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 (and nowayout
> @@ -299,6 +309,7 @@ out:
>
>   static int watchdog_release(struct inode *inode, struct file *file)
>   {
> +	struct watchdog_device *wdd = file->private_data;
>   	int err = -EBUSY;
>
>   	/*
> @@ -340,62 +351,90 @@ static struct miscdevice watchdog_miscdev = {
>   };
>
>   /*
> - *	watchdog_dev_register:
> + *	watchdog_dev_register: register a watchdog device
>    *	@watchdog: watchdog device
>    *
> - *	Register a watchdog device as /dev/watchdog. /dev/watchdog
> - *	is actually a miscdevice and thus we set it up like that.
> + *	Register a watchdog device including handling the legacy
> + *	/dev/watchdog node. /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;
> +	int err, devno;
> +
> +	if (watchdog->id == 0) {
> +		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);
> +			if (err == -EBUSY)
> +				pr_err("%s: a legacy watchdog module is probably present.\n",
> +					watchdog->info->identity);
> +			return err;
> +		}
> +		old_wdd = watchdog;
>   	}
>
> -	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;
> +	/* Fill in the data structures */
> +	devno = MKDEV(MAJOR(watchdog_devt), watchdog->id);
> +	cdev_init(&watchdog->cdev,&watchdog_fops);
> +	watchdog->cdev.owner = watchdog->ops->owner;
> +
> +	/* Add the device */
> +	err  = cdev_add(&watchdog->cdev, devno, 1);
> +	if (err) {
> +		pr_err("watchdog%d unable to add device %d:%d\n",
> +			watchdog->id,  MAJOR(watchdog_devt), watchdog->id);
> +		if (watchdog->id == 0) {
> +			misc_deregister(&watchdog_miscdev);
> +			old_wdd = NULL;
> +		}
>   	}
> -
> -	return 0;
> -
> -out:
> -	wdd = NULL;
> -	clear_bit(0,&watchdog_dev_busy);
>   	return err;
>   }
>
>   /*
> - *	watchdog_dev_unregister:
> + *	watchdog_dev_unregister: unregister a watchdog device
>    *	@watchdog: watchdog device
>    *
> - *	Deregister the /dev/watchdog device.
> + *	Unregister the watchdog and if needed the legacy /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;
> +	cdev_del(&watchdog->cdev);
> +	if (watchdog->id == 0) {
> +		misc_deregister(&watchdog_miscdev);
> +		old_wdd = NULL;
>   	}
> -
> -	misc_deregister(&watchdog_miscdev);
> -	wdd = NULL;
> -	clear_bit(0,&watchdog_dev_busy);
>   	return 0;
>   }
> +
> +/*
> + *	watchdog_init: module init call
> + *
> + *	Allocate a range of chardev nodes to use for watchdog devices
> + */
> +
> +int __init watchdog_init(void)
> +{
> +	int err = alloc_chrdev_region(&watchdog_devt, 0, MAX_DOGS, "watchdog");
> +	if (err<  0)
> +		pr_err("watchdog: unable to allocate char dev region\n");
> +	return err;
> +}
> +
> +/*
> + *	watchdog_exit: module exit
> + *
> + *	Release the range of chardev nodes used for watchdog devices
> + */
> +
> +void __exit watchdog_exit(void)
> +{
> +	unregister_chrdev_region(watchdog_devt, MAX_DOGS);
> +}
> +
> +module_init(watchdog_init);
> +module_exit(watchdog_exit);
> diff --git a/drivers/watchdog/watchdog_dev.h b/drivers/watchdog/watchdog_dev.h
> index bc7612b..0eb8f6f 100644
> --- a/drivers/watchdog/watchdog_dev.h
> +++ b/drivers/watchdog/watchdog_dev.h
> @@ -26,6 +26,8 @@
>    *	This material is provided "AS-IS" and at no charge.
>    */
>
> +#define MAX_DOGS	32	/* Maximum number of watchdog devices */
> +
>   /*
>    *	Functions/procedures to be called by the core
>    */
> diff --git a/include/linux/watchdog.h b/include/linux/watchdog.h
> index 1984ea6..fd648ea 100644
> --- a/include/linux/watchdog.h
> +++ b/include/linux/watchdog.h
> @@ -54,6 +54,8 @@ struct watchdog_info {
>   #ifdef __KERNEL__
>
>   #include<linux/bitops.h>
> +#include<linux/device.h>
> +#include<linux/cdev.h>
>
>   struct watchdog_ops;
>   struct watchdog_device;
> @@ -96,6 +98,8 @@ struct watchdog_ops {
>    * @min_timeout:The watchdog devices minimum timeout value.
>    * @max_timeout:The watchdog devices maximum timeout value.
>    * @driver-data:Pointer to the drivers private data.
> + * @id:		The watchdog's ID.
> + * @cdev:	The watchdog's Character device.
>    * @status:	Field that contains the devices internal status bits.
>    *
>    * The watchdog_device structure contains all information about a
> @@ -112,6 +116,8 @@ struct watchdog_device {
>   	unsigned int min_timeout;
>   	unsigned int max_timeout;
>   	void *driver_data;
> +	int id;
> +	struct cdev cdev;
>   	unsigned long status;
>   /* Bit numbers for status flags */
>   #define WDOG_ACTIVE		0	/* Is the watchdog running/active */


Regards,

Hans

Re: [PATCH 1/4] watchdog: Add multiple device support

[Hans de Goede]

Hi,

<snip>

>> @@ -102,6 +122,7 @@ void watchdog_unregister_device(struct watchdog_device *wdd)
>> ret = watchdog_dev_unregister(wdd);
>> if (ret)
>> pr_err("error unregistering /dev/watchdog (err=%d)\n", ret);
>> + ida_simple_remove(&watchdog_ida, wdd->id);
>> }
>> EXPORT_SYMBOL_GPL(watchdog_unregister_device);
>>
>
> A note to myself :) : This is going to become a problem once we support
> dynamic allocated watchdog structs, as the unregister will unref the struct,
> so it may be gone after the unregister, solution: store id in a local var
> before calling unregister.

Scrap that my support for dynamically allocated watchdog_dev structs patch does
not do an automatic unref on unregister, instead it expects the caller of
unregister to manually do the unref itself (for drivers which use the ref
counting), which actually is better, as it does not cause problems like the
above.

Regards,

Hans

Re: [PATCH 1/4] watchdog: Add multiple device support

[Hans de Goede]

Hi,

On 05/10/2012 09:20 PM, Wim Van Sebroeck wrote:
> Hi Alan, Hans,
>
>>> We keep the old /dev/watchdog interface file for the first watchdog via
>>> miscdev. This is basically a cut and paste of the relevant interface code
>>> from the rtc driver layer tweaked for watchdog.
>>>
>>> Revised to fix problems noted by Hans de Goede
>>>
>>> Signed-off-by: Alan Cox<alan@linux.intel.com>
>>
>> I'm ok with the principle. Need some more thoughts about the code (meaning: that the ID assignment should imho be in the watchdog_core.c file).
>>
>> I do have one question though: why did you use your own assign_id routines and not ida_simple_get/ida_simple_remove for instance?
>
> I re-tweaked it to fit the clean split between core and dev related stuff. I also used ida instead of the watchdog_assign/release_id code.
> For the rest it's basically the code that Alan produced with some minor changes (and also with the comments that Hans made).
>
> See attached the diff (I will need to add the kernel-api documentation update still).
> Please comment and test.

I've rebased Alan's other patches (they needed some adjustments) and my own patches on top
and run the results through several tests. Everything looks good.

I'll resend the rebased patches (both Alan's other patches and mine), right after this
mail. I've also thrown in the missing documentation update.

Regards,

Hans

Re: [PATCH 1/4] watchdog: Add multiple device support

[Wim Van Sebroeck]

Hi Hans,

> >+		if (id != 0)
> >+			return ret;
> 
> 
> I believe the check above should be:
> 		if (id != 0 || ret != -EBUSY)
> 			return ret;
> 
> IOW we should not retry the registration if it failed for another reason
> then the id already being taken by a legacy driver.

Than for readibility we should say:
		if (!(id == 0 && ret == -EBUSY))
			return ret

Kind regards,
Wim.

Re: [PATCH 1/4] watchdog: Add multiple device support

[Hans de Goede]

Hi,

On 05/11/2012 06:02 PM, Wim Van Sebroeck wrote:
> Hi Hans,
>
>>> +		if (id != 0)
>>> +			return ret;
>>
>>
>> I believe the check above should be:
>> 		if (id != 0 || ret != -EBUSY)
>> 			return ret;
>>
>> IOW we should not retry the registration if it failed for another reason
>> then the id already being taken by a legacy driver.
>
> Than for readibility we should say:
> 		if (!(id == 0&&  ret == -EBUSY))
> 			return ret

ACK.

Regards,

Hans

Re: [PATCH 1/4] watchdog: Add multiple device support

[Tomas Winkler]

On Fri, May 11, 2012 at 7:40 PM, Hans de Goede <hdegoede@redhat.com> wrote:
>
> Hi,
>
>
> On 05/11/2012 06:02 PM, Wim Van Sebroeck wrote:
>>
>> Hi Hans,
>>
>>>> +               if (id != 0)
>>>> +                       return ret;
>>>
>>>
>>>
>>> I believe the check above should be:
>>>                if (id != 0 || ret != -EBUSY)
>>>                        return ret;
>>>
>>> IOW we should not retry the registration if it failed for another reason
>>> then the id already being taken by a legacy driver.
>>
>>
>> Than for readibility we should say:
>>                if (!(id == 0&&  ret == -EBUSY))
>>                        return ret
>
>
> ACK.
>
> Regards,
>
> Hans
+ if (imajor(inode) == MISC_MAJOR)
+               wdd = old_wdd;
+       else wdd = container_of(inode->i_cdev, struct watchdog_device, cdev);

You should break the line after 'else'

+       else
+          wdd = container_of(inode->i_cdev, struct watchdog_device, cdev);

Thanks
Tomas
--
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

Re: [PATCH 1/4] watchdog: Add multiple device support

[Wim Van Sebroeck]

Hi Tomas,

> + if (imajor(inode) == MISC_MAJOR)
> +               wdd = old_wdd;
> +       else wdd = container_of(inode->i_cdev, struct watchdog_device, cdev);
> 
> You should break the line after 'else'
> 
> +       else
> +          wdd = container_of(inode->i_cdev, struct watchdog_device, cdev);

Will change this.

Kind regards,
Wim.