linux-kernel - Re: [PATCH RESEND] power: supply: qcom_battmgr: abs() on POWER

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <7wbot7sxm3y5y7in5ashcn5lpx3mi55abnbfrkz2jta7nm6jep@zk6zvocd3tuz>
Date: Sat, 15 Feb 2025 04:08:25 +0100
From: Sebastian Reichel <sebastian.reichel@...labora.com>
To: Dmitry Baryshkov <dmitry.baryshkov@...aro.org>
Cc: aruhier@...lbox.org, linux-arm-msm@...r.kernel.org, 
	linux-pm@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH RESEND] power: supply: qcom_battmgr: abs() on POWER_NOW
 property

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@...lbox.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@...lbox.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@...lbox.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