| Message ID | 20250213-patch-qcomm-bat-uint-power-v1-1-16e7e2a77a02@mailbox.org (mailing list archive) |
|---|---|
| State | Handled Elsewhere, archived |
| Headers | show |
| Series | [RESEND] power: supply: qcom_battmgr: abs() on POWER_NOW property | expand |
On Thu, Feb 13, 2025 at 05:51:38PM +0100, Anthony Ruhier via B4 Relay wrote: > From: Anthony Ruhier <aruhier@mailbox.org> > > The value for the POWER_NOW property is by default negative when the > battery is discharging, positive when charging. > > However on x1e laptops it breaks several userland tools that give a > prediction of the battery run time (such as the acpi command, powertop > or the waybar battery module), as these tools do not expect a negative > value for /sys/class/power_supply/qcom-battmgr-bat/power_now. They > estimate the battery run time by dividing the value of energy_full by > power_now. The battery percentage is calculated by dividing energy_full > by energy_now, therefore it is not impacted. > > While having a negative number during discharge makes sense, it is not > standard with how other battery drivers expose it. Instead, it seems > standard to have a positive value for power_now, and rely on the status > file instead to know if the battery is charging or discharging. It is > what other x86 laptops do. Documentation/ABI does not define ABI for the power_now. However for current_now it clearly defines that it can be positive or negative. > > Without the patch: > $ acpi > Battery 0: Discharging, 98%, discharging at zero rate - will never fully discharge. > > With the patch: > $ acpi > Battery 0: Discharging, 97%, 10:18:27 remaining > > --- > Signed-off-by: Anthony Ruhier <aruhier@mailbox.org> > --- > drivers/power/supply/qcom_battmgr.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-)
On Fri, Feb 14, 2025 at 12:24:18AM +0200, Dmitry Baryshkov wrote: > On Thu, Feb 13, 2025 at 05:51:38PM +0100, Anthony Ruhier via B4 Relay wrote: > > From: Anthony Ruhier <aruhier@mailbox.org> > > > > The value for the POWER_NOW property is by default negative when the > > battery is discharging, positive when charging. > > > > However on x1e laptops it breaks several userland tools that give a > > prediction of the battery run time (such as the acpi command, powertop > > or the waybar battery module), as these tools do not expect a negative > > value for /sys/class/power_supply/qcom-battmgr-bat/power_now. They > > estimate the battery run time by dividing the value of energy_full by > > power_now. The battery percentage is calculated by dividing energy_full > > by energy_now, therefore it is not impacted. > > > > While having a negative number during discharge makes sense, it is not > > standard with how other battery drivers expose it. Instead, it seems > > standard to have a positive value for power_now, and rely on the status > > file instead to know if the battery is charging or discharging. It is > > what other x86 laptops do. > > Documentation/ABI does not define ABI for the power_now. However for > current_now it clearly defines that it can be positive or negative. > > > > > Without the patch: > > $ acpi > > Battery 0: Discharging, 98%, discharging at zero rate - will never fully discharge. > > > > With the patch: > > $ acpi > > Battery 0: Discharging, 97%, 10:18:27 remaining > > > > --- > > Signed-off-by: Anthony Ruhier <aruhier@mailbox.org> > > --- > > drivers/power/supply/qcom_battmgr.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > -- > With best wishes > Dmitry I see. But as it breaks existing tools when power_now is negative, should we change the behavior of these tools or adapt the driver? As it does not seem common that power_now and current_now are negative in other drivers, tools using these values rely on the status anyway. I'm wondering if it provides anything to keep this behavior. -- Regards, Anthony Ruhier
On Fri, Feb 14, 2025 at 02:36:17AM +0100, aruhier@mailbox.org wrote: > On Fri, Feb 14, 2025 at 12:24:18AM +0200, Dmitry Baryshkov wrote: > > On Thu, Feb 13, 2025 at 05:51:38PM +0100, Anthony Ruhier via B4 Relay wrote: > > > From: Anthony Ruhier <aruhier@mailbox.org> > > > > > > The value for the POWER_NOW property is by default negative when the > > > battery is discharging, positive when charging. > > > > > > However on x1e laptops it breaks several userland tools that give a > > > prediction of the battery run time (such as the acpi command, powertop > > > or the waybar battery module), as these tools do not expect a negative > > > value for /sys/class/power_supply/qcom-battmgr-bat/power_now. They > > > estimate the battery run time by dividing the value of energy_full by > > > power_now. The battery percentage is calculated by dividing energy_full > > > by energy_now, therefore it is not impacted. > > > > > > While having a negative number during discharge makes sense, it is not > > > standard with how other battery drivers expose it. Instead, it seems > > > standard to have a positive value for power_now, and rely on the status > > > file instead to know if the battery is charging or discharging. It is > > > what other x86 laptops do. > > > > Documentation/ABI does not define ABI for the power_now. However for > > current_now it clearly defines that it can be positive or negative. > > > > > > > > Without the patch: > > > $ acpi > > > Battery 0: Discharging, 98%, discharging at zero rate - will never fully discharge. > > > > > > With the patch: > > > $ acpi > > > Battery 0: Discharging, 97%, 10:18:27 remaining > > > > > > --- > > > Signed-off-by: Anthony Ruhier <aruhier@mailbox.org> > > > --- > > > drivers/power/supply/qcom_battmgr.c | 2 +- > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > -- > > With best wishes > > Dmitry > > I see. But as it breaks existing tools when power_now is negative, should we > change the behavior of these tools or adapt the driver? > > As it does not seem common that power_now and current_now are negative in > other drivers, tools using these values rely on the status anyway. I'm > wondering if it provides anything to keep this behavior. I think it is a problem of the 'acpi' tool. At least 'upower -d' uses fabs internally since the initial commit in 2008.
Hi, On Fri, Feb 14, 2025 at 05:01:08AM +0200, Dmitry Baryshkov wrote: > On Fri, Feb 14, 2025 at 02:36:17AM +0100, aruhier@mailbox.org wrote: > > On Fri, Feb 14, 2025 at 12:24:18AM +0200, Dmitry Baryshkov wrote: > > > On Thu, Feb 13, 2025 at 05:51:38PM +0100, Anthony Ruhier via B4 Relay wrote: > > > > From: Anthony Ruhier <aruhier@mailbox.org> > > > > > > > > The value for the POWER_NOW property is by default negative when the > > > > battery is discharging, positive when charging. > > > > > > > > However on x1e laptops it breaks several userland tools that give a > > > > prediction of the battery run time (such as the acpi command, powertop > > > > or the waybar battery module), as these tools do not expect a negative > > > > value for /sys/class/power_supply/qcom-battmgr-bat/power_now. They > > > > estimate the battery run time by dividing the value of energy_full by > > > > power_now. The battery percentage is calculated by dividing energy_full > > > > by energy_now, therefore it is not impacted. > > > > > > > > While having a negative number during discharge makes sense, it is not > > > > standard with how other battery drivers expose it. Instead, it seems > > > > standard to have a positive value for power_now, and rely on the status > > > > file instead to know if the battery is charging or discharging. It is > > > > what other x86 laptops do. > > > > > > Documentation/ABI does not define ABI for the power_now. However for > > > current_now it clearly defines that it can be positive or negative. > > > > > > > > > > > Without the patch: > > > > $ acpi > > > > Battery 0: Discharging, 98%, discharging at zero rate - will never fully discharge. > > > > > > > > With the patch: > > > > $ acpi > > > > Battery 0: Discharging, 97%, 10:18:27 remaining > > > > > > > > --- > > > > Signed-off-by: Anthony Ruhier <aruhier@mailbox.org> > > > > --- > > > > drivers/power/supply/qcom_battmgr.c | 2 +- > > > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > > > -- > > > With best wishes > > > Dmitry > > > > I see. But as it breaks existing tools when power_now is negative, should we > > change the behavior of these tools or adapt the driver? > > > > As it does not seem common that power_now and current_now are negative in > > other drivers, tools using these values rely on the status anyway. I'm > > wondering if it provides anything to keep this behavior. There are other drivers reporting negative values as documented. Most of the embedded ones do this actually and there surely are (embedded) userspace programs relying on this by now. But the most used driver - generic ACPI battery - does not. That's why quite a few userspace tools handle it wrong without anyone noticing for quite some time. Fixing it to follow the ABI would obviously end up in a bunch of regression reports, so things are a bit messy :( > I think it is a problem of the 'acpi' tool. At least 'upower -d' uses > fabs internally since the initial commit in 2008. It's definitely sensible to fix the userspace tools. We can't change the documented ABI for current_now after that many years and while documentation for power_now is missing, it would be quite unexpected to have it behave differently than current_now. Also userspace tooling needs to handle current_now and power_now anyways. And we surely can't change the behaviour for all drivers reporting signed data. So let's keep qcom_battmgr as is. It follows the documented ABI and hopefully helps giving this more exposure (I'm typing this on a X1E laptop right now and can see your problem with waybar). But we should document the power_now property. It somehow fell through the cracks :) -- Sebastian
On Sat, Feb 15, 2025 at 04:08:25AM +0100, Sebastian Reichel wrote: > > There are other drivers reporting negative values as documented. > Most of the embedded ones do this actually and there surely are > (embedded) userspace programs relying on this by now. But the > most used driver - generic ACPI battery - does not. That's why > quite a few userspace tools handle it wrong without anyone > noticing for quite some time. Fixing it to follow the ABI would > obviously end up in a bunch of regression reports, so things are > a bit messy :( > > > I think it is a problem of the 'acpi' tool. At least 'upower -d' uses > > fabs internally since the initial commit in 2008. > > It's definitely sensible to fix the userspace tools. We can't change > the documented ABI for current_now after that many years and while > documentation for power_now is missing, it would be quite unexpected > to have it behave differently than current_now. Also userspace > tooling needs to handle current_now and power_now anyways. And we > surely can't change the behaviour for all drivers reporting signed > data. So let's keep qcom_battmgr as is. It follows the documented > ABI and hopefully helps giving this more exposure (I'm typing this > on a X1E laptop right now and can see your problem with waybar). > > But we should document the power_now property. It somehow fell > through the cracks :) > > -- Sebastian Hi Sebastian, Thanks a lot for the detailed answer, that makes sense for me. I was sending this patch more to know which direction to follow (changing the driver or the userspace tools), and you answered it perfectly. I started fixing the different desktop tools I use, starting with Waybar: https://github.com/Alexays/Waybar/pull/3942 For powertop, the fix seems straightforward. For acpiclient, due to no activity in almost 10 years, we'll see if it goes through. -- Thanks, Anthony Ruhier
diff --git a/drivers/power/supply/qcom_battmgr.c b/drivers/power/supply/qcom_battmgr.c index 47d29271ddf400b76dd5b0a1b8d1ba86c017afc0..3e2e0c5af2814df0eb0bfc408d4b3d26399ab4e4 100644 --- a/drivers/power/supply/qcom_battmgr.c +++ b/drivers/power/supply/qcom_battmgr.c @@ -530,7 +530,7 @@ static int qcom_battmgr_bat_get_property(struct power_supply *psy, val->intval = battmgr->status.current_now; break; case POWER_SUPPLY_PROP_POWER_NOW: - val->intval = battmgr->status.power_now; + val->intval = abs(battmgr->status.power_now); break; case POWER_SUPPLY_PROP_CHARGE_FULL_DESIGN: if (unit != QCOM_BATTMGR_UNIT_mAh)