Re: [PATCH v2 2/3] PCI/NPEM: Add Native PCIe Enclosure Management support

[Mariusz Tkaczyk <mariusz.tkaczyk@linux.intel.com>]

On Sat, 15 Jun 2024 12:33:45 +0200
Lukas Wunner <lukas@wunner.de> 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

> 
> 
> > (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.

We cannot expect from users to know and do that. If we cannot configure driver,
we should not register it. We have to guarantee that IMPI commands are
available at the point we are using them.

There is not better place to add _DSM device than its enumeration and I have a
feeling than sooner or later someone else will reach this problem so it would
be better for community to solve it now.

> 
> 
> > (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.
> 

Agree. I can do it and it should be safe but it is not addressing the issue.
We would limit time race window but we will not close it.

Thanks,
Mariusz