[patch 0/5] BKL another bunch

[patch 0/5] BKL another bunch

[Thomas Gleixner]

Please review / comment

Thanks,

	tglx

[patch 1/5] hwmon: Convert fschmd to unlocked_ioctl

[Thomas Gleixner]

[-- Attachment #1: hwmon-convert-fschmd-to-unlocked-ioctl.patch --]
[-- Type: text/plain, Size: 1643 bytes --]

The conversion of fschmd watchdog ioctl to unlocked_ioctl needs to
protect the static watchdog_info variable for the WDIOC_GETSUPPORT
command.

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@linutronix.de>
Cc: lm-sensors@lm-sensors.org
---
 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 = {
 		.options = WDIOF_KEEPALIVEPING | WDIOF_SETTIMEOUT |
@@ -857,11 +857,13 @@ static int watchdog_ioctl(struct inode *
 
 	switch (cmd) {
 	case WDIOC_GETSUPPORT:
+		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);
 		break;
 
 	case WDIOC_GETSTATUS:
@@ -921,7 +923,7 @@ static const struct file_operations watc
 	.open = watchdog_open,
 	.release = watchdog_release,
 	.write = watchdog_write,
-	.ioctl = watchdog_ioctl,
+	.unlocked_ioctl = watchdog_ioctl,
 };
 
 

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

[Thomas Gleixner]

The conversion of fschmd watchdog ioctl to unlocked_ioctl needs to
protect the static watchdog_info variable for the WDIOC_GETSUPPORT
command.

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@linutronix.de>
Cc: lm-sensors@lm-sensors.org
---
 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 = {
 		.options = WDIOF_KEEPALIVEPING | WDIOF_SETTIMEOUT |
@@ -857,11 +857,13 @@ static int watchdog_ioctl(struct inode *
 
 	switch (cmd) {
 	case WDIOC_GETSUPPORT:
+		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);
 		break;
 
 	case WDIOC_GETSTATUS:
@@ -921,7 +923,7 @@ static const struct file_operations watc
 	.open = watchdog_open,
 	.release = watchdog_release,
 	.write = watchdog_write,
-	.ioctl = watchdog_ioctl,
+	.unlocked_ioctl = watchdog_ioctl,
 };
 
 



_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

[patch 2/5] macintosh: Remove BKL from nvram driver

[-- Attachment #1: drivers-mac-nvram-remove-bkl.patch --]
[-- Type: text/plain, Size: 1655 bytes --]

Drop the bkl from nvram_llseek() as it obviously protects nothing. The
file offset is safe in essence.

The ioctl can be converted to unlocked_ioctl because it just calls
pmac_get_partition() which reads a value from an array which was
initialized at early boot time. No need for serialization.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Cc: linuxppc-dev@ozlabs.org
---
 drivers/macintosh/nvram.c |   11 +++--------
 1 file changed, 3 insertions(+), 8 deletions(-)

Index: linux-2.6-tip/drivers/macintosh/nvram.c
===================================================================
--- linux-2.6-tip.orig/drivers/macintosh/nvram.c
+++ linux-2.6-tip/drivers/macintosh/nvram.c
@@ -13,7 +13,6 @@
 #include <linux/fcntl.h>
 #include <linux/nvram.h>
 #include <linux/init.h>
-#include <linux/smp_lock.h>
 #include <asm/uaccess.h>
 #include <asm/nvram.h>
 
@@ -21,7 +20,6 @@
 
 static loff_t nvram_llseek(struct file *file, loff_t offset, int origin)
 {
-	lock_kernel();
 	switch (origin) {
 	case 1:
 		offset += file->f_pos;
@@ -30,12 +28,10 @@ static loff_t nvram_llseek(struct file *
 		offset += NVRAM_SIZE;
 		break;
 	}
-	if (offset < 0) {
-		unlock_kernel();
+	if (offset < 0)
 		return -EINVAL;
-	}
+
 	file->f_pos = offset;
-	unlock_kernel();
 	return file->f_pos;
 }
 
@@ -76,8 +72,7 @@ static ssize_t write_nvram(struct file *
 	return p - buf;
 }
 
-static int nvram_ioctl(struct inode *inode, struct file *file,
-	unsigned int cmd, unsigned long arg)
+static long nvram_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 {
 	switch(cmd) {
 		case PMAC_NVRAM_GET_OFFSET:

[patch 2/5] macintosh: Remove BKL from nvram driver

[Thomas Gleixner]

Drop the bkl from nvram_llseek() as it obviously protects nothing. The
file offset is safe in essence.

The ioctl can be converted to unlocked_ioctl because it just calls
pmac_get_partition() which reads a value from an array which was
initialized at early boot time. No need for serialization.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Cc: Benjamin Herrenschmidt <benh@kernel.crashing.org>
Cc: linuxppc-dev@ozlabs.org
---
 drivers/macintosh/nvram.c |   11 +++--------
 1 file changed, 3 insertions(+), 8 deletions(-)

Index: linux-2.6-tip/drivers/macintosh/nvram.c
===================================================================
--- linux-2.6-tip.orig/drivers/macintosh/nvram.c
+++ linux-2.6-tip/drivers/macintosh/nvram.c
@@ -13,7 +13,6 @@
 #include <linux/fcntl.h>
 #include <linux/nvram.h>
 #include <linux/init.h>
-#include <linux/smp_lock.h>
 #include <asm/uaccess.h>
 #include <asm/nvram.h>
 
@@ -21,7 +20,6 @@
 
 static loff_t nvram_llseek(struct file *file, loff_t offset, int origin)
 {
-	lock_kernel();
 	switch (origin) {
 	case 1:
 		offset += file->f_pos;
@@ -30,12 +28,10 @@ static loff_t nvram_llseek(struct file *
 		offset += NVRAM_SIZE;
 		break;
 	}
-	if (offset < 0) {
-		unlock_kernel();
+	if (offset < 0)
 		return -EINVAL;
-	}
+
 	file->f_pos = offset;
-	unlock_kernel();
 	return file->f_pos;
 }
 
@@ -76,8 +72,7 @@ static ssize_t write_nvram(struct file *
 	return p - buf;
 }
 
-static int nvram_ioctl(struct inode *inode, struct file *file,
-	unsigned int cmd, unsigned long arg)
+static long nvram_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 {
 	switch(cmd) {
 		case PMAC_NVRAM_GET_OFFSET:

[patch 3/5] sunrpc: Convert to unlocked_ioctl and remove stray smp_lock.h

[Thomas Gleixner]

[-- Attachment #1: net-sunrpc-convert-to-unlocked-ioctl.patch --]
[-- Type: text/plain, Size: 4907 bytes --]

The ioctls in sunrpc/cache.c can be converted to unlocked_ioctl safely
because the operation is already protected by queue_lock.

The ioctl in rpc_pipe.c can be converted by serializing via
inode->i_mutex.

Remove the stray smp_lock.h include in svc_xprt.c which was not
removed when the BKL was replaced in commit ed2d8aed

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: Trond Myklebust <Trond.Myklebust@netapp.com>
Cc: netdev@vger.kernel.org
---
 net/sunrpc/cache.c    |   25 ++++++++++++-------------
 net/sunrpc/rpc_pipe.c |   16 ++++++++++------
 net/sunrpc/svc_xprt.c |    1 -
 3 files changed, 22 insertions(+), 20 deletions(-)

Index: linux-2.6-tip/net/sunrpc/cache.c
===================================================================
--- linux-2.6-tip.orig/net/sunrpc/cache.c
+++ linux-2.6-tip/net/sunrpc/cache.c
@@ -815,9 +815,8 @@ static unsigned int cache_poll(struct fi
 	return mask;
 }
 
-static int cache_ioctl(struct inode *ino, struct file *filp,
-		       unsigned int cmd, unsigned long arg,
-		       struct cache_detail *cd)
+static long cache_ioctl(struct file *filp, unsigned int cmd, unsigned long arg,
+			struct cache_detail *cd)
 {
 	int len = 0;
 	struct cache_reader *rp = filp->private_data;
@@ -1332,12 +1331,12 @@ static unsigned int cache_poll_procfs(st
 	return cache_poll(filp, wait, cd);
 }
 
-static int cache_ioctl_procfs(struct inode *inode, struct file *filp,
-			      unsigned int cmd, unsigned long arg)
+static long cache_ioctl_procfs(struct file *filp, unsigned int cmd,
+			       unsigned long arg)
 {
-	struct cache_detail *cd = PDE(inode)->data;
+	struct cache_detail *cd = PDE(filp->f_path.dentry->d_inode)->data;
 
-	return cache_ioctl(inode, filp, cmd, arg, cd);
+	return cache_ioctl(filp, cmd, arg, cd);
 }
 
 static int cache_open_procfs(struct inode *inode, struct file *filp)
@@ -1360,7 +1359,7 @@ static const struct file_operations cach
 	.read		= cache_read_procfs,
 	.write		= cache_write_procfs,
 	.poll		= cache_poll_procfs,
-	.ioctl		= cache_ioctl_procfs, /* for FIONREAD */
+	.unlocked_ioctl	= cache_ioctl_procfs, /* for FIONREAD */
 	.open		= cache_open_procfs,
 	.release	= cache_release_procfs,
 };
@@ -1526,12 +1525,12 @@ static unsigned int cache_poll_pipefs(st
 	return cache_poll(filp, wait, cd);
 }
 
-static int cache_ioctl_pipefs(struct inode *inode, struct file *filp,
-			      unsigned int cmd, unsigned long arg)
+static long cache_ioctl_pipefs(struct file *filp, unsigned int cmd,
+			       unsigned long arg)
 {
-	struct cache_detail *cd = RPC_I(inode)->private;
+	struct cache_detail *cd = RPC_I(filp->f_path.dentry->d_inode)->private;
 
-	return cache_ioctl(inode, filp, cmd, arg, cd);
+	return cache_ioctl(filp, cmd, arg, cd);
 }
 
 static int cache_open_pipefs(struct inode *inode, struct file *filp)
@@ -1554,7 +1553,7 @@ const struct file_operations cache_file_
 	.read		= cache_read_pipefs,
 	.write		= cache_write_pipefs,
 	.poll		= cache_poll_pipefs,
-	.ioctl		= cache_ioctl_pipefs, /* for FIONREAD */
+	.unlocked_ioctl	= cache_ioctl_pipefs, /* for FIONREAD */
 	.open		= cache_open_pipefs,
 	.release	= cache_release_pipefs,
 };
Index: linux-2.6-tip/net/sunrpc/rpc_pipe.c
===================================================================
--- linux-2.6-tip.orig/net/sunrpc/rpc_pipe.c
+++ linux-2.6-tip/net/sunrpc/rpc_pipe.c
@@ -308,23 +308,27 @@ rpc_pipe_poll(struct file *filp, struct 
 	return mask;
 }
 
-static int
-rpc_pipe_ioctl(struct inode *ino, struct file *filp,
-		unsigned int cmd, unsigned long arg)
+static long
+rpc_pipe_ioctl(struct file *filp, unsigned int cmd, unsigned long arg)
 {
-	struct rpc_inode *rpci = RPC_I(filp->f_path.dentry->d_inode);
+	struct inode *inode = filp->f_path.dentry->d_inode;
+	struct rpc_inode *rpci = RPC_I(inode);
 	int len;
 
 	switch (cmd) {
 	case FIONREAD:
-		if (rpci->ops == NULL)
+		mutex_lock(&inode->i_mutex);
+		if (rpci->ops == NULL) {
+			mutex_unlock(&inode->i_mutex);
 			return -EPIPE;
+		}
 		len = rpci->pipelen;
 		if (filp->private_data) {
 			struct rpc_pipe_msg *msg;
 			msg = (struct rpc_pipe_msg *)filp->private_data;
 			len += msg->len - msg->copied;
 		}
+		mutex_unlock(&inode->i_mutex);
 		return put_user(len, (int __user *)arg);
 	default:
 		return -EINVAL;
@@ -337,7 +341,7 @@ static const struct file_operations rpc_
 	.read		= rpc_pipe_read,
 	.write		= rpc_pipe_write,
 	.poll		= rpc_pipe_poll,
-	.ioctl		= rpc_pipe_ioctl,
+	.unlocked_ioctl	= rpc_pipe_ioctl,
 	.open		= rpc_pipe_open,
 	.release	= rpc_pipe_release,
 };
Index: linux-2.6-tip/net/sunrpc/svc_xprt.c
===================================================================
--- linux-2.6-tip.orig/net/sunrpc/svc_xprt.c
+++ linux-2.6-tip/net/sunrpc/svc_xprt.c
@@ -5,7 +5,6 @@
  */
 
 #include <linux/sched.h>
-#include <linux/smp_lock.h>
 #include <linux/errno.h>
 #include <linux/freezer.h>
 #include <linux/kthread.h>

[patch 4/5] mtd: Remove BKL and convert to unlocked_ioctl

[Thomas Gleixner]

[-- Attachment #1: mtd-remove-bkl-and-convert-to-unlocked-ioctl.patch --]
[-- Type: text/plain, Size: 3115 bytes --]

mtd_open() got lock/unlock kernel from the big BKL push down, but it
never relied on the BKL serialization as get_mtd_device() takes care
of serialization vs. device init/teardown.

mtd_ioctl() is safe w/o the BKL as well. The data which is copied from
the mtd data structure is either set up during device initialization
or statistics which have never been protected by the BKL against
concurrent modification. The mtd functions which are called from
various ioctl commands are safe as well.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Cc: David Woodhouse <dwmw2@infradead.org>
Cc: linux-mtd@lists.infradead.org
---
 drivers/mtd/mtdchar.c |   30 ++++++++----------------------
 1 file changed, 8 insertions(+), 22 deletions(-)

Index: linux-2.6-tip/drivers/mtd/mtdchar.c
===================================================================
--- linux-2.6-tip.orig/drivers/mtd/mtdchar.c
+++ linux-2.6-tip/drivers/mtd/mtdchar.c
@@ -12,7 +12,6 @@
 #include <linux/module.h>
 #include <linux/slab.h>
 #include <linux/sched.h>
-#include <linux/smp_lock.h>
 #include <linux/backing-dev.h>
 #include <linux/compat.h>
 
@@ -74,18 +73,14 @@ static int mtd_open(struct inode *inode,
 	if ((file->f_mode & FMODE_WRITE) && (minor & 1))
 		return -EACCES;
 
-	lock_kernel();
 	mtd = get_mtd_device(NULL, devnum);
 
-	if (IS_ERR(mtd)) {
-		ret = PTR_ERR(mtd);
-		goto out;
-	}
+	if (IS_ERR(mtd))
+		return PTR_ERR(mtd);
 
 	if (mtd->type == MTD_ABSENT) {
 		put_mtd_device(mtd);
-		ret = -ENODEV;
-		goto out;
+		return -ENODEV;
 	}
 
 	if (mtd->backing_dev_info)
@@ -94,21 +89,17 @@ static int mtd_open(struct inode *inode,
 	/* You can't open it RW if it's not a writeable device */
 	if ((file->f_mode & FMODE_WRITE) && !(mtd->flags & MTD_WRITEABLE)) {
 		put_mtd_device(mtd);
-		ret = -EACCES;
-		goto out;
+		return -EACCES;
 	}
 
 	mfi = kzalloc(sizeof(*mfi), GFP_KERNEL);
 	if (!mfi) {
 		put_mtd_device(mtd);
-		ret = -ENOMEM;
-		goto out;
+		return -ENOMEM;
 	}
 	mfi->mtd = mtd;
 	file->private_data = mfi;
 
-out:
-	unlock_kernel();
 	return ret;
 } /* mtd_open */
 
@@ -450,13 +441,12 @@ static int mtd_do_readoob(struct mtd_inf
 	return ret;
 }
 
-static int mtd_ioctl(struct inode *inode, struct file *file,
-		     u_int cmd, u_long arg)
+static long mtd_ioctl(struct file *file, u_int cmd, u_long arg)
 {
 	struct mtd_file_info *mfi = file->private_data;
 	struct mtd_info *mtd = mfi->mtd;
 	void __user *argp = (void __user *)arg;
-	int ret = 0;
+	long ret = 0;
 	u_long size;
 	struct mtd_info_user info;
 
@@ -842,8 +832,6 @@ static long mtd_compat_ioctl(struct file
 	void __user *argp = compat_ptr(arg);
 	int ret = 0;
 
-	lock_kernel();
-
 	switch (cmd) {
 	case MEMWRITEOOB32:
 	{
@@ -877,8 +865,6 @@ static long mtd_compat_ioctl(struct file
 		ret = mtd_ioctl(inode, file, cmd, (unsigned long)argp);
 	}
 
-	unlock_kernel();
-
 	return ret;
 }
 
@@ -942,7 +928,7 @@ static const struct file_operations mtd_
 	.llseek		= mtd_lseek,
 	.read		= mtd_read,
 	.write		= mtd_write,
-	.ioctl		= mtd_ioctl,
+	.unlocked_ioctl	= mtd_ioctl,
 #ifdef CONFIG_COMPAT
 	.compat_ioctl	= mtd_compat_ioctl,
 #endif

[patch 4/5] mtd: Remove BKL and convert to unlocked_ioctl

[Thomas Gleixner]

[-- Attachment #1: mtd-remove-bkl-and-convert-to-unlocked-ioctl.patch --]
[-- Type: text/plain, Size: 3113 bytes --]

mtd_open() got lock/unlock kernel from the big BKL push down, but it
never relied on the BKL serialization as get_mtd_device() takes care
of serialization vs. device init/teardown.

mtd_ioctl() is safe w/o the BKL as well. The data which is copied from
the mtd data structure is either set up during device initialization
or statistics which have never been protected by the BKL against
concurrent modification. The mtd functions which are called from
various ioctl commands are safe as well.

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Cc: David Woodhouse <dwmw2@infradead.org>
Cc: linux-mtd@lists.infradead.org
---
 drivers/mtd/mtdchar.c |   30 ++++++++----------------------
 1 file changed, 8 insertions(+), 22 deletions(-)

Index: linux-2.6-tip/drivers/mtd/mtdchar.c
===================================================================
--- linux-2.6-tip.orig/drivers/mtd/mtdchar.c
+++ linux-2.6-tip/drivers/mtd/mtdchar.c
@@ -12,7 +12,6 @@
 #include <linux/module.h>
 #include <linux/slab.h>
 #include <linux/sched.h>
-#include <linux/smp_lock.h>
 #include <linux/backing-dev.h>
 #include <linux/compat.h>
 
@@ -74,18 +73,14 @@ static int mtd_open(struct inode *inode,
 	if ((file->f_mode & FMODE_WRITE) && (minor & 1))
 		return -EACCES;
 
-	lock_kernel();
 	mtd = get_mtd_device(NULL, devnum);
 
-	if (IS_ERR(mtd)) {
-		ret = PTR_ERR(mtd);
-		goto out;
-	}
+	if (IS_ERR(mtd))
+		return PTR_ERR(mtd);
 
 	if (mtd->type == MTD_ABSENT) {
 		put_mtd_device(mtd);
-		ret = -ENODEV;
-		goto out;
+		return -ENODEV;
 	}
 
 	if (mtd->backing_dev_info)
@@ -94,21 +89,17 @@ static int mtd_open(struct inode *inode,
 	/* You can't open it RW if it's not a writeable device */
 	if ((file->f_mode & FMODE_WRITE) && !(mtd->flags & MTD_WRITEABLE)) {
 		put_mtd_device(mtd);
-		ret = -EACCES;
-		goto out;
+		return -EACCES;
 	}
 
 	mfi = kzalloc(sizeof(*mfi), GFP_KERNEL);
 	if (!mfi) {
 		put_mtd_device(mtd);
-		ret = -ENOMEM;
-		goto out;
+		return -ENOMEM;
 	}
 	mfi->mtd = mtd;
 	file->private_data = mfi;
 
-out:
-	unlock_kernel();
 	return ret;
 } /* mtd_open */
 
@@ -450,13 +441,12 @@ static int mtd_do_readoob(struct mtd_inf
 	return ret;
 }
 
-static int mtd_ioctl(struct inode *inode, struct file *file,
-		     u_int cmd, u_long arg)
+static long mtd_ioctl(struct file *file, u_int cmd, u_long arg)
 {
 	struct mtd_file_info *mfi = file->private_data;
 	struct mtd_info *mtd = mfi->mtd;
 	void __user *argp = (void __user *)arg;
-	int ret = 0;
+	long ret = 0;
 	u_long size;
 	struct mtd_info_user info;
 
@@ -842,8 +832,6 @@ static long mtd_compat_ioctl(struct file
 	void __user *argp = compat_ptr(arg);
 	int ret = 0;
 
-	lock_kernel();
-
 	switch (cmd) {
 	case MEMWRITEOOB32:
 	{
@@ -877,8 +865,6 @@ static long mtd_compat_ioctl(struct file
 		ret = mtd_ioctl(inode, file, cmd, (unsigned long)argp);
 	}
 
-	unlock_kernel();
-
 	return ret;
 }
 
@@ -942,7 +928,7 @@ static const struct file_operations mtd_
 	.llseek		= mtd_lseek,
 	.read		= mtd_read,
 	.write		= mtd_write,
-	.ioctl		= mtd_ioctl,
+	.unlocked_ioctl	= mtd_ioctl,
 #ifdef CONFIG_COMPAT
 	.compat_ioctl	= mtd_compat_ioctl,
 #endif

[patch 5/5] bluetooth: Remove stub ioctl in hci_vhci

[Thomas Gleixner]

[-- Attachment #1: bluetooth-hci-vhci-remove-stub-ioctl.patch --]
[-- Type: text/plain, Size: 1115 bytes --]

Remove the empty ioctl which just returns -EINVAL. vfs_ioctl() will
return -ENOTTY instead, but I doubt that any application will notice
the difference :)

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Cc: Marcel Holtmann <marcel@holtmann.org>
Cc: linux-bluetooth@vger.kernel.org

---
 drivers/bluetooth/hci_vhci.c |    7 -------
 1 file changed, 7 deletions(-)

Index: linux-2.6-tip/drivers/bluetooth/hci_vhci.c
===================================================================
--- linux-2.6-tip.orig/drivers/bluetooth/hci_vhci.c
+++ linux-2.6-tip/drivers/bluetooth/hci_vhci.c
@@ -218,12 +218,6 @@ static unsigned int vhci_poll(struct fil
 	return POLLOUT | POLLWRNORM;
 }
 
-static int vhci_ioctl(struct inode *inode, struct file *file,
-					unsigned int cmd, unsigned long arg)
-{
-	return -EINVAL;
-}
-
 static int vhci_open(struct inode *inode, struct file *file)
 {
 	struct vhci_data *data;
@@ -287,7 +281,6 @@ static const struct file_operations vhci
 	.read		= vhci_read,
 	.write		= vhci_write,
 	.poll		= vhci_poll,
-	.ioctl		= vhci_ioctl,
 	.open		= vhci_open,
 	.release	= vhci_release,
 };

[patch 5/5] bluetooth: Remove stub ioctl in hci_vhci

[Thomas Gleixner]

Remove the empty ioctl which just returns -EINVAL. vfs_ioctl() will
return -ENOTTY instead, but I doubt that any application will notice
the difference :)

Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
Cc: Marcel Holtmann <marcel@holtmann.org>
Cc: linux-bluetooth@vger.kernel.org

---
 drivers/bluetooth/hci_vhci.c |    7 -------
 1 file changed, 7 deletions(-)

Index: linux-2.6-tip/drivers/bluetooth/hci_vhci.c
===================================================================
--- linux-2.6-tip.orig/drivers/bluetooth/hci_vhci.c
+++ linux-2.6-tip/drivers/bluetooth/hci_vhci.c
@@ -218,12 +218,6 @@ static unsigned int vhci_poll(struct fil
 	return POLLOUT | POLLWRNORM;
 }
 
-static int vhci_ioctl(struct inode *inode, struct file *file,
-					unsigned int cmd, unsigned long arg)
-{
-	return -EINVAL;
-}
-
 static int vhci_open(struct inode *inode, struct file *file)
 {
 	struct vhci_data *data;
@@ -287,7 +281,6 @@ static const struct file_operations vhci
 	.read		= vhci_read,
 	.write		= vhci_write,
 	.poll		= vhci_poll,
-	.ioctl		= vhci_ioctl,
 	.open		= vhci_open,
 	.release	= vhci_release,
 };

Re: [patch 4/5] mtd: Remove BKL and convert to unlocked_ioctl

[Artem Bityutskiy]

On Thu, 2009-10-15 at 20:28 +0000, Thomas Gleixner wrote:
> plain text document attachment
> (mtd-remove-bkl-and-convert-to-unlocked-ioctl.patch)
> mtd_open() got lock/unlock kernel from the big BKL push down, but it
> never relied on the BKL serialization as get_mtd_device() takes care
> of serialization vs. device init/teardown.
> 
> mtd_ioctl() is safe w/o the BKL as well. The data which is copied from
> the mtd data structure is either set up during device initialization
> or statistics which have never been protected by the BKL against
> concurrent modification. The mtd functions which are called from
> various ioctl commands are safe as well.
> 
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> Cc: David Woodhouse <dwmw2@infradead.org>
> Cc: linux-mtd@lists.infradead.org
> ---
>  drivers/mtd/mtdchar.c |   30 ++++++++----------------------
>  1 file changed, 8 insertions(+), 22 deletions(-)

[dedekind@eru l2-mtd-2.6.git]$ make -j8
  CHK     include/linux/version.h
  CHK     include/linux/utsrelease.h
  SYMLINK include/asm -> include/asm-x86
  CALL    scripts/checksyscalls.sh
  CHK     include/linux/compile.h
  CC [M]  drivers/mtd/mtdchar.o
drivers/mtd/mtdchar.c: In function ‘mtd_compat_ioctl’:
drivers/mtd/mtdchar.c:865: warning: passing argument 1 of ‘mtd_ioctl’ from incompatible pointer type
drivers/mtd/mtdchar.c:444: note: expected ‘struct file *’ but argument is of type ‘struct inode *’
drivers/mtd/mtdchar.c:865: warning: passing argument 2 of ‘mtd_ioctl’ makes integer from pointer without a cast
drivers/mtd/mtdchar.c:444: note: expected ‘u_int’ but argument is of type ‘struct file *’
drivers/mtd/mtdchar.c:865: error: too many arguments to function ‘mtd_ioctl’

-- 
Best Regards,
Artem Bityutskiy (Артём Битюцкий)

Re: [patch 4/5] mtd: Remove BKL and convert to unlocked_ioctl

[Artem Bityutskiy]

On Thu, 2009-10-15 at 20:28 +0000, Thomas Gleixner wrote:
> plain text document attachment
> (mtd-remove-bkl-and-convert-to-unlocked-ioctl.patch)
> mtd_open() got lock/unlock kernel from the big BKL push down, but it
> never relied on the BKL serialization as get_mtd_device() takes care
> of serialization vs. device init/teardown.
> 
> mtd_ioctl() is safe w/o the BKL as well. The data which is copied from
> the mtd data structure is either set up during device initialization
> or statistics which have never been protected by the BKL against
> concurrent modification. The mtd functions which are called from
> various ioctl commands are safe as well.
> 
> Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> Cc: David Woodhouse <dwmw2@infradead.org>
> Cc: linux-mtd@lists.infradead.org
> ---
>  drivers/mtd/mtdchar.c |   30 ++++++++----------------------
>  1 file changed, 8 insertions(+), 22 deletions(-)

[dedekind@eru l2-mtd-2.6.git]$ make -j8
  CHK     include/linux/version.h
  CHK     include/linux/utsrelease.h
  SYMLINK include/asm -> include/asm-x86
  CALL    scripts/checksyscalls.sh
  CHK     include/linux/compile.h
  CC [M]  drivers/mtd/mtdchar.o
drivers/mtd/mtdchar.c: In function ‘mtd_compat_ioctl’:
drivers/mtd/mtdchar.c:865: warning: passing argument 1 of ‘mtd_ioctl’ from incompatible pointer type
drivers/mtd/mtdchar.c:444: note: expected ‘struct file *’ but argument is of type ‘struct inode *’
drivers/mtd/mtdchar.c:865: warning: passing argument 2 of ‘mtd_ioctl’ makes integer from pointer without a cast
drivers/mtd/mtdchar.c:444: note: expected ‘u_int’ but argument is of type ‘struct file *’
drivers/mtd/mtdchar.c:865: error: too many arguments to function ‘mtd_ioctl’

-- 
Best Regards,
Artem Bityutskiy (Артём Битюцкий)

Re: [patch 4/5] mtd: Remove BKL and convert to unlocked_ioctl

[Thomas Gleixner]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 1894 bytes --]

On Fri, 16 Oct 2009, Artem Bityutskiy wrote:
> On Thu, 2009-10-15 at 20:28 +0000, Thomas Gleixner wrote:
> > plain text document attachment
> > (mtd-remove-bkl-and-convert-to-unlocked-ioctl.patch)
> > mtd_open() got lock/unlock kernel from the big BKL push down, but it
> > never relied on the BKL serialization as get_mtd_device() takes care
> > of serialization vs. device init/teardown.
> > 
> > mtd_ioctl() is safe w/o the BKL as well. The data which is copied from
> > the mtd data structure is either set up during device initialization
> > or statistics which have never been protected by the BKL against
> > concurrent modification. The mtd functions which are called from
> > various ioctl commands are safe as well.
> > 
> > Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> > Cc: David Woodhouse <dwmw2@infradead.org>
> > Cc: linux-mtd@lists.infradead.org
> > ---
> >  drivers/mtd/mtdchar.c |   30 ++++++++----------------------
> >  1 file changed, 8 insertions(+), 22 deletions(-)
> 
> [dedekind@eru l2-mtd-2.6.git]$ make -j8
>   CHK     include/linux/version.h
>   CHK     include/linux/utsrelease.h
>   SYMLINK include/asm -> include/asm-x86
>   CALL    scripts/checksyscalls.sh
>   CHK     include/linux/compile.h
>   CC [M]  drivers/mtd/mtdchar.o
> drivers/mtd/mtdchar.c: In function ‘mtd_compat_ioctl’:
> drivers/mtd/mtdchar.c:865: warning: passing argument 1 of ‘mtd_ioctl’ from incompatible pointer type
> drivers/mtd/mtdchar.c:444: note: expected ‘struct file *’ but argument is of type ‘struct inode *’
> drivers/mtd/mtdchar.c:865: warning: passing argument 2 of ‘mtd_ioctl’ makes integer from pointer without a cast
> drivers/mtd/mtdchar.c:444: note: expected ‘u_int’ but argument is of type ‘struct file *’
> drivers/mtd/mtdchar.c:865: error: too many arguments to function ‘mtd_ioctl’

Yeah, found and fixed that already.

      tglx

Re: [patch 4/5] mtd: Remove BKL and convert to unlocked_ioctl

[Thomas Gleixner]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 1894 bytes --]

On Fri, 16 Oct 2009, Artem Bityutskiy wrote:
> On Thu, 2009-10-15 at 20:28 +0000, Thomas Gleixner wrote:
> > plain text document attachment
> > (mtd-remove-bkl-and-convert-to-unlocked-ioctl.patch)
> > mtd_open() got lock/unlock kernel from the big BKL push down, but it
> > never relied on the BKL serialization as get_mtd_device() takes care
> > of serialization vs. device init/teardown.
> > 
> > mtd_ioctl() is safe w/o the BKL as well. The data which is copied from
> > the mtd data structure is either set up during device initialization
> > or statistics which have never been protected by the BKL against
> > concurrent modification. The mtd functions which are called from
> > various ioctl commands are safe as well.
> > 
> > Signed-off-by: Thomas Gleixner <tglx@linutronix.de>
> > Cc: David Woodhouse <dwmw2@infradead.org>
> > Cc: linux-mtd@lists.infradead.org
> > ---
> >  drivers/mtd/mtdchar.c |   30 ++++++++----------------------
> >  1 file changed, 8 insertions(+), 22 deletions(-)
> 
> [dedekind@eru l2-mtd-2.6.git]$ make -j8
>   CHK     include/linux/version.h
>   CHK     include/linux/utsrelease.h
>   SYMLINK include/asm -> include/asm-x86
>   CALL    scripts/checksyscalls.sh
>   CHK     include/linux/compile.h
>   CC [M]  drivers/mtd/mtdchar.o
> drivers/mtd/mtdchar.c: In function ‘mtd_compat_ioctl’:
> drivers/mtd/mtdchar.c:865: warning: passing argument 1 of ‘mtd_ioctl’ from incompatible pointer type
> drivers/mtd/mtdchar.c:444: note: expected ‘struct file *’ but argument is of type ‘struct inode *’
> drivers/mtd/mtdchar.c:865: warning: passing argument 2 of ‘mtd_ioctl’ makes integer from pointer without a cast
> drivers/mtd/mtdchar.c:444: note: expected ‘u_int’ but argument is of type ‘struct file *’
> drivers/mtd/mtdchar.c:865: error: too many arguments to function ‘mtd_ioctl’

Yeah, found and fixed that already.

      tglx

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

[Jean Delvare]

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
> command.
> 
> 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@linutronix.de>
> Cc: lm-sensors@lm-sensors.org
> ---
>  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 = {
>  		.options = WDIOF_KEEPALIVEPING | WDIOF_SETTIMEOUT |
> @@ -857,11 +857,13 @@ static int watchdog_ioctl(struct inode *
>  
>  	switch (cmd) {
>  	case WDIOC_GETSUPPORT:
> +		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);
>  		break;

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?

>  
>  	case WDIOC_GETSTATUS:
> @@ -921,7 +923,7 @@ static const struct file_operations watc
>  	.open = watchdog_open,
>  	.release = watchdog_release,
>  	.write = watchdog_write,
> -	.ioctl = watchdog_ioctl,
> +	.unlocked_ioctl = watchdog_ioctl,
>  };

-- 
Jean Delvare

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

[Jean Delvare]

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
> command.
> 
> 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@linutronix.de>
> Cc: lm-sensors@lm-sensors.org
> ---
>  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 = {
>  		.options = WDIOF_KEEPALIVEPING | WDIOF_SETTIMEOUT |
> @@ -857,11 +857,13 @@ static int watchdog_ioctl(struct inode *
>  
>  	switch (cmd) {
>  	case WDIOC_GETSUPPORT:
> +		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);
>  		break;

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?

>  
>  	case WDIOC_GETSTATUS:
> @@ -921,7 +923,7 @@ static const struct file_operations watc
>  	.open = watchdog_open,
>  	.release = watchdog_release,
>  	.write = watchdog_write,
> -	.ioctl = watchdog_ioctl,
> +	.unlocked_ioctl = watchdog_ioctl,
>  };

-- 
Jean Delvare

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

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

[Arnd Bergmann]

On Friday 06 November 2009, Jean Delvare wrote:
> 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?

The data copied into the watchdog_info data structure is completely
static. To make that clearer, I'd suggest moving it outside of
the ioctl function, and initializing it from fschmd_init.
Then it becomes obvious that the mutex is not needed to protect it.

	Arnd <><

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

[Arnd Bergmann]

On Friday 06 November 2009, Jean Delvare wrote:
> 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?

The data copied into the watchdog_info data structure is completely
static. To make that clearer, I'd suggest moving it outside of
the ioctl function, and initializing it from fschmd_init.
Then it becomes obvious that the mutex is not needed to protect it.

	Arnd <><

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

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

[Jean Delvare]

On Fri, 6 Nov 2009 14:22:49 +0100, Arnd Bergmann wrote:
> On Friday 06 November 2009, Jean Delvare wrote:
> > 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?
> 
> The data copied into the watchdog_info data structure is completely
> static. To make that clearer, I'd suggest moving it outside of
> the ioctl function, and initializing it from fschmd_init.
> Then it becomes obvious that the mutex is not needed to protect it.

Thinking about it some more... The driver supports the case where
several chips are present on the system. I don't think we've ever seen
this, but it is possible in theory. In this case, each chip would have
its data->revision, and then the watchdog_info data structure is no
longer static. We would need one watchdog_info data structure per
fschmd_data. Whether allocating and initializing them at device probing
time makes sense, I'm not so sure... I doubt this ioctl is
performance-critical, so generating the data on-the-fly sounds quite
sane to me.

-- 
Jean Delvare

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

[Jean Delvare]

On Fri, 6 Nov 2009 14:22:49 +0100, Arnd Bergmann wrote:
> On Friday 06 November 2009, Jean Delvare wrote:
> > 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?
> 
> The data copied into the watchdog_info data structure is completely
> static. To make that clearer, I'd suggest moving it outside of
> the ioctl function, and initializing it from fschmd_init.
> Then it becomes obvious that the mutex is not needed to protect it.

Thinking about it some more... The driver supports the case where
several chips are present on the system. I don't think we've ever seen
this, but it is possible in theory. In this case, each chip would have
its data->revision, and then the watchdog_info data structure is no
longer static. We would need one watchdog_info data structure per
fschmd_data. Whether allocating and initializing them at device probing
time makes sense, I'm not so sure... I doubt this ioctl is
performance-critical, so generating the data on-the-fly sounds quite
sane to me.

-- 
Jean Delvare

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

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

[Hans de Goede]

Hi,

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
>> command.
>>
>> 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@linutronix.de>
>> Cc: lm-sensors@lm-sensors.org
>> ---
>>   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 = {
>>   		.options = WDIOF_KEEPALIVEPING | WDIOF_SETTIMEOUT |
>> @@ -857,11 +857,13 @@ static int watchdog_ioctl(struct inode *
>>
>>   	switch (cmd) {
>>   	case WDIOC_GETSUPPORT:
>> +		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);
>>   		break;
>
> 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
here.

Regards,

Hans

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

[Hans de Goede]

Hi,

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
>> command.
>>
>> 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@linutronix.de>
>> Cc: lm-sensors@lm-sensors.org
>> ---
>>   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 = {
>>   		.options = WDIOF_KEEPALIVEPING | WDIOF_SETTIMEOUT |
>> @@ -857,11 +857,13 @@ static int watchdog_ioctl(struct inode *
>>
>>   	switch (cmd) {
>>   	case WDIOC_GETSUPPORT:
>> +		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);
>>   		break;
>
> 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
here.

Regards,

Hans

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

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

[Hans de Goede]

Hi,

On 11/06/2009 02:22 PM, Arnd Bergmann wrote:
> On Friday 06 November 2009, Jean Delvare wrote:
>> 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?
>
> The data copied into the watchdog_info data structure is completely
> static. To make that clearer, I'd suggest moving it outside of
> the ioctl function, and initializing it from fschmd_init.
> Then it becomes obvious that the mutex is not needed to protect it.
>

Moving the ident struct to the data structure would have it needlessly
using RAM all the time, the best and simplest fix for this is to simply
stop declaring ident static inside the ioctl function.

Regards,

Hans

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

[Hans de Goede]

Hi,

On 11/06/2009 02:22 PM, Arnd Bergmann wrote:
> On Friday 06 November 2009, Jean Delvare wrote:
>> 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?
>
> The data copied into the watchdog_info data structure is completely
> static. To make that clearer, I'd suggest moving it outside of
> the ioctl function, and initializing it from fschmd_init.
> Then it becomes obvious that the mutex is not needed to protect it.
>

Moving the ident struct to the data structure would have it needlessly
using RAM all the time, the best and simplest fix for this is to simply
stop declaring ident static inside the ioctl function.

Regards,

Hans

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

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

[Arnd Bergmann]

On Friday 06 November 2009, Hans de Goede wrote:
> Moving the ident struct to the data structure would have it needlessly
> using RAM all the time, the best and simplest fix for this is to simply
> stop declaring ident static inside the ioctl function.

Ok. Considering the case of multiple watchdogs in the system, that seems
best then. Of course, the watchdog infrastructure does not allow this
right now, but it may be added at some point.

	Arnd <><

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

[Arnd Bergmann]

On Friday 06 November 2009, Hans de Goede wrote:
> Moving the ident struct to the data structure would have it needlessly
> using RAM all the time, the best and simplest fix for this is to simply
> stop declaring ident static inside the ioctl function.

Ok. Considering the case of multiple watchdogs in the system, that seems
best then. Of course, the watchdog infrastructure does not allow this
right now, but it may be added at some point.

	Arnd <><

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

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

[Thomas Gleixner]

On Fri, 6 Nov 2009, Jean Delvare wrote:
> 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.

Right, we can change ident to be non static instead of abusing the
mutex. Stupid me. Will fix.

Thanks,

	tglx

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

[Thomas Gleixner]

On Fri, 6 Nov 2009, Jean Delvare wrote:
> 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.

Right, we can change ident to be non static instead of abusing the
mutex. Stupid me. Will fix.

Thanks,

	tglx

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors

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

[Jean Delvare]

Hi Thomas,

On Fri, 6 Nov 2009 16:56:34 +0100 (CET), Thomas Gleixner wrote:
> On Fri, 6 Nov 2009, Jean Delvare wrote:
> > 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.
> 
> Right, we can change ident to be non static instead of abusing the
> mutex. Stupid me. Will fix.

This was quite some time ago and I can't seem to find an updated version
of your patch that I could apply. Care to send one?

Thanks,
-- 
Jean Delvare

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

[Jean Delvare]

Hi Thomas,

On Fri, 6 Nov 2009 16:56:34 +0100 (CET), Thomas Gleixner wrote:
> On Fri, 6 Nov 2009, Jean Delvare wrote:
> > 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.
> 
> Right, we can change ident to be non static instead of abusing the
> mutex. Stupid me. Will fix.

This was quite some time ago and I can't seem to find an updated version
of your patch that I could apply. Care to send one?

Thanks,
-- 
Jean Delvare

_______________________________________________
lm-sensors mailing list
lm-sensors@lm-sensors.org
http://lists.lm-sensors.org/mailman/listinfo/lm-sensors