Skip to contentSkip to conversation

Generic Watchdog Timer Driver

RFC PATCH Superseded Wim Van Sebroeck 3 participants 5 messages Feb 23, 2011 – Mar 3, 2011

Subsystems (2)
Documentation Watchdog device drivers

v1 of 4 Superseded Latest is v4 →

lore
Share & tools
lore

Wim Van Sebroeck

author
#0 lore raw

RFC PATCH 6/10 Generic Watchdog Timer Driver

commit afe3de93859443b575407f39a2e655956c02e088

Author: Wim Van Sebroeck <[email protected]>
Date:   Sun Jul 18 10:39:00 2010 +0000

watchdog: WatchDog Timer Driver Core - Part 6

Since we don't want that a driver module can be removed while the watchdog timer is still active, we lock the driver module (by incremeting the reference counter). After a clean close of the watchdog driver we unlock the driver module.
If /dev/watchdog is closed uncleanly (and the watchdog is still active) then we do not unlock the driver module, but keep it, so that next time /dev/watchdog get's opened we can continue triggering the watchdog.

Signed-off-by Alan Cox <[email protected]>
Signed-off-by Wim Van Sebroeck <[email protected]>
Documentation/watchdog/src/watchdog-with-timer-example.c +1 -0
index f1d4f217..604fd91 100644
@@ -127,6 +127,7 @@ static const struct watchdog_info wdt_info = {
 };
 
 static const struct watchdog_ops wdt_ops = {
+	.owner =	THIS_MODULE,
 	.start =	wdt_start,
 	.stop =		wdt_stop,
 	.ping =		wdt_ping,
Documentation/watchdog/watchdog-kernel-api.txt +10 -0
index f15e8d4..472bfea 100644
@@ -63,6 +63,7 @@ It contains following fields:
 The list of watchdog operations is defined as:
 
 struct watchdog_ops {
+	struct module *owner;
 	/* mandatory operations */
 	int (*start)(struct watchdog_device *);
 	int (*stop)(struct watchdog_device *);

@@ -72,6 +73,10 @@ struct watchdog_ops {
 	int (*set_timeout)(struct watchdog_device *, int);
 };
 
+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. (You don't want to unload the module when you're
+watchdog timer device is still active).
 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

@@ -121,3 +126,8 @@ bit-operations. The status bit's that are defined are:
 * WDOG_DEV_OPEN: this status bit shows whether or not the watchdog device
   was opened via /dev/watchdog.
   (This bit should only be used by the WatchDog Timer Driver Core).
+* WDOG_ORPHAN: when /dev/watchdog was closed, but the watchdog is still active
+  then we don't unload the module. This bit is set when this situation occurs.
+  When re-opening /dev/watchdog before a reboot occurs, you can then still use
+  and ping the watchdog timer device.
+  (This bit should only be used by the WatchDog Timer Driver Core).
drivers/watchdog/core/watchdog_core.c +4 -0
index 52bc520..d1a824e 100644
@@ -60,6 +60,10 @@ int register_watchdogdevice(struct watchdog_device *wdd)
 	if (wdd == NULL || wdd->info == NULL || wdd->ops == NULL)
 		return -ENODATA;
 
+	/* Make sure that the owner of the watchdog operations exists */
+	if (wdd->ops->owner == NULL)
+		return -ENODATA;
+
 	/* Make sure that the mandatory operations are supported */
 	if (wdd->ops->start == NULL || wdd->ops->stop == NULL)
 		return -ENODATA;
drivers/watchdog/core/watchdog_dev.c +20 -3
index b80c6e6..d3dfac2 100644
@@ -259,7 +259,7 @@ static long watchdog_ioctl(struct file *file, unsigned int cmd,
 
 static int watchdog_open(struct inode *inode, struct file *file)
 {
-	int err;
+	int err = -EBUSY;
 
 	trace("%p, %p", inode, file);
 

@@ -267,14 +267,26 @@ static int watchdog_open(struct inode *inode, struct file *file)
 	if (test_and_set_bit(WDOG_DEV_OPEN, &wdd->status))
 		return -EBUSY;
 
+	/* if the /dev/watchdog device is open, we don't want the module
+	 * to be unloaded. */
+	if (!try_module_get(wdd->ops->owner))
+		goto out;
+
 	/* start the watchdog */
 	err = watchdog_start(wdd);
 	if (err < 0)
-		goto out;
+		goto out_mod;
+
+	/* We leaked a reference to lock the module in on close
+	 * now we can reclaim it as we re-opened before triggering */
+	if (test_and_clear_bit(WDOG_ORPHAN, &wdd->status))
+		module_put(wdd->ops->owner);
 
 	/* dev/watchdog is a virtual (and thus non-seekable) filesystem */
 	return nonseekable_open(inode, file);
 
+out_mod:
+	module_put(wdd->ops->owner);
 out:
 	clear_bit(WDOG_DEV_OPEN, &wdd->status);
 	return err;

@@ -296,8 +308,13 @@ static int watchdog_release(struct inode *inode, struct file *file)
 
 	/* stop the watchdog */
 	err = watchdog_stop(wdd);
-	if (err < 0) {
+
+	/* If the watchdog stopped correctly we let the module unload again */
+	if (err == 0)
+		module_put(wdd->ops->owner);
+	else {	/* If not we deliberately leak a module reference */
 		printk(KERN_CRIT "%s: not stopping watchdog!\n", wdd->name);
+		set_bit(WDOG_ORPHAN, &wdd->status);
 		watchdog_ping(wdd);
 	}
include/linux/watchdog.h +3 -0
index e35f51f..ba08f38 100644
@@ -64,6 +64,7 @@ struct watchdog_device;
 
 /* The watchdog-devices operations */
 struct watchdog_ops {
+	struct module *owner;
 	/* mandatory operations */
 	int (*start)(struct watchdog_device *);
 	int (*stop)(struct watchdog_device *);

@@ -84,6 +85,8 @@ struct watchdog_device {
 #define WDOG_ACTIVE		0	/* is the watchdog running/active */
 #define WDOG_DEV_OPEN		1	/* is the watchdog opened via
 					 * /dev/watchdog */
+#define WDOG_ORPHAN		2	/* is the device module still loaded
+					 * after closing /dev/watchdog */
 };
 
 /* drivers/watchdog/core/watchdog_core.c */
#1 lore raw
Quoted 16 lines · Wim Van Sebroeck

commit afe3de93859443b575407f39a2e655956c02e088

Author: Wim Van Sebroeck<[email protected]>
Date:   Sun Jul 18 10:39:00 2010 +0000

watchdog: WatchDog Timer Driver Core - Part 6

Since we don't want that a driver module can be removed while the watchdog timer is still active, we lock the driver module (by incremeting the reference counter). After a clean close of the watchdog driver we unlock the driver module.
If /dev/watchdog is closed uncleanly (and the watchdog is still active) then we do not unlock the driver module, but keep it, so that next time /dev/watchdog get's opened we can continue triggering the watchdog.

Quoted 3 lines
Signed-off-by Alan Cox<[email protected]>
Signed-off-by Wim Van Sebroeck<[email protected]>

*snip*

drivers/watchdog/core/watchdog_core.c int register_watchdogdevice(struct watchdog_device *wdd) · line 65

Quoted 11 lines
drivers/watchdog/core/watchdog_core.c +3 -0
index 52bc520..d1a824e 100644
@@ -60,6 +60,10 @@ int register_watchdogdevice(struct watchdog_device *wdd)
  	if (wdd == NULL || wdd->info == NULL || wdd->ops == NULL)
  		return -ENODATA;
 
+	/* Make sure that the owner of the watchdog operations exists */
+	if (wdd->ops->owner == NULL)
+		return -ENODATA;

This check is invalid when the driver is built in.

author
#2 lore raw

Hi Mike,

Quoted 8 lines
Quoted 8 lines
drivers/watchdog/core/watchdog_core.c +0 -0
index 52bc520..d1a824e 100644
@@ -60,6 +60,10 @@ int register_watchdogdevice(struct watchdog_device *wdd)
  	if (wdd == NULL || wdd->info == NULL || wdd->ops == NULL)
  		return -ENODATA;
Quoted 5 lines

drivers/watchdog/core/watchdog_core.c int register_watchdogdevice(struct watchdog_device *wdd) · line 65

Quoted 3 lines
drivers/watchdog/core/watchdog_core.c +3 -0
+	/* Make sure that the owner of the watchdog operations exists */
+	if (wdd->ops->owner == NULL)
+		return -ENODATA;

This check is invalid when the driver is built in.

Will have a look at it. Thanks!

Kind regards,
Wim.

#3 lore raw
In reply to Wim Van Sebroeck
Quoted 6 lines · Wim Van Sebroeck

commit afe3de93859443b575407f39a2e655956c02e088

Author: Wim Van Sebroeck <[email protected]>
Date:   Sun Jul 18 10:39:00 2010 +0000

watchdog: WatchDog Timer Driver Core - Part 6

Quoted 9 lines

Since we don't want that a driver module can be removed while the watchdog timer is still active, we lock the driver module (by incremeting the reference counter). After a clean close of the watchdog driver we unlock the driver module.
If /dev/watchdog is closed uncleanly (and the watchdog is still active) then we do not unlock the driver module, but keep it, so that next time /dev/watchdog get's opened we can continue triggering the watchdog.

Ahh, I was looking for too much in the earlier patches! Apologies.

Quoted 40 lines
Signed-off-by Alan Cox <[email protected]>
Signed-off-by Wim Van Sebroeck <[email protected]>
Documentation/watchdog/src/watchdog-with-timer-example.c +1 -0
index f1d4f217..604fd91 100644
@@ -127,6 +127,7 @@ static const struct watchdog_info wdt_info = {
 };
 
 static const struct watchdog_ops wdt_ops = {
+	.owner =	THIS_MODULE,
 	.start =	wdt_start,
 	.stop =		wdt_stop,
 	.ping =		wdt_ping,
Documentation/watchdog/watchdog-kernel-api.txt +5 -0
index f15e8d4..472bfea 100644
@@ -63,6 +63,7 @@ It contains following fields:
 The list of watchdog operations is defined as:
 
 struct watchdog_ops {
+	struct module *owner;
 	/* mandatory operations */
 	int (*start)(struct watchdog_device *);
 	int (*stop)(struct watchdog_device *);

@@ -72,6 +73,10 @@ struct watchdog_ops {
 	int (*set_timeout)(struct watchdog_device *, int);
 };
 
+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. (You don't want to unload the module when you're
+watchdog timer device is still active).
 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

Documentation/watchdog/watchdog-kernel-api.txt bit-operations. The status bit's that are defined are: · line 133

Quoted 8 lines
Documentation/watchdog/watchdog-kernel-api.txt +5 -0
@@ -121,3 +126,8 @@ bit-operations. The status bit's that are defined are:
 * WDOG_DEV_OPEN: this status bit shows whether or not the watchdog device
   was opened via /dev/watchdog.
   (This bit should only be used by the WatchDog Timer Driver Core).
+* WDOG_ORPHAN: when /dev/watchdog was closed, but the watchdog is still active
+  then we don't unload the module. This bit is set when this situation occurs.
+  When re-opening /dev/watchdog before a reboot occurs, you can then still use
+  and ping the watchdog timer device.
+  (This bit should only be used by the WatchDog Timer Driver Core).

I don't fully understand why the module refcounting is done in the open and release though. If you moved it into the registration and unregistration then doesn't that remove the need for WDOG_ORPHAN?

drivers/watchdog/core/watchdog_core.c int register_watchdogdevice(struct watchdog_device *wdd) · line 65

Quoted 11 lines
drivers/watchdog/core/watchdog_core.c +3 -0
index 52bc520..d1a824e 100644
@@ -60,6 +60,10 @@ int register_watchdogdevice(struct watchdog_device *wdd)
 	if (wdd == NULL || wdd->info == NULL || wdd->ops == NULL)
 		return -ENODATA;
 
+	/* Make sure that the owner of the watchdog operations exists */
+	if (wdd->ops->owner == NULL)
+		return -ENODATA;

Won't this be effectively NULL if the module is builtin? It looks like if it is builtin then THIS_MODULE would be defined as (struct module *)0.

Quoted 18 lines
drivers/watchdog/core/watchdog_core.c +1 -0
+
 	/* Make sure that the mandatory operations are supported */
 	if (wdd->ops->start == NULL || wdd->ops->stop == NULL)
 		return -ENODATA;
drivers/watchdog/core/watchdog_dev.c +1 -1
index b80c6e6..d3dfac2 100644
@@ -259,7 +259,7 @@ static long watchdog_ioctl(struct file *file, unsigned int cmd,
 
 static int watchdog_open(struct inode *inode, struct file *file)
 {
-	int err;
+	int err = -EBUSY;
 
 	trace("%p, %p", inode, file);

drivers/watchdog/core/watchdog_dev.c static int watchdog_open(struct inode *inode, struct file *file) · line 268

Quoted 2 lines
drivers/watchdog/core/watchdog_dev.c +0 -0
@@ -267,14 +267,26 @@ static int watchdog_open(struct inode *inode, struct file *file)
 	if (test_and_set_bit(WDOG_DEV_OPEN, &wdd->status))
 		return -EBUSY;

Couldn't wdd be potentially invalid if the module had been unloaded?

drivers/watchdog/core/watchdog_dev.c static int watchdog_open(struct inode *inode, struct file *file) · line 273

Quoted 5 lines
drivers/watchdog/core/watchdog_dev.c +4 -0
+	/* if the /dev/watchdog device is open, we don't want the module
+	 * to be unloaded. */
+	if (!try_module_get(wdd->ops->owner))
+		goto out;

Same here.

Quoted 60 lines
drivers/watchdog/core/watchdog_dev.c +15 -2
+
 	/* start the watchdog */
 	err = watchdog_start(wdd);
 	if (err < 0)
-		goto out;
+		goto out_mod;
+
+	/* We leaked a reference to lock the module in on close
+	 * now we can reclaim it as we re-opened before triggering */
+	if (test_and_clear_bit(WDOG_ORPHAN, &wdd->status))
+		module_put(wdd->ops->owner);
 
 	/* dev/watchdog is a virtual (and thus non-seekable) filesystem */
 	return nonseekable_open(inode, file);
 
+out_mod:
+	module_put(wdd->ops->owner);
 out:
 	clear_bit(WDOG_DEV_OPEN, &wdd->status);
 	return err;

@@ -296,8 +308,13 @@ static int watchdog_release(struct inode *inode, struct file *file)
 
 	/* stop the watchdog */
 	err = watchdog_stop(wdd);
-	if (err < 0) {
+
+	/* If the watchdog stopped correctly we let the module unload again */
+	if (err == 0)
+		module_put(wdd->ops->owner);
+	else {	/* If not we deliberately leak a module reference */
 		printk(KERN_CRIT "%s: not stopping watchdog!\n", wdd->name);
+		set_bit(WDOG_ORPHAN, &wdd->status);
 		watchdog_ping(wdd);
 	}
include/linux/watchdog.h +3 -1
index e35f51f..ba08f38 100644
@@ -64,6 +64,7 @@ struct watchdog_device;
 
 /* The watchdog-devices operations */
 struct watchdog_ops {
+	struct module *owner;
 	/* mandatory operations */
 	int (*start)(struct watchdog_device *);
 	int (*stop)(struct watchdog_device *);

@@ -84,6 +85,8 @@ struct watchdog_device {
 #define WDOG_ACTIVE		0	/* is the watchdog running/active */
 #define WDOG_DEV_OPEN		1	/* is the watchdog opened via
 					 * /dev/watchdog */
+#define WDOG_ORPHAN		2	/* is the device module still loaded
+					 * after closing /dev/watchdog */
 };
 
 /* drivers/watchdog/core/watchdog_core.c */
--

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

author
#4 lore raw

Hi Jamie,

Quoted 3 lines

I don't fully understand why the module refcounting is done in the open and release though. If you moved it into the registration and unregistration then doesn't that remove the need for WDOG_ORPHAN?

Hmm, nice suggestion. Will look into that.

Quoted 8 lines
Quoted 8 lines
drivers/watchdog/core/watchdog_core.c +0 -0
index 52bc520..d1a824e 100644
@@ -60,6 +60,10 @@ int register_watchdogdevice(struct watchdog_device *wdd)
 	if (wdd == NULL || wdd->info == NULL || wdd->ops == NULL)
 		return -ENODATA;
Quoted 7 lines

drivers/watchdog/core/watchdog_core.c int register_watchdogdevice(struct watchdog_device *wdd) · line 65

Quoted 3 lines
drivers/watchdog/core/watchdog_core.c +3 -0
+	/* Make sure that the owner of the watchdog operations exists */
+	if (wdd->ops->owner == NULL)
+		return -ENODATA;

Won't this be effectively NULL if the module is builtin? It looks like if it is builtin then THIS_MODULE would be defined as (struct module *)0.

Same for this: I will look into this.

Thanks,
Wim.