Watchdog timer core enhancements + sch5636 watchdog driver

Watchdog timer core enhancements + sch5636 watchdog driver

[Hans de Goede]

Hi,

Short selfintro for the watchdog people: I'm a FOSS developer / enthusiast
for close to 15 years now. I've written Linux kernel drivers for a ton
of webcams and various hwmon IC's.

Two of the hwmon drivers I maintain are for Fujitsu manufacturered IC's which
include a watchdog part in their hwmon solution. The fschmd driver for their
family of i2c hwmon IC's already supports this using the standard watchdog API.

While waiting for some input from Fujitsu needed to get the watchdog in their
new SMSC SCH5636 superio (with a programmable micro controller) based hwmon
solution working, I read the announcement of the watchdog timer driver core.

I'm really happy to see the watchdog timer driver core, as I just copy and
pasted a whole bunch of code from the fschmd driver to the sch5636 driver,
having a standard watchdog core definitely is a good idea.

So when working on finishing up my watchdog support for the sch5636 driver
I decided to convert it to the new watchdog timer driver core.

I hit 2 issues during this (and 2 minor others):
1) The driver core only works for 1 watchdog, but both my fschmd and my sch5636
test systems also have an iTCO watchdog and that tends to load first. Thus when
writing the fschmd watchdog code I made it support loading either on the
default watchdog minor, or on one of the additional reserved watchdog minors
(212-215). The second patch in this series adds support for this to the
watchdog timer driver core (which was quite straightforward to add).

2) Most Linux drivers use dynamically allocated memory for the driver data,
which gets allocated on calling the probe function and released on driver
unbind. This means that merely locking the module containing the driver as long
as /dev/watchdog is open is not sufficient, since the driver can still be
unbound. The third patch in this series fixes this.

The other 2 patches to the watchdog core fix 2 minor issues and are self
explanatory. The 5th patch in the series adds support for the integrated
watchdog to the sch5636 driver. It seems this will be the first driver
actually using the new core :)

I think it is best for this entire series to go in through the watchdog tree,
since the 5th patch depends on the others (even though the 5th patch touches
hwmon code).

Once this is upstream I'll also write a patch to convert the fschmd driver to
use the watchdog core, and maybe also the iTCO driver.

Regards,

Hans

[lm-sensors] Watchdog timer core enhancements + sch5636 watchdog

[Hans de Goede]

Hi,

Short selfintro for the watchdog people: I'm a FOSS developer / enthusiast
for close to 15 years now. I've written Linux kernel drivers for a ton
of webcams and various hwmon IC's.

Two of the hwmon drivers I maintain are for Fujitsu manufacturered IC's which
include a watchdog part in their hwmon solution. The fschmd driver for their
family of i2c hwmon IC's already supports this using the standard watchdog API.

While waiting for some input from Fujitsu needed to get the watchdog in their
new SMSC SCH5636 superio (with a programmable micro controller) based hwmon
solution working, I read the announcement of the watchdog timer driver core.

I'm really happy to see the watchdog timer driver core, as I just copy and
pasted a whole bunch of code from the fschmd driver to the sch5636 driver,
having a standard watchdog core definitely is a good idea.

So when working on finishing up my watchdog support for the sch5636 driver
I decided to convert it to the new watchdog timer driver core.

I hit 2 issues during this (and 2 minor others):
1) The driver core only works for 1 watchdog, but both my fschmd and my sch5636
test systems also have an iTCO watchdog and that tends to load first. Thus when
writing the fschmd watchdog code I made it support loading either on the
default watchdog minor, or on one of the additional reserved watchdog minors
(212-215). The second patch in this series adds support for this to the
watchdog timer driver core (which was quite straightforward to add).

2) Most Linux drivers use dynamically allocated memory for the driver data,
which gets allocated on calling the probe function and released on driver
unbind. This means that merely locking the module containing the driver as long
as /dev/watchdog is open is not sufficient, since the driver can still be
unbound. The third patch in this series fixes this.

The other 2 patches to the watchdog core fix 2 minor issues and are self
explanatory. The 5th patch in the series adds support for the integrated
watchdog to the sch5636 driver. It seems this will be the first driver
actually using the new core :)

I think it is best for this entire series to go in through the watchdog tree,
since the 5th patch depends on the others (even though the 5th patch touches
hwmon code).

Once this is upstream I'll also write a patch to convert the fschmd driver to
use the watchdog core, and maybe also the iTCO driver.

Regards,

Hans

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

[PATCH 1/5] watchdog_dev: Use wddev function parameter instead of wdd global

[Hans de Goede]

Various functions in watchdog_dev take the dev to operate on as
a parameter, yet they do some operations on the global wdd pointer, this
patch fixes this.

This is a preperation patch for adding support for multiple watchdogs.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/watchdog/watchdog_dev.c |   14 +++++++-------
 1 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/watchdog/watchdog_dev.c b/drivers/watchdog/watchdog_dev.c
