Re: [PATCH] ds1620: Push BKL down

[Russell King <rmk+lkml@arm.linux.org.uk>]

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: