Re: [PATCH v3] PCI/DOE: Expose the DOE protocols via sysfs

[Chaitanya Kulkarni]

On 8/9/2023 4:28 PM, 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
> implemented. The protocol allows a requester to obtain information about
> the other DOE protocols supported by the device.
> 
> The kernel is already querying the DOE protocols supported and cacheing
> the values. This patch exposes the values via sysfs. This will allow
> userspace to determine which DOE protocols 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 protocols we can
> allow userspace to parse and support the list, which might include
> vendor specific protocols as well as yet to be supported protocols.
> 
> Signed-off-by: Alistair Francis <alistair.francis@xxxxxxx>
> ---
> v3:
>   - Expose each DOE feature as a separate file
> 

it will be nice to have entire version history ...

>   Documentation/ABI/testing/sysfs-bus-pci |  10 +++
>   drivers/pci/doe.c                       | 107 ++++++++++++++++++++++++
>   drivers/pci/pci-sysfs.c                 |   7 ++
>   include/linux/pci-doe.h                 |   1 +
>   4 files changed, 125 insertions(+)
> 
> diff --git a/Documentation/ABI/testing/sysfs-bus-pci b/Documentation/ABI/testing/sysfs-bus-pci
> index ecf47559f495..e754b8efdb69 100644
> --- a/Documentation/ABI/testing/sysfs-bus-pci
> +++ b/Documentation/ABI/testing/sysfs-bus-pci
> @@ -500,3 +500,13 @@ 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_proto
> +Date:		July 2023
> +Contact:	Linux PCI developers <linux-pci@xxxxxxxxxxxxxxx>
> +Description:
> +		This diectory contains a list of the supported Data Object Exchange (DOE)
> +		features. Each feature is a single file.
> +		The value comes from the device and specifies the vendor and
> +		data object type supported. The lower byte is the data object type and the next
> +		two bytes are the vendor ID.

can you run spell check above ?

> diff --git a/drivers/pci/doe.c b/drivers/pci/doe.c
> index 1b97a5ab71a9..d5cbcb6a457a 100644
> --- a/drivers/pci/doe.c
> +++ b/drivers/pci/doe.c
> @@ -56,6 +56,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_protocol {
> @@ -92,6 +96,109 @@ struct pci_doe_task {
>   	struct pci_doe_mb *doe_mb;
>   };
>   
> +#ifdef CONFIG_SYSFS
> +static struct attribute *pci_dev_doe_proto_attrs[] = {
> +	NULL,
> +};
> +
> +static const struct attribute_group pci_dev_doe_proto_group = {
> +	.name	= "doe_protos",
> +	.attrs	= pci_dev_doe_proto_attrs,
> +};
> +
> +static void pci_doe_sysfs_remove_desc(struct pci_doe_mb *doe_mb)
> +{
> +	struct device_attribute *attrs = doe_mb->sysfs_attrs;
> +	unsigned long i;
> +	void *entry;
> +
> +	if (!attrs)
> +		return;
> +

nit : following reads well and matches next NULL assignment, feel free
to ignore :-

	if (!doe_mb->sysfs_attrs)
		return;


> +	doe_mb->sysfs_attrs = NULL;
> +	xa_for_each(&doe_mb->prots, i, entry)
> +		kfree(attrs[i].attr.name);
> +
> +	kfree(attrs);
> +}
> +
> +static int pci_doe_sysfs_proto_supports(struct pci_dev *pdev, struct pci_doe_mb *doe_mb)
> +{
> +	struct device_attribute *attrs;
> +	struct device *dev = &pdev->dev;
> +	unsigned long i;
> +	int ret;
> +	unsigned long num_protos = 0;
> +	unsigned long vid, type;
> +	void *entry;
> +

cosmetic: feel free to ignore but I'd use, feel free to ignore :-

	struct device *dev = &pdev->dev;
	struct device_attribute *attrs;
	unsigned long num_protos = 0;
	unsigned long vid, type;
	unsigned long i;
	void *entry;
	int ret;

> +	xa_for_each(&doe_mb->prots, i, entry)
> +		num_protos++;
> +
> +	attrs = kcalloc(num_protos, sizeof(*attrs), GFP_KERNEL);
> +	if (!attrs)
> +		return -ENOMEM;
> +
> +	doe_mb->sysfs_attrs = attrs;
> +	xa_for_each(&doe_mb->prots, 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, "0x%04lX:%02lX", vid, type);
> +		if (!attrs[i].attr.name) {
> +			ret = -ENOMEM;
> +			goto fail;
> +		}
> +
> +		attrs[i].attr.mode = 0444;
> +		attrs[i].show = NULL;
> +
> +		ret = sysfs_add_file_to_group(&dev->kobj, &attrs[i].attr,
> +					      pci_dev_doe_proto_group.name);
> +		if (ret)
> +			goto fail;
> +	}
> +
> +	return 0;
> +
> +fail:
> +	pci_doe_sysfs_remove_desc(doe_mb);
> +	return ret;
> +}
> +
> +int doe_sysfs_init(struct pci_dev *pdev)
> +{
> +	unsigned long index, j;
> +	int ret;
> +	struct pci_doe_mb *doe_mb;
> +	unsigned long total_protos = 0;
> +	void *entry;
> +

cosmetic: same as above feel free to ignore this comment :-

	unsigned long total_protos = 0;
	struct pci_doe_mb *doe_mb;
	unsigned long index, j;
	void *entry;
	int ret;

> +	xa_for_each(&pdev->doe_mbs, index, doe_mb) {
> +		xa_for_each(&doe_mb->prots, j, entry)
> +			total_protos++;
> +	}
> +
> +	if (total_protos == 0)
> +		return 0;
> +
> +	ret = devm_device_add_group(&pdev->dev, &pci_dev_doe_proto_group);
> +	if (ret) {
> +		pci_err(pdev, "can't create DOE goup: %d\n", ret);
> +		return ret;
> +	}
> +
> +	xa_for_each(&pdev->doe_mbs, index, doe_mb) {
> +		ret = pci_doe_sysfs_proto_supports(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 ab32a91f287b..cb25aba081bc 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>
> @@ -1226,6 +1227,12 @@ static int pci_create_resource_files(struct pci_dev *pdev)
>   	int i;
>   	int retval;
>   
> +#ifdef CONFIG_PCI_DOE
> +	retval = doe_sysfs_init(pdev);
> +	if (retval)
> +		return retval;
> +#endif
> +

how about following? unless there is a bug then ignore this comment ..

	if (IS_ENABLED(CONFIG_PCI_DOE)) {
		retval = doe_sysfs_init(pdev);
		if (retval)
			return retval;
	}

>   	/* Expose the PCI resources from this device as files */
>   	for (i = 0; i < PCI_STD_NUM_BARS; i++) {
>   
> diff --git a/include/linux/pci-doe.h b/include/linux/pci-doe.h
> index 1f14aed4354b..4cc13d9ccb50 100644
> --- a/include/linux/pci-doe.h
> +++ b/include/linux/pci-doe.h
> @@ -22,4 +22,5 @@ int pci_doe(struct pci_doe_mb *doe_mb, u16 vendor, u8 type,
>   	    const void *request, size_t request_sz,
>   	    void *response, size_t response_sz);
>   
> +int doe_sysfs_init(struct pci_dev *pci_dev);
>   #endif

-ck