[i2c] [patch 2.6.20-rc1 5/6] remove i2c_adapter.dev from i2c core and adapters

[Jean Delvare]

David,

On Wed, 20 Dec 2006 18:03:01 -0800, David Brownell wrote:
> On Wednesday 20 December 2006 1:57 pm, Jean Delvare wrote:
> 
> > > To cope with incomplete driver model conversions in I2C bus drivers, if
> > > there's no real device associated with the adapter, a warning is issued
> > > and a "legacy_i2c_adapter" platform device is created.  One hopes all
> > > the I2C bus drivers will be fixed over the next year or so, letting that
> > > hack be removed.
> > 
> > I'm not happy with this. We're in the process of cleaning the i2c-core,
> > and now we're adding yet another temporary workaround. When we later
> > remove this workaround, we'll once again modify the i2c_adapter
> > structure, and the sysfs representation will change as well. I'd rather
> > fix the offending drivers right now.
> 
> So there would be two changes:  (a) replace the code working around that
> "legacy adapter" case with a clean failure mode, and (b) remove a pointer
> from the struct.  Those aren't big changes ... I chose that tradeoff
> to minimize breakage, and expected most of the missing device nodes would
> be on the platform_bus anyway.

Granted, these aren't big changes. I am simply trying to limit the
possible impact of our changes on both user-space and the kernel users
of our structures and interfaces. You never know who is using what.
Secondly, making the patches more simple and more obvious is a good
thing too.

> > In a number of cases, the missing device is not declared, but it's
> > there. That's the case for all PCI and USB devices, mostly in
> > media/video drivers. I have a patch to fix these, attached.
> 
> Excellent!  As I did it originally, those could be added to the
> sequence as "#5.5 of 6".  (Or added to this #5 patch, although it
> seems quite separate to me.)

I am not sure where to put them myself. Could be #4.5, as I have it in
my stack right now, with the advantage that this can be tested and
merged independently of the other changes; patch #5 would have to
convert the added lines to use class_dev.dev instead of dev.parent, as
it does for all the other drivers. Could be #5.5, as you suggested
(using class_dev.dev directly), in which case it depends on #5. Or it
could be merged into #5. I guess it depends on what ends up being in
patch #5 exactly and how fast we can push it upstream.

> Or the "missing device" can be handed to i2c core using the old
> i2c_adapter.dev.parent field, if there's a delay getting #5 merged.

Hmm, yes, that'd make sense. I think I like the idea. Given that we
already have that fake device, it's probably better to use it rather
than introduce a new one. This limits the changes nicely. Could you
come up with a variant of patch #5 which would do that?

> The updates in #6 more or less rely on having a sane device hierarchy,
> and providing the "missing device" is a big help there.

I don't really mind if misbehaving drivers don't benefit from patch #6,
as long as it doesn't lead to an oops/crash.

> > For the rest, these are legacy drivers, I suppose we would need to
> > convert them to platform drivers. That would be a significant amount of
> > work... But after investigating a bit, I found that registering a class
> > device without an associated device should work. Other drivers do that
> > already (fbcon or lp for example.) So I suggest that we don't implement
> > that legacy_i2c_adapter hack at all.
> 
> The problem with that is that registering the class works fine, but then
> I'm not sure where the individual I2C devices would end up.  If we have

Ah, good point. Let's just try and see what happens? I guess that we
would need to declare the platform bus as their parent, as we were
doing for the i2c_adapter devices so far. Not really clean, but that
should work?

