| Message ID | 20250208162210.3929473-5-superm1@kernel.org |
|---|---|
| State | New |
| Headers | show |
| Series | Improvements to ACPI battery handling over s2idle | expand |
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 >> >>
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; }