On 7/15/08, Jean Delvare <[EMAIL PROTECTED]> wrote: > Hi Jon, > > > On Tue, 15 Jul 2008 13:01:07 -0400, Jon Smirl wrote: > > On 7/15/08, Jean Delvare <[EMAIL PROTECTED]> 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? > > > In general no, but there is a race where this can actually happen. > > > > Would combining i2c-dev into i2c core and integrating it > > with the core's lock protection make things easier to lock? > > > Definitely. That's something I want to do for other reasons anyway. > > > > 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? > > > i2c-dev will probably soon be the last user of > i2c_adapter.attach_adapter and i2c_adapter.detach_adapter, and this is > indeed one of my incentives to look into how things could be rearranged. > > > > 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. > > > I doubt we can remove 3 kB just by moving things around. The > book-keeping is relatively small. But this isn't my goal anyway. My > goal is to make things cleaner and possibly safer. > > One thing we may do if size is a concern, is keep the functional part > of i2c-dev in its separate module, and only merge the book-keeping and > char device creation into i2c-core. > > > > 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? > > > What do you mean by "device"? The i2c bus, or a specific i2c device > (client)? The i2c bus can be used by any amount of users, in-kernel or > user-space. For i2c devices, it's more complex, depending on whether an > i2c_client is registered or not. Only one i2c_client can be registered > at a given address, but nothing prevents "raw" accesses to the devices. > i2c-dev makes such "raw" accesses, even though it has an initial check > whether the i2c address is in used by a registered client or not. This > needs to be cleaned up from the ground up, the current approach is not > satisfactory at all and things are getting worse with the advent of > new-style i2c devices, because the have a different notion of business. >
My idea was that when a client registers it would indicate whether or not it wants a user space device. Most in-kernel clients would not want a user space device. A sysfs attribute could force the creation of a user space device for debugging. You might also want to make a global i2c sysfs attribute that you would echo an i2c address into. Doing that would cause a user space device to be created for that address. Now you can manipulate devices with no in-kernel driver. Of course we can't stop raw access but life is simpler if everyone avoids doing it. -- Jon Smirl [EMAIL PROTECTED] _______________________________________________ i2c mailing list [email protected] http://lists.lm-sensors.org/mailman/listinfo/i2c
