[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-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org> wrote:

> Hi Alan,
> 
> On Thu, 22 May 2008 22:23:27 +0100, Alan Cox wrote:
> > Signed-off-by: Alan Cox <alan-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> > 
> 
> 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.


_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c

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

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

[Jon Smirl]

On 5/23/08, Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org> wrote:
> Hi Alan,
>
>
>  On Thu, 22 May 2008 22:23:27 +0100, Alan Cox wrote:
>  > Signed-off-by: Alan Cox <alan-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
>  >
>
>
> 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 ;)


These BKL patches are part of a giant series of patches making the BLK
easier to remove.
There is a write up at LWN about the BKL.
http://lwn.net/Articles/281437/

Higher up in the kernel a global lock (BKL - big kernel lock) was
being taken before each IOCTL was called (and some other things). This
has the effect of serializing all IOCTL calls in the kernel. There is
no real need to serialize these IOCTLs in most cases.

Alan has removed the global lock/unlock code and now replicated it
across the 100s of IOCTLS in the kernel. This patch makes the
lock/unlock calls explicitly visible in the i2c code. He is trying to
remove this global lock since it hurts performance on SMP machines.

Now some programming needs to happen. The i2c ioctls need to be
inspected to see if they really need to be locked against multiple SMP
processors simultaneously entering them. There are multiple answers to
this question,  Do i2c IOCTLs need to be globally locked against all
other BLK users? Or do you only need a lock which is local to i2c? Or
is no lock needed at all? These question need to be answered for every
IOCTL in Linux.

i2c probably needs a lock per bus or controller to keep two SMP CPUs
from trying to program the controller simultaneously. Previously the
BKL had been serializing this.

So, the patch Alan supplied can be applied as is, it is just a
mechanically generated push down of the BKL. Applying it will continue
the same behavior as currently implemented in Linux. But Alan's patch
is probably not the optimal solution for i2c.


>
>  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
>
>
>  _______________________________________________
>  i2c mailing list
>  i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
>  http://lists.lm-sensors.org/mailman/listinfo/i2c
>


-- 
Jon Smirl
jonsmirl-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org

_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c

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

[Jean Delvare]

Hi Jon,

On Fri, 23 May 2008 10:23:46 -0400, Jon Smirl wrote:
> These BKL patches are part of a giant series of patches making the BLK
> easier to remove.
> There is a write up at LWN about the BKL.
> http://lwn.net/Articles/281437/

Thanks for the pointer, it all makes sense now. I indeed didn't know
that all ioctls were taking the BKL.

> Higher up in the kernel a global lock (BKL - big kernel lock) was
> being taken before each IOCTL was called (and some other things). This
> has the effect of serializing all IOCTL calls in the kernel. There is
> no real need to serialize these IOCTLs in most cases.
> 
> Alan has removed the global lock/unlock code and now replicated it
> across the 100s of IOCTLS in the kernel. This patch makes the
> lock/unlock calls explicitly visible in the i2c code. He is trying to
> remove this global lock since it hurts performance on SMP machines.
> 
> Now some programming needs to happen. The i2c ioctls need to be
> inspected to see if they really need to be locked against multiple SMP
> processors simultaneously entering them. There are multiple answers to
> this question,  Do i2c IOCTLs need to be globally locked against all
> other BLK users? Or do you only need a lock which is local to i2c? Or
> is no lock needed at all? These question need to be answered for every
> IOCTL in Linux.

My impression is that we don't need any locking at all. But I am no
locking expert, and it's easy to screw up that kind of things.

> i2c probably needs a lock per bus or controller to keep two SMP CPUs
> from trying to program the controller simultaneously. Previously the
> BKL had been serializing this.

This problem already exists (and is already solved) for the various I2C
chip drivers inside the kernel: two drivers with chips on the same I2C
bus might try to access the bus concurrently. We have a per-adapter
mutex to protect against this. I fail to see how user-space access
through i2c-dev would be fundamentally different. The only difference I
can think of is that i2c-dev can create two clients at the same address
while kernel drivers can't - but it doesn't seem to make a difference
as far as locking is concerned.

