Re: [patch 23/28] i2c: Remove big kernel lock from i2cdev_open

[Jean Delvare]

Hi Thomas,

On Sat, 10 Oct 2009 15:37:17 -0000, Thomas Gleixner wrote:
> The BKL is held over a kmalloc so cannot protect anything beyond that.
> The two calls before the kmalloc have their own locking.
> Improve device open function by removing the now unnecessary ret variable
> 
> Signed-off-by: Vincent Sanders <vince@xxxxxxxxxxxx>
> LKML-Reference: <1255175172-2666-1-git-send-email-vince@xxxxxxxxxxxx>
> Cc: Jean Delvare <khali@xxxxxxxxxxxx>
> Signed-off-by: Thomas Gleixner <tglx@xxxxxxxxxxxxx>
> ---
>  drivers/i2c/i2c-dev.c |   22 ++++++----------------
>  1 file changed, 6 insertions(+), 16 deletions(-)
> 
> Index: linux-2.6-tip/drivers/i2c/i2c-dev.c
> ===================================================================
> --- linux-2.6-tip.orig/drivers/i2c/i2c-dev.c
> +++ linux-2.6-tip/drivers/i2c/i2c-dev.c
> @@ -34,7 +34,6 @@
>  #include <linux/list.h>
>  #include <linux/i2c.h>
>  #include <linux/i2c-dev.h>
> -#include <linux/smp_lock.h>
>  #include <linux/jiffies.h>
>  #include <asm/uaccess.h>
>  
> @@ -445,20 +444,14 @@ static int i2cdev_open(struct inode *ino
>  	struct i2c_client *client;
>  	struct i2c_adapter *adap;
>  	struct i2c_dev *i2c_dev;
> -	int ret = 0;
>  
> -	lock_kernel();
>  	i2c_dev = i2c_dev_get_by_minor(minor);
> -	if (!i2c_dev) {
> -		ret = -ENODEV;
> -		goto out;
> -	}
> +	if (!i2c_dev)
> +		return -ENODEV;
>  
>  	adap = i2c_get_adapter(i2c_dev->adap->nr);
> -	if (!adap) {
> -		ret = -ENODEV;
> -		goto out;
> -	}
> +	if (!adap)
> +		return -ENODEV;
>  
>  	/* This creates an anonymous i2c_client, which may later be
>  	 * pointed to some address using I2C_SLAVE or I2C_SLAVE_FORCE.
> @@ -470,8 +463,7 @@ static int i2cdev_open(struct inode *ino
>  	client = kzalloc(sizeof(*client), GFP_KERNEL);
>  	if (!client) {
>  		i2c_put_adapter(adap);
> -		ret = -ENOMEM;
> -		goto out;
> +		return -ENOMEM;
>  	}
>  	snprintf(client->name, I2C_NAME_SIZE, "i2c-dev %d", adap->nr);
>  	client->driver = &i2cdev_driver;
> @@ -479,9 +471,7 @@ static int i2cdev_open(struct inode *ino
>  	client->adapter = adap;
>  	file->private_data = client;
>  
> -out:
> -	unlock_kernel();
> -	return ret;
> +	return 0;
>  }
>  
>  static int i2cdev_release(struct inode *inode, struct file *file)
> 

Looks good to me:

Acked-by: Jean Delvare <khali@xxxxxxxxxxxx>

Do you want me to pick this patch in my i2c tree, or will you take care
of pushing it upstream? If you're not going to push it quickly, I'd
prefer the first solution, to avoid conflicts.

(As a side note, i2c-dev would deserve a partial rewrite as it doesn't
integrate into the standard device driver model, and certainly has a
number of race conditions. But this is well beyond the scope of your
patch.)

Thanks,
-- 
Jean Delvare
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/