Re: [PATCH v2] PCI: Remove redudant call to pci_proc_attach_device()

[Krzysztof Wilczyński]

Hello,

> > Hence, remove the call to pci_proc_attach_device() from pci_proc_init().
> 
> Seems plausible, but given that this code has been this way basically
> forever, I'd sure like to figure out what's different about Shuan and
> Lorenzo's system that makes this issue show up now.

Sadly, removing the for_each_pci_dev() loop breaks x86/ACPI systems
where it's the only mechanism creating per-device procfs entries.

On x86 with ACPI, PCI enumeration happens at subsys_initcall (level 4),
two levels before pci_proc_init() runs at device_initcall (level 6).

When the pci_bus_add_device() calls pci_proc_attach_device() during
enumeration, proc_initialized is still 0, so the call returns -EACCES
and no entry is created:

  subsys_initcall (level 4):                                                                                                                                                                                      
    acpi_init()                                                                                                                                                                                                 
      acpi_scan_init()                                                                                                                                                                                            
        acpi_bus_scan()                                                                                                                                                                                           
          acpi_pci_root_add()                                                                                                                                                                                     
            pci_acpi_scan_root()             <- devices are discovered                                                                                                                                              
            pci_bus_add_devices()
              pci_bus_add_device()
                pci_proc_attach_device()
                  if (!proc_initialized)     <- variable is not yet set
                    return -EACCES

  device_initcall (level 6):
    pci_proc_init()
      proc_initialized = 1
      for_each_pci_dev()                     <- iterates existing devices
        pci_proc_attach_device()             <- creates the procfs entries

Without that loop, the /proc/bus/pci/XX/YY.Z entries are never created
on these systems.  This can be verified under QEMU q35 with ACPI, where
removing the loop results in an empty /proc/bus/pci directory.

On platforms using Devicetree, the loop is usually not a problem, as host
bridges probe at the same device_initcall level, and with the current link
order, the pci_proc_init() runs first, so the loop finds zero new devices
and then pci_bus_add_device() creates the entries inline.  The race happens
when async probing causes both paths to overlap.

The fix needs to keep the loop but make it safe.  I have a patch that
wraps it with pci_lock_rescan_remove() and adds a simple dev->procent
idempotency check to pci_proc_attach_device(), see:

  https://lore.kernel.org/linux-pci/20260430003542.455198-1-kwilczynski@xxxxxxxxxx

The lock serialises against concurrent pci_bus_add_device() calls,
and the dev->procent check makes the function idempotent so that
if both paths reach the same device, the second one returns early.
This also keeps code symmetric with pci_proc_detach_device(),
which already clears dev->procent.

Let me know what do you think, and if you have some other ideas.  Perhaps
there is a better way to do this.  Sadly, no static procfs attributes
exist for procfs, so this avenue is not available to us.

Thank you!

	Krzysztof