index d33520d..1199da0 100644
--- a/drivers/watchdog/watchdog_dev.c
+++ b/drivers/watchdog/watchdog_dev.c
@@ -59,7 +59,7 @@ static struct watchdog_device *wdd;
 
 static int watchdog_ping(struct watchdog_device *wddev)
 {
-	if (test_bit(WDOG_ACTIVE, &wdd->status)) {
+	if (test_bit(WDOG_ACTIVE, &wddev->status)) {
 		if (wddev->ops->ping)
 			return wddev->ops->ping(wddev);  /* ping the watchdog */
 		else
@@ -81,12 +81,12 @@ static int watchdog_start(struct watchdog_device *wddev)
 {
 	int err;
 
-	if (!test_bit(WDOG_ACTIVE, &wdd->status)) {
+	if (!test_bit(WDOG_ACTIVE, &wddev->status)) {
 		err = wddev->ops->start(wddev);
 		if (err < 0)
 			return err;
 
-		set_bit(WDOG_ACTIVE, &wdd->status);
+		set_bit(WDOG_ACTIVE, &wddev->status);
 	}
 	return 0;
 }
@@ -105,18 +105,18 @@ static int watchdog_stop(struct watchdog_device *wddev)
 {
 	int err = -EBUSY;
 
-	if (test_bit(WDOG_NO_WAY_OUT, &wdd->status)) {
+	if (test_bit(WDOG_NO_WAY_OUT, &wddev->status)) {
 		pr_info("%s: nowayout prevents watchdog to be stopped!\n",
-							wdd->info->identity);
+							wddev->info->identity);
 		return err;
 	}
 
-	if (test_bit(WDOG_ACTIVE, &wdd->status)) {
+	if (test_bit(WDOG_ACTIVE, &wddev->status)) {
 		err = wddev->ops->stop(wddev);
 		if (err < 0)
 			return err;
 
-		clear_bit(WDOG_ACTIVE, &wdd->status);
+		clear_bit(WDOG_ACTIVE, &wddev->status);
 	}
 	return 0;
 }
-- 
1.7.6.2

[lm-sensors] [PATCH 1/5] watchdog_dev: Use wddev function parameter

[Hans de Goede]

Various functions in watchdog_dev take the dev to operate on as
a parameter, yet they do some operations on the global wdd pointer, this
patch fixes this.

This is a preperation patch for adding support for multiple watchdogs.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/watchdog/watchdog_dev.c |   14 +++++++-------
 1 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/watchdog/watchdog_dev.c b/drivers/watchdog/watchdog_dev.c
index d33520d..1199da0 100644
--- a/drivers/watchdog/watchdog_dev.c
+++ b/drivers/watchdog/watchdog_dev.c
@@ -59,7 +59,7 @@ static struct watchdog_device *wdd;
 
 static int watchdog_ping(struct watchdog_device *wddev)
 {
-	if (test_bit(WDOG_ACTIVE, &wdd->status)) {
+	if (test_bit(WDOG_ACTIVE, &wddev->status)) {
 		if (wddev->ops->ping)
 			return wddev->ops->ping(wddev);  /* ping the watchdog */
 		else
@@ -81,12 +81,12 @@ static int watchdog_start(struct watchdog_device *wddev)
 {
 	int err;
 
-	if (!test_bit(WDOG_ACTIVE, &wdd->status)) {
+	if (!test_bit(WDOG_ACTIVE, &wddev->status)) {
 		err = wddev->ops->start(wddev);
 		if (err < 0)
 			return err;
 
-		set_bit(WDOG_ACTIVE, &wdd->status);
+		set_bit(WDOG_ACTIVE, &wddev->status);
 	}
 	return 0;
 }
@@ -105,18 +105,18 @@ static int watchdog_stop(struct watchdog_device *wddev)
 {
 	int err = -EBUSY;
 
-	if (test_bit(WDOG_NO_WAY_OUT, &wdd->status)) {
+	if (test_bit(WDOG_NO_WAY_OUT, &wddev->status)) {
 		pr_info("%s: nowayout prevents watchdog to be stopped!\n",
-							wdd->info->identity);
+							wddev->info->identity);
 		return err;
 	}
 
-	if (test_bit(WDOG_ACTIVE, &wdd->status)) {
+	if (test_bit(WDOG_ACTIVE, &wddev->status)) {
 		err = wddev->ops->stop(wddev);
 		if (err < 0)
 			return err;
 
-		clear_bit(WDOG_ACTIVE, &wdd->status);
+		clear_bit(WDOG_ACTIVE, &wddev->status);
 	}
 	return 0;
 }
-- 
1.7.6.2


_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

[PATCH 2/5] watchdog_dev: Add support for having more then 1 watchdog

[Hans de Goede]

I know that having more then one watchdog is not really usefull, but some
systems ship with more then one watchdog, so why not make all of them
available to the user. Maybe he wants to use a certain one because it
has additional features / a better resolution ... ?

Also having this support in watchdog_dev makes developing watchdog drivers
for watchdog hardware which happens to often be found on systems with
multiple watchdogs a lot easier.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/watchdog/watchdog_dev.c |  137 +++++++++++++++++++++++++++------------
 1 files changed, 96 insertions(+), 41 deletions(-)

diff --git a/drivers/watchdog/watchdog_dev.c b/drivers/watchdog/watchdog_dev.c
index 1199da0..6483327 100644
--- a/drivers/watchdog/watchdog_dev.c
+++ b/drivers/watchdog/watchdog_dev.c
@@ -42,10 +42,49 @@
 #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;
+#define MAX_WATCHDOGS 5
+
+static int watchdog_open(struct inode *inode, struct file *file);
+static int watchdog_release(struct inode *inode, struct file *file);
+static ssize_t watchdog_write(struct file *file, const char __user *data,
+						size_t len, loff_t *ppos);
+static long watchdog_ioctl(struct file *file, unsigned int cmd,
+						unsigned long arg);
+
+/* the watchdog devices behind /dev/watchdog* */
+static DEFINE_MUTEX(wdd_list_mutex);
+static struct watchdog_device *wdd_list[MAX_WATCHDOGS];
+
+static const struct file_operations watchdog_fops = {
+	.owner		= THIS_MODULE,
+	.write		= watchdog_write,
+	.unlocked_ioctl	= watchdog_ioctl,
+	.open		= watchdog_open,
+	.release	= watchdog_release,
+};
+
+static struct miscdevice watchdog_miscdev[MAX_WATCHDOGS] = { {
+	.minor		= WATCHDOG_MINOR,
+	.name		= "watchdog",
+	.fops		= &watchdog_fops,
+}, {
+	.minor		= 212,
+	.name		= "watchdog1",
+	.fops		= &watchdog_fops,
+}, {
+	.minor		= 213,
+	.name		= "watchdog2",
+	.fops		= &watchdog_fops,
+}, {
+	.minor		= 214,
+	.name		= "watchdog3",
+	.fops		= &watchdog_fops,
+}, {
+	.minor		= 215,
+	.name		= "watchdog4",
+	.fops		= &watchdog_fops,
+} };
+
 
 /*
  *	watchdog_ping: ping the watchdog.
@@ -136,6 +175,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 +215,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;
@@ -254,7 +295,15 @@ static long watchdog_ioctl(struct file *file, unsigned int cmd,
 
 static int watchdog_open(struct inode *inode, struct file *file)
 {
-	int err = -EBUSY;
+	int idx, err = -EBUSY;
+	struct watchdog_device *wdd = NULL;
+
+	/* Find our watchdog_device */
+	for (idx = 0; idx < MAX_WATCHDOGS; idx++)
+		if (watchdog_miscdev[idx].minor == iminor(inode)) {
+			wdd = wdd_list[idx];
+			break;
+		}
 
 	/* the watchdog is single open! */
 	if (test_and_set_bit(WDOG_DEV_OPEN, &wdd->status))
@@ -271,6 +320,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);
 
@@ -293,6 +344,7 @@ out:
 
 static int watchdog_release(struct inode *inode, struct file *file)
 {
+	struct watchdog_device *wdd = file->private_data;
 	int err = -EBUSY;
 
 	/*
@@ -319,20 +371,6 @@ static int watchdog_release(struct inode *inode, struct file *file)
 	return 0;
 }
 
-static const struct file_operations watchdog_fops = {
-	.owner		= THIS_MODULE,
-	.write		= watchdog_write,
-	.unlocked_ioctl	= watchdog_ioctl,
-	.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
@@ -343,28 +381,44 @@ static struct miscdevice watchdog_miscdev = {
 
 int watchdog_dev_register(struct watchdog_device *watchdog)
 {
-	int err;
+	int idx, 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;
+	/*
+	 * Note ideally we would just walk our wdd_list here and stop at the
+	 * first place which holds a NULL, but that assumes all watchdog
+	 * drivers use the watchdog_core, which is not true, hence the retry.
+	 * Once all drivers are converted, the check for EBUSY and goto retry
+	 * can be eliminated (and the while converted to a standard for loop).
+	 */
+	idx = 0;
+retry:
+	mutex_lock(&wdd_list_mutex);
+	while (idx < MAX_WATCHDOGS) {
+		if (wdd_list[idx] == NULL) {
+			wdd_list[idx] = watchdog;
+			break;
+		}
+		idx++;
 	}
+	mutex_unlock(&wdd_list_mutex);
 
-	wdd = watchdog;
+	if (idx == MAX_WATCHDOGS) {
+		pr_err("maximum number of watchdogs exceeded.\n");
+		return -EBUSY;
+	}
 
-	err = misc_register(&watchdog_miscdev);
+	err = misc_register(&watchdog_miscdev[idx]);
 	if (err != 0) {
+		wdd_list[idx] = NULL;
+		if (err == -EBUSY) {
+			idx++; /* Try again with the next watchdog minor */
+			goto retry;
+		}
 		pr_err("%s: cannot register miscdev on minor=%d (err=%d).\n",
-			watchdog->info->identity, WATCHDOG_MINOR, err);
-		goto out;
+			watchdog->info->identity, watchdog_miscdev[idx].minor,
+			err);
 	}
 
-	return 0;
-
-out:
-	wdd = NULL;
-	clear_bit(0, &watchdog_dev_busy);
 	return err;
 }
 
@@ -377,19 +431,20 @@ out:
 
 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;
+	int idx;
+
+	/* No need to lock */
+	for (idx = 0; idx < MAX_WATCHDOGS; idx++)
+		if (wdd_list[idx] == watchdog)
+			break;
 
-	/* We can only unregister the watchdog device that was registered */
-	if (watchdog != wdd) {
+	if (idx == MAX_WATCHDOGS) {
 		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);
+	misc_deregister(&watchdog_miscdev[idx]);
+	wdd_list[idx] = NULL;
 	return 0;
 }
-- 
1.7.6.2

[lm-sensors] [PATCH 2/5] watchdog_dev: Add support for having more

[Hans de Goede]

I know that having more then one watchdog is not really usefull, but some
systems ship with more then one watchdog, so why not make all of them
available to the user. Maybe he wants to use a certain one because it
has additional features / a better resolution ... ?

Also having this support in watchdog_dev makes developing watchdog drivers
for watchdog hardware which happens to often be found on systems with
multiple watchdogs a lot easier.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/watchdog/watchdog_dev.c |  137 +++++++++++++++++++++++++++------------
 1 files changed, 96 insertions(+), 41 deletions(-)

diff --git a/drivers/watchdog/watchdog_dev.c b/drivers/watchdog/watchdog_dev.c
index 1199da0..6483327 100644
--- a/drivers/watchdog/watchdog_dev.c
+++ b/drivers/watchdog/watchdog_dev.c
@@ -42,10 +42,49 @@
 #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;
+#define MAX_WATCHDOGS 5
+
+static int watchdog_open(struct inode *inode, struct file *file);
+static int watchdog_release(struct inode *inode, struct file *file);
+static ssize_t watchdog_write(struct file *file, const char __user *data,
+						size_t len, loff_t *ppos);
+static long watchdog_ioctl(struct file *file, unsigned int cmd,
+						unsigned long arg);
+
+/* the watchdog devices behind /dev/watchdog* */
+static DEFINE_MUTEX(wdd_list_mutex);
+static struct watchdog_device *wdd_list[MAX_WATCHDOGS];
+
+static const struct file_operations watchdog_fops = {
+	.owner		= THIS_MODULE,
+	.write		= watchdog_write,
+	.unlocked_ioctl	= watchdog_ioctl,
+	.open		= watchdog_open,
+	.release	= watchdog_release,
+};
+
+static struct miscdevice watchdog_miscdev[MAX_WATCHDOGS] = { {
+	.minor		= WATCHDOG_MINOR,
+	.name		= "watchdog",
+	.fops		= &watchdog_fops,
+}, {
+	.minor		= 212,
+	.name		= "watchdog1",
+	.fops		= &watchdog_fops,
+}, {
+	.minor		= 213,
+	.name		= "watchdog2",
+	.fops		= &watchdog_fops,
+}, {
+	.minor		= 214,
+	.name		= "watchdog3",
+	.fops		= &watchdog_fops,
+}, {
+	.minor		= 215,
+	.name		= "watchdog4",
+	.fops		= &watchdog_fops,
+} };
+
 
 /*
  *	watchdog_ping: ping the watchdog.
@@ -136,6 +175,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 +215,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;
@@ -254,7 +295,15 @@ static long watchdog_ioctl(struct file *file, unsigned int cmd,
 
 static int watchdog_open(struct inode *inode, struct file *file)
 {
-	int err = -EBUSY;
+	int idx, err = -EBUSY;
+	struct watchdog_device *wdd = NULL;
+
+	/* Find our watchdog_device */
+	for (idx = 0; idx < MAX_WATCHDOGS; idx++)
+		if (watchdog_miscdev[idx].minor = iminor(inode)) {
+			wdd = wdd_list[idx];
+			break;
+		}
 
 	/* the watchdog is single open! */
 	if (test_and_set_bit(WDOG_DEV_OPEN, &wdd->status))
@@ -271,6 +320,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);
 
@@ -293,6 +344,7 @@ out:
 
 static int watchdog_release(struct inode *inode, struct file *file)
 {
+	struct watchdog_device *wdd = file->private_data;
 	int err = -EBUSY;
 
 	/*
@@ -319,20 +371,6 @@ static int watchdog_release(struct inode *inode, struct file *file)
 	return 0;
 }
 
-static const struct file_operations watchdog_fops = {
-	.owner		= THIS_MODULE,
-	.write		= watchdog_write,
-	.unlocked_ioctl	= watchdog_ioctl,
-	.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
@@ -343,28 +381,44 @@ static struct miscdevice watchdog_miscdev = {
 
 int watchdog_dev_register(struct watchdog_device *watchdog)
 {
-	int err;
+	int idx, 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;
+	/*
+	 * Note ideally we would just walk our wdd_list here and stop at the
+	 * first place which holds a NULL, but that assumes all watchdog
+	 * drivers use the watchdog_core, which is not true, hence the retry.
+	 * Once all drivers are converted, the check for EBUSY and goto retry
+	 * can be eliminated (and the while converted to a standard for loop).
+	 */
+	idx = 0;
+retry:
+	mutex_lock(&wdd_list_mutex);
+	while (idx < MAX_WATCHDOGS) {
+		if (wdd_list[idx] = NULL) {
+			wdd_list[idx] = watchdog;
+			break;
+		}
+		idx++;
 	}
+	mutex_unlock(&wdd_list_mutex);
 
-	wdd = watchdog;
+	if (idx = MAX_WATCHDOGS) {
+		pr_err("maximum number of watchdogs exceeded.\n");
+		return -EBUSY;
+	}
 
-	err = misc_register(&watchdog_miscdev);
+	err = misc_register(&watchdog_miscdev[idx]);
 	if (err != 0) {
+		wdd_list[idx] = NULL;
+		if (err = -EBUSY) {
+			idx++; /* Try again with the next watchdog minor */
+			goto retry;
+		}
 		pr_err("%s: cannot register miscdev on minor=%d (err=%d).\n",
-			watchdog->info->identity, WATCHDOG_MINOR, err);
-		goto out;
+			watchdog->info->identity, watchdog_miscdev[idx].minor,
+			err);
 	}
 
-	return 0;
-
-out:
-	wdd = NULL;
-	clear_bit(0, &watchdog_dev_busy);
 	return err;
 }
 
@@ -377,19 +431,20 @@ out:
 
 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;
+	int idx;
+
+	/* No need to lock */
+	for (idx = 0; idx < MAX_WATCHDOGS; idx++)
+		if (wdd_list[idx] = watchdog)
+			break;
 
-	/* We can only unregister the watchdog device that was registered */
-	if (watchdog != wdd) {
+	if (idx = MAX_WATCHDOGS) {
 		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);
+	misc_deregister(&watchdog_miscdev[idx]);
+	wdd_list[idx] = NULL;
 	return 0;
 }
-- 
1.7.6.2


_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

[PATCH 3/5] watchdog_dev: Add support for dynamically allocated watchdog_device structs

[Hans de Goede]

If a driver's watchdog_device struct is part of a dynamically allocated
struct (which it often will be), merely locking the module is not enough,
even with a drivers module locked, the driver can be unbound from the device,
examples:
1) The root user can unbind it through sysfd
2) The i2c bus master driver being unloaded for an i2c watchdog

I will gladly admit that these are corner cases, but we still need to handle
them correctly.

The fix for this consists of 2 parts:
1) Add ref / unref operations, so that the driver can refcount the struct
   holding the watchdog_device struct and delay freeing it until any
   open filehandles referring to it are closed
2) Most driver operations will do IO on the device and the driver should not
   do any IO on the device after it has been unbound. Rather then letting each
   driver deal with this internally, it is better to ensure at the watchdog
   core level that no operations (other then unref) will get called after
   the driver has called watchdog_unregister_device(). This actually is the
   bulk of this patch.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 Documentation/watchdog/watchdog-kernel-api.txt |   26 +++-
 drivers/watchdog/watchdog_dev.c                |  186 +++++++++++++++++++-----
 include/linux/watchdog.h                       |    8 +
 3 files changed, 185 insertions(+), 35 deletions(-)

diff --git a/Documentation/watchdog/watchdog-kernel-api.txt b/Documentation/watchdog/watchdog-kernel-api.txt
index 4f7c894..1d3f020 100644
--- a/Documentation/watchdog/watchdog-kernel-api.txt
+++ b/Documentation/watchdog/watchdog-kernel-api.txt
@@ -1,6 +1,6 @@
 The Linux WatchDog Timer Driver Core kernel API.
 ===============================================
-Last reviewed: 22-Jul-2011
+Last reviewed: 10-Sep-2011
 
 Wim Van Sebroeck <wim@iguana.be>
 
@@ -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 mutex lock;
 	unsigned int bootstatus;
 	unsigned int timeout;
 	unsigned int min_timeout;
@@ -53,6 +54,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.
+* lock: Mutex for WatchDog Timer Driver Core internal use only.
 * 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).
@@ -74,6 +76,8 @@ struct watchdog_ops {
 	int (*start)(struct watchdog_device *);
 	int (*stop)(struct watchdog_device *);
 	/* optional operations */
+	void (*ref)(struct watchdog_device *);
+	void (*unref)(struct watchdog_device *);
 	int (*ping)(struct watchdog_device *);
 	unsigned int (*status)(struct watchdog_device *);
 	int (*set_timeout)(struct watchdog_device *, unsigned int);
@@ -84,6 +88,21 @@ 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).
+
+If the watchdog_device struct is dynamically allocated, just locking the module
+is not enough and a driver also needs to define the ref and unref operations to
+ensure the structure holding the watchdog_device does not go away.
+
+The simplest (and usually sufficient) implementation of this is to:
+1) Add a kref struct to the same structure which is holding the watchdog_device
+2) Define a release callback for the kref which frees the struct holding both
+3) Call kref_init on this kref *before* calling watchdog_register_device()
+4) Define a ref operation calling kref_get on this kref
+5) Define a unref operation calling kref_put on this kref
+6) When it is time to cleanup:
+ * Do not kfree() the struct holding both, the last kref_put will do this!
+ * *After* calling watchdog_unregister_device() call kref_put on the kref
+
 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
