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

[Greg KH <gregkh@linuxfoundation.org>]

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@linuxfoundation.org> 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@wunner.de> 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.

thanks,

greg k-h