Re: Again on virtual i2c adapter support.

[David Brownell <david-b-yBeKhBN/0LDR7s880joybQ@public.gmane.org>]

On Friday 23 January 2009, Jean Delvare wrote:
> > --- a/drivers/i2c/i2c-core.c
> > +++ b/drivers/i2c/i2c-core.c
> > @@ -799,10 +799,10 @@ void i2c_del_driver(struct i2c_driver *driver)
> >         class_for_each_device(&i2c_adapter_class, NULL, driver,
> >                               __detach_adapter);
> > 
> > +       mutex_unlock(&core_lock);
> > +
> >         driver_unregister(&driver->driver);
> >         pr_debug("i2c-core: driver [%s] unregistered\n",
> >         driver->driver.name);
> > -
> > -       mutex_unlock(&core_lock);
> >  }
> >  EXPORT_SYMBOL(i2c_del_driver);
> > 
> > Unluckely I'm not an i2c guru so I'd like to ask to you. If such
> > modification is correct I can provide a simple support with no
> > modifications to the i2c core! Apart this one, of course... :)
> 
> Honestly, I am not sure either. The locking model isn't completely
> cleared yet,

All legacy locking in the I2C core is problematic -- courtesy
of the original (lack-of-)design.  The core_lock is the worst
offender; it's mostly there to support some wierdness when
removing legacy drivers, and I recall thinking that it works
more by luck (and lack of heavy use!) than correctness.


> we still need to get rid of the 3 dozen remaining legacy 
> i2c chip drivers before we can finish it. I _think_ the above it
> correct, after all we do not hold the lock when calling
> driver_register() so I can't see why we would need to hold it when
> calling driver_unregister()... unless either i2c_client_release() or
> i2c_client_dev_release() needs this.
> 
> David should know better, as he designed the thing in the first place.

No I didn't ... I worked on making it play better with the
driver model, and simplifying a bunch of the rest.  I gave
up on fixing the legacy locking ... other than that patch
to remove i2c_adapter.clist_lock, a while back, I think the
only real fix is getting rid of those legacy calls (thus
using only the driver-model internal locks).


> David, can you please comment on the proposed change?

If it's a legacy driver, it will already have been decoupled
from devices, so the nastiest bit of stuff will have gotten
the protection it needs.  New-style drivers shouldn't care
about the legacy stuff at all.  So that much looks plausible.

But I don't have the patience to really analyse the other
locking implications; there were some rude and un-obvious
things lurking there, including abuse of complete() for
the legacy i2c_client lifecycle.

- Dave