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

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

[Kai-Heng Feng]

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

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 1216d72c650f..afa6e4d4bf46 100644
--- a/include/acpi/acpi_bus.h
+++ b/include/acpi/acpi_bus.h
@@ -821,11 +821,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__*/
-- 
2.34.1

[PATCH v4 2/2] hwmon: (acpi_power_meter) Ensure IPMI space handler is ready on Dell systems

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

[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>
---
v4:
 - No change.

v3:
 - Use helper.
 - Use return value to print warning message.

v2:
 - Use completion instead of request_module().

 drivers/hwmon/acpi_power_meter.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/hwmon/acpi_power_meter.c b/drivers/hwmon/acpi_power_meter.c
index 703666b95bf4..33fb9626633d 100644
--- a/drivers/hwmon/acpi_power_meter.c
+++ b/drivers/hwmon/acpi_power_meter.c
@@ -883,6 +883,12 @@ 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)) {
+		if (acpi_wait_for_acpi_ipmi())
+			dev_warn(&device->dev, "Waiting for ACPI IPMI timeout");
+	}
+
 	res = read_capabilities(resource);
 	if (res)
 		goto exit_free;
-- 
2.34.1

Re: [PATCH v4 2/2] hwmon: (acpi_power_meter) Ensure IPMI space handler is ready on Dell systems

[Guenter Roeck]

On 1/8/24 20:12, 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>
> ---
> v4:
>   - No change.
> 
> v3:
>   - Use helper.
>   - Use return value to print warning message.
> 
> v2:
>   - Use completion instead of request_module().
> 
>   drivers/hwmon/acpi_power_meter.c | 6 ++++++
>   1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/hwmon/acpi_power_meter.c b/drivers/hwmon/acpi_power_meter.c
> index 703666b95bf4..33fb9626633d 100644
> --- a/drivers/hwmon/acpi_power_meter.c
> +++ b/drivers/hwmon/acpi_power_meter.c
> @@ -883,6 +883,12 @@ 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)) {
> +		if (acpi_wait_for_acpi_ipmi())
> +			dev_warn(&device->dev, "Waiting for ACPI IPMI timeout");
> +	}
> +

