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

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

[Alan Cox]

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

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,
 				      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;
+		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,
 };

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

[Jean Delvare]

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

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

[Stefan Richter]

Jean Delvare wrote:
> 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 ;)

I wasn't asked, but:

The patch description was factored out.  ;-)
http://lkml.org/lkml/2008/5/22/333

AFAIU it's a preparation for

--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ ?? @@ struct file_operations {
 	unsigned int (*poll) (struct file *, struct poll_table_struct *);
-	int (*ioctl) (struct inode *, struct file *, unsigned int, unsigned long);
 	long (*unlocked_ioctl) (struct file *, unsigned int, unsigned long);

Obvious benefits:
  - No new .ioctl()s.
  - Heads up for subsystem people:  "Did you know you are taking the BKL?
    You probably don't need to, and you definitely don't want to."
-- 
Stefan Richter
-=====-==--- -=-= =-===
http://arcgraph.de/sr/

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

[Jean Delvare]

Hi Stefan,

On Fri, 23 May 2008 10:46:30 +0200, Stefan Richter wrote:
> Jean Delvare wrote:
> > 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 ;)
> 
> I wasn't asked, but:
> 
> The patch description was factored out.  ;-)
> http://lkml.org/lkml/2008/5/22/333

Hardly fits as a proper description for the git commit... But thanks
for the pointer.

> AFAIU it's a preparation for
> 
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ ?? @@ struct file_operations {
>  	unsigned int (*poll) (struct file *, struct poll_table_struct *);
> -	int (*ioctl) (struct inode *, struct file *, unsigned int, unsigned long);
>  	long (*unlocked_ioctl) (struct file *, unsigned int, unsigned long);
> 
> Obvious benefits:
>   - No new .ioctl()s.

I fail to see how this is related to the locking change.

>   - Heads up for subsystem people:  "Did you know you are taking the BKL?
>     You probably don't need to, and you definitely don't want to."

Good one... I admit that I didn't know.

-- 
Jean Delvare

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

[Stefan Richter]

Jean Delvare wrote:
> On Fri, 23 May 2008 10:46:30 +0200, Stefan Richter wrote:
>> AFAIU it's a preparation for
>> 
>> --- a/include/linux/fs.h
>> +++ b/include/linux/fs.h
>> @@ ?? @@ struct file_operations {
>>  	unsigned int (*poll) (struct file *, struct poll_table_struct *);
>> -	int (*ioctl) (struct inode *, struct file *, unsigned int, unsigned long);
>>  	long (*unlocked_ioctl) (struct file *, unsigned int, unsigned long);
>>
>> Obvious benefits:
>>   - No new .ioctl()s.
> 
> I fail to see how this is related to the locking change.

I meant:  After struct file_operations.ioctl is removed, developers can 
only implement unlocked_ioctl methods.  This includes code that is not 
(yet) in mainline.  To keep it compilable, it has to be converted to 
unlocked_ioctl.

[The quickest kind of conversion is to replace the ioctl method by an 
unlocked_ioctl method which itself takes the BKL.  This kind of 
conversion has of course no direct benefit for the functioning of the 
kernel, but as mentioned it at least puts the BKL issue into the line of 
sight of the people who work with the code.]
-- 
Stefan Richter
-=====-==--- -=-= =-===
http://arcgraph.de/sr/

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

[Alan Cox]

On Fri, 23 May 2008 09:35:45 +0200
Jean Delvare <khali@linux-fr.org> wrote:

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

It pushes the BKL down into the i2c driver. The intention is to remove
the big kernel lock ioctl method from the file_operations structure so
that we can work on getting rid of the big kernel lock for good. It's one
of a series of patches that give me an x86-32 tree with no ->ioctl method
at all.

Similar activity is going on for the other calls made under the BKL the
goal being to push it down into drivers and then eliminate it.

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

[Jon Smirl]

On 5/23/08, Alan Cox <alan@lxorguk.ukuu.org.uk> wrote:
> On Fri, 23 May 2008 09:35:45 +0200
>  Jean Delvare <khali@linux-fr.org> wrote:
>
>  > 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 ;)


Alan, for people not familiar with the BKL attaching a write up or
pointer to the patches on the recommended ways to convert these locks
to something else would help. Some embedded developers won't know what
to do with these patches and they don't follow lkml.


> It pushes the BKL down into the i2c driver. The intention is to remove
>  the big kernel lock ioctl method from the file_operations structure so
>  that we can work on getting rid of the big kernel lock for good. It's one
>  of a series of patches that give me an x86-32 tree with no ->ioctl method
>  at all.
>
>  Similar activity is going on for the other calls made under the BKL the
>  goal being to push it down into drivers and then eliminate it.
>
>
>
>  _______________________________________________
>  i2c mailing list
>  i2c@lm-sensors.org
>  http://lists.lm-sensors.org/mailman/listinfo/i2c
>


-- 
Jon Smirl
jonsmirl@gmail.com