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

[Greg KH <gregkh@linuxfoundation.org>]

On Fri, Jan 26, 2024 at 10:58:07AM -0800, Dan Williams wrote:
> Dan Williams wrote:
> > Greg KH wrote:
> > [..]
> > > > 
> > > > Hey Greg,
> > > > 
> > > > I wanted to follow up on this and see if you are able to provide more
> > > > details for reproducing or if you are able to look into it?
> > > 
> > > Last I tried this, it still crashed and would not boot either on my
> > > laptop or my workstation.  I don't know how it is working properly for
> > > you, what systems have you tried it on?
> > > 
> > > I'm not going to be able to look at this for many weeks due to
> > > conference stuff, so if you want to take the series and test it and
> > > hopefully catch my error, that would be great, I'd love to move forward
> > > and get this merged someday.
> > 
> > I mentioned to Lukas that I was working on a "sysfs group visibility"
> > patch and he pointed me to this thread. I will note that I tried to make
> > the "hide group if all attributes are invisible" approach work, but
> > reverted to a "new is_group_visible() callback" approach. I did read
> > through the thread and try to improve the argument in the changelog
> > accordingly.
> > 
> > I do admit to liking the cleanliness (not touching 'struct
> > attribute_group') of the "hide if no visible attribute" approch, but see
> > the criticism of that alternative below, and let me know if it is
> > convincing. I tested it locally with the following hack to make the
> > group disappear every other sysfs_update_group() event:
> 
> Hey Greg,
> 
> Ignore this version:
> 
> ---
> From: Dan Williams <dan.j.williams@intel.com>
> 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.

thanks,

greg k-h