linux-kernel - Re: [PATCH v8 2/3] 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]

Message-ID: <CAKmqyKP_duRdpxdDXJAxVuHfDWm1t8ZGZbtzzJBkMGXVSR4KYQ@mail.gmail.com>
Date:   Fri, 13 Oct 2023 12:55:45 +1000
From:   Alistair Francis <alistair23@...il.com>
To:     Lukas Wunner <lukas@...ner.de>
Cc:     bhelgaas@...gle.com, linux-pci@...r.kernel.org,
        Jonathan.Cameron@...wei.com, 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 v8 2/3] PCI/DOE: Expose the DOE features via sysfs

On Mon, Oct 2, 2023 at 3:53 AM Lukas Wunner <lukas@...ner.de> wrote:
>
> On Thu, Sep 21, 2023 at 03:55:30PM +1000, Alistair Francis wrote:
> > The PCIe 6 specification added support for the Data Object Exchange (DOE).
> > When DOE is supported the Discovery Data Object Protocol must be
>
> "... the DOE Discovery *Feature* must be implemented per PCIe r6.1
> sec 6.30.1.1"
>
>
> > implemented. 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. This patch exposes the values via sysfs. This will allow
>
> Instead of "This patch ...", prefer imperative mood, i.e.:
> "Expose the values in sysfs to allow user space to ..."
>
>
> > --- a/Documentation/ABI/testing/sysfs-bus-pci
> > +++ b/Documentation/ABI/testing/sysfs-bus-pci
> > @@ -500,3 +500,26 @@ Description:
> >               console drivers from the device.  Raw users of pci-sysfs
> >               resourceN attributes must be terminated prior to resizing.
> >               Success of the resizing operation is not guaranteed.
> > +
> > +What:                /sys/bus/pci/devices/.../doe_features
> > +Date:                August 2023
>
> Date says August but patch submission is from September.
>
>
> > --- a/drivers/pci/doe.c
> > +++ b/drivers/pci/doe.c
> > @@ -47,6 +47,7 @@
> >   * @wq: Wait queue for work item
> >   * @work_queue: Queue of pci_doe_work items
> >   * @flags: Bit array of PCI_DOE_FLAG_* flags
> > + * @sysfs_attrs: Array of sysfs device attributes
>
> What's the purpose of this pointer?  It's set in
> pci_doe_sysfs_feature_supports() but never used for anything.
>
> I'm guessing that you meant to use it to tear down the added attributes
> on device removal, but that's missing in the patch.

Yeah, the tear down got lost in one of the revisions. I have added it
back and reworked the patch to use the teardown.

>
> The attributes are added with sysfs_add_file_to_group(), but it seems
> to me they're not automatically removed by sysfs_remove_groups() on
> device teardown.  Am I missing something?
>
>
> > +static int pci_doe_sysfs_feature_supports(struct pci_dev *pdev,
> > +                                       struct pci_doe_mb *doe_mb)
>
> I don't quite understand the meaning of the function name:
> It sounds as if its purpose is to determine whether a feature
> is supported.  Maybe something like pci_doe_sysfs_add_features()
> instead?

I changed it to pci_doe_sysfs_feature_populate() which should be clear
and matches some similar functions in the kernel.

Alistair

>
>
> > +     doe_mb->sysfs_attrs = attrs;
>
> Set this after the xa_for_each() loop to avoid having to reset it
> to NULL on error.
>
>
> > +             attrs[i].show = pci_doe_sysfs_feature_show;
> > +
> > +             ret = sysfs_add_file_to_group(&dev->kobj, &attrs[i].attr,
> > +                                           pci_dev_doe_feature_group.name);
> > +             if (ret) {
> > +                     attrs[i].show = NULL;
> > +                     goto fail;
> > +             }
>
> The purpose of resetting attrs[i].show to NULL in the error path
> seems to be that you want to skip over features which haven't
> been created as attributes yet.
>
> It seems more straightforward to just iterate over the elements
> in attrs[] until you reach one whose mode is 0.
>
> Alternatively, use xa_for_each_range(&doe_mb->feats, i, entry, 0, i - 1).
>
>
> > +int doe_sysfs_init(struct pci_dev *pdev)
>
> Rename to pci_doe_sysfs_init() for consistency.
>
>
> > --- a/drivers/pci/pci.h
> > +++ b/drivers/pci/pci.h
> > @@ -186,6 +186,9 @@ extern const struct attribute_group *pci_dev_groups[];
> >  extern const struct attribute_group *pcibus_groups[];
> >  extern const struct device_type pci_dev_type;
> >  extern const struct attribute_group *pci_bus_groups[];
> > +#ifdef CONFIG_SYSFS
> > +extern const struct attribute_group pci_dev_doe_feature_group;
> > +#endif
>
> No #ifdef necessary.
>
> Thanks,
>
> Lukas