Re: [PATCH] i2c: Push ioctl BKL down into the i2c code

[Jean Delvare <khali@linux-fr.org>]

Hi Alan,

On Thu, 22 May 2008 22:23:27 +0100, Alan Cox wrote:
> Signed-off-by: Alan Cox <alan@redhat.com>
> 

Description of what the patch does and why it is needed, please. I
can't apply it without that. My first impression is a patch making the
code bigger and more complex with no obvious benefit ;)

Bonus points for a diffstat of the patch.

> diff --git a/drivers/i2c/i2c-dev.c b/drivers/i2c/i2c-dev.c
> index d34c14c..16cf0f2 100644
> --- a/drivers/i2c/i2c-dev.c
> +++ b/drivers/i2c/i2c-dev.c
> @@ -34,7 +34,8 @@
>  #include <linux/list.h>
>  #include <linux/i2c.h>
>  #include <linux/i2c-dev.h>
> -#include <asm/uaccess.h>
> +#include <linux/smp_lock.h>
> +#include <linux/uaccess.h>
>  
>  static struct i2c_driver i2cdev_driver;
>  
> @@ -266,8 +267,9 @@ static noinline int i2cdev_ioctl_rdrw(struct i2c_client *client,
>  		kfree(rdwr_pa);
>  		return res;
>  	}
> -
> +	lock_kernel();
>  	res = i2c_transfer(client->adapter, rdwr_pa, rdwr_arg.nmsgs);
> +	unlock_kernel();
>  	while (i-- > 0) {
>  		if (res >= 0 && (rdwr_pa[i].flags & I2C_M_RD)) {
>  			if (copy_to_user(data_ptrs[i], rdwr_pa[i].buf,
> @@ -320,11 +322,15 @@ static noinline int i2cdev_ioctl_smbus(struct i2c_client *client,
>  
>  	if ((data_arg.size == I2C_SMBUS_QUICK) ||
>  	    ((data_arg.size == I2C_SMBUS_BYTE) &&
> -	    (data_arg.read_write == I2C_SMBUS_WRITE)))
> +	    (data_arg.read_write == I2C_SMBUS_WRITE))) {
>  		/* These are special: we do not use data */
> -		return i2c_smbus_xfer(client->adapter, client->addr,
> +		lock_kernel();
> +		res =i2c_smbus_xfer(client->adapter, client->addr,

Coding style.

>  				      client->flags, data_arg.read_write,
>  				      data_arg.command, data_arg.size, NULL);
> +		unlock_kernel();
> +		return res;
> +	}
>  
>  	if (data_arg.data == NULL) {
>  		dev_dbg(&client->adapter->dev,
> @@ -355,8 +361,10 @@ static noinline int i2cdev_ioctl_smbus(struct i2c_client *client,
>  		if (data_arg.read_write == I2C_SMBUS_READ)
>  			temp.block[0] = I2C_SMBUS_BLOCK_MAX;
>  	}
> +	lock_kernel();
>  	res = i2c_smbus_xfer(client->adapter, client->addr, client->flags,
>  	      data_arg.read_write, data_arg.command, data_arg.size, &temp);
> +	unlock_kernel();
>  	if (!res && ((data_arg.size == I2C_SMBUS_PROC_CALL) ||
>  		     (data_arg.size == I2C_SMBUS_BLOCK_PROC_CALL) ||
>  		     (data_arg.read_write == I2C_SMBUS_READ))) {
> @@ -366,11 +374,12 @@ static noinline int i2cdev_ioctl_smbus(struct i2c_client *client,
>  	return res;
>  }
>  
> -static int i2cdev_ioctl(struct inode *inode, struct file *file,
> -		unsigned int cmd, unsigned long arg)
> +static long i2cdev_ioctl(struct file *file, unsigned int cmd,
> +						unsigned long arg)
>  {
>  	struct i2c_client *client = (struct i2c_client *)file->private_data;
>  	unsigned long funcs;
> +	int ret = 0;
>  
>  	dev_dbg(&client->adapter->dev, "ioctl, cmd=0x%02x, arg=0x%02lx\n",
>  		cmd, arg);
> @@ -388,28 +397,37 @@ static int i2cdev_ioctl(struct inode *inode, struct file *file,
>  		 * the PEC flag already set, the i2c-dev driver won't see
>  		 * (or use) this setting.
>  		 */
> +		lock_kernel();
>  		if ((arg > 0x3ff) ||
>  		    (((client->flags & I2C_M_TEN) == 0) && arg > 0x7f))
> -			return -EINVAL;
> -		if (cmd == I2C_SLAVE && i2cdev_check_addr(client->adapter, arg))
> -			return -EBUSY;
> -		/* REVISIT: address could become busy later */
> -		client->addr = arg;
> -		return 0;
> +			ret = -EINVAL;
> +		else if (cmd == I2C_SLAVE &&
> +				i2cdev_check_addr(client->adapter, arg))
> +			ret = -EBUSY;
> +		else
> +			/* REVISIT: address could become busy later */
> +			client->addr = arg;

No unlock?

> +		return ret;
>  	case I2C_TENBIT:
> +		lock_kernel();
>  		if (arg)
>  			client->flags |= I2C_M_TEN;
>  		else
>  			client->flags &= ~I2C_M_TEN;
> +		unlock_kernel();
>  		return 0;
>  	case I2C_PEC:
> +		lock_kernel();
>  		if (arg)
>  			client->flags |= I2C_CLIENT_PEC;
>  		else
>  			client->flags &= ~I2C_CLIENT_PEC;
> +		unlock_kernel();
>  		return 0;
>  	case I2C_FUNCS:
> +		lock_kernel();
>  		funcs = i2c_get_functionality(client->adapter);
> +		unlock_kernel();
>  		return put_user(funcs, (unsigned long __user *)arg);
>  
>  	case I2C_RDWR:
> @@ -419,10 +437,14 @@ static int i2cdev_ioctl(struct inode *inode, struct file *file,
>  		return i2cdev_ioctl_smbus(client, arg);
>  
>  	case I2C_RETRIES:
> +		lock_kernel();
>  		client->adapter->retries = arg;
> +		unlock_kernel();
>  		break;
>  	case I2C_TIMEOUT:
> +		lock_kernel();
>  		client->adapter->timeout = arg;
> +		unlock_kernel();
>  		break;
>  	default:
>  		/* NOTE:  returning a fault code here could cause trouble
> @@ -487,7 +509,7 @@ static const struct file_operations i2cdev_fops = {
>  	.llseek		= no_llseek,
>  	.read		= i2cdev_read,
>  	.write		= i2cdev_write,
> -	.ioctl		= i2cdev_ioctl,
> +	.unlocked_ioctl	= i2cdev_ioctl,
>  	.open		= i2cdev_open,
>  	.release	= i2cdev_release,
>  };


-- 
Jean Delvare