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

[Alistair Francis]

On Tue, Jul 25, 2023 at 11:00 AM Jonathan Cameron
<Jonathan.Cameron@xxxxxxxxxx> wrote:
>
> On Tue, 25 Jul 2023 13:57:55 +1000
> Alistair Francis <alistair23@xxxxxxxxx> wrote:
>
> > The PCIe 6 specification added support for the Data Object Exchange (DOE).
> > When DOE is supported the Discovery Data Object Protocol must be
> > implemented. The protocol allows a requester to obtain information about
> > the other DOE protocols supported by the device.
> >
> > The kernel is already querying the DOE protocols supported and cacheing
> > the values. This patch exposes the values via sysfs. This will allow
> > userspace to determine which DOE protocols are supported by the PCIe
> > device.
> >
> > Signed-off-by: Alistair Francis <alistair.francis@xxxxxxx>
>
> Hi Alistair,
>
> Needs documentation.
> Documentation/ABI/testing/sys-bus-pci probably the right place.

Ok, I'll add some

>
> Also, I wonder if we want to associate each DOE with the protocols
> it supports rather than clumping them together in one file...

In the future there could be vendor IDs or even standardised IDs that
the current kernel doesn't support/understand. So my thinking was that
by exposing all of the supported protocols userspace can use that
information as it wishes.

Then I wanted to work on supporting specific DOE protocols in the
kernel and exposing that to userspace. But I think a single sysfs
makes sense as a base point to convey information.

> Otherwise we'll loose information on sharing + we may well see
> repeated entries as it's fine to have more than one instance of
> given protocol.
>
> A few more comments inline about what happens when we do run out
> of space in the buffer.
>
> Thanks,
>
> Jonathan
>
> > ---
> >  drivers/pci/doe.c       | 28 ++++++++++++++++++++++++++++
> >  drivers/pci/pci-sysfs.c | 27 +++++++++++++++++++++++++++
> >  include/linux/pci-doe.h |  2 ++
> >  3 files changed, 57 insertions(+)
> >
> > diff --git a/drivers/pci/doe.c b/drivers/pci/doe.c
> > index 1b97a5ab71a9..cc1c23c78ac1 100644
> > --- a/drivers/pci/doe.c
> > +++ b/drivers/pci/doe.c
> > @@ -563,6 +563,34 @@ static bool pci_doe_supports_prot(struct pci_doe_mb *doe_mb, u16 vid, u8 type)
> >       return false;
> >  }
> >
> > +/**
> > + * pci_doe_sysfs_proto_supports() - Write the supported DOE protocols
> > + *                        to a sysfs buffer
> > + * @doe_mb: DOE mailbox capability to query
> > + * @buf: buffer to store the sysfs strings
> > + * @offset: offset in buffer to store the sysfs strings
> > + *
> > + * RETURNS: The number of bytes written, 0 means an error occured
> > + */
> > +unsigned long pci_doe_sysfs_proto_supports(struct pci_doe_mb *doe_mb,
> > +                                        char *buf, ssize_t offset)
> > +{
> > +     unsigned long index;
> > +     ssize_t ret = offset, r;
>
> I find that hard to parse.  Maybe
>
>         ssize_t ret = offset;
>         ssize_t r;
>
>
>
> > +     void *entry;
> > +
> > +     xa_for_each(&doe_mb->prots, index, entry) {
> > +             r = sysfs_emit_at(buf, ret, "0x%08lX\n", xa_to_value(entry));
> > +
> > +             if (r == 0)
> > +                     return 0;
>
> return ret here? Otherwise we aren't outputting everything that we have
> managed to print before the buffer fills up.
> It's also inconsistent because if the last entry happens to partly print
> we'll let it through whereas, if it doesn't fit at all we will drop
> all the info about this DOE instance.

Good point, I'll update this and the variable declarations

Alistair

>
>
> > +
> > +             ret += r;
> > +     }
> > +
> > +     return ret;
> > +}
> > +
> >  /**
> >   * pci_doe_submit_task() - Submit a task to be processed by the state machine
> >   *
> > diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
> > index ab32a91f287b..df93051e65bf 100644
> > --- a/drivers/pci/pci-sysfs.c
> > +++ b/drivers/pci/pci-sysfs.c
> > @@ -16,6 +16,7 @@
> >  #include <linux/kernel.h>
> >  #include <linux/sched.h>
> >  #include <linux/pci.h>
> > +#include <linux/pci-doe.h>
> >  #include <linux/stat.h>
> >  #include <linux/export.h>
> >  #include <linux/topology.h>
> > @@ -290,6 +291,29 @@ static ssize_t modalias_show(struct device *dev, struct device_attribute *attr,
> >  }
> >  static DEVICE_ATTR_RO(modalias);
> >
> > +#ifdef CONFIG_PCI_DOE
> > +static ssize_t doe_proto_show(struct device *dev, struct device_attribute *attr,
> > +                          char *buf)
> > +{
> > +     struct pci_dev *pci_dev = to_pci_dev(dev);
> > +     unsigned long index;
> > +     ssize_t ret = 0, r;
>
> As above.
>
> > +     struct pci_doe_mb *doe_mb;
> > +
> > +     xa_for_each(&pci_dev->doe_mbs, index, doe_mb) {
> > +             r = pci_doe_sysfs_proto_supports(doe_mb, buf, ret);
> > +
> > +             if (r == 0)
> > +                     return 0;
>
> As above, return ret here I think makes more sense than 0.
>
>
> > +
> > +             ret += r;
> > +     }
> > +
> > +     return ret;
> > +}
> > +static DEVICE_ATTR_RO(doe_proto);
> > +#endif
> > +
> >  static ssize_t enable_store(struct device *dev, struct device_attribute *attr,
> >                            const char *buf, size_t count)
> >  {
> > @@ -603,6 +627,9 @@ static struct attribute *pci_dev_attrs[] = {
> >       &dev_attr_local_cpus.attr,
> >       &dev_attr_local_cpulist.attr,
> >       &dev_attr_modalias.attr,
> > +#ifdef CONFIG_PCI_DOE
> > +     &dev_attr_doe_proto.attr,
> > +#endif
> >  #ifdef CONFIG_NUMA
> >       &dev_attr_numa_node.attr,
> >  #endif
> > diff --git a/include/linux/pci-doe.h b/include/linux/pci-doe.h
> > index 1f14aed4354b..066494a4dba3 100644
> > --- a/include/linux/pci-doe.h
> > +++ b/include/linux/pci-doe.h
> > @@ -21,5 +21,7 @@ struct pci_doe_mb *pci_find_doe_mailbox(struct pci_dev *pdev, u16 vendor,
> >  int pci_doe(struct pci_doe_mb *doe_mb, u16 vendor, u8 type,
> >           const void *request, size_t request_sz,
> >           void *response, size_t response_sz);
> > +unsigned long pci_doe_sysfs_proto_supports(struct pci_doe_mb *doe_mb,
> > +                                        char *buf, ssize_t offset);
> >
> >  #endif
>