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
