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

[Lukas Wunner]

On Fri, Jun 14, 2024 at 10:12:43AM +1000, Alistair Francis wrote:
> --- a/drivers/pci/doe.c
> +++ b/drivers/pci/doe.c
[...]
> +#ifdef CONFIG_SYSFS
> +static ssize_t doe_discovery_show(struct device *dev,
> +				  struct device_attribute *attr,
> +				  char *buf)
> +{
> +	return sysfs_emit(buf, "0001:00\n");
> +}
> +DEVICE_ATTR_RO(doe_discovery);

If you want to use "0001:00" as filename but can't because
"0001:00_show()" would not be a valid function name in C,
I think there's no harm in manually expanding the macro to:

struct device_attribute dev_attr_doe_discovery = \
	__ATTR(0001:00, 0444, pci_doe_sysfs_feature_show, NULL);

That also avoids the need to have an extra doe_discovery_show()
function.

Intuitively, when I saw there's a "doe_discovery" attribute,
my first thought was: "Oh maybe I need to write something there
to (re-)initiate DOE discovery?"


> +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) {
> +			vid = xa_to_value(entry) >> 8;
> +			type = xa_to_value(entry) & 0xFF;
> +
> +			if (vid == 0x01 && type == 0x00) {

Wherever possible, PCI_VENDOR_ID_PCI_SIG and PCI_DOE_PROTOCOL_DISCOVERY
macros should be used in lieu of 0x0001 and 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;
> +}

I agree with Jonathan that at first glance one would assume that
this function just always returns a->mode.


> +static bool pci_doe_features_sysfs_group_visible(struct kobject *kobj)
> +{
> +	struct pci_dev *pdev = to_pci_dev(kobj_to_dev(kobj));
> +	struct pci_doe_mb *doe_mb;
> +	unsigned long index;
> +
> +	xa_for_each(&pdev->doe_mbs, index, doe_mb) {
> +		if (!xa_empty(&doe_mb->feats))
> +			return true;
> +	}
> +
> +	return false;

So in principle, doe_mb->feats should never be empty because the
discovery protocol is always supported, right?  Wouldn't it then
suffice to just check for:

+	if (!xa_empty(&pdev->doe_mbs))
+		return true;

Or alternatively:

+	return !xa_empty(&pdev->doe_mbs);

Thanks,

Lukas