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