What a hack :-(.

This needs a comment in the driver explaining the rationale for this change, and
also a comment explaining why, for example, using late_initcall() does not help.

If CONFIG_IPMI_SI=n, acpi_wait_for_acpi_ipmi() will return 0, indicating success.
I can only imagine that this will result in a failure since the whole point
of this code is to wait until that driver is loaded. Please explain how and why
the code works with CONFIG_IPMI_SI=n. Similar, if the function returns an error,
I can not imagine how it would make sense to instantiate the driver. If it does
make sense to continue in this situation, a comment is needed in the code
describing the rationale.

Third, the new symbol is declared with CONFIG_ACPI, but defined with
CONFIG_IPMI_SI. I can not imagine how this would compile with CONFIG_ACPI=y
and CONFIG_IPMI_SI={m,n} and/or CONFIG_ACPI_IPMI={m,n}.

On top of that, IPMI_SI and ACPI_IPMI are is tristate, as is SENSORS_ACPI_POWER.
This means that SENSORS_ACPI_POWER=y combined with CONFIG_IPMI_SI={m,n} or
CONFIG_ACPI_IPMI={m,n} will result in a compile failure.

Please make sure that this code compiles with all possible symbol combinations.

Thanks,
Guenter

>   	res = read_capabilities(resource);
>   	if (res)
>   		goto exit_free;

Re: [PATCH v4 1/2] ACPI: IPMI: Add helper to wait for when SMI is selected

[Rafael J. Wysocki]

On Tue, Jan 9, 2024 at 5:12 AM Kai-Heng Feng
<kai.heng.feng@canonical.com> wrote:
>
> 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>

and I'm expecting this to be routed along with patch [2/2] that has
been posted elsewhere.

> ---
> 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(-)
>
> 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 1216d72c650f..afa6e4d4bf46 100644
> --- a/include/acpi/acpi_bus.h
> +++ b/include/acpi/acpi_bus.h
> @@ -821,11 +821,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__*/
> --

Re: [PATCH v4 2/2] hwmon: (acpi_power_meter) Ensure IPMI space handler is ready on Dell systems

[Corey Minyard]

On Tue, Jan 09, 2024 at 07:23:40AM -0800, Guenter Roeck wrote:
> On 1/8/24 20:12, 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>
> > ---
> > v4:
> >   - No change.
> > 
> > v3:
> >   - Use helper.
> >   - Use return value to print warning message.
> > 
> > v2:
> >   - Use completion instead of request_module().
> > 
> >   drivers/hwmon/acpi_power_meter.c | 6 ++++++
> >   1 file changed, 6 insertions(+)
> > 
> > diff --git a/drivers/hwmon/acpi_power_meter.c b/drivers/hwmon/acpi_power_meter.c
> > index 703666b95bf4..33fb9626633d 100644
> > --- a/drivers/hwmon/acpi_power_meter.c
> > +++ b/drivers/hwmon/acpi_power_meter.c
> > @@ -883,6 +883,12 @@ 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)) {
> > +		if (acpi_wait_for_acpi_ipmi())
> > +			dev_warn(&device->dev, "Waiting for ACPI IPMI timeout");
> > +	}
> > +
> 
> What a hack :-(.
> 
> This needs a comment in the driver explaining the rationale for this change, and
> also a comment explaining why, for example, using late_initcall() does not help.
> 
> If CONFIG_IPMI_SI=n, acpi_wait_for_acpi_ipmi() will return 0, indicating success.
> I can only imagine that this will result in a failure since the whole point
> of this code is to wait until that driver is loaded. Please explain how and why
> the code works with CONFIG_IPMI_SI=n. Similar, if the function returns an error,
> I can not imagine how it would make sense to instantiate the driver. If it does
> make sense to continue in this situation, a comment is needed in the code
> describing the rationale.

I'm trying to figure out where CONFIG_IPMI_SI comes in here.  It's
nowhere in these patches or in drivers/acpi.  ACPI_IPMI depends on
IPMI_HANDLER, but that's all I found.  However, ACPI_IPMI can be "m" as
you mention and SENSOR_ACPI_POWER is only under the ACPI config, which
is a problem.

I do think there are other issues with this patch, though.  The IPMI
handler code decouples the user from the driver from a dependency point
of view.  It seems to be fairly common to see IPMI_HANDLER and
ACPI_IPMI as "y" and IPMI_SI (and IPMI_SSIF, and others) as "m".  That
means this code will run but will wait for the IPMI device to appear,
which may not be until the module gets loaded, which may be far more
than 2 seconds later.

I'm not quite sure how to fix this.  Really, the add call for this
driver shouldn't be called until the IPMI device is present.  Doesn't
ACPI have mechanisms to handle this sort of thing?  If so, the hack may
need to be in the handling of that ACPI data (this field is not there
but should be), not here, which as Guenter says, is a big hack.

-corey

> 
> Third, the new symbol is declared with CONFIG_ACPI, but defined with
> CONFIG_IPMI_SI. I can not imagine how this would compile with CONFIG_ACPI=y
> and CONFIG_IPMI_SI={m,n} and/or CONFIG_ACPI_IPMI={m,n}.
> 
> On top of that, IPMI_SI and ACPI_IPMI are is tristate, as is SENSORS_ACPI_POWER.
> This means that SENSORS_ACPI_POWER=y combined with CONFIG_IPMI_SI={m,n} or
> CONFIG_ACPI_IPMI={m,n} will result in a compile failure.
> 
> Please make sure that this code compiles with all possible symbol combinations.
> 
> Thanks,
> Guenter
> 
> >   	res = read_capabilities(resource);
> >   	if (res)
> >   		goto exit_free;
> 
> 

Re: [PATCH v4 1/2] ACPI: IPMI: Add helper to wait for when SMI is selected

[Guenter Roeck]

On 1/9/24 11:50, Rafael J. Wysocki wrote:
> On Tue, Jan 9, 2024 at 5:12 AM Kai-Heng Feng
> <kai.heng.feng@canonical.com> wrote:
>>
>> 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>
> 
> and I'm expecting this to be routed along with patch [2/2] that has
> been posted elsewhere.
> 

Note that linux-hwmon@ isn't copied on this patch and it therefore
does not show up in hwmon patchwork. To me that indicates that the
patch should _not_ be applied through hwmon. If hwmon is supposed
to pick it up, please copy the hwmon mailing list.

Guenter

Re: [PATCH v4 2/2] hwmon: (acpi_power_meter) Ensure IPMI space handler is ready on Dell systems

[Guenter Roeck]

On 1/9/24 13:28, Corey Minyard wrote:
> On Tue, Jan 09, 2024 at 07:23:40AM -0800, Guenter Roeck wrote:
>> On 1/8/24 20:12, 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>
>>> ---
>>> v4:
>>>    - No change.
>>>
>>> v3:
>>>    - Use helper.
>>>    - Use return value to print warning message.
>>>
>>> v2:
>>>    - Use completion instead of request_module().
>>>
>>>    drivers/hwmon/acpi_power_meter.c | 6 ++++++
>>>    1 file changed, 6 insertions(+)
>>>
>>> diff --git a/drivers/hwmon/acpi_power_meter.c b/drivers/hwmon/acpi_power_meter.c
>>> index 703666b95bf4..33fb9626633d 100644
>>> --- a/drivers/hwmon/acpi_power_meter.c
>>> +++ b/drivers/hwmon/acpi_power_meter.c
>>> @@ -883,6 +883,12 @@ 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)) {
>>> +		if (acpi_wait_for_acpi_ipmi())
>>> +			dev_warn(&device->dev, "Waiting for ACPI IPMI timeout");
>>> +	}
>>> +
>>
>> What a hack :-(.
>>
>> This needs a comment in the driver explaining the rationale for this change, and
>> also a comment explaining why, for example, using late_initcall() does not help.
>>
>> If CONFIG_IPMI_SI=n, acpi_wait_for_acpi_ipmi() will return 0, indicating success.
>> I can only imagine that this will result in a failure since the whole point
>> of this code is to wait until that driver is loaded. Please explain how and why
>> the code works with CONFIG_IPMI_SI=n. Similar, if the function returns an error,
>> I can not imagine how it would make sense to instantiate the driver. If it does
>> make sense to continue in this situation, a comment is needed in the code
>> describing the rationale.
> 
> I'm trying to figure out where CONFIG_IPMI_SI comes in here.  It's
> nowhere in these patches or in drivers/acpi.  ACPI_IPMI depends on
> IPMI_HANDLER, but that's all I found.  However, ACPI_IPMI can be "m" as
> you mention and SENSOR_ACPI_POWER is only under the ACPI config, which
> is a problem.
> 

The patch above is looking for IPI0001, which is instantiated in

drivers/char/ipmi/ipmi_si_platform.c:   { "IPI0001", 0 },
drivers/char/ipmi/ipmi_ssif.c:  { "IPI0001", 0 },

Are you saying that the above code doesn't depend on it ? In that case,
why does it need to check for the IPI0001 device in the first place ?

That will need another comment/explanation in the code because people
(or maybe dummies) like me won't understand the non-dependency (i.e.,
the need to look for IPI0001 but not requiring the associated code).

