stuart hayes wrote: > > > On 6/18/2024 3:56 AM, Mariusz Tkaczyk wrote: > > On Sat, 15 Jun 2024 12:33:45 +0200 > > Lukas Wunner <lukas@xxxxxxxxx> wrote: > > > >> On Fri, Jun 14, 2024 at 04:06:14PM -0500, stuart hayes wrote: > >>> On 5/28/2024 8:19 AM, Mariusz Tkaczyk wrote: > >>>> +static int pci_npem_init(struct pci_dev *dev, const struct npem_ops *ops, > >>>> + int pos, u32 caps) > >>>> +{ > >> [...] > >>>> + ret = ops->get_active_indications(npem, &active); > >>>> + if (ret) { > >>>> + npem_free(npem); > >>>> + return -EACCES; > >>>> + } > >>> > >>> Failing pci_npem_init() if this ops->get_active_indications() fails > >>> will keep this from working on most (all?) Dell servers, because the > >>> _DSM get/set functions use an IPMI operation region to get/set the > >>> active LEDs, and this is getting run before the IPMI drivers and > >>> acpi_ipmi module (which provides ACPI access to IPMI operation > >>> regions) get loaded. (GET_SUPPORTED_STATES works without IPMI.) > >> > >> CONFIG_ACPI_IPMI is tristate. Even if it's built-in, the > >> module_initcall() becomes a device_initcall(). > >> > >> PCI enumeration happens from a subsys_initcall(), way earlier > >> than device_initcall(). > >> > >> If you set CONFIG_ACPI_IPMI=y and change the module_initcall() in > >> drivers/acpi/acpi_ipmi.c to arch_initcall(), does the issue go away? > > > > That seems to be the best option. Please test Lukas proposal and let me know. > > Shouldn't I make a dependency to ACPI_IPMI in Kconfig (with optional comment > > about initcall)? > > > > +config PCI_NPEM > > + bool "Native PCIe Enclosure Management" > > + depends on LEDS_CLASS=y > > + depends on ACPI_IPMI=y > > > > I tried it just to be sure, but changing the module_initcall() to an > arch_initcall() in acpi_ipmi.c (and compiling it into the kernel) doesn't > help... acpi_ipmi is loaded before npem, but the underlying IPMI drivers > that acpi_ipmi uses to provide the IPMI operation region in ACPI aren't > loaded until later... acpi_ipmi needs the IPMI device. > > I'll try to think of another solution. I don't see how to get the IPMI > drivers to load before PCI devices are enumerated, so it seems to me that > the only way to get it to work from the moment the LEDs show up in sysfs > is to somehow delay the npem driver initialization of the LEDs (at least > for _DSM) or use something to trigger a rescan later. > > I notice that acpi_ipmi provides a function to wait for it to have an > IPMI device registered (acpi_wait_for_acpi_ipmi()), which is used by > acpi_power_meter.c. It would be kind of awkward to use that... it just > just waits for a completion (with a 2 second timeout)--it isn't a notifier > or callback. On my system, the npem driver failed to run a _DSM at > 0.72 seconds, and ipmi_si driver didn't initialize the IPMI controller > until 9.54 seconds. It strikes me that playing these initcall games is a losing battle and that this case would be best served by late loading of NPEM functionality. Something similar is happening with PCI device security where the enabling depends on a third-party driver for a platform "security-manager" (TSM) to arrive. The approach there is to make the functionality independent of device-discovery vs TSM driver load order. So, if the TSM driver is loaded early then pci_init_capabilities() can immediately enable the functionality. If the TSM driver is loaded *after* some devices have already gone through pci_init_capabilities(), then that event is responsible for doing for_each_pci_dev() to catch up on devices that missed their initial chance to turn on device security details. So, for NPEM, the thought would be to implement the same rendezvous flow, i.e. s/TSM/NPEM/. I am an overdue for a refresh of the TSM patches, but you can see the last posting here: http://lore.kernel.org/171291190324.3532867.13480405752065082171.stgit@xxxxxxxxxxxxxxxxxxxxxxxxx