[RFC][2.6] Additional i2c adapter flags for i2c client isolation
Michael Hunold
the DVB subsystem currently has it's own i2c subsystem and I'm working
on going back to in-kernel i2c.
The original problem that made a colleague create a separate i2c
subsystem was that i2c client drivers were probing on new appearing
busses unconditionally. The worst example were the Matrox clients that
were driving other i2c busses besides the ones on the Matrox adapters nuts.
The problem is that some i2c busses on some dvb cards *really* don't
like being probed by unknown clients on unknown address ranges.
Most i2c client drivers check in their attach_adapter() function if they
need to probe by investigating the adapter->id. But not all.
Unfortunately, this only keeps well-written clients from attaching. The
i2c-core unconditionally tells every i2c client every new i2c adapter.
If the i2c-client doesn't check the adapter->id, then it can screw up
the i2c bus anyway:
int i2c_add_adapter(struct i2c_adapter *adap)
{
[...]
/* inform drivers of new adapters */
list_for_each(item,&drivers) {
driver = list_entry(item, struct i2c_driver, list);
if (driver->flags & I2C_DF_NOTIFY)
/* We ignore the return code; if it fails, too bad */
driver->attach_adapter(adap);
[...]
Here, all client drivers are unconditionally told to try and attach to
the adapter. There is no way that an i2c adapter can keep an i2c driver
away from the bus.
Currently, adapters can already specify a class, for DVB
I2C_ADAP_CLASS_TV_DIGITAL matches perfectly.
What I'd like to have is that client can specify some sort of "class",
too, and that i2c adapters can tell the core that only clients where the
class is matching are allowed to probe their existence.
The above code snippet would then probably look like this:
int i2c_add_adapter(struct i2c_adapter *adap)
{
[...]
/* inform drivers of new adapters */
list_for_each(item,&drivers) {
driver = list_entry(item, struct i2c_driver, list);
/* skip driver if it specifies a class and class to the
adapter does not match */
if (driver->class != 0 && driver->class != adap->client_class)
continue;
if (driver->flags & I2C_DF_NOTIFY)
/* We ignore the return code; if it fails, too bad */
driver->attach_adapter(adap);
[...]
This would greatly help to keep any i2c client driver away from the i2c
bus of the dvb card besides the dvb i2c drivers.
Do you think this is feasible? If so, I'd be happy to prepare a patch to
outline my ideas with some lines of code.
CU
Michael.
-
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majo...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Please read the FAQ at http://www.tux.org/lkml/
Adrian Cox
> What I'd like to have is that client can specify some sort of "class",
> too, and that i2c adapters can tell the core that only clients where the
> class is matching are allowed to probe their existence.
How about a general "never probe" flag combined with a function to
connect an adapter to a client? High level drivers like DVB or BTTV
could then do something like:
adapter = i2c_bit_add_bus(&my_card_ops);
i2c_connect_client(adapter, &client_ops, address);
This problem comes up a lot, and i2c probing is only necessary for
finding motherboard sensors. For add-in cards and embedded systems the
driver developer normally knows exactly what is wired to what.
- Adrian Cox
http://www.humboldt.co.uk/
Michael Hunold
On 16.03.2004 14:26, Adrian Cox wrote:
> On Tue, 2004-03-16 at 09:25, Michael Hunold wrote:
>>What I'd like to have is that client can specify some sort of "class",
>>too, and that i2c adapters can tell the core that only clients where the
>>class is matching are allowed to probe their existence.
>
>
> How about a general "never probe" flag combined with a function to
> connect an adapter to a client? High level drivers like DVB or BTTV
> could then do something like:
> adapter = i2c_bit_add_bus(&my_card_ops);
> i2c_connect_client(adapter, &client_ops, address);
>
> This problem comes up a lot, and i2c probing is only necessary for
> finding motherboard sensors. For add-in cards and embedded systems the
> driver developer normally knows exactly what is wired to what.
This is true for most devices (i2c eeprom, video decoder, ...), but not
for all. All DVB cards have a device called "frontend" (basically a
tuner with some additional stuff) connected via i2c.
Sometimes different frontends are used for the same revisons of one
card, so we need the probe functionality at least for these kinds of
devices.
> - Adrian Cox
> http://www.humboldt.co.uk/
CU
Michael.
Greg KH
> Here, all client drivers are unconditionally told to try and attach to
> the adapter. There is no way that an i2c adapter can keep an i2c driver
> away from the bus.
Yes, but the different i2c chip drivers all check for the class setting
to be correct before they really do anything, right?
> Currently, adapters can already specify a class, for DVB
> I2C_ADAP_CLASS_TV_DIGITAL matches perfectly.
Yes, and that is what you should check for. It's a bug if any of the
non-DVB i2c drivers probe devices with the .class set to
I2C_ADAP_CLASS_TV_DIGITAL. Fix that and you should be fine, right?
> What I'd like to have is that client can specify some sort of "class",
> too, and that i2c adapters can tell the core that only clients where the
> class is matching are allowed to probe their existence.
Yeah, right now it's up to the chip drivers to be honest. If you want
to implement a change to do this instead, I'll be glad to apply it.
thanks,
greg k-h
Jean Delvare
> to implement a change to do this instead, I'll be glad to apply it.
Does this mean that i2c_client would get an additional ".class" struct
element, of the same nature of the ".class" struct element of
i2c_adapter?
This sounds interesting. That way, the "compatibility" check would move
down to i2c-core and neither the bus drivers nor the chip drivers would
have to care (apart from defining this .class element). Sounds really
nice indeed.
I guess that chip drivers would be allowed to define only one class
while adapters could possibly define more than one?
We also would want to introduce an I2C_ADAP_CLASS_ANY define, which
would be what the eeprom driver would use, for example (since it can be
hosted on any kind of bus). Generic bus drivers such as i2c-parport
would also use I2C_ADAP_CLASS_ANY, since the nature of the hosted chips
is unknown.
Having clients define a class sounds also interesting from a
user-space's point of view. If we would export this information through
sysfs for example, programs such as "sensors" could limit their work to
chips of the correct class (I2C_ADAP_CLASS_SMBUS at the moment, but a
renaming is planned).
--
Jean Delvare
http://www.ensicaen.ismra.fr/~delvare/
Greg KH
>
> I guess that chip drivers would be allowed to define only one class
> while adapters could possibly define more than one?
Not necessarily. Just make the class a bit field, showing what kind of
devices each expects to handle.
> We also would want to introduce an I2C_ADAP_CLASS_ANY define, which
> would be what the eeprom driver would use, for example (since it can be
> hosted on any kind of bus). Generic bus drivers such as i2c-parport
> would also use I2C_ADAP_CLASS_ANY, since the nature of the hosted chips
> is unknown.
Sure:
#define I2C_ADAP_CLASS_ANY 0xffffffff
works for me :)
> Having clients define a class sounds also interesting from a
> user-space's point of view. If we would export this information through
> sysfs for example, programs such as "sensors" could limit their work to
> chips of the correct class (I2C_ADAP_CLASS_SMBUS at the moment, but a
> renaming is planned).
That also is a good idea.
thanks,
greg k-h
Jean Delvare
> > I guess that chip drivers would be allowed to define only one
> > class while adapters could possibly define more than one?
>
> Not necessarily. Just make the class a bit field, showing what kind
> of devices each expects to handle.
Of course, this is how I meant it. The way things are done for now, the
class value is already a bitfield and I'm fine with that. This makes
full sense for adapters. What I was wondering is whether it would be
allowed to set more than one class bit for a chip driver. Not that is
necessarily matters much, I'm just curious. Have we ever heard of
chips that would belong to more than one class as we defined them?
> > We also would want to introduce an I2C_ADAP_CLASS_ANY define,
> > which would be what the eeprom driver would use, for example
> > (since it can be hosted on any kind of bus). Generic bus drivers
> > such as i2c-parport would also use I2C_ADAP_CLASS_ANY, since the
> > nature of the hosted chips is unknown.
>
> Sure:
> #define I2C_ADAP_CLASS_ANY 0xffffffff
> works for me :)
Exactly what I had in mind ;)
> > Having clients define a class sounds also interesting from a
> > user-space's point of view. If we would export this information
> > through sysfs for example, programs such as "sensors" could
> > limit their work to chips of the correct class
> > (I2C_ADAP_CLASS_SMBUS at the moment, but a renaming is planned).
>
> That also is a good idea.
How would we export the value though? Numerical, with user-space headers
to be included by user-space applications? Or converted to some
explicit text strings so that no headers are needed?
--
Jean Delvare
http://www.ensicaen.ismra.fr/~delvare/
-
Greg KH
> How would we export the value though? Numerical, with user-space headers
> to be included by user-space applications? Or converted to some
> explicit text strings so that no headers are needed?
A text string would be simple enough to use.
thanks,
greg k-h
Jean Delvare
> > headers to be included by user-space applications? Or converted to
> > some explicit text strings so that no headers are needed?
>
> A text string would be simple enough to use.
I'm not sure. What about a chip driver that would belong to more than
one class? What about the eeprom driver which will belong to all
classes? With a numeric value, a simple binary operation handles all
the cases. With text strings we would end having to parse a possibly
multi-valued string, and do string comparisons, with at least one
exception to handle. This is likely to require much more resources,
don't you think?
--
Jean Delvare
http://www.ensicaen.ismra.fr/~delvare/
Greg KH
> > > How would we export the value though? Numerical, with user-space
> > > headers to be included by user-space applications? Or converted to
> > > some explicit text strings so that no headers are needed?
> >
> > A text string would be simple enough to use.
>
> I'm not sure. What about a chip driver that would belong to more than
> one class? What about the eeprom driver which will belong to all
> classes? With a numeric value, a simple binary operation handles all
> the cases. With text strings we would end having to parse a possibly
> multi-valued string, and do string comparisons, with at least one
> exception to handle. This is likely to require much more resources,
> don't you think?
Ok, I wasn't really awake when writing that, you are correct. Other
devices export "values" that have to be looked up in tables in userspace
(think device ids). We can just export a hex number that matches the
ones in i2c.h
Anyone want to write a patch? :)
thanks,
greg k-h
Michael Hunold
On 16.03.2004 16:44, Greg KH wrote:
> On Tue, Mar 16, 2004 at 10:25:25AM +0100, Michael Hunold wrote:
>
>>Here, all client drivers are unconditionally told to try and attach to
>>the adapter. There is no way that an i2c adapter can keep an i2c driver
>>away from the bus.
>
>
> Yes, but the different i2c chip drivers all check for the class setting
> to be correct before they really do anything, right?
As you've already noticed below, it's completely up to the driver -- and
I don't trust i2c drivers I haven't written... ;-)
>>Currently, adapters can already specify a class, for DVB
>>I2C_ADAP_CLASS_TV_DIGITAL matches perfectly.
> Yes, and that is what you should check for. It's a bug if any of the
> non-DVB i2c drivers probe devices with the .class set to
> I2C_ADAP_CLASS_TV_DIGITAL. Fix that and you should be fine, right?
In theory this should be sufficient, but doesn't help if you have some
"I don't care which bus this is and I'll probe it anyway" device.
Just think of all the i2c client drivers that are not part of the
official kernel. I don't want to fix all these, just keep them away from
"my" i2c busses...
>>What I'd like to have is that client can specify some sort of "class",
>>too, and that i2c adapters can tell the core that only clients where the
>>class is matching are allowed to probe their existence.
> Yeah, right now it's up to the chip drivers to be honest. If you want
> to implement a change to do this instead, I'll be glad to apply it.
Ok, thanks. I'm heading for the small solution that doesn't break any
existing "functionality" (if it's desired or not) and that doesn't
require to touch every i2c driver in the kernel.
Here is the agenda:
- add a "class" field to the i2c client driver struct (ie. let the
driver specify "I'm a DVB device" for example)
- add a flag to the adapter struct telling the i2c core "I only want
drivers with a matching class to probe on this bus"
- make the i2c core actually check if an adapter has set the flag
mentioned above and only let any i2c clientprobe on the bus if it has
the matching class field
This won't break existing functionality (because all "old" i2c client
drivers don't have the "class" field set and all "old" i2c adapters
don't have the new flag specified) and will keep all i2c client drivers
away from my DVB i2c busses.
The DVB i2c drivers, however, will be able to probe on the DVB i2c bus.
> thanks,
> greg k-h
CU
Michael.