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

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

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

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

[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