Watchdog timer core enhancements

Watchdog timer core enhancements

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

Three of the hwmon drivers I maintain are IC's which also include a watchdog
part in their hwmon solution.

This is a resend of a series of watchdog_dev patches which I initially send
on Sep 12 2011:
http://lists.lm-sensors.org/pipermail/lm-sensors/2011-September/033707.html
And which unfortunately has seen 0 reviews / feedback since then.

This resend is re-based on top of v3.3-rc7

Currently the hwmon drivers I maintain are implementing the watchdog API
all by themselves, I would very much like to switch them over to
watchdog_dev to remove a lot of code duplication, but there currently
are 2 things stopping me from moving them over:

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 first 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 second patch in this series fixes this.

Note that the 2nd issue is also hit by the recently posted:
"[PATCH V2 00/22] watchdog: ARM watchdogs: sp805 & mpcore updates & fixes"
patch series which convert the mpcore_wdt driver to watchdog_dev, doing
an unbind between the platform driver and the platform dev there from sysfs
while an app holds the /dev/watchdog file open will make file->private_data
for that file handle point to freed memory!

Once this series is upstream I'll also write a patches to convert all my
hwmon IC with watchdog watchdog driver parts to watchdog_dev.

Regards,

Hans

[PATCH 1/3] 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 file 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.9.3

Re: [PATCH 1/3] watchdog_dev: Add support for having more then 1 watchdog

[Alan Cox]

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

See the patches I posted. I (biased as ever ;)) think they way I've done
it is better, but its also produced no feedback so I'll resend them again
shortly if others also need them.

For x86 we need them as some systems can have both a TCO timer and a
management engine timer.


> +static struct miscdevice watchdog_miscdev[MAX_WATCHDOGS] = { {
> +	.minor		= WATCHDOG_MINOR,
> +	.name		= "watchdog",
> +	.fops		= &watchdog_fops,
> +}, {

The way I did it was to support a legacy /dev/watchdog and a proper range
of watchdogs on their own dynamic major.

> +	.minor		= 212,
> +	.name		= "watchdog1",

Also for submitted patches please don't go taking minor numbers from
miscdev which are statically allocating them without properly registering
them first...


> +	/* Find our watchdog_device */
> +	for (idx = 0; idx < MAX_WATCHDOGS; idx++)
> +		if (watchdog_miscdev[idx].minor == iminor(inode)) {
> +			wdd = wdd_list[idx];
> +			break;
> +		}

Locking model ?


> +	/* No need to lock */
> +	for (idx = 0; idx < MAX_WATCHDOGS; idx++)
> +		if (wdd_list[idx] == watchdog)
> +			break;

What about a parallel open ?

Alan

Re: [PATCH 1/3] watchdog_dev: Add support for having more then 1 watchdog

[Hans de Goede]

Hi,

On 03/15/2012 02:11 PM, Alan Cox wrote:
>> 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.
>
 > For x86 we need them as some systems can have both a TCO timer and a
 > management engine timer.

He he, iTCO + another watchdog exactly the same reason I need support
for this :)

> See the patches I posted. I (biased as ever ;)) think they way I've done
> it is better

I agree it is better, but I never saw the patch as I'm not subscribed
to either one of linux-watchdog or linux-kernel. I've subscribed
myself to linux-watchdog now.

> but its also produced no feedback so I'll resend them again
> shortly if others also need them.

Thanks to Pádraig Brady finding an archived version of your patch
was really easy. I've done a quick review and here are some remarks:

+static unsigned long dog_mask; /* Watcodgs that are in use */

Please fix (spelling) the comment after dog_mask.

+ *     watchdog_open: open the /dev/watchdog device.
+ *     @inode: inode of device
+ *     @file: file handle to device
+ *
+ *     When the /dev/watchdog device gets opened, we start the watchdog.
+ *     Watch out: the /dev/watchdog device is single open, so we make sure
+ *     it can only be opened once.
+ *
+ *     FIXME: review locking on old_wdd v unregister
+ */
+
+static int watchdog_open(struct inode *inode, struct file *file)
+{
+       if (old_wdd != NULL)
+               return watchdog_do_open(old_wdd, inode, file);
+       return -ENXIO;
+}