More specifically, unless I really don't understand the acpi code,
acpi_dev_get_first_match_dev() will return NULL if there is no matching
device. In that case, the above code won't call acpi_wait_for_acpi_ipmi().
Fine, but why would this driver have to wait for ipmi if and only if there
is a device (and thus a driver) for IPI0001 ?

Thanks,
Guenter

> I do think there are other issues with this patch, though.  The IPMI
> handler code decouples the user from the driver from a dependency point
> of view.  It seems to be fairly common to see IPMI_HANDLER and
> ACPI_IPMI as "y" and IPMI_SI (and IPMI_SSIF, and others) as "m".  That
> means this code will run but will wait for the IPMI device to appear,
> which may not be until the module gets loaded, which may be far more
> than 2 seconds later.
> 
> I'm not quite sure how to fix this.  Really, the add call for this
> driver shouldn't be called until the IPMI device is present.  Doesn't
> ACPI have mechanisms to handle this sort of thing?  If so, the hack may
> need to be in the handling of that ACPI data (this field is not there
> but should be), not here, which as Guenter says, is a big hack.
> 
> -corey
> 
>>
>> Third, the new symbol is declared with CONFIG_ACPI, but defined with
>> CONFIG_IPMI_SI. I can not imagine how this would compile with CONFIG_ACPI=y
>> and CONFIG_IPMI_SI={m,n} and/or CONFIG_ACPI_IPMI={m,n}.
>>
>> On top of that, IPMI_SI and ACPI_IPMI are is tristate, as is SENSORS_ACPI_POWER.
>> This means that SENSORS_ACPI_POWER=y combined with CONFIG_IPMI_SI={m,n} or
>> CONFIG_ACPI_IPMI={m,n} will result in a compile failure.
>>
>> Please make sure that this code compiles with all possible symbol combinations.
>>
>> Thanks,
>> Guenter
>>
>>>    	res = read_capabilities(resource);
>>>    	if (res)
>>>    		goto exit_free;
>>
>>
> 

