On Tue, 16 Jul 2024 09:37:51 +1000 Alistair Francis <alistair23@xxxxxxxxx> wrote: > On Wed, Jul 10, 2024 at 12:33 PM Alistair Francis <alistair23@xxxxxxxxx> 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@xxxxxxx> > > Reviewed-by: Jonathan Cameron <Jonathan.Cameron@xxxxxxxxxx> > > --- > > v14: > > - Revert back to v12 with extra pci_remove_resource_files() call > > Using dev->groups and device_add() path as discussed earlier [1] > doesn't work as the pci_doe_sysfs_group is global. Why can't we make a per device instance copy that is then modified? If you are going this route then per attribute is_visible becomes pointless as you don't add the ones that aren't visible. There are subsystems that do this for their internal devices. https://elixir.bootlin.com/linux/latest/source/drivers/iio/industrialio-core.c#L2029 is one I'm familiar because I wrote it. That does very complex instantiation of the sysfs groups and still uses dev->groups + device_add() to actually register everything. > > It is possible to create a global pci_doe_sysfs_group that contains > all possible DOE entries on the system and then have the show > functions determine if they should be displayed for that device. > > That would require that everytime we call pci_doe_init() we can check > for any missing entries in pci_doe_sysfs_group.attrs and then > reallocpci_doe_sysfs_group.attrs to add them. Definitely don't do that. FWIW I don't mind the solution here, but I'm not sure the reasoning above is against the right alternative. Jonathan > > It's complex and clunky so we are sticking with the > pci_remove_resource_files() implementation. See [2] for some more > details on this > > 1: https://lore.kernel.org/all/20231019165829.GA1381099@bhelgaas/ > 2: https://patchwork-proxy.ozlabs.org/project/linux-pci/patch/20240702060418.387500-3-alistair.francis@xxxxxxx/ > > > > v13: > > - Drop pci_doe_sysfs_init() and use pci_doe_sysfs_group > > - As discussed in https://lore.kernel.org/all/20231019165829.GA1381099@bhelgaas/ > > we can just modify pci_doe_sysfs_group at the DOE init and let > > device_add() handle the sysfs attributes. > > v12: > > - Drop pci_doe_features_sysfs_attr_visible() > > v11: > > - Gracefully handle multiple entried of same feature > > - Minor fixes and code cleanups > > v10: > > - Rebase to use DEFINE_SYSFS_GROUP_VISIBLE and remove > > special setup function > > v9: > > - Add a teardown function > > - Rename functions to be clearer > > - Tidy up the commit message > > - Remove #ifdef from header > > v8: > > - Inlucde an example in the docs > > - Fixup removing a file that wasn't added > > - Remove a blank line > > v7: > > - Fixup the #ifdefs to keep the test robot happy > > v6: > > - Use "feature" instead of protocol > > - Don't use any devm_* functions > > - Add two more patches to the series > > v5: > > - Return the file name as the file contents > > - Code cleanups and simplifications > > v4: > > - Fixup typos in the documentation > > - Make it clear that the file names contain the information > > - Small code cleanups > > - Remove most #ifdefs > > - Remove extra NULL assignment > > v3: > > - Expose each DOE feature as a separate file > > v2: > > - Add documentation > > - Code cleanups > > > > > > Documentation/ABI/testing/sysfs-bus-pci | 28 +++++ > > drivers/pci/doe.c | 151 ++++++++++++++++++++++++ > > drivers/pci/pci-sysfs.c | 15 +++ > > drivers/pci/pci.h | 10 ++ > > 4 files changed, 204 insertions(+) > > > > diff --git a/Documentation/ABI/testing/sysfs-bus-pci b/Documentation/ABI/testing/sysfs-bus-pci > > index ecf47559f495..65a3238ab701 100644 > > --- a/Documentation/ABI/testing/sysfs-bus-pci > > +++ b/Documentation/ABI/testing/sysfs-bus-pci > > @@ -500,3 +500,31 @@ 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: May 2024 > > +Contact: Linux PCI developers <linux-pci@xxxxxxxxxxxxxxx> > > +Description: > > + This directory contains a list of the supported > > + Data Object Exchange (DOE) features. The features are > > + the file name. The contents of each file is the raw vendor id and > > + data object feature values. > > + > > + The value comes from the device and specifies the vendor and > > + data object type supported. The lower (RHS of the colon) is > > + the data object type in hex. The upper (LHS of the colon) > > + is the vendor ID. > > + > > + As all DOE devices must support the DOE discovery protocol, if > > + DOE is supported you will at least see the doe_discovery file, with > > + this contents > > + > > + # cat doe_features/doe_discovery > > + 0001:00 > > + > > + If the device supports other protocols you will see other files > > + as well. For example is CMA/SPDM and secure CMA/SPDM are supported > > + the doe_features directory will look like this > > + > > + # ls doe_features > > + 0001:01 0001:02 doe_discovery > > 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 > > @@ -14,6 +14,7 @@ > > > > #include <linux/bitfield.h> > > #include <linux/delay.h> > > +#include <linux/device.h> > > #include <linux/jiffies.h> > > #include <linux/mutex.h> > > #include <linux/pci.h> > > @@ -47,6 +48,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 > > */ > > struct pci_doe_mb { > > struct pci_dev *pdev; > > @@ -56,6 +58,10 @@ struct pci_doe_mb { > > wait_queue_head_t wq; > > struct workqueue_struct *work_queue; > > unsigned long flags; > > + > > +#ifdef CONFIG_SYSFS > > + struct device_attribute *sysfs_attrs; > > +#endif > > }; > > > > struct pci_doe_feature { > > @@ -92,6 +98,151 @@ struct pci_doe_task { > > struct pci_doe_mb *doe_mb; > > }; > > > > +#ifdef CONFIG_SYSFS > > +static ssize_t doe_discovery_show(struct device *dev, > > + struct device_attribute *attr, > > + char *buf) > > +{ > > + return sysfs_emit(buf, "0001:00\n"); > > +} > > +DEVICE_ATTR_RO(doe_discovery); > > + > > +static struct attribute *pci_doe_sysfs_feature_attrs[] = { > > + &dev_attr_doe_discovery.attr, > > + NULL > > +}; > > + > > +static bool pci_doe_features_sysfs_group_visible(struct kobject *kobj) > > +{ > > + struct pci_dev *pdev = to_pci_dev(kobj_to_dev(kobj)); > > + struct pci_doe_mb *doe_mb; > > + unsigned long index; > > + > > + xa_for_each(&pdev->doe_mbs, index, doe_mb) { > > + if (!xa_empty(&doe_mb->feats)) > > + return true; > > + } > > + > > + return false; > > +} > > +DEFINE_SIMPLE_SYSFS_GROUP_VISIBLE(pci_doe_features_sysfs) > > + > > +const struct attribute_group pci_doe_sysfs_group = { > > + .name = "doe_features", > > + .attrs = pci_doe_sysfs_feature_attrs, > > + .is_visible = SYSFS_GROUP_VISIBLE(pci_doe_features_sysfs), > > +}; > > + > > +static ssize_t pci_doe_sysfs_feature_show(struct device *dev, > > + struct device_attribute *attr, > > + char *buf) > > +{ > > + return sysfs_emit(buf, "%s\n", attr->attr.name); > > +} > > + > > +static void pci_doe_sysfs_feature_remove(struct pci_dev *pdev, > > + struct pci_doe_mb *doe_mb) > > +{ > > + struct device_attribute *attrs = doe_mb->sysfs_attrs; > > + struct device *dev = &pdev->dev; > > + unsigned long i; > > + void *entry; > > + > > + if (!attrs) > > + return; > > + > > + doe_mb->sysfs_attrs = NULL; > > + xa_for_each(&doe_mb->feats, i, entry) { > > + if (attrs[i].show) > > + sysfs_remove_file_from_group(&dev->kobj, &attrs[i].attr, > > + pci_doe_sysfs_group.name); > > + kfree(attrs[i].attr.name); > > + } > > + kfree(attrs); > > +} > > + > > +static int pci_doe_sysfs_feature_populate(struct pci_dev *pdev, > > + struct pci_doe_mb *doe_mb) > > +{ > > + struct device *dev = &pdev->dev; > > + struct device_attribute *attrs; > > + unsigned long num_features = 0; > > + unsigned long vid, type; > > + unsigned long i; > > + void *entry; > > + int ret; > > + > > + xa_for_each(&doe_mb->feats, i, entry) > > + num_features++; > > + > > + attrs = kcalloc(num_features, sizeof(*attrs), GFP_KERNEL); > > + if (!attrs) > > + return -ENOMEM; > > + > > + doe_mb->sysfs_attrs = attrs; > > + xa_for_each(&doe_mb->feats, i, entry) { > > + sysfs_attr_init(&attrs[i].attr); > > + vid = xa_to_value(entry) >> 8; > > + type = xa_to_value(entry) & 0xFF; > > + > > + if (vid == 0x01 && type == 0x00) { > > + /* DOE Discovery, manually displayed by `dev_attr_doe_discovery` */ > > + continue; > > + } > > + > > + attrs[i].attr.name = kasprintf(GFP_KERNEL, > > + "%04lx:%02lx", vid, type); > > + if (!attrs[i].attr.name) { > > + ret = -ENOMEM; > > + goto fail; > > + } > > + > > + attrs[i].attr.mode = 0444; > > + attrs[i].show = pci_doe_sysfs_feature_show; > > + > > + ret = sysfs_add_file_to_group(&dev->kobj, &attrs[i].attr, > > + pci_doe_sysfs_group.name); > > + if (ret) { > > + attrs[i].show = NULL; > > + if (ret != -EEXIST) > > + goto fail; > > + else > > + kfree(attrs[i].attr.name); > > + } > > + } > > + > > + return 0; > > + > > +fail: > > + pci_doe_sysfs_feature_remove(pdev, doe_mb); > > + return ret; > > +} > > + > > +void pci_doe_sysfs_teardown(struct pci_dev *pdev) > > +{ > > + struct pci_doe_mb *doe_mb; > > + unsigned long index; > > + > > + xa_for_each(&pdev->doe_mbs, index, doe_mb) > > + pci_doe_sysfs_feature_remove(pdev, doe_mb); > > +} > > + > > +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); > > + 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..db795bfe3c56 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,14 @@ 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) { > > + pci_remove_resource_files(pdev); > > + return retval; > > + } > > + } > > + > > /* Expose the PCI resources from this device as files */ > > for (i = 0; i < PCI_STD_NUM_BARS; i++) { > > > > @@ -1661,6 +1673,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, > > }; > > diff --git a/drivers/pci/pci.h b/drivers/pci/pci.h > > index fd44565c4756..3aee231dcb0c 100644 > > --- a/drivers/pci/pci.h > > +++ b/drivers/pci/pci.h > > @@ -189,6 +189,7 @@ extern const struct attribute_group *pci_dev_groups[]; > > extern const struct attribute_group *pci_dev_attr_groups[]; > > extern const struct attribute_group *pcibus_groups[]; > > extern const struct attribute_group *pci_bus_groups[]; > > +extern const struct attribute_group pci_doe_sysfs_group; > > #else > > static inline int pci_create_sysfs_dev_files(struct pci_dev *pdev) { return 0; } > > static inline void pci_remove_sysfs_dev_files(struct pci_dev *pdev) { } > > @@ -196,6 +197,7 @@ static inline void pci_remove_sysfs_dev_files(struct pci_dev *pdev) { } > > #define pci_dev_attr_groups NULL > > #define pcibus_groups NULL > > #define pci_bus_groups NULL > > +#define pci_doe_sysfs_group NULL > > #endif > > > > extern unsigned long pci_hotplug_io_size; > > @@ -333,6 +335,14 @@ static inline void pci_doe_destroy(struct pci_dev *pdev) { } > > static inline void pci_doe_disconnected(struct pci_dev *pdev) { } > > #endif > > > > +#if defined(CONFIG_PCI_DOE) && defined(CONFIG_SYSFS) > > +int pci_doe_sysfs_init(struct pci_dev *pci_dev); > > +void pci_doe_sysfs_teardown(struct pci_dev *pdev); > > +#else > > +static inline int pci_doe_sysfs_init(struct pci_dev *pdev) { return 0; } > > +static inline void pci_doe_sysfs_teardown(struct pci_dev *pdev) { } > > +#endif > > + > > /** > > * pci_dev_set_io_state - Set the new error state if possible. > > * > > -- > > 2.45.2 > >
