diff mbox series

[v2] hwmon: (acpi_power_meter) Ensure IPMI space handler is ready on Dell systems

Message ID 20231227040401.548977-1-kai.heng.feng@canonical.com
State New
Headers show
Series [v2] hwmon: (acpi_power_meter) Ensure IPMI space handler is ready on Dell systems | expand

Commit Message

Kai-Heng Feng Dec. 27, 2023, 4:04 a.m. UTC
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.

[0] https://www.dell.com/support/manuals/en-us/redhat-enterprise-linux-v8.0/rhel8_rn_pub/advanced-configuration-and-power-interface-acpi-error-messages-displayed-in-dmesg?guid=guid-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(-)

Comments

Guenter Roeck Dec. 27, 2023, 9:57 p.m. UTC | #1
On Wed, Dec 27, 2023 at 12:04:00PM +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.
> 
> [0] https://www.dell.com/support/manuals/en-us/redhat-enterprise-linux-v8.0/rhel8_rn_pub/advanced-configuration-and-power-interface-acpi-error-messages-displayed-in-dmesg?guid=guid-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 ++++

This needs to be split into two patches.

>  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 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(&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_physical_address address,
>  	return status;
>  }
>  
> +void wait_for_acpi_ipmi(void)
> +{
> +	wait_for_completion_interruptible_timeout(&smi_selected, 2 * HZ);

wait_for_completion_interruptible_timeout) returns an error code which
should be returned to the caller.

> +}
> +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 = resource;
>  
> +	if (dmi_match(DMI_SYS_VENDOR, "Dell Inc.") &&
> +	    acpi_dev_get_first_match_dev("IPI0001", NULL, -1))
> +		wait_for_acpi_ipmi();

wait_for_acpi_ipmi() should return an error code which
should be handled here.

> +
>  	res = 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 *adev,
>  					       unsigned long long *status)
> @@ -672,6 +673,9 @@ static inline int acpi_install_cmos_rtc_space_handler(acpi_handle handle)
>  static inline void acpi_remove_cmos_rtc_space_handler(acpi_handle handle)
>  {
>  }
> +static inline void wait_for_acpi_ipmi(void)
> +{
> +}

Something with the conditional is wrong. See 0-day report.

Guenter
>  #endif
>  
>  #if IS_ENABLED(CONFIG_X86_ANDROID_TABLETS)
> -- 
> 2.34.1
> 
>
Kai-Heng Feng Jan. 3, 2024, 3:36 a.m. UTC | #2
On Thu, Dec 28, 2023 at 5:58 AM Guenter Roeck <linux@roeck-us.net> wrote:
>
> On Wed, Dec 27, 2023 at 12:04:00PM +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.
> >
> > [0] https://www.dell.com/support/manuals/en-us/redhat-enterprise-linux-v8.0/rhel8_rn_pub/advanced-configuration-and-power-interface-acpi-error-messages-displayed-in-dmesg?guid=guid-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 ++++
>
> This needs to be split into two patches.

Will do.

>
> >  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 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(&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_physical_address address,
> >       return status;
> >  }
> >
> > +void wait_for_acpi_ipmi(void)
> > +{
> > +     wait_for_completion_interruptible_timeout(&smi_selected, 2 * HZ);
>
> wait_for_completion_interruptible_timeout) returns an error code which
> should be returned to the caller.

Will do in next revision.

>
> > +}
> > +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 = resource;
> >
> > +     if (dmi_match(DMI_SYS_VENDOR, "Dell Inc.") &&
> > +         acpi_dev_get_first_match_dev("IPI0001", NULL, -1))
> > +             wait_for_acpi_ipmi();
>
> wait_for_acpi_ipmi() should return an error code which
> should be handled here.

OK, I think print an message should be informative.

>
> > +
> >       res = 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 *adev,
> >                                              unsigned long long *status)
> > @@ -672,6 +673,9 @@ static inline int acpi_install_cmos_rtc_space_handler(acpi_handle handle)
> >  static inline void acpi_remove_cmos_rtc_space_handler(acpi_handle handle)
> >  {
> >  }
> > +static inline void wait_for_acpi_ipmi(void)
> > +{
> > +}
>
> Something with the conditional is wrong. See 0-day report.

Will fix that in next revision.

Kai-Heng

