Re: [PATCH v13 3/4] PCI/DOE: Expose the DOE features via sysfs

[Jonathan Cameron <Jonathan.Cameron@Huawei.com>]

On Tue,  2 Jul 2024 16:04:17 +1000
Alistair Francis <alistair23@gmail.com> 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.
> 
> 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:00        0001:01        0001:02
> 
> Signed-off-by: Alistair Francis <alistair.francis@wdc.com>
> ---
> 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

Can't do that as it is global so you expose the same DOE features for
all DOEs.

Also, I think that this is only processing features on last doe_mb found
for a given device. Fix that and the duplicates problem resurfaces.


>        device_add() handle the sysfs attributes.


> diff --git a/drivers/pci/doe.c b/drivers/pci/doe.c
> index defc4be81bd4..e7b702afce88 100644
> --- a/drivers/pci/doe.c
> +++ b/drivers/pci/doe.c

> +
>  static int pci_doe_wait(struct pci_doe_mb *doe_mb, unsigned long timeout)
>  {
>  	if (wait_event_timeout(doe_mb->wq,
> @@ -687,6 +747,12 @@ void pci_doe_init(struct pci_dev *pdev)
>  {
>  	struct pci_doe_mb *doe_mb;
>  	u16 offset = 0;
> +	struct attribute **sysfs_attrs;
> +	struct device_attribute *attrs;
> +	unsigned long num_features = 0;
> +	unsigned long i;
> +	unsigned long vid, type;
> +	void *entry;
>  	int rc;
>  
>  	xa_init(&pdev->doe_mbs);
> @@ -707,6 +773,45 @@ void pci_doe_init(struct pci_dev *pdev)
>  			pci_doe_destroy_mb(doe_mb);
>  		}
>  	}

The above is looping over multiple DOEs but this just considers last one.
That doesn't look right...

I think this needs to be in the loop and having done that
the duplicate handing may be an issue.  I'm not sure what happens
in that path with a presupplied set of attributes.

> +
> +	if (doe_mb) {
> +		xa_for_each(&doe_mb->feats, i, entry)
> +			num_features++;
> +
> +		sysfs_attrs = kcalloc(num_features + 1, sizeof(*sysfs_attrs), GFP_KERNEL);
> +		if (!sysfs_attrs)
> +			return;
> +
> +		attrs = kcalloc(num_features, sizeof(*attrs), GFP_KERNEL);
> +		if (!attrs) {
> +			kfree(sysfs_attrs);
> +			return;
> +		}
> +
> +		doe_mb->device_attrs = attrs;
> +		doe_mb->sysfs_attrs = sysfs_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;
> +
> +			attrs[i].attr.name = kasprintf(GFP_KERNEL, "%04lx:%02lx", vid, type);
> +			if (!attrs[i].attr.name) {
> +				pci_doe_sysfs_feature_remove(pdev, doe_mb);
> +				return;
> +			}
> +			attrs[i].attr.mode = 0444;
> +			attrs[i].show = pci_doe_sysfs_feature_show;
> +
> +			sysfs_attrs[i] = &attrs[i].attr;
> +		}
> +
> +		sysfs_attrs[num_features] = NULL;
> +
> +		pci_doe_sysfs_group.attrs = sysfs_attrs;
Hmm. Isn't this global?  What if you have multiple devices.

> +	}
>  }
>