ds1620: Push BKL down

PATCH In review Alan Cox 2 participants 3 messages May 22, 2008 – Jul 8, 2008 Char and misc drivers

lore

Share & tools

lore

Alan Cox

author

2008-05-22 20:48 UTC #0 lore raw

PATCH 1/1 ds1620: Push BKL down

Push down the BKL and correct the ioctl return for unknown ioctls.

Signed-off-by Alan Cox <[email protected]>

drivers/char/ds1620.c +18 -9

index 334ad5b..6db699d 100644
@@ -8,10 +8,11 @@
 #include <linux/proc_fs.h>
 #include <linux/capability.h>
 #include <linux/init.h>
+#include <linux/smp_lock.h>
+#include <linux/uaccess.h>
 
 #include <asm/hardware.h>
 #include <asm/mach-types.h>
-#include <asm/uaccess.h>
 #include <asm/therm.h>
 
 #ifdef CONFIG_PROC_FS

@@ -225,8 +226,8 @@ ds1620_read(struct file *file, char __user *buf, size_t count, loff_t *ptr)
 	return 1;
 }
 
-static int
-ds1620_ioctl(struct inode *inode, struct file *file, unsigned int cmd, unsigned long arg)
+static long ds1620_ioctl(struct file *file, unsigned int cmd,
+							unsigned long arg)
 {
 	struct therm therm;
 	union {

@@ -254,13 +255,16 @@ ds1620_ioctl(struct inode *inode, struct file *file, unsigned int cmd, unsigned
 
 		therm.lo <<= 1;
 		therm.hi <<= 1;
-
+		lock_kernel();
 		ds1620_write_state(&therm);
+		unlock_kernel();
 		break;
 
 	case CMD_GET_THERMOSTATE:
 	case CMD_GET_THERMOSTATE2:
+		lock_kernel();
 		ds1620_read_state(&therm);
+		unlock_kernel();
 
 		therm.lo >>= 1;
 		therm.hi >>= 1;

@@ -276,20 +280,25 @@ ds1620_ioctl(struct inode *inode, struct file *file, unsigned int cmd, unsigned
 
 	case CMD_GET_TEMPERATURE:
 	case CMD_GET_TEMPERATURE2:
+		lock_kernel();
 		i = cvt_9_to_int(ds1620_in(THERM_READ_TEMP, 9));
 
 		if (cmd == CMD_GET_TEMPERATURE)
 			i >>= 1;
-
+		unlock_kernel();
 		return put_user(i, uarg.i) ? -EFAULT : 0;
 
 	case CMD_GET_STATUS:
+		lock_kernel();
 		i = ds1620_in(THERM_READ_CONFIG, 8) & 0xe3;
+		unlock_kernel();
 
 		return put_user(i, uarg.i) ? -EFAULT : 0;
 
 	case CMD_GET_FAN:
+		lock_kernel();
 		i = netwinder_get_fan();
+		unnlock_kernel();
 
 		return put_user(i, uarg.i) ? -EFAULT : 0;

@@ -299,14 +308,14 @@ ds1620_ioctl(struct inode *inode, struct file *file, unsigned int cmd, unsigned
 
 		if (get_user(i, uarg.i))
 			return -EFAULT;
-
+		lock_kernel();
 		netwinder_set_fan(i);
+		unlock_kernel();
 		break;
 		
 	default:
-		return -ENOIOCTLCMD;
+		return -ENOTTY;
 	}
-
 	return 0;
 }

@@ -338,7 +347,7 @@ static const struct file_operations ds1620_fops = {
 	.owner		= THIS_MODULE,
 	.open		= nonseekable_open,
 	.read		= ds1620_read,
-	.ioctl		= ds1620_ioctl,
+	.unlocked_ioctl	= ds1620_ioctl,
 };
 
 static struct miscdevice ds1620_miscdev = {

Russell King

2008-07-08 20:09 UTC #1 lore raw

Quoted 1 line · Alan Cox

Push down the BKL and correct the ioctl return for unknown ioctls.

In principle, no issues with the patch, and I'd ack it. However, I've made some comments about whether the BKL is needed in all these places. Whether your intention is just to push the BKL down or to actually eliminate it will of course determine whether you want to delete those.

Quoted 35 lines

Signed-off-by Alan Cox <[email protected]>

drivers/char/ds1620.c +4 -4

index 334ad5b..6db699d 100644
@@ -8,10 +8,11 @@
 #include <linux/proc_fs.h>
 #include <linux/capability.h>
 #include <linux/init.h>
+#include <linux/smp_lock.h>
+#include <linux/uaccess.h>
 
 #include <asm/hardware.h>
 #include <asm/mach-types.h>
-#include <asm/uaccess.h>
 #include <asm/therm.h>
 
 #ifdef CONFIG_PROC_FS

@@ -225,8 +226,8 @@ ds1620_read(struct file *file, char __user *buf, size_t count, loff_t *ptr)
 	return 1;
 }
 
-static int
-ds1620_ioctl(struct inode *inode, struct file *file, unsigned int cmd, unsigned long arg)
+static long ds1620_ioctl(struct file *file, unsigned int cmd,
+							unsigned long arg)
 {
 	struct therm therm;
 	union {

@@ -254,13 +255,16 @@ ds1620_ioctl(struct inode *inode, struct file *file, unsigned int cmd, unsigned
 
 		therm.lo <<= 1;
 		therm.hi <<= 1;
-

drivers/char/ds1620.c ds1620_ioctl(struct inode *inode, struct file *file, unsigned int cmd, unsigned · line 267

Quoted 10 lines

drivers/char/ds1620.c +4 -0

+		lock_kernel();
 		ds1620_write_state(&therm);
+		unlock_kernel();
 		break;
 
 	case CMD_GET_THERMOSTATE:
 	case CMD_GET_THERMOSTATE2:
+		lock_kernel();
 		ds1620_read_state(&therm);
+		unlock_kernel();

These two should stay since there's no other protection against reading half the state in one thread while another writes the state (where state is the high/low trip points for the fan speed.)

Quoted 4 lines

drivers/char/ds1620.c +0 -0

 		therm.lo >>= 1;
 		therm.hi >>= 1;

drivers/char/ds1620.c ds1620_ioctl(struct inode *inode, struct file *file, unsigned int cmd, unsigned · line 288

Quoted 10 lines

drivers/char/ds1620.c +2 -1

@@ -276,20 +280,25 @@ ds1620_ioctl(struct inode *inode, struct file *file, unsigned int cmd, unsigned
 
 	case CMD_GET_TEMPERATURE:
 	case CMD_GET_TEMPERATURE2:
+		lock_kernel();
 		i = cvt_9_to_int(ds1620_in(THERM_READ_TEMP, 9));
 
 		if (cmd == CMD_GET_TEMPERATURE)
 			i >>= 1;
-
+		unlock_kernel();

ds1620_in() is protected by a spinlock internally, and cvt_9_to_int is just a conversion function, so not required.

drivers/char/ds1620.c ds1620_ioctl(struct inode *inode, struct file *file, unsigned int cmd, unsigned · line 294

Quoted 6 lines

drivers/char/ds1620.c +2 -0

 		return put_user(i, uarg.i) ? -EFAULT : 0;
 
 	case CMD_GET_STATUS:
+		lock_kernel();
 		i = ds1620_in(THERM_READ_CONFIG, 8) & 0xe3;
+		unlock_kernel();

ds1620_in() is protected by a spinlock internally, so not required.

drivers/char/ds1620.c ds1620_ioctl(struct inode *inode, struct file *file, unsigned int cmd, unsigned · line 301

Quoted 7 lines

drivers/char/ds1620.c +2 -0

 		return put_user(i, uarg.i) ? -EFAULT : 0;
 
 	case CMD_GET_FAN:
+		lock_kernel();
 		i = netwinder_get_fan();
+		unnlock_kernel();

Not required - netwinder_get_fan() essentially just reads a pair of GPIO registers and reports the state of the GPIO.

drivers/char/ds1620.c ds1620_ioctl(struct inode *inode, struct file *file, unsigned int cmd, unsigned · line 313

Quoted 11 lines

drivers/char/ds1620.c +2 -1

 		return put_user(i, uarg.i) ? -EFAULT : 0;

@@ -299,14 +308,14 @@ ds1620_ioctl(struct inode *inode, struct file *file, unsigned int cmd, unsigned
 
 		if (get_user(i, uarg.i))
 			return -EFAULT;
-
+		lock_kernel();
 		netwinder_set_fan(i);
+		unlock_kernel();

Not needed - the internals of netwinder_set_fan() takes a spinlock.

Signature · 3 lines

Russell King
 Linux kernel    2.6 ARM Linux   - http://www.arm.linux.org.uk/
 maintainer of:

Alan Cox

author

2008-07-08 20:28 UTC #2 lore raw

Quoted 7 lines · On Tue, 8 Jul 2008 21:08:42 +0100 Russell King <[email protected]>…

Quoted 1 line · Alan Cox
Push down the BKL and correct the ioctl return for unknown ioctls.
In principle, no issues with the patch, and I'd ack it. However, I've made some comments about whether the BKL is needed in all these places. Whether your intention is just to push the BKL down or to actually eliminate it will of course determine whether you want to delete those.

My intention is to keep pushing until it becomes another maintainers problem to push further. I'll revamp this diff when I have a bit of time to get back around to it and I'll push down/remove the lock further in accordance with your comments.

Alan