| 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
| ||
|
Message-ID: <20190902183155.6nugohjfr3pktxve@earth.universe>
Date: Mon, 2 Sep 2019 20:31:55 +0200
From: Sebastian Reichel <sre@...nel.org>
To: Nick Crews <ncrews@...omium.org>
Cc: Richard Tresidder <rtresidd@...ctromag.com.au>,
Enric Balletbo i Serra <enric.balletbo@...labora.com>,
andrew.smirnov@...il.com, Guenter Roeck <groeck@...omium.org>,
david@...hnology.com, Thomas Gleixner <tglx@...utronix.de>,
Kate Stewart <kstewart@...uxfoundation.org>,
Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
rfontana@...hat.com, allison@...utok.net,
Baolin Wang <baolin.wang@...aro.org>, linux-pm@...r.kernel.org,
linux-kernel <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH v4 1/1] power/supply/sbs-battery: Fix confusing battery
status when idle or empty
Hi,
Looks good to me, but needs to be split (one patch adding STATUS_EMPTY
to the power-supply core and one patch fixing sbs-battery driver).
-- Sebastian
On Thu, Aug 01, 2019 at 12:34:18PM -0600, Nick Crews wrote:
> Thanks Richard, I still would like some more opinions
> on this changing the userspace experience, but LGTM
> otherwise.
>
> Reviewed-by: Nick Crews <ncrews@...omium.org>
>
> On Mon, Jul 29, 2019 at 8:00 PM Richard Tresidder
> <rtresidd@...ctromag.com.au> wrote:
> >
> > When a battery or batteries in a system are in parallel then one or more
> > may not be providing any current to the system.
> > This fixes an incorrect status indication of FULL for the battery simply
> > because it wasn't discharging at that point in time.
> > The battery will now be flagged as NOT CHARGING.
> > Have also added the additional check for the battery FULL DISCHARGED flag
> > which will now flag a status of EMPTY.
> >
> > Signed-off-by: Richard Tresidder <rtresidd@...ctromag.com.au>
> > ---
> >
> > Notes:
> > power/supply/sbs-battery: Fix confusing battery status when idle or empty
> >
> > When a battery or batteries in a system are in parallel then one or more
> > may not be providing any current to the system.
> > This fixes an incorrect status indication of FULL for the battery simply
> > because it wasn't discharging at that point in time.
> > The battery will now be flagged as NOT CHARGING.
> > Have also added the additional check for the battery FULL DISCHARGED flag
> > which will now flag a status of EMPTY.
> >
> > v2: Missed a later merge that should have been included in original patch
> > v3: Refactor the sbs_status_correct function to capture all the states for
> > normal operation rather than being spread across multile functions.
> > v4: Remove unnecessary brackets, rename sbs_status_correct to
> > sbs_correct_battery_status
> >
> > drivers/power/supply/power_supply_sysfs.c | 2 +-
> > drivers/power/supply/sbs-battery.c | 46 ++++++++++++-------------------
> > include/linux/power_supply.h | 1 +
> > 3 files changed, 19 insertions(+), 30 deletions(-)
> >
> > diff --git a/drivers/power/supply/power_supply_sysfs.c b/drivers/power/supply/power_supply_sysfs.c
> > index f37ad4e..305e833 100644
> > --- a/drivers/power/supply/power_supply_sysfs.c
> > +++ b/drivers/power/supply/power_supply_sysfs.c
> > @@ -51,7 +51,7 @@
> > };
> >
> > static const char * const power_supply_status_text[] = {
> > - "Unknown", "Charging", "Discharging", "Not charging", "Full"
> > + "Unknown", "Charging", "Discharging", "Not charging", "Full", "Empty"
> > };
> >
> > static const char * const power_supply_charge_type_text[] = {
> > diff --git a/drivers/power/supply/sbs-battery.c b/drivers/power/supply/sbs-battery.c
> > index 048d205..3ed70d4 100644
> > --- a/drivers/power/supply/sbs-battery.c
> > +++ b/drivers/power/supply/sbs-battery.c
> > @@ -283,7 +283,7 @@ static int sbs_write_word_data(struct i2c_client *client, u8 address,
> > return 0;
> > }
> >
> > -static int sbs_status_correct(struct i2c_client *client, int *intval)
> > +static int sbs_correct_battery_status(struct i2c_client *client, int *status)
> > {
> > int ret;
> >
> > @@ -293,16 +293,18 @@ static int sbs_status_correct(struct i2c_client *client, int *intval)
> >
> > ret = (s16)ret;
> >
> > - /* Not drawing current means full (cannot be not charging) */
> > - if (ret == 0)
> > - *intval = POWER_SUPPLY_STATUS_FULL;
> > -
> > - if (*intval == POWER_SUPPLY_STATUS_FULL) {
> > - /* Drawing or providing current when full */
> > - if (ret > 0)
> > - *intval = POWER_SUPPLY_STATUS_CHARGING;
> > - else if (ret < 0)
> > - *intval = POWER_SUPPLY_STATUS_DISCHARGING;
> > + if (ret > 0)
> > + *status = POWER_SUPPLY_STATUS_CHARGING;
> > + else if (ret < 0)
> > + *status = POWER_SUPPLY_STATUS_DISCHARGING;
> > + else {
> > + /* Current is 0, so how full is the battery? */
> > + if (*status & BATTERY_FULL_CHARGED)
> > + *status = POWER_SUPPLY_STATUS_FULL;
> > + else if (*status & BATTERY_FULL_DISCHARGED)
> > + *status = POWER_SUPPLY_STATUS_EMPTY;
> > + else
> > + *status = POWER_SUPPLY_STATUS_NOT_CHARGING;
> > }
> >
> > return 0;
> > @@ -421,14 +423,9 @@ static int sbs_get_battery_property(struct i2c_client *client,
> > return 0;
> > }
> >
> > - if (ret & BATTERY_FULL_CHARGED)
> > - val->intval = POWER_SUPPLY_STATUS_FULL;
> > - else if (ret & BATTERY_DISCHARGING)
> > - val->intval = POWER_SUPPLY_STATUS_DISCHARGING;
> > - else
> > - val->intval = POWER_SUPPLY_STATUS_CHARGING;
> > -
> > - sbs_status_correct(client, &val->intval);
> > + ret = sbs_correct_battery_status(client, &val->intval);
> > + if (ret < 0)
> > + return ret;
> >
> > if (chip->poll_time == 0)
> > chip->last_state = val->intval;
> > @@ -773,20 +770,11 @@ static void sbs_delayed_work(struct work_struct *work)
> >
> > ret = sbs_read_word_data(chip->client, sbs_data[REG_STATUS].addr);
> > /* if the read failed, give up on this work */
> > - if (ret < 0) {
> > + if (ret < 0 || sbs_correct_battery_status(chip->client, &ret) < 0) {
> > chip->poll_time = 0;
> > return;
> > }
> >
> > - if (ret & BATTERY_FULL_CHARGED)
> > - ret = POWER_SUPPLY_STATUS_FULL;
> > - else if (ret & BATTERY_DISCHARGING)
> > - ret = POWER_SUPPLY_STATUS_DISCHARGING;
> > - else
> > - ret = POWER_SUPPLY_STATUS_CHARGING;
> > -
> > - sbs_status_correct(chip->client, &ret);
> > -
> > if (chip->last_state != ret) {
> > chip->poll_time = 0;
> > power_supply_changed(chip->power_supply);
> > diff --git a/include/linux/power_supply.h b/include/linux/power_supply.h
> > index 28413f7..8fb10ec 100644
> > --- a/include/linux/power_supply.h
> > +++ b/include/linux/power_supply.h
> > @@ -37,6 +37,7 @@ enum {
> > POWER_SUPPLY_STATUS_DISCHARGING,
> > POWER_SUPPLY_STATUS_NOT_CHARGING,
> > POWER_SUPPLY_STATUS_FULL,
> > + POWER_SUPPLY_STATUS_EMPTY,
> > };
> >
> > /* What algorithm is the charger using? */
> > --
> > 1.8.3.1
> >
Download attachment "signature.asc" of type "application/pgp-signature" (834 bytes)
Powered by blists - more mailing lists