[PATCH 0/4] Improvements to ACPI battery handling over s2idle

[PATCH 0/4] Improvements to ACPI battery handling over s2idle

[Mario Limonciello]

From: Mario Limonciello <mario.limonciello@amd.com>

On Windows the system wakes up when connected or disconnected from an
AC adapter. On Linux the system will momentarily wake up but immediately
go back into suspend.

This difference in behavior can potentially expose lower level platform
firmware bugs. For example entering/exiting HW sleep rapidly might not
work properly [1]. It's also inconsistent in that plugging in a dock
might not wake up a system when it should.

This series most notably adjusts that behavior. It also adds a new sysfs
file to determine how much battery was lost over the last sleep cycle
which could aid in userspace statistics and in debugging high power
consumption over s2idle.

[1] https://gitlab.freedesktop.org/drm/amd/-/issues/3929

Mario Limonciello (4):
  PM: Add sysfs file for energy consumed over sleep cycle
  ACPI: battery: Save and report battery capacity over suspend
  ACPI: battery: Refactor wakeup reasons in acpi_battery_update()
  ACPI: battery: Wake system on AC plug or unplug in over s2idle

 Documentation/ABI/testing/sysfs-power |  8 ++++
 drivers/acpi/battery.c                | 62 ++++++++++++++++++++++-----
 include/linux/suspend.h               |  2 +
 kernel/power/main.c                   | 10 +++++
 4 files changed, 71 insertions(+), 11 deletions(-)

-- 
2.43.0

[PATCH 1/4] PM: Add sysfs file for energy consumed over sleep cycle

[Mario Limonciello]

From: Mario Limonciello <mario.limonciello@amd.com>

During a sleep cycle the system will consume power to keep the
platform in s2idle or s3. On systems running on a battery this
can be measured by comparing battery before and after the sleep
cycle.

Add a symbol for the battery to report current energy level and
a sysfs file to show this information to a user.

Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
 Documentation/ABI/testing/sysfs-power |  8 ++++++++
 include/linux/suspend.h               |  2 ++
 kernel/power/main.c                   | 10 ++++++++++
 3 files changed, 20 insertions(+)

diff --git a/Documentation/ABI/testing/sysfs-power b/Documentation/ABI/testing/sysfs-power
index a3942b1036e25..e263355f99fc1 100644
--- a/Documentation/ABI/testing/sysfs-power
+++ b/Documentation/ABI/testing/sysfs-power
@@ -442,6 +442,14 @@ Description:
 		'total_hw_sleep' and 'last_hw_sleep' may not be accurate.
 		This number is measured in microseconds.
 
+What:		/sys/power/suspend_stats/last_sleep_energy
+Date:		March 2025
+Contact:	Mario Limonciello <mario.limonciello@amd.com>
+Description:
+		The /sys/power/suspend_stats/last_sleep_energy file
+		contains the amount of energy that the battery consumed
+		during the last sleep cycle. This number is measured in mAh.
+
 What:		/sys/power/sync_on_suspend
 Date:		October 2019
 Contact:	Jonas Meurer <jonas@freesources.org>
diff --git a/include/linux/suspend.h b/include/linux/suspend.h
index da6ebca3ff774..9627e2394c8a9 100644
--- a/include/linux/suspend.h
+++ b/include/linux/suspend.h
@@ -441,6 +441,7 @@ extern int unregister_pm_notifier(struct notifier_block *nb);
 extern void ksys_sync_helper(void);
 extern void pm_report_hw_sleep_time(u64 t);
 extern void pm_report_max_hw_sleep(u64 t);
+extern void pm_report_sleep_energy(u64 t);
 
 #define pm_notifier(fn, pri) {				\
 	static struct notifier_block fn##_nb =			\
@@ -484,6 +485,7 @@ static inline int unregister_pm_notifier(struct notifier_block *nb)
 
 static inline void pm_report_hw_sleep_time(u64 t) {};
 static inline void pm_report_max_hw_sleep(u64 t) {};
