Re: [PATCH 11/13] RDMA/qib: Use attributes for the port sysfs

[Jason Gunthorpe <jgg@nvidia.com>]

On Mon, May 17, 2021 at 07:31:30PM +0200, Greg KH wrote:
> 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@nvidia.com>
> > > >  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.

Okay, that looks like it will work out and will be less complicated
code in a driver, and I can get rid of a function callback. See below

> 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.

No problem, it deletes a lot of horrible code. Every time I looked at
this in the past I wanted to poke at it. When Kee's pointed out what
it was doing it was just the last straw. I didn't expect it to expand
into cm, qib and hfi1 though..

Thanks,
Jason

diff --git a/drivers/infiniband/hw/qib/qib.h b/drivers/infiniband/hw/qib/qib.h
index 3decd6d0843172..b8a2deb5b4d295 100644
--- a/drivers/infiniband/hw/qib/qib.h
+++ b/drivers/infiniband/hw/qib/qib.h
@@ -521,7 +521,6 @@ struct qib_pportdata {
 
 	struct qib_devdata *dd;
 	struct qib_chippport_specific *cpspec; /* chip-specific per-port */
-	const struct attribute_group *groups[5];
 
 	/* GUID for this interface, in network order */
 	__be64 guid;
diff --git a/drivers/infiniband/hw/qib/qib_sysfs.c b/drivers/infiniband/hw/qib/qib_sysfs.c
index 2c81285d245fa7..a1e22c49871266 100644
--- a/drivers/infiniband/hw/qib/qib_sysfs.c
+++ b/drivers/infiniband/hw/qib/qib_sysfs.c
@@ -282,8 +282,19 @@ static struct bin_attribute *port_ccmgta_attributes[] = {
 	NULL,
 };
 
+static umode_t qib_ccmgta_is_bin_visible(struct kobject *kobj,
+				 struct bin_attribute *attr, int n)
+{
+	struct qib_pportdata *ppd = qib_get_pportdata_kobj(kobj);
+
+	if (!qib_cc_table_size || !ppd->congestion_entries_shadow)
+		return 0;
+	return attr->attr.mode;
+}
+
 static const struct attribute_group port_ccmgta_attribute_group = {
 	.name = "CCMgtA",
+	.is_bin_visible = qib_ccmgta_is_bin_visible,
 	.bin_attrs = port_ccmgta_attributes,
 };
 
@@ -534,6 +545,14 @@ static const struct attribute_group port_diagc_group = {
 
 /* End diag_counters */
 
+static const struct attribute_group *qib_port_groups[] = {
+	&port_linkcontrol_group,
+	&port_ccmgta_attribute_group,
+	&port_sl2vl_group,
+	&port_diagc_group,
+	NULL,
+};
+
 /* end of per-port file structures and support code */
 
 /*
@@ -718,19 +737,7 @@ const struct attribute_group qib_attr_group = {
 int qib_create_port_files(struct ib_device *ibdev, u32 port_num,
 			  struct kobject *kobj)
 {
-	struct qib_devdata *dd = dd_from_ibdev(ibdev);
-	struct qib_pportdata *ppd = &dd->pport[port_num - 1];
-	const struct attribute_group **cur_group;
-
-	cur_group = &ppd->groups[0];
-	*cur_group++ = &port_linkcontrol_group;
-	*cur_group++ = &port_sl2vl_group;
-	*cur_group++ = &port_diagc_group;
-
-	if (qib_cc_table_size && ppd->congestion_entries_shadow)
-		*cur_group++ = &port_ccmgta_attribute_group;
-
-	return ib_port_sysfs_create_groups(ibdev, port_num, ppd->groups);
+	return ib_port_sysfs_create_groups(ibdev, port_num, qib_port_groups);
 }
 
 /*
@@ -740,10 +747,7 @@ void qib_verbs_unregister_sysfs(struct qib_devdata *dd)
 {
 	int i;
 
-	for (i = 0; i < dd->num_pports; i++) {
-		struct qib_pportdata *ppd = &dd->pport[i];
-
+	for (i = 0; i < dd->num_pports; i++)
 		ib_port_sysfs_remove_groups(&dd->verbs_dev.rdi.ibdev, i,
-					    ppd->groups);
-	}
+					    qib_port_groups);
 }