Received-SPF: pass (google.com: domain of linux-kernel+bounces-16021-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45e3:2400::1 as permitted sender) client-ip=2604:1380:45e3:2400::1;
Message-ID: <430501f6-47fc-49c8-8db8-9cbdf2ba1ae5@gmx.de>
Date: Wed, 3 Jan 2024 22:23:44 +0100
Precedence: bulk
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: [PATCH v2] hwmon: (acpi_power_meter) Ensure IPMI space handler is
 ready on Dell systems
Content-Language: en-US
To: Kai-Heng Feng <kai.heng.feng@canonical.com>, jdelvare@suse.com,
 linux@roeck-us.net
Cc: "Rafael J. Wysocki" <rafael@kernel.org>, Len Brown <lenb@kernel.org>,
 Robert Moore <robert.moore@intel.com>, linux-acpi@vger.kernel.org,
 linux-kernel@vger.kernel.org, linux-hwmon@vger.kernel.org,
 acpica-devel@lists.linux.dev
References: <20231227040401.548977-1-kai.heng.feng@canonical.com>
From: Armin Wolf <W_Armin@gmx.de>
In-Reply-To: <20231227040401.548977-1-kai.heng.feng@canonical.com>
Content-Type: text/plain; charset=UTF-8; format=flowed
Content-Transfer-Encoding: quoted-printable
UI-OutboundReport: notjunk:1;M01:P0:jWNTYdIKL4k=;Fa7BTiBq3zDax/4BRvmo2KH5vrz
 /yCgBJSzTB8wgOlP+Em2Bx9ExqMlAWF0MpgJyai3aS6ETaNa3rXO2aeEuySGdXqzblmIxKi2Y
 Ktlb0Pavd819YcwaRORdJfxRfJV4+6Ivac0zBCtkMBEg6DeEQm6joGQXN20vJrqMAM8hDKLJg
 Y9k4UJh2FHEmdsaDuk/P9NHXdfFJ559V4G04Ahot6BNIZeBqPOSuKmMVjPyFs+zWM5IrziaiU
 NfC/2SJomw3Nn1zCiBK285gWlqyjQb+YtLC8pqN9o/7W4tHeH97mykcmAtXVCCvgrG9LMYfTp
 h1EsI28Sd+uKeic/rSyJuNkg3mbdHSlEaE2KJ86jBOKQ+WfK2jHfUvqI7ibt/i2snphf7tSa9
 dtTknPIN/JmuWdXb1B5saA8F+aNCrFGxRDIeiR+hFRbfLFUWUBdnpwjd5bW02sL/1v9mdow/Q
 0yL9At3mKYTOQ+onNnU+tGd5QDH//MQccqtHz82Ai8kGSQW4INAX0+qqXYC6dwVecNQ+uRHTq
 IunZW7PHRw4Jb1pY5l3nnVuMLC0O4nV3LHU4nF0TbZ0ROCR84sP7fRVbbkJncy/R4hv9WlbE5
 W6lidre87uJtmTtGw6yaQzvxlYcIpAuPk23wNNEXQX0pJHSD1i66fsmUjDXDqqUvPkUAr6TCp
 BOjp+2IsgdguhzfnB+mtfAx6DYds4LLFj8bbZjiE3B90WcXIfDGY1SLEp3/8axHIOVE8xmury
 VPeEQDkPhwJbX9xmZ6gjR8hntFUYD0a5D7bJqnD79qVo+xufPFBElAgdBNv9hlqcT7cA3Yd0b
 RmJzPBsjvswA2dfXhDH82wMh2Q/360NzfAoHLwiRKqy5QEacjD8wzP/1yfv8/QLjYMef4E/Mx
 BDXGa4YEOnWEN2X7dw3YIsZOVGqRb5XTecjIfvUU5vZD21XQ+rjsKPNYsO27U+iZVnospSm7O
 tKkR5/Okt/3wM3zu8vVdiXEyPco=

Am 27.12.23 um 05:04 schrieb Kai-Heng Feng:

> The following error can be observed at boot:
> [    3.717920] ACPI Error: No handler for Region [SYSI] (00000000ab9e62c=
5) [IPMI] (20230628/evregion-130)
> [    3.717928] ACPI Error: Region IPMI (ID=3D7) 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 previou=
s error (AE_NOT_EXIST) (20230628/psparse-529)
> [    3.717949] ACPI Error: Aborting method \_SB.PMI0._PMC due to previou=
s 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.

Hi,

could it be that the ACPI firmware expects us to support the ACPI SPMI tab=
le?
The ACPI SPMI table contains a description of the IPMI interface used by t=
he platform,
and its purpose seems to be similar to the ACPI ECDT table in that it allo=
ws the OS
to access IPMI resources before all ACPI namespaces are enumerated.

