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

[Alistair Francis]

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

Alistair

>
> thanks,
>
> greg k-h