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@xxxxxxxxxx>
> 
> 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