diff mbox series

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

Message ID 20250208162210.3929473-3-superm1@kernel.org
State New
Headers show
Series Improvements to ACPI battery handling over s2idle | expand

Commit Message

Mario Limonciello Feb. 8, 2025, 4:22 p.m. UTC
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(-)

Comments

Sebastian Reichel Feb. 10, 2025, 3:23 p.m. UTC | #1
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
> 
>
diff mbox series

Patch

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