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

[Greg KH <gregkh@linuxfoundation.org>]

On Tue, Aug 01, 2023 at 08:18:24AM -0400, 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@wdc.com>
> ---
>  Documentation/ABI/testing/sysfs-bus-pci | 11 ++++++
>  drivers/pci/doe.c                       | 52 +++++++++++++++++++++++++
>  drivers/pci/pci-sysfs.c                 |  8 ++++
>  include/linux/pci-doe.h                 |  2 +
>  4 files changed, 73 insertions(+)
> 
> diff --git a/Documentation/ABI/testing/sysfs-bus-pci b/Documentation/ABI/testing/sysfs-bus-pci
> index ecf47559f495..ae969bbfa631 100644
> --- a/Documentation/ABI/testing/sysfs-bus-pci
> +++ b/Documentation/ABI/testing/sysfs-bus-pci
> @@ -500,3 +500,14 @@ 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@vger.kernel.org>
> +Description:
> +		This file contains a list of the supported Data Object Exchange (DOE)
> +		protocols. The protocols are seperated by newlines.
> +		The value comes from the device and specifies the vendor and
> +		protocol supported. The lower byte is the protocol and the next
> +		two bytes are the vendor ID.
> +		The file is read only.

Sorry, but sysfs files are "one value per file", you can't have a "list
of protocols with new lines" in a one value-per-file rule.


> diff --git a/drivers/pci/doe.c b/drivers/pci/doe.c
> index 1b97a5ab71a9..70900b79b239 100644
> --- a/drivers/pci/doe.c
> +++ b/drivers/pci/doe.c
> @@ -563,6 +563,58 @@ static bool pci_doe_supports_prot(struct pci_doe_mb *doe_mb, u16 vid, u8 type)
>  	return false;
>  }
>  
> +#ifdef CONFIG_SYSFS
> +/**
> + * pci_doe_sysfs_proto_supports() - Write the supported DOE protocols
> + *			     to a sysfs buffer
> + * @doe_mb: DOE mailbox capability to query
> + * @buf: buffer to store the sysfs strings
> + * @offset: offset in buffer to store the sysfs strings
> + *
> + * RETURNS: The number of bytes written, 0 means an error occured
> + */
> +static unsigned long pci_doe_sysfs_proto_supports(struct pci_doe_mb *doe_mb,
> +						  char *buf, ssize_t offset)
> +{
> +	unsigned long index;
> +	ssize_t ret = offset;
> +	ssize_t r;
> +	void *entry;
> +
> +	xa_for_each(&doe_mb->prots, index, entry) {
> +		r = sysfs_emit_at(buf, ret, "0x%08lX\n", xa_to_value(entry));
> +

No need for a blank line.

> +		if (r == 0)
> +			return ret;



> +
> +		ret += r;
> +	}
> +
> +	return ret;
> +}
> +
> +ssize_t doe_proto_show(struct device *dev, struct device_attribute *attr,
> +		       char *buf)
> +{
> +	struct pci_dev *pci_dev = to_pci_dev(dev);
> +	unsigned long index;
> +	ssize_t ret = 0;
> +	ssize_t r;
> +	struct pci_doe_mb *doe_mb;
> +
> +	xa_for_each(&pci_dev->doe_mbs, index, doe_mb) {
> +		r = pci_doe_sysfs_proto_supports(doe_mb, buf, ret);
> +
> +		if (r == 0)
> +			return ret;
> +
> +		ret += r;
> +	}

So this is going to be a lot of data, what is ensuring that you didn't
truncate it?  Which again, is the reason why this is not a good idea for
sysfs, sorry.

What userspace tool wants this information?

thanks,

greg k-h