From 257f8c4aae392654d4ab846030b9f4518f16ed32 Mon Sep 17 00:00:00 2001 From: Viresh Kumar Date: Mon, 12 Mar 2012 09:51:56 +0530 Subject: watchdog: Add watchdog_active() routine Some watchdog may need to check if watchdog is ACTIVE or not, for example in their suspend/resume hooks. This patch adds this routine and changes the core drivers to use it. Signed-off-by: Viresh Kumar Signed-off-by: Wim Van Sebroeck --- include/linux/watchdog.h | 6 ++++++ 1 file changed, 6 insertions(+) (limited to 'include') diff --git a/include/linux/watchdog.h b/include/linux/watchdog.h index ac40716b44e9..1984ea610577 100644 --- a/include/linux/watchdog.h +++ b/include/linux/watchdog.h @@ -128,6 +128,12 @@ struct watchdog_device { #define WATCHDOG_NOWAYOUT_INIT_STATUS 0 #endif +/* Use the following function to check wether or not the watchdog is active */ +static inline bool watchdog_active(struct watchdog_device *wdd) +{ + return test_bit(WDOG_ACTIVE, &wdd->status); +} + /* Use the following function to set the nowayout feature */ static inline void watchdog_set_nowayout(struct watchdog_device *wdd, bool nowayout) { -- cgit v1.2.3 From 45f5fed30a6460ec58f159ff297a2974153a97de Mon Sep 17 00:00:00 2001 From: Alan Cox Date: Thu, 10 May 2012 21:48:59 +0200 Subject: watchdog: Add multiple device support We keep the old /dev/watchdog interface file for the first watchdog via miscdev. This is basically a cut and paste of the relevant interface code from the rtc driver layer tweaked for watchdog. Revised to fix problems noted by Hans de Goede Signed-off-by: Alan Cox Signed-off-by: Hans de Goede Signed-off-by: Tomas Winkler Signed-off-by: Wim Van Sebroeck --- Documentation/watchdog/watchdog-kernel-api.txt | 10 +- drivers/watchdog/watchdog_core.c | 43 ++++++++- drivers/watchdog/watchdog_core.h | 4 + drivers/watchdog/watchdog_dev.c | 127 ++++++++++++++++--------- include/linux/watchdog.h | 6 ++ 5 files changed, 140 insertions(+), 50 deletions(-) (limited to 'include') diff --git a/Documentation/watchdog/watchdog-kernel-api.txt b/Documentation/watchdog/watchdog-kernel-api.txt index 25fe4304f2fc..3c85fc7dc1f1 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: 16-Mar-2012 +Last reviewed: 21-May-2012 Wim Van Sebroeck @@ -39,6 +39,8 @@ watchdog_device structure. The watchdog device structure looks like this: struct watchdog_device { + int id; + struct cdev cdev; const struct watchdog_info *info; const struct watchdog_ops *ops; unsigned int bootstatus; @@ -50,6 +52,12 @@ struct watchdog_device { }; It contains following fields: +* id: set by watchdog_register_device, id 0 is special. It has both a + /dev/watchdog0 cdev (dynamic major, minor 0) as well as the old + /dev/watchdog miscdev. The id is set automatically when calling + watchdog_register_device. +* cdev: cdev for the dynamic /dev/watchdog device nodes. This + field is also populated by watchdog_register_device. * 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. diff --git a/drivers/watchdog/watchdog_core.c b/drivers/watchdog/watchdog_core.c index 8598308278d3..5f9879369003 100644 --- a/drivers/watchdog/watchdog_core.c +++ b/drivers/watchdog/watchdog_core.c @@ -34,9 +34,12 @@ #include /* For printk/panic/... */ #include /* For watchdog specific items */ #include /* For __init/__exit/... */ +#include /* For ida_* macros */ #include "watchdog_core.h" /* For watchdog_dev_register/... */ +static DEFINE_IDA(watchdog_ida); + /** * watchdog_register_device() - register a watchdog device * @wdd: watchdog device @@ -49,7 +52,7 @@ */ int watchdog_register_device(struct watchdog_device *wdd) { - int ret; + int ret, id; if (wdd == NULL || wdd->info == NULL || wdd->ops == NULL) return -EINVAL; @@ -74,11 +77,28 @@ int watchdog_register_device(struct watchdog_device *wdd) * corrupted in a later stage then we expect a kernel panic! */ - /* We only support 1 watchdog device via the /dev/watchdog interface */ + id = ida_simple_get(&watchdog_ida, 0, MAX_DOGS, GFP_KERNEL); + if (id < 0) + return id; + wdd->id = id; + ret = watchdog_dev_register(wdd); if (ret) { - pr_err("error registering /dev/watchdog (err=%d)\n", ret); - return ret; + ida_simple_remove(&watchdog_ida, id); + if (!(id == 0 && ret == -EBUSY)) + return ret; + + /* Retry in case a legacy watchdog module exists */ + id = ida_simple_get(&watchdog_ida, 1, MAX_DOGS, GFP_KERNEL); + if (id < 0) + return id; + wdd->id = id; + + ret = watchdog_dev_register(wdd); + if (ret) { + ida_simple_remove(&watchdog_ida, id); + return ret; + } } return 0; @@ -102,9 +122,24 @@ void watchdog_unregister_device(struct watchdog_device *wdd) ret = watchdog_dev_unregister(wdd); if (ret) pr_err("error unregistering /dev/watchdog (err=%d)\n", ret); + ida_simple_remove(&watchdog_ida, wdd->id); } EXPORT_SYMBOL_GPL(watchdog_unregister_device); +static int __init watchdog_init(void) +{ + return watchdog_dev_init(); +} + +static void __exit watchdog_exit(void) +{ + watchdog_dev_exit(); + ida_destroy(&watchdog_ida); +} + +subsys_initcall(watchdog_init); +module_exit(watchdog_exit); + MODULE_AUTHOR("Alan Cox "); MODULE_AUTHOR("Wim Van Sebroeck "); MODULE_DESCRIPTION("WatchDog Timer Driver Core"); diff --git a/drivers/watchdog/watchdog_core.h b/drivers/watchdog/watchdog_core.h index 80503f229385..6c951418fca7 100644 --- a/drivers/watchdog/watchdog_core.h +++ b/drivers/watchdog/watchdog_core.h @@ -26,8 +26,12 @@ * This material is provided "AS-IS" and at no charge. */ +#define MAX_DOGS 32 /* Maximum number of watchdog devices */ + /* * Functions/procedures to be called by the core */ extern int watchdog_dev_register(struct watchdog_device *); extern int watchdog_dev_unregister(struct watchdog_device *); +extern int __init watchdog_dev_init(void); +extern void __exit watchdog_dev_exit(void); diff --git a/drivers/watchdog/watchdog_dev.c b/drivers/watchdog/watchdog_dev.c index beaf9cb5541a..3b22582bcb04 100644 --- a/drivers/watchdog/watchdog_dev.c +++ b/drivers/watchdog/watchdog_dev.c @@ -44,10 +44,10 @@ #include "watchdog_core.h" -/* make sure we only register one /dev/watchdog device */ -static unsigned long watchdog_dev_busy; +/* the dev_t structure to store the dynamically allocated watchdog devices */ +static dev_t watchdog_devt; /* the watchdog device behind /dev/watchdog */ -static struct watchdog_device *wdd; +static struct watchdog_device *old_wdd; /* * watchdog_ping: ping the watchdog. @@ -138,6 +138,7 @@ static int watchdog_stop(struct watchdog_device *wddev) static ssize_t watchdog_write(struct file *file, const char __user *data, size_t len, loff_t *ppos) { + struct watchdog_device *wdd = file->private_data; size_t i; char c; @@ -177,6 +178,7 @@ static ssize_t watchdog_write(struct file *file, const char __user *data, static long watchdog_ioctl(struct file *file, unsigned int cmd, unsigned long arg) { + struct watchdog_device *wdd = file->private_data; void __user *argp = (void __user *)arg; int __user *p = argp; unsigned int val; @@ -249,11 +251,11 @@ static long watchdog_ioctl(struct file *file, unsigned int cmd, } /* - * watchdog_open: open the /dev/watchdog device. + * watchdog_open: open the /dev/watchdog* devices. * @inode: inode of device * @file: file handle to device * - * When the /dev/watchdog device gets opened, we start the watchdog. + * When the /dev/watchdog* device gets opened, we start the watchdog. * Watch out: the /dev/watchdog device is single open, so we make sure * it can only be opened once. */ @@ -261,6 +263,13 @@ static long watchdog_ioctl(struct file *file, unsigned int cmd, static int watchdog_open(struct inode *inode, struct file *file) { int err = -EBUSY; + struct watchdog_device *wdd; + + /* Get the corresponding watchdog device */ + if (imajor(inode) == MISC_MAJOR) + wdd = old_wdd; + else + wdd = container_of(inode->i_cdev, struct watchdog_device, cdev); /* the watchdog is single open! */ if (test_and_set_bit(WDOG_DEV_OPEN, &wdd->status)) @@ -277,6 +286,8 @@ static int watchdog_open(struct inode *inode, struct file *file) if (err < 0) goto out_mod; + file->private_data = wdd; + /* dev/watchdog is a virtual (and thus non-seekable) filesystem */ return nonseekable_open(inode, file); @@ -288,9 +299,9 @@ out: } /* - * watchdog_release: release the /dev/watchdog device. - * @inode: inode of device - * @file: file handle to device + * watchdog_release: release the watchdog device. + * @inode: inode of device + * @file: file handle to device * * This is the code for when /dev/watchdog gets closed. We will only * stop the watchdog when we have received the magic char (and nowayout @@ -299,6 +310,7 @@ out: static int watchdog_release(struct inode *inode, struct file *file) { + struct watchdog_device *wdd = file->private_data; int err = -EBUSY; /* @@ -340,62 +352,87 @@ static struct miscdevice watchdog_miscdev = { }; /* - * watchdog_dev_register: + * watchdog_dev_register: register a watchdog device * @watchdog: watchdog device * - * Register a watchdog device as /dev/watchdog. /dev/watchdog - * is actually a miscdevice and thus we set it up like that. + * Register a watchdog device including handling the legacy + * /dev/watchdog node. /dev/watchdog is actually a miscdevice and + * thus we set it up like that. */ int watchdog_dev_register(struct watchdog_device *watchdog) { - int err; - - /* Only one device can register for /dev/watchdog */ - if (test_and_set_bit(0, &watchdog_dev_busy)) { - pr_err("only one watchdog can use /dev/watchdog\n"); - return -EBUSY; + int err, devno; + + if (watchdog->id == 0) { + err = misc_register(&watchdog_miscdev); + if (err != 0) { + pr_err("%s: cannot register miscdev on minor=%d (err=%d).\n", + watchdog->info->identity, WATCHDOG_MINOR, err); + if (err == -EBUSY) + pr_err("%s: a legacy watchdog module is probably present.\n", + watchdog->info->identity); + return err; + } + old_wdd = watchdog; } - wdd = watchdog; - - err = misc_register(&watchdog_miscdev); - if (err != 0) { - pr_err("%s: cannot register miscdev on minor=%d (err=%d)\n", - watchdog->info->identity, WATCHDOG_MINOR, err); - goto out; + /* Fill in the data structures */ + devno = MKDEV(MAJOR(watchdog_devt), watchdog->id); + cdev_init(&watchdog->cdev, &watchdog_fops); + watchdog->cdev.owner = watchdog->ops->owner; + + /* Add the device */ + err = cdev_add(&watchdog->cdev, devno, 1); + if (err) { + pr_err("watchdog%d unable to add device %d:%d\n", + watchdog->id, MAJOR(watchdog_devt), watchdog->id); + if (watchdog->id == 0) { + misc_deregister(&watchdog_miscdev); + old_wdd = NULL; + } } - - return 0; - -out: - wdd = NULL; - clear_bit(0, &watchdog_dev_busy); return err; } /* - * watchdog_dev_unregister: + * watchdog_dev_unregister: unregister a watchdog device * @watchdog: watchdog device * - * Deregister the /dev/watchdog device. + * Unregister the watchdog and if needed the legacy /dev/watchdog device. */ int watchdog_dev_unregister(struct watchdog_device *watchdog) { - /* Check that a watchdog device was registered in the past */ - if (!test_bit(0, &watchdog_dev_busy) || !wdd) - return -ENODEV; - - /* We can only unregister the watchdog device that was registered */ - if (watchdog != wdd) { - pr_err("%s: watchdog was not registered as /dev/watchdog\n", - watchdog->info->identity); - return -ENODEV; + cdev_del(&watchdog->cdev); + if (watchdog->id == 0) { + misc_deregister(&watchdog_miscdev); + old_wdd = NULL; } - - misc_deregister(&watchdog_miscdev); - wdd = NULL; - clear_bit(0, &watchdog_dev_busy); return 0; } + +/* + * watchdog_dev_init: init dev part of watchdog core + * + * Allocate a range of chardev nodes to use for watchdog devices + */ + +int __init watchdog_dev_init(void) +{ + int err = alloc_chrdev_region(&watchdog_devt, 0, MAX_DOGS, "watchdog"); + if (err < 0) + pr_err("watchdog: unable to allocate char dev region\n"); + return err; +} + +/* + * watchdog_dev_exit: exit dev part of watchdog core + * + * Release the range of chardev nodes used for watchdog devices + */ + +void __exit watchdog_dev_exit(void) +{ + unregister_chrdev_region(watchdog_devt, MAX_DOGS); +} diff --git a/include/linux/watchdog.h b/include/linux/watchdog.h index 1984ea610577..508d56399e6d 100644 --- a/include/linux/watchdog.h +++ b/include/linux/watchdog.h @@ -54,6 +54,8 @@ struct watchdog_info { #ifdef __KERNEL__ #include +#include +#include struct watchdog_ops; struct watchdog_device; @@ -89,6 +91,8 @@ struct watchdog_ops { /** struct watchdog_device - The structure that defines a watchdog device * + * @id: The watchdog's ID. (Allocated by watchdog_register_device) + * @cdev: The watchdog's Character device. * @info: Pointer to a watchdog_info structure. * @ops: Pointer to the list of watchdog operations. * @bootstatus: Status of the watchdog device at boot. @@ -105,6 +109,8 @@ struct watchdog_ops { * via the watchdog_set_drvdata and watchdog_get_drvdata helpers. */ struct watchdog_device { + int id; + struct cdev cdev; const struct watchdog_info *info; const struct watchdog_ops *ops; unsigned int bootstatus; -- cgit v1.2.3 From 2bbeed016dd96045ec82c3a309afddcc3a0db1d2 Mon Sep 17 00:00:00 2001 From: Alan Cox Date: Fri, 11 May 2012 12:00:19 +0200 Subject: watchdog: Add a flag to indicate the watchdog doesn't reboot things Some watchdogs merely trigger external alarms and controls. In a managed environment this is very useful but we want drivers to be able to figure out which is which now multiple dogs can be loaded. Thus add an ALARMONLY feature flag. Signed-off-by: Alan Cox Signed-off-by: Hans de Goede Signed-off-by: Wim Van Sebroeck --- include/linux/watchdog.h | 2 ++ 1 file changed, 2 insertions(+) (limited to 'include') diff --git a/include/linux/watchdog.h b/include/linux/watchdog.h index 508d56399e6d..32678a50f98d 100644 --- a/include/linux/watchdog.h +++ b/include/linux/watchdog.h @@ -45,6 +45,8 @@ struct watchdog_info { #define WDIOF_SETTIMEOUT 0x0080 /* Set timeout (in seconds) */ #define WDIOF_MAGICCLOSE 0x0100 /* Supports magic close char */ #define WDIOF_PRETIMEOUT 0x0200 /* Pretimeout (in seconds), get/set */ +#define WDIOF_ALARMONLY 0x0400 /* Watchdog triggers a management or + other external alarm not a reboot */ #define WDIOF_KEEPALIVEPING 0x8000 /* Keep alive ping reply */ #define WDIOS_DISABLECARD 0x0001 /* Turn off the watchdog timer */ -- cgit v1.2.3 From d6b469d915ae348b3bb8b25034063d6870ff4a00 Mon Sep 17 00:00:00 2001 From: Alan Cox Date: Fri, 11 May 2012 12:00:20 +0200 Subject: watchdog: create all the proper device files Create the watchdog class and it's associated devices. Signed-off-by: Alan Cox Signed-off-by: Hans de Goede Signed-off-by: Wim Van Sebroeck --- Documentation/watchdog/watchdog-kernel-api.txt | 5 ++++ drivers/watchdog/watchdog_core.c | 34 ++++++++++++++++++++++++-- drivers/watchdog/watchdog_dev.c | 1 + include/linux/watchdog.h | 4 +++ 4 files changed, 42 insertions(+), 2 deletions(-) (limited to 'include') diff --git a/Documentation/watchdog/watchdog-kernel-api.txt b/Documentation/watchdog/watchdog-kernel-api.txt index 3c85fc7dc1f1..ce1fa22aa70b 100644 --- a/Documentation/watchdog/watchdog-kernel-api.txt +++ b/Documentation/watchdog/watchdog-kernel-api.txt @@ -41,6 +41,8 @@ The watchdog device structure looks like this: struct watchdog_device { int id; struct cdev cdev; + struct device *dev; + struct device *parent; const struct watchdog_info *info; const struct watchdog_ops *ops; unsigned int bootstatus; @@ -58,6 +60,9 @@ It contains following fields: watchdog_register_device. * cdev: cdev for the dynamic /dev/watchdog device nodes. This field is also populated by watchdog_register_device. +* dev: device under the watchdog class (created by watchdog_register_device). +* parent: set this to the parent device (or NULL) before calling + watchdog_register_device. * 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. diff --git a/drivers/watchdog/watchdog_core.c b/drivers/watchdog/watchdog_core.c index 5f9879369003..86a57673abf9 100644 --- a/drivers/watchdog/watchdog_core.c +++ b/drivers/watchdog/watchdog_core.c @@ -35,10 +35,12 @@ #include /* For watchdog specific items */ #include /* For __init/__exit/... */ #include /* For ida_* macros */ +#include /* For IS_ERR macros */ #include "watchdog_core.h" /* For watchdog_dev_register/... */ static DEFINE_IDA(watchdog_ida); +static struct class *watchdog_class; /** * watchdog_register_device() - register a watchdog device @@ -52,7 +54,7 @@ static DEFINE_IDA(watchdog_ida); */ int watchdog_register_device(struct watchdog_device *wdd) { - int ret, id; + int ret, id, devno; if (wdd == NULL || wdd->info == NULL || wdd->ops == NULL) return -EINVAL; @@ -101,6 +103,16 @@ int watchdog_register_device(struct watchdog_device *wdd) } } + devno = wdd->cdev.dev; + wdd->dev = device_create(watchdog_class, wdd->parent, devno, + NULL, "watchdog%d", wdd->id); + if (IS_ERR(wdd->dev)) { + watchdog_dev_unregister(wdd); + ida_simple_remove(&watchdog_ida, id); + ret = PTR_ERR(wdd->dev); + return ret; + } + return 0; } EXPORT_SYMBOL_GPL(watchdog_register_device); @@ -115,6 +127,7 @@ EXPORT_SYMBOL_GPL(watchdog_register_device); void watchdog_unregister_device(struct watchdog_device *wdd) { int ret; + int devno = wdd->cdev.dev; if (wdd == NULL) return; @@ -122,18 +135,35 @@ void watchdog_unregister_device(struct watchdog_device *wdd) ret = watchdog_dev_unregister(wdd); if (ret) pr_err("error unregistering /dev/watchdog (err=%d)\n", ret); + device_destroy(watchdog_class, devno); ida_simple_remove(&watchdog_ida, wdd->id); + wdd->dev = NULL; } EXPORT_SYMBOL_GPL(watchdog_unregister_device); static int __init watchdog_init(void) { - return watchdog_dev_init(); + int err; + + watchdog_class = class_create(THIS_MODULE, "watchdog"); + if (IS_ERR(watchdog_class)) { + pr_err("couldn't create class\n"); + return PTR_ERR(watchdog_class); + } + + err = watchdog_dev_init(); + if (err < 0) { + class_destroy(watchdog_class); + return err; + } + + return 0; } static void __exit watchdog_exit(void) { watchdog_dev_exit(); + class_destroy(watchdog_class); ida_destroy(&watchdog_ida); } diff --git a/drivers/watchdog/watchdog_dev.c b/drivers/watchdog/watchdog_dev.c index 3b22582bcb04..1f011f2d6e48 100644 --- a/drivers/watchdog/watchdog_dev.c +++ b/drivers/watchdog/watchdog_dev.c @@ -365,6 +365,7 @@ int watchdog_dev_register(struct watchdog_device *watchdog) int err, devno; if (watchdog->id == 0) { + watchdog_miscdev.parent = watchdog->parent; err = misc_register(&watchdog_miscdev); if (err != 0) { pr_err("%s: cannot register miscdev on minor=%d (err=%d).\n", diff --git a/include/linux/watchdog.h b/include/linux/watchdog.h index 32678a50f98d..c3545c5d918a 100644 --- a/include/linux/watchdog.h +++ b/include/linux/watchdog.h @@ -95,6 +95,8 @@ struct watchdog_ops { * * @id: The watchdog's ID. (Allocated by watchdog_register_device) * @cdev: The watchdog's Character device. + * @dev: The device for our watchdog + * @parent: The parent bus device * @info: Pointer to a watchdog_info structure. * @ops: Pointer to the list of watchdog operations. * @bootstatus: Status of the watchdog device at boot. @@ -113,6 +115,8 @@ struct watchdog_ops { struct watchdog_device { int id; struct cdev cdev; + struct device *dev; + struct device *parent; const struct watchdog_info *info; const struct watchdog_ops *ops; unsigned int bootstatus; -- cgit v1.2.3 From f4e9c82f64b524314a390b13d3ba7d483f09258f Mon Sep 17 00:00:00 2001 From: Hans de Goede Date: Tue, 22 May 2012 11:40:26 +0200 Subject: watchdog: Add Locking support This patch fixes some potential multithreading issues, despite only allowing one process to open the /dev/watchdog device, we can still get called multiple times at the same time, since a program could be using thread, or could share the fd after a fork. This causes 2 potential problems: 1) watchdog_start / open do an unlocked test_n_set / test_n_clear, if these 2 race, the watchdog could be stopped while the active bit indicates it is running or visa versa. 2) Most watchdog_dev drivers probably assume that only one watchdog-op will get called at a time, this is not necessary true atm. Signed-off-by: Hans de Goede Signed-off-by: Wim Van Sebroeck --- Documentation/watchdog/watchdog-kernel-api.txt | 2 ++ drivers/watchdog/watchdog_core.c | 1 + drivers/watchdog/watchdog_dev.c | 21 +++++++++++++++++++++ include/linux/watchdog.h | 5 +++++ 4 files changed, 29 insertions(+) (limited to 'include') diff --git a/Documentation/watchdog/watchdog-kernel-api.txt b/Documentation/watchdog/watchdog-kernel-api.txt index ce1fa22aa70b..08d34e11bc54 100644 --- a/Documentation/watchdog/watchdog-kernel-api.txt +++ b/Documentation/watchdog/watchdog-kernel-api.txt @@ -50,6 +50,7 @@ struct watchdog_device { unsigned int min_timeout; unsigned int max_timeout; void *driver_data; + struct mutex lock; unsigned long status; }; @@ -74,6 +75,7 @@ It contains following fields: * driver_data: a pointer to the drivers private data of a watchdog device. This data should only be accessed via the watchdog_set_drvdata and watchdog_get_drvdata routines. +* lock: Mutex for WatchDog Timer Driver Core internal use only. * status: this field contains a number of status bits that give extra information about the status of the device (Like: is the watchdog timer running/active, is the nowayout bit set, is the device opened via diff --git a/drivers/watchdog/watchdog_core.c b/drivers/watchdog/watchdog_core.c index 86a57673abf9..6aa46a90ff02 100644 --- a/drivers/watchdog/watchdog_core.c +++ b/drivers/watchdog/watchdog_core.c @@ -79,6 +79,7 @@ int watchdog_register_device(struct watchdog_device *wdd) * corrupted in a later stage then we expect a kernel panic! */ + mutex_init(&wdd->lock); id = ida_simple_get(&watchdog_ida, 0, MAX_DOGS, GFP_KERNEL); if (id < 0) return id; diff --git a/drivers/watchdog/watchdog_dev.c b/drivers/watchdog/watchdog_dev.c index 76d2572fed25..4d295d229a07 100644 --- a/drivers/watchdog/watchdog_dev.c +++ b/drivers/watchdog/watchdog_dev.c @@ -63,6 +63,8 @@ static int watchdog_ping(struct watchdog_device *wddev) { int err = 0; + mutex_lock(&wddev->lock); + if (!watchdog_active(wddev)) goto out_ping; @@ -72,6 +74,7 @@ static int watchdog_ping(struct watchdog_device *wddev) err = wddev->ops->start(wddev); /* restart watchdog */ out_ping: + mutex_unlock(&wddev->lock); return err; } @@ -88,6 +91,8 @@ static int watchdog_start(struct watchdog_device *wddev) { int err = 0; + mutex_lock(&wddev->lock); + if (watchdog_active(wddev)) goto out_start; @@ -96,6 +101,7 @@ static int watchdog_start(struct watchdog_device *wddev) set_bit(WDOG_ACTIVE, &wddev->status); out_start: + mutex_unlock(&wddev->lock); return err; } @@ -113,6 +119,8 @@ static int watchdog_stop(struct watchdog_device *wddev) { int err = 0; + mutex_lock(&wddev->lock); + if (!watchdog_active(wddev)) goto out_stop; @@ -127,6 +135,7 @@ static int watchdog_stop(struct watchdog_device *wddev) clear_bit(WDOG_ACTIVE, &wddev->status); out_stop: + mutex_unlock(&wddev->lock); return err; } @@ -147,8 +156,11 @@ static int watchdog_get_status(struct watchdog_device *wddev, if (!wddev->ops->status) return -EOPNOTSUPP; + mutex_lock(&wddev->lock); + *status = wddev->ops->status(wddev); + mutex_unlock(&wddev->lock); return err; } @@ -171,8 +183,11 @@ static int watchdog_set_timeout(struct watchdog_device *wddev, (timeout < wddev->min_timeout || timeout > wddev->max_timeout)) return -EINVAL; + mutex_lock(&wddev->lock); + err = wddev->ops->set_timeout(wddev, timeout); + mutex_unlock(&wddev->lock); return err; } @@ -193,8 +208,11 @@ static int watchdog_get_timeleft(struct watchdog_device *wddev, if (!wddev->ops->get_timeleft) return -EOPNOTSUPP; + mutex_lock(&wddev->lock); + *timeleft = wddev->ops->get_timeleft(wddev); + mutex_unlock(&wddev->lock); return err; } @@ -213,8 +231,11 @@ static int watchdog_ioctl_op(struct watchdog_device *wddev, unsigned int cmd, if (!wddev->ops->ioctl) return -ENOIOCTLCMD; + mutex_lock(&wddev->lock); + err = wddev->ops->ioctl(wddev, cmd, arg); + mutex_unlock(&wddev->lock); return err; } diff --git a/include/linux/watchdog.h b/include/linux/watchdog.h index c3545c5d918a..da1dc1b52744 100644 --- a/include/linux/watchdog.h +++ b/include/linux/watchdog.h @@ -104,6 +104,7 @@ struct watchdog_ops { * @min_timeout:The watchdog devices minimum timeout value. * @max_timeout:The watchdog devices maximum timeout value. * @driver-data:Pointer to the drivers private data. + * @lock: Lock for watchdog core internal use only. * @status: Field that contains the devices internal status bits. * * The watchdog_device structure contains all information about a @@ -111,6 +112,9 @@ 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 { int id; @@ -124,6 +128,7 @@ struct watchdog_device { unsigned int min_timeout; unsigned int max_timeout; void *driver_data; + struct mutex lock; unsigned long status; /* Bit numbers for status flags */ #define WDOG_ACTIVE 0 /* Is the watchdog running/active */ -- cgit v1.2.3 From e907df32725204d6d2cb79b872529911c8eadcdf Mon Sep 17 00:00:00 2001 From: Hans de Goede Date: Tue, 22 May 2012 11:40:26 +0200 Subject: watchdog: Add support for dynamically allocated watchdog_device structs 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 Signed-off-by: Wim Van Sebroeck --- Documentation/watchdog/watchdog-kernel-api.txt | 28 ++++++++++++- drivers/watchdog/watchdog_dev.c | 55 +++++++++++++++++++++++++- include/linux/watchdog.h | 5 +++ 3 files changed, 86 insertions(+), 2 deletions(-) (limited to 'include') diff --git a/Documentation/watchdog/watchdog-kernel-api.txt b/Documentation/watchdog/watchdog-kernel-api.txt index 08d34e11bc54..086638f6c82d 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: 21-May-2012 +Last reviewed: 22-May-2012 Wim Van Sebroeck @@ -93,6 +93,8 @@ struct watchdog_ops { unsigned int (*status)(struct watchdog_device *); int (*set_timeout)(struct watchdog_device *, unsigned int); unsigned int (*get_timeleft)(struct watchdog_device *); + void (*ref)(struct watchdog_device *); + void (*unref)(struct watchdog_device *); long (*ioctl)(struct watchdog_device *, unsigned int, unsigned long); }; @@ -100,6 +102,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 @@ -140,6 +157,10 @@ they are supported. These optional routines/operations are: (Note: the WDIOF_SETTIMEOUT needs to be set in the options field of the watchdog's info structure). * get_timeleft: this routines returns the time that's left before a reset. +* ref: the operation that calls kref_get on the kref of a dynamically + allocated watchdog_device struct. +* unref: the operation that calls kref_put on the kref of a dynamically + allocated watchdog_device struct. * ioctl: if this routine is present then it will be called first before we do our own internal ioctl call handling. This routine should return -ENOIOCTLCMD if a command is not supported. The parameters that are passed to the ioctl @@ -159,6 +180,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 4d295d229a07..672d169bf1da 100644 --- a/drivers/watchdog/watchdog_dev.c +++ b/drivers/watchdog/watchdog_dev.c @@ -65,6 +65,11 @@ static int watchdog_ping(struct watchdog_device *wddev) mutex_lock(&wddev->lock); + if (test_bit(WDOG_UNREGISTERED, &wddev->status)) { + err = -ENODEV; + goto out_ping; + } + if (!watchdog_active(wddev)) goto out_ping; @@ -93,6 +98,11 @@ static int watchdog_start(struct watchdog_device *wddev) mutex_lock(&wddev->lock); + if (test_bit(WDOG_UNREGISTERED, &wddev->status)) { + err = -ENODEV; + goto out_start; + } + if (watchdog_active(wddev)) goto out_start; @@ -121,6 +131,11 @@ static int watchdog_stop(struct watchdog_device *wddev) mutex_lock(&wddev->lock); + if (test_bit(WDOG_UNREGISTERED, &wddev->status)) { + err = -ENODEV; + goto out_stop; + } + if (!watchdog_active(wddev)) goto out_stop; @@ -158,8 +173,14 @@ static int watchdog_get_status(struct watchdog_device *wddev, mutex_lock(&wddev->lock); + if (test_bit(WDOG_UNREGISTERED, &wddev->status)) { + err = -ENODEV; + goto out_status; + } + *status = wddev->ops->status(wddev); +out_status: mutex_unlock(&wddev->lock); return err; } @@ -185,8 +206,14 @@ static int watchdog_set_timeout(struct watchdog_device *wddev, mutex_lock(&wddev->lock); + if (test_bit(WDOG_UNREGISTERED, &wddev->status)) { + err = -ENODEV; + goto out_timeout; + } + err = wddev->ops->set_timeout(wddev, timeout); +out_timeout: mutex_unlock(&wddev->lock); return err; } @@ -210,8 +237,14 @@ static int watchdog_get_timeleft(struct watchdog_device *wddev, mutex_lock(&wddev->lock); + if (test_bit(WDOG_UNREGISTERED, &wddev->status)) { + err = -ENODEV; + goto out_timeleft; + } + *timeleft = wddev->ops->get_timeleft(wddev); +out_timeleft: mutex_unlock(&wddev->lock); return err; } @@ -233,8 +266,14 @@ static int watchdog_ioctl_op(struct watchdog_device *wddev, unsigned int cmd, mutex_lock(&wddev->lock); + if (test_bit(WDOG_UNREGISTERED, &wddev->status)) { + err = -ENODEV; + goto out_ioctl; + } + err = wddev->ops->ioctl(wddev, cmd, arg); +out_ioctl: mutex_unlock(&wddev->lock); return err; } @@ -398,6 +437,9 @@ static int watchdog_open(struct inode *inode, struct file *file) 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); @@ -434,7 +476,10 @@ static int watchdog_release(struct inode *inode, struct file *file) /* If the watchdog was not stopped, send a keepalive ping */ if (err < 0) { - dev_crit(wdd->dev, "watchdog did not stop!\n"); + mutex_lock(&wdd->lock); + if (!test_bit(WDOG_UNREGISTERED, &wdd->status)) + dev_crit(wdd->dev, "watchdog did not stop!\n"); + mutex_unlock(&wdd->lock); watchdog_ping(wdd); } @@ -444,6 +489,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; } @@ -515,6 +564,10 @@ int watchdog_dev_register(struct watchdog_device *watchdog) int watchdog_dev_unregister(struct watchdog_device *watchdog) { + mutex_lock(&watchdog->lock); + set_bit(WDOG_UNREGISTERED, &watchdog->status); + mutex_unlock(&watchdog->lock); + cdev_del(&watchdog->cdev); if (watchdog->id == 0) { misc_deregister(&watchdog_miscdev); diff --git a/include/linux/watchdog.h b/include/linux/watchdog.h index da1dc1b52744..da70f0facd2b 100644 --- a/include/linux/watchdog.h +++ b/include/linux/watchdog.h @@ -71,6 +71,8 @@ struct watchdog_device; * @status: The routine that shows the status of the watchdog device. * @set_timeout:The routine for setting the watchdog devices timeout value. * @get_timeleft:The routine that get's the time that's left before a reset. + * @ref: The ref operation for dyn. allocated watchdog_device structs + * @unref: The unref operation for dyn. allocated watchdog_device structs * @ioctl: The routines that handles extra ioctl calls. * * The watchdog_ops structure contains a list of low-level operations @@ -88,6 +90,8 @@ struct watchdog_ops { unsigned int (*status)(struct watchdog_device *); int (*set_timeout)(struct watchdog_device *, unsigned int); unsigned int (*get_timeleft)(struct watchdog_device *); + void (*ref)(struct watchdog_device *); + void (*unref)(struct watchdog_device *); long (*ioctl)(struct watchdog_device *, unsigned int, unsigned long); }; @@ -135,6 +139,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 /* Has the device been unregistered */ }; #ifdef CONFIG_WATCHDOG_NOWAYOUT -- cgit v1.2.3