Re: i2c-remove-redundant-i2c_client-list.patch

[Greg KH <greg-U8xfFu+wG4EAvxtiuMwx3w@public.gmane.org>]

On Tue, Jan 08, 2008 at 01:44:45PM -0800, David Brownell wrote:
> On Tuesday 08 January 2008, Jean Delvare wrote:
> > > > 			device_for_each_child() relies on a 
> > > > klist_iter, and the documentation of klist_next mentions increasing the
> > > > reference count of the "active" list item. I would guess that you can't
> > > > remove a list item while its reference count is not zero, and that
> > > > would explain the lockup. The original code used list_for_each_safe(),
> > > > which explicitly allows the removal of the items while walking the list.
> > > > 
> > > > OTOH the header comment of lib/klist.c says:
> > > > 
> > > > ?*??The entire point is to provide an interface for iterating over a list
> > > > ?*??that is safe and allows for modification of the list during the
> > > > ?*??iteration (e.g. insertion and removal), including modification of the
> > > > ?*??current node on the list.
> > > >
> > > > So it is supposed to work somehow...
> > > 
> > > Yeah, but then again ... the i2c stack does that oddball stuff with
> > > complete(&client->released) for legacy drivers, too.  That's always
> > > been contrary to what the driver model expects, and I never quite
> > > bothered to sort out the issues.  Maybe this is one of them (sigh).
> > 
> > I see. I doubt I'll be of much help, as I could never figure out what
> > this completion stuff was for in the first place.
> 
> In which case I'm guessing it's stuff Greg did way back in the day.
> I cc'd him in hope that he can find time to comment on that ...

Ick, sorry about that mess :(

> My understanding is that it was written early in sysfs conversions
> to help ensure sysfs wasn't keeping an open handle on the device.
> However, that early scheme proved to be quite error prone and now
> things are set up differently.  There was a particularly annoying
> class of problem with module removal.  Devices allocated by a driver
> needed to be freed by that driver, which implies not unloading any
> driver's module until device nodes it had allocated were freed.

Yes, that should not be used anymore, and I thought we had removed it
for some reason.  I guess I was just wishing it were so...

> The simple way around that restriction involves never having modules
> allocate such devices ... clearly not easy for legacy I2C drivers.
> Another way around it was to synchronize driver removal with the
> cleanup of those devices.  And that's what the current I2C stack is
> (still) doing for legacy drivers, with that completion logic.
> 
> - Dave
> 
> p.s. Greg, for context:  there are two patches in Jean's I2C queue
>      to remove some redundant I2C lists, in favor of using versions
>      maintained by i2c-core.  $SUBJECT relates to a patch removing
>      a third redundancy:  i2c_adapter.clients and i2c_client.list,
>      plus (the original problem) the lock for that list.

That sounds like a good idea, if the above race doesn't happen :)

Anything I can do to help out?

thanks,

greg k-h