| Message ID | 20250208162210.3929473-5-superm1@kernel.org (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | Improvements to ACPI battery handling over s2idle | expand |
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? Frankly, I'd prefer the existing behavior to be still the default. > 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 > >
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 >> >>
Am 09.02.25 um 14:14 schrieb Mario Limonciello: > > > 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? Hi, i believe that handling the wakeup inside the ACPI power supply device would make more sense, because the current patch will also cause the machine to wake up if the battery has finished charging. This behavior would be quite counterintuitive. Thanks, Armin Wolf > >> >>> 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 >>> >>> > >
On Wed, Feb 12, 2025 at 2:49 PM Armin Wolf <W_Armin@gmx.de> wrote: > > Am 09.02.25 um 14:14 schrieb Mario Limonciello: > > > > > > > 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? > > Hi, > > i believe that handling the wakeup inside the ACPI power supply device would make more sense, because the current patch will also cause the machine to wake up > if the battery has finished charging. This behavior would be quite counterintuitive. Good point. Also, in Linux there is a concept of "system wakeup devices" which IIUC is not present in (current) Windows, at least not in this form. That is, the user can select devices that are allowed to wake up the system from sleep via the /sys/dev/ces/.../power/wakeup attributes (all wakeup-capable devices should have them). If this is enabled, device_may_wakeup() returns true and this is the case when the given device is expected to wake up the system from sleep.
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; }