Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Mon, 24 Feb 2020 12:22:06 +0100
From:   Boris Brezillon <boris.brezillon@collabora.com>
To:     Vitor Soares <Vitor.Soares@synopsys.com>
Cc:     "linux-kernel@vger.kernel.org" <linux-kernel@vger.kernel.org>,
        "linux-i3c@lists.infradead.org" <linux-i3c@lists.infradead.org>,
        Jose Abreu <Jose.Abreu@synopsys.com>,
        "corbet@lwn.net" <corbet@lwn.net>,
        Joao Pinto <Joao.Pinto@synopsys.com>,
        "arnd@arndb.de" <arnd@arndb.de>,
        "wsa@the-dreams.de" <wsa@the-dreams.de>,
        "gregkh@linuxfoundation.org" <gregkh@linuxfoundation.org>,
        "bbrezillon@kernel.org" <bbrezillon@kernel.org>,
        "broonie@kernel.org" <broonie@kernel.org>
Subject: Re: [PATCH v3 4/5] i3c: add i3cdev module to expose i3c dev in /dev
Message-ID: <20200224122206.10984b3f@collabora.com>
In-Reply-To: <CH2PR12MB42163351853CCC029D28164BAEEC0@CH2PR12MB4216.namprd12.prod.outlook.com>
References: <cover.1582069402.git.vitor.soares@synopsys.com>
        <e093ae9da81e7702c188a20d1e8b9d7f8024bfeb.1582069402.git.vitor.soares@synopsys.com>
        <20200221233216.3b2038f8@collabora.com>
        <CH2PR12MB42163351853CCC029D28164BAEEC0@CH2PR12MB4216.namprd12.prod.outlook.com>
Organization: Collabora
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Mon, 24 Feb 2020 11:04:50 +0000
Vitor Soares <Vitor.Soares@synopsys.com> wrote:

> Hi Boris,
> 
> From: Boris Brezillon <boris.brezillon@collabora.com>
> Date: Fri, Feb 21, 2020 at 22:32:16
> 
> > On Wed, 19 Feb 2020 01:20:42 +0100
> > Vitor Soares <Vitor.Soares@synopsys.com> wrote:
> >   
> > > +static int i3cdev_detach(struct device *dev, void *dummy)
> > > +{
> > > +	struct i3cdev_data *i3cdev;
> > > +	struct i3c_device *i3c;
> > > +
> > > +	if (dev->type == &i3c_masterdev_type)
> > > +		return 0;
> > > +
> > > +	i3c = dev_to_i3cdev(dev);
> > > +
> > > +	i3cdev = i3cdev_get_drvdata(i3c);
> > > +	if (!i3cdev)
> > > +		return 0;
> > > +
> > > +	/* Prevent transfers while cdev removal */
> > > +	mutex_lock(&i3cdev->xfer_lock);
> > > +	cdev_del(&i3cdev->cdev);  
> > 
> > When cdev_del() returns there might be opened FDs pointing to your
> > i3cdev [1] ...  
> 
> Yes, I know. I protected the driver part but I missed the 
> file->private_data.

Not sure what you mean by protection, but if you meant locking, then
it's not enough: you need to refcnt the struct if you want to prevent
use-after-free situations.

BTW, I had a closer look at the usbdev implementation, and maybe you
should base yours on usb instead of i2c. They seem to register a cdev at
module init time, and add a dev_t per device at device registration
time. Not sure how they handle the userspace-driver vs kernel-driver
concurrency, but maybe returning EACCES (or EBUSY) instead of
detaching/attaching the i3cdev everytime a device is bound/unbound
would be simpler.

Also, I think Arnd was right, it'd be simpler if i3cdev support was
integrated to the core (still left as a option so it can be disabled,
but with a dedicated i3cdev field in i3c_device instead hijacking the
driver private field).