Adding support for this table would solve this problem without stalling th=
e boot
process by waiting for the ACPI IPMI device to probe. It would also avoid =
any issues
should the ACPI IPMI device be removed later.

Armin Wolf

>
> [0] https://www.dell.com/support/manuals/en-us/redhat-enterprise-linux-v=
8.0/rhel8_rn_pub/advanced-configuration-and-power-interface-acpi-error-mes=
sages-displayed-in-dmesg?guid=3Dguid-0d5ae482-1977-42cf-b417-3ed5c3f5ee62
>
> Signed-off-by: Kai-Heng Feng <kai.heng.feng@canonical.com>
> ---
> v2:
>   - Use completion instead of request_module().
>
>   drivers/acpi/acpi_ipmi.c         | 13 ++++++++++++-
>   drivers/hwmon/acpi_power_meter.c |  4 ++++
>   include/acpi/acpi_bus.h          |  4 ++++
>   3 files changed, 20 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/acpi/acpi_ipmi.c b/drivers/acpi/acpi_ipmi.c
> index 0555f68c2dfd..2ea8b7e6cebf 100644
> --- a/drivers/acpi/acpi_ipmi.c
> +++ b/drivers/acpi/acpi_ipmi.c
> @@ -23,6 +23,8 @@ MODULE_LICENSE("GPL");
>   #define IPMI_TIMEOUT			(5000)
>   #define ACPI_IPMI_MAX_MSG_LENGTH	64
>
> +static struct completion smi_selected;
> +
>   struct acpi_ipmi_device {
>   	/* the device list attached to driver_data.ipmi_devices */
>   	struct list_head head;
> @@ -463,8 +465,10 @@ static void ipmi_register_bmc(int iface, struct dev=
ice *dev)
>   		if (temp->handle =3D=3D handle)
>   			goto err_lock;
>   	}
> -	if (!driver_data.selected_smi)
> +	if (!driver_data.selected_smi) {
>   		driver_data.selected_smi =3D ipmi_device;
> +		complete(&smi_selected);
> +	}
>   	list_add_tail(&ipmi_device->head, &driver_data.ipmi_devices);
>   	mutex_unlock(&driver_data.ipmi_lock);
>
> @@ -578,10 +582,17 @@ acpi_ipmi_space_handler(u32 function, acpi_physica=
l_address address,
>   	return status;
>   }
>
> +void wait_for_acpi_ipmi(void)
> +{
> +	wait_for_completion_interruptible_timeout(&smi_selected, 2 * HZ);
> +}
> +EXPORT_SYMBOL_GPL(wait_for_acpi_ipmi);
> +
>   static int __init acpi_ipmi_init(void)
>   {
>   	int result;
>   	acpi_status status;
> +	init_completion(&smi_selected);
>
>   	if (acpi_disabled)
>   		return 0;
> diff --git a/drivers/hwmon/acpi_power_meter.c b/drivers/hwmon/acpi_power=
_meter.c
> index 703666b95bf4..acaf1ae68dc8 100644
> --- a/drivers/hwmon/acpi_power_meter.c
> +++ b/drivers/hwmon/acpi_power_meter.c
> @@ -883,6 +883,10 @@ static int acpi_power_meter_add(struct acpi_device =
*device)
>   	strcpy(acpi_device_class(device), ACPI_POWER_METER_CLASS);
>   	device->driver_data =3D resource;
>
> +	if (dmi_match(DMI_SYS_VENDOR, "Dell Inc.") &&
> +	    acpi_dev_get_first_match_dev("IPI0001", NULL, -1))
> +		wait_for_acpi_ipmi();
> +
>   	res =3D read_capabilities(resource);
>   	if (res)
>   		goto exit_free;
> diff --git a/include/acpi/acpi_bus.h b/include/acpi/acpi_bus.h
> index 1216d72c650f..ed59fb89721e 100644
> --- a/include/acpi/acpi_bus.h
> +++ b/include/acpi/acpi_bus.h
> @@ -655,6 +655,7 @@ bool acpi_device_override_status(struct acpi_device =
*adev, unsigned long long *s
>   bool acpi_quirk_skip_acpi_ac_and_battery(void);
>   int acpi_install_cmos_rtc_space_handler(acpi_handle handle);
>   void acpi_remove_cmos_rtc_space_handler(acpi_handle handle);
> +void wait_for_acpi_ipmi(void);
>   #else
>   static inline bool acpi_device_override_status(struct acpi_device *ade=
v,
>   					       unsigned long long *status)
> @@ -672,6 +673,9 @@ static inline int acpi_install_cmos_rtc_space_handle=
r(acpi_handle handle)
>   static inline void acpi_remove_cmos_rtc_space_handler(acpi_handle hand=
le)
>   {
>   }
> +static inline void wait_for_acpi_ipmi(void)
> +{
> +}
>   #endif
>
>   #if IS_ENABLED(CONFIG_X86_ANDROID_TABLETS)