linux-kernel - Re: [PATCH] power: supply: qcom_battmgr: Report battery capacity

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: <CACF_fqksF+whYbGEdSvJ=87FQH03EzO+hSSf8eRc8MitR2hzxA@mail.gmail.com>
Date: Wed, 28 May 2025 11:57:32 +0200
From: Kornel Dulęba <korneld@...gle.com>
To: Dmitry Baryshkov <dmitry.baryshkov@....qualcomm.com>
Cc: Sebastian Reichel <sre@...nel.org>, Bjorn Andersson <andersson@...nel.org>, linux-arm-msm@...r.kernel.org, 
	linux-pm@...r.kernel.org, linux-kernel@...r.kernel.org, 
	chromeos-krk-upstreaming@...gle.com
Subject: Re: [PATCH] power: supply: qcom_battmgr: Report battery capacity

On Tue, May 27, 2025 at 9:34 PM 'Dmitry Baryshkov' via
chromeos-krk-upstreaming <chromeos-krk-upstreaming@...gle.com> wrote:
>
> On Tue, May 27, 2025 at 12:18:07PM +0000, Kornel Dulęba wrote:
> > Battery charge can be reported in several different ways. One of them is
> > is charge percentage referred to as POWER_SUPPLY_PROP_CAPACITY in the
> > power supply API. Currently the driver reports the capacity in this way
> > on SM8350, but not on the newer variants referred to as SC8280XP in the
> > driver. Although this is not a bug in itself, not reporting the
> > percentage can confuse some userspace consumers.
> > Mimic what is done in the ACPI driver (drivers/acpi/battery.c) and
> > calculate the percentage capacity by dividing the current charge value
> > by the full charge. Both values are expressed in either uWh, or
> > in uAh.
> >
> > Signed-off-by: Kornel Dulęba <korneld@...gle.com>
> > ---
> >  drivers/power/supply/qcom_battmgr.c | 18 ++++++++++++++++++
> >  1 file changed, 18 insertions(+)
> >
> > diff --git a/drivers/power/supply/qcom_battmgr.c b/drivers/power/supply/qcom_battmgr.c
> > index fe27676fbc7c..5ed5452ab51c 100644
> > --- a/drivers/power/supply/qcom_battmgr.c
> > +++ b/drivers/power/supply/qcom_battmgr.c
> > @@ -577,6 +577,8 @@ static int qcom_battmgr_bat_get_property(struct power_supply *psy,
> >               val->intval = battmgr->status.capacity;
> >               break;
> >       case POWER_SUPPLY_PROP_CAPACITY:
> > +             if (battmgr->status.percent == (unsigned int)-1)
> > +                     return -ENODATA;
> >               val->intval = battmgr->status.percent;
> >               break;
> >       case POWER_SUPPLY_PROP_TEMP:
> > @@ -617,6 +619,7 @@ static const enum power_supply_property sc8280xp_bat_props[] = {
> >       POWER_SUPPLY_PROP_STATUS,
> >       POWER_SUPPLY_PROP_PRESENT,
> >       POWER_SUPPLY_PROP_TECHNOLOGY,
> > +     POWER_SUPPLY_PROP_CAPACITY,
> >       POWER_SUPPLY_PROP_CYCLE_COUNT,
> >       POWER_SUPPLY_PROP_VOLTAGE_MAX_DESIGN,
> >       POWER_SUPPLY_PROP_VOLTAGE_NOW,
> > @@ -1063,6 +1066,21 @@ static void qcom_battmgr_sc8280xp_callback(struct qcom_battmgr *battmgr,
> >               battmgr->ac.online = source == BATTMGR_CHARGING_SOURCE_AC;
> >               battmgr->usb.online = source == BATTMGR_CHARGING_SOURCE_USB;
> >               battmgr->wireless.online = source == BATTMGR_CHARGING_SOURCE_WIRELESS;
> > +             if (battmgr->info.last_full_capacity != 0) {
> > +                     /*
> > +                      * 100 * battmgr->status.capacity can overflow a 32bit
> > +                      * unsigned integer. Do a temporary cast to avoid that.
> > +                      */
> > +                     battmgr->status.percent =
> > +                             (uint64_t)100 * battmgr->status.capacity /
> > +                             battmgr->info.last_full_capacity;
>
> Can you use mult_frac(), preventing the overflow?

Good idea, but I don't think mult_frac() helps in cases where the
dividend is smaller than the divider. Let's look at the sources:
#define mult_frac(x, n, d)      \
(...)
        typeof(x_) q = x_ / d_; \
        typeof(x_) r = x_ % d_; \
        q * n_ + r * n_ / d_;   \

Since in our case x_ < d_, q = 0 and r = x_ then r * n_ will still
result in an overflow.

Unfortunately, the cast-and-divide approach won't work either. I
received an email from a kernel test robot saying that this patch
breaks a 32-bit only build. (">> ERROR: modpost: "__udivdi3"
[drivers/power/supply/qcom_battmgr.ko] undefined!") See
https://lore.kernel.org/oe-kbuild-all/202505280344.GjzOItSS-lkp@intel.com/
for details.

I suppose I could just use a do_div with a temporary variable to work
around that. I noticed that all data read from FW is multiplied by
1000, so I leveraged that instead:
battmgr->status.percent =
    (100 * le32_to_cpu(resp->status.capacity)) /
      (battmgr->info.last_full_capacity / 1000);

Any thoughts?

>
> > +             } else {
> > +                     /*
> > +                      * Let the sysfs handler know no data is available at
> > +                      * this time.
> > +                      */
> > +                     battmgr->status.percent = (unsigned int)-1;
> > +             }
> >               break;
> >       case BATTMGR_BAT_DISCHARGE_TIME:
> >               battmgr->status.discharge_time = le32_to_cpu(resp->time);
> > --
> > 2.49.0.1151.ga128411c76-goog
> >
>
> --
> With best wishes
> Dmitry
>
> --
> You received this message because you are subscribed to the Google Groups "chromeos-krk-upstreaming" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to chromeos-krk-upstreaming+unsubscribe@...gle.com.
> To view this discussion visit https://groups.google.com/a/google.com/d/msgid/chromeos-krk-upstreaming/oa5okg7i2s6s7pxm5tn6nnanazze5lnnre4vnwrhopn5s5hsil%40vhh22j6b5cvq.
> For more options, visit https://groups.google.com/a/google.com/d/optout.