Hi Viresh, On Wed, 6 Jul 2016 13:55:40 -0700, Viresh Kumar wrote: > On 06-07-16, 19:12, Jean Delvare wrote: > > Well well... I don't like this patch at all to be honest. > > Sure, I didn't like it much as well. I just wanted people to comment on what > else we can do here. We don't really want to add out-of-mainline stuff here. > > > 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. > > We thought that buggy userspace shouldn't be allowed to get kernel into trouble. > Isn't that the case ? Buggy user-space run by root can cause any kind of trouble. And the point being discussed here is amongst the most minor of these. But I even disagree with calling it buggy. > In our case this is what happens: > - userspace opens the file descriptor > - we try to forcefully remove the module from phone (that doesn't talk to > userspace to stop using the device). > - The module doesn't get ejected unless the app closes the fd. So you are basically building a test case to cause the problem. It's artificial. The adapter reference being held while the device node is open isn't a bug, it is a design decision. I would consider revisiting that design if there was a real world case where it causes trouble, but not for an artificially created test case. I don't see anything fundamentally wrong in the design anyway. I do not expect to be allowed to remove a hard disk drive from my system while its partitions are mounted, and I don't expect to be able to unmount partitions while users have files opened on them. You always have to tear things down in the right order. Same here. > > Do you have user-space i2c device drivers on your system? Which ones, > > No. Its probably an app written by some of our module app developers. > > > and why (I would expect all useful i2c devices to have a kernel > > driver.) > > That's what we have. Still no details here. What app, what is it doing, to what device is it talking, why is it not a kernel driver, and why do they keep the device node opened all the time? > > Requesting and freeing the i2c adapter for every transaction is going > > Well, we are just finding it (taking a reference of it) and the dropping its > reference. Yes, that's what I meant, sorry for using the wrong terms. > > to add a lot of overhead to all existing tools :-( > > :( i2cdump typically runs 258 ioctls on the device node, i2cdetect 235. i2c_get_adapter isn't cheap. Multiple function calls, mutex locking/unlocking, preemption disabling/enabling... You don't want do to that repeatedly if it can be avoided. So, nack from me. > > 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. > > Hmmm. The problem is that they weren't told when the module tries to go away and > so they don't know that they need to close the fd. See my previous questions. We still don't know why they are doing what they are doing in user-space, nor why they think they have to keep the device node opened. They could always kill the application in question with: # fuser -k /dev/i2c-* before removing the module. Or find a more polite way to tell the application to quit. If they want to do it in user-space, they have to do it right. -- Jean Delvare SUSE L3 Support -- To unsubscribe from this list: send the line "unsubscribe linux-i2c" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
