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?
> (2) providing a mechanism to trigger this driver to rescan a PCI
> device later from user space
If this was a regular device driver and -EPROBE_DEFER was returned if
IPMI drivers aren't loaded yet, then this would be easy to solve.
But neither is the case here.
Of course it's possible to exercise the "remove" and "rescan" attributes
in sysfs to re-enumerate the device but that's not a great solution.
> (3) don't cache the active LEDs--just get the active states using
> NPEM/DSM when brightness is read, and do a get/modify/set when
> setting the brightness... then get_active_indications() wouldn't
> need to be called during init.
Not good. The LEDs are published in sysfs from a subsys_initcall().
Brightness changes through sysfs could in theory immediately happen
once they're published. If acpi_ipmi is a module and gets loaded way
later, we'd still have to cope with Set State or Get State DSMs going
nowhere.
Thanks,
Lukas