Re: [PATCH v4 2/2] hwmon: (acpi_power_meter) Ensure IPMI space handler is ready on Dell systems

[Corey Minyard]

On Tue, Jan 09, 2024 at 02:32:41PM -0800, Guenter Roeck wrote:
> On 1/9/24 13:28, Corey Minyard wrote:
> > On Tue, Jan 09, 2024 at 07:23:40AM -0800, Guenter Roeck wrote:
> > > On 1/8/24 20:12, 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>
> > > > ---
> > > > v4:
> > > >    - No change.
> > > > 
> > > > v3:
> > > >    - Use helper.
> > > >    - Use return value to print warning message.
> > > > 
> > > > v2:
> > > >    - Use completion instead of request_module().
> > > > 
> > > >    drivers/hwmon/acpi_power_meter.c | 6 ++++++
> > > >    1 file changed, 6 insertions(+)
> > > > 
> > > > diff --git a/drivers/hwmon/acpi_power_meter.c b/drivers/hwmon/acpi_power_meter.c
> > > > index 703666b95bf4..33fb9626633d 100644
> > > > --- a/drivers/hwmon/acpi_power_meter.c
> > > > +++ b/drivers/hwmon/acpi_power_meter.c
> > > > @@ -883,6 +883,12 @@ 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)) {
> > > > +		if (acpi_wait_for_acpi_ipmi())
> > > > +			dev_warn(&device->dev, "Waiting for ACPI IPMI timeout");
> > > > +	}
> > > > +
> > > 
> > > What a hack :-(.
> > > 
> > > This needs a comment in the driver explaining the rationale for this change, and
> > > also a comment explaining why, for example, using late_initcall() does not help.
> > > 
> > > If CONFIG_IPMI_SI=n, acpi_wait_for_acpi_ipmi() will return 0, indicating success.
> > > I can only imagine that this will result in a failure since the whole point
> > > of this code is to wait until that driver is loaded. Please explain how and why
> > > the code works with CONFIG_IPMI_SI=n. Similar, if the function returns an error,
> > > I can not imagine how it would make sense to instantiate the driver. If it does
> > > make sense to continue in this situation, a comment is needed in the code
> > > describing the rationale.
> > 
> > I'm trying to figure out where CONFIG_IPMI_SI comes in here.  It's
> > nowhere in these patches or in drivers/acpi.  ACPI_IPMI depends on
> > IPMI_HANDLER, but that's all I found.  However, ACPI_IPMI can be "m" as
> > you mention and SENSOR_ACPI_POWER is only under the ACPI config, which
> > is a problem.
> > 
> 
> The patch above is looking for IPI0001, which is instantiated in
> 
> drivers/char/ipmi/ipmi_si_platform.c:   { "IPI0001", 0 },
> drivers/char/ipmi/ipmi_ssif.c:  { "IPI0001", 0 },
> 
> Are you saying that the above code doesn't depend on it ? In that case,
> why does it need to check for the IPI0001 device in the first place ?
> 
> That will need another comment/explanation in the code because people
> (or maybe dummies) like me won't understand the non-dependency (i.e.,
> the need to look for IPI0001 but not requiring the associated code).
> 
> More specifically, unless I really don't understand the acpi code,
> acpi_dev_get_first_match_dev() will return NULL if there is no matching
> device. In that case, the above code won't call acpi_wait_for_acpi_ipmi().
> Fine, but why would this driver have to wait for ipmi if and only if there
> is a device (and thus a driver) for IPI0001 ?

