On Fri, 14 Jun 2024 10:12:43 +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,
One question inline. Feels like I'm either missing something or
the code has evolved in a fashion that left us with a pointless check
on attr visibility.
Jonathan
> diff --git a/drivers/pci/doe.c b/drivers/pci/doe.c
> index defc4be81bd4..9858b709c020 100644
> --- a/drivers/pci/doe.c
> +++ b/drivers/pci/doe.c
> +static umode_t pci_doe_features_sysfs_attr_visible(struct kobject *kobj,
> + struct attribute *a, int n)
> +{
> + struct pci_dev *pdev = to_pci_dev(kobj_to_dev(kobj));
> + struct pci_doe_mb *doe_mb;
> + unsigned long index, j;
> + unsigned long vid, type;
> + void *entry;
> +
> + xa_for_each(&pdev->doe_mbs, index, doe_mb) {
> + xa_for_each(&doe_mb->feats, j, entry) {
I'm confused. What is the intent here?
Given every DOE should have the discovery entry any call of this function
that actually finds a DOE should return a->mode, so why search the
actual entries?
Given absence of the files anyway (due to the directory visible checks)
if there are no DOEs, why not drop this function completely?
> + vid = xa_to_value(entry) >> 8;
> + type = xa_to_value(entry) & 0xFF;
> +
> + if (vid == 0x01 && type == 0x00) {
> + /*
> + * This is the DOE discovery protocol
> + * Every DOE instance must support this, so we
> + * give it a useful name.
> + */
> + return a->mode;
> + }
> + }
> + }
> +
> + return 0;
> +}