>
> Guenter
> >  #endif
> >
> >  #if IS_ENABLED(CONFIG_X86_ANDROID_TABLETS)
> > --
> > 2.34.1
> >
> >
Armin Wolf Jan. 3, 2024, 9:23 p.m. UTC | #3
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] (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.

Hi,

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

Adding support for this table would solve this problem without stalling the 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-v8.0/rhel8_rn_pub/advanced-configuration-and-power-interface-acpi-error-messages-displayed-in-dmesg?guid=guid-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 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(&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_physical_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 = resource;
>
> +	if (dmi_match(DMI_SYS_VENDOR, "Dell Inc.") &&
> +	    acpi_dev_get_first_match_dev("IPI0001", NULL, -1))
> +		wait_for_acpi_ipmi();
> +
>   	res = 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 *adev,
>   					       unsigned long long *status)
> @@ -672,6 +673,9 @@ static inline int acpi_install_cmos_rtc_space_handler(acpi_handle handle)
>   static inline void acpi_remove_cmos_rtc_space_handler(acpi_handle handle)
>   {
>   }
> +static inline void wait_for_acpi_ipmi(void)
> +{
> +}
>   #endif
>
>   #if IS_ENABLED(CONFIG_X86_ANDROID_TABLETS)
Kai-Heng Feng Jan. 4, 2024, 2:14 a.m. UTC | #4
On Thu, Jan 4, 2024 at 5:23 AM Armin Wolf <W_Armin@gmx.de> wrote:
>
> 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] (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.
>
> Hi,
>
> could it be that the ACPI firmware expects us to support the ACPI SPMI table?
> The ACPI SPMI table contains a description of the IPMI interface used by the platform,
> and its purpose seems to be similar to the ACPI ECDT table in that it allows the OS
> to access IPMI resources before all ACPI namespaces are enumerated.
>
> Adding support for this table would solve this problem without stalling the 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.

Hi, the ACPI firmware on the system doesn't have SPMI table:
$ ls /sys/firmware/acpi/tables/
APIC  BERT  data  DMAR  DSDT  dynamic  EINJ  ERST  FACP  FACS  HEST
HMAT  HPET  MCFG  MIGT  MSCT  NBFT  OEM4  SLIC  SLIT  SRAT  SSDT1
SSDT2  SSDT3  SSDT4  SSDT5  SSDT6  TPM2  UEFI1  UEFI2  VFCT  WSMT

Kai-Heng

>
> Armin Wolf
>
> >
> > [0] https://www.dell.com/support/manuals/en-us/redhat-enterprise-linux-v8.0/rhel8_rn_pub/advanced-configuration-and-power-interface-acpi-error-messages-displayed-in-dmesg?guid=guid-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 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(&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_physical_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 = resource;
> >
> > +     if (dmi_match(DMI_SYS_VENDOR, "Dell Inc.") &&
> > +         acpi_dev_get_first_match_dev("IPI0001", NULL, -1))
> > +             wait_for_acpi_ipmi();
> > +
> >       res = 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 *adev,
> >                                              unsigned long long *status)
> > @@ -672,6 +673,9 @@ static inline int acpi_install_cmos_rtc_space_handler(acpi_handle handle)
> >   static inline void acpi_remove_cmos_rtc_space_handler(acpi_handle handle)
> >   {
> >   }
> > +static inline void wait_for_acpi_ipmi(void)
> > +{
> > +}
> >   #endif
> >
> >   #if IS_ENABLED(CONFIG_X86_ANDROID_TABLETS)
Armin Wolf Jan. 4, 2024, 7:11 a.m. UTC | #5
Am 04.01.24 um 03:14 schrieb Kai-Heng Feng:

> On Thu, Jan 4, 2024 at 5:23 AM Armin Wolf <W_Armin@gmx.de> wrote:
>> 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] (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.
>> Hi,
>>
>> could it be that the ACPI firmware expects us to support the ACPI SPMI table?
>> The ACPI SPMI table contains a description of the IPMI interface used by the platform,
>> and its purpose seems to be similar to the ACPI ECDT table in that it allows the OS
>> to access IPMI resources before all ACPI namespaces are enumerated.
>>
>> Adding support for this table would solve this problem without stalling the 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.
> Hi, the ACPI firmware on the system doesn't have SPMI table:
> $ ls /sys/firmware/acpi/tables/
> APIC  BERT  data  DMAR  DSDT  dynamic  EINJ  ERST  FACP  FACS  HEST
> HMAT  HPET  MCFG  MIGT  MSCT  NBFT  OEM4  SLIC  SLIT  SRAT  SSDT1
> SSDT2  SSDT3  SSDT4  SSDT5  SSDT6  TPM2  UEFI1  UEFI2  VFCT  WSMT
>
> Kai-Heng

Thanks for checking this, in this case you might ignore my suggestion above.

Armin Wolf

>> Armin Wolf
>>
>>> [0] https://www.dell.com/support/manuals/en-us/redhat-enterprise-linux-v8.0/rhel8_rn_pub/advanced-configuration-and-power-interface-acpi-error-messages-displayed-in-dmesg?guid=guid-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 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(&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_physical_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 = resource;
>>>
>>> +     if (dmi_match(DMI_SYS_VENDOR, "Dell Inc.") &&
>>> +         acpi_dev_get_first_match_dev("IPI0001", NULL, -1))
>>> +             wait_for_acpi_ipmi();
>>> +
>>>        res = 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 *adev,
>>>                                               unsigned long long *status)
>>> @@ -672,6 +673,9 @@ static inline int acpi_install_cmos_rtc_space_handler(acpi_handle handle)
>>>    static inline void acpi_remove_cmos_rtc_space_handler(acpi_handle handle)
>>>    {
>>>    }
>>> +static inline void wait_for_acpi_ipmi(void)
>>> +{
>>> +}
>>>    #endif
>>>
>>>    #if IS_ENABLED(CONFIG_X86_ANDROID_TABLETS)
diff mbox series

Patch

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 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(&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_physical_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 = resource;
 
+	if (dmi_match(DMI_SYS_VENDOR, "Dell Inc.") &&
+	    acpi_dev_get_first_match_dev("IPI0001", NULL, -1))
+		wait_for_acpi_ipmi();
+
 	res = 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 *adev,
 					       unsigned long long *status)
@@ -672,6 +673,9 @@  static inline int acpi_install_cmos_rtc_space_handler(acpi_handle handle)
 static inline void acpi_remove_cmos_rtc_space_handler(acpi_handle handle)
 {
 }
+static inline void wait_for_acpi_ipmi(void)
+{
+}
 #endif
 
 #if IS_ENABLED(CONFIG_X86_ANDROID_TABLETS)