All variables required by the watchdog core to manage a watchdog are
currently stored in struct watchdog_device. The lifetime of those
variables is determined by the watchdog driver. However, the lifetime
of variables used by the watchdog core differs from the lifetime of
struct watchdog_device. To remedy this situation, watchdog drivers
can implement ref and unref callbacks, to be used by the watchdog
core to lock struct watchdog_device in memory. This mechanism was
introduced with commit e907df327252 ("watchdog: Add support for
dynamically allocated watchdog_device structs").
While this solves the immediate problem, it depends on watchdog drivers
to actually implement the ref/unref callbacks. This is error prone,
often not implemented in the first place, or not implemented correctly.
To solve the problem without requiring driver support, split the variables
in struct watchdog_device into two data structures - one for variables
associated with the watchdog driver, one for variables associated with
the watchdog core. With this approach, the watchdog core can keep track
of the variables it uses and no longer depends on ref/unref callbacks
in the driver. As a side effect, some of the variables originally in struct
watchdog_driver are now private to the watchdog core and no longer visible
in watchdog drivers.
The 'ref' and 'unref' callbacks in struct watchdog_driver are no longer
used and marked as deprecated.
Patch 1/5 moves watchdog device creation from watchdog_core.c to watchdog_dev.c
to simplify watchdog device handling.
Patch 2/5 separates variables in watchdog_device based on variable lifetime.
Patch 3/5 to 5/5 remove existing ref/unref functions from the drivers
implementing it.
The series applies on top of the current watchdog-next as well as the pending
patches introducing sysfs support ("watchdog: Use static struct class
watchdog_class in stead of pointer" and "watchdog: Read device status through
sysfs attributes") by Pratyush Anand.
The watchdog character device is currently created in watchdog_dev.c,
and the watchdog device in watchdog_core.c. This results in
cross-dependencies, since device creation needs to know the watchdog
character device number as well as the watchdog class, both of which
reside in watchdog_dev.c.
Create the watchdog device in watchdog_dev.c to simplify the code.
Inspired by earlier patch set from Damien Riegel.
Cc: Damien Riegel <[email protected]>
Signed-off-by: Guenter Roeck <[email protected]>
---
drivers/watchdog/watchdog_core.c | 33 ++++--------------
drivers/watchdog/watchdog_core.h | 4 +--
drivers/watchdog/watchdog_dev.c | 73 +++++++++++++++++++++++++++++++++-------
3 files changed, 69 insertions(+), 41 deletions(-)
diff --git a/drivers/watchdog/watchdog_core.c b/drivers/watchdog/watchdog_core.c
index 551af042867c..f0293f7d2b80 100644
--- a/drivers/watchdog/watchdog_core.c
+++ b/drivers/watchdog/watchdog_core.c
@@ -42,7 +42,6 @@
#include "watchdog_core.h" /* For watchdog_dev_register/... */
static DEFINE_IDA(watchdog_ida);
-static struct class *watchdog_class;
/*
* Deferred Registration infrastructure.
@@ -194,7 +193,7 @@ EXPORT_SYMBOL_GPL(watchdog_set_restart_priority);
static int __watchdog_register_device(struct watchdog_device *wdd)
{
- int ret, id = -1, devno;
+ int ret, id = -1;
if (wdd == NULL || wdd->info == NULL || wdd->ops == NULL)
return -EINVAL;
@@ -247,16 +246,6 @@ static int __watchdog_register_device(struct watchdog_device *wdd)
}
}
- devno = wdd->cdev.dev;
- wdd->dev = device_create(watchdog_class, wdd->parent, devno,
- wdd, "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;
- }
-
if (test_bit(WDOG_STOP_ON_REBOOT, &wdd->status)) {
wdd->reboot_nb.notifier_call = watchdog_reboot_notifier;
@@ -265,9 +254,7 @@ static int __watchdog_register_device(struct watchdog_device *wdd)
dev_err(wdd->dev, "Cannot register reboot notifier (%d)\n",
ret);
watchdog_dev_unregister(wdd);
- device_destroy(watchdog_class, devno);
ida_simple_remove(&watchdog_ida, wdd->id);
- wdd->dev = NULL;
return ret;
}
}
@@ -311,9 +298,6 @@ EXPORT_SYMBOL_GPL(watchdog_register_device);
static void __watchdog_unregister_device(struct watchdog_device *wdd)
{
- int ret;
- int devno;
-
if (wdd == NULL)
return;
@@ -323,13 +307,8 @@ static void __watchdog_unregister_device(struct watchdog_device *wdd)
if (test_bit(WDOG_STOP_ON_REBOOT, &wdd->status))
unregister_reboot_notifier(&wdd->reboot_nb);
- devno = wdd->cdev.dev;
- ret = watchdog_dev_unregister(wdd);
- if (ret)
- pr_err("error unregistering /dev/watchdog (err=%d)\n", ret);
- device_destroy(watchdog_class, devno);
+ watchdog_dev_unregister(wdd);
ida_simple_remove(&watchdog_ida, wdd->id);
- wdd->dev = NULL;
}
/**
@@ -370,9 +349,11 @@ static int __init watchdog_deferred_registration(void)
static int __init watchdog_init(void)
{
- watchdog_class = watchdog_dev_init();
- if (IS_ERR(watchdog_class))
- return PTR_ERR(watchdog_class);
+ int err;
+
+ err = watchdog_dev_init();
+ if (err < 0)
+ return err;
watchdog_deferred_registration();
return 0;
diff --git a/drivers/watchdog/watchdog_core.h b/drivers/watchdog/watchdog_core.h
index 1c8d6b0e68c7..86ff962d1e15 100644
--- a/drivers/watchdog/watchdog_core.h
+++ b/drivers/watchdog/watchdog_core.h
@@ -32,6 +32,6 @@
* 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 struct class * __init watchdog_dev_init(void);
+extern void 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 d93118e97d11..c24392623e98 100644
--- a/drivers/watchdog/watchdog_dev.c
+++ b/drivers/watchdog/watchdog_dev.c
@@ -626,17 +626,18 @@ static struct miscdevice watchdog_miscdev = {
};
/*
- * watchdog_dev_register: register a watchdog device
+ * watchdog_cdev_register: register watchdog character device
* @wdd: watchdog device
+ * @devno: character device number
*
- * Register a watchdog device including handling the legacy
+ * Register a watchdog character 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 *wdd)
+static int watchdog_cdev_register(struct watchdog_device *wdd, dev_t devno)
{
- int err, devno;
+ int err;
if (wdd->id == 0) {
old_wdd = wdd;
@@ -654,7 +655,6 @@ int watchdog_dev_register(struct watchdog_device *wdd)
}
/* Fill in the data structures */
- devno = MKDEV(MAJOR(watchdog_devt), wdd->id);
cdev_init(&wdd->cdev, &watchdog_fops);
wdd->cdev.owner = wdd->ops->owner;
@@ -672,13 +672,14 @@ int watchdog_dev_register(struct watchdog_device *wdd)
}
/*
- * watchdog_dev_unregister: unregister a watchdog device
+ * watchdog_cdev_unregister: unregister watchdog character device
* @watchdog: watchdog device
*
- * Unregister the watchdog and if needed the legacy /dev/watchdog device.
+ * Unregister watchdog character device and if needed the legacy
+ * /dev/watchdog device.
*/
-int watchdog_dev_unregister(struct watchdog_device *wdd)
+static void watchdog_cdev_unregister(struct watchdog_device *wdd)
{
mutex_lock(&wdd->lock);
set_bit(WDOG_UNREGISTERED, &wdd->status);
@@ -689,7 +690,6 @@ int watchdog_dev_unregister(struct watchdog_device *wdd)
misc_deregister(&watchdog_miscdev);
old_wdd = NULL;
}
- return 0;
}
static struct class watchdog_class = {
@@ -701,29 +701,76 @@ static struct class watchdog_class = {
};
/*
+ * watchdog_dev_register: register a watchdog device
+ * @wdd: watchdog device
+ *
+ * Register a watchdog device including handling the legacy
+ * /dev/watchdog node. /dev/watchdog is actually a miscdevice and
+ * thus we set it up like that.
+ */
+
+int watchdog_dev_register(struct watchdog_device *wdd)
+{
+ struct device *dev;
+ dev_t devno;
+ int ret;
+
+ devno = MKDEV(MAJOR(watchdog_devt), wdd->id);
+
+ ret = watchdog_cdev_register(wdd, devno);
+ if (ret)
+ return ret;
+
+ dev = device_create(&watchdog_class, wdd->parent, devno, wdd,
+ "watchdog%d", wdd->id);
+ if (IS_ERR(dev)) {
+ watchdog_cdev_unregister(wdd);
+ return PTR_ERR(dev);
+ }
+ wdd->dev = dev;
+
+ return ret;
+}
+
+/*
+ * watchdog_dev_unregister: unregister a watchdog device
+ * @watchdog: watchdog device
+ *
+ * Unregister watchdog device and if needed the legacy
+ * /dev/watchdog device.
+ */
+
+void watchdog_dev_unregister(struct watchdog_device *wdd)
+{
+ watchdog_cdev_unregister(wdd);
+ device_destroy(&watchdog_class, wdd->dev->devt);
+ wdd->dev = NULL;
+}
+
+/*
* watchdog_dev_init: init dev part of watchdog core
*
* Allocate a range of chardev nodes to use for watchdog devices
*/
-struct class * __init watchdog_dev_init(void)
+int __init watchdog_dev_init(void)
{
int err;
err = class_register(&watchdog_class);
if (err < 0) {
pr_err("couldn't register class\n");
- return ERR_PTR(err);
+ return err;
}
err = alloc_chrdev_region(&watchdog_devt, 0, MAX_DOGS, "watchdog");
if (err < 0) {
pr_err("watchdog: unable to allocate char dev region\n");
class_unregister(&watchdog_class);
- return ERR_PTR(err);
+ return err;
}
- return &watchdog_class;
+ return 0;
}
/*
--
2.1.4
All variables required by the watchdog core to manage a watchdog are
currently stored in struct watchdog_device. The lifetime of those
variables is determined by the watchdog driver. However, the lifetime
of variables used by the watchdog core differs from the lifetime of
struct watchdog_device. To remedy this situation, watchdog drivers
can implement ref and unref callbacks, to be used by the watchdog
core to lock struct watchdog_device in memory.
While this solves the immediate problem, it depends on watchdog drivers
to actually implement the ref/unref callbacks. This is error prone,
often not implemented in the first place, or not implemented correctly.
To solve the problem without requiring driver support, split the variables
in struct watchdog_device into two data structures - one for variables
associated with the watchdog driver, one for variables associated with
the watchdog core. With this approach, the watchdog core can keep track
of its variable lifetime and no longer depends on ref/unref callbacks
in the driver. As a side effect, some of the variables originally in
struct watchdog_driver are now private to the watchdog core and no longer
visible in watchdog drivers.
The 'ref' and 'unref' callbacks in struct watchdog_driver are no longer
used and marked as deprecated.
Signed-off-by: Guenter Roeck <[email protected]>
---
Documentation/watchdog/watchdog-kernel-api.txt | 45 +--
drivers/watchdog/watchdog_core.c | 2 -
drivers/watchdog/watchdog_core.h | 23 ++
drivers/watchdog/watchdog_dev.c | 377 +++++++++++++------------
include/linux/watchdog.h | 21 +-
5 files changed, 239 insertions(+), 229 deletions(-)
diff --git a/Documentation/watchdog/watchdog-kernel-api.txt b/Documentation/watchdog/watchdog-kernel-api.txt
index 0a37da76acef..3db5092924e5 100644
--- a/Documentation/watchdog/watchdog-kernel-api.txt
+++ b/Documentation/watchdog/watchdog-kernel-api.txt
@@ -44,7 +44,6 @@ 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;
@@ -56,7 +55,7 @@ struct watchdog_device {
struct notifier_block reboot_nb;
struct notifier_block restart_nb;
void *driver_data;
- struct mutex lock;
+ void *wdd_data;
unsigned long status;
struct list_head deferred;
};
@@ -66,8 +65,6 @@ It contains following fields:
/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<id> 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.
@@ -89,11 +86,10 @@ 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.
+* wdd_data: a pointer to watchdog core internal data.
* 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
- the /dev/watchdog interface or not, ...).
+ running/active, or is the nowayout bit set).
* deferred: entry in wtd_deferred_reg_list which is used to
register early initialized watchdogs.
@@ -110,8 +106,8 @@ struct watchdog_ops {
int (*set_timeout)(struct watchdog_device *, unsigned int);
unsigned int (*get_timeleft)(struct watchdog_device *);
int (*restart)(struct watchdog_device *);
- void (*ref)(struct watchdog_device *);
- void (*unref)(struct watchdog_device *);
+ void (*ref)(struct watchdog_device *) __deprecated;
+ void (*unref)(struct watchdog_device *) __deprecated;
long (*ioctl)(struct watchdog_device *, unsigned int, unsigned long);
};
@@ -120,20 +116,6 @@ 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
@@ -176,34 +158,21 @@ they are supported. These optional routines/operations are:
* get_timeleft: this routines returns the time that's left before a reset.
* restart: this routine restarts the machine. It returns 0 on success or a
negative errno code for failure.
-* 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
call are: watchdog_device, cmd and arg.
+The 'ref' and 'unref' operations are no longer used and deprecated.
+
The status bits should (preferably) be set with the set_bit and clear_bit alike
bit-operations. The status bits that are defined are:
* WDOG_ACTIVE: this status bit indicates whether or not a watchdog timer device
is active or not. When the watchdog is active after booting, then you should
set this status bit (Note: when you register the watchdog timer device with
this bit set, then opening /dev/watchdog will skip the start operation)
-* 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_ALLOW_RELEASE: this bit stores whether or not the magic close character
- has been sent (so that we can support the magic close feature).
- (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_core.c b/drivers/watchdog/watchdog_core.c
index f0293f7d2b80..ec1ab6c1a80b 100644
--- a/drivers/watchdog/watchdog_core.c
+++ b/drivers/watchdog/watchdog_core.c
@@ -210,8 +210,6 @@ static int __watchdog_register_device(struct watchdog_device *wdd)
* corrupted in a later stage then we expect a kernel panic!
*/
- mutex_init(&wdd->lock);
-
/* Use alias for watchdog id if possible */
if (wdd->parent) {
ret = of_alias_get_id(wdd->parent->of_node, "watchdog");
diff --git a/drivers/watchdog/watchdog_core.h b/drivers/watchdog/watchdog_core.h
index 86ff962d1e15..c9b0656284de 100644
--- a/drivers/watchdog/watchdog_core.h
+++ b/drivers/watchdog/watchdog_core.h
@@ -26,9 +26,32 @@
* This material is provided "AS-IS" and at no charge.
*/
+#include <linux/cdev.h>
+#include <linux/kref.h>
+#include <linux/mutex.h>
+#include <linux/watchdog.h>
+
#define MAX_DOGS 32 /* Maximum number of watchdog devices */
/*
+ * struct _watchdog_device - watchdog core internal data
+ * @kref: Reference count.
+ * @cdev: The watchdog's Character device.
+ * @wdd: Pointer to watchdog device.
+ * @lock: Lock for watchdog core.
+ * @status: Watchdog core internal status bits.
+ */
+struct _watchdog_device {
+ struct kref kref;
+ struct cdev cdev;
+ struct watchdog_device *wdd;
+ struct mutex lock;
+ unsigned long status; /* Internal status bits */
+#define _WDOG_DEV_OPEN 0 /* Opened ? */
+#define _WDOG_ALLOW_RELEASE 1 /* Did we receive the magic char ? */
+};
+
+/*
* Functions/procedures to be called by the core
*/
extern int watchdog_dev_register(struct watchdog_device *);
diff --git a/drivers/watchdog/watchdog_dev.c b/drivers/watchdog/watchdog_dev.c
index c24392623e98..e8416bdc7037 100644
--- a/drivers/watchdog/watchdog_dev.c
+++ b/drivers/watchdog/watchdog_dev.c
@@ -37,6 +37,7 @@
#include <linux/errno.h> /* For the -ENODEV/... values */
#include <linux/kernel.h> /* For printk/panic/... */
#include <linux/fs.h> /* For file operations */
+#include <linux/slab.h> /* For memory functions */
#include <linux/watchdog.h> /* For watchdog specific items */
#include <linux/miscdevice.h> /* For handling misc devices */
#include <linux/init.h> /* For __init/__exit/... */
@@ -47,12 +48,14 @@
/* 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 *old_wdd;
+static struct _watchdog_device *_old_wdd;
/*
* watchdog_ping: ping the watchdog.
* @wdd: the watchdog device to ping
*
+ * The caller must hold _wdd->lock.
+ *
* If the watchdog has no own ping operation then it needs to be
* restarted via the start operation. This wrapper function does
* exactly that.
@@ -61,25 +64,37 @@ static struct watchdog_device *old_wdd;
static int watchdog_ping(struct watchdog_device *wdd)
{
- int err = 0;
-
- mutex_lock(&wdd->lock);
-
- if (test_bit(WDOG_UNREGISTERED, &wdd->status)) {
- err = -ENODEV;
- goto out_ping;
- }
+ int err;
if (!watchdog_active(wdd))
- goto out_ping;
+ return 0;
if (wdd->ops->ping)
err = wdd->ops->ping(wdd); /* ping the watchdog */
else
err = wdd->ops->start(wdd); /* restart watchdog */
-out_ping:
- mutex_unlock(&wdd->lock);
+ return err;
+}
+
+/*
+ * _watchdog_ping: ping the watchdog.
+ * @_wdd: Watchdog core device data
+ *
+ * Acquire _wdd->lock and call watchdog_ping() unless the watchdog
+ * driver has been unregistered.
+ */
+static int _watchdog_ping(struct _watchdog_device *_wdd)
+{
+ struct watchdog_device *wdd;
+ int err = -ENODEV;
+
+ mutex_lock(&_wdd->lock);
+ wdd = _wdd->wdd;
+ if (wdd)
+ err = watchdog_ping(wdd);
+ mutex_unlock(&_wdd->lock);
+
return err;
}
@@ -87,6 +102,8 @@ out_ping:
* watchdog_start: wrapper to start the watchdog.
* @wdd: the watchdog device to start
*
+ * The caller must hold _wdd->lock.
+ *
* Start the watchdog if it is not active and mark it active.
* This function returns zero on success or a negative errno code for
* failure.
@@ -94,24 +111,15 @@ out_ping:
static int watchdog_start(struct watchdog_device *wdd)
{
- int err = 0;
-
- mutex_lock(&wdd->lock);
-
- if (test_bit(WDOG_UNREGISTERED, &wdd->status)) {
- err = -ENODEV;
- goto out_start;
- }
+ int err;
if (watchdog_active(wdd))
- goto out_start;
+ return 0;
err = wdd->ops->start(wdd);
if (err == 0)
set_bit(WDOG_ACTIVE, &wdd->status);
-out_start:
- mutex_unlock(&wdd->lock);
return err;
}
@@ -119,6 +127,8 @@ out_start:
* watchdog_stop: wrapper to stop the watchdog.
* @wdd: the watchdog device to stop
*
+ * The caller must hold _wdd->lock.
+ *
* Stop the watchdog if it is still active and unmark it active.
* This function returns zero on success or a negative errno code for
* failure.
@@ -127,93 +137,58 @@ out_start:
static int watchdog_stop(struct watchdog_device *wdd)
{
- int err = 0;
-
- mutex_lock(&wdd->lock);
-
- if (test_bit(WDOG_UNREGISTERED, &wdd->status)) {
- err = -ENODEV;
- goto out_stop;
- }
+ int err;
if (!watchdog_active(wdd))
- goto out_stop;
+ return 0;
if (test_bit(WDOG_NO_WAY_OUT, &wdd->status)) {
dev_info(wdd->dev, "nowayout prevents watchdog being stopped!\n");
- err = -EBUSY;
- goto out_stop;
+ return -EBUSY;
}
err = wdd->ops->stop(wdd);
if (err == 0)
clear_bit(WDOG_ACTIVE, &wdd->status);
-out_stop:
- mutex_unlock(&wdd->lock);
return err;
}
/*
* watchdog_get_status: wrapper to get the watchdog status
* @wdd: the watchdog device to get the status from
- * @status: the status of the watchdog device
+ *
+ * The caller must hold _wdd->lock.
*
* Get the watchdog's status flags.
*/
-static int watchdog_get_status(struct watchdog_device *wdd,
- unsigned int *status)
+static unsigned int watchdog_get_status(struct watchdog_device *wdd)
{
- int err = 0;
-
- *status = 0;
if (!wdd->ops->status)
- return -EOPNOTSUPP;
-
- mutex_lock(&wdd->lock);
-
- if (test_bit(WDOG_UNREGISTERED, &wdd->status)) {
- err = -ENODEV;
- goto out_status;
- }
-
- *status = wdd->ops->status(wdd);
+ return 0;
-out_status:
- mutex_unlock(&wdd->lock);
- return err;
+ return wdd->ops->status(wdd);
}
/*
* watchdog_set_timeout: set the watchdog timer timeout
* @wdd: the watchdog device to set the timeout for
* @timeout: timeout to set in seconds
+ *
+ * The caller must hold _wdd->lock.
*/
static int watchdog_set_timeout(struct watchdog_device *wdd,
unsigned int timeout)
{
- int err;
-
if (!wdd->ops->set_timeout || !(wdd->info->options & WDIOF_SETTIMEOUT))
return -EOPNOTSUPP;
if (watchdog_timeout_invalid(wdd, timeout))
return -EINVAL;
- mutex_lock(&wdd->lock);
-
- if (test_bit(WDOG_UNREGISTERED, &wdd->status)) {
- err = -ENODEV;
- goto out_timeout;
- }
-
- err = wdd->ops->set_timeout(wdd, timeout);
-
-out_timeout:
- mutex_unlock(&wdd->lock);
- return err;
+ return wdd->ops->set_timeout(wdd, timeout);
}
/*
@@ -221,30 +196,22 @@ out_timeout:
* @wdd: the watchdog device to get the remaining time from
* @timeleft: the time that's left
*
+ * The caller must hold _wdd->lock.
+ *
* Get the time before a watchdog will reboot (if not pinged).
*/
static int watchdog_get_timeleft(struct watchdog_device *wdd,
unsigned int *timeleft)
{
- int err = 0;
-
*timeleft = 0;
+
if (!wdd->ops->get_timeleft)
return -EOPNOTSUPP;
- mutex_lock(&wdd->lock);
-
- if (test_bit(WDOG_UNREGISTERED, &wdd->status)) {
- err = -ENODEV;
- goto out_timeleft;
- }
-
*timeleft = wdd->ops->get_timeleft(wdd);
-out_timeleft:
- mutex_unlock(&wdd->lock);
- return err;
+ return 0;
}
#ifdef CONFIG_WATCHDOG_SYSFS
@@ -261,14 +228,14 @@ static ssize_t status_show(struct device *dev, struct device_attribute *attr,
char *buf)
{
struct watchdog_device *wdd = dev_get_drvdata(dev);
- ssize_t status;
- unsigned int val;
+ struct _watchdog_device *_wdd = wdd->wdd_data;
+ unsigned int status;
- status = watchdog_get_status(wdd, &val);
- if (!status)
- status = sprintf(buf, "%u\n", val);
+ mutex_lock(&_wdd->lock);
+ status = watchdog_get_status(wdd);
+ mutex_unlock(&_wdd->lock);
- return status;
+ return sprintf(buf, "%u\n", status);
}
static DEVICE_ATTR_RO(status);
@@ -285,10 +252,13 @@ static ssize_t timeleft_show(struct device *dev, struct device_attribute *attr,
char *buf)
{
struct watchdog_device *wdd = dev_get_drvdata(dev);
+ struct _watchdog_device *_wdd = wdd->wdd_data;
ssize_t status;
unsigned int val;
+ mutex_lock(&_wdd->lock);
status = watchdog_get_timeleft(wdd, &val);
+ mutex_unlock(&_wdd->lock);
if (!status)
status = sprintf(buf, "%u\n", val);
@@ -363,28 +333,17 @@ __ATTRIBUTE_GROUPS(wdt);
* @wdd: the watchdog device to do the ioctl on
* @cmd: watchdog command
* @arg: argument pointer
+ *
+ * The caller must hold _wdd->lock.
*/
static int watchdog_ioctl_op(struct watchdog_device *wdd, unsigned int cmd,
unsigned long arg)
{
- int err;
-
if (!wdd->ops->ioctl)
return -ENOIOCTLCMD;
- mutex_lock(&wdd->lock);
-
- if (test_bit(WDOG_UNREGISTERED, &wdd->status)) {
- err = -ENODEV;
- goto out_ioctl;
- }
-
- err = wdd->ops->ioctl(wdd, cmd, arg);
-
-out_ioctl:
- mutex_unlock(&wdd->lock);
- return err;
+ return wdd->ops->ioctl(wdd, cmd, arg);
}
/*
@@ -402,7 +361,7 @@ out_ioctl:
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;
+ struct _watchdog_device *_wdd = file->private_data;
size_t i;
char c;
int err;
@@ -414,18 +373,18 @@ static ssize_t watchdog_write(struct file *file, const char __user *data,
* Note: just in case someone wrote the magic character
* five months ago...
*/
- clear_bit(WDOG_ALLOW_RELEASE, &wdd->status);
+ clear_bit(_WDOG_ALLOW_RELEASE, &_wdd->status);
/* scan to see whether or not we got the magic character */
for (i = 0; i != len; i++) {
if (get_user(c, data + i))
return -EFAULT;
if (c == 'V')
- set_bit(WDOG_ALLOW_RELEASE, &wdd->status);
+ set_bit(_WDOG_ALLOW_RELEASE, &_wdd->status);
}
/* someone wrote to us, so we send the watchdog a keepalive ping */
- err = watchdog_ping(wdd);
+ err = _watchdog_ping(_wdd);
if (err < 0)
return err;
@@ -445,71 +404,94 @@ 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;
+ struct _watchdog_device *_wdd = file->private_data;
void __user *argp = (void __user *)arg;
+ struct watchdog_device *wdd;
int __user *p = argp;
unsigned int val;
- int err;
+ int err = 0;
+
+ mutex_lock(&_wdd->lock);
+
+ wdd = _wdd->wdd;
+ if (!wdd) {
+ err = -ENODEV;
+ goto out_ioctl;
+ }
err = watchdog_ioctl_op(wdd, cmd, arg);
if (err != -ENOIOCTLCMD)
- return err;
+ goto out_ioctl;
switch (cmd) {
case WDIOC_GETSUPPORT:
- return copy_to_user(argp, wdd->info,
+ err = copy_to_user(argp, wdd->info,
sizeof(struct watchdog_info)) ? -EFAULT : 0;
+ break;
case WDIOC_GETSTATUS:
- err = watchdog_get_status(wdd, &val);
- if (err == -ENODEV)
- return err;
- return put_user(val, p);
+ val = watchdog_get_status(wdd);
+ err = put_user(val, p);
+ break;
case WDIOC_GETBOOTSTATUS:
- return put_user(wdd->bootstatus, p);
+ err = put_user(wdd->bootstatus, p);
+ break;
case WDIOC_SETOPTIONS:
- if (get_user(val, p))
- return -EFAULT;
+ if (get_user(val, p)) {
+ err = -EFAULT;
+ break;
+ }
if (val & WDIOS_DISABLECARD) {
err = watchdog_stop(wdd);
if (err < 0)
- return err;
+ break;
}
- if (val & WDIOS_ENABLECARD) {
+ if (val & WDIOS_ENABLECARD)
err = watchdog_start(wdd);
- if (err < 0)
- return err;
- }
- return 0;
+ break;
case WDIOC_KEEPALIVE:
- if (!(wdd->info->options & WDIOF_KEEPALIVEPING))
- return -EOPNOTSUPP;
- return watchdog_ping(wdd);
+ if (!(wdd->info->options & WDIOF_KEEPALIVEPING)) {
+ err = -EOPNOTSUPP;
+ break;
+ }
+ err = watchdog_ping(wdd);
+ break;
case WDIOC_SETTIMEOUT:
- if (get_user(val, p))
- return -EFAULT;
+ if (get_user(val, p)) {
+ err = -EFAULT;
+ break;
+ }
err = watchdog_set_timeout(wdd, val);
if (err < 0)
- return err;
+ break;
/* 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) */
err = watchdog_ping(wdd);
if (err < 0)
- return err;
+ break;
/* Fall */
case WDIOC_GETTIMEOUT:
/* timeout == 0 means that we don't know the timeout */
- if (wdd->timeout == 0)
- return -EOPNOTSUPP;
- return put_user(wdd->timeout, p);
+ if (wdd->timeout == 0) {
+ err = -EOPNOTSUPP;
+ break;
+ }
+ err = put_user(wdd->timeout, p);
+ break;
case WDIOC_GETTIMELEFT:
err = watchdog_get_timeleft(wdd, &val);
- if (err)
- return err;
- return put_user(val, p);
+ if (err < 0)
+ break;
+ err = put_user(val, p);
+ break;
default:
- return -ENOTTY;
+ err = -ENOTTY;
+ break;
}
+
+out_ioctl:
+ mutex_unlock(&_wdd->lock);
+ return err;
}
/*
@@ -524,45 +506,68 @@ 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;
struct watchdog_device *wdd;
+ int err;
/* Get the corresponding watchdog device */
if (imajor(inode) == MISC_MAJOR)
- wdd = old_wdd;
+ _wdd = _old_wdd;
else
- wdd = container_of(inode->i_cdev, struct watchdog_device, cdev);
+ _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))
+ if (test_and_set_bit(_WDOG_DEV_OPEN, &_wdd->status))
return -EBUSY;
+ mutex_lock(&_wdd->lock);
+
+ wdd = _wdd->wdd;
+ if (!wdd) {
+ err = -ENODEV;
+ goto out_clear;
+ }
+
/*
* 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;
+ if (!try_module_get(wdd->ops->owner)) {
+ err = -EBUSY;
+ goto out_clear;
+ }
err = watchdog_start(wdd);
if (err < 0)
goto out_mod;
- file->private_data = wdd;
+ file->private_data = _wdd;
- if (wdd->ops->ref)
- wdd->ops->ref(wdd);
+ kref_get(&_wdd->kref);
/* dev/watchdog is a virtual (and thus non-seekable) filesystem */
- return nonseekable_open(inode, file);
+ err = nonseekable_open(inode, file);
+ goto out_unlock;
out_mod:
- module_put(wdd->ops->owner);
-out:
- clear_bit(WDOG_DEV_OPEN, &wdd->status);
+ module_put(_wdd->wdd->ops->owner);
+out_clear:
+ clear_bit(_WDOG_DEV_OPEN, &_wdd->status);
+out_unlock:
+ mutex_unlock(&_wdd->lock);
return err;
}
+static void watchdog_wdd_release(struct kref *kref)
+{
+ struct _watchdog_device *_wdd;
+
+ _wdd = container_of(kref, struct _watchdog_device, kref);
+
+ kfree(_wdd);
+}
+
/*
* watchdog_release: release the watchdog device.
* @inode: inode of device
@@ -575,9 +580,16 @@ out:
static int watchdog_release(struct inode *inode, struct file *file)
{
- struct watchdog_device *wdd = file->private_data;
+ struct _watchdog_device *_wdd = file->private_data;
+ struct watchdog_device *wdd;
int err = -EBUSY;
+ mutex_lock(&_wdd->lock);
+
+ wdd = _wdd->wdd;
+ if (!wdd)
+ goto done;
+
/*
* We only stop the watchdog if we received the magic character
* or if WDIOF_MAGICCLOSE is not set. If nowayout was set then
@@ -585,29 +597,24 @@ static int watchdog_release(struct inode *inode, struct file *file)
*/
if (!test_bit(WDOG_ACTIVE, &wdd->status))
err = 0;
- else if (test_and_clear_bit(WDOG_ALLOW_RELEASE, &wdd->status) ||
+ else if (test_and_clear_bit(_WDOG_ALLOW_RELEASE, &_wdd->status) ||
!(wdd->info->options & WDIOF_MAGICCLOSE))
err = watchdog_stop(wdd);
/* If the watchdog was not stopped, send a keepalive ping */
if (err < 0) {
- mutex_lock(&wdd->lock);
- if (!test_bit(WDOG_UNREGISTERED, &wdd->status))
- dev_crit(wdd->dev, "watchdog did not stop!\n");
- mutex_unlock(&wdd->lock);
+ dev_crit(wdd->dev, "watchdog did not stop!\n");
watchdog_ping(wdd);
}
- /* Allow the owner module to be unloaded again */
- module_put(wdd->ops->owner);
-
/* 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);
+ clear_bit(_WDOG_DEV_OPEN, &_wdd->status);
+done:
+ mutex_unlock(&_wdd->lock);
+ /* Allow the owner module to be unloaded again */
+ module_put(_wdd->cdev.owner);
+ kref_put(&_wdd->kref, watchdog_wdd_release);
return 0;
}
@@ -637,10 +644,20 @@ static struct miscdevice watchdog_miscdev = {
static int watchdog_cdev_register(struct watchdog_device *wdd, dev_t devno)
{
+ struct _watchdog_device *_wdd;
int err;
+ _wdd = kzalloc(sizeof(struct _watchdog_device), GFP_KERNEL);
+ if (!_wdd)
+ return -ENOMEM;
+ kref_init(&_wdd->kref);
+ mutex_init(&_wdd->lock);
+
+ _wdd->wdd = wdd;
+ wdd->wdd_data = _wdd;
+
if (wdd->id == 0) {
- old_wdd = wdd;
+ _old_wdd = _wdd;
watchdog_miscdev.parent = wdd->parent;
err = misc_register(&watchdog_miscdev);
if (err != 0) {
@@ -649,23 +666,25 @@ static int watchdog_cdev_register(struct watchdog_device *wdd, dev_t devno)
if (err == -EBUSY)
pr_err("%s: a legacy watchdog module is probably present.\n",
wdd->info->identity);
- old_wdd = NULL;
+ _old_wdd = NULL;
+ kfree(_wdd);
return err;
}
}
/* Fill in the data structures */
- cdev_init(&wdd->cdev, &watchdog_fops);
- wdd->cdev.owner = wdd->ops->owner;
+ cdev_init(&_wdd->cdev, &watchdog_fops);
+ _wdd->cdev.owner = wdd->ops->owner;
/* Add the device */
- err = cdev_add(&wdd->cdev, devno, 1);
+ err = cdev_add(&_wdd->cdev, devno, 1);
if (err) {
pr_err("watchdog%d unable to add device %d:%d\n",
wdd->id, MAJOR(watchdog_devt), wdd->id);
if (wdd->id == 0) {
misc_deregister(&watchdog_miscdev);
- old_wdd = NULL;
+ _old_wdd = NULL;
+ kref_put(&_wdd->kref, watchdog_wdd_release);
}
}
return err;
@@ -681,15 +700,23 @@ static int watchdog_cdev_register(struct watchdog_device *wdd, dev_t devno)
static void watchdog_cdev_unregister(struct watchdog_device *wdd)
{
- mutex_lock(&wdd->lock);
- set_bit(WDOG_UNREGISTERED, &wdd->status);
- mutex_unlock(&wdd->lock);
+ struct _watchdog_device *_wdd = wdd->wdd_data;
- cdev_del(&wdd->cdev);
+ cdev_del(&_wdd->cdev);
if (wdd->id == 0) {
misc_deregister(&watchdog_miscdev);
- old_wdd = NULL;
+ _old_wdd = NULL;
}
+
+ if (watchdog_active(wdd))
+ pr_crit("watchdog%d: watchdog still running!\n", wdd->id);
+
+ mutex_lock(&_wdd->lock);
+ _wdd->wdd = NULL;
+ wdd->wdd_data = NULL;
+ mutex_unlock(&_wdd->lock);
+
+ kref_put(&_wdd->kref, watchdog_wdd_release);
}
static struct class watchdog_class = {
@@ -742,9 +769,9 @@ int watchdog_dev_register(struct watchdog_device *wdd)
void watchdog_dev_unregister(struct watchdog_device *wdd)
{
- watchdog_cdev_unregister(wdd);
device_destroy(&watchdog_class, wdd->dev->devt);
wdd->dev = NULL;
+ watchdog_cdev_unregister(wdd);
}
/*
diff --git a/include/linux/watchdog.h b/include/linux/watchdog.h
index a88f955fde92..1d3363aeb6e4 100644
--- a/include/linux/watchdog.h
+++ b/include/linux/watchdog.h
@@ -28,8 +28,6 @@ struct watchdog_device;
* @set_timeout:The routine for setting the watchdog devices timeout value (in seconds).
* @get_timeleft:The routine that gets the time left before a reset (in seconds).
* @restart: The routine for restarting the machine.
- * @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
@@ -48,15 +46,14 @@ struct watchdog_ops {
int (*set_timeout)(struct watchdog_device *, unsigned int);
unsigned int (*get_timeleft)(struct watchdog_device *);
int (*restart)(struct watchdog_device *);
- void (*ref)(struct watchdog_device *);
- void (*unref)(struct watchdog_device *);
+ void (*ref)(struct watchdog_device *) __deprecated;
+ void (*unref)(struct watchdog_device *) __deprecated;
long (*ioctl)(struct watchdog_device *, unsigned int, unsigned long);
};
/** 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.
* @dev: The device for our watchdog
* @parent: The parent bus device
* @info: Pointer to a watchdog_info structure.
@@ -67,8 +64,8 @@ struct watchdog_ops {
* @max_timeout:The watchdog devices maximum timeout value (in seconds).
* @reboot_nb: The notifier block to stop watchdog on reboot.
* @restart_nb: The notifier block to register a restart function.
- * @driver-data:Pointer to the drivers private data.
- * @lock: Lock for watchdog core internal use only.
+ * @driver_data:Pointer to the drivers private data.
+ * @wdd_data: Pointer to watchdog core internal data.
* @status: Field that contains the devices internal status bits.
* @deferred: entry in wtd_deferred_reg_list which is used to
* register early initialized watchdogs.
@@ -84,7 +81,6 @@ struct watchdog_ops {
*/
struct watchdog_device {
int id;
- struct cdev cdev;
struct device *dev;
struct device *parent;
const struct watchdog_info *info;
@@ -96,15 +92,12 @@ struct watchdog_device {
struct notifier_block reboot_nb;
struct notifier_block restart_nb;
void *driver_data;
- struct mutex lock;
+ void *wdd_data;
unsigned long status;
/* Bit numbers for status flags */
#define WDOG_ACTIVE 0 /* Is the watchdog running/active */
-#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 */
-#define WDOG_STOP_ON_REBOOT 5 /* Should be stopped on reboot */
+#define WDOG_NO_WAY_OUT 1 /* Is 'nowayout' feature set ? */
+#define WDOG_STOP_ON_REBOOT 2 /* Should be stopped on reboot */
struct list_head deferred;
};
--
2.1.4
Reference counting is now implemented in the watchdog core and no longer
required in watchdog drivers.
Signed-off-by: Guenter Roeck <[email protected]>
---
drivers/watchdog/da9052_wdt.c | 24 ------------------------
1 file changed, 24 deletions(-)
diff --git a/drivers/watchdog/da9052_wdt.c b/drivers/watchdog/da9052_wdt.c
index 67e67977bd29..2fc19a32a320 100644
--- a/drivers/watchdog/da9052_wdt.c
+++ b/drivers/watchdog/da9052_wdt.c
@@ -31,7 +31,6 @@
struct da9052_wdt_data {
struct watchdog_device wdt;
struct da9052 *da9052;
- struct kref kref;
unsigned long jpast;
};
@@ -51,10 +50,6 @@ static const struct {
};
-static void da9052_wdt_release_resources(struct kref *r)
-{
-}
-
static int da9052_wdt_set_timeout(struct watchdog_device *wdt_dev,
unsigned int timeout)
{
@@ -104,20 +99,6 @@ static int da9052_wdt_set_timeout(struct watchdog_device *wdt_dev,
return 0;
}
-static void da9052_wdt_ref(struct watchdog_device *wdt_dev)
-{
- struct da9052_wdt_data *driver_data = watchdog_get_drvdata(wdt_dev);
-
- kref_get(&driver_data->kref);
-}
-
-static void da9052_wdt_unref(struct watchdog_device *wdt_dev)
-{
- struct da9052_wdt_data *driver_data = watchdog_get_drvdata(wdt_dev);
-
- kref_put(&driver_data->kref, da9052_wdt_release_resources);
-}
-
static int da9052_wdt_start(struct watchdog_device *wdt_dev)
{
return da9052_wdt_set_timeout(wdt_dev, wdt_dev->timeout);
@@ -170,8 +151,6 @@ static const struct watchdog_ops da9052_wdt_ops = {
.stop = da9052_wdt_stop,
.ping = da9052_wdt_ping,
.set_timeout = da9052_wdt_set_timeout,
- .ref = da9052_wdt_ref,
- .unref = da9052_wdt_unref,
};
@@ -198,8 +177,6 @@ static int da9052_wdt_probe(struct platform_device *pdev)
da9052_wdt->parent = &pdev->dev;
watchdog_set_drvdata(da9052_wdt, driver_data);
- kref_init(&driver_data->kref);
-
ret = da9052_reg_update(da9052, DA9052_CONTROL_D_REG,
DA9052_CONTROLD_TWDSCALE, 0);
if (ret < 0) {
@@ -225,7 +202,6 @@ static int da9052_wdt_remove(struct platform_device *pdev)
struct da9052_wdt_data *driver_data = platform_get_drvdata(pdev);
watchdog_unregister_device(&driver_data->wdt);
- kref_put(&driver_data->kref, da9052_wdt_release_resources);
return 0;
}
--
2.1.4
Reference counting is now implemented in the watchdog core and no longer
required in watchdog drivers.
Signed-off-by: Guenter Roeck <[email protected]>
---
drivers/watchdog/da9055_wdt.c | 24 ------------------------
1 file changed, 24 deletions(-)
diff --git a/drivers/watchdog/da9055_wdt.c b/drivers/watchdog/da9055_wdt.c
index 04d1430d93d2..8377c43f3f20 100644
--- a/drivers/watchdog/da9055_wdt.c
+++ b/drivers/watchdog/da9055_wdt.c
@@ -35,7 +35,6 @@ MODULE_PARM_DESC(nowayout,
struct da9055_wdt_data {
struct watchdog_device wdt;
struct da9055 *da9055;
- struct kref kref;
};
static const struct {
@@ -99,24 +98,6 @@ static int da9055_wdt_ping(struct watchdog_device *wdt_dev)
DA9055_WATCHDOG_MASK, 1);
}
-static void da9055_wdt_release_resources(struct kref *r)
-{
-}
-
-static void da9055_wdt_ref(struct watchdog_device *wdt_dev)
-{
- struct da9055_wdt_data *driver_data = watchdog_get_drvdata(wdt_dev);
-
- kref_get(&driver_data->kref);
-}
-
-static void da9055_wdt_unref(struct watchdog_device *wdt_dev)
-{
- struct da9055_wdt_data *driver_data = watchdog_get_drvdata(wdt_dev);
-
- kref_put(&driver_data->kref, da9055_wdt_release_resources);
-}
-
static int da9055_wdt_start(struct watchdog_device *wdt_dev)
{
return da9055_wdt_set_timeout(wdt_dev, wdt_dev->timeout);
@@ -138,8 +119,6 @@ static const struct watchdog_ops da9055_wdt_ops = {
.stop = da9055_wdt_stop,
.ping = da9055_wdt_ping,
.set_timeout = da9055_wdt_set_timeout,
- .ref = da9055_wdt_ref,
- .unref = da9055_wdt_unref,
};
static int da9055_wdt_probe(struct platform_device *pdev)
@@ -165,8 +144,6 @@ static int da9055_wdt_probe(struct platform_device *pdev)
watchdog_set_nowayout(da9055_wdt, nowayout);
watchdog_set_drvdata(da9055_wdt, driver_data);
- kref_init(&driver_data->kref);
-
ret = da9055_wdt_stop(da9055_wdt);
if (ret < 0) {
dev_err(&pdev->dev, "Failed to stop watchdog, %d\n", ret);
@@ -189,7 +166,6 @@ static int da9055_wdt_remove(struct platform_device *pdev)
struct da9055_wdt_data *driver_data = platform_get_drvdata(pdev);
watchdog_unregister_device(&driver_data->wdt);
- kref_put(&driver_data->kref, da9055_wdt_release_resources);
return 0;
}
--
2.1.4
Reference counting is now implemented in the watchdog core and no longer
required in watchdog drivers.
Signed-off-by: Guenter Roeck <[email protected]>
---
drivers/hwmon/sch56xx-common.c | 30 ------------------------------
1 file changed, 30 deletions(-)
diff --git a/drivers/hwmon/sch56xx-common.c b/drivers/hwmon/sch56xx-common.c
index 738681983284..88dba68a9abb 100644
--- a/drivers/hwmon/sch56xx-common.c
+++ b/drivers/hwmon/sch56xx-common.c
@@ -30,7 +30,6 @@
#include <linux/watchdog.h>
#include <linux/miscdevice.h>
#include <linux/uaccess.h>
-#include <linux/kref.h>
#include <linux/slab.h>
#include "sch56xx-common.h"
@@ -67,7 +66,6 @@ MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once started (default="
struct sch56xx_watchdog_data {
u16 addr;
struct mutex *io_lock;
- struct kref kref;
struct watchdog_info wdinfo;
struct watchdog_device wddev;
u8 watchdog_preset;
@@ -258,15 +256,6 @@ EXPORT_SYMBOL(sch56xx_read_virtual_reg12);
* Watchdog routines
*/
-/* Release our data struct when we're unregistered *and*
- all references to our watchdog device are released */
-static void watchdog_release_resources(struct kref *r)
-{
- struct sch56xx_watchdog_data *data =
- container_of(r, struct sch56xx_watchdog_data, kref);
- kfree(data);
-}
-
static int watchdog_set_timeout(struct watchdog_device *wddev,
unsigned int timeout)
{
@@ -395,28 +384,12 @@ static int watchdog_stop(struct watchdog_device *wddev)
return 0;
}
-static void watchdog_ref(struct watchdog_device *wddev)
-{
- struct sch56xx_watchdog_data *data = watchdog_get_drvdata(wddev);
-
- kref_get(&data->kref);
-}
-
-static void watchdog_unref(struct watchdog_device *wddev)
-{
- struct sch56xx_watchdog_data *data = watchdog_get_drvdata(wddev);
-
- kref_put(&data->kref, watchdog_release_resources);
-}
-
static const struct watchdog_ops watchdog_ops = {
.owner = THIS_MODULE,
.start = watchdog_start,
.stop = watchdog_stop,
.ping = watchdog_trigger,
.set_timeout = watchdog_set_timeout,
- .ref = watchdog_ref,
- .unref = watchdog_unref,
};
struct sch56xx_watchdog_data *sch56xx_watchdog_register(struct device *parent,
@@ -448,7 +421,6 @@ struct sch56xx_watchdog_data *sch56xx_watchdog_register(struct device *parent,
data->addr = addr;
data->io_lock = io_lock;
- kref_init(&data->kref);
strlcpy(data->wdinfo.identity, "sch56xx watchdog",
sizeof(data->wdinfo.identity));
@@ -494,8 +466,6 @@ EXPORT_SYMBOL(sch56xx_watchdog_register);
void sch56xx_watchdog_unregister(struct sch56xx_watchdog_data *data)
{
watchdog_unregister_device(&data->wddev);
- kref_put(&data->kref, watchdog_release_resources);
- /* Don't touch data after this it may have been free-ed! */
}
EXPORT_SYMBOL(sch56xx_watchdog_unregister);
--
2.1.4
Hi,
On 20-12-15 22:05, Guenter Roeck wrote:
> Reference counting is now implemented in the watchdog core and no longer
> required in watchdog drivers.
>
> Signed-off-by: Guenter Roeck <[email protected]>
> ---
> drivers/hwmon/sch56xx-common.c | 30 ------------------------------
> 1 file changed, 30 deletions(-)
>
> diff --git a/drivers/hwmon/sch56xx-common.c b/drivers/hwmon/sch56xx-common.c
> index 738681983284..88dba68a9abb 100644
> --- a/drivers/hwmon/sch56xx-common.c
> +++ b/drivers/hwmon/sch56xx-common.c
> @@ -30,7 +30,6 @@
> #include <linux/watchdog.h>
> #include <linux/miscdevice.h>
> #include <linux/uaccess.h>
> -#include <linux/kref.h>
> #include <linux/slab.h>
> #include "sch56xx-common.h"
>
> @@ -67,7 +66,6 @@ MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once started (default="
> struct sch56xx_watchdog_data {
> u16 addr;
> struct mutex *io_lock;
> - struct kref kref;
> struct watchdog_info wdinfo;
> struct watchdog_device wddev;
> u8 watchdog_preset;
> @@ -258,15 +256,6 @@ EXPORT_SYMBOL(sch56xx_read_virtual_reg12);
> * Watchdog routines
> */
>
> -/* Release our data struct when we're unregistered *and*
> - all references to our watchdog device are released */
> -static void watchdog_release_resources(struct kref *r)
> -{
> - struct sch56xx_watchdog_data *data =
> - container_of(r, struct sch56xx_watchdog_data, kref);
> - kfree(data);
You're not replacing this kfree() anywhere, so now the data
allocated by sch56xx_watchdog_register() never gets free-ed
after a sch56xx_watchdog_unregister(). If I read this patch
set correctly then after this patch-set it is safe to
immediately free the memory containing the struct watchdog_device
(and also the struct wdinfo ?) after calling
watchdog_unregister_device(), even if userspace still has the
watchdog cdev open, correct ?
In that case (continue reading below) ...
> -}
> -
> static int watchdog_set_timeout(struct watchdog_device *wddev,
> unsigned int timeout)
> {
> @@ -395,28 +384,12 @@ static int watchdog_stop(struct watchdog_device *wddev)
> return 0;
> }
>
> -static void watchdog_ref(struct watchdog_device *wddev)
> -{
> - struct sch56xx_watchdog_data *data = watchdog_get_drvdata(wddev);
> -
> - kref_get(&data->kref);
> -}
> -
> -static void watchdog_unref(struct watchdog_device *wddev)
> -{
> - struct sch56xx_watchdog_data *data = watchdog_get_drvdata(wddev);
> -
> - kref_put(&data->kref, watchdog_release_resources);
> -}
> -
> static const struct watchdog_ops watchdog_ops = {
> .owner = THIS_MODULE,
> .start = watchdog_start,
> .stop = watchdog_stop,
> .ping = watchdog_trigger,
> .set_timeout = watchdog_set_timeout,
> - .ref = watchdog_ref,
> - .unref = watchdog_unref,
> };
>
> struct sch56xx_watchdog_data *sch56xx_watchdog_register(struct device *parent,
> @@ -448,7 +421,6 @@ struct sch56xx_watchdog_data *sch56xx_watchdog_register(struct device *parent,
>
> data->addr = addr;
> data->io_lock = io_lock;
> - kref_init(&data->kref);
>
> strlcpy(data->wdinfo.identity, "sch56xx watchdog",
> sizeof(data->wdinfo.identity));
> @@ -494,8 +466,6 @@ EXPORT_SYMBOL(sch56xx_watchdog_register);
> void sch56xx_watchdog_unregister(struct sch56xx_watchdog_data *data)
> {
> watchdog_unregister_device(&data->wddev);
> - kref_put(&data->kref, watchdog_release_resources);
> - /* Don't touch data after this it may have been free-ed! */
These 2 lines need to be replaced by a "kfree(data);" rather then being
removed.
> }
> EXPORT_SYMBOL(sch56xx_watchdog_unregister);
>
>
Regards,
Hans
p.s.
The patches for da9052_wdt and da9055_wdt are also interesting,
their da905#_wdt_release_resources() functions are nops,
making the entire refcounting exercise a nop.
For da9052_wdt is the case since this commit:
https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/drivers/watchdog/da9052_wdt.c?id=0360dffedd7bad92f174b2ce5e69e960451d2b59
Which fixes the bug it tries to fix in the wrong way, the right
way would have been to change the devm_kzalloc to a regular kzalloc,
since devm_*alloc cannot be used together with refcounting to keep
memory alive after device unregister time.
What this means is that these drivers actually have a bug wrt
freeing in use memory on driver unbind while userspace has
the watchdog cdev open, and this gets fixed by this patch-set,
it would be very good to mention this in the commit messages.
Hi Hans,
On 12/21/2015 02:37 AM, Hans de Goede wrote:
> Hi,
>
> On 20-12-15 22:05, Guenter Roeck wrote:
>> Reference counting is now implemented in the watchdog core and no longer
>> required in watchdog drivers.
>>
>> Signed-off-by: Guenter Roeck <[email protected]>
>> ---
>> drivers/hwmon/sch56xx-common.c | 30 ------------------------------
>> 1 file changed, 30 deletions(-)
>>
>> diff --git a/drivers/hwmon/sch56xx-common.c b/drivers/hwmon/sch56xx-common.c
>> index 738681983284..88dba68a9abb 100644
>> --- a/drivers/hwmon/sch56xx-common.c
>> +++ b/drivers/hwmon/sch56xx-common.c
>> @@ -30,7 +30,6 @@
>> #include <linux/watchdog.h>
>> #include <linux/miscdevice.h>
>> #include <linux/uaccess.h>
>> -#include <linux/kref.h>
>> #include <linux/slab.h>
>> #include "sch56xx-common.h"
>>
>> @@ -67,7 +66,6 @@ MODULE_PARM_DESC(nowayout, "Watchdog cannot be stopped once started (default="
>> struct sch56xx_watchdog_data {
>> u16 addr;
>> struct mutex *io_lock;
>> - struct kref kref;
>> struct watchdog_info wdinfo;
>> struct watchdog_device wddev;
>> u8 watchdog_preset;
>> @@ -258,15 +256,6 @@ EXPORT_SYMBOL(sch56xx_read_virtual_reg12);
>> * Watchdog routines
>> */
>>
>> -/* Release our data struct when we're unregistered *and*
>> - all references to our watchdog device are released */
>> -static void watchdog_release_resources(struct kref *r)
>> -{
>> - struct sch56xx_watchdog_data *data =
>> - container_of(r, struct sch56xx_watchdog_data, kref);
>> - kfree(data);
>
> You're not replacing this kfree() anywhere, so now the data
> allocated by sch56xx_watchdog_register() never gets free-ed
> after a sch56xx_watchdog_unregister(). If I read this patch
> set correctly then after this patch-set it is safe to
> immediately free the memory containing the struct watchdog_device
> (and also the struct wdinfo ?) after calling
> watchdog_unregister_device(), even if userspace still has the
> watchdog cdev open, correct ?
>
Yes, this is correct for both struct watchdog_device and struct wdinfo.
> In that case (continue reading below) ...
>
>> -}
>> -
>> static int watchdog_set_timeout(struct watchdog_device *wddev,
>> unsigned int timeout)
>> {
>> @@ -395,28 +384,12 @@ static int watchdog_stop(struct watchdog_device *wddev)
>> return 0;
>> }
>>
>> -static void watchdog_ref(struct watchdog_device *wddev)
>> -{
>> - struct sch56xx_watchdog_data *data = watchdog_get_drvdata(wddev);
>> -
>> - kref_get(&data->kref);
>> -}
>> -
>> -static void watchdog_unref(struct watchdog_device *wddev)
>> -{
>> - struct sch56xx_watchdog_data *data = watchdog_get_drvdata(wddev);
>> -
>> - kref_put(&data->kref, watchdog_release_resources);
>> -}
>> -
>> static const struct watchdog_ops watchdog_ops = {
>> .owner = THIS_MODULE,
>> .start = watchdog_start,
>> .stop = watchdog_stop,
>> .ping = watchdog_trigger,
>> .set_timeout = watchdog_set_timeout,
>> - .ref = watchdog_ref,
>> - .unref = watchdog_unref,
>> };
>>
>> struct sch56xx_watchdog_data *sch56xx_watchdog_register(struct device *parent,
>> @@ -448,7 +421,6 @@ struct sch56xx_watchdog_data *sch56xx_watchdog_register(struct device *parent,
>>
>> data->addr = addr;
>> data->io_lock = io_lock;
>> - kref_init(&data->kref);
>>
>> strlcpy(data->wdinfo.identity, "sch56xx watchdog",
>> sizeof(data->wdinfo.identity));
>> @@ -494,8 +466,6 @@ EXPORT_SYMBOL(sch56xx_watchdog_register);
>> void sch56xx_watchdog_unregister(struct sch56xx_watchdog_data *data)
>> {
>> watchdog_unregister_device(&data->wddev);
>> - kref_put(&data->kref, watchdog_release_resources);
>> - /* Don't touch data after this it may have been free-ed! */
>
> These 2 lines need to be replaced by a "kfree(data);" rather then being
> removed.
>
Good catch. Fixed.
>> }
>> EXPORT_SYMBOL(sch56xx_watchdog_unregister);
>>
>>
>
> Regards,
>
> Hans
>
>
> p.s.
>
> The patches for da9052_wdt and da9055_wdt are also interesting,
> their da905#_wdt_release_resources() functions are nops,
> making the entire refcounting exercise a nop.
>
> For da9052_wdt is the case since this commit:
>
> https://git.kernel.org/cgit/linux/kernel/git/torvalds/linux.git/commit/drivers/watchdog/da9052_wdt.c?id=0360dffedd7bad92f174b2ce5e69e960451d2b59
And for da9055 with commit ee8c94adff9b ("watchdog: da9055: Fix invalid free
of devm_ allocated data").
>
> Which fixes the bug it tries to fix in the wrong way, the right
> way would have been to change the devm_kzalloc to a regular kzalloc,
> since devm_*alloc cannot be used together with refcounting to keep
> memory alive after device unregister time.
>
> What this means is that these drivers actually have a bug wrt
> freeing in use memory on driver unbind while userspace has
> the watchdog cdev open, and this gets fixed by this patch-set,
> it would be very good to mention this in the commit messages.
Good point. I updated the commit log.
Thanks,
Guenter
On Sun, Dec 20, 2015 at 01:05:00PM -0800, Guenter Roeck wrote:
> All variables required by the watchdog core to manage a watchdog are
> currently stored in struct watchdog_device. The lifetime of those
> variables is determined by the watchdog driver. However, the lifetime
> of variables used by the watchdog core differs from the lifetime of
> struct watchdog_device. To remedy this situation, watchdog drivers
> can implement ref and unref callbacks, to be used by the watchdog
> core to lock struct watchdog_device in memory.
>
> While this solves the immediate problem, it depends on watchdog drivers
> to actually implement the ref/unref callbacks. This is error prone,
> often not implemented in the first place, or not implemented correctly.
>
> To solve the problem without requiring driver support, split the variables
> in struct watchdog_device into two data structures - one for variables
> associated with the watchdog driver, one for variables associated with
> the watchdog core. With this approach, the watchdog core can keep track
> of its variable lifetime and no longer depends on ref/unref callbacks
> in the driver. As a side effect, some of the variables originally in
> struct watchdog_driver are now private to the watchdog core and no longer
> visible in watchdog drivers.
>
> The 'ref' and 'unref' callbacks in struct watchdog_driver are no longer
> used and marked as deprecated.
Two comments below. It's great to see that unbinding a driver no longer
triggers a kernel panic.
>
> Signed-off-by: Guenter Roeck <[email protected]>
> ---
> Documentation/watchdog/watchdog-kernel-api.txt | 45 +--
> drivers/watchdog/watchdog_core.c | 2 -
> drivers/watchdog/watchdog_core.h | 23 ++
> drivers/watchdog/watchdog_dev.c | 377 +++++++++++++------------
> include/linux/watchdog.h | 21 +-
> 5 files changed, 239 insertions(+), 229 deletions(-)
>
> diff --git a/Documentation/watchdog/watchdog-kernel-api.txt b/Documentation/watchdog/watchdog-kernel-api.txt
> index 0a37da76acef..3db5092924e5 100644
> --- a/Documentation/watchdog/watchdog-kernel-api.txt
> +++ b/Documentation/watchdog/watchdog-kernel-api.txt
> @@ -44,7 +44,6 @@ 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;
> @@ -56,7 +55,7 @@ struct watchdog_device {
> struct notifier_block reboot_nb;
> struct notifier_block restart_nb;
> void *driver_data;
> - struct mutex lock;
> + void *wdd_data;
> unsigned long status;
> struct list_head deferred;
> };
> @@ -66,8 +65,6 @@ It contains following fields:
> /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<id> 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.
> @@ -89,11 +86,10 @@ 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.
> +* wdd_data: a pointer to watchdog core internal data.
> * 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
> - the /dev/watchdog interface or not, ...).
> + running/active, or is the nowayout bit set).
> * deferred: entry in wtd_deferred_reg_list which is used to
> register early initialized watchdogs.
>
> @@ -110,8 +106,8 @@ struct watchdog_ops {
> int (*set_timeout)(struct watchdog_device *, unsigned int);
> unsigned int (*get_timeleft)(struct watchdog_device *);
> int (*restart)(struct watchdog_device *);
> - void (*ref)(struct watchdog_device *);
> - void (*unref)(struct watchdog_device *);
> + void (*ref)(struct watchdog_device *) __deprecated;
> + void (*unref)(struct watchdog_device *) __deprecated;
> long (*ioctl)(struct watchdog_device *, unsigned int, unsigned long);
> };
>
> @@ -120,20 +116,6 @@ 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
> @@ -176,34 +158,21 @@ they are supported. These optional routines/operations are:
> * get_timeleft: this routines returns the time that's left before a reset.
> * restart: this routine restarts the machine. It returns 0 on success or a
> negative errno code for failure.
> -* 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
> call are: watchdog_device, cmd and arg.
>
> +The 'ref' and 'unref' operations are no longer used and deprecated.
> +
> The status bits should (preferably) be set with the set_bit and clear_bit alike
> bit-operations. The status bits that are defined are:
> * WDOG_ACTIVE: this status bit indicates whether or not a watchdog timer device
> is active or not. When the watchdog is active after booting, then you should
> set this status bit (Note: when you register the watchdog timer device with
> this bit set, then opening /dev/watchdog will skip the start operation)
> -* 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_ALLOW_RELEASE: this bit stores whether or not the magic close character
> - has been sent (so that we can support the magic close feature).
> - (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_core.c b/drivers/watchdog/watchdog_core.c
> index f0293f7d2b80..ec1ab6c1a80b 100644
> --- a/drivers/watchdog/watchdog_core.c
> +++ b/drivers/watchdog/watchdog_core.c
> @@ -210,8 +210,6 @@ static int __watchdog_register_device(struct watchdog_device *wdd)
> * corrupted in a later stage then we expect a kernel panic!
> */
>
> - mutex_init(&wdd->lock);
> -
> /* Use alias for watchdog id if possible */
> if (wdd->parent) {
> ret = of_alias_get_id(wdd->parent->of_node, "watchdog");
> diff --git a/drivers/watchdog/watchdog_core.h b/drivers/watchdog/watchdog_core.h
> index 86ff962d1e15..c9b0656284de 100644
> --- a/drivers/watchdog/watchdog_core.h
> +++ b/drivers/watchdog/watchdog_core.h
> @@ -26,9 +26,32 @@
> * This material is provided "AS-IS" and at no charge.
> */
>
> +#include <linux/cdev.h>
> +#include <linux/kref.h>
> +#include <linux/mutex.h>
> +#include <linux/watchdog.h>
> +
> #define MAX_DOGS 32 /* Maximum number of watchdog devices */
>
> /*
> + * struct _watchdog_device - watchdog core internal data
Think it should be /**. Anyway, I find it confusing to have both
_watchdog_device and watchdog_device, but I can't think of a better
name right now.
> + * @kref: Reference count.
> + * @cdev: The watchdog's Character device.
> + * @wdd: Pointer to watchdog device.
> + * @lock: Lock for watchdog core.
> + * @status: Watchdog core internal status bits.
> + */
> +struct _watchdog_device {
> + struct kref kref;
> + struct cdev cdev;
> + struct watchdog_device *wdd;
> + struct mutex lock;
> + unsigned long status; /* Internal status bits */
> +#define _WDOG_DEV_OPEN 0 /* Opened ? */
> +#define _WDOG_ALLOW_RELEASE 1 /* Did we receive the magic char ? */
> +};
> +
> +/*
> * Functions/procedures to be called by the core
> */
> extern int watchdog_dev_register(struct watchdog_device *);
> diff --git a/drivers/watchdog/watchdog_dev.c b/drivers/watchdog/watchdog_dev.c
> index c24392623e98..e8416bdc7037 100644
> --- a/drivers/watchdog/watchdog_dev.c
> +++ b/drivers/watchdog/watchdog_dev.c
> @@ -37,6 +37,7 @@
> #include <linux/errno.h> /* For the -ENODEV/... values */
> #include <linux/kernel.h> /* For printk/panic/... */
> #include <linux/fs.h> /* For file operations */
> +#include <linux/slab.h> /* For memory functions */
> #include <linux/watchdog.h> /* For watchdog specific items */
> #include <linux/miscdevice.h> /* For handling misc devices */
> #include <linux/init.h> /* For __init/__exit/... */
> @@ -47,12 +48,14 @@
> /* 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 *old_wdd;
> +static struct _watchdog_device *_old_wdd;
>
> /*
> * watchdog_ping: ping the watchdog.
> * @wdd: the watchdog device to ping
> *
> + * The caller must hold _wdd->lock.
> + *
> * If the watchdog has no own ping operation then it needs to be
> * restarted via the start operation. This wrapper function does
> * exactly that.
> @@ -61,25 +64,37 @@ static struct watchdog_device *old_wdd;
>
> static int watchdog_ping(struct watchdog_device *wdd)
> {
> - int err = 0;
> -
> - mutex_lock(&wdd->lock);
> -
> - if (test_bit(WDOG_UNREGISTERED, &wdd->status)) {
> - err = -ENODEV;
> - goto out_ping;
> - }
> + int err;
>
> if (!watchdog_active(wdd))
> - goto out_ping;
> + return 0;
>
> if (wdd->ops->ping)
> err = wdd->ops->ping(wdd); /* ping the watchdog */
> else
> err = wdd->ops->start(wdd); /* restart watchdog */
>
> -out_ping:
> - mutex_unlock(&wdd->lock);
> + return err;
> +}
> +
> +/*
> + * _watchdog_ping: ping the watchdog.
> + * @_wdd: Watchdog core device data
> + *
> + * Acquire _wdd->lock and call watchdog_ping() unless the watchdog
> + * driver has been unregistered.
> + */
> +static int _watchdog_ping(struct _watchdog_device *_wdd)
> +{
> + struct watchdog_device *wdd;
> + int err = -ENODEV;
> +
> + mutex_lock(&_wdd->lock);
> + wdd = _wdd->wdd;
> + if (wdd)
> + err = watchdog_ping(wdd);
> + mutex_unlock(&_wdd->lock);
> +
> return err;
> }
>
> @@ -87,6 +102,8 @@ out_ping:
> * watchdog_start: wrapper to start the watchdog.
> * @wdd: the watchdog device to start
> *
> + * The caller must hold _wdd->lock.
> + *
> * Start the watchdog if it is not active and mark it active.
> * This function returns zero on success or a negative errno code for
> * failure.
> @@ -94,24 +111,15 @@ out_ping:
>
> static int watchdog_start(struct watchdog_device *wdd)
> {
> - int err = 0;
> -
> - mutex_lock(&wdd->lock);
> -
> - if (test_bit(WDOG_UNREGISTERED, &wdd->status)) {
> - err = -ENODEV;
> - goto out_start;
> - }
> + int err;
>
> if (watchdog_active(wdd))
> - goto out_start;
> + return 0;
>
> err = wdd->ops->start(wdd);
> if (err == 0)
> set_bit(WDOG_ACTIVE, &wdd->status);
>
> -out_start:
> - mutex_unlock(&wdd->lock);
> return err;
> }
>
> @@ -119,6 +127,8 @@ out_start:
> * watchdog_stop: wrapper to stop the watchdog.
> * @wdd: the watchdog device to stop
> *
> + * The caller must hold _wdd->lock.
> + *
> * Stop the watchdog if it is still active and unmark it active.
> * This function returns zero on success or a negative errno code for
> * failure.
> @@ -127,93 +137,58 @@ out_start:
>
> static int watchdog_stop(struct watchdog_device *wdd)
> {
> - int err = 0;
> -
> - mutex_lock(&wdd->lock);
> -
> - if (test_bit(WDOG_UNREGISTERED, &wdd->status)) {
> - err = -ENODEV;
> - goto out_stop;
> - }
> + int err;
>
> if (!watchdog_active(wdd))
> - goto out_stop;
> + return 0;
>
> if (test_bit(WDOG_NO_WAY_OUT, &wdd->status)) {
> dev_info(wdd->dev, "nowayout prevents watchdog being stopped!\n");
> - err = -EBUSY;
> - goto out_stop;
> + return -EBUSY;
> }
>
> err = wdd->ops->stop(wdd);
> if (err == 0)
> clear_bit(WDOG_ACTIVE, &wdd->status);
>
> -out_stop:
> - mutex_unlock(&wdd->lock);
> return err;
> }
>
> /*
> * watchdog_get_status: wrapper to get the watchdog status
> * @wdd: the watchdog device to get the status from
> - * @status: the status of the watchdog device
> + *
> + * The caller must hold _wdd->lock.
> *
> * Get the watchdog's status flags.
> */
>
> -static int watchdog_get_status(struct watchdog_device *wdd,
> - unsigned int *status)
> +static unsigned int watchdog_get_status(struct watchdog_device *wdd)
> {
> - int err = 0;
> -
> - *status = 0;
> if (!wdd->ops->status)
> - return -EOPNOTSUPP;
> -
> - mutex_lock(&wdd->lock);
> -
> - if (test_bit(WDOG_UNREGISTERED, &wdd->status)) {
> - err = -ENODEV;
> - goto out_status;
> - }
> -
> - *status = wdd->ops->status(wdd);
> + return 0;
>
> -out_status:
> - mutex_unlock(&wdd->lock);
> - return err;
> + return wdd->ops->status(wdd);
> }
>
> /*
> * watchdog_set_timeout: set the watchdog timer timeout
> * @wdd: the watchdog device to set the timeout for
> * @timeout: timeout to set in seconds
> + *
> + * The caller must hold _wdd->lock.
> */
>
> static int watchdog_set_timeout(struct watchdog_device *wdd,
> unsigned int timeout)
> {
> - int err;
> -
> if (!wdd->ops->set_timeout || !(wdd->info->options & WDIOF_SETTIMEOUT))
> return -EOPNOTSUPP;
>
> if (watchdog_timeout_invalid(wdd, timeout))
> return -EINVAL;
>
> - mutex_lock(&wdd->lock);
> -
> - if (test_bit(WDOG_UNREGISTERED, &wdd->status)) {
> - err = -ENODEV;
> - goto out_timeout;
> - }
> -
> - err = wdd->ops->set_timeout(wdd, timeout);
> -
> -out_timeout:
> - mutex_unlock(&wdd->lock);
> - return err;
> + return wdd->ops->set_timeout(wdd, timeout);
> }
>
> /*
> @@ -221,30 +196,22 @@ out_timeout:
> * @wdd: the watchdog device to get the remaining time from
> * @timeleft: the time that's left
> *
> + * The caller must hold _wdd->lock.
> + *
> * Get the time before a watchdog will reboot (if not pinged).
> */
>
> static int watchdog_get_timeleft(struct watchdog_device *wdd,
> unsigned int *timeleft)
> {
> - int err = 0;
> -
> *timeleft = 0;
> +
> if (!wdd->ops->get_timeleft)
> return -EOPNOTSUPP;
>
> - mutex_lock(&wdd->lock);
> -
> - if (test_bit(WDOG_UNREGISTERED, &wdd->status)) {
> - err = -ENODEV;
> - goto out_timeleft;
> - }
> -
> *timeleft = wdd->ops->get_timeleft(wdd);
>
> -out_timeleft:
> - mutex_unlock(&wdd->lock);
> - return err;
> + return 0;
> }
>
> #ifdef CONFIG_WATCHDOG_SYSFS
> @@ -261,14 +228,14 @@ static ssize_t status_show(struct device *dev, struct device_attribute *attr,
> char *buf)
> {
> struct watchdog_device *wdd = dev_get_drvdata(dev);
> - ssize_t status;
> - unsigned int val;
> + struct _watchdog_device *_wdd = wdd->wdd_data;
> + unsigned int status;
>
> - status = watchdog_get_status(wdd, &val);
> - if (!status)
> - status = sprintf(buf, "%u\n", val);
> + mutex_lock(&_wdd->lock);
> + status = watchdog_get_status(wdd);
> + mutex_unlock(&_wdd->lock);
>
> - return status;
> + return sprintf(buf, "%u\n", status);
> }
> static DEVICE_ATTR_RO(status);
>
> @@ -285,10 +252,13 @@ static ssize_t timeleft_show(struct device *dev, struct device_attribute *attr,
> char *buf)
> {
> struct watchdog_device *wdd = dev_get_drvdata(dev);
> + struct _watchdog_device *_wdd = wdd->wdd_data;
> ssize_t status;
> unsigned int val;
>
> + mutex_lock(&_wdd->lock);
> status = watchdog_get_timeleft(wdd, &val);
> + mutex_unlock(&_wdd->lock);
> if (!status)
> status = sprintf(buf, "%u\n", val);
>
> @@ -363,28 +333,17 @@ __ATTRIBUTE_GROUPS(wdt);
> * @wdd: the watchdog device to do the ioctl on
> * @cmd: watchdog command
> * @arg: argument pointer
> + *
> + * The caller must hold _wdd->lock.
> */
>
> static int watchdog_ioctl_op(struct watchdog_device *wdd, unsigned int cmd,
> unsigned long arg)
> {
> - int err;
> -
> if (!wdd->ops->ioctl)
> return -ENOIOCTLCMD;
>
> - mutex_lock(&wdd->lock);
> -
> - if (test_bit(WDOG_UNREGISTERED, &wdd->status)) {
> - err = -ENODEV;
> - goto out_ioctl;
> - }
> -
> - err = wdd->ops->ioctl(wdd, cmd, arg);
> -
> -out_ioctl:
> - mutex_unlock(&wdd->lock);
> - return err;
> + return wdd->ops->ioctl(wdd, cmd, arg);
> }
>
> /*
> @@ -402,7 +361,7 @@ out_ioctl:
> 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;
> + struct _watchdog_device *_wdd = file->private_data;
> size_t i;
> char c;
> int err;
> @@ -414,18 +373,18 @@ static ssize_t watchdog_write(struct file *file, const char __user *data,
> * Note: just in case someone wrote the magic character
> * five months ago...
> */
> - clear_bit(WDOG_ALLOW_RELEASE, &wdd->status);
> + clear_bit(_WDOG_ALLOW_RELEASE, &_wdd->status);
>
> /* scan to see whether or not we got the magic character */
> for (i = 0; i != len; i++) {
> if (get_user(c, data + i))
> return -EFAULT;
> if (c == 'V')
> - set_bit(WDOG_ALLOW_RELEASE, &wdd->status);
> + set_bit(_WDOG_ALLOW_RELEASE, &_wdd->status);
> }
>
> /* someone wrote to us, so we send the watchdog a keepalive ping */
> - err = watchdog_ping(wdd);
> + err = _watchdog_ping(_wdd);
> if (err < 0)
> return err;
>
> @@ -445,71 +404,94 @@ 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;
> + struct _watchdog_device *_wdd = file->private_data;
> void __user *argp = (void __user *)arg;
> + struct watchdog_device *wdd;
> int __user *p = argp;
> unsigned int val;
> - int err;
> + int err = 0;
> +
> + mutex_lock(&_wdd->lock);
> +
> + wdd = _wdd->wdd;
> + if (!wdd) {
> + err = -ENODEV;
> + goto out_ioctl;
> + }
>
> err = watchdog_ioctl_op(wdd, cmd, arg);
> if (err != -ENOIOCTLCMD)
> - return err;
> + goto out_ioctl;
>
> switch (cmd) {
> case WDIOC_GETSUPPORT:
> - return copy_to_user(argp, wdd->info,
> + err = copy_to_user(argp, wdd->info,
> sizeof(struct watchdog_info)) ? -EFAULT : 0;
> + break;
> case WDIOC_GETSTATUS:
> - err = watchdog_get_status(wdd, &val);
> - if (err == -ENODEV)
> - return err;
> - return put_user(val, p);
> + val = watchdog_get_status(wdd);
> + err = put_user(val, p);
> + break;
> case WDIOC_GETBOOTSTATUS:
> - return put_user(wdd->bootstatus, p);
> + err = put_user(wdd->bootstatus, p);
> + break;
> case WDIOC_SETOPTIONS:
> - if (get_user(val, p))
> - return -EFAULT;
> + if (get_user(val, p)) {
> + err = -EFAULT;
> + break;
> + }
> if (val & WDIOS_DISABLECARD) {
> err = watchdog_stop(wdd);
> if (err < 0)
> - return err;
> + break;
> }
> - if (val & WDIOS_ENABLECARD) {
> + if (val & WDIOS_ENABLECARD)
> err = watchdog_start(wdd);
> - if (err < 0)
> - return err;
> - }
> - return 0;
> + break;
> case WDIOC_KEEPALIVE:
> - if (!(wdd->info->options & WDIOF_KEEPALIVEPING))
> - return -EOPNOTSUPP;
> - return watchdog_ping(wdd);
> + if (!(wdd->info->options & WDIOF_KEEPALIVEPING)) {
> + err = -EOPNOTSUPP;
> + break;
> + }
> + err = watchdog_ping(wdd);
> + break;
> case WDIOC_SETTIMEOUT:
> - if (get_user(val, p))
> - return -EFAULT;
> + if (get_user(val, p)) {
> + err = -EFAULT;
> + break;
> + }
> err = watchdog_set_timeout(wdd, val);
> if (err < 0)
> - return err;
> + break;
> /* 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) */
> err = watchdog_ping(wdd);
> if (err < 0)
> - return err;
> + break;
> /* Fall */
> case WDIOC_GETTIMEOUT:
> /* timeout == 0 means that we don't know the timeout */
> - if (wdd->timeout == 0)
> - return -EOPNOTSUPP;
> - return put_user(wdd->timeout, p);
> + if (wdd->timeout == 0) {
> + err = -EOPNOTSUPP;
> + break;
> + }
> + err = put_user(wdd->timeout, p);
> + break;
> case WDIOC_GETTIMELEFT:
> err = watchdog_get_timeleft(wdd, &val);
> - if (err)
> - return err;
> - return put_user(val, p);
> + if (err < 0)
> + break;
> + err = put_user(val, p);
> + break;
> default:
> - return -ENOTTY;
> + err = -ENOTTY;
> + break;
> }
> +
> +out_ioctl:
> + mutex_unlock(&_wdd->lock);
> + return err;
> }
>
> /*
> @@ -524,45 +506,68 @@ 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;
> struct watchdog_device *wdd;
> + int err;
>
> /* Get the corresponding watchdog device */
> if (imajor(inode) == MISC_MAJOR)
> - wdd = old_wdd;
> + _wdd = _old_wdd;
> else
> - wdd = container_of(inode->i_cdev, struct watchdog_device, cdev);
> + _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))
> + if (test_and_set_bit(_WDOG_DEV_OPEN, &_wdd->status))
> return -EBUSY;
>
> + mutex_lock(&_wdd->lock);
> +
> + wdd = _wdd->wdd;
> + if (!wdd) {
> + err = -ENODEV;
> + goto out_clear;
> + }
> +
> /*
> * 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;
> + if (!try_module_get(wdd->ops->owner)) {
> + err = -EBUSY;
> + goto out_clear;
> + }
>
> err = watchdog_start(wdd);
> if (err < 0)
> goto out_mod;
>
> - file->private_data = wdd;
> + file->private_data = _wdd;
>
> - if (wdd->ops->ref)
> - wdd->ops->ref(wdd);
> + kref_get(&_wdd->kref);
>
> /* dev/watchdog is a virtual (and thus non-seekable) filesystem */
> - return nonseekable_open(inode, file);
> + err = nonseekable_open(inode, file);
> + goto out_unlock;
>
> out_mod:
> - module_put(wdd->ops->owner);
> -out:
> - clear_bit(WDOG_DEV_OPEN, &wdd->status);
> + module_put(_wdd->wdd->ops->owner);
> +out_clear:
> + clear_bit(_WDOG_DEV_OPEN, &_wdd->status);
> +out_unlock:
> + mutex_unlock(&_wdd->lock);
> return err;
> }
>
> +static void watchdog_wdd_release(struct kref *kref)
> +{
> + struct _watchdog_device *_wdd;
> +
> + _wdd = container_of(kref, struct _watchdog_device, kref);
> +
> + kfree(_wdd);
> +}
> +
> /*
> * watchdog_release: release the watchdog device.
> * @inode: inode of device
> @@ -575,9 +580,16 @@ out:
>
> static int watchdog_release(struct inode *inode, struct file *file)
> {
> - struct watchdog_device *wdd = file->private_data;
> + struct _watchdog_device *_wdd = file->private_data;
> + struct watchdog_device *wdd;
> int err = -EBUSY;
>
> + mutex_lock(&_wdd->lock);
> +
> + wdd = _wdd->wdd;
> + if (!wdd)
> + goto done;
> +
> /*
> * We only stop the watchdog if we received the magic character
> * or if WDIOF_MAGICCLOSE is not set. If nowayout was set then
> @@ -585,29 +597,24 @@ static int watchdog_release(struct inode *inode, struct file *file)
> */
> if (!test_bit(WDOG_ACTIVE, &wdd->status))
> err = 0;
> - else if (test_and_clear_bit(WDOG_ALLOW_RELEASE, &wdd->status) ||
> + else if (test_and_clear_bit(_WDOG_ALLOW_RELEASE, &_wdd->status) ||
> !(wdd->info->options & WDIOF_MAGICCLOSE))
> err = watchdog_stop(wdd);
>
> /* If the watchdog was not stopped, send a keepalive ping */
> if (err < 0) {
> - mutex_lock(&wdd->lock);
> - if (!test_bit(WDOG_UNREGISTERED, &wdd->status))
> - dev_crit(wdd->dev, "watchdog did not stop!\n");
> - mutex_unlock(&wdd->lock);
> + dev_crit(wdd->dev, "watchdog did not stop!\n");
> watchdog_ping(wdd);
> }
>
> - /* Allow the owner module to be unloaded again */
> - module_put(wdd->ops->owner);
> -
> /* 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);
> + clear_bit(_WDOG_DEV_OPEN, &_wdd->status);
>
> +done:
> + mutex_unlock(&_wdd->lock);
> + /* Allow the owner module to be unloaded again */
> + module_put(_wdd->cdev.owner);
> + kref_put(&_wdd->kref, watchdog_wdd_release);
> return 0;
> }
>
> @@ -637,10 +644,20 @@ static struct miscdevice watchdog_miscdev = {
>
> static int watchdog_cdev_register(struct watchdog_device *wdd, dev_t devno)
> {
> + struct _watchdog_device *_wdd;
> int err;
>
> + _wdd = kzalloc(sizeof(struct _watchdog_device), GFP_KERNEL);
> + if (!_wdd)
> + return -ENOMEM;
> + kref_init(&_wdd->kref);
> + mutex_init(&_wdd->lock);
> +
> + _wdd->wdd = wdd;
> + wdd->wdd_data = _wdd;
> +
> if (wdd->id == 0) {
> - old_wdd = wdd;
> + _old_wdd = _wdd;
> watchdog_miscdev.parent = wdd->parent;
> err = misc_register(&watchdog_miscdev);
> if (err != 0) {
> @@ -649,23 +666,25 @@ static int watchdog_cdev_register(struct watchdog_device *wdd, dev_t devno)
> if (err == -EBUSY)
> pr_err("%s: a legacy watchdog module is probably present.\n",
> wdd->info->identity);
> - old_wdd = NULL;
> + _old_wdd = NULL;
> + kfree(_wdd);
> return err;
> }
> }
>
> /* Fill in the data structures */
> - cdev_init(&wdd->cdev, &watchdog_fops);
> - wdd->cdev.owner = wdd->ops->owner;
> + cdev_init(&_wdd->cdev, &watchdog_fops);
> + _wdd->cdev.owner = wdd->ops->owner;
>
> /* Add the device */
> - err = cdev_add(&wdd->cdev, devno, 1);
> + err = cdev_add(&_wdd->cdev, devno, 1);
> if (err) {
> pr_err("watchdog%d unable to add device %d:%d\n",
> wdd->id, MAJOR(watchdog_devt), wdd->id);
> if (wdd->id == 0) {
> misc_deregister(&watchdog_miscdev);
> - old_wdd = NULL;
> + _old_wdd = NULL;
> + kref_put(&_wdd->kref, watchdog_wdd_release);
> }
> }
> return err;
> @@ -681,15 +700,23 @@ static int watchdog_cdev_register(struct watchdog_device *wdd, dev_t devno)
>
> static void watchdog_cdev_unregister(struct watchdog_device *wdd)
> {
> - mutex_lock(&wdd->lock);
> - set_bit(WDOG_UNREGISTERED, &wdd->status);
> - mutex_unlock(&wdd->lock);
> + struct _watchdog_device *_wdd = wdd->wdd_data;
>
> - cdev_del(&wdd->cdev);
> + cdev_del(&_wdd->cdev);
> if (wdd->id == 0) {
> misc_deregister(&watchdog_miscdev);
> - old_wdd = NULL;
> + _old_wdd = NULL;
> }
> +
> + if (watchdog_active(wdd))
> + pr_crit("watchdog%d: watchdog still running!\n", wdd->id);
As it is now safe to unbind and rebind a driver, it means that a
watchdog driver probe function can now be called with a running
watchdog. Some drivers handle this situation, but I think that most of
them expect the watchdog to be off at this point.
> +
> + mutex_lock(&_wdd->lock);
> + _wdd->wdd = NULL;
> + wdd->wdd_data = NULL;
> + mutex_unlock(&_wdd->lock);
> +
> + kref_put(&_wdd->kref, watchdog_wdd_release);
> }
>
> static struct class watchdog_class = {
> @@ -742,9 +769,9 @@ int watchdog_dev_register(struct watchdog_device *wdd)
>
> void watchdog_dev_unregister(struct watchdog_device *wdd)
> {
> - watchdog_cdev_unregister(wdd);
> device_destroy(&watchdog_class, wdd->dev->devt);
> wdd->dev = NULL;
> + watchdog_cdev_unregister(wdd);
> }
>
> /*
> diff --git a/include/linux/watchdog.h b/include/linux/watchdog.h
> index a88f955fde92..1d3363aeb6e4 100644
> --- a/include/linux/watchdog.h
> +++ b/include/linux/watchdog.h
> @@ -28,8 +28,6 @@ struct watchdog_device;
> * @set_timeout:The routine for setting the watchdog devices timeout value (in seconds).
> * @get_timeleft:The routine that gets the time left before a reset (in seconds).
> * @restart: The routine for restarting the machine.
> - * @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
> @@ -48,15 +46,14 @@ struct watchdog_ops {
> int (*set_timeout)(struct watchdog_device *, unsigned int);
> unsigned int (*get_timeleft)(struct watchdog_device *);
> int (*restart)(struct watchdog_device *);
> - void (*ref)(struct watchdog_device *);
> - void (*unref)(struct watchdog_device *);
> + void (*ref)(struct watchdog_device *) __deprecated;
> + void (*unref)(struct watchdog_device *) __deprecated;
> long (*ioctl)(struct watchdog_device *, unsigned int, unsigned long);
> };
>
> /** 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.
> * @dev: The device for our watchdog
> * @parent: The parent bus device
> * @info: Pointer to a watchdog_info structure.
> @@ -67,8 +64,8 @@ struct watchdog_ops {
> * @max_timeout:The watchdog devices maximum timeout value (in seconds).
> * @reboot_nb: The notifier block to stop watchdog on reboot.
> * @restart_nb: The notifier block to register a restart function.
> - * @driver-data:Pointer to the drivers private data.
> - * @lock: Lock for watchdog core internal use only.
> + * @driver_data:Pointer to the drivers private data.
> + * @wdd_data: Pointer to watchdog core internal data.
> * @status: Field that contains the devices internal status bits.
> * @deferred: entry in wtd_deferred_reg_list which is used to
> * register early initialized watchdogs.
> @@ -84,7 +81,6 @@ struct watchdog_ops {
> */
> struct watchdog_device {
> int id;
> - struct cdev cdev;
> struct device *dev;
> struct device *parent;
> const struct watchdog_info *info;
> @@ -96,15 +92,12 @@ struct watchdog_device {
> struct notifier_block reboot_nb;
> struct notifier_block restart_nb;
> void *driver_data;
> - struct mutex lock;
> + void *wdd_data;
> unsigned long status;
> /* Bit numbers for status flags */
> #define WDOG_ACTIVE 0 /* Is the watchdog running/active */
> -#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 */
> -#define WDOG_STOP_ON_REBOOT 5 /* Should be stopped on reboot */
> +#define WDOG_NO_WAY_OUT 1 /* Is 'nowayout' feature set ? */
> +#define WDOG_STOP_ON_REBOOT 2 /* Should be stopped on reboot */
> struct list_head deferred;
> };
>
> --
> 2.1.4
>
On Sun, Dec 20, 2015 at 01:04:59PM -0800, Guenter Roeck wrote:
> The watchdog character device is currently created in watchdog_dev.c,
> and the watchdog device in watchdog_core.c. This results in
> cross-dependencies, since device creation needs to know the watchdog
> character device number as well as the watchdog class, both of which
> reside in watchdog_dev.c.
>
> Create the watchdog device in watchdog_dev.c to simplify the code.
>
> Inspired by earlier patch set from Damien Riegel.
Hi Guenter,
The main purpose of my patch was to inverse the device creation and the
cdev registration to avoid a racy situation, bu you have dropped that in
this version. Is there a reason for that?
Thanks,
Damien
>
> Cc: Damien Riegel <[email protected]>
> Signed-off-by: Guenter Roeck <[email protected]>
> ---
> drivers/watchdog/watchdog_core.c | 33 ++++--------------
> drivers/watchdog/watchdog_core.h | 4 +--
> drivers/watchdog/watchdog_dev.c | 73 +++++++++++++++++++++++++++++++++-------
> 3 files changed, 69 insertions(+), 41 deletions(-)
>
> diff --git a/drivers/watchdog/watchdog_core.c b/drivers/watchdog/watchdog_core.c
> index 551af042867c..f0293f7d2b80 100644
> --- a/drivers/watchdog/watchdog_core.c
> +++ b/drivers/watchdog/watchdog_core.c
> @@ -42,7 +42,6 @@
> #include "watchdog_core.h" /* For watchdog_dev_register/... */
>
> static DEFINE_IDA(watchdog_ida);
> -static struct class *watchdog_class;
>
> /*
> * Deferred Registration infrastructure.
> @@ -194,7 +193,7 @@ EXPORT_SYMBOL_GPL(watchdog_set_restart_priority);
>
> static int __watchdog_register_device(struct watchdog_device *wdd)
> {
> - int ret, id = -1, devno;
> + int ret, id = -1;
>
> if (wdd == NULL || wdd->info == NULL || wdd->ops == NULL)
> return -EINVAL;
> @@ -247,16 +246,6 @@ static int __watchdog_register_device(struct watchdog_device *wdd)
> }
> }
>
> - devno = wdd->cdev.dev;
> - wdd->dev = device_create(watchdog_class, wdd->parent, devno,
> - wdd, "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;
> - }
> -
> if (test_bit(WDOG_STOP_ON_REBOOT, &wdd->status)) {
> wdd->reboot_nb.notifier_call = watchdog_reboot_notifier;
>
> @@ -265,9 +254,7 @@ static int __watchdog_register_device(struct watchdog_device *wdd)
> dev_err(wdd->dev, "Cannot register reboot notifier (%d)\n",
> ret);
> watchdog_dev_unregister(wdd);
> - device_destroy(watchdog_class, devno);
> ida_simple_remove(&watchdog_ida, wdd->id);
> - wdd->dev = NULL;
> return ret;
> }
> }
> @@ -311,9 +298,6 @@ EXPORT_SYMBOL_GPL(watchdog_register_device);
>
> static void __watchdog_unregister_device(struct watchdog_device *wdd)
> {
> - int ret;
> - int devno;
> -
> if (wdd == NULL)
> return;
>
> @@ -323,13 +307,8 @@ static void __watchdog_unregister_device(struct watchdog_device *wdd)
> if (test_bit(WDOG_STOP_ON_REBOOT, &wdd->status))
> unregister_reboot_notifier(&wdd->reboot_nb);
>
> - devno = wdd->cdev.dev;
> - ret = watchdog_dev_unregister(wdd);
> - if (ret)
> - pr_err("error unregistering /dev/watchdog (err=%d)\n", ret);
> - device_destroy(watchdog_class, devno);
> + watchdog_dev_unregister(wdd);
> ida_simple_remove(&watchdog_ida, wdd->id);
> - wdd->dev = NULL;
> }
>
> /**
> @@ -370,9 +349,11 @@ static int __init watchdog_deferred_registration(void)
>
> static int __init watchdog_init(void)
> {
> - watchdog_class = watchdog_dev_init();
> - if (IS_ERR(watchdog_class))
> - return PTR_ERR(watchdog_class);
> + int err;
> +
> + err = watchdog_dev_init();
> + if (err < 0)
> + return err;
>
> watchdog_deferred_registration();
> return 0;
> diff --git a/drivers/watchdog/watchdog_core.h b/drivers/watchdog/watchdog_core.h
> index 1c8d6b0e68c7..86ff962d1e15 100644
> --- a/drivers/watchdog/watchdog_core.h
> +++ b/drivers/watchdog/watchdog_core.h
> @@ -32,6 +32,6 @@
> * 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 struct class * __init watchdog_dev_init(void);
> +extern void 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 d93118e97d11..c24392623e98 100644
> --- a/drivers/watchdog/watchdog_dev.c
> +++ b/drivers/watchdog/watchdog_dev.c
> @@ -626,17 +626,18 @@ static struct miscdevice watchdog_miscdev = {
> };
>
> /*
> - * watchdog_dev_register: register a watchdog device
> + * watchdog_cdev_register: register watchdog character device
> * @wdd: watchdog device
> + * @devno: character device number
> *
> - * Register a watchdog device including handling the legacy
> + * Register a watchdog character 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 *wdd)
> +static int watchdog_cdev_register(struct watchdog_device *wdd, dev_t devno)
> {
> - int err, devno;
> + int err;
>
> if (wdd->id == 0) {
> old_wdd = wdd;
> @@ -654,7 +655,6 @@ int watchdog_dev_register(struct watchdog_device *wdd)
> }
>
> /* Fill in the data structures */
> - devno = MKDEV(MAJOR(watchdog_devt), wdd->id);
> cdev_init(&wdd->cdev, &watchdog_fops);
> wdd->cdev.owner = wdd->ops->owner;
>
> @@ -672,13 +672,14 @@ int watchdog_dev_register(struct watchdog_device *wdd)
> }
>
> /*
> - * watchdog_dev_unregister: unregister a watchdog device
> + * watchdog_cdev_unregister: unregister watchdog character device
> * @watchdog: watchdog device
> *
> - * Unregister the watchdog and if needed the legacy /dev/watchdog device.
> + * Unregister watchdog character device and if needed the legacy
> + * /dev/watchdog device.
> */
>
> -int watchdog_dev_unregister(struct watchdog_device *wdd)
> +static void watchdog_cdev_unregister(struct watchdog_device *wdd)
> {
> mutex_lock(&wdd->lock);
> set_bit(WDOG_UNREGISTERED, &wdd->status);
> @@ -689,7 +690,6 @@ int watchdog_dev_unregister(struct watchdog_device *wdd)
> misc_deregister(&watchdog_miscdev);
> old_wdd = NULL;
> }
> - return 0;
> }
>
> static struct class watchdog_class = {
> @@ -701,29 +701,76 @@ static struct class watchdog_class = {
> };
>
> /*
> + * watchdog_dev_register: register a watchdog device
> + * @wdd: watchdog device
> + *
> + * Register a watchdog device including handling the legacy
> + * /dev/watchdog node. /dev/watchdog is actually a miscdevice and
> + * thus we set it up like that.
> + */
> +
> +int watchdog_dev_register(struct watchdog_device *wdd)
> +{
> + struct device *dev;
> + dev_t devno;
> + int ret;
> +
> + devno = MKDEV(MAJOR(watchdog_devt), wdd->id);
> +
> + ret = watchdog_cdev_register(wdd, devno);
> + if (ret)
> + return ret;
> +
> + dev = device_create(&watchdog_class, wdd->parent, devno, wdd,
> + "watchdog%d", wdd->id);
> + if (IS_ERR(dev)) {
> + watchdog_cdev_unregister(wdd);
> + return PTR_ERR(dev);
> + }
> + wdd->dev = dev;
> +
> + return ret;
> +}
> +
> +/*
> + * watchdog_dev_unregister: unregister a watchdog device
> + * @watchdog: watchdog device
> + *
> + * Unregister watchdog device and if needed the legacy
> + * /dev/watchdog device.
> + */
> +
> +void watchdog_dev_unregister(struct watchdog_device *wdd)
> +{
> + watchdog_cdev_unregister(wdd);
> + device_destroy(&watchdog_class, wdd->dev->devt);
> + wdd->dev = NULL;
> +}
> +
> +/*
> * watchdog_dev_init: init dev part of watchdog core
> *
> * Allocate a range of chardev nodes to use for watchdog devices
> */
>
> -struct class * __init watchdog_dev_init(void)
> +int __init watchdog_dev_init(void)
> {
> int err;
>
> err = class_register(&watchdog_class);
> if (err < 0) {
> pr_err("couldn't register class\n");
> - return ERR_PTR(err);
> + return err;
> }
>
> err = alloc_chrdev_region(&watchdog_devt, 0, MAX_DOGS, "watchdog");
> if (err < 0) {
> pr_err("watchdog: unable to allocate char dev region\n");
> class_unregister(&watchdog_class);
> - return ERR_PTR(err);
> + return err;
> }
>
> - return &watchdog_class;
> + return 0;
> }
>
> /*
> --
> 2.1.4
>
On 12/21/2015 09:31 AM, Damien Riegel wrote:
> On Sun, Dec 20, 2015 at 01:04:59PM -0800, Guenter Roeck wrote:
>> The watchdog character device is currently created in watchdog_dev.c,
>> and the watchdog device in watchdog_core.c. This results in
>> cross-dependencies, since device creation needs to know the watchdog
>> character device number as well as the watchdog class, both of which
>> reside in watchdog_dev.c.
>>
>> Create the watchdog device in watchdog_dev.c to simplify the code.
>>
>> Inspired by earlier patch set from Damien Riegel.
>
> Hi Guenter,
>
> The main purpose of my patch was to inverse the device creation and the
> cdev registration to avoid a racy situation, bu you have dropped that in
> this version. Is there a reason for that?
>
Every other driver I looked at does it in the same order (cdev first, device
second). I don't really know if doing it differently has any undesired
side effect, so I wanted to play safe.
It would help a lot if someone listening to this exchange can confirm
that it is ok to create the device first, followed by the character device.
Thanks,
Guenter
On Mon, Dec 21, 2015 at 7:28 PM, Damien Riegel
<[email protected]> wrote:
>
> On Sun, Dec 20, 2015 at 01:05:00PM -0800, Guenter Roeck wrote:
> > All variables required by the watchdog core to manage a watchdog are
> > currently stored in struct watchdog_device. The lifetime of those
> > variables is determined by the watchdog driver. However, the lifetime
> > of variables used by the watchdog core differs from the lifetime of
> > struct watchdog_device. To remedy this situation, watchdog drivers
> > can implement ref and unref callbacks, to be used by the watchdog
> > core to lock struct watchdog_device in memory.
> >
> > While this solves the immediate problem, it depends on watchdog drivers
> > to actually implement the ref/unref callbacks. This is error prone,
> > often not implemented in the first place, or not implemented correctly.
> >
> > To solve the problem without requiring driver support, split the variables
> > in struct watchdog_device into two data structures - one for variables
> > associated with the watchdog driver, one for variables associated with
> > the watchdog core. With this approach, the watchdog core can keep track
> > of its variable lifetime and no longer depends on ref/unref callbacks
> > in the driver. As a side effect, some of the variables originally in
> > struct watchdog_driver are now private to the watchdog core and no longer
> > visible in watchdog drivers.
> >
> > The 'ref' and 'unref' callbacks in struct watchdog_driver are no longer
> > used and marked as deprecated.
>
> Two comments below. It's great to see that unbinding a driver no longer
> triggers a kernel panic.
>
> >
> > Signed-off-by: Guenter Roeck <[email protected]>
> > ---
> > Documentation/watchdog/watchdog-kernel-api.txt | 45 +--
> > drivers/watchdog/watchdog_core.c | 2 -
> > drivers/watchdog/watchdog_core.h | 23 ++
> > drivers/watchdog/watchdog_dev.c | 377 +++++++++++++------------
> > include/linux/watchdog.h | 21 +-
> > 5 files changed, 239 insertions(+), 229 deletions(-)
> >
> > diff --git a/Documentation/watchdog/watchdog-kernel-api.txt b/Documentation/watchdog/watchdog-kernel-api.txt
> > index 0a37da76acef..3db5092924e5 100644
> > --- a/Documentation/watchdog/watchdog-kernel-api.txt
> > +++ b/Documentation/watchdog/watchdog-kernel-api.txt
> > @@ -44,7 +44,6 @@ 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;
> > @@ -56,7 +55,7 @@ struct watchdog_device {
> > struct notifier_block reboot_nb;
> > struct notifier_block restart_nb;
> > void *driver_data;
> > - struct mutex lock;
> > + void *wdd_data;
> > unsigned long status;
> > struct list_head deferred;
> > };
> > @@ -66,8 +65,6 @@ It contains following fields:
> > /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<id> 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.
> > @@ -89,11 +86,10 @@ 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.
> > +* wdd_data: a pointer to watchdog core internal data.
> > * 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
> > - the /dev/watchdog interface or not, ...).
> > + running/active, or is the nowayout bit set).
> > * deferred: entry in wtd_deferred_reg_list which is used to
> > register early initialized watchdogs.
> >
> > @@ -110,8 +106,8 @@ struct watchdog_ops {
> > int (*set_timeout)(struct watchdog_device *, unsigned int);
> > unsigned int (*get_timeleft)(struct watchdog_device *);
> > int (*restart)(struct watchdog_device *);
> > - void (*ref)(struct watchdog_device *);
> > - void (*unref)(struct watchdog_device *);
> > + void (*ref)(struct watchdog_device *) __deprecated;
> > + void (*unref)(struct watchdog_device *) __deprecated;
> > long (*ioctl)(struct watchdog_device *, unsigned int, unsigned long);
> > };
> >
> > @@ -120,20 +116,6 @@ 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
> > @@ -176,34 +158,21 @@ they are supported. These optional routines/operations are:
> > * get_timeleft: this routines returns the time that's left before a reset.
> > * restart: this routine restarts the machine. It returns 0 on success or a
> > negative errno code for failure.
> > -* 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
> > call are: watchdog_device, cmd and arg.
> >
> > +The 'ref' and 'unref' operations are no longer used and deprecated.
> > +
> > The status bits should (preferably) be set with the set_bit and clear_bit alike
> > bit-operations. The status bits that are defined are:
> > * WDOG_ACTIVE: this status bit indicates whether or not a watchdog timer device
> > is active or not. When the watchdog is active after booting, then you should
> > set this status bit (Note: when you register the watchdog timer device with
> > this bit set, then opening /dev/watchdog will skip the start operation)
> > -* 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_ALLOW_RELEASE: this bit stores whether or not the magic close character
> > - has been sent (so that we can support the magic close feature).
> > - (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_core.c b/drivers/watchdog/watchdog_core.c
> > index f0293f7d2b80..ec1ab6c1a80b 100644
> > --- a/drivers/watchdog/watchdog_core.c
> > +++ b/drivers/watchdog/watchdog_core.c
> > @@ -210,8 +210,6 @@ static int __watchdog_register_device(struct watchdog_device *wdd)
> > * corrupted in a later stage then we expect a kernel panic!
> > */
> >
> > - mutex_init(&wdd->lock);
> > -
> > /* Use alias for watchdog id if possible */
> > if (wdd->parent) {
> > ret = of_alias_get_id(wdd->parent->of_node, "watchdog");
> > diff --git a/drivers/watchdog/watchdog_core.h b/drivers/watchdog/watchdog_core.h
> > index 86ff962d1e15..c9b0656284de 100644
> > --- a/drivers/watchdog/watchdog_core.h
> > +++ b/drivers/watchdog/watchdog_core.h
> > @@ -26,9 +26,32 @@
> > * This material is provided "AS-IS" and at no charge.
> > */
> >
> > +#include <linux/cdev.h>
> > +#include <linux/kref.h>
> > +#include <linux/mutex.h>
> > +#include <linux/watchdog.h>
> > +
> > #define MAX_DOGS 32 /* Maximum number of watchdog devices */
> >
> > /*
> > + * struct _watchdog_device - watchdog core internal data
>
> Think it should be /**. Anyway, I find it confusing to have both
> _watchdog_device and watchdog_device, but I can't think of a better
> name right now.
>
> > + * @kref: Reference count.
> > + * @cdev: The watchdog's Character device.
> > + * @wdd: Pointer to watchdog device.
> > + * @lock: Lock for watchdog core.
> > + * @status: Watchdog core internal status bits.
> > + */
> > +struct _watchdog_device {
We should probably find a better name for this structure... watchdog
_adapter, _descriptor, or even _data
Also this style is quite confusing when __func() is wrapping func(),
usually this would be otherway around
> > + struct kref kref;
> > + struct cdev cdev;
> > + struct watchdog_device *wdd;
> > + struct mutex lock;
> > + unsigned long status; /* Internal status bits */
> > +#define _WDOG_DEV_OPEN 0 /* Opened ? */
> > +#define _WDOG_ALLOW_RELEASE 1 /* Did we receive the magic char ? */
> > +};
> > +
> > +/*
> > * Functions/procedures to be called by the core
> > */
> > extern int watchdog_dev_register(struct watchdog_device *);
> > diff --git a/drivers/watchdog/watchdog_dev.c b/drivers/watchdog/watchdog_dev.c
> > index c24392623e98..e8416bdc7037 100644
> > --- a/drivers/watchdog/watchdog_dev.c
> > +++ b/drivers/watchdog/watchdog_dev.c
> > @@ -37,6 +37,7 @@
> > #include <linux/errno.h> /* For the -ENODEV/... values */
> > #include <linux/kernel.h> /* For printk/panic/... */
> > #include <linux/fs.h> /* For file operations */
> > +#include <linux/slab.h> /* For memory functions */
> > #include <linux/watchdog.h> /* For watchdog specific items */
> > #include <linux/miscdevice.h> /* For handling misc devices */
> > #include <linux/init.h> /* For __init/__exit/... */
> > @@ -47,12 +48,14 @@
> > /* 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 *old_wdd;
> > +static struct _watchdog_device *_old_wdd;
> >
> > /*
> > * watchdog_ping: ping the watchdog.
> > * @wdd: the watchdog device to ping
> > *
> > + * The caller must hold _wdd->lock.
> > + *
> > * If the watchdog has no own ping operation then it needs to be
> > * restarted via the start operation. This wrapper function does
> > * exactly that.
> > @@ -61,25 +64,37 @@ static struct watchdog_device *old_wdd;
> >
> > static int watchdog_ping(struct watchdog_device *wdd)
> > {
Not sure this lockless wrappers are really needed.
> > - int err = 0;
> > -
> > - mutex_lock(&wdd->lock);
> > -
> > - if (test_bit(WDOG_UNREGISTERED, &wdd->status)) {
> > - err = -ENODEV;
> > - goto out_ping;
> > - }
> > + int err;
> >
> > if (!watchdog_active(wdd))
> > - goto out_ping;
> > + return 0;
> >
> > if (wdd->ops->ping)
> > err = wdd->ops->ping(wdd); /* ping the watchdog */
> > else
> > err = wdd->ops->start(wdd); /* restart watchdog */
> >
> > -out_ping:
> > - mutex_unlock(&wdd->lock);
> > + return err;
> > +}
> > +
> > +/*
> > + * _watchdog_ping: ping the watchdog.
> > + * @_wdd: Watchdog core device data
> > + *
> > + * Acquire _wdd->lock and call watchdog_ping() unless the watchdog
> > + * driver has been unregistered.
> > + */
> > +static int _watchdog_ping(struct _watchdog_device *_wdd)
Use of double underscore __ is more comon .
> > +{
> > + struct watchdog_device *wdd;
> > + int err = -ENODEV;
> > +
> > + mutex_lock(&_wdd->lock);
> > + wdd = _wdd->wdd;
> > + if (wdd)
> > + err = watchdog_ping(wdd);
> > + mutex_unlock(&_wdd->lock);
> > +
> > return err;
> > }
> >
> > @@ -87,6 +102,8 @@ out_ping:
> > * watchdog_start: wrapper to start the watchdog.
> > * @wdd: the watchdog device to start
> > *
> > + * The caller must hold _wdd->lock.
> > + *
> > * Start the watchdog if it is not active and mark it active.
> > * This function returns zero on success or a negative errno code for
> > * failure.
> > @@ -94,24 +111,15 @@ out_ping:
> >
> > static int watchdog_start(struct watchdog_device *wdd)
> > {
> > - int err = 0;
> > -
> > - mutex_lock(&wdd->lock);
> > -
> > - if (test_bit(WDOG_UNREGISTERED, &wdd->status)) {
> > - err = -ENODEV;
> > - goto out_start;
> > - }
> > + int err;
> >
> > if (watchdog_active(wdd))
> > - goto out_start;
> > + return 0;
> >
> > err = wdd->ops->start(wdd);
> > if (err == 0)
> > set_bit(WDOG_ACTIVE, &wdd->status);
> >
> > -out_start:
> > - mutex_unlock(&wdd->lock);
> > return err;
> > }
> >
> > @@ -119,6 +127,8 @@ out_start:
> > * watchdog_stop: wrapper to stop the watchdog.
> > * @wdd: the watchdog device to stop
> > *
> > + * The caller must hold _wdd->lock.
> > + *
> > * Stop the watchdog if it is still active and unmark it active.
> > * This function returns zero on success or a negative errno code for
> > * failure.
> > @@ -127,93 +137,58 @@ out_start:
> >
> > static int watchdog_stop(struct watchdog_device *wdd)
> > {
> > - int err = 0;
> > -
> > - mutex_lock(&wdd->lock);
> > -
> > - if (test_bit(WDOG_UNREGISTERED, &wdd->status)) {
> > - err = -ENODEV;
> > - goto out_stop;
> > - }
> > + int err;
> >
> > if (!watchdog_active(wdd))
> > - goto out_stop;
> > + return 0;
> >
> > if (test_bit(WDOG_NO_WAY_OUT, &wdd->status)) {
> > dev_info(wdd->dev, "nowayout prevents watchdog being stopped!\n");
> > - err = -EBUSY;
> > - goto out_stop;
> > + return -EBUSY;
> > }
> >
> > err = wdd->ops->stop(wdd);
> > if (err == 0)
> > clear_bit(WDOG_ACTIVE, &wdd->status);
> >
> > -out_stop:
> > - mutex_unlock(&wdd->lock);
> > return err;
> > }
> >
> > /*
> > * watchdog_get_status: wrapper to get the watchdog status
> > * @wdd: the watchdog device to get the status from
> > - * @status: the status of the watchdog device
> > + *
> > + * The caller must hold _wdd->lock.
> > *
> > * Get the watchdog's status flags.
> > */
> >
> > -static int watchdog_get_status(struct watchdog_device *wdd,
> > - unsigned int *status)
> > +static unsigned int watchdog_get_status(struct watchdog_device *wdd)
> > {
> > - int err = 0;
> > -
> > - *status = 0;
> > if (!wdd->ops->status)
> > - return -EOPNOTSUPP;
> > -
> > - mutex_lock(&wdd->lock);
> > -
> > - if (test_bit(WDOG_UNREGISTERED, &wdd->status)) {
> > - err = -ENODEV;
> > - goto out_status;
> > - }
> > -
> > - *status = wdd->ops->status(wdd);
> > + return 0;
> >
> > -out_status:
> > - mutex_unlock(&wdd->lock);
> > - return err;
> > + return wdd->ops->status(wdd);
> > }
> >
> > /*
> > * watchdog_set_timeout: set the watchdog timer timeout
> > * @wdd: the watchdog device to set the timeout for
> > * @timeout: timeout to set in seconds
> > + *
> > + * The caller must hold _wdd->lock.
> > */
> >
> > static int watchdog_set_timeout(struct watchdog_device *wdd,
> > unsigned int timeout)
> > {
> > - int err;
> > -
> > if (!wdd->ops->set_timeout || !(wdd->info->options & WDIOF_SETTIMEOUT))
> > return -EOPNOTSUPP;
> >
> > if (watchdog_timeout_invalid(wdd, timeout))
> > return -EINVAL;
> >
> > - mutex_lock(&wdd->lock);
> > -
> > - if (test_bit(WDOG_UNREGISTERED, &wdd->status)) {
> > - err = -ENODEV;
> > - goto out_timeout;
> > - }
> > -
> > - err = wdd->ops->set_timeout(wdd, timeout);
> > -
> > -out_timeout:
> > - mutex_unlock(&wdd->lock);
> > - return err;
> > + return wdd->ops->set_timeout(wdd, timeout);
> > }
> >
> > /*
> > @@ -221,30 +196,22 @@ out_timeout:
> > * @wdd: the watchdog device to get the remaining time from
> > * @timeleft: the time that's left
> > *
> > + * The caller must hold _wdd->lock.
> > + *
> > * Get the time before a watchdog will reboot (if not pinged).
> > */
> >
> > static int watchdog_get_timeleft(struct watchdog_device *wdd,
> > unsigned int *timeleft)
> > {
> > - int err = 0;
> > -
> > *timeleft = 0;
> > +
> > if (!wdd->ops->get_timeleft)
> > return -EOPNOTSUPP;
> >
> > - mutex_lock(&wdd->lock);
> > -
> > - if (test_bit(WDOG_UNREGISTERED, &wdd->status)) {
> > - err = -ENODEV;
> > - goto out_timeleft;
> > - }
> > -
> > *timeleft = wdd->ops->get_timeleft(wdd);
> >
> > -out_timeleft:
> > - mutex_unlock(&wdd->lock);
> > - return err;
> > + return 0;
> > }
> >
> > #ifdef CONFIG_WATCHDOG_SYSFS
> > @@ -261,14 +228,14 @@ static ssize_t status_show(struct device *dev, struct device_attribute *attr,
> > char *buf)
> > {
> > struct watchdog_device *wdd = dev_get_drvdata(dev);
> > - ssize_t status;
> > - unsigned int val;
> > + struct _watchdog_device *_wdd = wdd->wdd_data;
> > + unsigned int status;
> >
> > - status = watchdog_get_status(wdd, &val);
> > - if (!status)
> > - status = sprintf(buf, "%u\n", val);
> > + mutex_lock(&_wdd->lock);
> > + status = watchdog_get_status(wdd);
> > + mutex_unlock(&_wdd->lock);
> >
> > - return status;
> > + return sprintf(buf, "%u\n", status);
> > }
> > static DEVICE_ATTR_RO(status);
> >
> > @@ -285,10 +252,13 @@ static ssize_t timeleft_show(struct device *dev, struct device_attribute *attr,
> > char *buf)
> > {
> > struct watchdog_device *wdd = dev_get_drvdata(dev);
> > + struct _watchdog_device *_wdd = wdd->wdd_data;
> > ssize_t status;
> > unsigned int val;
> >
> > + mutex_lock(&_wdd->lock);
> > status = watchdog_get_timeleft(wdd, &val);
> > + mutex_unlock(&_wdd->lock);
> > if (!status)
> > status = sprintf(buf, "%u\n", val);
> >
> > @@ -363,28 +333,17 @@ __ATTRIBUTE_GROUPS(wdt);
> > * @wdd: the watchdog device to do the ioctl on
> > * @cmd: watchdog command
> > * @arg: argument pointer
> > + *
> > + * The caller must hold _wdd->lock.
> > */
> >
> > static int watchdog_ioctl_op(struct watchdog_device *wdd, unsigned int cmd,
> > unsigned long arg)
> > {
> > - int err;
> > -
> > if (!wdd->ops->ioctl)
> > return -ENOIOCTLCMD;
> >
> > - mutex_lock(&wdd->lock);
> > -
> > - if (test_bit(WDOG_UNREGISTERED, &wdd->status)) {
> > - err = -ENODEV;
> > - goto out_ioctl;
> > - }
> > -
> > - err = wdd->ops->ioctl(wdd, cmd, arg);
> > -
> > -out_ioctl:
> > - mutex_unlock(&wdd->lock);
> > - return err;
> > + return wdd->ops->ioctl(wdd, cmd, arg);
> > }
> >
> > /*
> > @@ -402,7 +361,7 @@ out_ioctl:
> > 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;
> > + struct _watchdog_device *_wdd = file->private_data;
> > size_t i;
> > char c;
> > int err;
> > @@ -414,18 +373,18 @@ static ssize_t watchdog_write(struct file *file, const char __user *data,
> > * Note: just in case someone wrote the magic character
> > * five months ago...
> > */
> > - clear_bit(WDOG_ALLOW_RELEASE, &wdd->status);
> > + clear_bit(_WDOG_ALLOW_RELEASE, &_wdd->status);
> >
> > /* scan to see whether or not we got the magic character */
> > for (i = 0; i != len; i++) {
> > if (get_user(c, data + i))
> > return -EFAULT;
> > if (c == 'V')
> > - set_bit(WDOG_ALLOW_RELEASE, &wdd->status);
> > + set_bit(_WDOG_ALLOW_RELEASE, &_wdd->status);
> > }
> >
> > /* someone wrote to us, so we send the watchdog a keepalive ping */
> > - err = watchdog_ping(wdd);
> > + err = _watchdog_ping(_wdd);
> > if (err < 0)
> > return err;
> >
> > @@ -445,71 +404,94 @@ 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;
> > + struct _watchdog_device *_wdd = file->private_data;
> > void __user *argp = (void __user *)arg;
> > + struct watchdog_device *wdd;
> > int __user *p = argp;
> > unsigned int val;
> > - int err;
> > + int err = 0;
> > +
> > + mutex_lock(&_wdd->lock);
> > +
> > + wdd = _wdd->wdd;
> > + if (!wdd) {
> > + err = -ENODEV;
> > + goto out_ioctl;
> > + }
> >
> > err = watchdog_ioctl_op(wdd, cmd, arg);
> > if (err != -ENOIOCTLCMD)
> > - return err;
> > + goto out_ioctl;
> >
> > switch (cmd) {
> > case WDIOC_GETSUPPORT:
> > - return copy_to_user(argp, wdd->info,
> > + err = copy_to_user(argp, wdd->info,
> > sizeof(struct watchdog_info)) ? -EFAULT : 0;
> > + break;
> > case WDIOC_GETSTATUS:
> > - err = watchdog_get_status(wdd, &val);
> > - if (err == -ENODEV)
> > - return err;
> > - return put_user(val, p);
> > + val = watchdog_get_status(wdd);
> > + err = put_user(val, p);
> > + break;
> > case WDIOC_GETBOOTSTATUS:
> > - return put_user(wdd->bootstatus, p);
> > + err = put_user(wdd->bootstatus, p);
> > + break;
> > case WDIOC_SETOPTIONS:
> > - if (get_user(val, p))
> > - return -EFAULT;
> > + if (get_user(val, p)) {
> > + err = -EFAULT;
> > + break;
> > + }
> > if (val & WDIOS_DISABLECARD) {
> > err = watchdog_stop(wdd);
> > if (err < 0)
> > - return err;
> > + break;
> > }
> > - if (val & WDIOS_ENABLECARD) {
> > + if (val & WDIOS_ENABLECARD)
> > err = watchdog_start(wdd);
> > - if (err < 0)
> > - return err;
> > - }
> > - return 0;
> > + break;
> > case WDIOC_KEEPALIVE:
> > - if (!(wdd->info->options & WDIOF_KEEPALIVEPING))
> > - return -EOPNOTSUPP;
> > - return watchdog_ping(wdd);
> > + if (!(wdd->info->options & WDIOF_KEEPALIVEPING)) {
> > + err = -EOPNOTSUPP;
> > + break;
> > + }
> > + err = watchdog_ping(wdd);
> > + break;
> > case WDIOC_SETTIMEOUT:
> > - if (get_user(val, p))
> > - return -EFAULT;
> > + if (get_user(val, p)) {
> > + err = -EFAULT;
> > + break;
> > + }
> > err = watchdog_set_timeout(wdd, val);
> > if (err < 0)
> > - return err;
> > + break;
> > /* 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) */
> > err = watchdog_ping(wdd);
> > if (err < 0)
> > - return err;
> > + break;
You are changing behaviour for the driver here as you are keeping lock
over two driver op calls.
> > /* Fall */
> > case WDIOC_GETTIMEOUT:
> > /* timeout == 0 means that we don't know the timeout */
> > - if (wdd->timeout == 0)
> > - return -EOPNOTSUPP;
> > - return put_user(wdd->timeout, p);
> > + if (wdd->timeout == 0) {
> > + err = -EOPNOTSUPP;
> > + break;
> > + }
> > + err = put_user(wdd->timeout, p);
> > + break;
> > case WDIOC_GETTIMELEFT:
> > err = watchdog_get_timeleft(wdd, &val);
> > - if (err)
> > - return err;
> > - return put_user(val, p);
> > + if (err < 0)
> > + break;
> > + err = put_user(val, p);
> > + break;
> > default:
> > - return -ENOTTY;
> > + err = -ENOTTY;
> > + break;
> > }
> > +
> > +out_ioctl:
> > + mutex_unlock(&_wdd->lock);
> > + return err;
> > }
> >
> > /*
> > @@ -524,45 +506,68 @@ 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;
> > struct watchdog_device *wdd;
> > + int err;
> >
> > /* Get the corresponding watchdog device */
> > if (imajor(inode) == MISC_MAJOR)
> > - wdd = old_wdd;
> > + _wdd = _old_wdd;
> > else
> > - wdd = container_of(inode->i_cdev, struct watchdog_device, cdev);
> > + _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))
> > + if (test_and_set_bit(_WDOG_DEV_OPEN, &_wdd->status))
> > return -EBUSY;
> >
> > + mutex_lock(&_wdd->lock);
> > +
> > + wdd = _wdd->wdd;
> > + if (!wdd) {
> > + err = -ENODEV;
> > + goto out_clear;
> > + }
> > +
> > /*
> > * 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;
> > + if (!try_module_get(wdd->ops->owner)) {
> > + err = -EBUSY;
> > + goto out_clear;
> > + }
> >
> > err = watchdog_start(wdd);
> > if (err < 0)
> > goto out_mod;
> >
> > - file->private_data = wdd;
> > + file->private_data = _wdd;
> >
> > - if (wdd->ops->ref)
> > - wdd->ops->ref(wdd);
> > + kref_get(&_wdd->kref);
> >
> > /* dev/watchdog is a virtual (and thus non-seekable) filesystem */
> > - return nonseekable_open(inode, file);
> > + err = nonseekable_open(inode, file);
> > + goto out_unlock;
> >
> > out_mod:
> > - module_put(wdd->ops->owner);
> > -out:
> > - clear_bit(WDOG_DEV_OPEN, &wdd->status);
> > + module_put(_wdd->wdd->ops->owner);
> > +out_clear:
> > + clear_bit(_WDOG_DEV_OPEN, &_wdd->status);
> > +out_unlock:
> > + mutex_unlock(&_wdd->lock);
> > return err;
> > }
> >
> > +static void watchdog_wdd_release(struct kref *kref)
> > +{
> > + struct _watchdog_device *_wdd;
> > +
> > + _wdd = container_of(kref, struct _watchdog_device, kref);
> > +
> > + kfree(_wdd);
> > +}
> > +
> > /*
> > * watchdog_release: release the watchdog device.
> > * @inode: inode of device
> > @@ -575,9 +580,16 @@ out:
> >
> > static int watchdog_release(struct inode *inode, struct file *file)
> > {
> > - struct watchdog_device *wdd = file->private_data;
> > + struct _watchdog_device *_wdd = file->private_data;
> > + struct watchdog_device *wdd;
> > int err = -EBUSY;
> >
> > + mutex_lock(&_wdd->lock);
> > +
> > + wdd = _wdd->wdd;
> > + if (!wdd)
> > + goto done;
> > +
> > /*
> > * We only stop the watchdog if we received the magic character
> > * or if WDIOF_MAGICCLOSE is not set. If nowayout was set then
> > @@ -585,29 +597,24 @@ static int watchdog_release(struct inode *inode, struct file *file)
> > */
> > if (!test_bit(WDOG_ACTIVE, &wdd->status))
> > err = 0;
> > - else if (test_and_clear_bit(WDOG_ALLOW_RELEASE, &wdd->status) ||
> > + else if (test_and_clear_bit(_WDOG_ALLOW_RELEASE, &_wdd->status) ||
> > !(wdd->info->options & WDIOF_MAGICCLOSE))
> > err = watchdog_stop(wdd);
> >
> > /* If the watchdog was not stopped, send a keepalive ping */
> > if (err < 0) {
> > - mutex_lock(&wdd->lock);
> > - if (!test_bit(WDOG_UNREGISTERED, &wdd->status))
> > - dev_crit(wdd->dev, "watchdog did not stop!\n");
> > - mutex_unlock(&wdd->lock);
> > + dev_crit(wdd->dev, "watchdog did not stop!\n");
> > watchdog_ping(wdd);
> > }
> >
> > - /* Allow the owner module to be unloaded again */
> > - module_put(wdd->ops->owner);
> > -
> > /* 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);
> > + clear_bit(_WDOG_DEV_OPEN, &_wdd->status);
> >
> > +done:
> > + mutex_unlock(&_wdd->lock);
> > + /* Allow the owner module to be unloaded again */
> > + module_put(_wdd->cdev.owner);
> > + kref_put(&_wdd->kref, watchdog_wdd_release);
> > return 0;
> > }
> >
> > @@ -637,10 +644,20 @@ static struct miscdevice watchdog_miscdev = {
> >
> > static int watchdog_cdev_register(struct watchdog_device *wdd, dev_t devno)
> > {
> > + struct _watchdog_device *_wdd;
> > int err;
> >
> > + _wdd = kzalloc(sizeof(struct _watchdog_device), GFP_KERNEL);
> > + if (!_wdd)
> > + return -ENOMEM;
> > + kref_init(&_wdd->kref);
> > + mutex_init(&_wdd->lock);
> > +
> > + _wdd->wdd = wdd;
> > + wdd->wdd_data = _wdd;
> > +
> > if (wdd->id == 0) {
> > - old_wdd = wdd;
> > + _old_wdd = _wdd;
> > watchdog_miscdev.parent = wdd->parent;
> > err = misc_register(&watchdog_miscdev);
> > if (err != 0) {
> > @@ -649,23 +666,25 @@ static int watchdog_cdev_register(struct watchdog_device *wdd, dev_t devno)
> > if (err == -EBUSY)
> > pr_err("%s: a legacy watchdog module is probably present.\n",
> > wdd->info->identity);
> > - old_wdd = NULL;
> > + _old_wdd = NULL;
> > + kfree(_wdd);
> > return err;
> > }
> > }
> >
> > /* Fill in the data structures */
> > - cdev_init(&wdd->cdev, &watchdog_fops);
> > - wdd->cdev.owner = wdd->ops->owner;
> > + cdev_init(&_wdd->cdev, &watchdog_fops);
> > + _wdd->cdev.owner = wdd->ops->owner;
> >
> > /* Add the device */
> > - err = cdev_add(&wdd->cdev, devno, 1);
> > + err = cdev_add(&_wdd->cdev, devno, 1);
> > if (err) {
> > pr_err("watchdog%d unable to add device %d:%d\n",
> > wdd->id, MAJOR(watchdog_devt), wdd->id);
> > if (wdd->id == 0) {
> > misc_deregister(&watchdog_miscdev);
> > - old_wdd = NULL;
> > + _old_wdd = NULL;
> > + kref_put(&_wdd->kref, watchdog_wdd_release);
> > }
> > }
> > return err;
> > @@ -681,15 +700,23 @@ static int watchdog_cdev_register(struct watchdog_device *wdd, dev_t devno)
> >
> > static void watchdog_cdev_unregister(struct watchdog_device *wdd)
> > {
> > - mutex_lock(&wdd->lock);
> > - set_bit(WDOG_UNREGISTERED, &wdd->status);
> > - mutex_unlock(&wdd->lock);
> > + struct _watchdog_device *_wdd = wdd->wdd_data;
> >
> > - cdev_del(&wdd->cdev);
> > + cdev_del(&_wdd->cdev);
> > if (wdd->id == 0) {
> > misc_deregister(&watchdog_miscdev);
> > - old_wdd = NULL;
> > + _old_wdd = NULL;
> > }
> > +
> > + if (watchdog_active(wdd))
> > + pr_crit("watchdog%d: watchdog still running!\n", wdd->id);
>
> As it is now safe to unbind and rebind a driver, it means that a
> watchdog driver probe function can now be called with a running
> watchdog. Some drivers handle this situation, but I think that most of
> them expect the watchdog to be off at this point.
>
> > +
> > + mutex_lock(&_wdd->lock);
> > + _wdd->wdd = NULL;
> > + wdd->wdd_data = NULL;
> > + mutex_unlock(&_wdd->lock);
> > +
> > + kref_put(&_wdd->kref, watchdog_wdd_release);
> > }
> >
> > static struct class watchdog_class = {
> > @@ -742,9 +769,9 @@ int watchdog_dev_register(struct watchdog_device *wdd)
> >
> > void watchdog_dev_unregister(struct watchdog_device *wdd)
> > {
> > - watchdog_cdev_unregister(wdd);
> > device_destroy(&watchdog_class, wdd->dev->devt);
> > wdd->dev = NULL;
> > + watchdog_cdev_unregister(wdd);
> > }
> >
> > /*
> > diff --git a/include/linux/watchdog.h b/include/linux/watchdog.h
> > index a88f955fde92..1d3363aeb6e4 100644
> > --- a/include/linux/watchdog.h
> > +++ b/include/linux/watchdog.h
> > @@ -28,8 +28,6 @@ struct watchdog_device;
> > * @set_timeout:The routine for setting the watchdog devices timeout value (in seconds).
> > * @get_timeleft:The routine that gets the time left before a reset (in seconds).
> > * @restart: The routine for restarting the machine.
> > - * @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
> > @@ -48,15 +46,14 @@ struct watchdog_ops {
> > int (*set_timeout)(struct watchdog_device *, unsigned int);
> > unsigned int (*get_timeleft)(struct watchdog_device *);
> > int (*restart)(struct watchdog_device *);
> > - void (*ref)(struct watchdog_device *);
> > - void (*unref)(struct watchdog_device *);
> > + void (*ref)(struct watchdog_device *) __deprecated;
> > + void (*unref)(struct watchdog_device *) __deprecated;
> > long (*ioctl)(struct watchdog_device *, unsigned int, unsigned long);
> > };
> >
> > /** 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.
> > * @dev: The device for our watchdog
> > * @parent: The parent bus device
> > * @info: Pointer to a watchdog_info structure.
> > @@ -67,8 +64,8 @@ struct watchdog_ops {
> > * @max_timeout:The watchdog devices maximum timeout value (in seconds).
> > * @reboot_nb: The notifier block to stop watchdog on reboot.
> > * @restart_nb: The notifier block to register a restart function.
> > - * @driver-data:Pointer to the drivers private data.
> > - * @lock: Lock for watchdog core internal use only.
> > + * @driver_data:Pointer to the drivers private data.
> > + * @wdd_data: Pointer to watchdog core internal data.
> > * @status: Field that contains the devices internal status bits.
> > * @deferred: entry in wtd_deferred_reg_list which is used to
> > * register early initialized watchdogs.
> > @@ -84,7 +81,6 @@ struct watchdog_ops {
> > */
> > struct watchdog_device {
> > int id;
> > - struct cdev cdev;
> > struct device *dev;
> > struct device *parent;
> > const struct watchdog_info *info;
> > @@ -96,15 +92,12 @@ struct watchdog_device {
> > struct notifier_block reboot_nb;
> > struct notifier_block restart_nb;
> > void *driver_data;
> > - struct mutex lock;
> > + void *wdd_data;
> > unsigned long status;
> > /* Bit numbers for status flags */
> > #define WDOG_ACTIVE 0 /* Is the watchdog running/active */
> > -#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 */
> > -#define WDOG_STOP_ON_REBOOT 5 /* Should be stopped on reboot */
> > +#define WDOG_NO_WAY_OUT 1 /* Is 'nowayout' feature set ? */
> > +#define WDOG_STOP_ON_REBOOT 2 /* Should be stopped on reboot */
> > struct list_head deferred;
> > };
> >
> > --
> > 2.1.4
> >
> --
> 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
On 12/21/2015 09:28 AM, Damien Riegel wrote:
> On Sun, Dec 20, 2015 at 01:05:00PM -0800, Guenter Roeck wrote:
>> All variables required by the watchdog core to manage a watchdog are
>> currently stored in struct watchdog_device. The lifetime of those
>> variables is determined by the watchdog driver. However, the lifetime
>> of variables used by the watchdog core differs from the lifetime of
>> struct watchdog_device. To remedy this situation, watchdog drivers
>> can implement ref and unref callbacks, to be used by the watchdog
>> core to lock struct watchdog_device in memory.
>>
>> While this solves the immediate problem, it depends on watchdog drivers
>> to actually implement the ref/unref callbacks. This is error prone,
>> often not implemented in the first place, or not implemented correctly.
>>
>> To solve the problem without requiring driver support, split the variables
>> in struct watchdog_device into two data structures - one for variables
>> associated with the watchdog driver, one for variables associated with
>> the watchdog core. With this approach, the watchdog core can keep track
>> of its variable lifetime and no longer depends on ref/unref callbacks
>> in the driver. As a side effect, some of the variables originally in
>> struct watchdog_driver are now private to the watchdog core and no longer
>> visible in watchdog drivers.
>>
>> The 'ref' and 'unref' callbacks in struct watchdog_driver are no longer
>> used and marked as deprecated.
>
> Two comments below. It's great to see that unbinding a driver no longer
> triggers a kernel panic.
>
It should not have caused a panic to start with, but the ref/unref functions
for the most part were either not or wrongly implemented. Not really
surprising - it took me a while to understand the problem.
[ ... ]
>>
>> /*
>> + * struct _watchdog_device - watchdog core internal data
>
> Think it should be /**. Anyway, I find it confusing to have both
> _watchdog_device and watchdog_device, but I can't think of a better
> name right now.
I renamed the data structure to watchdog_data and moved it into watchdog_dev.c
since it is only used there. No '**', though, because it is not a published
API, but just an internal data structure.
I also renamed the matching variable name to 'wd_data' (from '_wdd').
>>
>> static void watchdog_cdev_unregister(struct watchdog_device *wdd)
>> {
>> - mutex_lock(&wdd->lock);
>> - set_bit(WDOG_UNREGISTERED, &wdd->status);
>> - mutex_unlock(&wdd->lock);
>> + struct _watchdog_device *_wdd = wdd->wdd_data;
>>
>> - cdev_del(&wdd->cdev);
>> + cdev_del(&_wdd->cdev);
>> if (wdd->id == 0) {
>> misc_deregister(&watchdog_miscdev);
>> - old_wdd = NULL;
>> + _old_wdd = NULL;
>> }
>> +
>> + if (watchdog_active(wdd))
>> + pr_crit("watchdog%d: watchdog still running!\n", wdd->id);
>
> As it is now safe to unbind and rebind a driver, it means that a
> watchdog driver probe function can now be called with a running
> watchdog. Some drivers handle this situation, but I think that most of
> them expect the watchdog to be off at this point.
>
No semantics change, though, and no change in behavior. Drivers _should_
handle that situation today. Sure, many don't, but that is a different issue.
I'll address handling an already-running watchdog by the watchdog core until
the character device is opened in a separate patch set, but we'll have to have
this series accepted before I re-introduce that. Even with that, it will still
be the driver's responsibility to detect and report that/if a watchdog is
already running.
Thanks,
Guenter
On 12/21/2015 03:36 PM, Tomas Winkler wrote:
> On Mon, Dec 21, 2015 at 7:28 PM, Damien Riegel
> <[email protected]> wrote:
>>
>> On Sun, Dec 20, 2015 at 01:05:00PM -0800, Guenter Roeck wrote:
>>> All variables required by the watchdog core to manage a watchdog are
>>> currently stored in struct watchdog_device. The lifetime of those
>>> variables is determined by the watchdog driver. However, the lifetime
>>> of variables used by the watchdog core differs from the lifetime of
>>> struct watchdog_device. To remedy this situation, watchdog drivers
>>> can implement ref and unref callbacks, to be used by the watchdog
>>> core to lock struct watchdog_device in memory.
>>>
>>> While this solves the immediate problem, it depends on watchdog drivers
>>> to actually implement the ref/unref callbacks. This is error prone,
>>> often not implemented in the first place, or not implemented correctly.
>>>
>>> To solve the problem without requiring driver support, split the variables
>>> in struct watchdog_device into two data structures - one for variables
>>> associated with the watchdog driver, one for variables associated with
>>> the watchdog core. With this approach, the watchdog core can keep track
>>> of its variable lifetime and no longer depends on ref/unref callbacks
>>> in the driver. As a side effect, some of the variables originally in
>>> struct watchdog_driver are now private to the watchdog core and no longer
>>> visible in watchdog drivers.
>>>
>>> The 'ref' and 'unref' callbacks in struct watchdog_driver are no longer
>>> used and marked as deprecated.
>>
>> Two comments below. It's great to see that unbinding a driver no longer
>> triggers a kernel panic.
>>
>>>
>>> Signed-off-by: Guenter Roeck <[email protected]>
>>> ---
>>> Documentation/watchdog/watchdog-kernel-api.txt | 45 +--
>>> drivers/watchdog/watchdog_core.c | 2 -
>>> drivers/watchdog/watchdog_core.h | 23 ++
>>> drivers/watchdog/watchdog_dev.c | 377 +++++++++++++------------
>>> include/linux/watchdog.h | 21 +-
>>> 5 files changed, 239 insertions(+), 229 deletions(-)
>>>
>>> diff --git a/Documentation/watchdog/watchdog-kernel-api.txt b/Documentation/watchdog/watchdog-kernel-api.txt
>>> index 0a37da76acef..3db5092924e5 100644
>>> --- a/Documentation/watchdog/watchdog-kernel-api.txt
>>> +++ b/Documentation/watchdog/watchdog-kernel-api.txt
>>> @@ -44,7 +44,6 @@ 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;
>>> @@ -56,7 +55,7 @@ struct watchdog_device {
>>> struct notifier_block reboot_nb;
>>> struct notifier_block restart_nb;
>>> void *driver_data;
>>> - struct mutex lock;
>>> + void *wdd_data;
>>> unsigned long status;
>>> struct list_head deferred;
>>> };
>>> @@ -66,8 +65,6 @@ It contains following fields:
>>> /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<id> 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.
>>> @@ -89,11 +86,10 @@ 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.
>>> +* wdd_data: a pointer to watchdog core internal data.
>>> * 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
>>> - the /dev/watchdog interface or not, ...).
>>> + running/active, or is the nowayout bit set).
>>> * deferred: entry in wtd_deferred_reg_list which is used to
>>> register early initialized watchdogs.
>>>
>>> @@ -110,8 +106,8 @@ struct watchdog_ops {
>>> int (*set_timeout)(struct watchdog_device *, unsigned int);
>>> unsigned int (*get_timeleft)(struct watchdog_device *);
>>> int (*restart)(struct watchdog_device *);
>>> - void (*ref)(struct watchdog_device *);
>>> - void (*unref)(struct watchdog_device *);
>>> + void (*ref)(struct watchdog_device *) __deprecated;
>>> + void (*unref)(struct watchdog_device *) __deprecated;
>>> long (*ioctl)(struct watchdog_device *, unsigned int, unsigned long);
>>> };
>>>
>>> @@ -120,20 +116,6 @@ 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
>>> @@ -176,34 +158,21 @@ they are supported. These optional routines/operations are:
>>> * get_timeleft: this routines returns the time that's left before a reset.
>>> * restart: this routine restarts the machine. It returns 0 on success or a
>>> negative errno code for failure.
>>> -* 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
>>> call are: watchdog_device, cmd and arg.
>>>
>>> +The 'ref' and 'unref' operations are no longer used and deprecated.
>>> +
>>> The status bits should (preferably) be set with the set_bit and clear_bit alike
>>> bit-operations. The status bits that are defined are:
>>> * WDOG_ACTIVE: this status bit indicates whether or not a watchdog timer device
>>> is active or not. When the watchdog is active after booting, then you should
>>> set this status bit (Note: when you register the watchdog timer device with
>>> this bit set, then opening /dev/watchdog will skip the start operation)
>>> -* 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_ALLOW_RELEASE: this bit stores whether or not the magic close character
>>> - has been sent (so that we can support the magic close feature).
>>> - (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_core.c b/drivers/watchdog/watchdog_core.c
>>> index f0293f7d2b80..ec1ab6c1a80b 100644
>>> --- a/drivers/watchdog/watchdog_core.c
>>> +++ b/drivers/watchdog/watchdog_core.c
>>> @@ -210,8 +210,6 @@ static int __watchdog_register_device(struct watchdog_device *wdd)
>>> * corrupted in a later stage then we expect a kernel panic!
>>> */
>>>
>>> - mutex_init(&wdd->lock);
>>> -
>>> /* Use alias for watchdog id if possible */
>>> if (wdd->parent) {
>>> ret = of_alias_get_id(wdd->parent->of_node, "watchdog");
>>> diff --git a/drivers/watchdog/watchdog_core.h b/drivers/watchdog/watchdog_core.h
>>> index 86ff962d1e15..c9b0656284de 100644
>>> --- a/drivers/watchdog/watchdog_core.h
>>> +++ b/drivers/watchdog/watchdog_core.h
>>> @@ -26,9 +26,32 @@
>>> * This material is provided "AS-IS" and at no charge.
>>> */
>>>
>>> +#include <linux/cdev.h>
>>> +#include <linux/kref.h>
>>> +#include <linux/mutex.h>
>>> +#include <linux/watchdog.h>
>>> +
>>> #define MAX_DOGS 32 /* Maximum number of watchdog devices */
>>>
>>> /*
>>> + * struct _watchdog_device - watchdog core internal data
>>
>> Think it should be /**. Anyway, I find it confusing to have both
>> _watchdog_device and watchdog_device, but I can't think of a better
>> name right now.
>>
>>> + * @kref: Reference count.
>>> + * @cdev: The watchdog's Character device.
>>> + * @wdd: Pointer to watchdog device.
>>> + * @lock: Lock for watchdog core.
>>> + * @status: Watchdog core internal status bits.
>>> + */
>>> +struct _watchdog_device {
> We should probably find a better name for this structure... watchdog
> _adapter, _descriptor, or even _data
> Also this style is quite confusing when __func() is wrapping func(),
> usually this would be otherway around
>
I ended up using watchdog_data. I also moved the data structure into
watchdog_dev.c, as it is only used there.
>>> + struct kref kref;
>>> + struct cdev cdev;
>>> + struct watchdog_device *wdd;
>>> + struct mutex lock;
>>> + unsigned long status; /* Internal status bits */
>>> +#define _WDOG_DEV_OPEN 0 /* Opened ? */
>>> +#define _WDOG_ALLOW_RELEASE 1 /* Did we receive the magic char ? */
>>> +};
>>> +
>>> +/*
>>> * Functions/procedures to be called by the core
>>> */
>>> extern int watchdog_dev_register(struct watchdog_device *);
>>> diff --git a/drivers/watchdog/watchdog_dev.c b/drivers/watchdog/watchdog_dev.c
>>> index c24392623e98..e8416bdc7037 100644
>>> --- a/drivers/watchdog/watchdog_dev.c
>>> +++ b/drivers/watchdog/watchdog_dev.c
>>> @@ -37,6 +37,7 @@
>>> #include <linux/errno.h> /* For the -ENODEV/... values */
>>> #include <linux/kernel.h> /* For printk/panic/... */
>>> #include <linux/fs.h> /* For file operations */
>>> +#include <linux/slab.h> /* For memory functions */
>>> #include <linux/watchdog.h> /* For watchdog specific items */
>>> #include <linux/miscdevice.h> /* For handling misc devices */
>>> #include <linux/init.h> /* For __init/__exit/... */
>>> @@ -47,12 +48,14 @@
>>> /* 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 *old_wdd;
>>> +static struct _watchdog_device *_old_wdd;
>>>
>>> /*
>>> * watchdog_ping: ping the watchdog.
>>> * @wdd: the watchdog device to ping
>>> *
>>> + * The caller must hold _wdd->lock.
>>> + *
>>> * If the watchdog has no own ping operation then it needs to be
>>> * restarted via the start operation. This wrapper function does
>>> * exactly that.
>>> @@ -61,25 +64,37 @@ static struct watchdog_device *old_wdd;
>>>
>>> static int watchdog_ping(struct watchdog_device *wdd)
>>> {
> Not sure this lockless wrappers are really needed.
I dropped _watchdog_ping() and handle locking from the calling code.
>>> - int err = 0;
>>> -
>>> - mutex_lock(&wdd->lock);
>>> -
>>> - if (test_bit(WDOG_UNREGISTERED, &wdd->status)) {
>>> - err = -ENODEV;
>>> - goto out_ping;
>>> - }
>>> + int err;
>>>
>>> if (!watchdog_active(wdd))
>>> - goto out_ping;
>>> + return 0;
>>>
>>> if (wdd->ops->ping)
>>> err = wdd->ops->ping(wdd); /* ping the watchdog */
>>> else
>>> err = wdd->ops->start(wdd); /* restart watchdog */
>>>
>>> -out_ping:
>>> - mutex_unlock(&wdd->lock);
>>> + return err;
>>> +}
>>> +
>>> +/*
>>> + * _watchdog_ping: ping the watchdog.
>>> + * @_wdd: Watchdog core device data
>>> + *
>>> + * Acquire _wdd->lock and call watchdog_ping() unless the watchdog
>>> + * driver has been unregistered.
>>> + */
>>> +static int _watchdog_ping(struct _watchdog_device *_wdd)
> Use of double underscore __ is more comon .
As mentioned above, I ended up dropping the function entirely.
>>> +{
>>> + struct watchdog_device *wdd;
>>> + int err = -ENODEV;
>>> +
>>> + mutex_lock(&_wdd->lock);
>>> + wdd = _wdd->wdd;
>>> + if (wdd)
>>> + err = watchdog_ping(wdd);
>>> + mutex_unlock(&_wdd->lock);
>>> +
>>> return err;
>>> }
>>>
>>> @@ -87,6 +102,8 @@ out_ping:
>>> * watchdog_start: wrapper to start the watchdog.
>>> * @wdd: the watchdog device to start
>>> *
>>> + * The caller must hold _wdd->lock.
>>> + *
>>> * Start the watchdog if it is not active and mark it active.
>>> * This function returns zero on success or a negative errno code for
>>> * failure.
>>> @@ -94,24 +111,15 @@ out_ping:
>>>
>>> static int watchdog_start(struct watchdog_device *wdd)
>>> {
>>> - int err = 0;
>>> -
>>> - mutex_lock(&wdd->lock);
>>> -
>>> - if (test_bit(WDOG_UNREGISTERED, &wdd->status)) {
>>> - err = -ENODEV;
>>> - goto out_start;
>>> - }
>>> + int err;
>>>
>>> if (watchdog_active(wdd))
>>> - goto out_start;
>>> + return 0;
>>>
>>> err = wdd->ops->start(wdd);
>>> if (err == 0)
>>> set_bit(WDOG_ACTIVE, &wdd->status);
>>>
>>> -out_start:
>>> - mutex_unlock(&wdd->lock);
>>> return err;
>>> }
>>>
>>> @@ -119,6 +127,8 @@ out_start:
>>> * watchdog_stop: wrapper to stop the watchdog.
>>> * @wdd: the watchdog device to stop
>>> *
>>> + * The caller must hold _wdd->lock.
>>> + *
>>> * Stop the watchdog if it is still active and unmark it active.
>>> * This function returns zero on success or a negative errno code for
>>> * failure.
>>> @@ -127,93 +137,58 @@ out_start:
>>>
>>> static int watchdog_stop(struct watchdog_device *wdd)
>>> {
>>> - int err = 0;
>>> -
>>> - mutex_lock(&wdd->lock);
>>> -
>>> - if (test_bit(WDOG_UNREGISTERED, &wdd->status)) {
>>> - err = -ENODEV;
>>> - goto out_stop;
>>> - }
>>> + int err;
>>>
>>> if (!watchdog_active(wdd))
>>> - goto out_stop;
>>> + return 0;
>>>
>>> if (test_bit(WDOG_NO_WAY_OUT, &wdd->status)) {
>>> dev_info(wdd->dev, "nowayout prevents watchdog being stopped!\n");
>>> - err = -EBUSY;
>>> - goto out_stop;
>>> + return -EBUSY;
>>> }
>>>
>>> err = wdd->ops->stop(wdd);
>>> if (err == 0)
>>> clear_bit(WDOG_ACTIVE, &wdd->status);
>>>
>>> -out_stop:
>>> - mutex_unlock(&wdd->lock);
>>> return err;
>>> }
>>>
>>> /*
>>> * watchdog_get_status: wrapper to get the watchdog status
>>> * @wdd: the watchdog device to get the status from
>>> - * @status: the status of the watchdog device
>>> + *
>>> + * The caller must hold _wdd->lock.
>>> *
>>> * Get the watchdog's status flags.
>>> */
>>>
>>> -static int watchdog_get_status(struct watchdog_device *wdd,
>>> - unsigned int *status)
>>> +static unsigned int watchdog_get_status(struct watchdog_device *wdd)
>>> {
>>> - int err = 0;
>>> -
>>> - *status = 0;
>>> if (!wdd->ops->status)
>>> - return -EOPNOTSUPP;
>>> -
>>> - mutex_lock(&wdd->lock);
>>> -
>>> - if (test_bit(WDOG_UNREGISTERED, &wdd->status)) {
>>> - err = -ENODEV;
>>> - goto out_status;
>>> - }
>>> -
>>> - *status = wdd->ops->status(wdd);
>>> + return 0;
>>>
>>> -out_status:
>>> - mutex_unlock(&wdd->lock);
>>> - return err;
>>> + return wdd->ops->status(wdd);
>>> }
>>>
>>> /*
>>> * watchdog_set_timeout: set the watchdog timer timeout
>>> * @wdd: the watchdog device to set the timeout for
>>> * @timeout: timeout to set in seconds
>>> + *
>>> + * The caller must hold _wdd->lock.
>>> */
>>>
>>> static int watchdog_set_timeout(struct watchdog_device *wdd,
>>> unsigned int timeout)
>>> {
>>> - int err;
>>> -
>>> if (!wdd->ops->set_timeout || !(wdd->info->options & WDIOF_SETTIMEOUT))
>>> return -EOPNOTSUPP;
>>>
>>> if (watchdog_timeout_invalid(wdd, timeout))
>>> return -EINVAL;
>>>
>>> - mutex_lock(&wdd->lock);
>>> -
>>> - if (test_bit(WDOG_UNREGISTERED, &wdd->status)) {
>>> - err = -ENODEV;
>>> - goto out_timeout;
>>> - }
>>> -
>>> - err = wdd->ops->set_timeout(wdd, timeout);
>>> -
>>> -out_timeout:
>>> - mutex_unlock(&wdd->lock);
>>> - return err;
>>> + return wdd->ops->set_timeout(wdd, timeout);
>>> }
>>>
>>> /*
>>> @@ -221,30 +196,22 @@ out_timeout:
>>> * @wdd: the watchdog device to get the remaining time from
>>> * @timeleft: the time that's left
>>> *
>>> + * The caller must hold _wdd->lock.
>>> + *
>>> * Get the time before a watchdog will reboot (if not pinged).
>>> */
>>>
>>> static int watchdog_get_timeleft(struct watchdog_device *wdd,
>>> unsigned int *timeleft)
>>> {
>>> - int err = 0;
>>> -
>>> *timeleft = 0;
>>> +
>>> if (!wdd->ops->get_timeleft)
>>> return -EOPNOTSUPP;
>>>
>>> - mutex_lock(&wdd->lock);
>>> -
>>> - if (test_bit(WDOG_UNREGISTERED, &wdd->status)) {
>>> - err = -ENODEV;
>>> - goto out_timeleft;
>>> - }
>>> -
>>> *timeleft = wdd->ops->get_timeleft(wdd);
>>>
>>> -out_timeleft:
>>> - mutex_unlock(&wdd->lock);
>>> - return err;
>>> + return 0;
>>> }
>>>
>>> #ifdef CONFIG_WATCHDOG_SYSFS
>>> @@ -261,14 +228,14 @@ static ssize_t status_show(struct device *dev, struct device_attribute *attr,
>>> char *buf)
>>> {
>>> struct watchdog_device *wdd = dev_get_drvdata(dev);
>>> - ssize_t status;
>>> - unsigned int val;
>>> + struct _watchdog_device *_wdd = wdd->wdd_data;
>>> + unsigned int status;
>>>
>>> - status = watchdog_get_status(wdd, &val);
>>> - if (!status)
>>> - status = sprintf(buf, "%u\n", val);
>>> + mutex_lock(&_wdd->lock);
>>> + status = watchdog_get_status(wdd);
>>> + mutex_unlock(&_wdd->lock);
>>>
>>> - return status;
>>> + return sprintf(buf, "%u\n", status);
>>> }
>>> static DEVICE_ATTR_RO(status);
>>>
>>> @@ -285,10 +252,13 @@ static ssize_t timeleft_show(struct device *dev, struct device_attribute *attr,
>>> char *buf)
>>> {
>>> struct watchdog_device *wdd = dev_get_drvdata(dev);
>>> + struct _watchdog_device *_wdd = wdd->wdd_data;
>>> ssize_t status;
>>> unsigned int val;
>>>
>>> + mutex_lock(&_wdd->lock);
>>> status = watchdog_get_timeleft(wdd, &val);
>>> + mutex_unlock(&_wdd->lock);
>>> if (!status)
>>> status = sprintf(buf, "%u\n", val);
>>>
>>> @@ -363,28 +333,17 @@ __ATTRIBUTE_GROUPS(wdt);
>>> * @wdd: the watchdog device to do the ioctl on
>>> * @cmd: watchdog command
>>> * @arg: argument pointer
>>> + *
>>> + * The caller must hold _wdd->lock.
>>> */
>>>
>>> static int watchdog_ioctl_op(struct watchdog_device *wdd, unsigned int cmd,
>>> unsigned long arg)
>>> {
>>> - int err;
>>> -
>>> if (!wdd->ops->ioctl)
>>> return -ENOIOCTLCMD;
>>>
>>> - mutex_lock(&wdd->lock);
>>> -
>>> - if (test_bit(WDOG_UNREGISTERED, &wdd->status)) {
>>> - err = -ENODEV;
>>> - goto out_ioctl;
>>> - }
>>> -
>>> - err = wdd->ops->ioctl(wdd, cmd, arg);
>>> -
>>> -out_ioctl:
>>> - mutex_unlock(&wdd->lock);
>>> - return err;
>>> + return wdd->ops->ioctl(wdd, cmd, arg);
>>> }
>>>
>>> /*
>>> @@ -402,7 +361,7 @@ out_ioctl:
>>> 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;
>>> + struct _watchdog_device *_wdd = file->private_data;
>>> size_t i;
>>> char c;
>>> int err;
>>> @@ -414,18 +373,18 @@ static ssize_t watchdog_write(struct file *file, const char __user *data,
>>> * Note: just in case someone wrote the magic character
>>> * five months ago...
>>> */
>>> - clear_bit(WDOG_ALLOW_RELEASE, &wdd->status);
>>> + clear_bit(_WDOG_ALLOW_RELEASE, &_wdd->status);
>>>
>>> /* scan to see whether or not we got the magic character */
>>> for (i = 0; i != len; i++) {
>>> if (get_user(c, data + i))
>>> return -EFAULT;
>>> if (c == 'V')
>>> - set_bit(WDOG_ALLOW_RELEASE, &wdd->status);
>>> + set_bit(_WDOG_ALLOW_RELEASE, &_wdd->status);
>>> }
>>>
>>> /* someone wrote to us, so we send the watchdog a keepalive ping */
>>> - err = watchdog_ping(wdd);
>>> + err = _watchdog_ping(_wdd);
>>> if (err < 0)
>>> return err;
>>>
>>> @@ -445,71 +404,94 @@ 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;
>>> + struct _watchdog_device *_wdd = file->private_data;
>>> void __user *argp = (void __user *)arg;
>>> + struct watchdog_device *wdd;
>>> int __user *p = argp;
>>> unsigned int val;
>>> - int err;
>>> + int err = 0;
>>> +
>>> + mutex_lock(&_wdd->lock);
>>> +
>>> + wdd = _wdd->wdd;
>>> + if (!wdd) {
>>> + err = -ENODEV;
>>> + goto out_ioctl;
>>> + }
>>>
>>> err = watchdog_ioctl_op(wdd, cmd, arg);
>>> if (err != -ENOIOCTLCMD)
>>> - return err;
>>> + goto out_ioctl;
>>>
>>> switch (cmd) {
>>> case WDIOC_GETSUPPORT:
>>> - return copy_to_user(argp, wdd->info,
>>> + err = copy_to_user(argp, wdd->info,
>>> sizeof(struct watchdog_info)) ? -EFAULT : 0;
>>> + break;
>>> case WDIOC_GETSTATUS:
>>> - err = watchdog_get_status(wdd, &val);
>>> - if (err == -ENODEV)
>>> - return err;
>>> - return put_user(val, p);
>>> + val = watchdog_get_status(wdd);
>>> + err = put_user(val, p);
>>> + break;
>>> case WDIOC_GETBOOTSTATUS:
>>> - return put_user(wdd->bootstatus, p);
>>> + err = put_user(wdd->bootstatus, p);
>>> + break;
>>> case WDIOC_SETOPTIONS:
>>> - if (get_user(val, p))
>>> - return -EFAULT;
>>> + if (get_user(val, p)) {
>>> + err = -EFAULT;
>>> + break;
>>> + }
>>> if (val & WDIOS_DISABLECARD) {
>>> err = watchdog_stop(wdd);
>>> if (err < 0)
>>> - return err;
>>> + break;
>>> }
>>> - if (val & WDIOS_ENABLECARD) {
>>> + if (val & WDIOS_ENABLECARD)
>>> err = watchdog_start(wdd);
>>> - if (err < 0)
>>> - return err;
>>> - }
>>> - return 0;
>>> + break;
>>> case WDIOC_KEEPALIVE:
>>> - if (!(wdd->info->options & WDIOF_KEEPALIVEPING))
>>> - return -EOPNOTSUPP;
>>> - return watchdog_ping(wdd);
>>> + if (!(wdd->info->options & WDIOF_KEEPALIVEPING)) {
>>> + err = -EOPNOTSUPP;
>>> + break;
>>> + }
>>> + err = watchdog_ping(wdd);
>>> + break;
>>> case WDIOC_SETTIMEOUT:
>>> - if (get_user(val, p))
>>> - return -EFAULT;
>>> + if (get_user(val, p)) {
>>> + err = -EFAULT;
>>> + break;
>>> + }
>>> err = watchdog_set_timeout(wdd, val);
>>> if (err < 0)
>>> - return err;
>>> + break;
>>> /* 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) */
>>> err = watchdog_ping(wdd);
>>> if (err < 0)
>>> - return err;
>>> + break;
> You are changing behaviour for the driver here as you are keeping lock
> over two driver op calls.
Yes, but the alternative (unlock, lock again, and check again if the
watchdog was unregistered) would be awkward, and I don't see where
this can be a problem.
Do you see a situation where holding the lock between calls into the driver
might be a problem ?
>>> /* Fall */
>>> case WDIOC_GETTIMEOUT:
>>> /* timeout == 0 means that we don't know the timeout */
>>> - if (wdd->timeout == 0)
>>> - return -EOPNOTSUPP;
>>> - return put_user(wdd->timeout, p);
>>> + if (wdd->timeout == 0) {
>>> + err = -EOPNOTSUPP;
>>> + break;
>>> + }
>>> + err = put_user(wdd->timeout, p);
This is another semantics change - the old code would succeed here if
the watchdog device was unregistered, unless the driver implements an
ioctl command. Now it fails with -ENODEV. This is also true for some
of the other ioctls above. I'll mention that in the commit log.
The alternative would be to keep the locking in the wrapper functions,
but I think the code is cleaner and more consistent this way.
Thanks,
Guenter
On Mon, Dec 21, 2015 at 03:28:39PM -0800, Guenter Roeck wrote:
> On 12/21/2015 09:31 AM, Damien Riegel wrote:
> >On Sun, Dec 20, 2015 at 01:04:59PM -0800, Guenter Roeck wrote:
> >>The watchdog character device is currently created in watchdog_dev.c,
> >>and the watchdog device in watchdog_core.c. This results in
> >>cross-dependencies, since device creation needs to know the watchdog
> >>character device number as well as the watchdog class, both of which
> >>reside in watchdog_dev.c.
> >>
> >>Create the watchdog device in watchdog_dev.c to simplify the code.
> >>
> >>Inspired by earlier patch set from Damien Riegel.
> >
> >Hi Guenter,
> >
> >The main purpose of my patch was to inverse the device creation and the
> >cdev registration to avoid a racy situation, bu you have dropped that in
> >this version. Is there a reason for that?
> >
> Every other driver I looked at does it in the same order (cdev first, device
> second). I don't really know if doing it differently has any undesired
> side effect, so I wanted to play safe.
>
> It would help a lot if someone listening to this exchange can confirm
> that it is ok to create the device first, followed by the character device.
The issue is that some drivers use watchdog_device->dev in their
watchdog_ops functions. With a quick grep, I could spot 3 examples:
- bcm2835_wdt_stop in bcm2835_wdt.c
- gpio_wdt_hwping in gpio_wdt.c
- a21_wdt_set_timeout in mena21_wdt.c
Maybe we should simply fix these drivers and keep watchdog_device->dev
for core internal usage?
Thanks,
Damien
On Mon, Dec 21, 2015 at 05:10:58PM -0800, Guenter Roeck wrote:
> On 12/21/2015 09:28 AM, Damien Riegel wrote:
> >On Sun, Dec 20, 2015 at 01:05:00PM -0800, Guenter Roeck wrote:
> >>All variables required by the watchdog core to manage a watchdog are
> >>currently stored in struct watchdog_device. The lifetime of those
> >>variables is determined by the watchdog driver. However, the lifetime
> >>of variables used by the watchdog core differs from the lifetime of
> >>struct watchdog_device. To remedy this situation, watchdog drivers
> >>can implement ref and unref callbacks, to be used by the watchdog
> >>core to lock struct watchdog_device in memory.
> >>
> >>While this solves the immediate problem, it depends on watchdog drivers
> >>to actually implement the ref/unref callbacks. This is error prone,
> >>often not implemented in the first place, or not implemented correctly.
> >>
> >>To solve the problem without requiring driver support, split the variables
> >>in struct watchdog_device into two data structures - one for variables
> >>associated with the watchdog driver, one for variables associated with
> >>the watchdog core. With this approach, the watchdog core can keep track
> >>of its variable lifetime and no longer depends on ref/unref callbacks
> >>in the driver. As a side effect, some of the variables originally in
> >>struct watchdog_driver are now private to the watchdog core and no longer
> >>visible in watchdog drivers.
> >>
> >>The 'ref' and 'unref' callbacks in struct watchdog_driver are no longer
> >>used and marked as deprecated.
> >
> >Two comments below. It's great to see that unbinding a driver no longer
> >triggers a kernel panic.
> >
> It should not have caused a panic to start with, but the ref/unref functions
> for the most part were either not or wrongly implemented. Not really
> surprising - it took me a while to understand the problem.
I tested on a driver which did not implement ref/unref. When ping is
called, it tries to dereference a freed 'struct watchdog_device' in
watchdog_get_drvdata, leading to a panic.
>
> [ ... ]
>
> >>
> >> /*
> >>+ * struct _watchdog_device - watchdog core internal data
> >
> >Think it should be /**. Anyway, I find it confusing to have both
> >_watchdog_device and watchdog_device, but I can't think of a better
> >name right now.
>
> I renamed the data structure to watchdog_data and moved it into watchdog_dev.c
> since it is only used there. No '**', though, because it is not a published
> API, but just an internal data structure.
>
> I also renamed the matching variable name to 'wd_data' (from '_wdd').
Okay. Also, why didn't you use the explicit type for 'wdd_data' in
'struct watchdog_device' instead of a void*?
>
> >>
> >> static void watchdog_cdev_unregister(struct watchdog_device *wdd)
> >> {
> >>- mutex_lock(&wdd->lock);
> >>- set_bit(WDOG_UNREGISTERED, &wdd->status);
> >>- mutex_unlock(&wdd->lock);
> >>+ struct _watchdog_device *_wdd = wdd->wdd_data;
> >>
> >>- cdev_del(&wdd->cdev);
> >>+ cdev_del(&_wdd->cdev);
> >> if (wdd->id == 0) {
> >> misc_deregister(&watchdog_miscdev);
> >>- old_wdd = NULL;
> >>+ _old_wdd = NULL;
> >> }
> >>+
> >>+ if (watchdog_active(wdd))
> >>+ pr_crit("watchdog%d: watchdog still running!\n", wdd->id);
> >
> >As it is now safe to unbind and rebind a driver, it means that a
> >watchdog driver probe function can now be called with a running
> >watchdog. Some drivers handle this situation, but I think that most of
> >them expect the watchdog to be off at this point.
> >
> No semantics change, though, and no change in behavior. Drivers _should_
> handle that situation today. Sure, many don't, but that is a different issue.
All right, that's what confused me. It was, and still will be, driver
responsiblity to handle this situation.
>
> I'll address handling an already-running watchdog by the watchdog core until
> the character device is opened in a separate patch set, but we'll have to have
> this series accepted before I re-introduce that. Even with that, it will still
> be the driver's responsibility to detect and report that/if a watchdog is
> already running.
>
> Thanks,
> Guenter
>
Thanks,
Damien
On 12/22/2015 07:33 AM, Damien Riegel wrote:
> On Mon, Dec 21, 2015 at 03:28:39PM -0800, Guenter Roeck wrote:
>> On 12/21/2015 09:31 AM, Damien Riegel wrote:
>>> On Sun, Dec 20, 2015 at 01:04:59PM -0800, Guenter Roeck wrote:
>>>> The watchdog character device is currently created in watchdog_dev.c,
>>>> and the watchdog device in watchdog_core.c. This results in
>>>> cross-dependencies, since device creation needs to know the watchdog
>>>> character device number as well as the watchdog class, both of which
>>>> reside in watchdog_dev.c.
>>>>
>>>> Create the watchdog device in watchdog_dev.c to simplify the code.
>>>>
>>>> Inspired by earlier patch set from Damien Riegel.
>>>
>>> Hi Guenter,
>>>
>>> The main purpose of my patch was to inverse the device creation and the
>>> cdev registration to avoid a racy situation, bu you have dropped that in
>>> this version. Is there a reason for that?
>>>
>> Every other driver I looked at does it in the same order (cdev first, device
>> second). I don't really know if doing it differently has any undesired
>> side effect, so I wanted to play safe.
>>
>> It would help a lot if someone listening to this exchange can confirm
>> that it is ok to create the device first, followed by the character device.
>
> The issue is that some drivers use watchdog_device->dev in their
> watchdog_ops functions. With a quick grep, I could spot 3 examples:
>
> - bcm2835_wdt_stop in bcm2835_wdt.c
> - gpio_wdt_hwping in gpio_wdt.c
> - a21_wdt_set_timeout in mena21_wdt.c
>
> Maybe we should simply fix these drivers and keep watchdog_device->dev
> for core internal usage?
>
Yes, I have been thinking about that - essentially move the ->dev pointer
to the internal data structure and either use pr_ functions in those drivers,
or save a pointer to the platform device (if available) and use it.
I think I'll start working on a follow-up patch set to do just that.
Guenter
On 12/22/2015 08:09 AM, Damien Riegel wrote:
> On Mon, Dec 21, 2015 at 05:10:58PM -0800, Guenter Roeck wrote:
>> On 12/21/2015 09:28 AM, Damien Riegel wrote:
>>> On Sun, Dec 20, 2015 at 01:05:00PM -0800, Guenter Roeck wrote:
>>>> All variables required by the watchdog core to manage a watchdog are
>>>> currently stored in struct watchdog_device. The lifetime of those
>>>> variables is determined by the watchdog driver. However, the lifetime
>>>> of variables used by the watchdog core differs from the lifetime of
>>>> struct watchdog_device. To remedy this situation, watchdog drivers
>>>> can implement ref and unref callbacks, to be used by the watchdog
>>>> core to lock struct watchdog_device in memory.
>>>>
>>>> While this solves the immediate problem, it depends on watchdog drivers
>>>> to actually implement the ref/unref callbacks. This is error prone,
>>>> often not implemented in the first place, or not implemented correctly.
>>>>
>>>> To solve the problem without requiring driver support, split the variables
>>>> in struct watchdog_device into two data structures - one for variables
>>>> associated with the watchdog driver, one for variables associated with
>>>> the watchdog core. With this approach, the watchdog core can keep track
>>>> of its variable lifetime and no longer depends on ref/unref callbacks
>>>> in the driver. As a side effect, some of the variables originally in
>>>> struct watchdog_driver are now private to the watchdog core and no longer
>>>> visible in watchdog drivers.
>>>>
>>>> The 'ref' and 'unref' callbacks in struct watchdog_driver are no longer
>>>> used and marked as deprecated.
>>>
>>> Two comments below. It's great to see that unbinding a driver no longer
>>> triggers a kernel panic.
>>>
>> It should not have caused a panic to start with, but the ref/unref functions
>> for the most part were either not or wrongly implemented. Not really
>> surprising - it took me a while to understand the problem.
>
> I tested on a driver which did not implement ref/unref. When ping is
> called, it tries to dereference a freed 'struct watchdog_device' in
> watchdog_get_drvdata, leading to a panic.
>
Yes, that will happen. Problem here is that the driver is buggy -
pretty much all drivers which dynamically allocate struct watchdog_device
have this problem.
This is the ultimate reason for coming up with this patch.
>>
>> [ ... ]
>>
>>>>
>>>> /*
>>>> + * struct _watchdog_device - watchdog core internal data
>>>
>>> Think it should be /**. Anyway, I find it confusing to have both
>>> _watchdog_device and watchdog_device, but I can't think of a better
>>> name right now.
>>
>> I renamed the data structure to watchdog_data and moved it into watchdog_dev.c
>> since it is only used there. No '**', though, because it is not a published
>> API, but just an internal data structure.
>>
>> I also renamed the matching variable name to 'wd_data' (from '_wdd').
>
> Okay. Also, why didn't you use the explicit type for 'wdd_data' in
> 'struct watchdog_device' instead of a void*?
>
This is to hide the data type, since the structure is not exported
to drivers.
I could pre-declare the structure with 'struct watchdog_data;', but
then I'd have to use a different structure name (watchdog_cdev_data,
maybe, or watchdog_core_data) to make it less generic. Any opinion ?
Would that be better / preferred ? I am 50/50 about it.
Thanks,
Guenter
On Tue, Dec 22, 2015 at 08:22:40AM -0800, Guenter Roeck wrote:
> On 12/22/2015 08:09 AM, Damien Riegel wrote:
> >On Mon, Dec 21, 2015 at 05:10:58PM -0800, Guenter Roeck wrote:
> >>On 12/21/2015 09:28 AM, Damien Riegel wrote:
> >>>On Sun, Dec 20, 2015 at 01:05:00PM -0800, Guenter Roeck wrote:
> >>>>All variables required by the watchdog core to manage a watchdog are
> >>>>currently stored in struct watchdog_device. The lifetime of those
> >>>>variables is determined by the watchdog driver. However, the lifetime
> >>>>of variables used by the watchdog core differs from the lifetime of
> >>>>struct watchdog_device. To remedy this situation, watchdog drivers
> >>>>can implement ref and unref callbacks, to be used by the watchdog
> >>>>core to lock struct watchdog_device in memory.
> >>>>
> >>>>While this solves the immediate problem, it depends on watchdog drivers
> >>>>to actually implement the ref/unref callbacks. This is error prone,
> >>>>often not implemented in the first place, or not implemented correctly.
> >>>>
> >>>>To solve the problem without requiring driver support, split the variables
> >>>>in struct watchdog_device into two data structures - one for variables
> >>>>associated with the watchdog driver, one for variables associated with
> >>>>the watchdog core. With this approach, the watchdog core can keep track
> >>>>of its variable lifetime and no longer depends on ref/unref callbacks
> >>>>in the driver. As a side effect, some of the variables originally in
> >>>>struct watchdog_driver are now private to the watchdog core and no longer
> >>>>visible in watchdog drivers.
> >>>>
> >>>>The 'ref' and 'unref' callbacks in struct watchdog_driver are no longer
> >>>>used and marked as deprecated.
> >>>
> >>>Two comments below. It's great to see that unbinding a driver no longer
> >>>triggers a kernel panic.
> >>>
> >>It should not have caused a panic to start with, but the ref/unref functions
> >>for the most part were either not or wrongly implemented. Not really
> >>surprising - it took me a while to understand the problem.
> >
> >I tested on a driver which did not implement ref/unref. When ping is
> >called, it tries to dereference a freed 'struct watchdog_device' in
> >watchdog_get_drvdata, leading to a panic.
> >
> Yes, that will happen. Problem here is that the driver is buggy -
> pretty much all drivers which dynamically allocate struct watchdog_device
> have this problem.
>
> This is the ultimate reason for coming up with this patch.
>
> >>
> >>[ ... ]
> >>
> >>>>
> >>>> /*
> >>>>+ * struct _watchdog_device - watchdog core internal data
> >>>
> >>>Think it should be /**. Anyway, I find it confusing to have both
> >>>_watchdog_device and watchdog_device, but I can't think of a better
> >>>name right now.
> >>
> >>I renamed the data structure to watchdog_data and moved it into watchdog_dev.c
> >>since it is only used there. No '**', though, because it is not a published
> >>API, but just an internal data structure.
> >>
> >>I also renamed the matching variable name to 'wd_data' (from '_wdd').
> >
> >Okay. Also, why didn't you use the explicit type for 'wdd_data' in
> >'struct watchdog_device' instead of a void*?
> >
>
> This is to hide the data type, since the structure is not exported
> to drivers.
>
> I could pre-declare the structure with 'struct watchdog_data;', but
> then I'd have to use a different structure name (watchdog_cdev_data,
> maybe, or watchdog_core_data) to make it less generic. Any opinion ?
> Would that be better / preferred ? I am 50/50 about it.
My personal preference would be to be explicit. That makes code
nagivation easier and it might help the compiler catch some mistakes.
Plus, as you moved the structure definition in watchdog_dev.c, it is
very clear that drivers aren't supposed to use it.
Thanks,
Damien
On 12/22/2015 11:28 AM, Damien Riegel wrote:
> On Tue, Dec 22, 2015 at 08:22:40AM -0800, Guenter Roeck wrote:
>> On 12/22/2015 08:09 AM, Damien Riegel wrote:
>>> On Mon, Dec 21, 2015 at 05:10:58PM -0800, Guenter Roeck wrote:
>>>> On 12/21/2015 09:28 AM, Damien Riegel wrote:
>>>>> On Sun, Dec 20, 2015 at 01:05:00PM -0800, Guenter Roeck wrote:
>>>>>> All variables required by the watchdog core to manage a watchdog are
>>>>>> currently stored in struct watchdog_device. The lifetime of those
>>>>>> variables is determined by the watchdog driver. However, the lifetime
>>>>>> of variables used by the watchdog core differs from the lifetime of
>>>>>> struct watchdog_device. To remedy this situation, watchdog drivers
>>>>>> can implement ref and unref callbacks, to be used by the watchdog
>>>>>> core to lock struct watchdog_device in memory.
>>>>>>
>>>>>> While this solves the immediate problem, it depends on watchdog drivers
>>>>>> to actually implement the ref/unref callbacks. This is error prone,
>>>>>> often not implemented in the first place, or not implemented correctly.
>>>>>>
>>>>>> To solve the problem without requiring driver support, split the variables
>>>>>> in struct watchdog_device into two data structures - one for variables
>>>>>> associated with the watchdog driver, one for variables associated with
>>>>>> the watchdog core. With this approach, the watchdog core can keep track
>>>>>> of its variable lifetime and no longer depends on ref/unref callbacks
>>>>>> in the driver. As a side effect, some of the variables originally in
>>>>>> struct watchdog_driver are now private to the watchdog core and no longer
>>>>>> visible in watchdog drivers.
>>>>>>
>>>>>> The 'ref' and 'unref' callbacks in struct watchdog_driver are no longer
>>>>>> used and marked as deprecated.
>>>>>
>>>>> Two comments below. It's great to see that unbinding a driver no longer
>>>>> triggers a kernel panic.
>>>>>
>>>> It should not have caused a panic to start with, but the ref/unref functions
>>>> for the most part were either not or wrongly implemented. Not really
>>>> surprising - it took me a while to understand the problem.
>>>
>>> I tested on a driver which did not implement ref/unref. When ping is
>>> called, it tries to dereference a freed 'struct watchdog_device' in
>>> watchdog_get_drvdata, leading to a panic.
>>>
>> Yes, that will happen. Problem here is that the driver is buggy -
>> pretty much all drivers which dynamically allocate struct watchdog_device
>> have this problem.
>>
>> This is the ultimate reason for coming up with this patch.
>>
>>>>
>>>> [ ... ]
>>>>
>>>>>>
>>>>>> /*
>>>>>> + * struct _watchdog_device - watchdog core internal data
>>>>>
>>>>> Think it should be /**. Anyway, I find it confusing to have both
>>>>> _watchdog_device and watchdog_device, but I can't think of a better
>>>>> name right now.
>>>>
>>>> I renamed the data structure to watchdog_data and moved it into watchdog_dev.c
>>>> since it is only used there. No '**', though, because it is not a published
>>>> API, but just an internal data structure.
>>>>
>>>> I also renamed the matching variable name to 'wd_data' (from '_wdd').
>>>
>>> Okay. Also, why didn't you use the explicit type for 'wdd_data' in
>>> 'struct watchdog_device' instead of a void*?
>>>
>>
>> This is to hide the data type, since the structure is not exported
>> to drivers.
>>
>> I could pre-declare the structure with 'struct watchdog_data;', but
>> then I'd have to use a different structure name (watchdog_cdev_data,
>> maybe, or watchdog_core_data) to make it less generic. Any opinion ?
>> Would that be better / preferred ? I am 50/50 about it.
>
> My personal preference would be to be explicit. That makes code
> nagivation easier and it might help the compiler catch some mistakes.
> Plus, as you moved the structure definition in watchdog_dev.c, it is
> very clear that drivers aren't supposed to use it.
>
Ok, makes sense. I'll do that.
Guenter
On 12/22/2015 02:05 PM, Tomas Winkler wrote:
>
> > Do you see a situation where holding the lock between calls into the driver
> > might be a problem ?
>
> I don't think u are holding the lock now in watchdog_unregister when WDOG_UNREGISTERED was dropped.
the lock is held while clearing the pointers:
mutex_lock(&wd_data->lock);
wd_data->wdd = NULL;
wdd->wd_data = NULL;
mutex_unlock(&wd_data->lock);
Guenter