Received-SPF: pass (google.com: domain of linux-kernel+bounces-215890-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Date: Sat, 15 Jun 2024 15:05:29 +0200
From: Lukas Wunner <lukas@wunner.de>
To: Alistair Francis <alistair23@gmail.com>
Cc: bhelgaas@google.com, linux-pci@vger.kernel.org,
	Jonathan.Cameron@huawei.com, alex.williamson@redhat.com,
	christian.koenig@amd.com, kch@nvidia.com,
	gregkh@linuxfoundation.org, logang@deltatee.com,
	linux-kernel@vger.kernel.org, chaitanyak@nvidia.com,
	rdunlap@infradead.org, Alistair Francis <alistair.francis@wdc.com>
Subject: Re: [PATCH v11 3/4] PCI/DOE: Expose the DOE features via sysfs
Message-ID: <Zm2RmWnSWEEX8WtV@wunner.de>
References: <20240614001244.925401-1-alistair.francis@wdc.com>
 <20240614001244.925401-3-alistair.francis@wdc.com>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20240614001244.925401-3-alistair.francis@wdc.com>

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