[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <2023082325-cognitive-dispose-1180@gregkh>
Date: Wed, 23 Aug 2023 09:02:36 +0200
From: Greg KH <gregkh@...uxfoundation.org>
To: Alistair Francis <alistair23@...il.com>
Cc: bhelgaas@...gle.com, linux-pci@...r.kernel.org,
Jonathan.Cameron@...wei.com, lukas@...ner.de,
alex.williamson@...hat.com, christian.koenig@....com,
kch@...dia.com, logang@...tatee.com, linux-kernel@...r.kernel.org,
chaitanyak@...dia.com, rdunlap@...radead.org,
Alistair Francis <alistair.francis@....com>
Subject: Re: [PATCH v6 2/3] sysfs: Add a attr_is_visible function to
attribute_group
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@...uxfoundation.org> 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.
> 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.
thanks,
greg k-h
Powered by blists - more mailing lists