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

[Alistair Francis <Alistair.Francis@wdc.com>]

On Sat, 2024-06-15 at 15:05 +0200, Lukas Wunner wrote:
> 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?"

I prefer the `doe_discovery` as it's clear what the protocol is, but if
it's preferred I can change it to `0001:00`

Alistair

> 
> 
> > +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