Re: [PATCH v6 2/3] sysfs: Add a attr_is_visible function to attribute_group

[Dan Williams]

Dan Williams wrote:
> > > Hey Greg,
> > > 
> > > Ignore this version:
> > > 
> > > ---
> > > From: Dan Williams <dan.j.williams@xxxxxxxxx>
> > > Date: Tue, 23 Jan 2024 20:20:39 -0800
> > > Subject: [PATCH] sysfs: Introduce is_group_visible() for attribute_groups
> > > ---
> > > 
> > > I am going back to your approach without a new callback, and some fixups
> > > to avoid unintended directory removal. I will post that shortly with its
> > > consumer.
> > 
> > Ignore it?  I was just about to write an email that said "maybe this is
> > the right way forward" :)
> > 
> > What happened to cause it to not be ok? And if you can find the bug in
> > the posted patch here, that would be great as well.
> 
> It was an aha moment this morning that I could rely on something like:
> 
> #define SYSFS_GROUP_INVISIBLE ((umode_t)-1)
> 
> ...as the return value from either is_visible() or attr_is_visible() and
> not need to define a new is_group_visible() callback.
> 
> The only downside I can see is that there is no way to know if the
> is_visible() handler might return SYSFS_GROUP_INVISIBLE to decide when
> to warn about deletion not finding anything to delete, or update not
> finding the existing node to update.
> 
> I'll type it up and see how it looks, but if you're not worried about
> the is_group_visible() addition to 'struct atttribute_group' I think
> that way is less hacky than the above.

Ok, here it is (below the scissors line). I ended up including a way to
determine if an attribute_group is opting into this new capability, and
I do think this is more in line with your direction to just reuse
existing is_visible() callbacks. This was tested with the following hack
to a dynamic visibility group in CXL:

diff --git a/drivers/cxl/core/region.c b/drivers/cxl/core/region.c
index 3e817a6f94c6..286b91e29c88 100644
--- a/drivers/cxl/core/region.c
+++ b/drivers/cxl/core/region.c
@@ -2010,8 +2010,8 @@ static struct attribute *target_attrs[] = {
 	NULL,
 };
 
-static umode_t cxl_region_target_visible(struct kobject *kobj,
-					 struct attribute *a, int n)
+static umode_t cxl_region_target_attr_visible(struct kobject *kobj,
+					      struct attribute *a, int n)
 {
 	struct device *dev = kobj_to_dev(kobj);
 	struct cxl_region *cxlr = to_cxl_region(dev);
@@ -2022,9 +2022,22 @@ static umode_t cxl_region_target_visible(struct kobject *kobj,
 	return 0;
 }
 
+static bool cxl_region_target_group_visible(struct kobject *kobj)
+{
+	struct device *dev = kobj_to_dev(kobj);
+	struct cxl_region *cxlr = to_cxl_region(dev);
+	struct cxl_region_params *p = &cxlr->params;
+
+	if (!p->interleave_ways || p->interleave_ways > 2)
+		return false;
+	return true;
+}
+DEFINE_SYSFS_GROUP_VISIBLE(cxl_region_target);
+
 static const struct attribute_group cxl_region_target_group = {
+	.name = "target_group",
 	.attrs = target_attrs,
-	.is_visible = cxl_region_target_visible,
+	.is_visible = SYSFS_GROUP_VISIBLE(cxl_region_target),
 };
 
 static const struct attribute_group *get_cxl_region_target_group(void)

-- >8 --