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
