[i2c] [patch 5/5] i2c_register_adapter()

[David Brownell]

On Sunday 04 March 2007 1:58 am, Jean Delvare wrote:
> Hi David,
> 
> On Thu, 15 Feb 2007 00:46:43 -0800, David Brownell wrote:
> > This adds a new call, i2c_register_adapter(), to register an I2C adapter
> > using a specific bus number and then create I2C device nodes for any
> > pre-declared devices on that bus.  It builds on previous patches adding
> > I2C probe() and remove() support, and pre-declaration of devices.
> > 
> > There's also a minor related cleanup, using class infrastructure to
> > create and remove the device attribute and associated code movement.
> 
> As usual, I will ask for a separate patch for this. This is not
> specifically related to the rest of what the patch does.

OK.


> > +	int res = 0;
> >  
> > -	adap->nr =  id & MAX_ID_MASK;
> >  	mutex_init(&adap->bus_lock);
> >  	mutex_init(&adap->clist_lock);
> >  	list_add_tail(&adap->list,&adapters);
> >  	INIT_LIST_HEAD(&adap->clients);
> >  
> > +	mutex_lock(&core_lists);
> > +
> 
> Looks like a bug to me, I think you must hold the code_lists mutex for
> the list_add_tail instruction above.

Whoops!  Yes.  Fixed.



> > +	mutex_lock(&core_lists);
> > +	/* "above" here means "above or equal to", sigh */
> > +	res = idr_get_new_above(&i2c_adapter_idr, adap,
> > +				__i2c_first_dynamic_bus_num, &id);
> > +	adap->nr = id;
> > +	mutex_unlock(&core_lists);
> > +	if (res < 0) {
> > +		if (res == -EAGAIN)
> > +			goto retry;
> > +		return res;
> > +	}
> 
> You should not set adap->nr before checking that the call to
> idr_get_new_above was successful.

Easily done, but I don't see why changing adap->nr from one undefined
value to another should matter.


> > +
> > +	if (res == 0)
> 
> Isn't this always true?

Yeah, that can be simplified.


> > +		res = __i2c_register_adapter(adap);
> > +	return res;
> > +}