Honestly, I don't really understand the acpi code that well, either.
What I think it's saying is that if IPI0001 is present in the ACPI
tables (there is an acpi_device present), then wait for the driver to
get loaded.  It could be IPMI_SI or IPMI_SSIF, but there's no direct
connection between this code and the low-level IPMI driver.  I don't
think it forces the driver to load, at least not from what I can tell.

And another bug.  From the description of acpi_dev_get_first_match_dev():
The caller is responsible for invoking acpi_dev_put() on the returned device.

As you said, this is a big hack.  There must be a better way.

-corey

> 
> Thanks,
> Guenter
> 
> > I do think there are other issues with this patch, though.  The IPMI
> > handler code decouples the user from the driver from a dependency point
> > of view.  It seems to be fairly common to see IPMI_HANDLER and
> > ACPI_IPMI as "y" and IPMI_SI (and IPMI_SSIF, and others) as "m".  That
> > means this code will run but will wait for the IPMI device to appear,
> > which may not be until the module gets loaded, which may be far more
> > than 2 seconds later.
> > 
> > I'm not quite sure how to fix this.  Really, the add call for this
> > driver shouldn't be called until the IPMI device is present.  Doesn't
> > ACPI have mechanisms to handle this sort of thing?  If so, the hack may
> > need to be in the handling of that ACPI data (this field is not there
> > but should be), not here, which as Guenter says, is a big hack.
> > 
> > -corey
> > 
> > > 
> > > Third, the new symbol is declared with CONFIG_ACPI, but defined with
> > > CONFIG_IPMI_SI. I can not imagine how this would compile with CONFIG_ACPI=y
> > > and CONFIG_IPMI_SI={m,n} and/or CONFIG_ACPI_IPMI={m,n}.
> > > 
> > > On top of that, IPMI_SI and ACPI_IPMI are is tristate, as is SENSORS_ACPI_POWER.
> > > This means that SENSORS_ACPI_POWER=y combined with CONFIG_IPMI_SI={m,n} or
> > > CONFIG_ACPI_IPMI={m,n} will result in a compile failure.
> > > 
> > > Please make sure that this code compiles with all possible symbol combinations.
> > > 
> > > Thanks,
> > > Guenter
> > > 
> > > >    	res = read_capabilities(resource);
> > > >    	if (res)
> > > >    		goto exit_free;
> > > 
> > > 
> > 
> 
> 

Re: [PATCH v4 2/2] hwmon: (acpi_power_meter) Ensure IPMI space handler is ready on Dell systems

[kernel test robot]

Hi Kai-Heng,

kernel test robot noticed the following build errors:

