i2c: Push ioctl BKL down into the i2c code

Signed-off-by Alan Cox <[email protected]>

drivers/i2c/i2c-dev.c +4 -2

index d34c14c..16cf0f2 100644
@@ -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,

drivers/i2c/i2c-dev.c +9 -5

 				      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;

drivers/i2c/i2c-dev.c +12 -1

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

Quoted 2 lines · Alan Cox

Signed-off-by Alan Cox <[email protected]>

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

Quoted 3 lines · Jean Delvare

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

include/linux/fs.h +0 -0

@@ ?? @@ struct file_operations {

include/linux/fs.h +0 -1

 	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."

Quoted 9 lines · Stefan Richter

AFAIU it's a preparation for

include/linux/fs.h +0 -0

@@ ?? @@ struct file_operations {

include/linux/fs.h +0 -1

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

Quoted 2 lines

Obvious benefits:
- No new .ioctl()s.

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

Hi Alan,

Quoted 2 lines · Alan Cox

Signed-off-by Alan Cox <[email protected]>

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

Quoted 2 lines · On Fri, 23 May 2008 09:35:45 +0200 Jean Delvare <[email protected]> wrote:

Hi Alan,

Quoted 7 lines · Alan Cox

Quoted 2 lines

Signed-off-by Alan Cox <[email protected]>

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
[email protected]
http://lists.lm-sensors.org/mailman/listinfo/i2c

Hi Alan,

Quoted 2 lines · Alan Cox

Signed-off-by Alan Cox <[email protected]>

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.

Quoted 26 lines

drivers/i2c/i2c-dev.c +4 -2

index d34c14c..16cf0f2 100644
@@ -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,

drivers/i2c/i2c-dev.c static noinline int i2cdev_ioctl_smbus(struct i2c_client *client, · line 328

Quoted 9 lines

drivers/i2c/i2c-dev.c +3 -2

@@ -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.

Quoted 44 lines

drivers/i2c/i2c-dev.c +9 -5

                                    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;

drivers/i2c/i2c-dev.c static int i2cdev_ioctl(struct inode *inode, struct file *file, · line 409

Quoted 10 lines

drivers/i2c/i2c-dev.c +7 -3

-             /* 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?

Quoted 40 lines

drivers/i2c/i2c-dev.c +11 -0

+             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

drivers/i2c/i2c-dev.c static const struct file_operations i2cdev_fops = { · line 515

Quoted 8 lines

drivers/i2c/i2c-dev.c +1 -1

@@ -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
[email protected]
http://lists.lm-sensors.org/mailman/listinfo/i2c

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.

Quoted 6 lines

Quoted 6 lines

drivers/i2c/i2c-dev.c +0 -0

index d34c14c..16cf0f2 100644
(...)

@@ -487,7 +509,7 @@ static const struct file_operations i2cdev_fops = {

Quoted 8 lines

drivers/i2c/i2c-dev.c static const struct file_operations i2cdev_fops = { · line 515

Quoted 8 lines

drivers/i2c/i2c-dev.c +1 -1

      .llseek         = no_llseek,
      .read           = i2cdev_read,
      .write          = i2cdev_write,
-     .ioctl          = i2cdev_ioctl,
+     .unlocked_ioctl = i2cdev_ioctl,
      .open           = i2cdev_open,
      .release        = i2cdev_release,
 };

Hi Jon,

Quoted 4 lines · Jon Smirl

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.

Quoted 10 lines

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.

Quoted 7 lines

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.

Quoted 3 lines

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.

Quoted 6 lines

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:

Quoted 7 lines

Quoted 6 lines

Quoted 6 lines

drivers/i2c/i2c-dev.c +0 -0

index d34c14c..16cf0f2 100644
 
(...)

Quoted 9 lines

Quoted 9 lines

drivers/i2c/i2c-dev.c static const struct file_operations i2cdev_fops = { · line 515

Quoted 9 lines

drivers/i2c/i2c-dev.c +1 -1

@@ -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