On Wed, 26 Jun 2024 14:59:25 +1000
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>
Hi Alistair,
I think I missed an error path issue in earlier reviews.
Suggestion for minimal fix inline. If that is fine feel
free to add
Reviewed-by: Jonathan Cameron <Jonathan.Cameron@xxxxxxxxxx>
> 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
> +
> +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);
This doesn't feel quite right. If we wait after a doe_mb features
set succeeds and then an error occurs this code doesn't cleanup and...
> + 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..b5db191cb29f 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,12 @@ 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)
... this doesn't call pci_remove_resource_files() unlike te
other error path in this function which does.
I think just calling that here would be sufficient and inline
with how error cleanup works for the rest of this code.
Personally I prefer driving for a function to have no side effects
but such is life.
> + return retval;
> + }
> +
> /* Expose the PCI resources from this device as files */
> for (i = 0; i < PCI_STD_NUM_BARS; i++) {
>
> @@ -1661,6 +1671,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,
> };