> So, the patch Alan supplied can be applied as is, it is just a
> mechanically generated push down of the BKL. Applying it will continue
> the same behavior as currently implemented in Linux. But Alan's patch
> is probably not the optimal solution for i2c.

OK... But my impression is that it is adding code which we will end up
removing quickly because it's simply not needed. So I wonder if we
shouldn't simply keep:

> >  > 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
> >  > (...)
> >  > @@ -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,
> >  >  };

And discard all the rest.

-- 
Jean Delvare

_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c

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

[Jon Smirl]

On 5/23/08, Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org> wrote:
> Hi Jon,
>
>
>  On Fri, 23 May 2008 10:23:46 -0400, Jon Smirl wrote:
>  > These BKL patches are part of a giant series of patches making the BLK
>  > easier to remove.
>  > There is a write up at LWN about the BKL.
>  > http://lwn.net/Articles/281437/
>
>
> Thanks for the pointer, it all makes sense now. I indeed didn't know
>  that all ioctls were taking the BKL.
>
>
>  > Higher up in the kernel a global lock (BKL - big kernel lock) was
>  > being taken before each IOCTL was called (and some other things). This
>  > has the effect of serializing all IOCTL calls in the kernel. There is
>  > no real need to serialize these IOCTLs in most cases.
>  >
>  > Alan has removed the global lock/unlock code and now replicated it
>  > across the 100s of IOCTLS in the kernel. This patch makes the
>  > lock/unlock calls explicitly visible in the i2c code. He is trying to
>  > remove this global lock since it hurts performance on SMP machines.
>  >
>  > Now some programming needs to happen. The i2c ioctls need to be
>  > inspected to see if they really need to be locked against multiple SMP
>  > processors simultaneously entering them. There are multiple answers to
>  > this question,  Do i2c IOCTLs need to be globally locked against all
>  > other BLK users? Or do you only need a lock which is local to i2c? Or
>  > is no lock needed at all? These question need to be answered for every
>  > IOCTL in Linux.
>
>
> My impression is that we don't need any locking at all. But I am no
>  locking expert, and it's easy to screw up that kind of things.
>
>
>  > i2c probably needs a lock per bus or controller to keep two SMP CPUs
>  > from trying to program the controller simultaneously. Previously the
>  > BKL had been serializing this.
>
>
> This problem already exists (and is already solved) for the various I2C
>  chip drivers inside the kernel: two drivers with chips on the same I2C
>  bus might try to access the bus concurrently. We have a per-adapter
>  mutex to protect against this. I fail to see how user-space access
>  through i2c-dev would be fundamentally different. The only difference I
>  can think of is that i2c-dev can create two clients at the same address
>  while kernel drivers can't - but it doesn't seem to make a difference
>  as far as locking is concerned.

This is the right way to look at the problem. The only code you need
to worry about is the kernel code. But also you need to assume that
most parts (not init/exit) of the i2c kernel code can get two SMP CPUs
in it unless you explicitly use locks to protect it.

Some things that need locks:
1) one user on a bus at a time. this lock should be in the bus driver
2) global lists, don't want two CPUs manipulating a list at the same time.
3) any other global variables

>
>
>  > So, the patch Alan supplied can be applied as is, it is just a
>  > mechanically generated push down of the BKL. Applying it will continue
>  > the same behavior as currently implemented in Linux. But Alan's patch
>  > is probably not the optimal solution for i2c.
>
>
> OK... But my impression is that it is adding code which we will end up
>  removing quickly because it's simply not needed. So I wonder if we
>  shouldn't simply keep:

Alan has just sent you the default solution. If you have a better
solution you can ignore his patch and use yours. He probably hopes
that every patch he is generating gets replaced with a more optimal
solution. He has sent out several hundred patches like this.

>  > >  > 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
>
> > >  > (...)
>
> > >  > @@ -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,
>  > >  >  };
>
>
> And discard all the rest.
>
>  --
>
> Jean Delvare
>


-- 
Jon Smirl
jonsmirl-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org

_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c

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

[Jean Delvare]

From: Alan Cox <alan-qBU/x9rampVanCEyBjwyrvXRex20P6io@public.gmane.org>

