Re: [lm-sensors] [patch 1/5] hwmon: Convert fschmd to unlocked_ioctl

From: Hans de Goede
Date: Fri Nov 06 2009 - 08:30:53 EST


On 11/06/2009 02:15 PM, Jean Delvare wrote:
Hi Thomas,

Sorry for the late answer.

On Thu, 15 Oct 2009 20:28:31 -0000, Thomas Gleixner wrote:
The conversion of fschmd watchdog ioctl to unlocked_ioctl needs to
protect the static watchdog_info variable for the WDIOC_GETSUPPORT

All other commands are safe w/o BKL as the called watchdog functions
are already serialized with watchdog_lock of the sensor.

Signed-off-by: Thomas Gleixner<tglx@xxxxxxxxxxxxx>
Cc: lm-sensors@xxxxxxxxxxxxxx
drivers/hwmon/fschmd.c | 8 +++++---
1 file changed, 5 insertions(+), 3 deletions(-)

Index: linux-2.6-tip/drivers/hwmon/fschmd.c
--- linux-2.6-tip.orig/drivers/hwmon/fschmd.c
+++ linux-2.6-tip/drivers/hwmon/fschmd.c
@@ -844,8 +844,8 @@ static ssize_t watchdog_write(struct fil
return count;

-static int watchdog_ioctl(struct inode *inode, struct file *filp,
- unsigned int cmd, unsigned long arg)
+static long watchdog_ioctl(struct file *filp, unsigned int cmd,
+ unsigned long arg)
static struct watchdog_info ident = {
@@ -857,11 +857,13 @@ static int watchdog_ioctl(struct inode *

switch (cmd) {
+ mutex_lock(&watchdog_data_mutex);
ident.firmware_version = data->revision;
if (!nowayout)
ident.options |= WDIOF_MAGICCLOSE;
if (copy_to_user((void __user *)arg,&ident, sizeof(ident)))
ret = -EFAULT;
+ mutex_unlock(&watchdog_data_mutex);

I'm not sure why we need to hold the mutex here? My understanding is
that watchdog_data_mutex protects watchdog_data_list and each
watchdog's kref. And the above code doesn't touch either.

What I am more worried about is why ident is declared static. This
looks like a bug to me. Instead of abusing watchdog_data_mutex to
workaround this, I'd rather remove the "static". I guess that the
current code happens to work because neither data->revision nor
nowayout can change over time, but this looks needlessly fragile.

Hans, any comment?

Note I'm on the road so do not have the code at question handy, but
I agree having ident static is not needed and is what needs to be fixed


To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at
Please read the FAQ at