On Mon, May 17, 2021 at 02:13:47PM -0300, Jason Gunthorpe wrote:
> On Mon, May 17, 2021 at 07:11:52PM +0200, Greg KH wrote:
> > On Mon, May 17, 2021 at 01:47:39PM -0300, Jason Gunthorpe wrote:
> > > qib should not be creating a mess of kobjects to attach to the port
> > > kobject - this is all attributes. The proper API is to create an
> > > attribute_group list and create it against the port's kobject.
> > >
> > > Signed-off-by: Jason Gunthorpe <jgg@xxxxxxxxxx>
> > > drivers/infiniband/hw/qib/qib.h | 5 +-
> > > drivers/infiniband/hw/qib/qib_sysfs.c | 596 +++++++++++---------------
> > > 2 files changed, 248 insertions(+), 353 deletions(-)
> > >
> > > diff --git a/drivers/infiniband/hw/qib/qib.h b/drivers/infiniband/hw/qib/qib.h
> > > index 88497739029e02..3decd6d0843172 100644
> > > +++ b/drivers/infiniband/hw/qib/qib.h
> > > @@ -521,10 +521,7 @@ struct qib_pportdata {
> > >
> > > struct qib_devdata *dd;
> > > struct qib_chippport_specific *cpspec; /* chip-specific per-port */
> > > - struct kobject pport_kobj;
> > > - struct kobject pport_cc_kobj;
> > > - struct kobject sl2vl_kobj;
> > > - struct kobject diagc_kobj;
> > > + const struct attribute_group *groups[5];
> >
> > As you initialize these all at once, why not just make this:
> > struct attribute_group **groups;
> >
> > and then set the groups up at build time instead of runtime as part of a
> > larger structure like the ATTRIBUTE_GROUPS() macro does for "simple"
> > drivers? That way you aren't fixed at the array size here and someone
> > has to go and check to verify you really have properly 0 terminated the
> > list and set up the pointers properly.
>
> qib has a variable list of group memberships that can only be
> determined at runtime:
>
> if (qib_cc_table_size && ppd->congestion_entries_shadow)
> *cur_group++ = &port_ccmgta_attribute_group;
>
> So it can't be setup statically at compile time.
That attribute group can have a is_visable() callback to allow those
files to show up or not, instead of having to determine this when you
are setting up the list of groups.
But that's your call, not a big deal, overall this series looks like a
lot of good cleanups to me, thanks for doing it.
greg k-h