+static inline void pm_report_sleep_energy(u64 t) {};
 
 static inline void ksys_sync_helper(void) {}
 
diff --git a/kernel/power/main.c b/kernel/power/main.c
index 6254814d48171..9305c86e0b91a 100644
--- a/kernel/power/main.c
+++ b/kernel/power/main.c
@@ -321,6 +321,7 @@ struct suspend_stats {
 	u64 last_hw_sleep;
 	u64 total_hw_sleep;
 	u64 max_hw_sleep;
+	u64 last_sleep_energy;
 	enum suspend_stat_step failed_steps[REC_FAILED_NUM];
 };
 
@@ -368,6 +369,12 @@ void pm_report_hw_sleep_time(u64 t)
 }
 EXPORT_SYMBOL_GPL(pm_report_hw_sleep_time);
 
+void pm_report_sleep_energy(u64 t)
+{
+	suspend_stats.last_sleep_energy = t;
+}
+EXPORT_SYMBOL_GPL(pm_report_sleep_energy);
+
 void pm_report_max_hw_sleep(u64 t)
 {
 	suspend_stats.max_hw_sleep = t;
@@ -399,6 +406,7 @@ suspend_attr(fail, "%u\n");
 suspend_attr(last_hw_sleep, "%llu\n");
 suspend_attr(total_hw_sleep, "%llu\n");
 suspend_attr(max_hw_sleep, "%llu\n");
+suspend_attr(last_sleep_energy, "%llu\n");
 
 #define suspend_step_attr(_name, step)		\
 static ssize_t _name##_show(struct kobject *kobj,		\
@@ -477,6 +485,7 @@ static struct attribute *suspend_attrs[] = {
 	&last_hw_sleep.attr,
 	&total_hw_sleep.attr,
 	&max_hw_sleep.attr,
+	&last_sleep_energy.attr,
 	NULL,
 };
 
@@ -484,6 +493,7 @@ static umode_t suspend_attr_is_visible(struct kobject *kobj, struct attribute *a
 {
 	if (attr != &last_hw_sleep.attr &&
 	    attr != &total_hw_sleep.attr &&
+	    attr != &last_sleep_energy.attr &&
 	    attr != &max_hw_sleep.attr)
 		return 0444;
 
-- 
2.43.0

[PATCH 2/4] ACPI: battery: Save and report battery capacity over suspend

[Mario Limonciello]

From: Mario Limonciello <mario.limonciello@amd.com>

To find out if a device is malfunctioning over suspend it's often
interesting to know how much battery was lost.

At the start of the suspend cycle cache the amount of battery.
During resume, read the battery level again and if the battery
has decreased report the difference to the suspend stats using
pm_report_sleep_energy()

Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
 drivers/acpi/battery.c | 30 ++++++++++++++++++++++++++----
 1 file changed, 26 insertions(+), 4 deletions(-)

diff --git a/drivers/acpi/battery.c b/drivers/acpi/battery.c
index 6760330a8af55..f21bfd02a26d1 100644
--- a/drivers/acpi/battery.c
+++ b/drivers/acpi/battery.c
@@ -124,6 +124,7 @@ struct acpi_battery {
 	char oem_info[MAX_STRING_LENGTH];
 	int state;
 	int power_unit;
+	int capacity_suspend;
 	unsigned long flags;
 };
 
@@ -1011,9 +1012,6 @@ static int acpi_battery_update(struct acpi_battery *battery, bool resume)
 		return 0;
 	}
 
-	if (resume)
-		return 0;
-
 	if (!battery->update_time) {
 		result = acpi_battery_get_info(battery);
 		if (result)
@@ -1032,6 +1030,14 @@ static int acpi_battery_update(struct acpi_battery *battery, bool resume)
 			return result;
 	}
 
+	if (resume) {
+		if (battery->capacity_suspend > battery->capacity_now)
+			pm_report_sleep_energy(battery->capacity_suspend - battery->capacity_now);
+		else
+			pm_report_sleep_energy(0);
+		return 0;
+	}
+
 	/*
 	 * Wakeup the system if battery is critical low
 	 * or lower than the alarm level
@@ -1285,6 +1291,22 @@ static void acpi_battery_remove(struct acpi_device *device)
 }
 
 /* this is needed to learn about changes made in suspended state */
+static int acpi_battery_suspend(struct device *dev)
+{
+	struct acpi_battery *battery;
+
+	if (!dev)
+		return -EINVAL;
+
+	battery = acpi_driver_data(to_acpi_device(dev));
+	if (!battery)
+		return -EINVAL;
+
+	battery->capacity_suspend = battery->capacity_now;
+
+	return 0;
+}
+
 static int acpi_battery_resume(struct device *dev)
 {
 	struct acpi_battery *battery;
@@ -1301,7 +1323,7 @@ static int acpi_battery_resume(struct device *dev)
 	return 0;
 }
 
-static DEFINE_SIMPLE_DEV_PM_OPS(acpi_battery_pm, NULL, acpi_battery_resume);
+static DEFINE_SIMPLE_DEV_PM_OPS(acpi_battery_pm, acpi_battery_suspend, acpi_battery_resume);
 
 static struct acpi_driver acpi_battery_driver = {
 	.name = "battery",
-- 
2.43.0

Re: [PATCH 2/4] ACPI: battery: Save and report battery capacity over suspend

[Sebastian Reichel]

[-- Attachment #1: Type: text/plain, Size: 3189 bytes --]

Hi,

On Sat, Feb 08, 2025 at 10:22:08AM -0600, Mario Limonciello wrote:
> From: Mario Limonciello <mario.limonciello@amd.com>
> 
> To find out if a device is malfunctioning over suspend it's often
> interesting to know how much battery was lost.
> 
> At the start of the suspend cycle cache the amount of battery.
> During resume, read the battery level again and if the battery
> has decreased report the difference to the suspend stats using
> pm_report_sleep_energy()
> 
> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
> ---

This code assumes, that there is only a single battery, but there
can be more than one battery supplying the system. For example
Thinkpads used to have an internal battery and a user swappable
one.

Also it seems in almost all cases debugging this from userspace
by dropping a script in /usr/lib/systemd/system-sleep is good
enough, so I wonder if extending the kernel ABI makes sense at
all.

Greetings,

-- Sebastian

>  drivers/acpi/battery.c | 30 ++++++++++++++++++++++++++----
>  1 file changed, 26 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/acpi/battery.c b/drivers/acpi/battery.c
> index 6760330a8af55..f21bfd02a26d1 100644
> --- a/drivers/acpi/battery.c
> +++ b/drivers/acpi/battery.c
> @@ -124,6 +124,7 @@ struct acpi_battery {
>  	char oem_info[MAX_STRING_LENGTH];
>  	int state;
>  	int power_unit;
> +	int capacity_suspend;
>  	unsigned long flags;
>  };
>  
> @@ -1011,9 +1012,6 @@ static int acpi_battery_update(struct acpi_battery *battery, bool resume)
>  		return 0;
>  	}
>  
> -	if (resume)
> -		return 0;
> -
>  	if (!battery->update_time) {
>  		result = acpi_battery_get_info(battery);
>  		if (result)
> @@ -1032,6 +1030,14 @@ static int acpi_battery_update(struct acpi_battery *battery, bool resume)
>  			return result;
>  	}
>  
> +	if (resume) {
> +		if (battery->capacity_suspend > battery->capacity_now)
> +			pm_report_sleep_energy(battery->capacity_suspend - battery->capacity_now);
> +		else
> +			pm_report_sleep_energy(0);
> +		return 0;
> +	}
> +
>  	/*
>  	 * Wakeup the system if battery is critical low
>  	 * or lower than the alarm level
> @@ -1285,6 +1291,22 @@ static void acpi_battery_remove(struct acpi_device *device)
>  }
>  
>  /* this is needed to learn about changes made in suspended state */
> +static int acpi_battery_suspend(struct device *dev)
> +{
> +	struct acpi_battery *battery;
> +
> +	if (!dev)
> +		return -EINVAL;
> +
> +	battery = acpi_driver_data(to_acpi_device(dev));
> +	if (!battery)
> +		return -EINVAL;
> +
> +	battery->capacity_suspend = battery->capacity_now;
> +
> +	return 0;
> +}
> +
>  static int acpi_battery_resume(struct device *dev)
>  {
>  	struct acpi_battery *battery;
> @@ -1301,7 +1323,7 @@ static int acpi_battery_resume(struct device *dev)
>  	return 0;
>  }
>  
> -static DEFINE_SIMPLE_DEV_PM_OPS(acpi_battery_pm, NULL, acpi_battery_resume);
> +static DEFINE_SIMPLE_DEV_PM_OPS(acpi_battery_pm, acpi_battery_suspend, acpi_battery_resume);
>  
>  static struct acpi_driver acpi_battery_driver = {
>  	.name = "battery",
> -- 
> 2.43.0
> 
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 2/4] ACPI: battery: Save and report battery capacity over suspend

[Mario Limonciello]

On 2/10/2025 09:23, Sebastian Reichel wrote:
> Hi,
> 
> On Sat, Feb 08, 2025 at 10:22:08AM -0600, Mario Limonciello wrote:
>> From: Mario Limonciello <mario.limonciello@amd.com>
>>
>> To find out if a device is malfunctioning over suspend it's often
>> interesting to know how much battery was lost.
>>
>> At the start of the suspend cycle cache the amount of battery.
>> During resume, read the battery level again and if the battery
>> has decreased report the difference to the suspend stats using
>> pm_report_sleep_energy()
>>
>> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
>> ---
> 
> This code assumes, that there is only a single battery, but there
> can be more than one battery supplying the system. For example
> Thinkpads used to have an internal battery and a user swappable
> one.
> 
> Also it seems in almost all cases debugging this from userspace
> by dropping a script in /usr/lib/systemd/system-sleep is good
> enough, so I wonder if extending the kernel ABI makes sense at
> all.
> 

Thanks for looking.  I think it could be extended to add all the 
batteries up and collectively look at how much each went down.

But that's a good point it's pretty easy to do the same thing from 
userspace.

> Greetings,
> 
> -- Sebastian
> 
>>   drivers/acpi/battery.c | 30 ++++++++++++++++++++++++++----
>>   1 file changed, 26 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/acpi/battery.c b/drivers/acpi/battery.c
>> index 6760330a8af55..f21bfd02a26d1 100644
>> --- a/drivers/acpi/battery.c
>> +++ b/drivers/acpi/battery.c
>> @@ -124,6 +124,7 @@ struct acpi_battery {
>>   	char oem_info[MAX_STRING_LENGTH];
>>   	int state;
>>   	int power_unit;
>> +	int capacity_suspend;
>>   	unsigned long flags;
>>   };
>>   
>> @@ -1011,9 +1012,6 @@ static int acpi_battery_update(struct acpi_battery *battery, bool resume)
>>   		return 0;
>>   	}
>>   
>> -	if (resume)
>> -		return 0;
>> -
>>   	if (!battery->update_time) {
>>   		result = acpi_battery_get_info(battery);
>>   		if (result)
>> @@ -1032,6 +1030,14 @@ static int acpi_battery_update(struct acpi_battery *battery, bool resume)
>>   			return result;
>>   	}
>>   
>> +	if (resume) {
>> +		if (battery->capacity_suspend > battery->capacity_now)
>> +			pm_report_sleep_energy(battery->capacity_suspend - battery->capacity_now);
>> +		else
>> +			pm_report_sleep_energy(0);
>> +		return 0;
>> +	}
>> +
>>   	/*
>>   	 * Wakeup the system if battery is critical low
>>   	 * or lower than the alarm level
>> @@ -1285,6 +1291,22 @@ static void acpi_battery_remove(struct acpi_device *device)
>>   }
>>   
>>   /* this is needed to learn about changes made in suspended state */
>> +static int acpi_battery_suspend(struct device *dev)
>> +{
>> +	struct acpi_battery *battery;
>> +
>> +	if (!dev)
>> +		return -EINVAL;
>> +
>> +	battery = acpi_driver_data(to_acpi_device(dev));
>> +	if (!battery)
>> +		return -EINVAL;
>> +
>> +	battery->capacity_suspend = battery->capacity_now;
>> +
>> +	return 0;
>> +}
>> +
>>   static int acpi_battery_resume(struct device *dev)
>>   {
>>   	struct acpi_battery *battery;
>> @@ -1301,7 +1323,7 @@ static int acpi_battery_resume(struct device *dev)
>>   	return 0;
>>   }
>>   
>> -static DEFINE_SIMPLE_DEV_PM_OPS(acpi_battery_pm, NULL, acpi_battery_resume);
>> +static DEFINE_SIMPLE_DEV_PM_OPS(acpi_battery_pm, acpi_battery_suspend, acpi_battery_resume);
>>   
>>   static struct acpi_driver acpi_battery_driver = {
>>   	.name = "battery",
>> -- 
>> 2.43.0
>>
>>

[PATCH 3/4] ACPI: battery: Refactor wakeup reasons in acpi_battery_update()

[Mario Limonciello]

From: Mario Limonciello <mario.limonciello@amd.com>

Another reason to wake is to be introduced, so refactor the code
to make it easier to follow. Also introduce PM debug messages so
that it's more obvious when these wake events have occurred.

Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
 drivers/acpi/battery.c | 24 +++++++++++++++++-------
 1 file changed, 17 insertions(+), 7 deletions(-)

diff --git a/drivers/acpi/battery.c b/drivers/acpi/battery.c
index f21bfd02a26d1..72c8a509695e6 100644
--- a/drivers/acpi/battery.c
+++ b/drivers/acpi/battery.c
@@ -999,6 +999,22 @@ static void acpi_battery_quirks(struct acpi_battery *battery)
 	}
 }
 
+static inline bool acpi_battery_should_wake(struct acpi_battery *battery)
+{
+	if (battery->state & ACPI_BATTERY_STATE_CRITICAL) {
+		pm_pr_dbg("Waking due critical battery level");
+		return true;
+	}
+
+	if (test_bit(ACPI_BATTERY_ALARM_PRESENT, &battery->flags) &&
+	    battery->capacity_now <= battery->alarm) {
+		pm_pr_dbg("Waking due to battery alarm");
+		return true;
+	}
+
+	return false;
+}
+
 static int acpi_battery_update(struct acpi_battery *battery, bool resume)
 {
 	int result = acpi_battery_get_status(battery);
@@ -1038,13 +1054,7 @@ static int acpi_battery_update(struct acpi_battery *battery, bool resume)
 		return 0;
 	}
 
-	/*
-	 * Wakeup the system if battery is critical low
-	 * or lower than the alarm level
-	 */
-	if ((battery->state & ACPI_BATTERY_STATE_CRITICAL) ||
-	    (test_bit(ACPI_BATTERY_ALARM_PRESENT, &battery->flags) &&
-	     (battery->capacity_now <= battery->alarm)))
+	if (acpi_battery_should_wake(battery))
 		acpi_pm_wakeup_event(&battery->device->dev);
 
 	return result;
-- 
2.43.0

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

[Mario Limonciello]

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.

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

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

[Rafael J. Wysocki]

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
>
>

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

[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?

> 
>> 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
>>
>>

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

[Armin Wolf]

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
>>>
>>>
>
>

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

[Rafael J. Wysocki]

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.

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

[K G]

As only a mildly technical Linux user, I would rather this not be
rolled out as the default behavior. In case this fixes issues some
users are having, I would prefer this to be enabled via a bootup
option that is documented somewhere so that only affected users can
use it. FWIW I'm on  6.8.0-51-generic on a ThinkPad E14 G4 and the
power plugin/plugout behavior is just fine. Thank you.