Re: i2c: cdev lock_kernel() pushdown

[Ben Dooks <ben-linux-elnMNo+KYs3YtjvyW6yDsg@public.gmane.org>]

On Tue, Jul 15, 2008 at 01:01:07PM -0400, Jon Smirl wrote:
> On 7/15/08, Jean Delvare <khali-PUYAD+kWke1g9hUCZPvPmw@public.gmane.org> wrote:
> > On Tue, 15 Jul 2008 10:14:06 -0600, Jonathan Corbet wrote:
> >  > Hi, Jean,
> >  >
> >  > > I am looking at this patch of yours:
> >  > > http://git.kernel.org/?p=linux/kernel/git/torvalds/linux-2.6.git;a=commitdiff;h=3db633ee352bfe20d4a2b0c3c8a46ce31a6c7149
> >  > >
> >  > > I believe that no locking is needed in i2cdev_open(). Do you have any
> >  > > reason to think it does? If not, can I simply revert this patch?
> >  >
> >  > Before now, i2cdev_open() has always had the protection of the BKL.
> >  > When I pushed that locking down into the individual open() functions, I
> >  > really had to take a pretty conservative approach and assume that the
> >  > BKL was needed unless that was really obviously not the case.  In
> >  > i2cdev_open(), for example, there's:
> >  >
> >  >       i2c_dev = i2c_dev_get_by_minor(minor);
> >  >
> >  > and I really don't know what keeps *i2c_dev from going away during the
> >  > rest of the call.  I'm *not* saying there is a problem here; I just
> >  > don't know.  So the only thing I could really do is to push the BKL
> >  > down and let the maintainers sort it out.
> >  >
> >  > ...all of which is my long-winded way of saying that, if you're
> >  > convinced that i2cdev_open() is safe in the absence of the BKL, feel
> >  > free to take it out.
> >
> >
> > Good point. i2c_dev is guaranteed to stay thanks to the call to
> >  i2c_get_adapter(), however it happens _after_ the call to
> >  i2c_dev_get_by_minor(), so without additional locking, this is racy.
> >  That being said, I'm not sure how lock_kernel() is supposed to help. Is
> >  the BKL held when i2cdev_detach_adapter() is called? If not, then I
> >  suspect that the current code is already racy.
> >
> >  I'll look into this, thanks for the hint.
> 
> Is i2c-dev vulnerable to the i2c device disappearing, for example
> rmmod it? Would combining i2c-dev into i2c core and integrating it
> with the core's lock protection make things easier to lock? You could
> make a compile time option to remove it for small systems. If it's in
> the core is attach/detach adapter still needed? I haven't looked at
> any of this in detail, but i2c-dev is only 6K of code. Half of the 6K
> might disappear if integrated into the core.

The i2c-dev code calls i2c_get_adapter() op open, so as long as the
device stays open the adapter should not be able to go away as
i2c_get_adater() calls try_module_get() on the adapter's module.

The i2c-dev has it's own THIS_MODULE in the fops field, so should
be kept as long as there is a file open.
 
> What happens if user space and an in-kernel user both try using the
> device? I've never tried doing that. Should the presence of an
> in-kernel user make the user space device disappear?
> 
> -- 
> Jon Smirl
> jonsmirl-Re5JQEeQqe8AvxtiuMwx3w@public.gmane.org
> 
> _______________________________________________
> i2c mailing list
> i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
> http://lists.lm-sensors.org/mailman/listinfo/i2c

-- 
Ben (ben-elnMNo+KYs3YtjvyW6yDsg@public.gmane.org, http://www.fluff.org/)

  'a smiley only costs 4 bytes'

_______________________________________________
i2c mailing list
i2c-GZX6beZjE8VD60Wz+7aTrA@public.gmane.org
http://lists.lm-sensors.org/mailman/listinfo/i2c