You set old_wdd to NULL after misc_deregister, and after misc_deregister
has completed watchdog_open can no longer run (watchdog_open runs
with the misc_mtx hold). So you can remove the FIXME as well as the check
for old_wdd being NULL.

+       err = watchdog_assign_id(watchdog, 0);
+       if (err < 0)
+               return err;
+       if (err == 0) {
+               err = misc_register(&watchdog_miscdev);
+               if (err == 0)
+                       old_wdd = watchdog;
+               else {
+                       pr_err("%s: cannot register miscdev on minor=%d (err=%d
+                               watchdog->info->identity, WATCHDOG_MINOR, err);
+                       pr_err("%s: probably a legacy watchdog module is presen
+                               watchdog->info->identity);
+                       watchdog_release_id(watchdog);
+                       err = watchdog_assign_id(watchdog, 1);
+                       if (err < 0)
+                               return err;
+               }
+       }

You may want to check for err == -EBUSY in the fallback instead of assuming
the problem is a legacy watchdog driver

+       err = watchdog_add_device(watchdog);
+       }

You do this even for watchdog0 / for the watchdog we've also registered as
misc_dev. I guess this is intentional and you want the watchdog to be
available as both misc,130 as well as as MAJOR(dog_devt),0 ?

+       if (err < 0) {
+               if (watchdog->id == 0)
+                       old_wdd = NULL;
+               watchdog_release_id(watchdog);

That is missing a misc_deregister(&watchdog_miscdev); under the
if (watchdog->id == 0). IOW this should be:

+       if (err < 0) {
+               if (watchdog->id == 0) {
+                       misc_deregister(&watchdog_miscdev);
+                       old_wdd = NULL;
+               }
+               watchdog_release_id(watchdog);

  int watchdog_dev_unregister(struct watchdog_device *watchdog)
  {
<snip>
+       watchdog_del_device(watchdog);
+       if (watchdog->id == 0) {
+               misc_deregister(&watchdog_miscdev);
+               old_wdd = NULL;
         }

You're calling watchdog_del_device here even for watchdog0, assuming
that the add_device for watchdog0 above is intended that is fine, but
if that was unintended this needs a check too.

Other then these few remarks it looks fine. If you can resend it
with my remarks addressed I'll add a Reviewed-By.

###

To be able to convert my drivers to watchdog_dev too, I really need this
one also:
[PATCH 2/4] watchdog_dev: Add support for dynamically allocated watchdog_device structs

Any chance you could review that? Also if Wim is too busy to take
your multiple device support patch and my dyn alloc watchdog_device
patch, could you then send these to Linus for 3.4?

The reason I'm asking is because I'm about to send patches for adding
watchdog support for the smc sch5627 and smc sch5636 superio hwmon parts,
and my plan was to just use a DIY approach to the watchdog interface given
my watchdog_dev patches seemed to not be getting anywhere. But if these
can go upstream for 3.4 then I would prefer to directly submit a version
using watchdog_dev, rather then converting it later.

###

And here is me response to your review of my multiple devices
support patch, not that it matters much if we decide to go with
yours:

>> +	.minor		= 212,
>> +	.name		= "watchdog1",
>
> Also for submitted patches please don't go taking minor numbers from
> miscdev which are statically allocating them without properly registering
> them first...

These are already used in other drivers, and claimed in
Documentation/devices.txt, they just lack defines which is why
I hardcoded them. The fact that they are already claimed and in
use is why I opted for this approach. But I agree your approach is
better.

>
>
>> +	/* Find our watchdog_device */
>> +	for (idx = 0; idx<  MAX_WATCHDOGS; idx++)
>> +		if (watchdog_miscdev[idx].minor == iminor(inode)) {
>> +			wdd = wdd_list[idx];
>> +			break;
>> +		}
>
> Locking model ?
>
>
>> +	/* No need to lock */
>> +	for (idx = 0; idx<  MAX_WATCHDOGS; idx++)
>> +		if (wdd_list[idx] == watchdog)
>> +			break;
>
> What about a parallel open ?

1) open can not run after misc_deregister completing
2) wdd_list[idx] gets set to NULL after misc_deregister

Thanks & Regards,

Hans
--
To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/3] watchdog_dev: Add support for having more then 1 watchdog