[auto build test ERROR on rafael-pm/linux-next]
[also build test ERROR on groeck-staging/hwmon-next rafael-pm/acpi-bus linus/master v6.7 next-20240112]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/Kai-Heng-Feng/hwmon-acpi_power_meter-Ensure-IPMI-space-handler-is-ready-on-Dell-systems/20240109-121520
base:   https://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git linux-next
patch link:    https://lore.kernel.org/r/20240109041218.980674-2-kai.heng.feng%40canonical.com
patch subject: [PATCH v4 2/2] hwmon: (acpi_power_meter) Ensure IPMI space handler is ready on Dell systems
config: loongarch-randconfig-r112-20240113 (https://download.01.org/0day-ci/archive/20240113/202401131750.F51XQF3H-lkp@intel.com/config)
compiler: loongarch64-linux-gcc (GCC) 13.2.0
reproduce: (https://download.01.org/0day-ci/archive/20240113/202401131750.F51XQF3H-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202401131750.F51XQF3H-lkp@intel.com/

All errors (new ones prefixed by >>):

   loongarch64-linux-ld: drivers/hwmon/acpi_power_meter.o: in function `.L212':
>> acpi_power_meter.c:(.text+0x15c0): undefined reference to `acpi_wait_for_acpi_ipmi'

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH v4 2/2] hwmon: (acpi_power_meter) Ensure IPMI space handler is ready on Dell systems

[Kai-Heng Feng]

Hi Guenter,

Sorry for the belated response.

On Wed, Jan 10, 2024 at 6:32 AM Guenter Roeck <linux@roeck-us.net> wrote:
>
> On 1/9/24 13:28, Corey Minyard wrote:
> > On Tue, Jan 09, 2024 at 07:23:40AM -0800, Guenter Roeck wrote:
> >> On 1/8/24 20:12, 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>
> >>> ---
> >>> v4:
> >>>    - No change.
> >>>
> >>> v3:
> >>>    - Use helper.
> >>>    - Use return value to print warning message.
> >>>
> >>> v2:
> >>>    - Use completion instead of request_module().
> >>>
> >>>    drivers/hwmon/acpi_power_meter.c | 6 ++++++
> >>>    1 file changed, 6 insertions(+)
> >>>
> >>> diff --git a/drivers/hwmon/acpi_power_meter.c b/drivers/hwmon/acpi_power_meter.c
> >>> index 703666b95bf4..33fb9626633d 100644
> >>> --- a/drivers/hwmon/acpi_power_meter.c
> >>> +++ b/drivers/hwmon/acpi_power_meter.c
> >>> @@ -883,6 +883,12 @@ 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)) {
> >>> +           if (acpi_wait_for_acpi_ipmi())
> >>> +                   dev_warn(&device->dev, "Waiting for ACPI IPMI timeout");
> >>> +   }
> >>> +
> >>
> >> What a hack :-(.
> >>
> >> This needs a comment in the driver explaining the rationale for this change, and
> >> also a comment explaining why, for example, using late_initcall() does not help.
> >>
> >> If CONFIG_IPMI_SI=n, acpi_wait_for_acpi_ipmi() will return 0, indicating success.
> >> I can only imagine that this will result in a failure since the whole point
> >> of this code is to wait until that driver is loaded. Please explain how and why
> >> the code works with CONFIG_IPMI_SI=n. Similar, if the function returns an error,
> >> I can not imagine how it would make sense to instantiate the driver. If it does
> >> make sense to continue in this situation, a comment is needed in the code
> >> describing the rationale.
> >
> > I'm trying to figure out where CONFIG_IPMI_SI comes in here.  It's
> > nowhere in these patches or in drivers/acpi.  ACPI_IPMI depends on
> > IPMI_HANDLER, but that's all I found.  However, ACPI_IPMI can be "m" as
> > you mention and SENSOR_ACPI_POWER is only under the ACPI config, which
> > is a problem.
> >
>
> The patch above is looking for IPI0001, which is instantiated in
>
> drivers/char/ipmi/ipmi_si_platform.c:   { "IPI0001", 0 },
> drivers/char/ipmi/ipmi_ssif.c:  { "IPI0001", 0 },
>
> Are you saying that the above code doesn't depend on it ? In that case,
> why does it need to check for the IPI0001 device in the first place ?

Only Dell hardware has this "implicit" dependency that requires IPMI
opregion to be functional.

>
> That will need another comment/explanation in the code because people
> (or maybe dummies) like me won't understand the non-dependency (i.e.,
> the need to look for IPI0001 but not requiring the associated code).

Sure, will do.

>
> More specifically, unless I really don't understand the acpi code,
> acpi_dev_get_first_match_dev() will return NULL if there is no matching
> device. In that case, the above code won't call acpi_wait_for_acpi_ipmi().
> Fine, but why would this driver have to wait for ipmi if and only if there
> is a device (and thus a driver) for IPI0001 ?

Because acpi_power_meter's ACPI method access IPMI opregion, which
requires the driver to install the opregion handler and wait for SMI
being selected.

AFAIK there's no mean to discover the "dependency" here, hence the hack...

Kai-Heng

