| Message ID | 021486be2f5425ce2379219a7ac163ee14ba2aba.1467772840.git.viresh.kumar@linaro.org |
|---|---|
| State | New |
| Headers | show |
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 -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 */
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(-) -- 2.7.4 -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html