From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754823AbcGFOnZ (ORCPT ); Wed, 6 Jul 2016 10:43:25 -0400 Received: from www381.your-server.de ([78.46.137.84]:50532 "EHLO www381.your-server.de" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753724AbcGFOnY (ORCPT ); Wed, 6 Jul 2016 10:43:24 -0400 Subject: Re: [PATCH 2/2] i2c-dev: Don't block the adapter from unregistering To: Viresh Kumar , Peter Rosin References: <021486be2f5425ce2379219a7ac163ee14ba2aba.1467772840.git.viresh.kumar@linaro.org> <415dcaa7-8ad9-492e-5c5c-605173bc4345@axentia.se> <20160706143323.GK2671@ubuntu> Cc: Wolfram Sang , Jean Delvare , linux-i2c@vger.kernel.org, linux-kernel@vger.kernel.org, gregkh@linuxfoundation.org, Johan Hovold , Alex Elder From: Lars-Peter Clausen Message-ID: <577D1907.20002@metafoo.de> Date: Wed, 6 Jul 2016 16:43:19 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Icedove/38.8.0 MIME-Version: 1.0 In-Reply-To: <20160706143323.GK2671@ubuntu> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 7bit X-Authenticated-Sender: lars@metafoo.de Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 >>> --- >>> 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.