>
> Thanks,
> Guenter
>
> > I do think there are other issues with this patch, though.  The IPMI
> > handler code decouples the user from the driver from a dependency point
> > of view.  It seems to be fairly common to see IPMI_HANDLER and
> > ACPI_IPMI as "y" and IPMI_SI (and IPMI_SSIF, and others) as "m".  That
> > means this code will run but will wait for the IPMI device to appear,
> > which may not be until the module gets loaded, which may be far more
> > than 2 seconds later.
> >
> > I'm not quite sure how to fix this.  Really, the add call for this
> > driver shouldn't be called until the IPMI device is present.  Doesn't
> > ACPI have mechanisms to handle this sort of thing?  If so, the hack may
> > need to be in the handling of that ACPI data (this field is not there
> > but should be), not here, which as Guenter says, is a big hack.
> >
> > -corey
> >
> >>
> >> Third, the new symbol is declared with CONFIG_ACPI, but defined with
> >> CONFIG_IPMI_SI. I can not imagine how this would compile with CONFIG_ACPI=y
> >> and CONFIG_IPMI_SI={m,n} and/or CONFIG_ACPI_IPMI={m,n}.
> >>
> >> On top of that, IPMI_SI and ACPI_IPMI are is tristate, as is SENSORS_ACPI_POWER.
> >> This means that SENSORS_ACPI_POWER=y combined with CONFIG_IPMI_SI={m,n} or
> >> CONFIG_ACPI_IPMI={m,n} will result in a compile failure.
> >>
> >> Please make sure that this code compiles with all possible symbol combinations.
> >>
> >> Thanks,
> >> Guenter
> >>
> >>>     res = read_capabilities(resource);
> >>>     if (res)
> >>>             goto exit_free;
> >>
> >>
> >
>

Re: [PATCH v4 2/2] hwmon: (acpi_power_meter) Ensure IPMI space handler is ready on Dell systems

[Kai-Heng Feng]

Hi Corey,

On Wed, Jan 10, 2024 at 7:52 AM Corey Minyard <minyard@acm.org> wrote:
>
> On Tue, Jan 09, 2024 at 02:32:41PM -0800, Guenter Roeck wrote:
> > On 1/9/24 13:28, Corey Minyard wrote:
> > > On Tue, Jan 09, 2024 at 07:23:40AM -0800, Guenter Roeck wrote:
> > > > On 1/8/24 20:12, 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>
> > > > > ---
> > > > > v4:
> > > > >    - No change.
> > > > >
> > > > > v3:
> > > > >    - Use helper.
> > > > >    - Use return value to print warning message.
> > > > >
> > > > > v2:
> > > > >    - Use completion instead of request_module().
> > > > >
> > > > >    drivers/hwmon/acpi_power_meter.c | 6 ++++++
> > > > >    1 file changed, 6 insertions(+)
> > > > >
> > > > > diff --git a/drivers/hwmon/acpi_power_meter.c b/drivers/hwmon/acpi_power_meter.c
> > > > > index 703666b95bf4..33fb9626633d 100644
> > > > > --- a/drivers/hwmon/acpi_power_meter.c
> > > > > +++ b/drivers/hwmon/acpi_power_meter.c
> > > > > @@ -883,6 +883,12 @@ 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)) {
> > > > > +               if (acpi_wait_for_acpi_ipmi())
> > > > > +                       dev_warn(&device->dev, "Waiting for ACPI IPMI timeout");
> > > > > +       }
> > > > > +
> > > >
> > > > What a hack :-(.
> > > >
> > > > This needs a comment in the driver explaining the rationale for this change, and
> > > > also a comment explaining why, for example, using late_initcall() does not help.
> > > >
> > > > If CONFIG_IPMI_SI=n, acpi_wait_for_acpi_ipmi() will return 0, indicating success.
> > > > I can only imagine that this will result in a failure since the whole point
> > > > of this code is to wait until that driver is loaded. Please explain how and why
> > > > the code works with CONFIG_IPMI_SI=n. Similar, if the function returns an error,
> > > > I can not imagine how it would make sense to instantiate the driver. If it does
> > > > make sense to continue in this situation, a comment is needed in the code
> > > > describing the rationale.
> > >
> > > I'm trying to figure out where CONFIG_IPMI_SI comes in here.  It's
> > > nowhere in these patches or in drivers/acpi.  ACPI_IPMI depends on
> > > IPMI_HANDLER, but that's all I found.  However, ACPI_IPMI can be "m" as
> > > you mention and SENSOR_ACPI_POWER is only under the ACPI config, which
> > > is a problem.
> > >
> >
> > The patch above is looking for IPI0001, which is instantiated in
> >
> > drivers/char/ipmi/ipmi_si_platform.c:   { "IPI0001", 0 },
> > drivers/char/ipmi/ipmi_ssif.c:  { "IPI0001", 0 },
> >
> > Are you saying that the above code doesn't depend on it ? In that case,
> > why does it need to check for the IPI0001 device in the first place ?
> >
> > That will need another comment/explanation in the code because people
> > (or maybe dummies) like me won't understand the non-dependency (i.e.,
> > the need to look for IPI0001 but not requiring the associated code).
> >
> > More specifically, unless I really don't understand the acpi code,
> > acpi_dev_get_first_match_dev() will return NULL if there is no matching
> > device. In that case, the above code won't call acpi_wait_for_acpi_ipmi().
> > Fine, but why would this driver have to wait for ipmi if and only if there
> > is a device (and thus a driver) for IPI0001 ?
>
> Honestly, I don't really understand the acpi code that well, either.
> What I think it's saying is that if IPI0001 is present in the ACPI
> tables (there is an acpi_device present), then wait for the driver to
> get loaded.  It could be IPMI_SI or IPMI_SSIF, but there's no direct
> connection between this code and the low-level IPMI driver.  I don't
> think it forces the driver to load, at least not from what I can tell.

