linux-kernel - Re: [PATCH 2/3] power: supply: bq27xxx: fix 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] [day] [month] [year] [list]

Message-ID: <b9fb6bc3255c085c6336736c2fd7e76b1fcb8de9.camel@ew.tq-group.com>
Date:   Wed, 24 Feb 2021 12:50:39 +0100
From:   Matthias Schiffer <matthias.schiffer@...tq-group.com>
To:     Dan Murphy <dmurphy@...com>,
        Pali Rohár <pali@...nel.org>,
        Sebastian Reichel <sre@...nel.org>
Cc:     Andreas Kemnade <andreas@...nade.info>, linux-pm@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/3] power: supply: bq27xxx: fix power_avg

On Tue, 2021-02-23 at 15:11 +0100, Matthias Schiffer wrote:
> On all newer bq27xxx ICs, the AveragePower register contains a signed
> value; in addition to handling the raw value as unsigned, the driver
> code also didn't convert it to µW as expected.
> 
> At least for the BQ28Z610, the reference manual incorrectly states that
> the value is in units of 1mA and not 10mA. I have no way of knowing
> whether the manuals of other supported ICs contain the same error, or if
> there are models that actually use 1mA. At least, the new code shouldn't
> be *less* correct than the old version for any device

Ugh, of course this section should talk about mW and not mA. I'll wait
if there is more feedback and then send a v2.


> .
> 
> power_avg is removed from the cache structure, se we don't have to
> extend it to store both a signed value and an error code. Always getting
> an up-to-date value may be desirable anyways, as it avoids inconsistent
> current and power readings when switching between charging and
> discharging.
> 
> Signed-off-by: Matthias Schiffer <matthias.schiffer@...tq-group.com>
> ---
>  drivers/power/supply/bq27xxx_battery.c | 51 ++++++++++++++------------
>  include/linux/power/bq27xxx_battery.h  |  1 -
>  2 files changed, 27 insertions(+), 25 deletions(-)
> 
> diff --git a/drivers/power/supply/bq27xxx_battery.c b/drivers/power/supply/bq27xxx_battery.c
> index cb6ebd2f905e..20e1dc8a87cf 100644
> --- a/drivers/power/supply/bq27xxx_battery.c
> +++ b/drivers/power/supply/bq27xxx_battery.c
> @@ -1661,27 +1661,6 @@ static int bq27xxx_battery_read_time(struct bq27xxx_device_info *di, u8 reg)
>  	return tval * 60;
>  }
>  
> -/*
> - * Read an average power register.
> - * Return < 0 if something fails.
> - */
> -static int bq27xxx_battery_read_pwr_avg(struct bq27xxx_device_info *di)
> -{
> -	int tval;
> -
> -	tval = bq27xxx_read(di, BQ27XXX_REG_AP, false);
> -	if (tval < 0) {
> -		dev_err(di->dev, "error reading average power register  %02x: %d\n",
> -			BQ27XXX_REG_AP, tval);
> -		return tval;
> -	}
> -
> -	if (di->opts & BQ27XXX_O_ZERO)
> -		return (tval * BQ27XXX_POWER_CONSTANT) / BQ27XXX_RS;
> -	else
> -		return tval;
> -}
> -
>  /*
>   * Returns true if a battery over temperature condition is detected
>   */
> @@ -1769,8 +1748,6 @@ void bq27xxx_battery_update(struct bq27xxx_device_info *di)
>  		}
>  		if (di->regs[BQ27XXX_REG_CYCT] != INVALID_REG_ADDR)
>  			cache.cycle_count = bq27xxx_battery_read_cyct(di);
> -		if (di->regs[BQ27XXX_REG_AP] != INVALID_REG_ADDR)
> -			cache.power_avg = bq27xxx_battery_read_pwr_avg(di);
>  
>  		/* We only have to read charge design full once */
>  		if (di->charge_design_full <= 0)
> @@ -1833,6 +1810,32 @@ static int bq27xxx_battery_current(struct bq27xxx_device_info *di,
>  	return 0;
>  }
>  
> +/*
> + * Get the average power in µW
> + * Return < 0 if something fails.
> + */
> +static int bq27xxx_battery_pwr_avg(struct bq27xxx_device_info *di,
> +				   union power_supply_propval *val)
> +{
> +	int power;
> +
> +	power = bq27xxx_read(di, BQ27XXX_REG_AP, false);
> +	if (power < 0) {
> +		dev_err(di->dev,
> +			"error reading average power register %02x: %d\n",
> +			BQ27XXX_REG_AP, power);
> +		return power;
> +	}
> +
> +	if (di->opts & BQ27XXX_O_ZERO)
> +		val->intval = (power * BQ27XXX_POWER_CONSTANT) / BQ27XXX_RS;
> +	else
> +		/* Other gauges return a signed value in units of 10mW */
> +		val->intval = (int)((s16)power) * 10000;
> +
> +	return 0;
> +}
> +
>  static int bq27xxx_battery_status(struct bq27xxx_device_info *di,
>  				  union power_supply_propval *val)
>  {
> @@ -2020,7 +2023,7 @@ static int bq27xxx_battery_get_property(struct power_supply *psy,
>  		ret = bq27xxx_simple_value(di->cache.energy, val);
>  		break;
>  	case POWER_SUPPLY_PROP_POWER_AVG:
> -		ret = bq27xxx_simple_value(di->cache.power_avg, val);
> +		ret = bq27xxx_battery_pwr_avg(di, val);
>  		break;
>  	case POWER_SUPPLY_PROP_HEALTH:
>  		ret = bq27xxx_simple_value(di->cache.health, val);
> diff --git a/include/linux/power/bq27xxx_battery.h b/include/linux/power/bq27xxx_battery.h
> index 111a40d0d3d5..8d5f4f40fb41 100644
> --- a/include/linux/power/bq27xxx_battery.h
> +++ b/include/linux/power/bq27xxx_battery.h
> @@ -53,7 +53,6 @@ struct bq27xxx_reg_cache {
>  	int capacity;
>  	int energy;
>  	int flags;
> -	int power_avg;
>  	int health;
>  };
>