linux-kernel - Re: [PATCH v12 3/4] PCI/DOE: Expose the DOE features via sysfs

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Date: Tue, 2 Jul 2024 15:59:21 +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 v12 3/4] PCI/DOE: Expose the DOE features via sysfs

On Mon, Jul 1, 2024 at 9:27 PM Jonathan Cameron
<Jonathan.Cameron@...wei.com> wrote:
>
> On Wed, 26 Jun 2024 14:59:25 +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.
> >
> > As the DOE Discovery feature must always be supported we treat it as a
> > special named attribute case. This allows the usual PCI attribute_group
> > handling to correctly create the doe_features directory when registering
> > pci_doe_sysfs_group (otherwise it doesn't and sysfs_add_file_to_group()
> > will seg fault).
> >
> > 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:01        0001:02        doe_discovery
> >
> > Signed-off-by: Alistair Francis <alistair.francis@....com>
> Hi Alistair,
>
> I think I missed an error path issue in earlier reviews.
>
> Suggestion for minimal fix inline. If that is fine feel
> free to add
>
> Reviewed-by: Jonathan Cameron <Jonathan.Cameron@...wei.com>
>
> > diff --git a/drivers/pci/doe.c b/drivers/pci/doe.c
> > index defc4be81bd4..580370dc71ee 100644
> > --- a/drivers/pci/doe.c
> > +++ b/drivers/pci/doe.c
>
>
> > +
> > +int pci_doe_sysfs_init(struct pci_dev *pdev)
> > +{
> > +     struct pci_doe_mb *doe_mb;
> > +     unsigned long index;
> > +     int ret;
> > +
> > +     xa_for_each(&pdev->doe_mbs, index, doe_mb) {
> > +             ret = pci_doe_sysfs_feature_populate(pdev, doe_mb);
>
> This doesn't feel quite right.  If we wait after a doe_mb features
> set succeeds and then an error occurs this code doesn't cleanup and...
>
> > +             if (ret)
> > +                     return ret;
> > +     }
> > +
> > +     return 0;
> > +}
> > +#endif
> > +
> >  static int pci_doe_wait(struct pci_doe_mb *doe_mb, unsigned long timeout)
> >  {
> >       if (wait_event_timeout(doe_mb->wq,
> > diff --git a/drivers/pci/pci-sysfs.c b/drivers/pci/pci-sysfs.c
> > index 40cfa716392f..b5db191cb29f 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>
> > @@ -1143,6 +1144,9 @@ static void pci_remove_resource_files(struct pci_dev *pdev)
> >  {
> >       int i;
> >
> > +     if (IS_ENABLED(CONFIG_PCI_DOE))
> > +             pci_doe_sysfs_teardown(pdev);
> > +
> >       for (i = 0; i < PCI_STD_NUM_BARS; i++) {
> >               struct bin_attribute *res_attr;
> >
> > @@ -1227,6 +1231,12 @@ static int pci_create_resource_files(struct pci_dev *pdev)
> >       int i;
> >       int retval;
> >
> > +     if (IS_ENABLED(CONFIG_PCI_DOE)) {
> > +             retval = pci_doe_sysfs_init(pdev);
> > +             if (retval)
>
> ... this doesn't call pci_remove_resource_files() unlike te
> other error path in this function which does.
>
> I think just calling that here would be sufficient and inline
> with how error cleanup works for the rest of this code.
> Personally I prefer driving for a function to have no side effects
> but such is life.

Thanks. While looking at this I realised we can actually drop
pci_doe_sysfs_init() entirely (with a few other changes). So I have
done that in v13.

Alistair

>
> > +                     return retval;
> > +     }
> > +
> >       /* Expose the PCI resources from this device as files */
> >       for (i = 0; i < PCI_STD_NUM_BARS; i++) {
> >
> > @@ -1661,6 +1671,9 @@ const struct attribute_group *pci_dev_attr_groups[] = {
> >  #endif
> >  #ifdef CONFIG_PCIEASPM
> >       &aspm_ctrl_attr_group,
> > +#endif
> > +#ifdef CONFIG_PCI_DOE
> > +     &pci_doe_sysfs_group,
> >  #endif
> >       NULL,
> >  };
>