| Message ID | 20240320084317.366853-1-kai.heng.feng@canonical.com |
|---|---|
| State | Accepted |
| Commit | 670e98a34a9e44cd384bafbda681c8c8e072b714 |
| Headers | show |
| Series | [v5,1/2] ACPI: IPMI: Add helper to wait for when SMI is selected | expand |
On Wed, Mar 20, 2024 at 04:43:17PM +0800, Kai-Heng Feng wrote: > The following error can be observed at boot: > [ 3.717920] ACPI Error: No handler for Region [SYSI] (00000000ab9e62c5) [IPMI] (20230628/evregion-130) > [ 3.717928] ACPI Error: Region IPMI (ID=7) has no handler (20230628/exfldio-261) > > [ 3.717936] No Local Variables are initialized for Method [_GHL] > > [ 3.717938] No Arguments are initialized for method [_GHL] > > [ 3.717940] ACPI Error: Aborting method \_SB.PMI0._GHL due to previous error (AE_NOT_EXIST) (20230628/psparse-529) > [ 3.717949] ACPI Error: Aborting method \_SB.PMI0._PMC due to previous error (AE_NOT_EXIST) (20230628/psparse-529) > [ 3.717957] ACPI: \_SB_.PMI0: _PMC evaluation failed: AE_NOT_EXIST > > On Dell systems several methods of acpi_power_meter access variables in > IPMI region [0], so wait until IPMI space handler is installed by > acpi_ipmi and also wait until SMI is selected to make the space handler > fully functional. > > Since the dependency is inside BIOS's ASL code and it's not > discoverable, so use this fixup is a hack to workaround BIOS issue. ... > + if (dmi_match(DMI_SYS_VENDOR, "Dell Inc.")) { > + ipi_device = acpi_dev_get_first_match_dev("IPI0001", NULL, -1); > + > + if (ipi_device) { > + if (acpi_wait_for_acpi_ipmi()) > + dev_warn(&device->dev, "Waiting for ACPI IPMI timeout"); > + acpi_dev_put(ipi_device); > + } Can be written as if (dmi_match(DMI_SYS_VENDOR, "Dell Inc.")) { ipi_device = acpi_dev_get_first_match_dev("IPI0001", NULL, -1); if (ipi_device && acpi_wait_for_acpi_ipmi()) dev_warn(&device->dev, "Waiting for ACPI IPMI timeout"); acpi_dev_put(ipi_device); > + }
On 3/25/24 05:38, Andy Shevchenko wrote: > On Wed, Mar 20, 2024 at 04:43:17PM +0800, Kai-Heng Feng wrote: >> The following error can be observed at boot: >> [ 3.717920] ACPI Error: No handler for Region [SYSI] (00000000ab9e62c5) [IPMI] (20230628/evregion-130) >> [ 3.717928] ACPI Error: Region IPMI (ID=7) has no handler (20230628/exfldio-261) >> >> [ 3.717936] No Local Variables are initialized for Method [_GHL] >> >> [ 3.717938] No Arguments are initialized for method [_GHL] >> >> [ 3.717940] ACPI Error: Aborting method \_SB.PMI0._GHL due to previous error (AE_NOT_EXIST) (20230628/psparse-529) >> [ 3.717949] ACPI Error: Aborting method \_SB.PMI0._PMC due to previous error (AE_NOT_EXIST) (20230628/psparse-529) >> [ 3.717957] ACPI: \_SB_.PMI0: _PMC evaluation failed: AE_NOT_EXIST >> >> On Dell systems several methods of acpi_power_meter access variables in >> IPMI region [0], so wait until IPMI space handler is installed by >> acpi_ipmi and also wait until SMI is selected to make the space handler >> fully functional. >> >> Since the dependency is inside BIOS's ASL code and it's not >> discoverable, so use this fixup is a hack to workaround BIOS issue. > > ... > >> + if (dmi_match(DMI_SYS_VENDOR, "Dell Inc.")) { >> + ipi_device = acpi_dev_get_first_match_dev("IPI0001", NULL, -1); >> + >> + if (ipi_device) { >> + if (acpi_wait_for_acpi_ipmi()) >> + dev_warn(&device->dev, "Waiting for ACPI IPMI timeout"); >> + acpi_dev_put(ipi_device); >> + } > > Can be written as > > if (dmi_match(DMI_SYS_VENDOR, "Dell Inc.")) { > ipi_device = acpi_dev_get_first_match_dev("IPI0001", NULL, -1); > if (ipi_device && acpi_wait_for_acpi_ipmi()) > dev_warn(&device->dev, "Waiting for ACPI IPMI timeout"); > acpi_dev_put(ipi_device); > >> + } > Ah yes, acpi_dev_put() checks if the parameter is NULL. Good point. I'll make that change, no need to resend. Thanks, Guenter
diff --git a/drivers/acpi/acpi_ipmi.c b/drivers/acpi/acpi_ipmi.c index 0555f68c2dfd..5fba4dab5d08 100644 --- a/drivers/acpi/acpi_ipmi.c +++ b/drivers/acpi/acpi_ipmi.c @@ -22,6 +22,8 @@ MODULE_LICENSE("GPL"); /* the IPMI timeout is 5s */ #define IPMI_TIMEOUT (5000) #define ACPI_IPMI_MAX_MSG_LENGTH 64 +/* 2s should be suffient for SMI being selected */ +#define ACPI_IPMI_SMI_SELECTION_TIMEOUT (2 * HZ) struct acpi_ipmi_device { /* the device list attached to driver_data.ipmi_devices */ @@ -54,6 +56,7 @@ struct ipmi_driver_data { * to this selected global IPMI system interface. */ struct acpi_ipmi_device *selected_smi; + struct completion smi_selection_done; }; struct acpi_ipmi_msg { @@ -463,8 +466,10 @@ static void ipmi_register_bmc(int iface, struct device *dev) if (temp->handle == handle) goto err_lock; } - if (!driver_data.selected_smi) + if (!driver_data.selected_smi) { driver_data.selected_smi = ipmi_device; + complete(&driver_data.smi_selection_done); + } list_add_tail(&ipmi_device->head, &driver_data.ipmi_devices); mutex_unlock(&driver_data.ipmi_lock); @@ -578,6 +583,20 @@ acpi_ipmi_space_handler(u32 function, acpi_physical_address address, return status; } +int acpi_wait_for_acpi_ipmi(void) +{ + long ret; + + ret = wait_for_completion_interruptible_timeout(&driver_data.smi_selection_done, + ACPI_IPMI_SMI_SELECTION_TIMEOUT); + + if (ret <= 0) + return -ETIMEDOUT; + + return 0; +} +EXPORT_SYMBOL_GPL(acpi_wait_for_acpi_ipmi); + static int __init acpi_ipmi_init(void) { int result; @@ -586,6 +605,8 @@ static int __init acpi_ipmi_init(void) if (acpi_disabled) return 0; + init_completion(&driver_data.smi_selection_done); + status = acpi_install_address_space_handler(ACPI_ROOT_OBJECT, ACPI_ADR_SPACE_IPMI, &acpi_ipmi_space_handler, diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h index 5de954e2b18a..5a69cbd58c5e 100644 --- a/include/acpi/acpi_bus.h +++ b/include/acpi/acpi_bus.h @@ -976,11 +976,16 @@ static inline void acpi_put_acpi_dev(struct acpi_device *adev) { acpi_dev_put(adev); } + +int acpi_wait_for_acpi_ipmi(void); + #else /* CONFIG_ACPI */ static inline int register_acpi_bus_type(void *bus) { return 0; } static inline int unregister_acpi_bus_type(void *bus) { return 0; } +static inline int acpi_wait_for_acpi_ipmi(void) { return 0; } + #endif /* CONFIG_ACPI */ #endif /*__ACPI_BUS_H__*/