[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <Zm2RmWnSWEEX8WtV@wunner.de>
Date: Sat, 15 Jun 2024 15:05:29 +0200
From: Lukas Wunner <lukas@...ner.de>
To: Alistair Francis <alistair23@...il.com>
Cc: bhelgaas@...gle.com, linux-pci@...r.kernel.org,
Jonathan.Cameron@...wei.com, alex.williamson@...hat.com,
christian.koenig@....com, kch@...dia.com,
gregkh@...uxfoundation.org, logang@...tatee.com,
linux-kernel@...r.kernel.org, chaitanyak@...dia.com,
rdunlap@...radead.org, Alistair Francis <alistair.francis@....com>
Subject: Re: [PATCH v11 3/4] PCI/DOE: Expose the DOE features via sysfs
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
Powered by blists - more mailing lists