[Alan Cox]

> +static unsigned long dog_mask; /* Watcodgs that are in use */
> 
> Please fix (spelling) the comment after dog_mask.

Oops

> +               err = misc_register(&watchdog_miscdev);
> +               if (err == 0)
> +                       old_wdd = watchdog;
> +               else {
> +                       pr_err("%s: cannot register miscdev on minor=%d (err=%d
> +                               watchdog->info->identity, WATCHDOG_MINOR, err);
> +                       pr_err("%s: probably a legacy watchdog module is presen
> +                               watchdog->info->identity);
> +                       watchdog_release_id(watchdog);
> +                       err = watchdog_assign_id(watchdog, 1);
> +                       if (err < 0)
> +                               return err;
> +               }
> +       }
> 
> You may want to check for err == -EBUSY in the fallback instead of assuming
> the problem is a legacy watchdog driver

Yes that is probably wise

> +       err = watchdog_add_device(watchdog);
> +       }
> 
> You do this even for watchdog0 / for the watchdog we've also registered as
> misc_dev. I guess this is intentional and you want the watchdog to be
> available as both misc,130 as well as as MAJOR(dog_devt),0 ?

Yes - so that it appears in the watchdog class in sysfs for example.

> Other then these few remarks it looks fine. If you can resend it
> with my remarks addressed I'll add a Reviewed-By.

Ok I'll sort those next week.

Thanks

Alan

Re: [PATCH 1/3] watchdog_dev: Add support for having more then 1 watchdog

[Wim Van Sebroeck]

Hi Alan,

> > You do this even for watchdog0 / for the watchdog we've also registered as
> > misc_dev. I guess this is intentional and you want the watchdog to be
> > available as both misc,130 as well as as MAJOR(dog_devt),0 ?
> 
> Yes - so that it appears in the watchdog class in sysfs for example.

What happens if one program opens the miscdevice and another program opens the MAJOR(dog_devt),0 device?
I presume that the WDOG_DEV_OPEN status bit will prohibit the opening of the second device in that case...

Thanks,
Wim.

Re: [PATCH 1/3] watchdog_dev: Add support for having more then 1 watchdog

[Hans de Goede]

Hi,

On 05/04/2012 02:34 PM, Wim Van Sebroeck wrote:
> Hi Alan,
>
>>> You do this even for watchdog0 / for the watchdog we've also registered as
>>> misc_dev. I guess this is intentional and you want the watchdog to be
>>> available as both misc,130 as well as as MAJOR(dog_devt),0 ?
>>
>> Yes - so that it appears in the watchdog class in sysfs for example.
>
> What happens if one program opens the miscdevice and another program opens the MAJOR(dog_devt),0 device?
> I presume that the WDOG_DEV_OPEN status bit will prohibit the opening of the second device in that case...

Correct.

Regards,

Hans

[PATCH 2/3] 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 4b93c28..b43c40c 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: 29-Nov-2011
+Last reviewed: 13-Mar-2012
 
 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
 
   To set the WDOG_NO_WAY_OUT status bit (before registering your watchdog
   timer device) you can either:
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 43ba5b3..132346d 100644
--- a/include/linux/watchdog.h
+++ b/include/linux/watchdog.h
@@ -79,6 +79,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);
@@ -89,6 +91,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.
@@ -101,10 +104,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;
@@ -116,6 +123,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 */
 };
 
 #ifdef CONFIG_WATCHDOG_NOWAYOUT
-- 
1.7.9.3

[PATCH 3/3] 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/staging/mei/wd.c                       |    1 +
 drivers/watchdog/s3c2410_wdt.c                 |    2 ++
 drivers/watchdog/via_wdt.c                     |    1 +
 drivers/watchdog/watchdog_dev.c                |    2 --
 drivers/watchdog/wm831x_wdt.c                  |    1 +
 6 files changed, 9 insertions(+), 5 deletions(-)

