From f4e9c82f64b524314a390b13d3ba7d483f09258f Mon Sep 17 00:00:00 2001 From: Hans de Goede Date: Tue, 22 May 2012 11:40:26 +0200 Subject: [PATCH] watchdog: Add Locking support This patch fixes some potential multithreading issues, despite only allowing one process to open the /dev/watchdog device, we can still get called multiple times at the same time, since a program could be using thread, or could share the fd after a fork. This causes 2 potential problems: 1) watchdog_start / open do an unlocked test_n_set / test_n_clear, if these 2 race, the watchdog could be stopped while the active bit indicates it is running or visa versa. 2) Most watchdog_dev drivers probably assume that only one watchdog-op will get called at a time, this is not necessary true atm. Signed-off-by: Hans de Goede Signed-off-by: Wim Van Sebroeck --- .../watchdog/watchdog-kernel-api.txt | 2 ++ drivers/watchdog/watchdog_core.c | 1 + drivers/watchdog/watchdog_dev.c | 21 +++++++++++++++++++ include/linux/watchdog.h | 5 +++++ 4 files changed, 29 insertions(+) diff --git a/Documentation/watchdog/watchdog-kernel-api.txt b/Documentation/watchdog/watchdog-kernel-api.txt index ce1fa22aa70b..08d34e11bc54 100644 --- a/Documentation/watchdog/watchdog-kernel-api.txt +++ b/Documentation/watchdog/watchdog-kernel-api.txt @@ -50,6 +50,7 @@ struct watchdog_device { unsigned int min_timeout; unsigned int max_timeout; void *driver_data; + struct mutex lock; unsigned long status; }; @@ -74,6 +75,7 @@ It contains following fields: * driver_data: a pointer to the drivers private data of a watchdog device. This data should only be accessed via the watchdog_set_drvdata and watchdog_get_drvdata routines. +* lock: Mutex for WatchDog Timer Driver Core internal use only. * status: this field contains a number of status bits that give extra information about the status of the device (Like: is the watchdog timer running/active, is the nowayout bit set, is the device opened via diff --git a/drivers/watchdog/watchdog_core.c b/drivers/watchdog/watchdog_core.c index 86a57673abf9..6aa46a90ff02 100644 --- a/drivers/watchdog/watchdog_core.c +++ b/drivers/watchdog/watchdog_core.c @@ -79,6 +79,7 @@ int watchdog_register_device(struct watchdog_device *wdd) * corrupted in a later stage then we expect a kernel panic! */ + mutex_init(&wdd->lock); id = ida_simple_get(&watchdog_ida, 0, MAX_DOGS, GFP_KERNEL); if (id < 0) return id; diff --git a/drivers/watchdog/watchdog_dev.c b/drivers/watchdog/watchdog_dev.c index 76d2572fed25..4d295d229a07 100644 --- a/drivers/watchdog/watchdog_dev.c +++ b/drivers/watchdog/watchdog_dev.c @@ -63,6 +63,8 @@ static int watchdog_ping(struct watchdog_device *wddev) { int err = 0; + mutex_lock(&wddev->lock); + if (!watchdog_active(wddev)) goto out_ping; @@ -72,6 +74,7 @@ static int watchdog_ping(struct watchdog_device *wddev) err = wddev->ops->start(wddev); /* restart watchdog */ out_ping: + mutex_unlock(&wddev->lock); return err; } @@ -88,6 +91,8 @@ static int watchdog_start(struct watchdog_device *wddev) { int err = 0; + mutex_lock(&wddev->lock); + if (watchdog_active(wddev)) goto out_start; @@ -96,6 +101,7 @@ static int watchdog_start(struct watchdog_device *wddev) set_bit(WDOG_ACTIVE, &wddev->status); out_start: + mutex_unlock(&wddev->lock); return err; } @@ -113,6 +119,8 @@ static int watchdog_stop(struct watchdog_device *wddev) { int err = 0; + mutex_lock(&wddev->lock); + if (!watchdog_active(wddev)) goto out_stop; @@ -127,6 +135,7 @@ static int watchdog_stop(struct watchdog_device *wddev) clear_bit(WDOG_ACTIVE, &wddev->status); out_stop: + mutex_unlock(&wddev->lock); return err; } @@ -147,8 +156,11 @@ static int watchdog_get_status(struct watchdog_device *wddev, if (!wddev->ops->status) return -EOPNOTSUPP; + mutex_lock(&wddev->lock); + *status = wddev->ops->status(wddev); + mutex_unlock(&wddev->lock); return err; } @@ -171,8 +183,11 @@ static int watchdog_set_timeout(struct watchdog_device *wddev, (timeout < wddev->min_timeout || timeout > wddev->max_timeout)) return -EINVAL; + mutex_lock(&wddev->lock); + err = wddev->ops->set_timeout(wddev, timeout); + mutex_unlock(&wddev->lock); return err; } @@ -193,8 +208,11 @@ static int watchdog_get_timeleft(struct watchdog_device *wddev, if (!wddev->ops->get_timeleft) return -EOPNOTSUPP; + mutex_lock(&wddev->lock); + *timeleft = wddev->ops->get_timeleft(wddev); + mutex_unlock(&wddev->lock); return err; } @@ -213,8 +231,11 @@ static int watchdog_ioctl_op(struct watchdog_device *wddev, unsigned int cmd, if (!wddev->ops->ioctl) return -ENOIOCTLCMD; + mutex_lock(&wddev->lock); + err = wddev->ops->ioctl(wddev, cmd, arg); + mutex_unlock(&wddev->lock); return err; } diff --git a/include/linux/watchdog.h b/include/linux/watchdog.h index c3545c5d918a..da1dc1b52744 100644 --- a/include/linux/watchdog.h +++ b/include/linux/watchdog.h @@ -104,6 +104,7 @@ struct watchdog_ops { * @min_timeout:The watchdog devices minimum timeout value. * @max_timeout:The watchdog devices maximum timeout value. * @driver-data:Pointer to the drivers private data. + * @lock: Lock for watchdog core internal use only. * @status: Field that contains the devices internal status bits. * * The watchdog_device structure contains all information about a @@ -111,6 +112,9 @@ struct watchdog_ops { * * The driver-data field may not be accessed directly. It must be accessed * via the watchdog_set_drvdata and watchdog_get_drvdata helpers. + * + * The lock field is for watchdog core internal use only and should not be + * touched. */ struct watchdog_device { int id; @@ -124,6 +128,7 @@ struct watchdog_device { unsigned int min_timeout; unsigned int max_timeout; void *driver_data; + struct mutex lock; unsigned long status; /* Bit numbers for status flags */ #define WDOG_ACTIVE 0 /* Is the watchdog running/active */ -- 2.20.1