[PATCH] ds1620: Push BKL down

[PATCH] ds1620: Push BKL down

[Alan Cox]

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

Signed-off-by: Alan Cox <alan@redhat.com>

diff --git a/drivers/char/ds1620.c b/drivers/char/ds1620.c
index 334ad5b..6db699d 100644
--- a/drivers/char/ds1620.c
+++ b/drivers/char/ds1620.c
@@ -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 = {

Re: [PATCH] ds1620: Push BKL down

[Alan Cox]

On Tue, 8 Jul 2008 21:08:42 +0100
Russell King <rmk+lkml@arm.linux.org.uk> wrote:

> On Thu, May 22, 2008 at 09:35:29PM +0100, Alan Cox wrote:
> > 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

Re: [PATCH] ds1620: Push BKL down

[Russell King]

On Thu, May 22, 2008 at 09:35:29PM +0100, Alan Cox wrote:
> 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.

> Signed-off-by: Alan Cox <alan@redhat.com>
> 
> diff --git a/drivers/char/ds1620.c b/drivers/char/ds1620.c
> index 334ad5b..6db699d 100644
> --- a/drivers/char/ds1620.c
> +++ b/drivers/char/ds1620.c
> @@ -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();

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.)

>  
>  		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();

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

>  		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.

>  
>  		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.

>  
>  		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.

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