diff --git a/Documentation/watchdog/watchdog-kernel-api.txt b/Documentation/watchdog/watchdog-kernel-api.txt
index b43c40c..e7a7de1 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/staging/mei/wd.c b/drivers/staging/mei/wd.c
index 8094941..9503c32 100644
--- a/drivers/staging/mei/wd.c
+++ b/drivers/staging/mei/wd.c
@@ -326,6 +326,7 @@ static int mei_wd_ops_set_timeout(struct watchdog_device *wd_dev, unsigned int t
 
 	dev->wd_timeout = timeout;
 	mei_wd_set_start_timeout(dev, dev->wd_timeout);
+	wd_dev->timeout = dev->wd_timeout;
 
 	mutex_unlock(&dev->device_lock);
 
diff --git a/drivers/watchdog/s3c2410_wdt.c b/drivers/watchdog/s3c2410_wdt.c
index 404172f..6ea79b0 100644
--- a/drivers/watchdog/s3c2410_wdt.c
+++ b/drivers/watchdog/s3c2410_wdt.c
@@ -200,6 +200,8 @@ static int s3c2410wdt_set_heartbeat(struct watchdog_device *wdd, unsigned timeou
 	writel(count, wdt_base + S3C2410_WTDAT);
 	writel(wtcon, wdt_base + S3C2410_WTCON);
 
+	wdd->timeout = (count * divisor) / freq;
+
 	return 0;
 }
 
diff --git a/drivers/watchdog/via_wdt.c b/drivers/watchdog/via_wdt.c
index 8f07dd4..47afe5b 100644
--- a/drivers/watchdog/via_wdt.c
+++ b/drivers/watchdog/via_wdt.c
@@ -126,6 +126,7 @@ static int wdt_set_timeout(struct watchdog_device *wdd,
 {
 	writel(new_timeout, wdt_mem + VIA_WDT_COUNT);
 	timeout = new_timeout;
+	wdd->timeout = timeout;
 	return 0;
 }
 
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;
diff --git a/drivers/watchdog/wm831x_wdt.c b/drivers/watchdog/wm831x_wdt.c
index 263c883..8664ff1 100644
--- a/drivers/watchdog/wm831x_wdt.c
+++ b/drivers/watchdog/wm831x_wdt.c
@@ -157,6 +157,7 @@ static int wm831x_wdt_set_timeout(struct watchdog_device *wdt_dev,
 		ret = wm831x_set_bits(wm831x, WM831X_WATCHDOG,
 				      WM831X_WDOG_TO_MASK,
 				      wm831x_wdt_cfgs[i].val);
+		wdt_dev->timeout = timeout;
 		wm831x_reg_lock(wm831x);
 	} else {
 		dev_err(wm831x->dev, "Failed to unlock security key: %d\n",
-- 
1.7.9.3

Re: Watchdog timer core enhancements

[Pádraig Brady]

On 03/15/2012 11:59 AM, Hans de Goede wrote:
> 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.
> 
> Three of the hwmon drivers I maintain are IC's which also include a watchdog
> part in their hwmon solution.
> 
> This is a resend of a series of watchdog_dev patches which I initially send
> on Sep 12 2011:
> http://lists.lm-sensors.org/pipermail/lm-sensors/2011-September/033707.html
> And which unfortunately has seen 0 reviews / feedback since then.
> 
> This resend is re-based on top of v3.3-rc7
> 
> Currently the hwmon drivers I maintain are implementing the watchdog API
> all by themselves, I would very much like to switch them over to
> watchdog_dev to remove a lot of code duplication, but there currently
> are 2 things stopping me from moving them over:
> 
> 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 first patch in this series adds support for this to the
> watchdog timer driver core (which was quite straightforward to add).

I noticed Alan Cox recently proposed multiple watchdog device support
https://lkml.org/lkml/2012/2/28/473

cheers,
Pádraig.
--
To unsubscribe from this list: send the line "unsubscribe linux-watchdog" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Watchdog timer core enhancements

[Wim Van Sebroeck]

Hi Hans,

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

Let me also give a short intro about myself: As a hobby I maintain the
linux-watchdog drivers since years now. I'm not paid by anyone to do that and
I'm doing this in my spare time. Apart from that I have a wive and 2 children
that also need attention. In real life I'm a freelancer (doing mostly network
and telecoms stuff) and that's how I try to survive.
If you then add the fact that on the 12th of February an upgrade of my home
system made sure that I lost a big part of my e-mail :-( and made sure that
I didn't had outgoing and local mail anymore :-( then you understand probably
why I have been struggling the last month... FYI: the last week I have been
busy working on my backlog of patches for the watchdog trees, before doing
anything else (like responding to e-mails).

> This is a resend of a series of watchdog_dev patches which I initially send
> on Sep 12 2011:
> http://lists.lm-sensors.org/pipermail/lm-sensors/2011-September/033707.html
> And which unfortunately has seen 0 reviews / feedback since then.

I have been looking at these 5 patches last week. This is what I think of them:
patch 1: Use wddev function parameter instead of wdd global ->
	was allready sent out by someone else also and thus has been incorporated
patch 2: watchdog_dev: Add support for having more then 1 watchdog -> 
	I am sure that we all agree that we need support for multiple watchdogs.
	But I am not convinced we have the right approach for it yet.
	I saw that you started looking at Alan's Code also.
	Please continue the discussion about this.
	The support for multiple watchdogs is not for this merge window, but for
	the one after that.
patch 3: watchdog_dev: Add support for dynamically allocated watchdog_device structs ->
	Have to look in more detail on that patch, but I think this is a good thing to add.
	I'm trying to get that one still in before the merge window opens (together
	with the ep93xx_wdt conversion and the sp805 and mpcore patches from Viresh
	(but no promisses here))
patch 4: watchdog_dev: Let the driver update the timeout field on set_timeout success ->
	This patch is in linux-watchdog-next since yesterday (and it was in my testing tree for a week).
	It's in linux-next since this morning together with fixes for the drivers that allready use the watchdog-api.
patch 5: hwmon/sch5636: Add support for the integrated watchdog ->
	Depends off-course on patch 2.

> This resend is re-based on top of v3.3-rc7
> 
> Currently the hwmon drivers I maintain are implementing the watchdog API
> all by themselves, I would very much like to switch them over to
> watchdog_dev to remove a lot of code duplication, but there currently
> are 2 things stopping me from moving them over:
> 
> 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 first 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 second patch in this series fixes this.
> 
> Note that the 2nd issue is also hit by the recently posted:
> "[PATCH V2 00/22] watchdog: ARM watchdogs: sp805 & mpcore updates & fixes"
> patch series which convert the mpcore_wdt driver to watchdog_dev, doing
> an unbind between the platform driver and the platform dev there from sysfs
> while an app holds the /dev/watchdog file open will make file->private_data
> for that file handle point to freed memory!
> 
> Once this series is upstream I'll also write a patches to convert all my
> hwmon IC with watchdog watchdog driver parts to watchdog_dev.

You points are valid, but as said, I'm not convinced yet we have a good solution
for multiple watchdogs. But I'm sure we are getting there.

I do appreciate your patches and comments and hope to not frustrate you to much
because I am not able to respond quicker. My sincere apologies for that, although
I cannot change this easily.

Also a quick note about the linux-watchdog mailing list: there is an archive at:
http://www.spinics.net/lists/linux-watchdog/

Kind regards,
Wim.

Re: Watchdog timer core enhancements

[Hans de Goede]

Hi,

On 03/15/2012 10:13 PM, Wim Van Sebroeck wrote:
> Hi Hans,
>
>> 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.
>
> Let me also give a short intro about myself: As a hobby I maintain the
> linux-watchdog drivers since years now. I'm not paid by anyone to do that and
> I'm doing this in my spare time. Apart from that I have a wive and 2 children
> that also need attention. In real life I'm a freelancer (doing mostly network
> and telecoms stuff) and that's how I try to survive.

I completely understand, my intend of the resend was not to complain, merely to
hopefully get some attention for it. FWIW despite my email address my kernel
contributions are mostly a hobby too.

> If you then add the fact that on the 12th of February an upgrade of my home
> system made sure that I lost a big part of my e-mail :-( and made sure that
> I didn't had outgoing and local mail anymore :-( then you understand probably
> why I have been struggling the last month... FYI: the last week I have been
> busy working on my backlog of patches for the watchdog trees, before doing
> anything else (like responding to e-mails).

Sorry to hear that, and I'm glad to see that you're back in business wrt you
email.

>> This is a resend of a series of watchdog_dev patches which I initially send
>> on Sep 12 2011:
>> http://lists.lm-sensors.org/pipermail/lm-sensors/2011-September/033707.html
>> And which unfortunately has seen 0 reviews / feedback since then.
>
> I have been looking at these 5 patches last week. This is what I think of them:
> patch 1: Use wddev function parameter instead of wdd global ->
> 	was allready sent out by someone else also and thus has been incorporated
> patch 2: watchdog_dev: Add support for having more then 1 watchdog ->
> 	I am sure that we all agree that we need support for multiple watchdogs.
> 	But I am not convinced we have the right approach for it yet.
> 	I saw that you started looking at Alan's Code also.
> 	Please continue the discussion about this.

I've reviewed Alan's patch and I agree that his approach is better, as soon as
he resends his patch with some (small) remarks from the review addressed. I'll
add my Reviewed-by.

> 	The support for multiple watchdogs is not for this merge window, but for
> 	the one after that.

Ok.

> patch 3: watchdog_dev: Add support for dynamically allocated watchdog_device structs ->
> 	Have to look in more detail on that patch, but I think this is a good thing to add.
> 	I'm trying to get that one still in before the merge window opens (together
> 	with the ep93xx_wdt conversion and the sp805 and mpcore patches from Viresh
> 	(but no promisses here))

As I already said in my previous mail, please note that Viresh's patches for
mpcore run into the same issue I'm running into. To clarify the problem a bit,
it could be reproduced on a (hypothetical) mpcore arm system by:

Have an app open /dev/watchdog and keep that app running.

Then do the following as root:
cd /sys/bus/platform/drivers/mpcore_wdt
ls

You should see the name of the watchdog platform dev here
(as a symlink), something among the lines of: mpcore_wdt.XXXX

Now you can unbind the platform driver from the platform dev
doing the following:
cd /sys/bus/platform/drivers/mpcore_wdt
echo mpcore_wdt.XXXX > unbind
ls

With the ls the symlink should be gone. And /dev/watchdog
too, as the mpcore_wdt_remove function has now run! This
also means that the mpcore_wdt struct, which contains the
watchdog_device struct has been free-ed.

But the app which opened /dev/watchdog still has an open
fd to it, and when calls are made on that, watchdog_dev.c
will try to use file->private_data which now points
to free-ed memory!

I admit that triggering this is not easy and not something which
will happen accidentally, but still I believe this is something which
we ought to fix.

> patch 4: watchdog_dev: Let the driver update the timeout field on set_timeout success ->
> 	This patch is in linux-watchdog-next since yesterday (and it was in my testing tree for a week).
> 	It's in linux-next since this morning together with fixes for the drivers that allready use the watchdog-api.

Great, thanks.


> patch 5: hwmon/sch5636: Add support for the integrated watchdog ->
> 	Depends off-course on patch 2.

It depends more on patch 3 then on 2. it will still work without patch 2,
but to test (and use it on some systems) I would need to blacklist the iTCO
module.

Also I've recently received information on how the watchdog in the sch5627
works, And it turns out to be exactly the same (*). So please drop this one
from your queue I'm working on a new version where most of the code is
shared between the sch5627 and sch5636 drivers (there already is a
sch56xx-common file used by both).

*) But it needs the BIOS to setup / poke some motherboard specific things,
so it only works on boards with BIOS support for the wd


Thanks & Regards,

Hans

Re: Watchdog timer core enhancements

[Wolfram Sang]

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


> If you then add the fact that on the 12th of February an upgrade of my home
> system made sure that I lost a big part of my e-mail :-( and made sure that
> I didn't had outgoing and local mail anymore :-( then you understand probably

Sorry to hear about that!

> why I have been struggling the last month... FYI: the last week I have been
> busy working on my backlog of patches for the watchdog trees, before doing
> anything else (like responding to e-mails).

One small email what happened and that you are already catching up might be a
good idea :)

Thanks for the work!

   Wolfram

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

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