[i2c] [patch 2.6.23-rc9] i2c-dev "how does it work" comments

David Brownell david-b at pacbell.net
Fri Oct 12 20:45:13 CEST 2007


On Wednesday 10 October 2007, Jean Delvare wrote:
> Hi David,
> 
> On Sun, 07 Oct 2007 13:31:56 -0700, David Brownell wrote:
> > > > After making sense of this code, I think that the anonymous i2c_client
> > > > is pretty shady.
> > >
> > > Agreed. The fact that it isn't registered makes the i2c-dev interface
> > > pretty unsafe to use. Concurrent i2c-dev accesses are not detected,
> > 
> > That's potentially trouble, but the usual way to handle it is by a
> > "single open" rule.  I'd think it would be better applied at the
> > level of i2c_client nodes than i2c_adapter nodes; but /dev/i2c-X
> > nodes are for the latter, not the former.  And then there's the
> > issue of I2C_RDWR transactions involving multiple I2C slaves; any
> > such "single open" rule should accommodate such operations.
> 
> I see no possible "single open" rule applying here, as the device nodes
> are for whole buses rather than individual I2C devices. Forbidding
> concurrent accesses through i2c-dev to a given i2c bus is too
> restrictive, and doesn't even solve the problem (conflicts can still
> happen between the single i2c-dev instance and kernel drivers.)

There could be a "single open" rule applying to I2C_SLAVE{,_FORCE}
though.  Agreed that applying it at the /dev/i2c-X level would be
impractical ... that'd essentially be a new API.

Today's behavior is evidently buggy, since the i2c_client is neither
allocated on a per-FD basis nor is it registered.  So if two apps
open /dev/i2c-0 and use I2C_SLAVE with different addresses ...


> The I2C_RDRW problem does exist beyond i2c-dev: i2c_transfer has the
> exact same problem, it can be used to access an otherwise registered
> I2C device. My feeling is that i2c_transfer() should be an
> internal-only function, but so many V4L, DVB and RTC drivers rely on it
> that it's probably no longer an option.

They rely on it because there's no alternative.  Provide an
alternative -- maybe something like an

	int i2c_combined(struct i2c_client *client,
			struct i2c_msg *msgs,
			size_t nmsg);

returning something sane (errno or count), and new code could use
that instead of i2c_transfer().  Old code could start to convert.

The implementation could stuff the addressing info into the messages,
then call do what i2c_transfer() does.


> > > and an in-kernel i2c_client can be registered at the same address as an
> > > i2c-dev user
> > 
> > That's not necessarily a problem;  I can easily imagine cases where
> > the kernel driver might want to be pretty minimal, and rely on some
> > userspace code for much of the work.
> > 
> > For example, the kernel code is the only practical place to handle
> > SMBALERT# and host notifications, but a PMBus slave might have from
> > one to a few hundred attributes, in different pages.  I suspect it
> > would be better to have userspace tell the driver which attributes
> > to create, rather than have a different driver for each type of
> > slave ... and in some cases that will need to involve querying a
> > device which already has a kernel driver.
> 
> I don't like this as all. We don't have any way for i2c-dev and a
> kernel driver to synchronize in such a situation. My feeling is that
> each device should be handled by a single driver, and that driver
> should be either in user-space or in the kernel.

Maybe i2c-dev should't be the userspace component, but I really
don't see a way around splitting responsibilities between kernel
and userspace.  In fact, such splits are pretty standard.  You
can quibble about labels (which is the "driver"), but in the big
picture that's just noise.

Anything using hwmon has a user/kernel split; and all hwmon code
I've ever seen looks like it's just a very simple version of what
PMBus is targetting...


> > >	as long as i2c-dev picks it first. I believe that the
> > > i2c_client used by i2c_dev should be registered.
> > 
> > I'd like to see that too.  But then, what about combined messages
> > with more than one slave involved?
> 
> Good question. I've never seen this used in practice.

Me either, but then the ability to do that is very explicitly
part of the PMBus spec.  Hosts need to coordinate the actions of
multiple power converters, and the way they do that is send a
bunch of commands and rely on the STOP to trigger those actions
among several devices.

Yeah, that's likely a bit "forward looking", but on the other
hand that's the kind of stuff any "what should the framework be
able to do" discussion needs to consider.


> >                                     Or other messages sent to a
> > slave which isn't the one associated with that i2c_client?
> 
> I don't remember seeing this used in practice either. User-space
> programs either use only I2C_RDRW and/or direct reads/writes, or only
> slave-based access, typically they don't mix.

The way I look at it, there should be calls which are explicitly
targetted at single devices -- including all current smbus calls,
and some TBD i2c_combined() call -- and separate infrastructure
for multi-device operations.  That separate stuff might as well
be today's i2c_transfer().

