[PATCH 0/2] i2c-dev: Don't let userspace block adapter

[PATCH 0/2] i2c-dev: Don't let userspace block adapter

[Viresh Kumar]

Hi Wolfram/Jean,

I am part of the kernel team for Google's projectara [1], where we are
building a module smart phone.

This series tries to fix one of the problems we hit on our system as we
are required to hotplug pretty much every thing on the phone and so this
fixes hotplug issues with i2c-dev.

As described in the second patch, the current implementation of i2c-dev
file operations doesn't let the modules (hardware attached to the phone)
eject from the phone as the cleanup path for the module hasn't finished
yet (i2c adapter not removed).

We can't let the userspace block the kernel devices forever in such
cases.

I was able to test them on the ARA phone with kernel 3.10 only and not
mainline.

--
viresh

[1] https://atap.google.com/ara/

Viresh Kumar (2):
  i2c-dev: don't get i2c adapter via i2c_dev
  i2c-dev: Don't block the adapter from unregistering

 drivers/i2c/i2c-dev.c | 79 ++++++++++++++++++++++++++++++++++++++++++---------
 include/linux/i2c.h   |  1 +
 2 files changed, 67 insertions(+), 13 deletions(-)

-- 
2.7.4

[PATCH 1/2] i2c-dev: don't get i2c adapter via i2c_dev

[Viresh Kumar]

There is no code protecting i2c_dev to be freed after it is returned
from i2c_dev_get_by_minor() and using it to access the value which we
already have (minor) isn't safe really.

Avoid using it and get the adapter directly from 'minor'.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/i2c/i2c-dev.c | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/drivers/i2c/i2c-dev.c b/drivers/i2c/i2c-dev.c
index 6ecfd76270f2..66f323fd3982 100644
--- a/drivers/i2c/i2c-dev.c
+++ b/drivers/i2c/i2c-dev.c
@@ -485,13 +485,8 @@ static int i2cdev_open(struct inode *inode, struct file *file)
 	unsigned int minor = iminor(inode);
 	struct i2c_client *client;
 	struct i2c_adapter *adap;
-	struct i2c_dev *i2c_dev;
-
-	i2c_dev = i2c_dev_get_by_minor(minor);
-	if (!i2c_dev)
-		return -ENODEV;
 
-	adap = i2c_get_adapter(i2c_dev->adap->nr);
+	adap = i2c_get_adapter(minor);
 	if (!adap)
 		return -ENODEV;
 
-- 
2.7.4

Re: [PATCH 1/2] i2c-dev: don't get i2c adapter via i2c_dev

[Jean Delvare]

Hi Viresh,

A bit of background: at some point in time, the i2c adapter number (as
represented internally by the kernel) could be different from the i2c
device node number (as seen by user-space.) I put an end to this
madness years ago, but it seems some legacy code from that time
survived in i2c-dev.

On Tue,  5 Jul 2016 19:57:06 -0700, Viresh Kumar wrote:
> There is no code protecting i2c_dev to be freed after it is returned
> from i2c_dev_get_by_minor() and using it to access the value which we
> already have (minor) isn't safe really.

I agree that i2c_dev_get_by_minor() looks racy by nature. It is
possible that i2c_dev_get_by_minor() can be removed altogether. There
are 2 other calling locations beyond the one you want to remove. If one
can be removed then I suspect others can be removed as well (maybe with
some more work though.)

If i2c_dev_get_by_minor() needs to stay for whatever reason, then my
next worry is that struct i2c_dev carries an unaccounted reference to
an i2c_adapter. This looks seriously broken. i2c_dev->adap should only
be set on open, and cleared on close.

> Avoid using it and get the adapter directly from 'minor'.
> 
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---
>  drivers/i2c/i2c-dev.c | 7 +------
>  1 file changed, 1 insertion(+), 6 deletions(-)
> 
> diff --git a/drivers/i2c/i2c-dev.c b/drivers/i2c/i2c-dev.c
> index 6ecfd76270f2..66f323fd3982 100644
> --- a/drivers/i2c/i2c-dev.c
> +++ b/drivers/i2c/i2c-dev.c
> @@ -485,13 +485,8 @@ static int i2cdev_open(struct inode *inode, struct file *file)
>  	unsigned int minor = iminor(inode);
>  	struct i2c_client *client;
>  	struct i2c_adapter *adap;
> -	struct i2c_dev *i2c_dev;
> -
> -	i2c_dev = i2c_dev_get_by_minor(minor);
> -	if (!i2c_dev)
> -		return -ENODEV;
>  
> -	adap = i2c_get_adapter(i2c_dev->adap->nr);
> +	adap = i2c_get_adapter(minor);
>  	if (!adap)
>  		return -ENODEV;
>  

This is the most simple fix to your immediate problem. However it
doesn't address the big design issue.

-- 
Jean Delvare
SUSE L3 Support

Re: [PATCH 1/2] i2c-dev: don't get i2c adapter via i2c_dev

[Viresh Kumar]

Hi Jean,

Thanks for the explanation.

On 06-07-16, 19:04, Jean Delvare wrote:
> Hi Viresh,
> 
> A bit of background: at some point in time, the i2c adapter number (as
> represented internally by the kernel) could be different from the i2c
> device node number (as seen by user-space.) I put an end to this
> madness years ago, but it seems some legacy code from that time
> survived in i2c-dev.
> 
> On Tue,  5 Jul 2016 19:57:06 -0700, Viresh Kumar wrote:
> > There is no code protecting i2c_dev to be freed after it is returned
> > from i2c_dev_get_by_minor() and using it to access the value which we
> > already have (minor) isn't safe really.
> 
> I agree that i2c_dev_get_by_minor() looks racy by nature. It is
> possible that i2c_dev_get_by_minor() can be removed altogether. There
> are 2 other calling locations beyond the one you want to remove. If one
> can be removed then I suspect others can be removed as well (maybe with
> some more work though.)
> 
> If i2c_dev_get_by_minor() needs to stay for whatever reason, then my
> next worry is that struct i2c_dev carries an unaccounted reference to
> an i2c_adapter. This looks seriously broken. i2c_dev->adap should only
> be set on open, and cleared on close.
> 
> > Avoid using it and get the adapter directly from 'minor'.
> > 
> > Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> > ---
> >  drivers/i2c/i2c-dev.c | 7 +------
> >  1 file changed, 1 insertion(+), 6 deletions(-)
> > 
> > diff --git a/drivers/i2c/i2c-dev.c b/drivers/i2c/i2c-dev.c
> > index 6ecfd76270f2..66f323fd3982 100644
> > --- a/drivers/i2c/i2c-dev.c
> > +++ b/drivers/i2c/i2c-dev.c
> > @@ -485,13 +485,8 @@ static int i2cdev_open(struct inode *inode, struct file *file)
> >  	unsigned int minor = iminor(inode);
> >  	struct i2c_client *client;
> >  	struct i2c_adapter *adap;
> > -	struct i2c_dev *i2c_dev;
> > -
> > -	i2c_dev = i2c_dev_get_by_minor(minor);
> > -	if (!i2c_dev)
> > -		return -ENODEV;
> >  
> > -	adap = i2c_get_adapter(i2c_dev->adap->nr);
> > +	adap = i2c_get_adapter(minor);
> >  	if (!adap)
> >  		return -ENODEV;
> >  
> 
> This is the most simple fix to your immediate problem. However it
> doesn't address the big design issue.

Yeah, I was just looking to fix the file operation paths for my
particular problem and didn't try to do a core wide fix as I had
little knowledge of the I2C subsystem :(

-- 
viresh

Re: [PATCH 1/2] i2c-dev: don't get i2c adapter via i2c_dev

[Jean Delvare]

On Tue,  5 Jul 2016 19:57:06 -0700, Viresh Kumar wrote:
> There is no code protecting i2c_dev to be freed after it is returned
> from i2c_dev_get_by_minor() and using it to access the value which we
> already have (minor) isn't safe really.
> 
> Avoid using it and get the adapter directly from 'minor'.
> 
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---
>  drivers/i2c/i2c-dev.c | 7 +------
>  1 file changed, 1 insertion(+), 6 deletions(-)
> 
> diff --git a/drivers/i2c/i2c-dev.c b/drivers/i2c/i2c-dev.c
> index 6ecfd76270f2..66f323fd3982 100644
> --- a/drivers/i2c/i2c-dev.c
> +++ b/drivers/i2c/i2c-dev.c
> @@ -485,13 +485,8 @@ static int i2cdev_open(struct inode *inode, struct file *file)
>  	unsigned int minor = iminor(inode);
>  	struct i2c_client *client;
>  	struct i2c_adapter *adap;
> -	struct i2c_dev *i2c_dev;
> -
> -	i2c_dev = i2c_dev_get_by_minor(minor);
> -	if (!i2c_dev)
> -		return -ENODEV;
>  
> -	adap = i2c_get_adapter(i2c_dev->adap->nr);
> +	adap = i2c_get_adapter(minor);
>  	if (!adap)
>  		return -ENODEV;
>  

Anyway, this is a good cleanup/optimization on its own, on top of
fixing a race condition, independently of what we decide for the rest
of the problems being discussed in this thread. So:

Reviewed-by: Jean Delvare <jdelvare@suse.de>
Tested-by: Jean Delvare <jdelvare@suse.de>

Thanks,
-- 
Jean Delvare
SUSE L3 Support

Re: [PATCH 1/2] i2c-dev: don't get i2c adapter via i2c_dev

[Viresh Kumar]

On 07-07-16, 15:16, Jean Delvare wrote:
> Anyway, this is a good cleanup/optimization on its own, on top of
> fixing a race condition, independently of what we decide for the rest
> of the problems being discussed in this thread. So:
> 
> Reviewed-by: Jean Delvare <jdelvare@suse.de>
> Tested-by: Jean Delvare <jdelvare@suse.de>

Thanks Jean.

-- 
viresh

Re: [PATCH 1/2] i2c-dev: don't get i2c adapter via i2c_dev

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 511 bytes --]

