Date: Fri, 5 Jul 2024 11:24:25 +1000
From: Alistair Francis <alistair23@...il.com>
To: Jonathan Cameron <Jonathan.Cameron@...wei.com>
Cc: bhelgaas@...gle.com, linux-pci@...r.kernel.org, lukas@...ner.de, 
	alex.williamson@...hat.com, christian.koenig@....com, kch@...dia.com, 
	gregkh@...uxfoundation.org, logang@...tatee.com, linux-kernel@...r.kernel.org, 
	chaitanyak@...dia.com, rdunlap@...radead.org, 
	Alistair Francis <alistair.francis@....com>
Subject: Re: [PATCH v13 3/4] PCI/DOE: Expose the DOE features via sysfs

On Tue, Jul 2, 2024 at 11:58 PM Jonathan Cameron
<Jonathan.Cameron@...wei.com> wrote:
>
> On Tue,  2 Jul 2024 16:04:17 +1000
> Alistair Francis <alistair23@...il.com> wrote:
>
> > The PCIe 6 specification added support for the Data Object
> > Exchange (DOE).
> > When DOE is supported the DOE Discovery Feature must be implemented per
> > PCIe r6.1 sec 6.30.1.1. The protocol allows a requester to obtain
> > information about the other DOE features supported by the device.
> >
> > The kernel is already querying the DOE features supported and cacheing
> > the values. Expose the values in sysfs to allow user space to
> > determine which DOE features are supported by the PCIe device.
> >
> > By exposing the information to userspace tools like lspci can relay the
> > information to users. By listing all of the supported features we can
> > allow userspace to parse the list, which might include
> > vendor specific features as well as yet to be supported features.
> >
> > After this patch is supported you can see something like this when
> > attaching a DOE device
> >
> > $ ls /sys/devices/pci0000:00/0000:00:02.0//doe*
> > 0001:00        0001:01        0001:02
> >
> > Signed-off-by: Alistair Francis <alistair.francis@....com>
> > ---
> > v13:
> >  - Drop pci_doe_sysfs_init() and use pci_doe_sysfs_group
> >      - As discussed in https://lore.kernel.org/all/20231019165829.GA1381099@bhelgaas/
> >        we can just modify pci_doe_sysfs_group at the DOE init and let
>
> Can't do that as it is global so you expose the same DOE features for
> all DOEs.
>
> Also, I think that this is only processing features on last doe_mb found
> for a given device. Fix that and the duplicates problem resurfaces.
>
>
> >        device_add() handle the sysfs attributes.
>
>
> > diff --git a/drivers/pci/doe.c b/drivers/pci/doe.c
> > index defc4be81bd4..e7b702afce88 100644
> > --- a/drivers/pci/doe.c
> > +++ b/drivers/pci/doe.c
>
> > +
> >  static int pci_doe_wait(struct pci_doe_mb *doe_mb, unsigned long timeout)
> >  {
> >       if (wait_event_timeout(doe_mb->wq,
> > @@ -687,6 +747,12 @@ void pci_doe_init(struct pci_dev *pdev)
> >  {
> >       struct pci_doe_mb *doe_mb;
> >       u16 offset = 0;
> > +     struct attribute **sysfs_attrs;
> > +     struct device_attribute *attrs;
> > +     unsigned long num_features = 0;
> > +     unsigned long i;
> > +     unsigned long vid, type;
> > +     void *entry;
> >       int rc;
> >
> >       xa_init(&pdev->doe_mbs);
> > @@ -707,6 +773,45 @@ void pci_doe_init(struct pci_dev *pdev)
> >                       pci_doe_destroy_mb(doe_mb);
> >               }
> >       }
>
> The above is looping over multiple DOEs but this just considers last one.
> That doesn't look right...

Yeah... That isn't

>
> I think this needs to be in the loop and having done that
> the duplicate handing may be an issue.  I'm not sure what happens
> in that path with a presupplied set of attributes.
>
> > +
> > +     if (doe_mb) {
> > +             xa_for_each(&doe_mb->feats, i, entry)
> > +                     num_features++;
> > +
> > +             sysfs_attrs = kcalloc(num_features + 1, sizeof(*sysfs_attrs), GFP_KERNEL);
> > +             if (!sysfs_attrs)
> > +                     return;
> > +
> > +             attrs = kcalloc(num_features, sizeof(*attrs), GFP_KERNEL);
> > +             if (!attrs) {
> > +                     kfree(sysfs_attrs);
> > +                     return;
> > +             }
> > +
> > +             doe_mb->device_attrs = attrs;
> > +             doe_mb->sysfs_attrs = sysfs_attrs;
> > +
> > +             xa_for_each(&doe_mb->feats, i, entry) {
> > +                     sysfs_attr_init(&attrs[i].attr);
> > +
> > +                     vid = xa_to_value(entry) >> 8;
> > +                     type = xa_to_value(entry) & 0xFF;
> > +
> > +                     attrs[i].attr.name = kasprintf(GFP_KERNEL, "%04lx:%02lx", vid, type);
> > +                     if (!attrs[i].attr.name) {
> > +                             pci_doe_sysfs_feature_remove(pdev, doe_mb);
> > +                             return;
> > +                     }
> > +                     attrs[i].attr.mode = 0444;
> > +                     attrs[i].show = pci_doe_sysfs_feature_show;
> > +
> > +                     sysfs_attrs[i] = &attrs[i].attr;
> > +             }
> > +
> > +             sysfs_attrs[num_features] = NULL;
> > +
> > +             pci_doe_sysfs_group.attrs = sysfs_attrs;
> Hmm. Isn't this global?  What if you have multiple devices.

Any input from a PCI maintainer here?

There are basically two approaches.

 1. We can have a pci_doe_sysfs_init() function that is called where
we dynamically add the entries, like in v12
 2. We can go down the dev->groups and device_add() path, like this
patch and discussed at
https://lore.kernel.org/all/20231019165829.GA1381099@bhelgaas/

For the second we will have to create a global pci_doe_sysfs_group
that contains all possible DOE entries on the system and then have the
show functions determine if they should be displayed for that device.

Everytime we call pci_doe_init() we can check for any missing entries
in pci_doe_sysfs_group.attrs and then realloc
pci_doe_sysfs_group.attrs to add them. Untested, but that should work
even for hot-plugged devices. pci_doe_sysfs_group.attrs would just
grow forever though as I don't think we have an easy way to deallocate
anything as we aren't sure if we are the only entry.

Thoughts?

Alistair

Powered by blists - more mailing lists