On Tue, 2015-05-12 at 17:48 +0000, Weiny, Ira wrote:
> > > In preparation for this create a structure for per port immutable data
> > > and place the pkey and gid table lengths within this structure.
> > >
> > > This type of data requires a new call back "port_immutable" parameter
> > > to ib_register_device to allow each driver to create this data as appropriate.
> > > This callback is added to ib_register_device rather than as a new
> > > device function because the callback should only be used when devices
> > > are first registered.
> > >
> 
> I tried to explain my decision here...

I know.  I just couldn't handle the function declaration ;-)

> > > @@ -273,7 +273,9 @@ out:
> > >   */
> > >  int ib_register_device(struct ib_device *device,
> > >  		       int (*port_callback)(struct ib_device *,
> > > -					    u8, struct kobject *))
> > > +					    u8, struct kobject *),
> > > +		       int (*port_immutable)(struct ib_device *, u8,
> > > +					     struct ib_port_immutable *))
> > 
> > I'm having a hard time getting past how ugly this is.  Passing callbacks as
> > arguments to a registration function should be a last resort.  I would rather see
> > this added to the device mandatory list (and it *is* mandatory, we use it
> > without checking for NULL).  
> 
> Your correct I should have checked for NULL and returned an error if not supplied.
> 
> > In truth, I'd rather see both of those moved to the
> > driver callback struct.  They can be placed at the end, after a comment that
> > says something like "These are single use entry points for initialization, keep
> > them away from the rest of the entry points to help prevent growing the entry
> > point list beyond any more cachelines that needed for the more commonly
> > used entry points".  I would find that much preferable to this.
> > 
> 
> I'll change to this method in the next version.

Excellent.


-- 
Doug Ledford <dledford-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
              GPG KeyID: 0E572FDD