On Tue, Jul 05, 2016 at 07:57:06PM -0700, Viresh Kumar wrote:
> There is no code protecting i2c_dev to be freed after it is returned
> from i2c_dev_get_by_minor() and using it to access the value which we
> already have (minor) isn't safe really.
> 
> Avoid using it and get the adapter directly from 'minor'.
> 
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>

Applied to for-next, thanks!

Jean, thanks for providing the background information! That was exactly
the missing piece for me.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

[PATCH 2/2] i2c-dev: Don't block the adapter from unregistering

[Viresh Kumar]

The i2c-dev calls i2c_get_adapter() from the .open() callback, which
doesn't let the adapter device unregister unless the .close() callback
is called.

On some platforms (like Google ARA), this doesn't let the modules
(hardware attached to the phone) eject from the phone as the cleanup
path for the module hasn't finished yet (i2c adapter not removed).

We can't let the userspace block the kernel forever in such cases.

Fix this by calling i2c_get_adapter() from all other file operations,
i.e.  read/write/ioctl, to make sure the adapter doesn't get away while
we are in the middle of a operation, but not otherwise. In .open() we
will release the adapter device before returning and so if there is no
data transfer in progress, then the i2c-dev doesn't block the adapter
from unregistering.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/i2c/i2c-dev.c | 72 ++++++++++++++++++++++++++++++++++++++++++++++-----
 include/linux/i2c.h   |  1 +
 2 files changed, 66 insertions(+), 7 deletions(-)

