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

[Mario Limonciello <superm1@kernel.org>]

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