On Mon, Oct 2, 2023 at 3:53 AM Lukas Wunner <lukas@xxxxxxxxx> 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