@@ -141,6 +160,11 @@ bit-operations. The status bits that are defined are:
   (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.
+* WDOG_UNREGISTERED: this bit gets set by the WatchDog Timer Driver Core
+  after calling watchdog_unregister_device, and then checked before calling
+  any watchdog_ops, so that you can be sure that no operations (other then
+  unref) will get called after unregister, even if userspace still holds a
+  reference to /dev/watchdog
 
 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
diff --git a/drivers/watchdog/watchdog_dev.c b/drivers/watchdog/watchdog_dev.c
index 6483327..15dc3df 100644
--- a/drivers/watchdog/watchdog_dev.c
+++ b/drivers/watchdog/watchdog_dev.c
@@ -98,13 +98,24 @@ static struct miscdevice watchdog_miscdev[MAX_WATCHDOGS] = { {
 
 static int watchdog_ping(struct watchdog_device *wddev)
 {
-	if (test_bit(WDOG_ACTIVE, &wddev->status)) {
-		if (wddev->ops->ping)
-			return wddev->ops->ping(wddev);  /* ping the watchdog */
-		else
-			return wddev->ops->start(wddev); /* restart watchdog */
+	int ret;
+
+	if (!test_bit(WDOG_ACTIVE, &wddev->status))
+		return 0;
+
+	mutex_lock(&wddev->lock);
+	if (test_bit(WDOG_UNREGISTERED, &wddev->status)) {
+		ret = -ENODEV;
+		goto leave;
 	}
-	return 0;
+
+	if (wddev->ops->ping)
+		ret = wddev->ops->ping(wddev);  /* ping the watchdog */
+	else
+		ret = wddev->ops->start(wddev); /* restart watchdog */
+leave:
+	mutex_unlock(&wddev->lock);
+	return ret;
 }
 
 /*
@@ -118,16 +129,23 @@ static int watchdog_ping(struct watchdog_device *wddev)
 
 static int watchdog_start(struct watchdog_device *wddev)
 {
-	int err;
+	int ret;
 
-	if (!test_bit(WDOG_ACTIVE, &wddev->status)) {
-		err = wddev->ops->start(wddev);
-		if (err < 0)
-			return err;
+	if (test_bit(WDOG_ACTIVE, &wddev->status))
+		return 0;
 
-		set_bit(WDOG_ACTIVE, &wddev->status);
+	mutex_lock(&wddev->lock);
+	if (test_bit(WDOG_UNREGISTERED, &wddev->status)) {
+		ret = -ENODEV;
+		goto leave;
 	}
-	return 0;
+
+	ret = wddev->ops->start(wddev);
+	if (ret == 0)
+		set_bit(WDOG_ACTIVE, &wddev->status);
+leave:
+	mutex_unlock(&wddev->lock);
+	return ret;
 }
 
 /*
@@ -142,22 +160,116 @@ static int watchdog_start(struct watchdog_device *wddev)
 
 static int watchdog_stop(struct watchdog_device *wddev)
 {
-	int err = -EBUSY;
+	int ret;
+
+	if (!test_bit(WDOG_ACTIVE, &wddev->status))
+		return 0;
 
 	if (test_bit(WDOG_NO_WAY_OUT, &wddev->status)) {
 		pr_info("%s: nowayout prevents watchdog to be stopped!\n",
 							wddev->info->identity);
-		return err;
+		return -EBUSY;
 	}
 
-	if (test_bit(WDOG_ACTIVE, &wddev->status)) {
-		err = wddev->ops->stop(wddev);
-		if (err < 0)
-			return err;
+	mutex_lock(&wddev->lock);
+	if (test_bit(WDOG_UNREGISTERED, &wddev->status)) {
+		ret = -ENODEV;
+		goto leave;
+	}
 
+	ret = wddev->ops->stop(wddev);
+	if (ret == 0)
 		clear_bit(WDOG_ACTIVE, &wddev->status);
+leave:
+	mutex_unlock(&wddev->lock);
+	return ret;
+}
+
+/*
+ *	watchdog_ioctl_op: call the watchdog drivers ioctl op if defined
+ *	@wddev: the watchdog device to do the ioctl on
+ *	@cmd: watchdog command
+ *	@arg: argument pointer
+ */
+
+static int watchdog_ioctl_op(struct watchdog_device *wddev, unsigned int cmd,
+					unsigned long arg)
+{
+	int ret;
+
+	if (!wddev->ops->ioctl)
+		return -ENOIOCTLCMD;
+
+	mutex_lock(&wddev->lock);
+	if (test_bit(WDOG_UNREGISTERED, &wddev->status)) {
+		ret = -ENODEV;
+		goto leave;
 	}
-	return 0;
+
+	ret = wddev->ops->ioctl(wddev, cmd, arg);
+leave:
+	mutex_unlock(&wddev->lock);
+	return ret;
+}
+
+/*
+ *	watchdog_get_status: get the watchdog status
+ *	@wddev: the watchdog device to get the status from
+ */
+
+static int watchdog_get_status(struct watchdog_device *wddev,
+					unsigned int *status)
+{
+	int ret = 0;
+
+	*status = 0;
+	if (!wddev->ops->status)
+		return 0;
+
+	mutex_lock(&wddev->lock);
+	if (test_bit(WDOG_UNREGISTERED, &wddev->status)) {
+		ret = -ENODEV;
+		goto leave;
+	}
+
+	*status = wddev->ops->status(wddev);
+leave:
+	mutex_unlock(&wddev->lock);
+	return ret;
+}
+
+/*
+ *	watchdog_set_timeout: set the watchdog timer timeout
+ *	@wddev: the watchdog device to set the timeout for
+ *	@timeout: timeout to set in seconds
+ *	@arg: argument pointer
+ */
+
+static int watchdog_set_timeout(struct watchdog_device *wddev,
+					unsigned int timeout)
+{
+	int ret;
+
+	if ((wddev->ops->set_timeout == NULL) ||
+	    !(wddev->info->options & WDIOF_SETTIMEOUT))
+		return -EOPNOTSUPP;
+
+	if ((wddev->max_timeout != 0) &&
+	    (timeout < wddev->min_timeout || timeout > wddev->max_timeout))
+		return -EINVAL;
+
+	mutex_lock(&wddev->lock);
+	if (test_bit(WDOG_UNREGISTERED, &wddev->status)) {
+		ret = -ENODEV;
+		goto leave;
+	}
+
+	ret = wddev->ops->set_timeout(wddev, timeout);
+	if (ret == 0)
+		wddev->timeout = timeout;
+leave:
+	mutex_unlock(&wddev->lock);
+	return ret;
 }
 
 /*
@@ -221,18 +333,18 @@ 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;
-	}
+	err = watchdog_ioctl_op(wdd, cmd, arg);
+	if (err != -ENOIOCTLCMD)
+		return err;
 
 	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;
+		err = watchdog_get_status(wdd, &val);
+		if (err)
+			return err;
 		return put_user(val, p);
 	case WDIOC_GETBOOTSTATUS:
 		return put_user(wdd->bootstatus, p);
@@ -256,18 +368,11 @@ static long watchdog_ioctl(struct file *file, unsigned int cmd,
 		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;
-		if ((wdd->max_timeout != 0) &&
-		    (val < wdd->min_timeout || val > wdd->max_timeout))
-				return -EINVAL;
-		err = wdd->ops->set_timeout(wdd, val);
+		err = watchdog_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) */
@@ -321,6 +426,8 @@ static int watchdog_open(struct inode *inode, struct file *file)
 		goto out_mod;
 
 	file->private_data = wdd;
+	if (wdd->ops->ref)
+		wdd->ops->ref(wdd);
 
 	/* dev/watchdog is a virtual (and thus non-seekable) filesystem */
 	return nonseekable_open(inode, file);
@@ -368,6 +475,10 @@ static int watchdog_release(struct inode *inode, struct file *file)
 	/* make sure that /dev/watchdog can be re-opened */
 	clear_bit(WDOG_DEV_OPEN, &wdd->status);
 
+	/* Note wdd may be gone after this, do not use after this! */
+	if (wdd->ops->unref)
+		wdd->ops->unref(wdd);
+
 	return 0;
 }
 
@@ -383,6 +494,8 @@ int watchdog_dev_register(struct watchdog_device *watchdog)
 {
 	int idx, err;
 
+	mutex_init(&watchdog->lock);
+
 	/*
 	 * Note ideally we would just walk our wdd_list here and stop at the
 	 * first place which holds a NULL, but that assumes all watchdog
@@ -446,5 +559,10 @@ int watchdog_dev_unregister(struct watchdog_device *watchdog)
 
 	misc_deregister(&watchdog_miscdev[idx]);
 	wdd_list[idx] = NULL;
+
+	mutex_lock(&watchdog->lock);
+	set_bit(WDOG_UNREGISTERED, &watchdog->status);
+	mutex_unlock(&watchdog->lock);
+
 	return 0;
 }
diff --git a/include/linux/watchdog.h b/include/linux/watchdog.h
index 111843f..17a55ba 100644
--- a/include/linux/watchdog.h
+++ b/include/linux/watchdog.h
@@ -83,6 +83,8 @@ struct watchdog_ops {
 	int (*start)(struct watchdog_device *);
 	int (*stop)(struct watchdog_device *);
 	/* optional operations */
+	void (*ref)(struct watchdog_device *);
+	void (*unref)(struct watchdog_device *);
 	int (*ping)(struct watchdog_device *);
 	unsigned int (*status)(struct watchdog_device *);
 	int (*set_timeout)(struct watchdog_device *, unsigned int);
@@ -93,6 +95,7 @@ struct watchdog_ops {
  *
  * @info:	Pointer to a watchdog_info structure.
  * @ops:	Pointer to the list of watchdog operations.
+ * @lock:       Lock for watchdog core internal use only.
  * @bootstatus:	Status of the watchdog device at boot.
  * @timeout:	The watchdog devices timeout value.
  * @min_timeout:The watchdog devices minimum timeout value.
@@ -105,10 +108,14 @@ struct watchdog_ops {
  *
  * The driver-data field may not be accessed directly. It must be accessed
  * via the watchdog_set_drvdata and watchdog_get_drvdata helpers.
+ *
+ * The lock field is for watchdog core internal use only and should not be
+ * touched.
  */
 struct watchdog_device {
 	const struct watchdog_info *info;
 	const struct watchdog_ops *ops;
+	struct mutex lock;
 	unsigned int bootstatus;
 	unsigned int timeout;
 	unsigned int min_timeout;
@@ -120,6 +127,7 @@ struct watchdog_device {
 #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 ? */
+#define WDOG_UNREGISTERED	4	/* The device has been unregistered */
 };
 
 /* Use the following functions to manipulate watchdog driver specific data */
-- 
1.7.6.2

[lm-sensors] [PATCH 3/5] watchdog_dev: Add support for dynamically

[Hans de Goede]

If a driver's watchdog_device struct is part of a dynamically allocated
struct (which it often will be), merely locking the module is not enough,
even with a drivers module locked, the driver can be unbound from the device,
examples:
1) The root user can unbind it through sysfd
2) The i2c bus master driver being unloaded for an i2c watchdog

I will gladly admit that these are corner cases, but we still need to handle
them correctly.

The fix for this consists of 2 parts:
1) Add ref / unref operations, so that the driver can refcount the struct
   holding the watchdog_device struct and delay freeing it until any
   open filehandles referring to it are closed
2) Most driver operations will do IO on the device and the driver should not
   do any IO on the device after it has been unbound. Rather then letting each
   driver deal with this internally, it is better to ensure at the watchdog
   core level that no operations (other then unref) will get called after
   the driver has called watchdog_unregister_device(). This actually is the
   bulk of this patch.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 Documentation/watchdog/watchdog-kernel-api.txt |   26 +++-
 drivers/watchdog/watchdog_dev.c                |  186 +++++++++++++++++++-----
 include/linux/watchdog.h                       |    8 +
 3 files changed, 185 insertions(+), 35 deletions(-)

diff --git a/Documentation/watchdog/watchdog-kernel-api.txt b/Documentation/watchdog/watchdog-kernel-api.txt
index 4f7c894..1d3f020 100644
--- a/Documentation/watchdog/watchdog-kernel-api.txt
+++ b/Documentation/watchdog/watchdog-kernel-api.txt
@@ -1,6 +1,6 @@
 The Linux WatchDog Timer Driver Core kernel API.
 =======================-Last reviewed: 22-Jul-2011
+Last reviewed: 10-Sep-2011
 
 Wim Van Sebroeck <wim@iguana.be>
 
@@ -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 mutex lock;
 	unsigned int bootstatus;
 	unsigned int timeout;
 	unsigned int min_timeout;
@@ -53,6 +54,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.
+* lock: Mutex for WatchDog Timer Driver Core internal use only.
 * 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).
@@ -74,6 +76,8 @@ struct watchdog_ops {
 	int (*start)(struct watchdog_device *);
 	int (*stop)(struct watchdog_device *);
 	/* optional operations */
+	void (*ref)(struct watchdog_device *);
+	void (*unref)(struct watchdog_device *);
 	int (*ping)(struct watchdog_device *);
 	unsigned int (*status)(struct watchdog_device *);
 	int (*set_timeout)(struct watchdog_device *, unsigned int);
@@ -84,6 +88,21 @@ 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).
+
+If the watchdog_device struct is dynamically allocated, just locking the module
+is not enough and a driver also needs to define the ref and unref operations to
+ensure the structure holding the watchdog_device does not go away.
+
+The simplest (and usually sufficient) implementation of this is to:
+1) Add a kref struct to the same structure which is holding the watchdog_device
+2) Define a release callback for the kref which frees the struct holding both
+3) Call kref_init on this kref *before* calling watchdog_register_device()
+4) Define a ref operation calling kref_get on this kref
+5) Define a unref operation calling kref_put on this kref
+6) When it is time to cleanup:
+ * Do not kfree() the struct holding both, the last kref_put will do this!
+ * *After* calling watchdog_unregister_device() call kref_put on the kref
+
 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
