Received-SPF: pass (google.com: domain of linux-kernel+bounces-21418-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.48.161 as permitted sender) client-ip=147.75.48.161;
Sender: Corey Minyard <tcminyard@gmail.com>
Date: Tue, 9 Jan 2024 15:28:25 -0600
From: Corey Minyard <minyard@acm.org>
To: Guenter Roeck <linux@roeck-us.net>
Cc: Kai-Heng Feng <kai.heng.feng@canonical.com>, jdelvare@suse.com,
	linux-hwmon@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH v4 2/2] hwmon: (acpi_power_meter) Ensure IPMI space
 handler is ready on Dell systems
Message-ID: <ZZ26ea5KV9Xg1MDc@mail.minyard.net>
Reply-To: minyard@acm.org
References: <20240109041218.980674-1-kai.heng.feng@canonical.com>
 <20240109041218.980674-2-kai.heng.feng@canonical.com>
 <1b8ff82e-ff26-410e-a37e-0d818494bac3@roeck-us.net>
Precedence: bulk
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <1b8ff82e-ff26-410e-a37e-0d818494bac3@roeck-us.net>

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