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

[Greg KH]

On Mon, Aug 28, 2023 at 03:05:41PM +1000, Alistair Francis wrote:
> On Wed, Aug 23, 2023 at 5:02 PM Greg KH <gregkh@xxxxxxxxxxxxxxxxxxx> wrote:
> >
> > On Tue, Aug 22, 2023 at 04:20:06PM -0400, Alistair Francis wrote:
> > > On Sat, Aug 19, 2023 at 6:57 AM Greg KH <gregkh@xxxxxxxxxxxxxxxxxxx> wrote:
> > > >
> > > > On Thu, Aug 17, 2023 at 07:58:09PM -0400, Alistair Francis wrote:
> > > > > The documentation for sysfs_merge_group() specifically says
> > > > >
> > > > >     This function returns an error if the group doesn't exist or any of the
> > > > >     files already exist in that group, in which case none of the new files
> > > > >     are created.
> > > > >
> > > > > So just not adding the group if it's empty doesn't work, at least not
> > > > > with the code we currently have. The code can be changed to support
> > > > > this, but it is difficult to determine how this will affect existing use
> > > > > cases.
> > > >
> > > > Did you try?  I'd really really really prefer we do it this way as it's
> > > > much simpler overall to have the sysfs core "do the right thing
> > > > automatically" than to force each and every bus/subsystem to have to
> > > > manually add this type of attribute.
> > > >
> > > > Also, on a personal level, I want this function to work as it will allow
> > > > us to remove some code in some busses that don't really need to be there
> > > > (dynamic creation of some device attributes), which will then also allow
> > > > me to remove some apis in the driver core that should not be used at all
> > > > anymore (but keep creeping back in as there is still a few users.)
> > > >
> > > > So I'll be selfish here and say "please try to get my proposed change to
> > > > work, it's really the correct thing to do here."
> > >
> > > I did try!
> > >
> > > This is an attempt:
> > > https://github.com/alistair23/linux/commit/56b55756a2d7a66f7b6eb8a5692b1b5e2f81a9a9
> > >
> > > It is based on your original patch with a bunch of:
> > >
> > > if (!parent) {
> > >     parent = kernfs_create_dir_ns(kobj->sd, grp->name,
> > >                   S_IRWXU | S_IRUGO | S_IXUGO,
> > >                   uid, gid, kobj, NULL);
> > >     ...
> > >     }
> > >
> > >
> > > added throughout the code base.
> > >
> > > Very basic testing shows that it does what I need it to do and I don't
> > > see any kernel oops on boot.
> >
> > Nice!
> >
> > Mind if I take it and clean it up a bit and test with it here?  Again, I
> > need this functionality for other stuff as well, so getting it merged
> > for your feature is fine with me.
> 
> Sure! Go ahead. Sorry I was travelling last week.
> 
> >
> > > I prefer the approach I have in this mailing list patch. But if you
> > > like the commit mentioned above I can tidy and clean it up and then
> > > use that instead
> >
> > I would rather do it this way.  I can work on this on Friday if you want
> > me to.
> 
> Yeah, that's fine with me. If you aren't able to let me know and I can
> finish up the patch and send it with this series

Great, and for the email record, as github links are not stable, here's
the patch that you have above attached below.  I'll test this out and
clean it up a bit more and see how it goes...

thanks,

greg k-h