@@ -141,6 +160,11 @@ bit-operations. The status bits that are defined are:
   (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.
+* WDOG_UNREGISTERED: this bit gets set by the WatchDog Timer Driver Core
+  after calling watchdog_unregister_device, and then checked before calling
+  any watchdog_ops, so that you can be sure that no operations (other then
+  unref) will get called after unregister, even if userspace still holds a
+  reference to /dev/watchdog
 
 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
diff --git a/drivers/watchdog/watchdog_dev.c b/drivers/watchdog/watchdog_dev.c
index 6483327..15dc3df 100644
--- a/drivers/watchdog/watchdog_dev.c
+++ b/drivers/watchdog/watchdog_dev.c
@@ -98,13 +98,24 @@ static struct miscdevice watchdog_miscdev[MAX_WATCHDOGS] = { {
 
 static int watchdog_ping(struct watchdog_device *wddev)
 {
-	if (test_bit(WDOG_ACTIVE, &wddev->status)) {
-		if (wddev->ops->ping)
-			return wddev->ops->ping(wddev);  /* ping the watchdog */
-		else
-			return wddev->ops->start(wddev); /* restart watchdog */
+	int ret;
+
+	if (!test_bit(WDOG_ACTIVE, &wddev->status))
+		return 0;
+
+	mutex_lock(&wddev->lock);
+	if (test_bit(WDOG_UNREGISTERED, &wddev->status)) {
+		ret = -ENODEV;
+		goto leave;
 	}
-	return 0;
+
+	if (wddev->ops->ping)
+		ret = wddev->ops->ping(wddev);  /* ping the watchdog */
+	else
+		ret = wddev->ops->start(wddev); /* restart watchdog */
+leave:
+	mutex_unlock(&wddev->lock);
+	return ret;
 }
 
 /*
@@ -118,16 +129,23 @@ static int watchdog_ping(struct watchdog_device *wddev)
 
 static int watchdog_start(struct watchdog_device *wddev)
 {
-	int err;
+	int ret;
 
-	if (!test_bit(WDOG_ACTIVE, &wddev->status)) {
-		err = wddev->ops->start(wddev);
-		if (err < 0)
-			return err;
+	if (test_bit(WDOG_ACTIVE, &wddev->status))
+		return 0;
 
-		set_bit(WDOG_ACTIVE, &wddev->status);
+	mutex_lock(&wddev->lock);
+	if (test_bit(WDOG_UNREGISTERED, &wddev->status)) {
+		ret = -ENODEV;
+		goto leave;
 	}
-	return 0;
+
+	ret = wddev->ops->start(wddev);
+	if (ret = 0)
+		set_bit(WDOG_ACTIVE, &wddev->status);
+leave:
+	mutex_unlock(&wddev->lock);
+	return ret;
 }
 
 /*
@@ -142,22 +160,116 @@ static int watchdog_start(struct watchdog_device *wddev)
 
 static int watchdog_stop(struct watchdog_device *wddev)
 {
-	int err = -EBUSY;
+	int ret;
+
+	if (!test_bit(WDOG_ACTIVE, &wddev->status))
+		return 0;
 
 	if (test_bit(WDOG_NO_WAY_OUT, &wddev->status)) {
 		pr_info("%s: nowayout prevents watchdog to be stopped!\n",
 							wddev->info->identity);
-		return err;
+		return -EBUSY;
 	}
 
-	if (test_bit(WDOG_ACTIVE, &wddev->status)) {
-		err = wddev->ops->stop(wddev);
-		if (err < 0)
-			return err;
+	mutex_lock(&wddev->lock);
+	if (test_bit(WDOG_UNREGISTERED, &wddev->status)) {
+		ret = -ENODEV;
+		goto leave;
+	}
 
+	ret = wddev->ops->stop(wddev);
+	if (ret = 0)
 		clear_bit(WDOG_ACTIVE, &wddev->status);
+leave:
+	mutex_unlock(&wddev->lock);
+	return ret;
+}
+
+/*
+ *	watchdog_ioctl_op: call the watchdog drivers ioctl op if defined
+ *	@wddev: the watchdog device to do the ioctl on
+ *	@cmd: watchdog command
+ *	@arg: argument pointer
+ */
+
+static int watchdog_ioctl_op(struct watchdog_device *wddev, unsigned int cmd,
+					unsigned long arg)
+{
+	int ret;
+
+	if (!wddev->ops->ioctl)
+		return -ENOIOCTLCMD;
+
+	mutex_lock(&wddev->lock);
+	if (test_bit(WDOG_UNREGISTERED, &wddev->status)) {
+		ret = -ENODEV;
+		goto leave;
 	}
-	return 0;
+
+	ret = wddev->ops->ioctl(wddev, cmd, arg);
+leave:
+	mutex_unlock(&wddev->lock);
+	return ret;
+}
+
+/*
+ *	watchdog_get_status: get the watchdog status
+ *	@wddev: the watchdog device to get the status from
+ */
+
+static int watchdog_get_status(struct watchdog_device *wddev,
+					unsigned int *status)
+{
+	int ret = 0;
+
+	*status = 0;
+	if (!wddev->ops->status)
+		return 0;
+
+	mutex_lock(&wddev->lock);
+	if (test_bit(WDOG_UNREGISTERED, &wddev->status)) {
+		ret = -ENODEV;
+		goto leave;
+	}
+
+	*status = wddev->ops->status(wddev);
+leave:
+	mutex_unlock(&wddev->lock);
+	return ret;
+}
+
+/*
+ *	watchdog_set_timeout: set the watchdog timer timeout
+ *	@wddev: the watchdog device to set the timeout for
+ *	@timeout: timeout to set in seconds
+ *	@arg: argument pointer
+ */
+
+static int watchdog_set_timeout(struct watchdog_device *wddev,
+					unsigned int timeout)
+{
+	int ret;
+
+	if ((wddev->ops->set_timeout = NULL) ||
+	    !(wddev->info->options & WDIOF_SETTIMEOUT))
+		return -EOPNOTSUPP;
+
+	if ((wddev->max_timeout != 0) &&
+	    (timeout < wddev->min_timeout || timeout > wddev->max_timeout))
+		return -EINVAL;
+
+	mutex_lock(&wddev->lock);
+	if (test_bit(WDOG_UNREGISTERED, &wddev->status)) {
+		ret = -ENODEV;
+		goto leave;
+	}
+
+	ret = wddev->ops->set_timeout(wddev, timeout);
+	if (ret = 0)
+		wddev->timeout = timeout;
+leave:
+	mutex_unlock(&wddev->lock);
+	return ret;
 }
 
 /*
@@ -221,18 +333,18 @@ 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;
-	}
+	err = watchdog_ioctl_op(wdd, cmd, arg);
+	if (err != -ENOIOCTLCMD)
+		return err;
 
 	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;
+		err = watchdog_get_status(wdd, &val);
+		if (err)
+			return err;
 		return put_user(val, p);
 	case WDIOC_GETBOOTSTATUS:
 		return put_user(wdd->bootstatus, p);
@@ -256,18 +368,11 @@ static long watchdog_ioctl(struct file *file, unsigned int cmd,
 		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;
-		if ((wdd->max_timeout != 0) &&
-		    (val < wdd->min_timeout || val > wdd->max_timeout))
-				return -EINVAL;
-		err = wdd->ops->set_timeout(wdd, val);
+		err = watchdog_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) */
@@ -321,6 +426,8 @@ static int watchdog_open(struct inode *inode, struct file *file)
 		goto out_mod;
 
 	file->private_data = wdd;
+	if (wdd->ops->ref)
+		wdd->ops->ref(wdd);
 
 	/* dev/watchdog is a virtual (and thus non-seekable) filesystem */
 	return nonseekable_open(inode, file);
@@ -368,6 +475,10 @@ static int watchdog_release(struct inode *inode, struct file *file)
 	/* make sure that /dev/watchdog can be re-opened */
 	clear_bit(WDOG_DEV_OPEN, &wdd->status);
 
+	/* Note wdd may be gone after this, do not use after this! */
+	if (wdd->ops->unref)
+		wdd->ops->unref(wdd);
+
 	return 0;
 }
 
@@ -383,6 +494,8 @@ int watchdog_dev_register(struct watchdog_device *watchdog)
 {
 	int idx, err;
 
+	mutex_init(&watchdog->lock);
+
 	/*
 	 * Note ideally we would just walk our wdd_list here and stop at the
 	 * first place which holds a NULL, but that assumes all watchdog
@@ -446,5 +559,10 @@ int watchdog_dev_unregister(struct watchdog_device *watchdog)
 
 	misc_deregister(&watchdog_miscdev[idx]);
 	wdd_list[idx] = NULL;
+
+	mutex_lock(&watchdog->lock);
+	set_bit(WDOG_UNREGISTERED, &watchdog->status);
+	mutex_unlock(&watchdog->lock);
+
 	return 0;
 }
diff --git a/include/linux/watchdog.h b/include/linux/watchdog.h
index 111843f..17a55ba 100644
--- a/include/linux/watchdog.h
+++ b/include/linux/watchdog.h
@@ -83,6 +83,8 @@ struct watchdog_ops {
 	int (*start)(struct watchdog_device *);
 	int (*stop)(struct watchdog_device *);
 	/* optional operations */
+	void (*ref)(struct watchdog_device *);
+	void (*unref)(struct watchdog_device *);
 	int (*ping)(struct watchdog_device *);
 	unsigned int (*status)(struct watchdog_device *);
 	int (*set_timeout)(struct watchdog_device *, unsigned int);
@@ -93,6 +95,7 @@ struct watchdog_ops {
  *
  * @info:	Pointer to a watchdog_info structure.
  * @ops:	Pointer to the list of watchdog operations.
+ * @lock:       Lock for watchdog core internal use only.
  * @bootstatus:	Status of the watchdog device at boot.
  * @timeout:	The watchdog devices timeout value.
  * @min_timeout:The watchdog devices minimum timeout value.
@@ -105,10 +108,14 @@ struct watchdog_ops {
  *
  * The driver-data field may not be accessed directly. It must be accessed
  * via the watchdog_set_drvdata and watchdog_get_drvdata helpers.
+ *
+ * The lock field is for watchdog core internal use only and should not be
+ * touched.
  */
 struct watchdog_device {
 	const struct watchdog_info *info;
 	const struct watchdog_ops *ops;
+	struct mutex lock;
 	unsigned int bootstatus;
 	unsigned int timeout;
 	unsigned int min_timeout;
@@ -120,6 +127,7 @@ struct watchdog_device {
 #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 ? */
+#define WDOG_UNREGISTERED	4	/* The device has been unregistered */
 };
 
 /* Use the following functions to manipulate watchdog driver specific data */
-- 
1.7.6.2


_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

[PATCH 4/5] watchdog_dev: Let the driver update the timeout field on set_timeout success

[Hans de Goede]

When a set_timeout operation succeeds this does not necessarily mean that
the exact timeout requested has been achieved, because the watchdog does not
necessarily have a 1 second resolution. So rather then have the core set
the timeout member of the watchdog_device struct to the exact requested
value, instead the driver should set it to the actually achieved timeout value.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 Documentation/watchdog/watchdog-kernel-api.txt |    7 ++++---
 drivers/watchdog/watchdog_dev.c                |    2 --
 2 files changed, 4 insertions(+), 5 deletions(-)

diff --git a/Documentation/watchdog/watchdog-kernel-api.txt b/Documentation/watchdog/watchdog-kernel-api.txt
index 1d3f020..5b8cff4 100644
--- a/Documentation/watchdog/watchdog-kernel-api.txt
+++ b/Documentation/watchdog/watchdog-kernel-api.txt
@@ -136,9 +136,10 @@ they are supported. These optional routines/operations are:
   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.
+  and -EIO for "could not write value to the watchdog". On success this
+  routine should set the timeout value of the watchdog_device to the
+  achieved timeout value (which may be different from the requested one
+  because the watchdog does not necessarily has a 1 second resolution).
   (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
diff --git a/drivers/watchdog/watchdog_dev.c b/drivers/watchdog/watchdog_dev.c
index 15dc3df..c8a594e 100644
--- a/drivers/watchdog/watchdog_dev.c
+++ b/drivers/watchdog/watchdog_dev.c
@@ -265,8 +265,6 @@ static int watchdog_set_timeout(struct watchdog_device *wddev,
 	}
 
 	ret = wddev->ops->set_timeout(wddev, timeout);
-	if (ret == 0)
-		wddev->timeout = timeout;
 leave:
 	mutex_unlock(&wddev->lock);
 	return ret;
-- 
1.7.6.2

[lm-sensors] [PATCH 4/5] watchdog_dev: Let the driver update the

[Hans de Goede]

When a set_timeout operation succeeds this does not necessarily mean that
the exact timeout requested has been achieved, because the watchdog does not
necessarily have a 1 second resolution. So rather then have the core set
the timeout member of the watchdog_device struct to the exact requested
value, instead the driver should set it to the actually achieved timeout value.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 Documentation/watchdog/watchdog-kernel-api.txt |    7 ++++---
 drivers/watchdog/watchdog_dev.c                |    2 --
 2 files changed, 4 insertions(+), 5 deletions(-)

diff --git a/Documentation/watchdog/watchdog-kernel-api.txt b/Documentation/watchdog/watchdog-kernel-api.txt
index 1d3f020..5b8cff4 100644
--- a/Documentation/watchdog/watchdog-kernel-api.txt
+++ b/Documentation/watchdog/watchdog-kernel-api.txt
@@ -136,9 +136,10 @@ they are supported. These optional routines/operations are:
   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.
+  and -EIO for "could not write value to the watchdog". On success this
+  routine should set the timeout value of the watchdog_device to the
+  achieved timeout value (which may be different from the requested one
+  because the watchdog does not necessarily has a 1 second resolution).
   (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
diff --git a/drivers/watchdog/watchdog_dev.c b/drivers/watchdog/watchdog_dev.c
index 15dc3df..c8a594e 100644
--- a/drivers/watchdog/watchdog_dev.c
+++ b/drivers/watchdog/watchdog_dev.c
@@ -265,8 +265,6 @@ static int watchdog_set_timeout(struct watchdog_device *wddev,
 	}
 
 	ret = wddev->ops->set_timeout(wddev, timeout);
-	if (ret = 0)
-		wddev->timeout = timeout;
 leave:
 	mutex_unlock(&wddev->lock);
 	return ret;
-- 
1.7.6.2


_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

[PATCH 5/5] hwmon/sch5636: Add support for the integrated watchdog

[Hans de Goede]

Add support for the watchdog integrated into the (Fujitsu Theseus version of)
the sch5636 superio hwmon part. Using the new watchdog timer core.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/hwmon/sch5636.c |  233 ++++++++++++++++++++++++++++++++++++++++++++++-
 1 files changed, 231 insertions(+), 2 deletions(-)

diff --git a/drivers/hwmon/sch5636.c b/drivers/hwmon/sch5636.c
index 244407a..8cf3630 100644
--- a/drivers/hwmon/sch5636.c
+++ b/drivers/hwmon/sch5636.c
@@ -28,14 +28,23 @@
 #include <linux/hwmon-sysfs.h>
 #include <linux/err.h>
 #include <linux/mutex.h>
+#include <linux/watchdog.h>
+#include <linux/kref.h>
 #include "sch56xx-common.h"
 
+/* Insmod parameters */
+static int nowayout = WATCHDOG_NOWAYOUT;
+module_param(nowayout, int, 0);
+MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once started (default="
+	__MODULE_STRING(WATCHDOG_NOWAYOUT) ")");
+
 #define DRVNAME "sch5636"
 #define DEVNAME "theseus" /* We only support one model for now */
 
 #define SCH5636_REG_FUJITSU_ID		0x780
 #define SCH5636_REG_FUJITSU_REV		0x783
 
+/* hwmon registers */
 #define SCH5636_NO_INS			5
 #define SCH5636_NO_TEMPS		16
 #define SCH5636_NO_FANS			8
@@ -63,11 +72,18 @@ static const u16 SCH5636_REG_FAN_VAL[SCH5636_NO_FANS] = {
 #define SCH5636_FAN_NOT_PRESENT		0x08
 #define SCH5636_FAN_DEACTIVATED		0x80
 
+/* Watchdog registers */
+#define SCH5636_REG_WDOG_PRESET		0x58B
+#define SCH5636_REG_WDOG_CONTROL	0x58C
+#define SCH5636_WDOG_TIME_BASE_SEC	0x01
+#define SCH5636_REG_WDOG_OUTPUT_ENABLE	0x58E
+#define SCH5636_WDOG_OUTPUT_ENABLE	0x02
 
 struct sch5636_data {
 	unsigned short addr;
-	struct device *hwmon_dev;
 
+	/* hwmon related variables */
+	struct device *hwmon_dev;
 	struct mutex update_lock;
 	char valid;			/* !=0 if following fields are valid */
 	unsigned long last_updated;	/* In jiffies */
@@ -76,8 +92,25 @@ struct sch5636_data {
 	u8 temp_ctrl[SCH5636_NO_TEMPS];
 	u16 fan_val[SCH5636_NO_FANS];
 	u8 fan_ctrl[SCH5636_NO_FANS];
+
+	/* watchdog related variables */
+	struct watchdog_device wddev;
+	struct watchdog_info wdinfo;
+	struct kref kref;
+	bool watchdog_registered;
+	u8 watchdog_preset;
+	u8 watchdog_control;
+	u8 watchdog_output_enable;
 };
 
+/* Release our data struct when the platform device has been released *and*
+   all references to our watchdog device are released */
+static void sch5636_release_resources(struct kref *r)
+{
+	struct sch5636_data *data = container_of(r, struct sch5636_data, kref);
+	kfree(data);
+}
+
 static struct sch5636_data *sch5636_update_device(struct device *dev)
 {
 	struct sch5636_data *data = dev_get_drvdata(dev);
@@ -379,11 +412,154 @@ static struct sensor_device_attribute sch5636_fan_attr[] = {
 	SENSOR_ATTR(fan8_alarm, 0444, show_fan_alarm, NULL, 7),
 };
 
+/*
+ * Watchdog routines
+ */
+
+static int watchdog_set_timeout(struct watchdog_device *wddev,
+				unsigned int timeout)
+{
+	struct sch5636_data *data = watchdog_get_drvdata(wddev);
+	int err, resolution;
+	u8 control;
+
+	/* 1 second or 60 second resolution? */
+	if (timeout <= 255)
+		resolution = 1;
+	else
+		resolution = 60;
+
+	if (resolution == 1)
+		control = data->watchdog_control | SCH5636_WDOG_TIME_BASE_SEC;
+	else
+		control = data->watchdog_control & ~SCH5636_WDOG_TIME_BASE_SEC;
+
+	if (data->watchdog_control != control) {
+		err = sch56xx_write_virtual_reg(data->addr,
+						SCH5636_REG_WDOG_CONTROL,
+						control);
+		if (err)
+			return err;
+
+		data->watchdog_control = control;
+	}
+
+	/* Remember new timeout value, but do not write as that (re)starts
+	   the watchdog countdown */
+	data->watchdog_preset = DIV_ROUND_UP(timeout, resolution);
+	wddev->timeout = data->watchdog_preset * resolution;
+
+	return 0;
+}
+
+static int watchdog_start(struct watchdog_device *wddev)
+{
+	struct sch5636_data *data = watchdog_get_drvdata(wddev);
+	int err;
+	u8 val;
+
+	/*
+	 * The sch5636's watchdog cannot really be started / stopped
+	 * it is always running, but we can avoid the timer expiring
+	 * from causing a system reset by clearing the output enable bit.
+	 *
+	 * The sch5636's watchdog will set the watchdog event bit, bit 0
+	 * of the second interrupt source register (at base-address + 9),
+	 * when the timer expires.
+	 *
+	 * This will only cause a system reset if the 0-1 flank happens when
+	 * output enable is true. Setting output enable after the flank will
+	 * not cause a reset, nor will the timer expiring a second time.
+	 * This means we must clear the watchdog event bit in case it is set.
+	 *
+	 * The timer may still be running (after a recent watchdog_stop) and
+	 * mere milliseconds away from expiring, so the timer must be reset
+	 * first!
+	 */
+
+	/* 1. Reset the watchdog countdown counter */
+	err = sch56xx_write_virtual_reg(data->addr, SCH5636_REG_WDOG_PRESET,
+					data->watchdog_preset);
+	if (err)
+		return err;
+
+	/* 2. Enable output (if not already enabled) */
+	val = data->watchdog_output_enable | SCH5636_WDOG_OUTPUT_ENABLE;
+	err = sch56xx_write_virtual_reg(data->addr,
+					SCH5636_REG_WDOG_OUTPUT_ENABLE, val);
+	if (err)
+		return err;
+	data->watchdog_output_enable = val;
+
+	/* 3. Clear the watchdog event bit if set */
+	val = inb(data->addr + 9);
+	if (val & 0x01)
+		outb(0x01, data->addr + 9);
+
+	return 0;
+}
+
+static int watchdog_trigger(struct watchdog_device *wddev)
+{
+	struct sch5636_data *data = watchdog_get_drvdata(wddev);
+
+	/* Reset the watchdog countdown counter */
+	return sch56xx_write_virtual_reg(data->addr, SCH5636_REG_WDOG_PRESET,
+					 data->watchdog_preset);
+}
+
+static int watchdog_stop(struct watchdog_device *wddev)
+{
+	struct sch5636_data *data = watchdog_get_drvdata(wddev);
+	int err;
+	u8 val;
+
+	val = data->watchdog_output_enable & ~SCH5636_WDOG_OUTPUT_ENABLE;
+	err = sch56xx_write_virtual_reg(data->addr,
+					SCH5636_REG_WDOG_OUTPUT_ENABLE, val);
+	if (err)
+		return err;
+	data->watchdog_output_enable = val;
+
+	return 0;
+}
+
+static void watchdog_ref(struct watchdog_device *wddev)
+{
+	struct sch5636_data *data = watchdog_get_drvdata(wddev);
+
+	kref_get(&data->kref);
+}
+
+static void watchdog_unref(struct watchdog_device *wddev)
+{
+	struct sch5636_data *data = watchdog_get_drvdata(wddev);
+
+	kref_put(&data->kref, sch5636_release_resources);
+}
+
+static const struct watchdog_ops watchdog_ops = {
+	.owner		= THIS_MODULE,
+	.start		= watchdog_start,
+	.stop		= watchdog_stop,
+	.ref		= watchdog_ref,
+	.unref		= watchdog_unref,
+	.ping		= watchdog_trigger,
+	.set_timeout	= watchdog_set_timeout,
+};
+
+/*
+ * Remove, probe, register and unregister device functions
+ */
+
 static int sch5636_remove(struct platform_device *pdev)
 {
 	struct sch5636_data *data = platform_get_drvdata(pdev);
 	int i;
 
+	if (data->watchdog_registered)
+		watchdog_unregister_device(&data->wddev);
+
 	if (data->hwmon_dev)
 		hwmon_device_unregister(data->hwmon_dev);
 
@@ -399,7 +575,7 @@ static int sch5636_remove(struct platform_device *pdev)
 				   &sch5636_fan_attr[i].dev_attr);
 
 	platform_set_drvdata(pdev, NULL);
-	kfree(data);
+	kref_put(&data->kref, sch5636_release_resources);
 
 	return 0;
 }
@@ -416,6 +592,7 @@ static int __devinit sch5636_probe(struct platform_device *pdev)
 
 	data->addr = platform_get_resource(pdev, IORESOURCE_IO, 0)->start;
 	mutex_init(&data->update_lock);
+	kref_init(&data->kref);
 	platform_set_drvdata(pdev, data);
 
 	for (i = 0; i < 3; i++) {
@@ -450,6 +627,8 @@ static int __devinit sch5636_probe(struct platform_device *pdev)
 	pr_info("Found %s chip at %#hx, revison: %d.%02d\n", DEVNAME,
 		data->addr, revision[0], revision[1]);
 
+	/********* hwmon initialization *********/
+
 	/* Read all temp + fan ctrl registers to determine which are active */
 	for (i = 0; i < SCH5636_NO_TEMPS; i++) {
 		val = sch56xx_read_virtual_reg(data->addr,
@@ -505,6 +684,56 @@ static int __devinit sch5636_probe(struct platform_device *pdev)
 		goto error;
 	}
 
+	/********* watchdog initialization *********/
+
+	data->wdinfo.options = WDIOF_KEEPALIVEPING | WDIOF_SETTIMEOUT;
+	strlcpy(data->wdinfo.identity, "Fujitsu watchdog",
+		sizeof(data->wdinfo.identity));
+	data->wdinfo.firmware_version = revision[0];
+	data->wddev.info = &data->wdinfo;
+	data->wddev.ops = &watchdog_ops;
+	data->wddev.min_timeout = 1;
+	data->wddev.max_timeout = 255 * 60;
+	if (nowayout)
+		data->wddev.status |= WDOG_NO_WAY_OUT;
+	else
+		data->wdinfo.options |= WDIOF_MAGICCLOSE;
+
+	/* Since the watchdog uses a downcounter there is no register to read
+	   the BIOS set timeout from (if any was set at all) -> choose a preset
+	   which will give us a 1 minute timeout */
+	val = sch56xx_read_virtual_reg(data->addr, SCH5636_REG_WDOG_CONTROL);
+	if (val < 0) {
+		err = val;
+		goto error;
+	}
+	data->watchdog_control = val;
+	if (data->watchdog_control & SCH5636_WDOG_TIME_BASE_SEC)
+		data->watchdog_preset = 60; /* seconds */
+	else
+		data->watchdog_preset = 1; /* minute */
+	data->wddev.timeout = 60;
+
+	/* Check if the watchdog was already enabled by the BIOS */
+	val = sch56xx_read_virtual_reg(data->addr,
+				       SCH5636_REG_WDOG_OUTPUT_ENABLE);
+	if (val < 0) {
+		err = val;
+		goto error;
+	}
+	data->watchdog_output_enable = val;
+	if (data->watchdog_output_enable & SCH5636_WDOG_OUTPUT_ENABLE)
+		data->wddev.status |= WDOG_ACTIVE;
+
+	/*
+	 * Note we don't abort on failure to register the watchdog, as we
+	 * still want to stick around for the hwmon stuff.
+	 */
+	watchdog_set_drvdata(&data->wddev, data);
+	err = watchdog_register_device(&data->wddev);
+	if (!err)
+		data->watchdog_registered = true;
+
 	return 0;
 
 error:
-- 
1.7.6.2

[lm-sensors] [PATCH 5/5] hwmon/sch5636: Add support for the

[Hans de Goede]

Add support for the watchdog integrated into the (Fujitsu Theseus version of)
the sch5636 superio hwmon part. Using the new watchdog timer core.

Signed-off-by: Hans de Goede <hdegoede@redhat.com>
---
 drivers/hwmon/sch5636.c |  233 ++++++++++++++++++++++++++++++++++++++++++++++-
 1 files changed, 231 insertions(+), 2 deletions(-)

diff --git a/drivers/hwmon/sch5636.c b/drivers/hwmon/sch5636.c
index 244407a..8cf3630 100644
--- a/drivers/hwmon/sch5636.c
+++ b/drivers/hwmon/sch5636.c
@@ -28,14 +28,23 @@
 #include <linux/hwmon-sysfs.h>
 #include <linux/err.h>
 #include <linux/mutex.h>
+#include <linux/watchdog.h>
+#include <linux/kref.h>
 #include "sch56xx-common.h"
 
+/* Insmod parameters */
+static int nowayout = WATCHDOG_NOWAYOUT;
+module_param(nowayout, int, 0);
+MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once started (default="
+	__MODULE_STRING(WATCHDOG_NOWAYOUT) ")");
+
 #define DRVNAME "sch5636"
 #define DEVNAME "theseus" /* We only support one model for now */
 
 #define SCH5636_REG_FUJITSU_ID		0x780
 #define SCH5636_REG_FUJITSU_REV		0x783
 
+/* hwmon registers */
 #define SCH5636_NO_INS			5
 #define SCH5636_NO_TEMPS		16
 #define SCH5636_NO_FANS			8
@@ -63,11 +72,18 @@ static const u16 SCH5636_REG_FAN_VAL[SCH5636_NO_FANS] = {
 #define SCH5636_FAN_NOT_PRESENT		0x08
 #define SCH5636_FAN_DEACTIVATED		0x80
 
+/* Watchdog registers */
+#define SCH5636_REG_WDOG_PRESET		0x58B
+#define SCH5636_REG_WDOG_CONTROL	0x58C
+#define SCH5636_WDOG_TIME_BASE_SEC	0x01
+#define SCH5636_REG_WDOG_OUTPUT_ENABLE	0x58E
+#define SCH5636_WDOG_OUTPUT_ENABLE	0x02
 
 struct sch5636_data {
 	unsigned short addr;
-	struct device *hwmon_dev;
 
+	/* hwmon related variables */
+	struct device *hwmon_dev;
 	struct mutex update_lock;
 	char valid;			/* !=0 if following fields are valid */
 	unsigned long last_updated;	/* In jiffies */
@@ -76,8 +92,25 @@ struct sch5636_data {
 	u8 temp_ctrl[SCH5636_NO_TEMPS];
 	u16 fan_val[SCH5636_NO_FANS];
 	u8 fan_ctrl[SCH5636_NO_FANS];
+
+	/* watchdog related variables */
+	struct watchdog_device wddev;
+	struct watchdog_info wdinfo;
+	struct kref kref;
+	bool watchdog_registered;
+	u8 watchdog_preset;
+	u8 watchdog_control;
+	u8 watchdog_output_enable;
 };
 
+/* Release our data struct when the platform device has been released *and*
+   all references to our watchdog device are released */
+static void sch5636_release_resources(struct kref *r)
+{
+	struct sch5636_data *data = container_of(r, struct sch5636_data, kref);
+	kfree(data);
+}
+
 static struct sch5636_data *sch5636_update_device(struct device *dev)
 {
 	struct sch5636_data *data = dev_get_drvdata(dev);
@@ -379,11 +412,154 @@ static struct sensor_device_attribute sch5636_fan_attr[] = {
 	SENSOR_ATTR(fan8_alarm, 0444, show_fan_alarm, NULL, 7),
 };
 
+/*
+ * Watchdog routines
+ */
+
+static int watchdog_set_timeout(struct watchdog_device *wddev,
+				unsigned int timeout)
+{
+	struct sch5636_data *data = watchdog_get_drvdata(wddev);
+	int err, resolution;
+	u8 control;
+
+	/* 1 second or 60 second resolution? */
+	if (timeout <= 255)
+		resolution = 1;
+	else
+		resolution = 60;
+
+	if (resolution = 1)
+		control = data->watchdog_control | SCH5636_WDOG_TIME_BASE_SEC;
+	else
+		control = data->watchdog_control & ~SCH5636_WDOG_TIME_BASE_SEC;
+
+	if (data->watchdog_control != control) {
+		err = sch56xx_write_virtual_reg(data->addr,
+						SCH5636_REG_WDOG_CONTROL,
+						control);
+		if (err)
+			return err;
+
+		data->watchdog_control = control;
+	}
+
+	/* Remember new timeout value, but do not write as that (re)starts
+	   the watchdog countdown */
+	data->watchdog_preset = DIV_ROUND_UP(timeout, resolution);
+	wddev->timeout = data->watchdog_preset * resolution;
+
+	return 0;
+}
+
+static int watchdog_start(struct watchdog_device *wddev)
+{
+	struct sch5636_data *data = watchdog_get_drvdata(wddev);
+	int err;
+	u8 val;
+
+	/*
+	 * The sch5636's watchdog cannot really be started / stopped
+	 * it is always running, but we can avoid the timer expiring
+	 * from causing a system reset by clearing the output enable bit.
+	 *
+	 * The sch5636's watchdog will set the watchdog event bit, bit 0
+	 * of the second interrupt source register (at base-address + 9),
+	 * when the timer expires.
+	 *
+	 * This will only cause a system reset if the 0-1 flank happens when
+	 * output enable is true. Setting output enable after the flank will
+	 * not cause a reset, nor will the timer expiring a second time.
+	 * This means we must clear the watchdog event bit in case it is set.
+	 *
+	 * The timer may still be running (after a recent watchdog_stop) and
+	 * mere milliseconds away from expiring, so the timer must be reset
+	 * first!
+	 */
+
+	/* 1. Reset the watchdog countdown counter */
+	err = sch56xx_write_virtual_reg(data->addr, SCH5636_REG_WDOG_PRESET,
+					data->watchdog_preset);
+	if (err)
+		return err;
+
+	/* 2. Enable output (if not already enabled) */
+	val = data->watchdog_output_enable | SCH5636_WDOG_OUTPUT_ENABLE;
+	err = sch56xx_write_virtual_reg(data->addr,
+					SCH5636_REG_WDOG_OUTPUT_ENABLE, val);
+	if (err)
+		return err;
+	data->watchdog_output_enable = val;
+
+	/* 3. Clear the watchdog event bit if set */
+	val = inb(data->addr + 9);
+	if (val & 0x01)
+		outb(0x01, data->addr + 9);
+
+	return 0;
+}
+
+static int watchdog_trigger(struct watchdog_device *wddev)
+{
+	struct sch5636_data *data = watchdog_get_drvdata(wddev);
+
+	/* Reset the watchdog countdown counter */
+	return sch56xx_write_virtual_reg(data->addr, SCH5636_REG_WDOG_PRESET,
+					 data->watchdog_preset);
+}
+
+static int watchdog_stop(struct watchdog_device *wddev)
+{
+	struct sch5636_data *data = watchdog_get_drvdata(wddev);
+	int err;
+	u8 val;
+
+	val = data->watchdog_output_enable & ~SCH5636_WDOG_OUTPUT_ENABLE;
+	err = sch56xx_write_virtual_reg(data->addr,
+					SCH5636_REG_WDOG_OUTPUT_ENABLE, val);
+	if (err)
+		return err;
+	data->watchdog_output_enable = val;
+
+	return 0;
+}
+
+static void watchdog_ref(struct watchdog_device *wddev)
+{
+	struct sch5636_data *data = watchdog_get_drvdata(wddev);
+
+	kref_get(&data->kref);
+}
+
+static void watchdog_unref(struct watchdog_device *wddev)
+{
+	struct sch5636_data *data = watchdog_get_drvdata(wddev);
+
+	kref_put(&data->kref, sch5636_release_resources);
+}
+
+static const struct watchdog_ops watchdog_ops = {
+	.owner		= THIS_MODULE,
+	.start		= watchdog_start,
+	.stop		= watchdog_stop,
+	.ref		= watchdog_ref,
+	.unref		= watchdog_unref,
+	.ping		= watchdog_trigger,
+	.set_timeout	= watchdog_set_timeout,
+};
+
+/*
+ * Remove, probe, register and unregister device functions
+ */
+
 static int sch5636_remove(struct platform_device *pdev)
 {
 	struct sch5636_data *data = platform_get_drvdata(pdev);
 	int i;
 
+	if (data->watchdog_registered)
+		watchdog_unregister_device(&data->wddev);
+
 	if (data->hwmon_dev)
 		hwmon_device_unregister(data->hwmon_dev);
 
@@ -399,7 +575,7 @@ static int sch5636_remove(struct platform_device *pdev)
 				   &sch5636_fan_attr[i].dev_attr);
 
 	platform_set_drvdata(pdev, NULL);
-	kfree(data);
+	kref_put(&data->kref, sch5636_release_resources);
 
 	return 0;
 }
@@ -416,6 +592,7 @@ static int __devinit sch5636_probe(struct platform_device *pdev)
 
 	data->addr = platform_get_resource(pdev, IORESOURCE_IO, 0)->start;
 	mutex_init(&data->update_lock);
+	kref_init(&data->kref);
 	platform_set_drvdata(pdev, data);
 
 	for (i = 0; i < 3; i++) {
@@ -450,6 +627,8 @@ static int __devinit sch5636_probe(struct platform_device *pdev)
 	pr_info("Found %s chip at %#hx, revison: %d.%02d\n", DEVNAME,
 		data->addr, revision[0], revision[1]);
 
+	/********* hwmon initialization *********/
+
 	/* Read all temp + fan ctrl registers to determine which are active */
 	for (i = 0; i < SCH5636_NO_TEMPS; i++) {
 		val = sch56xx_read_virtual_reg(data->addr,
@@ -505,6 +684,56 @@ static int __devinit sch5636_probe(struct platform_device *pdev)
 		goto error;
 	}
 
+	/********* watchdog initialization *********/
+
+	data->wdinfo.options = WDIOF_KEEPALIVEPING | WDIOF_SETTIMEOUT;
+	strlcpy(data->wdinfo.identity, "Fujitsu watchdog",
+		sizeof(data->wdinfo.identity));
+	data->wdinfo.firmware_version = revision[0];
+	data->wddev.info = &data->wdinfo;
+	data->wddev.ops = &watchdog_ops;
+	data->wddev.min_timeout = 1;
+	data->wddev.max_timeout = 255 * 60;
+	if (nowayout)
+		data->wddev.status |= WDOG_NO_WAY_OUT;
+	else
+		data->wdinfo.options |= WDIOF_MAGICCLOSE;
+
+	/* Since the watchdog uses a downcounter there is no register to read
+	   the BIOS set timeout from (if any was set at all) -> choose a preset
+	   which will give us a 1 minute timeout */
+	val = sch56xx_read_virtual_reg(data->addr, SCH5636_REG_WDOG_CONTROL);
+	if (val < 0) {
+		err = val;
+		goto error;
+	}
+	data->watchdog_control = val;
+	if (data->watchdog_control & SCH5636_WDOG_TIME_BASE_SEC)
+		data->watchdog_preset = 60; /* seconds */
+	else
+		data->watchdog_preset = 1; /* minute */
+	data->wddev.timeout = 60;
+
+	/* Check if the watchdog was already enabled by the BIOS */
+	val = sch56xx_read_virtual_reg(data->addr,
+				       SCH5636_REG_WDOG_OUTPUT_ENABLE);
+	if (val < 0) {
+		err = val;
+		goto error;
+	}
+	data->watchdog_output_enable = val;
+	if (data->watchdog_output_enable & SCH5636_WDOG_OUTPUT_ENABLE)
+		data->wddev.status |= WDOG_ACTIVE;
+
+	/*
+	 * Note we don't abort on failure to register the watchdog, as we
+	 * still want to stick around for the hwmon stuff.
+	 */
+	watchdog_set_drvdata(&data->wddev, data);
+	err = watchdog_register_device(&data->wddev);
+	if (!err)
+		data->watchdog_registered = true;
+
 	return 0;
 
 error:
-- 
1.7.6.2


_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] [PATCH 5/5] hwmon/sch5636: Add support for the integrated watchdog

[Guenter Roeck]

On Mon, 2011-09-12 at 05:57 -0400, Hans de Goede wrote:
> Add support for the watchdog integrated into the (Fujitsu Theseus version of)
> the sch5636 superio hwmon part. Using the new watchdog timer core.
> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  drivers/hwmon/sch5636.c |  233 ++++++++++++++++++++++++++++++++++++++++++++++-
>  1 files changed, 231 insertions(+), 2 deletions(-)
> 
[ resent to proper thread ]

Given that this is no longer a pure hwmon driver, it may make sense to
split it into separate mfd/hwmon/watchdog drivers. But on the other side
I notice that other drivers in the hwmon directory implement watchdog
functionality as well. Bad precedent :(. Wonder what happens with those
watchdogs if HWMON is disabled.

Not really sure if we should let this happen, or start being more
restrictive and enforce cleaner code. Jean, any thoughts/comments ?

Guenter

Re: [lm-sensors] [PATCH 5/5] hwmon/sch5636: Add support for the

[Guenter Roeck]

On Mon, 2011-09-12 at 05:57 -0400, Hans de Goede wrote:
> Add support for the watchdog integrated into the (Fujitsu Theseus version of)
> the sch5636 superio hwmon part. Using the new watchdog timer core.
> 
> Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> ---
>  drivers/hwmon/sch5636.c |  233 ++++++++++++++++++++++++++++++++++++++++++++++-
>  1 files changed, 231 insertions(+), 2 deletions(-)
> 
[ resent to proper thread ]

Given that this is no longer a pure hwmon driver, it may make sense to
split it into separate mfd/hwmon/watchdog drivers. But on the other side
I notice that other drivers in the hwmon directory implement watchdog
functionality as well. Bad precedent :(. Wonder what happens with those
watchdogs if HWMON is disabled.

Not really sure if we should let this happen, or start being more
restrictive and enforce cleaner code. Jean, any thoughts/comments ?

Guenter



_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] [PATCH 5/5] hwmon/sch5636: Add support for the integrated watchdog

[Hans de Goede]

Hi,

On 09/12/2011 07:33 PM, Guenter Roeck wrote:
> On Mon, 2011-09-12 at 05:57 -0400, Hans de Goede wrote:
>> Add support for the watchdog integrated into the (Fujitsu Theseus version of)
>> the sch5636 superio hwmon part. Using the new watchdog timer core.
>>
>> Signed-off-by: Hans de Goede<hdegoede@redhat.com>
>> ---
>>   drivers/hwmon/sch5636.c |  233 ++++++++++++++++++++++++++++++++++++++++++++++-
>>   1 files changed, 231 insertions(+), 2 deletions(-)
>>
> [ resent to proper thread ]
>
> Given that this is no longer a pure hwmon driver, it may make sense to
> split it into separate mfd/hwmon/watchdog drivers. But on the other side
> I notice that other drivers in the hwmon directory implement watchdog
> functionality as well. Bad precedent :(. Wonder what happens with those
> watchdogs if HWMON is disabled.
>
> Not really sure if we should let this happen, or start being more
> restrictive and enforce cleaner code. Jean, any thoughts/comments ?

The problem with the 2 hwmon drivers (sch5636 and fschmd) I'm responsible
for which have a watchdog integrated is that the watchdog is part of the
same "logical unit" as the hwmon part. This actually makes sense from
a hw point of view. Various watchdogs also have functions like brownout
detection / under / over volt monitoring and temp monitoring, resetting
the system not only when the application fails to ping the watchdog,
but also on various other abnormal conditions. The watchdog API even
has some (rather crude) interfaces for getting readings for some of these
sensors embedded inside wacthdogs. Although it would be better for those
drivers that support this to just move to the sensors interface for those
parts.

So back to the 1 logical unit part, you are talking about using / creating
some sort of mfd (multi function device) framework. Actually we already have
that for the sch5636 case, it is a superio and a superio is divided into
logical devices, which usually each have their own driver. Sometimes drivers
need to touch the shared superio config space, and we've sharing mechanisms
for those in place too now a days.

But the sch5636 hardware monitoring and watchdog function are part of the
same logical device, they share the same (isa) io ports, and the same
convoluted talk to the embedded microcontroller by banging and polling
io ports communications "protocol".

I'm pretty firmly set on a one logical unit one driver model, anything
else will mean writing some sort of specialized sharing framework just
for this model IC, and then another one for the next, etc. If the
only argument against just exporting the 2 interfaces with one driver
is that the watchdog function will go away if HWMON is not enabled, waying
it against the amount of work necessary to get a clean split, I don't
find that enough of a reason for split drivers.

Also not that we will actually never have a really clean split,
since as soon as the real hardware has both functions integrated into
a single logical unit any sharing / multiplexing solution will be highly
device specific.

Actually in v4l land we are currently working on fixing a historic mistake
where for certain devices (so called dual mode cameras) we have 2 drivers
for 1 logical unit (1 usb interface). The driver infighting happening there
is not pretty and we've decided to fix this once and for all by moving
both functions into a single driver...

Regards,

Hans

Re: [lm-sensors] [PATCH 5/5] hwmon/sch5636: Add support for the

[Hans de Goede]

Hi,

On 09/12/2011 07:33 PM, Guenter Roeck wrote:
> On Mon, 2011-09-12 at 05:57 -0400, Hans de Goede wrote:
>> Add support for the watchdog integrated into the (Fujitsu Theseus version of)
>> the sch5636 superio hwmon part. Using the new watchdog timer core.
>>
>> Signed-off-by: Hans de Goede<hdegoede@redhat.com>
>> ---
>>   drivers/hwmon/sch5636.c |  233 ++++++++++++++++++++++++++++++++++++++++++++++-
>>   1 files changed, 231 insertions(+), 2 deletions(-)
>>
> [ resent to proper thread ]
>
> Given that this is no longer a pure hwmon driver, it may make sense to
> split it into separate mfd/hwmon/watchdog drivers. But on the other side
> I notice that other drivers in the hwmon directory implement watchdog
> functionality as well. Bad precedent :(. Wonder what happens with those
> watchdogs if HWMON is disabled.
>
> Not really sure if we should let this happen, or start being more
> restrictive and enforce cleaner code. Jean, any thoughts/comments ?

The problem with the 2 hwmon drivers (sch5636 and fschmd) I'm responsible
for which have a watchdog integrated is that the watchdog is part of the
same "logical unit" as the hwmon part. This actually makes sense from
a hw point of view. Various watchdogs also have functions like brownout
detection / under / over volt monitoring and temp monitoring, resetting
the system not only when the application fails to ping the watchdog,
but also on various other abnormal conditions. The watchdog API even
has some (rather crude) interfaces for getting readings for some of these
sensors embedded inside wacthdogs. Although it would be better for those
drivers that support this to just move to the sensors interface for those
parts.

So back to the 1 logical unit part, you are talking about using / creating
some sort of mfd (multi function device) framework. Actually we already have
that for the sch5636 case, it is a superio and a superio is divided into
logical devices, which usually each have their own driver. Sometimes drivers
need to touch the shared superio config space, and we've sharing mechanisms
for those in place too now a days.

But the sch5636 hardware monitoring and watchdog function are part of the
same logical device, they share the same (isa) io ports, and the same
convoluted talk to the embedded microcontroller by banging and polling
io ports communications "protocol".

I'm pretty firmly set on a one logical unit one driver model, anything
else will mean writing some sort of specialized sharing framework just
for this model IC, and then another one for the next, etc. If the
only argument against just exporting the 2 interfaces with one driver
is that the watchdog function will go away if HWMON is not enabled, waying
it against the amount of work necessary to get a clean split, I don't
find that enough of a reason for split drivers.

Also not that we will actually never have a really clean split,
since as soon as the real hardware has both functions integrated into
a single logical unit any sharing / multiplexing solution will be highly
device specific.

Actually in v4l land we are currently working on fixing a historic mistake
where for certain devices (so called dual mode cameras) we have 2 drivers
for 1 logical unit (1 usb interface). The driver infighting happening there
is not pretty and we've decided to fix this once and for all by moving
both functions into a single driver...

Regards,

Hans

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] [PATCH 5/5] hwmon/sch5636: Add support for the integrated watchdog

[Jean Delvare]

On Mon, 12 Sep 2011 10:33:46 -0700, Guenter Roeck wrote:
> On Mon, 2011-09-12 at 05:57 -0400, Hans de Goede wrote:
> > Add support for the watchdog integrated into the (Fujitsu Theseus version of)
> > the sch5636 superio hwmon part. Using the new watchdog timer core.
> > 
> > Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> > ---
> >  drivers/hwmon/sch5636.c |  233 ++++++++++++++++++++++++++++++++++++++++++++++-
> >  1 files changed, 231 insertions(+), 2 deletions(-)
> > 
> [ resent to proper thread ]
> 
> Given that this is no longer a pure hwmon driver, it may make sense to
> split it into separate mfd/hwmon/watchdog drivers. But on the other side
> I notice that other drivers in the hwmon directory implement watchdog
> functionality as well. Bad precedent :(.

IIRC the first driver doing this was one of the FSC drivers and this was
before the MFD framework was created. It was simply never migrated, and
other drivers followed the trend.

> Wonder what happens with those watchdogs if HWMON is disabled.

Then the watchdog feature isn't available. The idea as I understand it
is that hardware monitoring is the main feature of these chips and
watchdog is only an add-on, so we don't expect the user to want the
watchdog feature without the hardware monitoring feature.

> Not really sure if we should let this happen, or start being more
> restrictive and enforce cleaner code. Jean, any thoughts/comments ?

I have no objection to merging the code as is, as you found out it's not
doing anything other drivers haven't already.

If anybody cares, that person can clean up one or more drivers, later.
That being said, at least for I2C devices, I don't mind leaving things
as they are, as doing things "better" means more complex code for very
little benefit. For Super-I/O chips which are multifunction by nature,
the MFD framework is certainly the right way to go.

-- 
Jean Delvare

Re: [lm-sensors] [PATCH 5/5] hwmon/sch5636: Add support for the

[Jean Delvare]

On Mon, 12 Sep 2011 10:33:46 -0700, Guenter Roeck wrote:
> On Mon, 2011-09-12 at 05:57 -0400, Hans de Goede wrote:
> > Add support for the watchdog integrated into the (Fujitsu Theseus version of)
> > the sch5636 superio hwmon part. Using the new watchdog timer core.
> > 
> > Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> > ---
> >  drivers/hwmon/sch5636.c |  233 ++++++++++++++++++++++++++++++++++++++++++++++-
> >  1 files changed, 231 insertions(+), 2 deletions(-)
> > 
> [ resent to proper thread ]
> 
> Given that this is no longer a pure hwmon driver, it may make sense to
> split it into separate mfd/hwmon/watchdog drivers. But on the other side
> I notice that other drivers in the hwmon directory implement watchdog
> functionality as well. Bad precedent :(.

IIRC the first driver doing this was one of the FSC drivers and this was
before the MFD framework was created. It was simply never migrated, and
other drivers followed the trend.

> Wonder what happens with those watchdogs if HWMON is disabled.

Then the watchdog feature isn't available. The idea as I understand it
is that hardware monitoring is the main feature of these chips and
watchdog is only an add-on, so we don't expect the user to want the
watchdog feature without the hardware monitoring feature.

> Not really sure if we should let this happen, or start being more
> restrictive and enforce cleaner code. Jean, any thoughts/comments ?

I have no objection to merging the code as is, as you found out it's not
doing anything other drivers haven't already.

If anybody cares, that person can clean up one or more drivers, later.
That being said, at least for I2C devices, I don't mind leaving things
as they are, as doing things "better" means more complex code for very
little benefit. For Super-I/O chips which are multifunction by nature,
the MFD framework is certainly the right way to go.

-- 
Jean Delvare

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

Re: [lm-sensors] [PATCH 5/5] hwmon/sch5636: Add support for the integrated watchdog

[Guenter Roeck]

On Mon, 2011-09-12 at 14:03 -0400, Jean Delvare wrote:
> On Mon, 12 Sep 2011 10:33:46 -0700, Guenter Roeck wrote:
> > On Mon, 2011-09-12 at 05:57 -0400, Hans de Goede wrote:
> > > Add support for the watchdog integrated into the (Fujitsu Theseus version of)
> > > the sch5636 superio hwmon part. Using the new watchdog timer core.
> > > 
> > > Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> > > ---
> > >  drivers/hwmon/sch5636.c |  233 ++++++++++++++++++++++++++++++++++++++++++++++-
> > >  1 files changed, 231 insertions(+), 2 deletions(-)
> > > 
> > [ resent to proper thread ]
> > 
> > Given that this is no longer a pure hwmon driver, it may make sense to
> > split it into separate mfd/hwmon/watchdog drivers. But on the other side
> > I notice that other drivers in the hwmon directory implement watchdog
> > functionality as well. Bad precedent :(.
> 
> IIRC the first driver doing this was one of the FSC drivers and this was
> before the MFD framework was created. It was simply never migrated, and
> other drivers followed the trend.
> 
> > Wonder what happens with those watchdogs if HWMON is disabled.
> 
> Then the watchdog feature isn't available. The idea as I understand it
> is that hardware monitoring is the main feature of these chips and
> watchdog is only an add-on, so we don't expect the user to want the
> watchdog feature without the hardware monitoring feature.
> 
> > Not really sure if we should let this happen, or start being more
> > restrictive and enforce cleaner code. Jean, any thoughts/comments ?
> 
> I have no objection to merging the code as is, as you found out it's not
> doing anything other drivers haven't already.
> 
> If anybody cares, that person can clean up one or more drivers, later.
> That being said, at least for I2C devices, I don't mind leaving things
> as they are, as doing things "better" means more complex code for very
> little benefit. For Super-I/O chips which are multifunction by nature,
> the MFD framework is certainly the right way to go.
> 
Makes sense.

Thanks,
Guenter

Re: [lm-sensors] [PATCH 5/5] hwmon/sch5636: Add support for the

[Guenter Roeck]

On Mon, 2011-09-12 at 14:03 -0400, Jean Delvare wrote:
> On Mon, 12 Sep 2011 10:33:46 -0700, Guenter Roeck wrote:
> > On Mon, 2011-09-12 at 05:57 -0400, Hans de Goede wrote:
> > > Add support for the watchdog integrated into the (Fujitsu Theseus version of)
> > > the sch5636 superio hwmon part. Using the new watchdog timer core.
> > > 
> > > Signed-off-by: Hans de Goede <hdegoede@redhat.com>
> > > ---
> > >  drivers/hwmon/sch5636.c |  233 ++++++++++++++++++++++++++++++++++++++++++++++-
> > >  1 files changed, 231 insertions(+), 2 deletions(-)
> > > 
> > [ resent to proper thread ]
> > 
> > Given that this is no longer a pure hwmon driver, it may make sense to
> > split it into separate mfd/hwmon/watchdog drivers. But on the other side
> > I notice that other drivers in the hwmon directory implement watchdog
> > functionality as well. Bad precedent :(.
> 
> IIRC the first driver doing this was one of the FSC drivers and this was
> before the MFD framework was created. It was simply never migrated, and
> other drivers followed the trend.
> 
> > Wonder what happens with those watchdogs if HWMON is disabled.
> 
> Then the watchdog feature isn't available. The idea as I understand it
> is that hardware monitoring is the main feature of these chips and
> watchdog is only an add-on, so we don't expect the user to want the
> watchdog feature without the hardware monitoring feature.
> 
> > Not really sure if we should let this happen, or start being more
> > restrictive and enforce cleaner code. Jean, any thoughts/comments ?
> 
> I have no objection to merging the code as is, as you found out it's not
> doing anything other drivers haven't already.
> 
> If anybody cares, that person can clean up one or more drivers, later.
> That being said, at least for I2C devices, I don't mind leaving things
> as they are, as doing things "better" means more complex code for very
> little benefit. For Super-I/O chips which are multifunction by nature,
> the MFD framework is certainly the right way to go.
> 
Makes sense.

Thanks,
Guenter



_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors