Hi Nathan:
* Nathan Lutchansky <lutchann at litech.org> [2006-05-31 01:49:25 -0400]:
> I was hoping I'd have more time to finish this up, but it's becoming
> apparent that it's not going to fit anywhere in my schedule. I'm sorry I
> have to drop this in a really unfinished state, but ignoring your emails
> isn't helping anything.
Hrm, I hope I haven't put you off somehow.
> I'll try to address your major questions below, though:
>
> On Sat, 27 May 2006, Mark M. Hoffman wrote:
>
> > Summary: the biggest immediate problem here is that i2c_add_client()
> > leaks a struct i2c_client by design, AFAICT.
>
> Right, clients added with i2c_add_client() remain present until the bus is
> removed from the system, even if the driver for that client is removed.
> This is the behavior you would expect from i2c buses with a fixed, known
> list of clients present. However, for auto-probed clients, it is a change
> in behavior that users probably would not appreciate.
I meant leak literally... as in never kfree'ed. But now I see that it would
get kfree'ed from within i2c_del_adapter(). I'm not sure how I missed that.
> > I do see that this approach addresses a requirement of embedded systems
> > and others who know a priori what hardware is present. It is also nice
> > that it does not force any changes to current drivers.
> >
> > However, I think the i2c_add_client() function is awkward. You say that
> > this function could be called by an adapter that knows what devices it
> > has... but what if the adapter is a generic driver like i2c-i801? For
> > the embedded scenario, it would be preferable to further decouple these
> > two pieces:
> >
> > [ adapter driver for bus foo ]
> > [ bus foo has devices x, y & z ]
>
> Yes, it would be nice to be able to pass a list of attached devices to
> some adapter drivers. Possibly this could be passed in as a module
> parameter or as a field in the platform data structure for platform
> devices. However, this list might be dependent upon the contents of an
> EEPROM device located on that bus, so it would be good to leave this part
> as flexible as possible.
In the embedded scenario, would it be appropriate to pass such a list
from platform_notify()? That looks like the right place to me.
> > I would argue against a general move to this method for hwmon and others.
> > There would be no benefit that I can tell; in the long term I would expect
> > the i2c_add_client() function to disappear anyway.
>
> What do you expect would be a more suitable method for hwmon to
> instantiate clients? The current i2c_probe() mechanism is rather lacking;
> requiring users to pass arcane module parameters to each chip module is
> not ideal,
It may not be ideal, but it's rarely necessary for hwmon at any rate.
[see below for an answer to "what do you expect..."]
> and is completely impossible to use where the client drivers
> are compiled directly into the kernel.
Documentation/kernel-parameters.txt: 3rd paragraph
> I suspect you'll eventually need
> to go to a system where drivers can be bound at runtime to specific
> addresses using sysfs, similar to the SCSI subsystem where new devices can
> be probed by echoing "scsi add-single-device 4 0 6 0" to /proc/scsi/scsi.
> Such a mechanism would probably use something like i2c_add_client on the
> backend.
There are some i2c devices that are 100% not detectable by probing. This
would be a good alternative for those.
> > > static int i2c_device_match(struct device *dev, struct device_driver *drv)
> > > {
> > > - return 1;
> > > + struct i2c_client *client = to_i2c_client(dev);
> > > + struct i2c_driver *driver = to_i2c_driver(drv);
> > > +
> > > + return client->driver_id == driver->id;
> > > }
> >
> > As I mentioned previously - I think this function will ultimately
> > need to change. As you have it, it short-circuits the driver model
> > probing for any client without a specific driver ID. I understand
> > that this doesn't matter at the moment.
>
> I'm still not sure I understand why some drivers would not have IDs
> assigned. For embedded systems or V4L devices, where there is a fixed
> list of clients known to be present, how else would the proper client
> driver for each address be specified?
I will try to answer this and "What do you expect ..." above at the same
time, but it probably takes a patch to make it clear.
If we're going to better integrate the i2c subsystem with the driver model,
I think the i2c core has to instantiate all client structs. It would
enumerate through the i2c address space looking to match (i2c_device_match)
at each address. The client->driver_id may or may not be filled in at
that point; depends on whether the adapter has a known list of devices.
If the client does have a driver_id, that will serve the purpose of
*ruling out* all other devices. That is why I suggested (the equivalent
to) this earlier:
if (client->driver_id)
return client->driver_id == driver->id;
If we make it past that, we look for a match with the driver's address
data. Assuming that's good, we'll do i2c_device_probe() much as you
have it. We would continue to use adapter->class to restrict or modify
probing behavior, but we might be able to reduce it to PROBE and NOPROBE.
At least for the embedded scenario, there should be an I2C_CLASS_NONE.
That's my vision for now. I will send patches as soon as I can.
> > > static int i2c_device_probe(struct device *dev)
> > > {
> > > - return -ENODEV;
> > > + struct i2c_client *client = to_i2c_client(dev);
> > > + struct i2c_driver *driver = to_i2c_driver(dev->driver);
> > > + int error = -ENODEV;
> > > +
> > > + if (driver->probe) {
> > > + client->driver = driver;
> > > + error = driver->probe(client);
> > > + if (error < 0)
> > > + client->driver = NULL;
> > > + else
> > > + i2c_notify_adapter(client);
> > > + }
> > > + return error < 0 ? error : 0;
> > > }
> >
> > What's wrong with "if (error)" here? Likewise "return error" at the end?
>
> I believe I intended for the driver->probe() function to return either 0
> or a positive value on success, although I don't remember what those would
> have indicated. But i2c_device_probe() would always need to return only 0
> on success, which is why the strange comparisons are there.
>
> > > +static int __i2c_detach_client(struct i2c_client *client)
> > > +{
> > > + struct i2c_driver *driver = client->driver;
> > > + int error;
> > > +
> > > + if (driver && driver->detach_client)
> > > + return driver->detach_client(client);
> > > +
> >
> > [ (!driver) is impossible there, i.e. indicates a bug, no? ]
>
> No, it's perfectly fine for a client to exist on a bus with no associated
> client driver. This will happen if a client is instantiated with
> i2c_add_client() and the indicated driver ID does not correspond to an
> existing driver, perhaps because a module is not loaded or the driver is
> not compiled into the kernel. Allowing driverless clients to exist was
> the point of the patch, really...
OK, thanks. Your second patch had no examples of calling i2c_add_client()
from an adapter rather than from a driver, so I didn't consider that case.
> > ... to here you're just making the driver->detach_client() callback
> > optional. That's a problem: I don't see where you're going to
> > kfree the struct i2c_client that i2c_add_client() allocates. This might
> > explain the "can't unload then reload a client driver module" caveat
> > that you mentioned earlier.
>
> Right, yes. Any i2c_client struct created by i2c_add_client() will only
> ever get destroyed by i2c_del_adapter(), as discussed above.
>
> > > + error = i2c_add_to_bus(client);
> > > + if (error)
> > > + return error;
> >
> > [ I prefer "if ((error = i2c_add_to_bus(client)))" here ]
>
> I'm not fond of hiding assignments in if() statements, but I think that's
> just personal style.
>
> > > - module_put(client->driver->driver.owner);
> > > + if (client->driver)
> > > + module_put(client->driver->driver.owner);
> >
> > As Jean Delvare pointed out... !(client->driver) is a bug. In this
> > case "BUG_ON" is no better than just leaving it as it was - it will
> > oops either way.
>
> Yeah, I didn't think about the module reference counting, it definitely
> needs to be fixed.
>
> Again, I'm really sorry to have to drop this, but I have too much going on
> at the moment. I'd still be interested in following future discussion on
> the topic, though. -Nathan
No problem. I'm sorry it took so long to review as well.
Regards,
--
Mark M. Hoffman
mhoffman at lightlink.com
