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 <[email protected]>
>
> 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:
On Tue, 8 Jul 2008 21:08:42 +0100
Russell King <[email protected]> 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