Re: [PATCH 2/2] i2c-dev: Don't block the adapter from unregistering

[Viresh Kumar]

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