Received-SPF: pass (google.com: domain of linux-kernel+bounces-101138-linux.lists.archive=gmail.com@vger.kernel.org designates 2604:1380:45d1:ec00::1 as permitted sender) client-ip=2604:1380:45d1:ec00::1;
Precedence: bulk
MIME-Version: 1.0
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>
 <ZZ26ea5KV9Xg1MDc@mail.minyard.net> <e090d7f5-208a-4a4d-8162-7202ad6b0183@roeck-us.net>
 <ZZ3cSygeCTAr35nz@mail.minyard.net>
In-Reply-To: <ZZ3cSygeCTAr35nz@mail.minyard.net>
From: Kai-Heng Feng <kai.heng.feng@canonical.com>
Date: Wed, 13 Mar 2024 14:24:37 +0800
Message-ID: <CAAd53p71sS+eV1OmPgSDrAcqu8-FJZX-9sd=3a6ZNAvaZV9jbQ@mail.gmail.com>
Subject: Re: [PATCH v4 2/2] hwmon: (acpi_power_meter) Ensure IPMI space
 handler is ready on Dell systems
To: minyard@acm.org
Cc: Guenter Roeck <linux@roeck-us.net>, jdelvare@suse.com, linux-hwmon@vger.kernel.org, 
	linux-kernel@vger.kernel.org
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

Hi Corey,

On Wed, Jan 10, 2024 at 7:52=E2=80=AFAM Corey Minyard <minyard@acm.org> wro=
te:
>
> 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] (00000000=
ab9e62c5) [IPMI] (20230628/evregion-130)
> > > > > [    3.717928] ACPI Error: Region IPMI (ID=3D7) has no handler (2=
0230628/exfldio-261)
> > > > >
> > > > > [    3.717936] No Local Variables are initialized for Method [_GH=
L]
> > > > >
> > > > > [    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_E=
XIST
> > > > >
> > > > > On Dell systems several methods of acpi_power_meter access variab=
les 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 h=
andler
> > > > > 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-err=
or-messages-displayed-in-dmesg?guid=3Dguid-0d5ae482-1977-42cf-b417-3ed5c3f5=
ee62
> > > > >
> > > > > 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/acp=
i_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 =3D 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 thi=
s change, and
> > > > also a comment explaining why, for example, using late_initcall() d=
oes not help.
> > > >
> > > > If CONFIG_IPMI_SI=3Dn, acpi_wait_for_acpi_ipmi() will return 0, ind=
icating success.
> > > > I can only imagine that this will result in a failure since the who=
le point
> > > > of this code is to wait until that driver is loaded. Please explain=
 how and why
> > > > the code works with CONFIG_IPMI_SI=3Dn. Similar, if the function re=
turns 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 th=
e 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, whic=
h
> > > 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 th=
ere
> > 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 dev=
ice.

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 poi=
nt
> > > 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".  Tha=
t
> > > 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 m=
ay
> > > 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 wit=
h
> > > > CONFIG_IPMI_SI. I can not imagine how this would compile with CONFI=
G_ACPI=3Dy
> > > > and CONFIG_IPMI_SI=3D{m,n} and/or CONFIG_ACPI_IPMI=3D{m,n}.
> > > >
> > > > On top of that, IPMI_SI and ACPI_IPMI are is tristate, as is SENSOR=
S_ACPI_POWER.
> > > > This means that SENSORS_ACPI_POWER=3Dy combined with CONFIG_IPMI_SI=
=3D{m,n} or
> > > > CONFIG_ACPI_IPMI=3D{m,n} will result in a compile failure.
> > > >
> > > > Please make sure that this code compiles with all possible symbol c=
ombinations.
> > > >
> > > > Thanks,
> > > > Guenter
> > > >
> > > > >         res =3D read_capabilities(resource);
> > > > >         if (res)
> > > > >                 goto exit_free;
> > > >
> > > >
> > >
> >
> >