Re: [PATCH 4/4] ACPI: battery: Wake system on AC plug or unplug in over s2idle

[Mario Limonciello <superm1@kernel.org>]

On 2/8/25 11:59, Rafael J. Wysocki wrote:
> On Sat, Feb 8, 2025 at 5:22 PM Mario Limonciello <superm1@kernel.org> wrote:
>>
>> From: Mario Limonciello <mario.limonciello@amd.com>
>>
>> On Windows the OS will wake up when plugged or unplugged from AC adapter.
>> Depending upon whether the system was plugged in or unplugged will
>> determine whether the "display turns on".  If there is no user activity
>> for some time then it goes back to sleep.
>>
>> In Linux plugging or unplugging an adapter will wake the SoC from HW
>> sleep but then the Linux kernel puts it right back into HW sleep
>> immediately unless there is another interrupt active (such as a PME or
>> GPIO).
>>
>> To get closer to the Windows behavior, record the state of the battery
>> when going into suspend and compare it when updating battery status
>> during the s2idle loop. If it's changed, wake the system.
>>
>> This can be restored to previous behavior by disabling the ACPI battery
>> device `power/wakeup` sysfs file.
> 
> Why is this desirable?
> 
> What if the AC is connected to a suspended laptop when the lid is
> still closed?  Is it really a good idea to resume it then?

Yes; that's the exact situation I wanted this to work.  I have a dock 
connected to some monitors, power supply, keyboard, and mouse.  I want 
the machine to wake up when it's connected to the dock but still closed.

That's how Windows works at least.

> 
> Frankly, I'd prefer the existing behavior to be still the default.

Since this is hooking into the existing wakeups that can happen for 
battery I guess that there isn't a good way to configure one but not the 
other.

Would it be better to do something similar in the ACPI power supply device?

> 
>> Link: https://learn.microsoft.com/en-us/windows-hardware/design/device-experiences/modern-standby-wake-sources#environmental-context-changes-1
>> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
>> ---
>>   drivers/acpi/battery.c | 8 ++++++++
>>   1 file changed, 8 insertions(+)
>>
>> diff --git a/drivers/acpi/battery.c b/drivers/acpi/battery.c
>> index 72c8a509695e6..91f79927cc720 100644
>> --- a/drivers/acpi/battery.c
>> +++ b/drivers/acpi/battery.c
>> @@ -125,6 +125,7 @@ struct acpi_battery {
>>          int state;
>>          int power_unit;
>>          int capacity_suspend;
>> +       int suspend_state;
>>          unsigned long flags;
>>   };
>>
>> @@ -1012,6 +1013,12 @@ static inline bool acpi_battery_should_wake(struct acpi_battery *battery)
>>                  return true;
>>          }
>>
>> +       if (battery->state != battery->suspend_state) {
>> +               pm_pr_dbg("Waking due to battery state changed from 0x%x to 0x%x",
>> +                         battery->suspend_state, battery->state);
>> +               return true;
>> +       }
>> +
>>          return false;
>>   }
>>
>> @@ -1313,6 +1320,7 @@ static int acpi_battery_suspend(struct device *dev)
>>                  return -EINVAL;
>>
>>          battery->capacity_suspend = battery->capacity_now;
>> +       battery->suspend_state = battery->state;
>>
>>          return 0;
>>   }
>> --
>> 2.43.0
>>
>>