diff --git a/drivers/i2c/i2c-dev.c b/drivers/i2c/i2c-dev.c
index 66f323fd3982..b2562603daa9 100644
--- a/drivers/i2c/i2c-dev.c
+++ b/drivers/i2c/i2c-dev.c
@@ -142,13 +142,25 @@ static ssize_t i2cdev_read(struct file *file, char __user *buf, size_t count,
 	int ret;
 
 	struct i2c_client *client = file->private_data;
+	struct i2c_adapter *adap;
+
+	adap = i2c_get_adapter(client->adapter_nr);
+	if (!adap)
+		return -ENODEV;
+
+	if (adap != client->adapter) {
+		ret = -EINVAL;
+		goto put_adapter;
+	}
 
 	if (count > 8192)
 		count = 8192;
 
 	tmp = kmalloc(count, GFP_KERNEL);
-	if (tmp == NULL)
-		return -ENOMEM;
+	if (tmp == NULL) {
+		ret = -ENOMEM;
+		goto put_adapter;
+	}
 
 	pr_debug("i2c-dev: i2c-%d reading %zu bytes.\n",
 		iminor(file_inode(file)), count);
@@ -157,6 +169,9 @@ static ssize_t i2cdev_read(struct file *file, char __user *buf, size_t count,
 	if (ret >= 0)
 		ret = copy_to_user(buf, tmp, count) ? -EFAULT : ret;
 	kfree(tmp);
+
+put_adapter:
+	i2c_put_adapter(adap);
 	return ret;
 }
 
@@ -166,19 +181,34 @@ static ssize_t i2cdev_write(struct file *file, const char __user *buf,
 	int ret;
 	char *tmp;
 	struct i2c_client *client = file->private_data;
+	struct i2c_adapter *adap;
+
+	adap = i2c_get_adapter(client->adapter_nr);
+	if (!adap)
+		return -ENODEV;
+
+	if (adap != client->adapter) {
+		ret = -EINVAL;
+		goto put_adapter;
+	}
 
 	if (count > 8192)
 		count = 8192;
 
 	tmp = memdup_user(buf, count);
-	if (IS_ERR(tmp))
-		return PTR_ERR(tmp);
+	if (IS_ERR(tmp)) {
+		ret = PTR_ERR(tmp);
+		goto put_adapter;
+	}
 
 	pr_debug("i2c-dev: i2c-%d writing %zu bytes.\n",
 		iminor(file_inode(file)), count);
 
 	ret = i2c_master_send(client, tmp, count);
 	kfree(tmp);
+
+put_adapter:
+	i2c_put_adapter(adap);
 	return ret;
 }
 
@@ -412,9 +442,9 @@ static noinline int i2cdev_ioctl_smbus(struct i2c_client *client,
 	return res;
 }
 
-static long i2cdev_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
+static long __i2cdev_ioctl(struct i2c_client *client, unsigned int cmd,
+			   unsigned long arg)
 {
-	struct i2c_client *client = file->private_data;
 	unsigned long funcs;
 
 	dev_dbg(&client->adapter->dev, "ioctl, cmd=0x%02x, arg=0x%02lx\n",
@@ -480,6 +510,28 @@ static long i2cdev_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
 	return 0;
 }
 
+static long i2cdev_ioctl(struct file *file, unsigned int cmd, unsigned long arg)
+{
+	struct i2c_client *client = file->private_data;
+	struct i2c_adapter *adap;
+	unsigned long ret;
+
+	adap = i2c_get_adapter(client->adapter_nr);
+	if (!adap)
+		return -ENODEV;
+
+	if (adap != client->adapter) {
+		ret = -EINVAL;
+		goto put_adapter;
+	}
+
+	ret = __i2cdev_ioctl(client, cmd, arg);
+
+put_adapter:
+	i2c_put_adapter(adap);
+	return ret;
+}
+
 static int i2cdev_open(struct inode *inode, struct file *file)
 {
 	unsigned int minor = iminor(inode);
@@ -504,9 +556,16 @@ static int i2cdev_open(struct inode *inode, struct file *file)
 	}
 	snprintf(client->name, I2C_NAME_SIZE, "i2c-dev %d", adap->nr);
 
+	client->adapter_nr = minor;
 	client->adapter = adap;
 	file->private_data = client;
 
+	/*
+	 * Allow the adapter to unregister while userspace has opened the i2c
+	 * device.
+	 */
+	i2c_put_adapter(client->adapter);
+
 	return 0;
 }
 
@@ -514,7 +573,6 @@ static int i2cdev_release(struct inode *inode, struct file *file)
 {
 	struct i2c_client *client = file->private_data;
 
-	i2c_put_adapter(client->adapter);
 	kfree(client);
 	file->private_data = NULL;
 
diff --git a/include/linux/i2c.h b/include/linux/i2c.h
index fffdc270ca18..38c8fe8ca681 100644
--- a/include/linux/i2c.h
+++ b/include/linux/i2c.h
@@ -234,6 +234,7 @@ struct i2c_client {
 	struct i2c_adapter *adapter;	/* the adapter we sit on	*/
 	struct device dev;		/* the device structure		*/
 	int irq;			/* irq issued by device		*/
+	int adapter_nr;
 	struct list_head detected;
 #if IS_ENABLED(CONFIG_I2C_SLAVE)
 	i2c_slave_cb_t slave_cb;	/* callback for slave mode	*/
-- 
2.7.4

Re: [PATCH 2/2] i2c-dev: Don't block the adapter from unregistering

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 3202 bytes --]

Hi,

[auto build test WARNING on wsa/i2c/for-next]
[also build test WARNING on v4.7-rc6 next-20160705]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Viresh-Kumar/i2c-dev-Don-t-let-userspace-block-adapter/20160706-110245
base:   https://git.kernel.org/pub/scm/linux/kernel/git/wsa/linux.git i2c/for-next
reproduce: make htmldocs

All warnings (new ones prefixed by >>):

   include/linux/init.h:1: warning: no structured comments found
   kernel/sched/core.c:2079: warning: No description found for parameter 'cookie'
   kernel/sys.c:1: warning: no structured comments found
   drivers/dma-buf/seqno-fence.c:1: warning: no structured comments found
>> include/linux/i2c.h:242: warning: No description found for parameter 'adapter_nr'

vim +/adapter_nr +242 include/linux/i2c.h

d64f73be David Brownell  2007-07-12  226   * managing the device.
^1da177e Linus Torvalds  2005-04-16  227   */
^1da177e Linus Torvalds  2005-04-16  228  struct i2c_client {
2096b956 David Brownell  2007-05-01  229  	unsigned short flags;		/* div., see below		*/
5071860a Jean Delvare    2005-07-20  230  	unsigned short addr;		/* chip address - NOTE: 7bit	*/
^1da177e Linus Torvalds  2005-04-16  231  					/* addresses are stored in the	*/
5071860a Jean Delvare    2005-07-20  232  					/* _LOWER_ 7 bits		*/
2096b956 David Brownell  2007-05-01  233  	char name[I2C_NAME_SIZE];
^1da177e Linus Torvalds  2005-04-16  234  	struct i2c_adapter *adapter;	/* the adapter we sit on	*/
^1da177e Linus Torvalds  2005-04-16  235  	struct device dev;		/* the device structure		*/
8e29da9e Wolfram Sang    2008-07-01  236  	int irq;			/* irq issued by device		*/
42fc20b8 Viresh Kumar    2016-07-05  237  	int adapter_nr;
4735c98f Jean Delvare    2008-07-14  238  	struct list_head detected;
d5fd120e Jean Delvare    2015-01-26  239  #if IS_ENABLED(CONFIG_I2C_SLAVE)
4b1acc43 Wolfram Sang    2014-11-18  240  	i2c_slave_cb_t slave_cb;	/* callback for slave mode	*/
d5fd120e Jean Delvare    2015-01-26  241  #endif
^1da177e Linus Torvalds  2005-04-16 @242  };
^1da177e Linus Torvalds  2005-04-16  243  #define to_i2c_client(d) container_of(d, struct i2c_client, dev)
^1da177e Linus Torvalds  2005-04-16  244  
9b766b81 David Brownell  2008-01-27  245  extern struct i2c_client *i2c_verify_client(struct device *dev);
643dd09e Stephen Warren  2012-04-17  246  extern struct i2c_adapter *i2c_verify_adapter(struct device *dev);
9b766b81 David Brownell  2008-01-27  247  
a61fc683 Ben Gardner     2005-07-27  248  static inline struct i2c_client *kobj_to_i2c_client(struct kobject *kobj)
a61fc683 Ben Gardner     2005-07-27  249  {
d75d53cd Mark M. Hoffman 2007-07-12  250  	struct device * const dev = container_of(kobj, struct device, kobj);

:::::: The code at line 242 was first introduced by commit
:::::: 1da177e4c3f41524e886b7f1b8a0c1fc7321cac2 Linux-2.6.12-rc2

:::::: TO: Linus Torvalds <torvalds@ppc970.osdl.org>
:::::: CC: Linus Torvalds <torvalds@ppc970.osdl.org>

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/octet-stream, Size: 6370 bytes --]

Re: [PATCH 2/2] i2c-dev: Don't block the adapter from unregistering

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 3202 bytes --]

Hi,

[auto build test WARNING on wsa/i2c/for-next]
[also build test WARNING on v4.7-rc6 next-20160705]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Viresh-Kumar/i2c-dev-Don-t-let-userspace-block-adapter/20160706-110245
base:   https://git.kernel.org/pub/scm/linux/kernel/git/wsa/linux.git i2c/for-next
reproduce: make htmldocs

All warnings (new ones prefixed by >>):

   include/linux/init.h:1: warning: no structured comments found
   kernel/sched/core.c:2079: warning: No description found for parameter 'cookie'
   kernel/sys.c:1: warning: no structured comments found
   drivers/dma-buf/seqno-fence.c:1: warning: no structured comments found
>> include/linux/i2c.h:242: warning: No description found for parameter 'adapter_nr'

vim +/adapter_nr +242 include/linux/i2c.h

d64f73be David Brownell  2007-07-12  226   * managing the device.
^1da177e Linus Torvalds  2005-04-16  227   */
^1da177e Linus Torvalds  2005-04-16  228  struct i2c_client {
2096b956 David Brownell  2007-05-01  229  	unsigned short flags;		/* div., see below		*/
5071860a Jean Delvare    2005-07-20  230  	unsigned short addr;		/* chip address - NOTE: 7bit	*/
^1da177e Linus Torvalds  2005-04-16  231  					/* addresses are stored in the	*/
5071860a Jean Delvare    2005-07-20  232  					/* _LOWER_ 7 bits		*/
2096b956 David Brownell  2007-05-01  233  	char name[I2C_NAME_SIZE];
^1da177e Linus Torvalds  2005-04-16  234  	struct i2c_adapter *adapter;	/* the adapter we sit on	*/
^1da177e Linus Torvalds  2005-04-16  235  	struct device dev;		/* the device structure		*/
8e29da9e Wolfram Sang    2008-07-01  236  	int irq;			/* irq issued by device		*/
42fc20b8 Viresh Kumar    2016-07-05  237  	int adapter_nr;
4735c98f Jean Delvare    2008-07-14  238  	struct list_head detected;
d5fd120e Jean Delvare    2015-01-26  239  #if IS_ENABLED(CONFIG_I2C_SLAVE)
4b1acc43 Wolfram Sang    2014-11-18  240  	i2c_slave_cb_t slave_cb;	/* callback for slave mode	*/
d5fd120e Jean Delvare    2015-01-26  241  #endif
^1da177e Linus Torvalds  2005-04-16 @242  };
^1da177e Linus Torvalds  2005-04-16  243  #define to_i2c_client(d) container_of(d, struct i2c_client, dev)
^1da177e Linus Torvalds  2005-04-16  244  
9b766b81 David Brownell  2008-01-27  245  extern struct i2c_client *i2c_verify_client(struct device *dev);
643dd09e Stephen Warren  2012-04-17  246  extern struct i2c_adapter *i2c_verify_adapter(struct device *dev);
9b766b81 David Brownell  2008-01-27  247  
a61fc683 Ben Gardner     2005-07-27  248  static inline struct i2c_client *kobj_to_i2c_client(struct kobject *kobj)
a61fc683 Ben Gardner     2005-07-27  249  {
d75d53cd Mark M. Hoffman 2007-07-12  250  	struct device * const dev = container_of(kobj, struct device, kobj);

:::::: The code at line 242 was first introduced by commit
:::::: 1da177e4c3f41524e886b7f1b8a0c1fc7321cac2 Linux-2.6.12-rc2

:::::: TO: Linus Torvalds <torvalds@ppc970.osdl.org>
:::::: CC: Linus Torvalds <torvalds@ppc970.osdl.org>

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/octet-stream, Size: 6370 bytes --]

Re: [PATCH 2/2] i2c-dev: Don't block the adapter from unregistering

[Wolfram Sang]

[-- Attachment #1: Type: text/plain, Size: 1496 bytes --]

On Tue, Jul 05, 2016 at 07:57:07PM -0700, Viresh Kumar wrote:
> The i2c-dev calls i2c_get_adapter() from the .open() callback, which
> doesn't let the adapter device unregister unless the .close() callback
> is called.
> 
> On some platforms (like Google ARA), this doesn't let the modules
> (hardware attached to the phone) eject from the phone as the cleanup
> path for the module hasn't finished yet (i2c adapter not removed).
> 
> We can't let the userspace block the kernel forever in such cases.
> 
> Fix this by calling i2c_get_adapter() from all other file operations,
> i.e.  read/write/ioctl, to make sure the adapter doesn't get away while
> we are in the middle of a operation, but not otherwise. In .open() we
> will release the adapter device before returning and so if there is no
> data transfer in progress, then the i2c-dev doesn't block the adapter
> from unregistering.
> 
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>

I'd think Jean has more experience with I2C hotplugging approaches and
difficulties, so I'd be interested in his high level review.

However:

> @@ -234,6 +234,7 @@ struct i2c_client {
>  	struct i2c_adapter *adapter;	/* the adapter we sit on	*/
>  	struct device dev;		/* the device structure		*/
>  	int irq;			/* irq issued by device		*/
> +	int adapter_nr;
>  	struct list_head detected;

Adding something to *every* i2c_client for this corner case sounds
pretty expensive to me.

Regards,

   Wolfram


[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH 2/2] i2c-dev: Don't block the adapter from unregistering

[Viresh Kumar]

On 06-07-16, 15:55, Wolfram Sang wrote:
> Adding something to *every* i2c_client for this corner case sounds
> pretty expensive to me.

I agree with you on that. I wanted to avoid it, but I couldn't :(

Lets see how Jean suggests to handle it.

-- 
viresh

Re: [PATCH 2/2] i2c-dev: Don't block the adapter from unregistering

[Jean Delvare]

On Wed, 6 Jul 2016 15:55:24 +0900, Wolfram Sang wrote:
> On Tue, Jul 05, 2016 at 07:57:07PM -0700, Viresh Kumar wrote:
> > The i2c-dev calls i2c_get_adapter() from the .open() callback, which
> > doesn't let the adapter device unregister unless the .close() callback
> > is called.
> > 
> > On some platforms (like Google ARA), this doesn't let the modules
> > (hardware attached to the phone) eject from the phone as the cleanup
> > path for the module hasn't finished yet (i2c adapter not removed).
> > 
> > We can't let the userspace block the kernel forever in such cases.
> > 
> > Fix this by calling i2c_get_adapter() from all other file operations,
> > i.e.  read/write/ioctl, to make sure the adapter doesn't get away while
> > we are in the middle of a operation, but not otherwise. In .open() we
> > will release the adapter device before returning and so if there is no
> > data transfer in progress, then the i2c-dev doesn't block the adapter
> > from unregistering.
> > 
> > Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> 
> I'd think Jean has more experience with I2C hotplugging approaches and
> difficulties, so I'd be interested in his high level review.

Well well... I don't like this patch at all to be honest.

My first question would be: what is keeping /dev/i2c-* open all the
time? Originally i2c-dev was developed with development and debugging
tools in mind (the i2c-tools suite.) The device nodes were never meant
to be kept open for more than a few seconds.

Do you have user-space i2c device drivers on your system? Which ones,
and why (I would expect all useful i2c devices to have a kernel
driver.) And why do they keep their i2c device node opened all the time?

Requesting and freeing the i2c adapter for every transaction is going
to add a lot of overhead to all existing tools :-(

It's not like every user can open i2c device nodes and block the
system. Only selected users should be able to open i2c device nodes
(only root by default) so they should be responsible for not
misbehaving.

-- 
Jean Delvare
SUSE L3 Support

Re: [PATCH 2/2] i2c-dev: Don't block the adapter from unregistering

[Viresh Kumar]

On 06-07-16, 19:12, Jean Delvare wrote:
> Well well... I don't like this patch at all to be honest.

Sure, I didn't like it much as well. I just wanted people to comment on what
else we can do here. We don't really want to add out-of-mainline stuff here.

> My first question would be: what is keeping /dev/i2c-* open all the
> time? Originally i2c-dev was developed with development and debugging
> tools in mind (the i2c-tools suite.) The device nodes were never meant
> to be kept open for more than a few seconds.

We thought that buggy userspace shouldn't be allowed to get kernel into trouble.
Isn't that the case ?

In our case this is what happens:
- userspace opens the file descriptor
- we try to forcefully remove the module from phone (that doesn't talk to
  userspace to stop using the device).
- The module doesn't get ejected unless the app closes the fd.

> Do you have user-space i2c device drivers on your system? Which ones,

No. Its probably an app written by some of our module app developers.

> and why (I would expect all useful i2c devices to have a kernel
> driver.)

That's what we have.

> Requesting and freeing the i2c adapter for every transaction is going

Well, we are just finding it (taking a reference of it) and the dropping its
reference.

> to add a lot of overhead to all existing tools :-(

:(

> It's not like every user can open i2c device nodes and block the
> system. Only selected users should be able to open i2c device nodes
> (only root by default) so they should be responsible for not
> misbehaving.

Hmmm. The problem is that they weren't told when the module tries to go away and
so they don't know that they need to close the fd.

Also coming to the earlier thing, I though even the buggy userspace thing
shouldn't be allowed to block kernel device unregisteration.

-- 
viresh

Re: [PATCH 2/2] i2c-dev: Don't block the adapter from unregistering

[Jean Delvare]

Hi Viresh,

On Wed, 6 Jul 2016 13:55:40 -0700, Viresh Kumar wrote:
> On 06-07-16, 19:12, Jean Delvare wrote:
> > Well well... I don't like this patch at all to be honest.
> 
> Sure, I didn't like it much as well. I just wanted people to comment on what
> else we can do here. We don't really want to add out-of-mainline stuff here.
> 
> > My first question would be: what is keeping /dev/i2c-* open all the
> > time? Originally i2c-dev was developed with development and debugging
> > tools in mind (the i2c-tools suite.) The device nodes were never meant
> > to be kept open for more than a few seconds.
> 
> We thought that buggy userspace shouldn't be allowed to get kernel into trouble.
> Isn't that the case ?

Buggy user-space run by root can cause any kind of trouble. And the
point being discussed here is amongst the most minor of these. But I
even disagree with calling it buggy.

> In our case this is what happens:
> - userspace opens the file descriptor
> - we try to forcefully remove the module from phone (that doesn't talk to
>   userspace to stop using the device).
> - The module doesn't get ejected unless the app closes the fd.

So you are basically building a test case to cause the problem. It's
artificial. The adapter reference being held while the device node is
open isn't a bug, it is a design decision. I would consider revisiting
that design if there was a real world case where it causes trouble, but
not for an artificially created test case.

I don't see anything fundamentally wrong in the design anyway. I do not
expect to be allowed to remove a hard disk drive from my system while
its partitions are mounted, and I don't expect to be able to unmount
partitions while users have files opened on them. You always have to
tear things down in the right order. Same here.

> > Do you have user-space i2c device drivers on your system? Which ones,
> 
> No. Its probably an app written by some of our module app developers.
> 
> > and why (I would expect all useful i2c devices to have a kernel
> > driver.)
> 
> That's what we have.

Still no details here. What app, what is it doing, to what device is it
talking, why is it not a kernel driver, and why do they keep the device
node opened all the time?

> > Requesting and freeing the i2c adapter for every transaction is going
> 
> Well, we are just finding it (taking a reference of it) and the dropping its
> reference.

Yes, that's what I meant, sorry for using the wrong terms.

> > to add a lot of overhead to all existing tools :-(
> 
> :(

i2cdump typically runs 258 ioctls on the device node, i2cdetect 235.
i2c_get_adapter isn't cheap. Multiple function calls, mutex
locking/unlocking, preemption disabling/enabling... You don't want do
to that repeatedly if it can be avoided. So, nack from me.

> > It's not like every user can open i2c device nodes and block the
> > system. Only selected users should be able to open i2c device nodes
> > (only root by default) so they should be responsible for not
> > misbehaving.
> 
> Hmmm. The problem is that they weren't told when the module tries to go away and
> so they don't know that they need to close the fd.

See my previous questions. We still don't know why they are doing what
they are doing in user-space, nor why they think they have to keep the
device node opened.

They could always kill the application in question with:
# fuser -k /dev/i2c-*
before removing the module. Or find a more polite way to tell the
application to quit. If they want to do it in user-space, they have to
do it right.

-- 
Jean Delvare
SUSE L3 Support

Re: [PATCH 2/2] i2c-dev: Don't block the adapter from unregistering

[Greg KH]

On Mon, Jul 11, 2016 at 02:22:09PM +0200, Jean Delvare wrote:
> Hi Viresh,
> 
> On Wed, 6 Jul 2016 13:55:40 -0700, Viresh Kumar wrote:
> > On 06-07-16, 19:12, Jean Delvare wrote:
> > > Well well... I don't like this patch at all to be honest.
> > 
> > Sure, I didn't like it much as well. I just wanted people to comment on what
> > else we can do here. We don't really want to add out-of-mainline stuff here.
> > 
> > > My first question would be: what is keeping /dev/i2c-* open all the
> > > time? Originally i2c-dev was developed with development and debugging
> > > tools in mind (the i2c-tools suite.) The device nodes were never meant
> > > to be kept open for more than a few seconds.
> > 
> > We thought that buggy userspace shouldn't be allowed to get kernel into trouble.
> > Isn't that the case ?
> 
> Buggy user-space run by root can cause any kind of trouble. And the
> point being discussed here is amongst the most minor of these. But I
> even disagree with calling it buggy.
> 
> > In our case this is what happens:
> > - userspace opens the file descriptor
> > - we try to forcefully remove the module from phone (that doesn't talk to
> >   userspace to stop using the device).
> > - The module doesn't get ejected unless the app closes the fd.
> 
> So you are basically building a test case to cause the problem. It's
> artificial. The adapter reference being held while the device node is
> open isn't a bug, it is a design decision. I would consider revisiting
> that design if there was a real world case where it causes trouble, but
> not for an artificially created test case.
> 
> I don't see anything fundamentally wrong in the design anyway. I do not
> expect to be allowed to remove a hard disk drive from my system while
> its partitions are mounted, and I don't expect to be able to unmount
> partitions while users have files opened on them. You always have to
> tear things down in the right order. Same here.

But that's exactly what you do today when your USB disk falls out of
it's connection.  The kernel recovers and you move on.  Hopefully it
wasn't your root partition :)

Same for a serial device that has open userspace descriptors that is
removed from the system, or almost any other type of device that can be
physically removed from a system while it is currently running (PCI,
firewire, thunderbolt, etc.)  What happens if you have an i2c device on
a PCI card that is removed while userspace has that device descriptor
open?  You need to "invalidate" it and not oops if userspace keeps
reading/writing to it.


> > > Do you have user-space i2c device drivers on your system? Which ones,
> > 
> > No. Its probably an app written by some of our module app developers.
> > 
> > > and why (I would expect all useful i2c devices to have a kernel
> > > driver.)
> > 
> > That's what we have.
> 
> Still no details here. What app, what is it doing, to what device is it
> talking, why is it not a kernel driver, and why do they keep the device
> node opened all the time?

Any random application can write to an i2c device in this system, as
long as it has "permission" to do so.  But, it's hardware, and on a bus
that sometimes can be yanked out of the system.  When that happens,
userspace will be notified of the removal, and "should" be nice and
clean up after itself.  But there will always be some lag between the
actual removal when the kernel figures it out, and when userspace
finally closes that device node.

So not crashing the kernel is a nice thing to prevent from happening
during that window of when the device is removed and userspace hasn't
quite noticed it yet.

> > > Requesting and freeing the i2c adapter for every transaction is going
> > 
> > Well, we are just finding it (taking a reference of it) and the dropping its
> > reference.
> 
> Yes, that's what I meant, sorry for using the wrong terms.
> 
> > > to add a lot of overhead to all existing tools :-(
> > 
> > :(
> 
> i2cdump typically runs 258 ioctls on the device node, i2cdetect 235.
> i2c_get_adapter isn't cheap. Multiple function calls, mutex
> locking/unlocking, preemption disabling/enabling... You don't want do
> to that repeatedly if it can be avoided. So, nack from me.
> 
> > > It's not like every user can open i2c device nodes and block the
> > > system. Only selected users should be able to open i2c device nodes
> > > (only root by default) so they should be responsible for not
> > > misbehaving.
> > 
> > Hmmm. The problem is that they weren't told when the module tries to go away and
> > so they don't know that they need to close the fd.
> 
> See my previous questions. We still don't know why they are doing what
> they are doing in user-space, nor why they think they have to keep the
> device node opened.
> 
> They could always kill the application in question with:
> # fuser -k /dev/i2c-*
> before removing the module. Or find a more polite way to tell the
> application to quit. If they want to do it in user-space, they have to
> do it right.

Ideally, yes, userspace will have closed that device node.  But again,
hardware isn't kind and sometimes decides to be yanked out by users
before they tell the kernel about it.  We handle this for almost all
other device subsystems, i2c is one of the last to be fixed up in this
manner.  Sorry it's taken us well over a decade to get here :)

hope that explains things better,

greg k-h

Re: [PATCH 2/2] i2c-dev: Don't block the adapter from unregistering

[Viresh Kumar]

On 11-07-16, 14:50, Greg KH wrote:
> Ideally, yes, userspace will have closed that device node.  But again,
> hardware isn't kind and sometimes decides to be yanked out by users
> before they tell the kernel about it.  We handle this for almost all
> other device subsystems, i2c is one of the last to be fixed up in this
> manner.  Sorry it's taken us well over a decade to get here :)
> 
> hope that explains things better,

@Jean: Ping :)

-- 
viresh

Re: [PATCH 2/2] i2c-dev: Don't block the adapter from unregistering

[Jean Delvare]

Hi Greg, Viresh,

On Mon, 11 Jul 2016 14:50:58 -0700, Greg KH wrote:
> On Mon, Jul 11, 2016 at 02:22:09PM +0200, Jean Delvare wrote:
> > So you are basically building a test case to cause the problem. It's
> > artificial. The adapter reference being held while the device node is
> > open isn't a bug, it is a design decision. I would consider revisiting
> > that design if there was a real world case where it causes trouble, but
> > not for an artificially created test case.
> > 
> > I don't see anything fundamentally wrong in the design anyway. I do not
> > expect to be allowed to remove a hard disk drive from my system while
> > its partitions are mounted, and I don't expect to be able to unmount
> > partitions while users have files opened on them. You always have to
> > tear things down in the right order. Same here.
> 
> But that's exactly what you do today when your USB disk falls out of
> it's connection.  The kernel recovers and you move on.  Hopefully it
> wasn't your root partition :)
> 
> Same for a serial device that has open userspace descriptors that is
> removed from the system, or almost any other type of device that can be
> physically removed from a system while it is currently running (PCI,
> firewire, thunderbolt, etc.)  What happens if you have an i2c device on
> a PCI card that is removed while userspace has that device descriptor
> open?  You need to "invalidate" it and not oops if userspace keeps
> reading/writing to it.

Honestly I have no idea what would happen in this case. I would expect
the PCI card to be offlined by software first, before it is physically
removed. Hot-unplug doesn't mean hot-tearoff ;-)

In case unprepared hardware tear-off actually happens (more
realistically on USB rather than PCI I suppose) I agree we want to
avoid oopses and other horrible consequences and behave as smoothly as
possible. The code was not written with this scenario in mind, so
additional work is certainly needed.

> > (...)
> > Still no details here. What app, what is it doing, to what device is it
> > talking, why is it not a kernel driver, and why do they keep the device
> > node opened all the time?
> 
> Any random application can write to an i2c device in this system, as
> long as it has "permission" to do so.  But, it's hardware, and on a bus
> that sometimes can be yanked out of the system.  When that happens,
> userspace will be notified of the removal, and "should" be nice and

I don't think there is any such notification on i2c device nodes at the
moment. Unless it's something generic. But I'm certain i2cdump etc.
aren't listening anyway.

> clean up after itself.  But there will always be some lag between the
> actual removal when the kernel figures it out, and when userspace
> finally closes that device node.
> 
> So not crashing the kernel is a nice thing to prevent from happening
> during that window of when the device is removed and userspace hasn't
> quite noticed it yet.

I agree.

> > (...)
> > See my previous questions. We still don't know why they are doing what
> > they are doing in user-space, nor why they think they have to keep the
> > device node opened.
> > 
> > They could always kill the application in question with:
> > # fuser -k /dev/i2c-*
> > before removing the module. Or find a more polite way to tell the
> > application to quit. If they want to do it in user-space, they have to
> > do it right.
> 
> Ideally, yes, userspace will have closed that device node.  But again,
> hardware isn't kind and sometimes decides to be yanked out by users
> before they tell the kernel about it.  We handle this for almost all
> other device subsystems, i2c is one of the last to be fixed up in this
> manner.  Sorry it's taken us well over a decade to get here :)

The problem is that the patch proposed by Viresh has nothing to do with
this. It's not adding notifications, just changing the time frame during
which user-space holds a reference to the i2c (bus) device. The goal as
I understand it is to allow *prepared* hot-unplug (in the form of
"rmmod i2c-bus-device-driver" or sysfs-based offlining?) while
user-space processes have i2c device nodes open. Unprepared hot-unplug
will still go wrong exactly as it goes now.

My point is that prepared hot-unplug can already be achieved today
without any patch. Or possibly improved by adding a notification
mechanism. But not by changing the reference holding design.

Not only the proposed patch does not help and degrades the performance,
but it breaks assumptions. For example, it would allow an application
to open an i2c bus, then you remove its driver and load another i2c bus
driver, which gets the same bus number, and now the application writes
to a completely different I2C bus segment. The current reference model
prevents that, on purpose.

So, again, nack from me.

-- 
Jean Delvare
SUSE L3 Support

Re: [PATCH 2/2] i2c-dev: Don't block the adapter from unregistering

[Viresh Kumar]

Hi Jean,

On 25-07-16, 11:39, Jean Delvare wrote:
> The problem is that the patch proposed by Viresh has nothing to do with
> this. It's not adding notifications, just changing the time frame during
> which user-space holds a reference to the i2c (bus) device. The goal as
> I understand it is to allow *prepared* hot-unplug (in the form of
> "rmmod i2c-bus-device-driver" or sysfs-based offlining?) while

Not really. We are concerned about both prepared and Unprepared cases.

This *hacky* patch was useful in case of *unprepared* hot-unplug as well.

Here is the sequence of events:
- open() i2c device from userspace
- do some operations on the device read/write/ioctls() ..
- Module hot-unplugged (*unprepared*)
- Some of the ongoing i2c transactions may just fail, that is fine ..
- Kernel detected the interrupt about module removal and tries to
  cleanup the devices..
- Now, kernel can not remove the i2c device, unless user application
  has closed the file descriptor.

And so kernel is waiting in the driver's ->remove() callback forever.

Also, there is no way to co-ordinate (in Android) with the
Applications using the device.  They can crash or fail out if they
want to, but the kernel shouldn't stop removal of a hardware module in
that case.

> user-space processes have i2c device nodes open. Unprepared hot-unplug
> will still go wrong exactly as it goes now.

> My point is that prepared hot-unplug can already be achieved today
> without any patch.

Yeah, if we have the option of stopping the applications before the
device is gone.

> Or possibly improved by adding a notification
> mechanism. But not by changing the reference holding design.
> 
> Not only the proposed patch does not help and degrades the performance,
> but it breaks assumptions. For example, it would allow an application
> to open an i2c bus, then you remove its driver and load another i2c bus
> driver, which gets the same bus number, and now the application writes
> to a completely different I2C bus segment. The current reference model
> prevents that, on purpose.
> 
> So, again, nack from me.

Yeah, the patch wasn't great and I knew it from the beginning. But we
are looking for a solution that can be accepted and so need advice
from you guys :)

-- 
viresh

Re: [PATCH 2/2] i2c-dev: Don't block the adapter from unregistering

[Jean Delvare]

Hi Viresh,

On Mon, 25 Jul 2016 15:31:40 -0700, Viresh Kumar wrote:
> On 25-07-16, 11:39, Jean Delvare wrote:
> > The problem is that the patch proposed by Viresh has nothing to do with
> > this. It's not adding notifications, just changing the time frame during
> > which user-space holds a reference to the i2c (bus) device. The goal as
> > I understand it is to allow *prepared* hot-unplug (in the form of
> > "rmmod i2c-bus-device-driver" or sysfs-based offlining?) while
> 
> Not really. We are concerned about both prepared and Unprepared cases.
> 
> This *hacky* patch was useful in case of *unprepared* hot-unplug as well.
> 
> Here is the sequence of events:
> - open() i2c device from userspace
> - do some operations on the device read/write/ioctls() ..
> - Module hot-unplugged (*unprepared*)
> - Some of the ongoing i2c transactions may just fail, that is fine ..
> - Kernel detected the interrupt about module removal and tries to
>   cleanup the devices..
> - Now, kernel can not remove the i2c device, unless user application
>   has closed the file descriptor.

Well, what is the application doing at that point? What error codes is
it getting? Should it not close the device node and bail out?

> And so kernel is waiting in the driver's ->remove() callback forever.
> 
> Also, there is no way to co-ordinate (in Android) with the
> Applications using the device.

Why? Looks like a serious limitation.

At this point I still don't know what application we are talking about,
why it has to be in user-space when I2C device drivers are supposed to
be on the kernel side, nor why they have to keep the i2c device node
opened all the time.

Why do you insist on relying on i2c-dev when it is so clearly no
compatible with your requirements?

> They can crash or fail out if they
> want to, but the kernel shouldn't stop removal of a hardware module in
> that case.

If they crash or bail out, that would close the device, so no problem.
The problem would be if they stay alive and misbehave.

If the kernel should be completely independent from what user-space is
doing, this is simply not compatible with reference counting. The whole
point of counting references is to know when a device is still in use,
and prevent it from being removed while this is the case. If you say
that devices can be removed at any time and this is OK, then you should
not be counting references at all, this is pointless. But I doubt the
i2c code is currently ready for this.

> > user-space processes have i2c device nodes open. Unprepared hot-unplug
> > will still go wrong exactly as it goes now.
> 
> > My point is that prepared hot-unplug can already be achieved today
> > without any patch.
> 
> Yeah, if we have the option of stopping the applications before the
> device is gone.
> 
> > Or possibly improved by adding a notification
> > mechanism. But not by changing the reference holding design.
> > 
> > Not only the proposed patch does not help and degrades the performance,
> > but it breaks assumptions. For example, it would allow an application
> > to open an i2c bus, then you remove its driver and load another i2c bus
> > driver, which gets the same bus number, and now the application writes
> > to a completely different I2C bus segment. The current reference model
> > prevents that, on purpose.
> > 
> > So, again, nack from me.
> 
> Yeah, the patch wasn't great and I knew it from the beginning. But we
> are looking for a solution that can be accepted and so need advice
> from you guys :)

I have no idea, sorry. Hopefully Greg or Wolfram know more? Check what
other subsystems are doing? 

-- 
Jean Delvare
SUSE L3 Support

Re: [PATCH 2/2] i2c-dev: Don't block the adapter from unregistering

[Dmitry Torokhov]

Hi Jean,

On Tue, Jul 26, 2016 at 12:41 AM, Jean Delvare <jdelvare@suse.de> wrote:
> Hi Viresh,
>
> On Mon, 25 Jul 2016 15:31:40 -0700, Viresh Kumar wrote:
>> On 25-07-16, 11:39, Jean Delvare wrote:
>> > The problem is that the patch proposed by Viresh has nothing to do with
>> > this. It's not adding notifications, just changing the time frame during
>> > which user-space holds a reference to the i2c (bus) device. The goal as
>> > I understand it is to allow *prepared* hot-unplug (in the form of
>> > "rmmod i2c-bus-device-driver" or sysfs-based offlining?) while
>>
>> Not really. We are concerned about both prepared and Unprepared cases.
>>
>> This *hacky* patch was useful in case of *unprepared* hot-unplug as well.
>>
>> Here is the sequence of events:
>> - open() i2c device from userspace
>> - do some operations on the device read/write/ioctls() ..
>> - Module hot-unplugged (*unprepared*)
>> - Some of the ongoing i2c transactions may just fail, that is fine ..
>> - Kernel detected the interrupt about module removal and tries to
>>   cleanup the devices..
>> - Now, kernel can not remove the i2c device, unless user application
>>   has closed the file descriptor.
>
> Well, what is the application doing at that point? What error codes is
> it getting? Should it not close the device node and bail out?

It should. Will it? It is up to the application. It may decide to keep
that file descriptor open forever.

>
>> And so kernel is waiting in the driver's ->remove() callback forever.
>>
>> Also, there is no way to co-ordinate (in Android) with the
>> Applications using the device.
>
> Why? Looks like a serious limitation.
>
> At this point I still don't know what application we are talking about,
> why it has to be in user-space when I2C device drivers are supposed to
> be on the kernel side, nor why they have to keep the i2c device node
> opened all the time.
>
> Why do you insist on relying on i2c-dev when it is so clearly no
> compatible with your requirements?

If you provide an interface expect it to be used, maybe even in the
ways you did not anticipated. Kernel's (and out task) is to make the
interface behave properly, not police users.

>
>> They can crash or fail out if they
>> want to, but the kernel shouldn't stop removal of a hardware module in
>> that case.
>
> If they crash or bail out, that would close the device, so no problem.
> The problem would be if they stay alive and misbehave.
>
> If the kernel should be completely independent from what user-space is
> doing, this is simply not compatible with reference counting. The whole
> point of counting references is to know when a device is still in use,
> and prevent it from being removed while this is the case. If you say
> that devices can be removed at any time and this is OK, then you should
> not be counting references at all, this is pointless. But I doubt the
> i2c code is currently ready for this.

There are multitude of reference counts, all counting different
things, not one refcount for the entire thing. You have refcount for
the file object, you have memory refcounts, you have device object
refcounts. And note that device and driver bond is not refcounted at
all. It is supposed to be allowed to be broken at [pretty much] any
moment. You just need to be careful when doing so ;)

>
>> > user-space processes have i2c device nodes open. Unprepared hot-unplug
>> > will still go wrong exactly as it goes now.
>>
>> > My point is that prepared hot-unplug can already be achieved today
>> > without any patch.
>>
>> Yeah, if we have the option of stopping the applications before the
>> device is gone.
>>
>> > Or possibly improved by adding a notification
>> > mechanism. But not by changing the reference holding design.
>> >
>> > Not only the proposed patch does not help and degrades the performance,
>> > but it breaks assumptions. For example, it would allow an application
>> > to open an i2c bus, then you remove its driver and load another i2c bus
>> > driver, which gets the same bus number, and now the application writes
>> > to a completely different I2C bus segment. The current reference model
>> > prevents that, on purpose.
>> >
>> > So, again, nack from me.
>>
>> Yeah, the patch wasn't great and I knew it from the beginning. But we
>> are looking for a solution that can be accepted and so need advice
>> from you guys :)
>
> I have no idea, sorry. Hopefully Greg or Wolfram know more? Check what
> other subsystems are doing?

One option is to, upon device removal, to mark it as "dead" to allow
accesses through i2c-dev to return error, release all hardware
resources, but keep the object in memory in zombie state, waiting for
the last user to drop the reference. Once that happens (maybe years
later) you finally can release last bits of memory. You can check how
we do that for input_dev/evdev pair.

Thanks.

-- 
Dmitry

Re: [PATCH 2/2] i2c-dev: Don't block the adapter from unregistering

[Peter Rosin]

On 2016-07-06 04:57, Viresh Kumar wrote:
> The i2c-dev calls i2c_get_adapter() from the .open() callback, which
> doesn't let the adapter device unregister unless the .close() callback
> is called.
> 
> On some platforms (like Google ARA), this doesn't let the modules
> (hardware attached to the phone) eject from the phone as the cleanup
> path for the module hasn't finished yet (i2c adapter not removed).
> 
> We can't let the userspace block the kernel forever in such cases.
> 
> Fix this by calling i2c_get_adapter() from all other file operations,
> i.e.  read/write/ioctl, to make sure the adapter doesn't get away while
> we are in the middle of a operation, but not otherwise. In .open() we
> will release the adapter device before returning and so if there is no
> data transfer in progress, then the i2c-dev doesn't block the adapter
> from unregistering.
> 
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---
>  drivers/i2c/i2c-dev.c | 72 ++++++++++++++++++++++++++++++++++++++++++++++-----
>  include/linux/i2c.h   |  1 +
>  2 files changed, 66 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/i2c/i2c-dev.c b/drivers/i2c/i2c-dev.c
> index 66f323fd3982..b2562603daa9 100644
> --- a/drivers/i2c/i2c-dev.c
> +++ b/drivers/i2c/i2c-dev.c
> @@ -142,13 +142,25 @@ static ssize_t i2cdev_read(struct file *file, char __user *buf, size_t count,
>  	int ret;
>  
>  	struct i2c_client *client = file->private_data;
> +	struct i2c_adapter *adap;
> +
> +	adap = i2c_get_adapter(client->adapter_nr);
> +	if (!adap)
> +		return -ENODEV;
> +
> +	if (adap != client->adapter) {
> +		ret = -EINVAL;
> +		goto put_adapter;
> +	}

I don't see how this can work with the i2c-demux-pinctrl driver.
I also wonder if/how other muxes handle this relaxed adapter
lifetime thingy?

Out of curiosity, why would client->adapter change anyway?
(that is, if not because of a demux-pinctrl op)

Or maybe I'm just paranoid, but this is not obvious...

Cheers,
Peter

Re: [PATCH 2/2] i2c-dev: Don't block the adapter from unregistering

[Peter Rosin]

On 2016-07-06 04:57, Viresh Kumar wrote:
> The i2c-dev calls i2c_get_adapter() from the .open() callback, which
> doesn't let the adapter device unregister unless the .close() callback
> is called.
> 
> On some platforms (like Google ARA), this doesn't let the modules
> (hardware attached to the phone) eject from the phone as the cleanup
> path for the module hasn't finished yet (i2c adapter not removed).
> 
> We can't let the userspace block the kernel forever in such cases.
> 
> Fix this by calling i2c_get_adapter() from all other file operations,
> i.e.  read/write/ioctl, to make sure the adapter doesn't get away while
> we are in the middle of a operation, but not otherwise. In .open() we
> will release the adapter device before returning and so if there is no
> data transfer in progress, then the i2c-dev doesn't block the adapter
> from unregistering.
> 
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---
>  drivers/i2c/i2c-dev.c | 72 ++++++++++++++++++++++++++++++++++++++++++++++-----
>  include/linux/i2c.h   |  1 +
>  2 files changed, 66 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/i2c/i2c-dev.c b/drivers/i2c/i2c-dev.c
> index 66f323fd3982..b2562603daa9 100644
> --- a/drivers/i2c/i2c-dev.c
> +++ b/drivers/i2c/i2c-dev.c
> @@ -142,13 +142,25 @@ static ssize_t i2cdev_read(struct file *file, char __user *buf, size_t count,
>  	int ret;
>  
>  	struct i2c_client *client = file->private_data;
> +	struct i2c_adapter *adap;
> +
> +	adap = i2c_get_adapter(client->adapter_nr);
> +	if (!adap)
> +		return -ENODEV;
> +
> +	if (adap != client->adapter) {
> +		ret = -EINVAL;
> +		goto put_adapter;
> +	}

I don't see how this can work with the i2c-demux-pinctrl driver.
I also wonder if/how other muxes handle this relaxed adapter
lifetime thingy?

Out of curiosity, why would client->adapter change anyway?
(that is, if not because of a demux-pinctrl op)

Or maybe I'm just paranoid, but this is not obvious...

Cheers,
Peter

Re: [PATCH 2/2] i2c-dev: Don't block the adapter from unregistering

[Viresh Kumar]

On 06-07-16, 10:22, Peter Rosin wrote:
> On 2016-07-06 04:57, Viresh Kumar wrote:
> > The i2c-dev calls i2c_get_adapter() from the .open() callback, which
> > doesn't let the adapter device unregister unless the .close() callback
> > is called.
> > 
> > On some platforms (like Google ARA), this doesn't let the modules
> > (hardware attached to the phone) eject from the phone as the cleanup
> > path for the module hasn't finished yet (i2c adapter not removed).
> > 
> > We can't let the userspace block the kernel forever in such cases.
> > 
> > Fix this by calling i2c_get_adapter() from all other file operations,
> > i.e.  read/write/ioctl, to make sure the adapter doesn't get away while
> > we are in the middle of a operation, but not otherwise. In .open() we
> > will release the adapter device before returning and so if there is no
> > data transfer in progress, then the i2c-dev doesn't block the adapter
> > from unregistering.
> > 
> > Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> > ---
> >  drivers/i2c/i2c-dev.c | 72 ++++++++++++++++++++++++++++++++++++++++++++++-----
> >  include/linux/i2c.h   |  1 +
> >  2 files changed, 66 insertions(+), 7 deletions(-)
> > 
> > diff --git a/drivers/i2c/i2c-dev.c b/drivers/i2c/i2c-dev.c
> > index 66f323fd3982..b2562603daa9 100644
> > --- a/drivers/i2c/i2c-dev.c
> > +++ b/drivers/i2c/i2c-dev.c
> > @@ -142,13 +142,25 @@ static ssize_t i2cdev_read(struct file *file, char __user *buf, size_t count,
> >  	int ret;
> >  
> >  	struct i2c_client *client = file->private_data;
> > +	struct i2c_adapter *adap;
> > +
> > +	adap = i2c_get_adapter(client->adapter_nr);
> > +	if (!adap)
> > +		return -ENODEV;
> > +
> > +	if (adap != client->adapter) {
> > +		ret = -EINVAL;
> > +		goto put_adapter;
> > +	}
> 
> I don't see how this can work with the i2c-demux-pinctrl driver.
> I also wonder if/how other muxes handle this relaxed adapter
> lifetime thingy?

I would like to mention here that I am no I2C expert and have limited
knowledge of it :)

I haven't had a look at the muxes implementation earlier, now that I
looked at them, I see that they unregister/register the adapter,
perhaps while switching functionality.

I am not sure though, if this patch will break it or not. And I don't
have a way of testing it out.

> Out of curiosity, why would client->adapter change anyway?
> (that is, if not because of a demux-pinctrl op)

I didn't mean that it will change, and perhaps we can add a
WARN_ON(adap != client->adapter).

But, thinking about it again now, I think it is possible.

What about this sequence:

- i2c-adap-register (address P1)
- .open(), client->adapter = P1;
- .read/write/ioctl()..
- i2c-adap-unregister (adapter freed)
- i2c-adap-register with same adapter_nr (address P2);
- .read/write/ioctl().

Wouldn't the address differ here ?

-- 
viresh

Re: [PATCH 2/2] i2c-dev: Don't block the adapter from unregistering

[Lars-Peter Clausen]

On 07/06/2016 04:33 PM, Viresh Kumar wrote:
> On 06-07-16, 10:22, Peter Rosin wrote:
>> On 2016-07-06 04:57, Viresh Kumar wrote:
>>> The i2c-dev calls i2c_get_adapter() from the .open() callback, which
>>> doesn't let the adapter device unregister unless the .close() callback
>>> is called.
>>>
>>> On some platforms (like Google ARA), this doesn't let the modules
>>> (hardware attached to the phone) eject from the phone as the cleanup
>>> path for the module hasn't finished yet (i2c adapter not removed).
>>>
>>> We can't let the userspace block the kernel forever in such cases.
>>>
>>> Fix this by calling i2c_get_adapter() from all other file operations,
>>> i.e.  read/write/ioctl, to make sure the adapter doesn't get away while
>>> we are in the middle of a operation, but not otherwise. In .open() we
>>> will release the adapter device before returning and so if there is no
>>> data transfer in progress, then the i2c-dev doesn't block the adapter
>>> from unregistering.
>>>
>>> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
>>> ---
>>>  drivers/i2c/i2c-dev.c | 72 ++++++++++++++++++++++++++++++++++++++++++++++-----
>>>  include/linux/i2c.h   |  1 +
>>>  2 files changed, 66 insertions(+), 7 deletions(-)
>>>
>>> diff --git a/drivers/i2c/i2c-dev.c b/drivers/i2c/i2c-dev.c
>>> index 66f323fd3982..b2562603daa9 100644
>>> --- a/drivers/i2c/i2c-dev.c
>>> +++ b/drivers/i2c/i2c-dev.c
>>> @@ -142,13 +142,25 @@ static ssize_t i2cdev_read(struct file *file, char __user *buf, size_t count,
>>>  	int ret;
>>>  
>>>  	struct i2c_client *client = file->private_data;
>>> +	struct i2c_adapter *adap;
>>> +
>>> +	adap = i2c_get_adapter(client->adapter_nr);
>>> +	if (!adap)
>>> +		return -ENODEV;
>>> +
>>> +	if (adap != client->adapter) {
>>> +		ret = -EINVAL;
>>> +		goto put_adapter;
>>> +	}
>>
>> I don't see how this can work with the i2c-demux-pinctrl driver.
>> I also wonder if/how other muxes handle this relaxed adapter
>> lifetime thingy?
> 
> I would like to mention here that I am no I2C expert and have limited
> knowledge of it :)
> 
> I haven't had a look at the muxes implementation earlier, now that I
> looked at them, I see that they unregister/register the adapter,
> perhaps while switching functionality.
> 
> I am not sure though, if this patch will break it or not. And I don't
> have a way of testing it out.
> 
>> Out of curiosity, why would client->adapter change anyway?
>> (that is, if not because of a demux-pinctrl op)
> 
> I didn't mean that it will change, and perhaps we can add a
> WARN_ON(adap != client->adapter).
> 
> But, thinking about it again now, I think it is possible.
> 
> What about this sequence:
> 
> - i2c-adap-register (address P1)
> - .open(), client->adapter = P1;
> - .read/write/ioctl()..
> - i2c-adap-unregister (adapter freed)
> - i2c-adap-register with same adapter_nr (address P2);
> - .read/write/ioctl().
> 
> Wouldn't the address differ here ?

There is no guarantee that the address will be different. While it is
unlikely the memory allocated might give out the same address for the second
adapter if the first one has been freed.

Re: [PATCH 2/2] i2c-dev: Don't block the adapter from unregistering

[Peter Rosin]

On 2016-07-06 16:43, Lars-Peter Clausen wrote:
> On 07/06/2016 04:33 PM, Viresh Kumar wrote:
>> On 06-07-16, 10:22, Peter Rosin wrote:
>>> On 2016-07-06 04:57, Viresh Kumar wrote:
>>>> The i2c-dev calls i2c_get_adapter() from the .open() callback, which
>>>> doesn't let the adapter device unregister unless the .close() callback
>>>> is called.
>>>>
>>>> On some platforms (like Google ARA), this doesn't let the modules
>>>> (hardware attached to the phone) eject from the phone as the cleanup
>>>> path for the module hasn't finished yet (i2c adapter not removed).
>>>>
>>>> We can't let the userspace block the kernel forever in such cases.
>>>>
>>>> Fix this by calling i2c_get_adapter() from all other file operations,
>>>> i.e.  read/write/ioctl, to make sure the adapter doesn't get away while
>>>> we are in the middle of a operation, but not otherwise. In .open() we
>>>> will release the adapter device before returning and so if there is no
>>>> data transfer in progress, then the i2c-dev doesn't block the adapter
>>>> from unregistering.
>>>>
>>>> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
>>>> ---
>>>>  drivers/i2c/i2c-dev.c | 72 ++++++++++++++++++++++++++++++++++++++++++++++-----
>>>>  include/linux/i2c.h   |  1 +
>>>>  2 files changed, 66 insertions(+), 7 deletions(-)
>>>>
>>>> diff --git a/drivers/i2c/i2c-dev.c b/drivers/i2c/i2c-dev.c
>>>> index 66f323fd3982..b2562603daa9 100644
>>>> --- a/drivers/i2c/i2c-dev.c
>>>> +++ b/drivers/i2c/i2c-dev.c
>>>> @@ -142,13 +142,25 @@ static ssize_t i2cdev_read(struct file *file, char __user *buf, size_t count,
>>>>  	int ret;
>>>>  
>>>>  	struct i2c_client *client = file->private_data;
>>>> +	struct i2c_adapter *adap;
>>>> +
>>>> +	adap = i2c_get_adapter(client->adapter_nr);
>>>> +	if (!adap)
>>>> +		return -ENODEV;
>>>> +
>>>> +	if (adap != client->adapter) {
>>>> +		ret = -EINVAL;
>>>> +		goto put_adapter;
>>>> +	}
>>>
>>> I don't see how this can work with the i2c-demux-pinctrl driver.
>>> I also wonder if/how other muxes handle this relaxed adapter
>>> lifetime thingy?
>>
>> I would like to mention here that I am no I2C expert and have limited
>> knowledge of it :)
>>
>> I haven't had a look at the muxes implementation earlier, now that I
>> looked at them, I see that they unregister/register the adapter,
>> perhaps while switching functionality.
>>
>> I am not sure though, if this patch will break it or not. And I don't
>> have a way of testing it out.
>>
>>> Out of curiosity, why would client->adapter change anyway?
>>> (that is, if not because of a demux-pinctrl op)
>>
>> I didn't mean that it will change, and perhaps we can add a
>> WARN_ON(adap != client->adapter).
>>
>> But, thinking about it again now, I think it is possible.
>>
>> What about this sequence:
>>
>> - i2c-adap-register (address P1)
>> - .open(), client->adapter = P1;
>> - .read/write/ioctl()..
>> - i2c-adap-unregister (adapter freed)
>> - i2c-adap-register with same adapter_nr (address P2);
>> - .read/write/ioctl().
>>
>> Wouldn't the address differ here ?
> 
> There is no guarantee that the address will be different. While it is
> unlikely the memory allocated might give out the same address for the second
> adapter if the first one has been freed.

Exactly, so the stored address had better be correct, and in
that case there is no need for the new adapter_nr in every
client, you could just go with client->adapter->nr instead.
Which just shows that the whole thing is fishy and that the
adapter has to remain alive. BTW, is there any guarantee that
adapter numbers will not get reused?

Cheers,
Peter

Re: [PATCH 2/2] i2c-dev: Don't block the adapter from unregistering

[Peter Rosin]

On 2016-07-06 16:43, Lars-Peter Clausen wrote:
> On 07/06/2016 04:33 PM, Viresh Kumar wrote:
>> On 06-07-16, 10:22, Peter Rosin wrote:
>>> On 2016-07-06 04:57, Viresh Kumar wrote:
>>>> The i2c-dev calls i2c_get_adapter() from the .open() callback, which
>>>> doesn't let the adapter device unregister unless the .close() callback
>>>> is called.
>>>>
>>>> On some platforms (like Google ARA), this doesn't let the modules
>>>> (hardware attached to the phone) eject from the phone as the cleanup
>>>> path for the module hasn't finished yet (i2c adapter not removed).
>>>>
>>>> We can't let the userspace block the kernel forever in such cases.
>>>>
>>>> Fix this by calling i2c_get_adapter() from all other file operations,
>>>> i.e.  read/write/ioctl, to make sure the adapter doesn't get away while
>>>> we are in the middle of a operation, but not otherwise. In .open() we
>>>> will release the adapter device before returning and so if there is no
>>>> data transfer in progress, then the i2c-dev doesn't block the adapter
>>>> from unregistering.
>>>>
>>>> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
>>>> ---
>>>>  drivers/i2c/i2c-dev.c | 72 ++++++++++++++++++++++++++++++++++++++++++++++-----
>>>>  include/linux/i2c.h   |  1 +
>>>>  2 files changed, 66 insertions(+), 7 deletions(-)
>>>>
>>>> diff --git a/drivers/i2c/i2c-dev.c b/drivers/i2c/i2c-dev.c
>>>> index 66f323fd3982..b2562603daa9 100644
>>>> --- a/drivers/i2c/i2c-dev.c
>>>> +++ b/drivers/i2c/i2c-dev.c
>>>> @@ -142,13 +142,25 @@ static ssize_t i2cdev_read(struct file *file, char __user *buf, size_t count,
>>>>  	int ret;
>>>>  
>>>>  	struct i2c_client *client = file->private_data;
>>>> +	struct i2c_adapter *adap;
>>>> +
>>>> +	adap = i2c_get_adapter(client->adapter_nr);
>>>> +	if (!adap)
>>>> +		return -ENODEV;
>>>> +
>>>> +	if (adap != client->adapter) {
>>>> +		ret = -EINVAL;
>>>> +		goto put_adapter;
>>>> +	}
>>>
>>> I don't see how this can work with the i2c-demux-pinctrl driver.
>>> I also wonder if/how other muxes handle this relaxed adapter
>>> lifetime thingy?
>>
>> I would like to mention here that I am no I2C expert and have limited
>> knowledge of it :)
>>
>> I haven't had a look at the muxes implementation earlier, now that I
>> looked at them, I see that they unregister/register the adapter,
>> perhaps while switching functionality.
>>
>> I am not sure though, if this patch will break it or not. And I don't
>> have a way of testing it out.
>>
>>> Out of curiosity, why would client->adapter change anyway?
>>> (that is, if not because of a demux-pinctrl op)
>>
>> I didn't mean that it will change, and perhaps we can add a
>> WARN_ON(adap != client->adapter).
>>
>> But, thinking about it again now, I think it is possible.
>>
>> What about this sequence:
>>
>> - i2c-adap-register (address P1)
>> - .open(), client->adapter = P1;
>> - .read/write/ioctl()..
>> - i2c-adap-unregister (adapter freed)
>> - i2c-adap-register with same adapter_nr (address P2);
>> - .read/write/ioctl().
>>
>> Wouldn't the address differ here ?
> 
> There is no guarantee that the address will be different. While it is
> unlikely the memory allocated might give out the same address for the second
> adapter if the first one has been freed.

Exactly, so the stored address had better be correct, and in
that case there is no need for the new adapter_nr in every
client, you could just go with client->adapter->nr instead.
Which just shows that the whole thing is fishy and that the
adapter has to remain alive. BTW, is there any guarantee that
adapter numbers will not get reused?

Cheers,
Peter

Re: [PATCH 2/2] i2c-dev: Don't block the adapter from unregistering

[Viresh Kumar]

On 06-07-16, 17:04, Peter Rosin wrote:
> Exactly, so the stored address had better be correct, and in

No.

> that case there is no need for the new adapter_nr in every
> client, you could just go with client->adapter->nr instead.

client->adapter may be a dangling pointer at this point if the adapter
is freed, so we can't use that blindly for sure.

> Which just shows that the whole thing is fishy and that the
> adapter has to remain alive. BTW, is there any guarantee that
> adapter numbers will not get reused?

We are allocating them from idr and that will reuse them once they get
freed.

-- 
viresh

Re: [PATCH 2/2] i2c-dev: Don't block the adapter from unregistering

[Viresh Kumar]

On 06-07-16, 16:43, Lars-Peter Clausen wrote:
> There is no guarantee that the address will be different. While it is
> unlikely the memory allocated might give out the same address for the second
> adapter if the first one has been freed.

Oh yeah, thanks for correcting me :)

-- 
viresh

Re: [PATCH 0/2] i2c-dev: Don't let userspace block adapter

[Lars-Peter Clausen]

On 07/06/2016 04:57 AM, Viresh Kumar wrote:
> Hi Wolfram/Jean,
> 
> I am part of the kernel team for Google's projectara [1], where we are
> building a module smart phone.
> 
> This series tries to fix one of the problems we hit on our system as we
> are required to hotplug pretty much every thing on the phone and so this
> fixes hotplug issues with i2c-dev.
> 
> As described in the second patch, the current implementation of i2c-dev
> file operations doesn't let the modules (hardware attached to the phone)
> eject from the phone as the cleanup path for the module hasn't finished
> yet (i2c adapter not removed).
> 
> We can't let the userspace block the kernel devices forever in such
> cases.
> 
> I was able to test them on the ARA phone with kernel 3.10 only and not
> mainline.

This sounds like you want hot-unplug. This is currently not support by the
I2C framework for adapters. A better approach compared to this series might
be to implement full hot-unplug support for I2C adapters. This will probably
also be useful for additional usecases.

Re: [PATCH 0/2] i2c-dev: Don't let userspace block adapter

[Viresh Kumar]

On 06-07-16, 16:41, Lars-Peter Clausen wrote:
> On 07/06/2016 04:57 AM, Viresh Kumar wrote:
> > Hi Wolfram/Jean,
> > 
> > I am part of the kernel team for Google's projectara [1], where we are
> > building a module smart phone.
> > 
> > This series tries to fix one of the problems we hit on our system as we
> > are required to hotplug pretty much every thing on the phone and so this
> > fixes hotplug issues with i2c-dev.
> > 
> > As described in the second patch, the current implementation of i2c-dev
> > file operations doesn't let the modules (hardware attached to the phone)
> > eject from the phone as the cleanup path for the module hasn't finished
> > yet (i2c adapter not removed).
> > 
> > We can't let the userspace block the kernel devices forever in such
> > cases.
> > 
> > I was able to test them on the ARA phone with kernel 3.10 only and not
> > mainline.
> 
> This sounds like you want hot-unplug. This is currently not support by the
> I2C framework for adapters. A better approach compared to this series might
> be to implement full hot-unplug support for I2C adapters. This will probably
> also be useful for additional usecases.

Yeah, we need hot-unplug.

Hmm, doing that would require more knowledge of the framework and I am
afraid I don't have it right now, not that it can't be done :)

-- 
viresh