Re: [PATCH v9 2/3] PCI/DOE: Expose the DOE features via sysfs

[Lukas Wunner]

On Thu, Oct 19, 2023 at 11:58:29AM -0500, Bjorn Helgaas wrote:
> On Fri, Oct 13, 2023 at 01:41:57PM +1000, Alistair Francis wrote:
> > +	xa_for_each(&pdev->doe_mbs, index, doe_mb) {
> > +		xa_for_each(&doe_mb->feats, j, entry)
> > +			return a->mode;
> > +	}
> > +
> > +	return 0;
> 
> The nested loops that don't test anything look a little weird and
> maybe I'm missing something, but this looks like it returns a->mode if
> any mailbox with a feature exists, and 0 otherwise.
> 
> Is that the same as this:
> 
>   if (pdev->doe_mbs)
>     return a->mode;
> 
>   return 0;
> 
> since it sounds like a mailbox must support at least one feature?

In theory it's the same, in practice there *might* be non-compliant
devices which lack support for the discovery feature.


> > +		attrs[i].attr.name = kasprintf(GFP_KERNEL,
> > +					       "0x%04lX:%02lX", vid, type);
> 
> What's the rationale for using "0x" on the vendor ID but not on the
> type?  "0x1234:10" hints that the "10" might be decimal since it lacks
> "0x".
> 
> Suggest lower-case "%04lx:%02lx" either way.
> 
> FWIW, there's no "0x" prefix on the hex vendor IDs in "lspci -n"
> output and dmesg messages like this:
> 
>   pci 0000:01:00.0: [10de:13b6] type 00

The existing attributes "vendor", "device" etc do emit the "0x".

>From drivers/pci/pci-sysfs.c:

pci_config_attr(vendor, "0x%04x\n");
pci_config_attr(device, "0x%04x\n");
pci_config_attr(subsystem_vendor, "0x%04x\n");
pci_config_attr(subsystem_device, "0x%04x\n");
pci_config_attr(revision, "0x%02x\n");
pci_config_attr(class, "0x%06x\n");


> I try hard to avoid calling *anything* from the
> pci_create_sysfs_dev_files() path because it has the nasty
> "sysfs_initialized" check and the associated pci_sysfs_init()
> initcall.

What's the purpose of sysfs_initialized anyway?

It was introduced by this historic commit:
https://git.kernel.org/tglx/history/c/f6d553444da2

Can PCI_ROM_RESOURCEs appear after device enumeration but before
the late_initcall stage?

If sysfs_initialized is only needed for PCI_ROM_RESOURCEs, can we
constrain pci_sysfs_init() to those and avoid creating all the
other runtime sysfs attributes in the initcall?

Thanks,

Lukas