The reason for calls targetted at single devices is basically the
classic "principle of least surprise".  The i2c_client has been
traditionally sort of a bastard stepchild, which is a surprise
since the standard model for Linux drivers revolves around handles
to a single device.  We took one step by allowing probe()/remove()
style drivers.  Another -- easier!! -- step starts moving away
from the i2c_transfer() model, towards one using i2c_client.

 
> >                                                             Both
> > of those work today with I2C_RDWR, and that functionality should
> > remain...
> 
> As the functionality doesn't appear to be needed nor used, I don't
> really care if whatever we do to address the real issues happens to
> break this.

See above.  So long as "single device" and "multi-device" I/O are
both still possible, I won't much object to binary incompatibility
for the latter.


> > >	The user-space
> > > i2c tools (i2cdetect, i2cdump...) have been using it unconditionally
> > > for years, but I recently changed them to use I2C_SLAVE by default
> > > instead.
> > 
> > I'm surprised they ever used FORCE by default!  At any rate,
> > that issue is not a *kernel* bug.
> 
> It wasn't, but now it is: i2c-dev doesn't deal properly with new-style
> devices, as you found out yourself.

With *unbound* ones, from what you said before.  Though if you're
going to talk about buggy behaviors there, I think the "anonymous"
i2c_client handles (shared between FDs!) is much more worrisome.

 
> > >	I want the users to realize that what they are asking for is
> > > potentially dangerous and unsafe. Which means that we should change the
> > > i2c-dev code so that I2C_SLAVE_FORCE is not needed to access unbound
> > > new-style i2c devices from user-space (that's what the attached patch
> > > is doing.)
> > 
> > That makes sense.  Though it's not what a doc-only patch would do.
> 
> Agreed, I didn't mean to merge that change into your patch. I just
> wanted to let you comment on it. I can't remember why I did not push it
> yet, probably there is some underlying problem that needs fixing first.

Maybe it didn't get much discussion?  Or maybe you wanted to do a
better coupling between i2c_client and the i2c-dev file descriptor
after the I2C_SLAVE (or I2C_SLAVE_FORCE) request was made.

The cleanest way to handle this would be to have the i2c_client bound
to a real i2c_driver in all cases, not just for i2c_client nodes set
up for use with new-style drivers.  (I can't find your email just now
to see what you actually did.)


> > > > @@ -170,8 +198,18 @@ static int i2cdev_ioctl(struct inode *in
> > > >  		cmd, arg);
> > > >  
> > > >  	switch ( cmd ) {
> > > > -	case I2C_SLAVE:
> > >
> > > Why are you switching I2C_SLAVE and I2C_SLAVE_FORCE? Doesn't make sense
> > > to me, especially as the comment below applies to both.
> > 
> > Maybe some parts do, but I was focussing on the fact that to talk
> > to *ANY* new-style device the FORCE mode is required.
> 
> It's probably not worth insisting on something which shouldn't be and
> which we want to fix quickly ;) And anyway, that comment is as much
> about I2C_SLAVE than about I2C_SLAVE_FORCE. So please revert that
> change (or I'll do.) Splitting both cases with that big comment adds to
> confusion more than it helps, methinks.

Why don't you do that.


> > > >  	case I2C_SLAVE_FORCE:
> > > > +		/* NOTE:  devices set up to work with "new style" drivers
> > > > +		 * can't use I2C_SLAVE, even when the device node is not
> > > > +		 * bound to a driver.  Only I2C_SLAVE_FORCE will work.
> > > > +		 *
> > > > +		 * Setting PEC or TENBIT flags won't affect kernel drivers,
> > > > +		 * which will be using the i2c_client node registered with
> > > > +		 * the driver model core.  Likewise, when that client has
> > > > +		 * one of those flags already set, the i2c-dev driver won't
> > > > +		 * see (or use) those settings.
> > >
> > > This comment makes sense for PEC but _not_ for TENBIT.
> > 
> > How so?  Both are consequences of the non-registration, whereby
> > the i2c-X device nodes have their own client flags.
> 
> No. If the i2c-dev client was shared with the in-kernel client for a
> given SMBus device, then the PEC flag would be shared as well, this is
> correct. But the TENBIT flag is not a run-time attribute of the client,

Except ... today, it *is*.

> you can't set it or clear it after the client has been created; it's
> part of the client's address itself. Thus, even if the kernel code was
> perfect, an i2c-dev client with address 0x42 and TENBIT set would still
> not share anything with an in-kernel client with address 0x42 and
> TENBIT not set: they are truly different clients, for truly different
> devices on the bus.

That should all be true some day.  But that day isn't today...

I think the best way to handle that will likely be to add an extra
bit to the address when it's TENBIT, so most of the code wouldn't need
to change.  Maybe just set bit 12, so it's easier to ignore that bit
in a hex version of the address.

- Dave




More information about the i2c mailing list