This is part of the effort to get rid of the BKL.

[JD: In fact i2c-dev doesn't need more locking than is already done
for the other i2c drivers, so we can simply switch to unlocked_ioctl.]

Signed-off-by: Alan Cox <alan-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
Signed-off-by: Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org>
---
 drivers/i2c/i2c-dev.c |    6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

--- linux-2.6.26-rc3.orig/drivers/i2c/i2c-dev.c	2008-05-04 09:49:48.000000000 +0200
+++ linux-2.6.26-rc3/drivers/i2c/i2c-dev.c	2008-05-24 08:33:29.000000000 +0200
@@ -366,8 +366,8 @@ static noinline int i2cdev_ioctl_smbus(s
 	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;
@@ -487,7 +487,7 @@ static const struct file_operations i2cd
 	.llseek		= no_llseek,
 	.read		= i2cdev_read,
 	.write		= i2cdev_write,
-	.ioctl		= i2cdev_ioctl,
+	.unlocked_ioctl	= i2cdev_ioctl,
 	.open		= i2cdev_open,
 	.release	= i2cdev_release,
 };


-- 
Jean Delvare

_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c

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

[Jon Smirl]

On 5/24/08, Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org> wrote:
> From: Alan Cox <alan-qBU/x9rampVanCEyBjwyrvXRex20P6io@public.gmane.org>
>
>  This is part of the effort to get rid of the BKL.
>
>  [JD: In fact i2c-dev doesn't need more locking than is already done
>  for the other i2c drivers, so we can simply switch to unlocked_ioctl.]

Note that the existing locking code in i2c (which may be correct
without the BLK) hasn't really been tested since the BKL was
serializing things before they got to the i2c code.

>  Signed-off-by: Alan Cox <alan-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
>  Signed-off-by: Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org>
>  ---
>   drivers/i2c/i2c-dev.c |    6 +++---
>   1 file changed, 3 insertions(+), 3 deletions(-)
>
>  --- linux-2.6.26-rc3.orig/drivers/i2c/i2c-dev.c 2008-05-04 09:49:48.000000000 +0200
>  +++ linux-2.6.26-rc3/drivers/i2c/i2c-dev.c      2008-05-24 08:33:29.000000000 +0200
>  @@ -366,8 +366,8 @@ static noinline int i2cdev_ioctl_smbus(s
>         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;
>  @@ -487,7 +487,7 @@ static const struct file_operations i2cd
>         .llseek         = no_llseek,
>         .read           = i2cdev_read,
>         .write          = i2cdev_write,
>  -       .ioctl          = i2cdev_ioctl,
>  +       .unlocked_ioctl = i2cdev_ioctl,
>         .open           = i2cdev_open,
>         .release        = i2cdev_release,
>   };
>
>
>
>  --
>  Jean Delvare
>
>  _______________________________________________
>  i2c mailing list
>  i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
>  http://lists.lm-sensors.org/mailman/listinfo/i2c
>


-- 
Jon Smirl
jonsmirl-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org

_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c

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

[Jean Delvare]

On Sat, 24 May 2008 10:50:48 -0400, Jon Smirl wrote:
> On 5/24/08, Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org> wrote:
> > From: Alan Cox <alan-qBU/x9rampVanCEyBjwyrvXRex20P6io@public.gmane.org>
> >
> >  This is part of the effort to get rid of the BKL.
> >
> >  [JD: In fact i2c-dev doesn't need more locking than is already done
> >  for the other i2c drivers, so we can simply switch to unlocked_ioctl.]
> 
> Note that the existing locking code in i2c (which may be correct
> without the BLK) hasn't really been tested since the BKL was
> serializing things before they got to the i2c code.

On the i2c-dev side, yes. But all other i2c chip drivers didn't get to
take the BKL, so the locking code in i2c (which definitely needs some
clean ups, but that's another story) has been tested by these and
i2c-dev isn't that different from them. That's why I believe no
particular locking is needed for i2c-dev. But of course if someone can
think of a reason why i2c-dev is different and needs additional care,
please speak up.

-- 
Jean Delvare

_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c