[i2c] [patch 2/5] i2c can remove()

[David Brownell]

On Friday 02 March 2007 9:09 am, Jean Delvare wrote:
> Hi David,
> 
> On Thu, 15 Feb 2007 00:41:59 -0800, David Brownell wrote:
> > More update for new style driver support:  add a remove() method, and
> > use it in the relevant code paths. ...

> > +static void i2c_unregister_device(struct i2c_client *client)
> > +{
> > +	struct i2c_adapter	*adap = client->adapter;
> > +	struct i2c_driver	*driver = client->driver;
> > +	int			status;
> > +
> > +	if (driver && !driver->remove) {
> > +		dev_err(&client->dev, "can't unregister devices "
> > +			"with legacy drivers\n");
> > +		WARN_ON(1);
> > +		return;
> > +	}
> 
> I'm not sure I agree with this. Isn't it valid for a (device driver
> model compliant) driver not to have a remove() method? Also, I don't
> quite see how this method would ever be called for a legacy driver, as
> you check explicitely against that at registration time.

A later patch in this series makes this method public, so those
cases could happen that way.  The overall patch series is simpler
(and smaller) if this routine doesn't need to change twice.


> > +
> > +	/* unbind driver */
> > +	if (driver) {
> > +		/* REVISIT driver model core would handle this for us, and
> > +		 * once i2c_detach_client() stops updating lists, it should.
> > +		 */
> 
> Actually my current i2c stack no longer has the faulty i2c_adapter
> driver, so it should be possible to remove the duplicate list
> management. Could you provide a patch doing this? Would it let you
> make this one patch cleaner?

I'd much rather get this patch series merged first, then clean up those
superfluous lists later.  Removing those lists first implies respinning
most patches in this series (not just this one), extra testing time, and
further magnifies the delays involved.  Simpler to remove them later.


> > +	/* now remove the device, and free its memory */
> > +	status = i2c_detach_client(client);
> > +	if (status < 0)
> > +		dev_err(&adap->dev, "detach failed (%d) for client [%s] "
> > +			"at address 0x%02x\n",
> > +			status, client->name, client->addr);
> > +	else
> > +		put_device(&client->dev);
> 
> Am I right assuming that we're in big trouble if i2c_detach_client()
> actually fails?

No more than if any other driver model unregister() call fails.  They
are all declared as void ... as in, "the best we can do is log errors
and leak the memory".


> > @@ -411,12 +470,17 @@ int i2c_del_driver(struct i2c_driver *dr
> >  
> >  	int res = 0;
> >  
> > -	mutex_lock(&core_lists);
> > +	/* For new-style drivers, driver model unregistration handles
> > +	 * all the unbind-from-device logic.  Just Do It.
> 
> I would avoid the extra caps. Don't want to be sued by some company for
> using their trademark without permission ;)

I avoided using them in any context where there could be a possibility
of confusion (i.e. grounds for any legal action).  Since that company
isn't even *in* markets for computer softwear, and the phrase has long
been in common use, this is safe.


> > @@ -444,12 +508,13 @@ int i2c_del_driver(struct i2c_driver *dr
> >  			}
> >  		}
> >  	}
> > +out_unlock:
> > +	mutex_unlock(&core_lists);
> >  
> > +unregister:
> >  	driver_unregister(&driver->driver);
> >  	pr_debug("i2c-core: driver [%s] unregistered\n", driver->driver.name);
> >  
> > - out_unlock:
> > -	mutex_unlock(&core_lists);
> >  	return 0;
> >  }
> 
> It's not correct for legacy drivers, I think. The original code did
> _not_ call driver_unregister on error, and now it does. I don't think
> your patch is supposed to change anything for legacy drivers, is it?

Hmm, you have a point.  But if you think for a moment about what the
code would look like if it preserves that original behavior:

	if (new-style driver)
		driver_unregister();
	else
		current code;

Maybe it would be better if i2c_del_driver() were legacy-only, and
new-style drivers used a new i2c_unregister_driver() call.  (And if
we go that route, maybe similarly on the registration side.)  What
would you think of that approach?


> When the patches are ready, I think I'll fold this patch into the first
> one (i2c stack can probe()). Each doesn't make much sense without the
> other.

If you want to merge the patches, that's your call.  I will point out
that it's common to split out sequences of patches this way ... and in
fact, neither of these make sense without the later patches either.
So in a sense it's a tradeoff between One Big Patch (that's not very
digestible/reviewable) and a series of bite-sized ones.

- Dave