Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:8 as permitted sender) client-ip=2620:137:e000::3:8;
MIME-Version: 1.0
References: <20230921055531.2028834-1-alistair.francis@wdc.com>
 <20230921055531.2028834-2-alistair.francis@wdc.com> <20231001175331.GA13453@wunner.de>
In-Reply-To: <20231001175331.GA13453@wunner.de>
From:   Alistair Francis <alistair23@gmail.com>
Date:   Fri, 13 Oct 2023 12:55:45 +1000
Message-ID: <CAKmqyKP_duRdpxdDXJAxVuHfDWm1t8ZGZbtzzJBkMGXVSR4KYQ@mail.gmail.com>
Subject: Re: [PATCH v8 2/3] PCI/DOE: Expose the DOE features via sysfs
To:     Lukas Wunner <lukas@wunner.de>
Cc:     bhelgaas@google.com, linux-pci@vger.kernel.org,
        Jonathan.Cameron@huawei.com, alex.williamson@redhat.com,
        christian.koenig@amd.com, kch@nvidia.com,
        gregkh@linuxfoundation.org, logang@deltatee.com,
        linux-kernel@vger.kernel.org, chaitanyak@nvidia.com,
        rdunlap@infradead.org, Alistair Francis <alistair.francis@wdc.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Precedence: bulk

On Mon, Oct 2, 2023 at 3:53=E2=80=AFAM Lukas Wunner <lukas@wunner.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 (DO=
E).
> > 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 abou=
t
> > 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 =3D attrs;
>
> Set this after the xa_for_each() loop to avoid having to reset it
> to NULL on error.
>
>
> > +             attrs[i].show =3D pci_doe_sysfs_feature_show;
> > +
> > +             ret =3D sysfs_add_file_to_group(&dev->kobj, &attrs[i].att=
r,
> > +                                           pci_dev_doe_feature_group.n=
ame);
> > +             if (ret) {
> > +                     attrs[i].show =3D 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