Message-ID: <e2e76ae3-ba99-4975-b9b7-2946f593c800@maciej.szmigiero.name>
Date: Tue, 14 Jan 2025 01:27:01 +0100
From: "Maciej S. Szmigiero" <mail@...iej.szmigiero.name>
To: "Rafael J. Wysocki" <rafael@...nel.org>
Cc: Len Brown <lenb@...nel.org>, linux-acpi@...r.kernel.org,
 linux-kernel@...r.kernel.org
Subject: Re: [PATCH] ACPI: PM: Add HP EliteBook 855 G7 WWAN modem power
 resource quirk

On 7.01.2025 19:46, Rafael J. Wysocki wrote:
> On Sun, Dec 29, 2024 at 5:45 PM Maciej S. Szmigiero
> <mail@...iej.szmigiero.name> wrote:
>>
>> This laptop (and possibly similar models too) has power resource called
>> "GP12.PXP_" for its Intel XMM7360 WWAN modem.
>>
>> For this power resource to turn ON power for the modem it needs certain
>> internal flag called "ONEN" to be set:
>> Method (_ON, 0, NotSerialized) // _ON_: Power On
>> {
>>          If (^^^LPCB.EC0.ECRG)
>>          {
>>                  If ((ONEN == Zero))
>>                  {
>>                          Return (Zero)
>>                  }
>> (..)
>>          }
>> }
>>
>>
>> This flag only gets set from this power resource "_OFF" method, while the
>> actual modem power gets turned off during suspend by "GP12.PTS" method
>> called from the global "_PTS" (Prepare To Sleep) method.
>>
>> In fact, this power resource "_OFF" method implementation just sets the
>> aforementioned flag:
>> Method (_OFF, 0, NotSerialized) // _OFF: Power Off
>> {
>>          OFEN = Zero
>>          ONEN = One
>> }
>>
>> Upon hibernation finish we try to set this power resource back ON since its
>> "_STA" method returns 0 and the resource is still considered in use as it
>> is declared as required for D0 for both the modem ACPI device (GP12.PWAN)
>> and its parent PCIe port ACPI device (GP12).
>> But the "_ON" method won't do anything since that "ONEN" flag is not set.
>>
>> Overall, this means the modem is dead after hibernation finish until the
>> laptop is rebooted since the modem power has been cut by "_PTS" and its PCI
>> configuration was lost and not able to be restored.
>>
>> The easiest way to workaround this issue is to call this power resource
>> "_OFF" method before calling the "_ON" method to make sure the "ONEN"
>> flag gets properly set.
>>
>> This makes the modem alive once again after hibernation finish - with
>> properly restored PCI configuration space.
>>
>> Signed-off-by: Maciej S. Szmigiero <mail@...iej.szmigiero.name>
> 
> Thanks for the patch, but I'd rather find a different way of
> addressing the problem at hand because there are at least two
> potential issues with this approach.
> 
>> ---
>>   drivers/acpi/power.c | 75 ++++++++++++++++++++++++++++++++++++++++++++
>>   1 file changed, 75 insertions(+)
>>
>> diff --git a/drivers/acpi/power.c b/drivers/acpi/power.c
>> index 25174c24d3d7..1db93cf8e4f6 100644
>> --- a/drivers/acpi/power.c
>> +++ b/drivers/acpi/power.c
>> @@ -23,6 +23,7 @@
>>
>>   #define pr_fmt(fmt) "ACPI: PM: " fmt
>>
>> +#include <linux/delay.h>
>>   #include <linux/dmi.h>
>>   #include <linux/kernel.h>
>>   #include <linux/module.h>
>> @@ -991,9 +992,57 @@ struct acpi_device *acpi_add_power_resource(acpi_handle handle)
>>   }
>>
>>   #ifdef CONFIG_ACPI_SLEEP
>> +static const struct dmi_system_id dmi_hp_elitebook_gp12pxp_quirk[] = {
>> +/*
>> + * This laptop (and possibly similar models too) has power resource called
>> + * "GP12.PXP_" for its WWAN modem.
>> + *
>> + * For this power resource to turn ON power for the modem it needs certain
>> + * internal flag called "ONEN" to be set.
>> + * This flag only gets set from this power resource "_OFF" method, while the
>> + * actual modem power gets turned off during suspend by "GP12.PTS" method
>> + * called from the global "_PTS" (Prepare To Sleep) method.
>> + * On the other hand, this power resource "_OFF" method implementation just
>> + * sets the aforementioned flag without actually doing anything else (it
>> + * doesn't contain any code to actually turn off power).
>> + *
>> + * The above means that when upon hibernation finish we try to set this
>> + * power resource back ON since its "_STA" method returns 0 (while the resource
>> + * is still considered in use) its "_ON" method won't do anything since
>> + * that "ONEN" flag is not set.
>> + * Overall, this means the modem is dead until laptop is rebooted since its
>> + * power has been cut by "_PTS" and its PCI configuration was lost and not able
>> + * to be restored.
>> + *
>> + * The easiest way to workaround the issue is to call this power resource
>> + * "_OFF" method before calling the "_ON" method to make sure the "ONEN"
>> + * flag gets properly set.
>> + */
>> +       {
>> +               .matches = {
>> +                       DMI_MATCH(DMI_SYS_VENDOR, "HP"),
>> +                       DMI_MATCH(DMI_PRODUCT_NAME, "HP EliteBook 855 G7 Notebook PC"),
>> +               },
>> +       },
>> +       {}
>> +};
>> +
>> +static bool resource_is_gp12pxp(acpi_handle handle)
>> +{
>> +       const char *path;
>> +       bool ret;
>> +
>> +       path = acpi_handle_path(handle);
>> +       ret = path && strcmp(path, "\\_SB_.PCI0.GP12.PXP_") == 0;
>> +       kfree(path);
>> +
>> +       return ret;
>> +}
>> +
>>   void acpi_resume_power_resources(void)
>>   {
>>          struct acpi_power_resource *resource;
>> +       bool hp_eb_gp12pxp_quirk = dmi_check_system(dmi_hp_elitebook_gp12pxp_quirk);
>>
>>          mutex_lock(&power_resource_list_lock);
>>
>> @@ -1012,8 +1061,34 @@ void acpi_resume_power_resources(void)
>>
>>                  if (state == ACPI_POWER_RESOURCE_STATE_OFF
>>                      && resource->ref_count) {
>> +                       bool eb_gp12pxp = hp_eb_gp12pxp_quirk &&
>> +                               resource_is_gp12pxp(resource->device.handle);
> 
> This is quite a bit of a useless overhead for all of the systems that
> don't actually contain this defective power resource.

There's already an existing dmi_check_system() in similar
acpi_turn_off_unused_power_resources() function.

This function is called just once during system wake so it is
hardly a hot path.
In any case, that hp_eb_gp12pxp_quirk flag initialization part
could be easily moved to some init function that's run just once
during system boot.

Then the whole overhead on other systems amounts to hp_eb_gp12pxp_quirk
flag comparison as resource_is_gp12pxp() won't be called on them
due to && operator short-circuit evaluation behavior.

> Also, according to your description, the problem is
> hibernation-specific and this function is also called on resume from
> suspend-to-RAM.

This platform supports just s2idle - no S3 support there, so that's
not a problem.

>> +
>> +                       if (eb_gp12pxp) {
>> +                               acpi_handle_notice(resource->device.handle,
>> +                                                  "HP EB quirk - turning OFF before ON\n");
>> +                               __acpi_power_off(resource);
>> +                       }
>> +
>>                          acpi_handle_debug(resource->device.handle, "Turning ON\n");
>>                          __acpi_power_on(resource);
>> +
>> +                       if (eb_gp12pxp) {
>> +                               /*
>> +                                * Use the same delay as DSDT uses in modem _RST
>> +                                * method.
>> +                                *
>> +                                * Otherwise we get "Unable to change power
>> +                                * state from unknown to D0, device
>> +                                * inaccessible" error for the modem PCI device
>> +                                * after thaw.
>> +                                *
>> +                                * This power resource is normally being enabled
>> +                                * only during thaw (once) so this wait is not
>> +                                * a performance issue.
>> +                                */
>> +                               msleep(200);
>> +                       }
>>                  }
>>
>>                  mutex_unlock(&resource->resource_lock);
> 
> It looks like the modem's driver is modular and not included into the
> initrd image, so the restore kernel doesn't initialize it during
> resume from hibernation.
> 
> Have you tried to build that driver into the kernel or add it to the initrd?

I did a quick test with the WWAN modem driver that's built into the kernel
and in the case of 100% successful hibernation the modem indeed works after
resume.

But in case of aborted (or failed) hibernation it is still broken
(this scenario can be simulated easily by writing to /sys/power/pm_test).

While a failed hibernation entry might sound like a corner case in practice
it is far from it unfortunately - I start to experience it regularly after
about 2 weeks of uptime when the machine RAM usage starts to exceed 50%
(I start to get memory allocation failures during hibernation as that other
50% of RAM gets used by AFAIK snapshot copy of the memory).

Other things, like for example btrfs scrub running in background, or active
wake event may also cause failed hibernation entry.

What's worse, even successful hibernation and restore after that doesn't
bring back the modem to the working state - a clean reboot of the machine
is necessary for that.

It's also worth noting that the root cause of the problem is firmware bug
that reports this power resource as OFF via "_STA" but does not allow turning
it ON via just "_ON" method call so that's what we probably should be fixing.

I've updated that laptop BIOS to the latest version but sadly it does not
seem to bring any DSDT changes with respect to the previous one.

Thanks,
Maciej

Powered by blists - more mailing lists