linux-kernel - Re: [PATCH v11 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] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAKmqyKN+e-cPP+PGQGAZStrBrXt8wPzfCD49wDq6o9cV_TgMuw@mail.gmail.com>
Date: Wed, 26 Jun 2024 13:59:14 +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 v11 3/4] PCI/DOE: Expose the DOE features via sysfs

On Fri, Jun 14, 2024 at 6:59 PM Jonathan Cameron
<Jonathan.Cameron@...wei.com> wrote:
>
> On Fri, 14 Jun 2024 10:12:43 +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,
>
> One question inline.  Feels like I'm either missing something or
> the code has evolved in a fashion that left us with a pointless check
> on attr visibility.

It's the second :)

>
> Jonathan
>
> > diff --git a/drivers/pci/doe.c b/drivers/pci/doe.c
> > index defc4be81bd4..9858b709c020 100644
> > --- a/drivers/pci/doe.c
> > +++ b/drivers/pci/doe.c
>
> > +static umode_t pci_doe_features_sysfs_attr_visible(struct kobject *kobj,
> > +                                                struct attribute *a, int n)
> > +{
> > +     struct pci_dev *pdev = to_pci_dev(kobj_to_dev(kobj));
> > +     struct pci_doe_mb *doe_mb;
> > +     unsigned long index, j;
> > +     unsigned long vid, type;
> > +     void *entry;
> > +
> > +     xa_for_each(&pdev->doe_mbs, index, doe_mb) {
> > +             xa_for_each(&doe_mb->feats, j, entry) {
>
> I'm confused.  What is the intent here?

I feel like this was required at some point, but you are right, it
doesn't seem useful now

>
> Given every DOE should have the discovery entry any call of this function
> that actually finds a DOE should return a->mode, so why search the
> actual entries?
>
> Given absence of the files anyway (due to the directory visible checks)
> if there are no DOEs, why not drop this function completely?

Done!

Alistair

>
> > +                     vid = xa_to_value(entry) >> 8;
> > +                     type = xa_to_value(entry) & 0xFF;
> > +
> > +                     if (vid == 0x01 && type == 0x00) {
> > +                             /*
> > +                              * This is the DOE discovery protocol
> > +                              * Every DOE instance must support this, so we
> > +                              * give it a useful name.
> > +                              */
> > +                             return a->mode;
> > +                     }
> > +             }
> > +     }
> > +
> > +     return 0;
> > +}
>
>
>