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