The dependency isn't discoverable at software level. It's all in the BIOS' ASL

>
> And another bug.  From the description of acpi_dev_get_first_match_dev():
> The caller is responsible for invoking acpi_dev_put() on the returned device.

Sure. I'll fix it in next revision.

>
> As you said, this is a big hack.  There must be a better way.

Unfortunately I really can't find a better alternative if the
dependency isn't declared anywhere.

Kai-Heng

>
> -corey
>
> >
> > Thanks,
> > Guenter
> >
> > > I do think there are other issues with this patch, though.  The IPMI
> > > handler code decouples the user from the driver from a dependency point
> > > of view.  It seems to be fairly common to see IPMI_HANDLER and
> > > ACPI_IPMI as "y" and IPMI_SI (and IPMI_SSIF, and others) as "m".  That
> > > means this code will run but will wait for the IPMI device to appear,
> > > which may not be until the module gets loaded, which may be far more
> > > than 2 seconds later.
> > >
> > > I'm not quite sure how to fix this.  Really, the add call for this
> > > driver shouldn't be called until the IPMI device is present.  Doesn't
> > > ACPI have mechanisms to handle this sort of thing?  If so, the hack may
> > > need to be in the handling of that ACPI data (this field is not there
> > > but should be), not here, which as Guenter says, is a big hack.
> > >
> > > -corey
> > >
> > > >
> > > > Third, the new symbol is declared with CONFIG_ACPI, but defined with
> > > > CONFIG_IPMI_SI. I can not imagine how this would compile with CONFIG_ACPI=y
> > > > and CONFIG_IPMI_SI={m,n} and/or CONFIG_ACPI_IPMI={m,n}.
> > > >
> > > > On top of that, IPMI_SI and ACPI_IPMI are is tristate, as is SENSORS_ACPI_POWER.
> > > > This means that SENSORS_ACPI_POWER=y combined with CONFIG_IPMI_SI={m,n} or
> > > > CONFIG_ACPI_IPMI={m,n} will result in a compile failure.
> > > >
> > > > Please make sure that this code compiles with all possible symbol combinations.
> > > >
> > > > Thanks,
> > > > Guenter
> > > >
> > > > >         res = read_capabilities(resource);
> > > > >         if (res)
> > > > >                 goto exit_free;
> > > >
> > > >
> > >
> >
> >