Re: [PATCH v3] PCI/DOE: Expose the DOE protocols via sysfs

[Alistair Francis]

On Tue, Aug 15, 2023 at 4:10 PM Greg KH <gregkh@xxxxxxxxxxxxxxxxxxx> wrote:
>
> On Tue, Aug 15, 2023 at 03:50:31PM -0400, Alistair Francis wrote:
> > On Tue, Aug 15, 2023 at 11:11 AM Greg KH <gregkh@xxxxxxxxxxxxxxxxxxx> wrote:
> > >
> > > On Tue, Aug 15, 2023 at 09:44:32AM -0400, Alistair Francis wrote:
> > > > On Sat, Aug 12, 2023 at 4:26 AM Greg KH <gregkh@xxxxxxxxxxxxxxxxxxx> wrote:
> > > > >
> > > > > On Sat, Aug 12, 2023 at 10:15:26AM +0200, Lukas Wunner wrote:
> > > > > > On Thu, Aug 10, 2023 at 11:34:11AM -0400, Alistair Francis wrote:
> > > > > > > On Thu, Aug 10, 2023 at 3:34???AM Lukas Wunner <lukas@xxxxxxxxx> wrote:
> > > > > > > > On Wed, Aug 09, 2023 at 07:28:51PM -0400, Alistair Francis wrote:
> > > > > > > > > --- a/drivers/pci/pci-sysfs.c
> > > > > > > > > +++ b/drivers/pci/pci-sysfs.c
> > > > > > > > > @@ -1226,6 +1227,12 @@ static int pci_create_resource_files(struct pci_dev *pdev)
> > > > > > > > >       int i;
> > > > > > > > >       int retval;
> > > > > > > > >
> > > > > > > > > +#ifdef CONFIG_PCI_DOE
> > > > > > > > > +     retval = doe_sysfs_init(pdev);
> > > > > > > > > +     if (retval)
> > > > > > > > > +             return retval;
> > > > > > > > > +#endif
> > > > > > > > > +
> > > > > > > >
> > > > > > > > The preferred way to expose PCI sysfs attributes nowadays is to add them
> > > > > > > > to pci_dev_attr_groups[] and use the ->is_visible callback to check
> > > > > > > > whether they're applicable to a particular pci_dev.  The alternative
> > > > > > > > via pci_create_resource_files() has race conditions which I think
> > > > > > > > still haven't been fixed. Bjorn recommended the ->is_visible approach
> > > > > > > > in response to the most recent attempt to fix the race:
> > > > > > > >
> > > > > > > > https://lore.kernel.org/linux-pci/20230427161458.GA249886@bhelgaas/
> > > > > > >
> > > > > > > The is_visible doen't seem to work in this case.
> > > > > > >
> > > > > > > AFAIK is_visible only applies to the attributes under the group. Which
> > > > > > > means that every PCIe device will see a `doe_protos` directory, no
> > > > > > > matter if DOE is supported.
> > > > > >
> > > > > > internal_create_group() in fs/sysfs/group.c does this:
> > > > > >
> > > > > >       if (grp->name) {
> > > > > >                       ...
> > > > > >                       kn = kernfs_create_dir_ns(kobj->sd, grp->name, ...
> > > > > >
> > > > > > So I'm under the impression that if you set the ->name member of
> > > > > > struct attribute_group, the attributes in that group appear under
> > > > > > a directory of that name.
> > > > > >
> > > > > > In fact, the kernel-doc for struct attribute_group claims as much:
> > > > > >
> > > > > >  * struct attribute_group - data structure used to declare an attribute group.
> > > > > >  * @name:     Optional: Attribute group name
> > > > > >  *            If specified, the attribute group will be created in
> > > > > >  *            a new subdirectory with this name.
> > > > > >
> > > > > > So I don't quite understand why you think that "every PCIe device will
> > > > > > see a `doe_protos` directory, no matter if DOE is supported"?
> > > > > >
> > > > > > Am I missing something?
> > > > >
> > > > > I think the issue might be that the directory will be created even if no
> > > > > attributes are present in it due to the is_visable() check not returning
> > > > > any valid files?
> > > >
> > > > Yes, that's what I'm seeing. I see the directory for all PCIe devices
> > > >
> > > > This is a WIP that I had:
> > > > https://github.com/alistair23/linux/commit/61925cd174c31386eaa7e51e3a1be606b38f847c
> > > >
> > > > >
> > > > > If so, I had a patch somewhere around here where I was trying to fix
> > > > > that up:
> > > > >         https://git.kernel.org/pub/scm/linux/kernel/git/gregkh/driver-core.git/commit/?h=debugfs_cleanup&id=f670945dfbaf353fe068544c31e3fa45575da5b5
> > > > > but it didn't seem to work properly and kept crashing.  I didn't spend
> > > > > much time on looking into it, but if this is an issue, I can work on
> > > > > fixing this properly.
> > > >
> > > > That patch sounds like it would fix the issue of empty directories
> > > > that I'm seeing. Do you mind fixing it up properly?
> > >
> > > I am currently unable to do so due to travel and stuff for a few weeks,
> > > sorry.  Feel free to take it and fix the boot crash that is seen with it
> > > and make it part of your patch series if you can't wait that long.
> >
> > No worries.
> >
> > It's much harder than I first thought though. There are currently lots
> > of users who expect the group to remain even if empty, as they
> > dynamically add/merge properties later. Which is what we end up doing
> > for DOE as well
> >
> > I'll keep looking into this and see if I can figure something out.
>
> Yeah, now that I think about it, that's where stuff fell apart for me as
> well.  We should be able to create the group and then only create the
> file when they are added/merged, so I bet I missed a codepath somewhere.

Yeah, it's tricky.

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.

as an empty group isn't created with your patch it doesn't work with
sysfs_merge_group().

I'm assuming we don't want to change those public functions by
creating the group in sysfs_merge_group() if it isn't created.

Creating the group is just creating the directory, so I don't see a
way we can create the group without creating the directory

Alistair

>
> > Would an .attr_is_visible() function pointer for struct
> > attribute_group something that the kernel would accept?
>
> Worst case, yes, that would be acceptable.  But try to see where I
> messed up on the original patch, it should be able to be done
> automatically somehow...
>
> thanks,
>
> greg k-h