> such a legacy_i2c_adapter node, we know:  they'll be children of that,
> and the suspend/resume/shutdown call sequences (from patch #6) will always
> be correct with respect to the adapter.  (That is, the i2c_client devices
> will suspend or shutdown before the adapter, and resume after it.)

I don't expect power management to work with drivers which don't even
fit in the device driver model at the moment, so I don't care (as long
as it doesn't lead to a crash, as mentioned above.)

> > > +struct class i2c_adapter_class = {
> > > +	.name =			"i2c-adapter",
> > > +	.owner =		THIS_MODULE,
> > > +	.class_dev_attrs =	adapter_attrs,
> > > +	.release =		&i2c_adapter_class_dev_release,
> > > +};
> > 
> > I see why this is necessary,
> 
> Because the place the attribute used to live isn't there any more.  :)
> 
> > but it changes the interface we offer to 
> > user-space, and will break some tools and libraries. For example
> > libsensors uses this field to present the adapter name to the user, and
> > will need to be updated to get the class name attribute instead of the
> > device name attribute.
> > 
> > Unfortunately there's no way around it now. We should have anticipated
> > the change by creating the class attribute earlier, and by teaching
> > libsensors to read from it earlier. It's too late now... But we might
> > have to delay a bit the merging of this patch because of this :(
> 
> Grr, I see what you mean.
> 
> So what's the migration strategy ... can we get 2.6.20 to expose the
> class attribute, and start deploying smarter libsensors ASAP?  (The
> change would be to try for the class attribute first, then fallback.)
> I don't even know who maintains libsensors.

That would be Mark M. Hoffman and myself. I committed the required
change to libsensors earlier today, it's pretty trivial:
  http://lm-sensors.org/changeset/4267
The plan is to release lm_sensors 2.10.2 soon, probably mid-January.

> That'd cause some chaos later in these patch series, but it'd be
> manageable.  In functional terms, the rest of these patches can be
> decoupled from this particular patch ... none of them really "changed"
> things (other than stopping use of i2c_adapter.dev), they're all
> additions.  It'll make a mess of the later patches, which now apply
> against code that assumes this bit, but that's fixable; and earlier
> patches will apply just fine without it.

I'd say: prepare a patch #0 which adds the classdev .name attribute, so
that user-space can start using it, and announcing that the
i2c_adapter.dev .name attribute will be deprecated soon (say June
2007.) I could push this patch quickly into 2.6.20.

Then we keep patches #1 to #4 stacked for 2.6.21. Patch #5 will need
some rework, I need to test some more things, as discussed above,
before I decide exactly what is acceptable and what isn't. Patch #6 and
later, I didn't have the time to review yet, so I can't tell. Odds are,
a part will be OK for 2.6.21, and a part might be too late. But we'll
see.

> > > +	 * As a **TEMPORARY migration aid** we create a platform
> > > +	 * device for drivers that don't yet use the driver model.
> > > +	 * Expect this to vanish by January 2008.
> > 
> > If we really do this, then this must be documented in
> > Documentation/feature-removal-schedule.txt, not just here. But as I
> > said above, I don't think we need to introduce these fake devices.
> 
> If you don't want to do that, so be it.  I'd make this block of code
> report a fatal error then, for the reasons I sketched above.

Give me some time to see exactly what the possibilities are, and I'll
get back to you with my decision when I'm done. I wonder if we could
keep i2c_adapter.dev for now, while still killing i2c_adapter_driver;
that would allow some cleanups in parallel.

> > BTW, it's still a mystery to me what these operations we do on
> > dev_released are for, if you could explain that to me...
> 
> As I recall, the basic issue is that for nodes that are visible
> in sysfs you need to remember what happens with sequences like:
> 
> 	# cd /sys/.../node
> 	# rmmod "module that created node"
> 
> In that case, it's not safe to free the memory associated with
> the node until after your shell changes directory out of that node. 
> (There are many other similar cases, with files open for other
> reasons.)  So the wait_for_completion() before the free is
> preventing some nastiness.
> 
> But that also highlights a few other glitches in the process.
> Again using that example, the "rmmod" should complete, so that
> your shell can change directory.
> 
> The _simple_ way to work around this stuff is not to have drivers
> allocate or free the memory associated with devices (or classes).
> For example, by using routines like class_device_create(), rather
> than doing what i2c now does... that sort of change was more than
> I wanted to do myself, but it'd be easier after everyone (including
> i2c core) stops using i2c_adapter.dev and after some other cleanup
> to the i2c core code.

OK, I understand the idea. What I'm not sure about is when exactly we
are supposed to call init_completion() and wait_for_completion(), and
what these functions do. Likewise, I don't know when the .release
callback will be called. I assume that wait_for_completion(x) blocks
until someone (presumably the driver core) calls complete(x)?

> > >  /* We implement an interface which resembles i2c_{add,del}_driver,
> > > @@ -137,44 +140,33 @@ static int __init i2c_isa_init(void)
> > >  
> > >  	mutex_init(&isa_adapter.clist_lock);
> > >  	INIT_LIST_HEAD(&isa_adapter.clients);
> > > +	init_completion(&isa_adapter.dev_released);
> > > +	init_completion(&isa_adapter.class_dev_released);
> > 
> > Now I'm confused. Here we initialize them at the beginning, while in
> > i2c-core we initialize them right before deleting the i2c_adapter?
> 
> If you look at "part II patch #4" you'll see this is fixed there.
> In general, it's correct to init completions right when they're
> allocated ... but I didn't want to make this patch ("part I #5")
> larger than needed.  The i2c-isa bits needed more rework.

OK, I admit I didn't have the time to review this second patchset yet.
(And you'll have to be patient...)

> > I start wondering if we shouldn't kill i2c-isa right now so that we
> > don't have to modify it (and break user-space compatibility...)
> 
> Well, if i2c_adapter.dev has to linger until libsysfs changes get
> deployed (call it 6 months), that'd seem plenty of time for you
> to finish your "kill i2c-isa" jihad.  ;)

The problem is that libsensors didn't properly handle platform drivers
before lm_sensors 2.10.1, which was released only recently (September
2006), so once again we have a userspace compatibility issue. We might
have to wait until kernel 2.6.22 before finally getting rid of i2c-isa.

> How do you think we should proceed with these patches then?  What
> I think you're implying is:
> 
>  - #1-4 in this series are all but ready to go into MM;
>    I think I've addressed all your review comments.

They _are_ ready to go into MM. I will push them there shortly.

> 
>  - #5 needs to wait until libsysfs gets updated, which for
>    now I'm calling "6 months".

libsensors, not libsysfs. Yes, 6 months sounds reasonable.

> 
>  - Some parts of #5, notably creation of class attribute,
>    should be merged soonish, to support new libsysfs.

Yes, this specific part of #5 should even be merged right now, as
suggested above. Another part could go to -mm when we have our battle
plan ready. And one part probably needs to be postponed, unfortunately.

> 
>  - "#5.5" (which you attached to this email) should get
>    respun to init using i2c_adapter.dev.parent, so that it
>    doesn't require #5.

Err, it _does_ use i2c_adapter.dev.parent at the moment, so it doesn't
require #5. But I think that this part of #5 should be preserved. We
can consider i2c_adapter.class_dev.dev to be the public access point to
the underlying device, and i2c_adapter.dev.parent to be an internal
implementation detail. So i2c-core would get i2c_adapter.class_dev.dev
from the the drivers, and copy that value to i2c_adapter.dev.parent,
rather than the other way around. That way the kernel drivers part of
the cleanup is already done, and only the user-space part is delayed.
Does it sound feasable to you?

> 
>  - #6 you've not commented on recently, but I believe that once
>    it gets respun to apply without #5, it's ready to go to MM.
> 
>  - All the "Part II" patches ("new style" driver binding) would
>    need to get respun to apply without #5 ... but pending more
>    review (they had one round already), I think they should go
>    to MM in expectation of a 2.6.21 merge.

I'll let you know when I'm there.

> Regardless, attached you'll find an updated version of #5 which
> addresses your comments above.

Thanks.

-- 
Jean Delvare