diff mbox series

[v5,1/2] ACPI: IPMI: Add helper to wait for when SMI is selected

Message ID 20240320084317.366853-1-kai.heng.feng@canonical.com
State New
Headers show
Series [v5,1/2] ACPI: IPMI: Add helper to wait for when SMI is selected | expand

Commit Message

Kai-Heng Feng March 20, 2024, 8:43 a.m. UTC 
On Dell servers, many APCI methods of acpi_power_meter module evaluate
variables inside IPMI region, so the region handler needs to be
installed. In addition to that, the handler needs to be fully
functional, and that depends on SMI being selected.

So add a helper to let acpi_power_meter know when the handler is
installed and ready to be used.

Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
Acked-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
v5:
 - No change.

v4:
 - Wording.
 - Define and comment on timeout value.
 - Move the completion to driver_data.
 - Remove the tenary operator.

v3:
 - New patch.

 drivers/acpi/acpi_ipmi.c | 23 ++++++++++++++++++++++-
 include/acpi/acpi_bus.h  |  5 +++++
 2 files changed, 27 insertions(+), 1 deletion(-)

Comments

Andy Shevchenko March 25, 2024, 12:38 p.m. UTC | #1 
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);

> +	}
Guenter Roeck March 25, 2024, 2:40 p.m. UTC | #2 
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 mbox series

Patch

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__*/