Re: [PATCH v2] power: supply: bq27xxx: Divide the reg cache to each register

From: Sebastian Reichel
Date: Mon Apr 01 2024 - 09:16:08 EST

Next message: Jarkko Sakkinen: "Re: [GIT PULL] tpmdd changes for v6.9-rc2"
Previous message: Shengjiu Wang: "[PATCH v2 2/2] ASoC: dt-bindings: fsl-asoc-card: convert to YAML"
Next in thread: Andrew Davis: "Re: [PATCH v2] power: supply: bq27xxx: Divide the reg cache to each register"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

[+cc Andrew Davis]

Hello Hermes,

On Wed, Mar 06, 2024 at 06:09:03PM +0800, Hermes Zhang wrote:
> The reg cache used to be grouped and activated for each property
> access, regardless of whether it was part of the group. That will
> lead to a significant increase in I2C transmission.
> Divide the cache group and create a cache for every register. The
> cache won't work until the register has been fetched. This will help
> in reducing the quantity of pointless I2C communication and avoiding
> the error -16 (EBUSY) that occurs while using an I2C bus that is
> shared by many devices.
>
> Signed-off-by: Hermes Zhang <Hermes.Zhang@xxxxxxxx>
> ---

Sorry for the delay. This arrived too close to the 6.9 merge window.
I had a look now and while the patch looks fine to me on a conceptual
level, it did not apply. It looks like you used a pre-2024 kernel tree
to generate the patch against. Please always use something recent base
tree (and ideally use git's --base option to document the used
parent commit).

Other than that I just applied a series from Andrew, which cleans up
the register caching in bq27xxx and removed most registers from the
cache. That's something I did not consider earlier, since I thought
the cache was introduced to fix a different issue. But that was
apparently sbs-battery and not bq27xxx.

Anyways, there is only two registers left in the cache now. I'm fine
with having a per-register cache for them, if that is still needed
to further reduce I2C traffic on your device.

And... re-reading your problem description, I wonder if we need to
reintroduce the caching for all registers (on a per register basis)
to avoid userspace being able to do a denial of service by quickly
polling the battery information.

Any thoughts?

-- Sebastian

> v2:
> - Refactor implementation.
>
> drivers/power/supply/bq27xxx_battery.c | 231 +++++++++++++++++--------
> include/linux/power/bq27xxx_battery.h | 30 ++--
> 2 files changed, 179 insertions(+), 82 deletions(-)
>
> diff --git a/drivers/power/supply/bq27xxx_battery.c b/drivers/power/supply/bq27xxx_battery.c
> index 1c4a9d137744..cc724322f4f0 100644
> --- a/drivers/power/supply/bq27xxx_battery.c
> +++ b/drivers/power/supply/bq27xxx_battery.c
> @@ -1746,14 +1746,16 @@ static bool bq27xxx_battery_capacity_inaccurate(struct bq27xxx_device_info *di,
>
> static int bq27xxx_battery_read_health(struct bq27xxx_device_info *di)
> {
> + int flags = di->cache[CACHE_REG_FLAGS].value;
> +
> /* Unlikely but important to return first */
> - if (unlikely(bq27xxx_battery_overtemp(di, di->cache.flags)))
> + if (unlikely(bq27xxx_battery_overtemp(di, flags)))
> return POWER_SUPPLY_HEALTH_OVERHEAT;
> - if (unlikely(bq27xxx_battery_undertemp(di, di->cache.flags)))
> + if (unlikely(bq27xxx_battery_undertemp(di, flags)))
> return POWER_SUPPLY_HEALTH_COLD;
> - if (unlikely(bq27xxx_battery_dead(di, di->cache.flags)))
> + if (unlikely(bq27xxx_battery_dead(di, flags)))
> return POWER_SUPPLY_HEALTH_DEAD;
> - if (unlikely(bq27xxx_battery_capacity_inaccurate(di, di->cache.flags)))
> + if (unlikely(bq27xxx_battery_capacity_inaccurate(di, flags)))
> return POWER_SUPPLY_HEALTH_CALIBRATION_REQUIRED;
>
> return POWER_SUPPLY_HEALTH_GOOD;
> @@ -1778,7 +1780,7 @@ static int bq27xxx_battery_current_and_status(
> struct bq27xxx_device_info *di,
> union power_supply_propval *val_curr,
> union power_supply_propval *val_status,
> - struct bq27xxx_reg_cache *cache)
> + struct bq27xxx_cache_reg *reg)
> {
> bool single_flags = (di->opts & BQ27XXX_O_ZERO);
> int curr;
> @@ -1790,8 +1792,8 @@ static int bq27xxx_battery_current_and_status(
> return curr;
> }
>
> - if (cache) {
> - flags = cache->flags;
> + if (reg) {
> + flags = reg->value;
> } else {
> flags = bq27xxx_read(di, BQ27XXX_REG_FLAGS, single_flags);
> if (flags < 0) {
> @@ -1832,57 +1834,128 @@ static int bq27xxx_battery_current_and_status(
> return 0;
> }
>
> -static void bq27xxx_battery_update_unlocked(struct bq27xxx_device_info *di)
> +static int bq27xxx_cached_reg_value_unlocked(struct bq27xxx_device_info *di,
> + enum bq27xxx_cache_registers item)
> {
> - union power_supply_propval status = di->last_status;
> - struct bq27xxx_reg_cache cache = {0, };
> - bool has_singe_flag = di->opts & BQ27XXX_O_ZERO;
> -
> - cache.flags = bq27xxx_read(di, BQ27XXX_REG_FLAGS, has_singe_flag);
> - if ((cache.flags & 0xff) == 0xff)
> - cache.flags = -1; /* read error */
> - if (cache.flags >= 0) {
> - cache.temperature = bq27xxx_battery_read_temperature(di);
> + struct bq27xxx_cache_reg *reg;
> + int tmp = -EINVAL;
> +
> + reg = &di->cache[item];
> +
> + if (time_is_after_jiffies(reg->last_update + 5 * HZ))
> + return reg->value;
> +
> + switch (item) {
> + case CACHE_REG_TEMPERATURE:
> + tmp = bq27xxx_battery_read_temperature(di);
> + break;
> + case CACHE_REG_TIME_TO_EMPTY:
> if (di->regs[BQ27XXX_REG_TTE] != INVALID_REG_ADDR)
> - cache.time_to_empty = bq27xxx_battery_read_time(di, BQ27XXX_REG_TTE);
> + tmp = bq27xxx_battery_read_time(di, BQ27XXX_REG_TTE);
> + break;
> + case CACHE_REG_TIME_TO_EMPTY_AVG:
> if (di->regs[BQ27XXX_REG_TTECP] != INVALID_REG_ADDR)
> - cache.time_to_empty_avg = bq27xxx_battery_read_time(di, BQ27XXX_REG_TTECP);
> + tmp = bq27xxx_battery_read_time(di, BQ27XXX_REG_TTECP);
> + break;
> + case CACHE_REG_TIME_TO_FULL:
> if (di->regs[BQ27XXX_REG_TTF] != INVALID_REG_ADDR)
> - cache.time_to_full = bq27xxx_battery_read_time(di, BQ27XXX_REG_TTF);
> -
> - cache.charge_full = bq27xxx_battery_read_fcc(di);
> - cache.capacity = bq27xxx_battery_read_soc(di);
> - if (di->regs[BQ27XXX_REG_AE] != INVALID_REG_ADDR)
> - cache.energy = bq27xxx_battery_read_energy(di);
> - di->cache.flags = cache.flags;
> - cache.health = bq27xxx_battery_read_health(di);
> + tmp = bq27xxx_battery_read_time(di, BQ27XXX_REG_TTF);
> + break;
> + case CACHE_REG_CHARGE_FULL:
> + tmp = bq27xxx_battery_read_fcc(di);
> + break;
> + case CACHE_REG_CYCLE_COUNT:
> if (di->regs[BQ27XXX_REG_CYCT] != INVALID_REG_ADDR)
> - cache.cycle_count = bq27xxx_battery_read_cyct(di);
> -
> - /*
> - * On gauges with signed current reporting the current must be
> - * checked to detect charging <-> discharging status changes.
> - */
> - if (!(di->opts & BQ27XXX_O_ZERO))
> - bq27xxx_battery_current_and_status(di, NULL, &status, &cache);
> -
> - /* We only have to read charge design full once */
> - if (di->charge_design_full <= 0)
> - di->charge_design_full = bq27xxx_battery_read_dcap(di);
> + tmp = bq27xxx_battery_read_cyct(di);
> + break;
> + case CACHE_REG_CAPACITY:
> + tmp = bq27xxx_battery_read_soc(di);
> + break;
> + case CACHE_REG_ENERGY:
> + if (di->regs[BQ27XXX_REG_AE] != INVALID_REG_ADDR)
> + tmp = bq27xxx_battery_read_energy(di);
> + break;
> + case CACHE_REG_FLAGS:
> + bool has_singe_flag = di->opts & BQ27XXX_O_ZERO;
> +
> + tmp = bq27xxx_read(di, BQ27XXX_REG_FLAGS, has_singe_flag);
> + if ((tmp & 0xff) == 0xff)
> + tmp = -1; /* read error */
> + break;
> + default:
> + break;
> + }
> +
> + /* only update cache value when successful */
> + if (tmp >= 0) {
> + reg->value = tmp;
> + reg->last_update = jiffies;
> }
>
> - if ((di->cache.capacity != cache.capacity) ||
> - (di->cache.flags != cache.flags) ||
> + return tmp;
> +}
> +
> +static int bq27xxx_cached_reg_value(struct bq27xxx_device_info *di,
> + enum bq27xxx_cache_registers item)
> +{
> + int ret;
> +
> + mutex_lock(&di->lock);
> + ret = bq27xxx_cached_reg_value_unlocked(di, item);
> + mutex_unlock(&di->lock);
> +
> + return ret;
> +}
> +
> +static void bq27xxx_battery_update_unlocked(struct bq27xxx_device_info *di)
> +{
> + union power_supply_propval status = di->last_status;
> + int old_flags, flags;
> + int old_capacity, capacity;
> +
> + old_capacity = di->cache[CACHE_REG_CAPACITY].value;
> + capacity = old_capacity;
> +
> + old_flags = di->cache[CACHE_REG_FLAGS].value;
> + flags = bq27xxx_cached_reg_value_unlocked(di, CACHE_REG_FLAGS);
> +
> + if (flags < 0)
> + goto out;
> +
> + bq27xxx_cached_reg_value_unlocked(di, CACHE_REG_TEMPERATURE);
> + if (di->regs[BQ27XXX_REG_TTE] != INVALID_REG_ADDR)
> + bq27xxx_cached_reg_value_unlocked(di, CACHE_REG_TIME_TO_EMPTY);
> + if (di->regs[BQ27XXX_REG_TTECP] != INVALID_REG_ADDR)
> + bq27xxx_cached_reg_value_unlocked(di, CACHE_REG_TIME_TO_EMPTY_AVG);
> + if (di->regs[BQ27XXX_REG_TTF] != INVALID_REG_ADDR)
> + bq27xxx_cached_reg_value_unlocked(di, CACHE_REG_TIME_TO_FULL);
> +
> + bq27xxx_cached_reg_value_unlocked(di, CACHE_REG_CHARGE_FULL);
> + bq27xxx_cached_reg_value_unlocked(di, CACHE_REG_CAPACITY);
> + if (di->regs[BQ27XXX_REG_AE] != INVALID_REG_ADDR)
> + bq27xxx_cached_reg_value_unlocked(di, CACHE_REG_ENERGY);
> + if (di->regs[BQ27XXX_REG_CYCT] != INVALID_REG_ADDR)
> + bq27xxx_cached_reg_value_unlocked(di, CACHE_REG_CYCLE_COUNT);
> +
> + /*
> + * On gauges with signed current reporting the current must be
> + * checked to detect charging <-> discharging status changes.
> + */
> + if (!(di->opts & BQ27XXX_O_ZERO))
> + bq27xxx_battery_current_and_status(di, NULL, &status,
> + &di->cache[CACHE_REG_FLAGS]);
> +
> + /* We only have to read charge design full once */
> + if (di->charge_design_full <= 0)
> + di->charge_design_full = bq27xxx_battery_read_dcap(di);
> +
> +out:
> + if ((old_capacity != capacity) || (old_flags != flags) ||
> (di->last_status.intval != status.intval)) {
> di->last_status.intval = status.intval;
> power_supply_changed(di->bat);
> }
>
> - if (memcmp(&di->cache, &cache, sizeof(cache)) != 0)
> - di->cache = cache;
> -
> - di->last_update = jiffies;
> -
> if (!di->removed && poll_interval > 0)
> mod_delayed_work(system_wq, &di->work, poll_interval * HZ);
> }
> @@ -1934,29 +2007,32 @@ static int bq27xxx_battery_capacity_level(struct bq27xxx_device_info *di,
> union power_supply_propval *val)
> {
> int level;
> + int flags;
> +
> + flags = di->cache[CACHE_REG_FLAGS].value;
>
> if (di->opts & BQ27XXX_O_ZERO) {
> - if (di->cache.flags & BQ27000_FLAG_FC)
> + if (flags & BQ27000_FLAG_FC)
> level = POWER_SUPPLY_CAPACITY_LEVEL_FULL;
> - else if (di->cache.flags & BQ27000_FLAG_EDVF)
> + else if (flags & BQ27000_FLAG_EDVF)
> level = POWER_SUPPLY_CAPACITY_LEVEL_CRITICAL;
> - else if (di->cache.flags & BQ27000_FLAG_EDV1)
> + else if (flags & BQ27000_FLAG_EDV1)
> level = POWER_SUPPLY_CAPACITY_LEVEL_LOW;
> else
> level = POWER_SUPPLY_CAPACITY_LEVEL_NORMAL;
> } else if (di->opts & BQ27Z561_O_BITS) {
> - if (di->cache.flags & BQ27Z561_FLAG_FC)
> + if (flags & BQ27Z561_FLAG_FC)
> level = POWER_SUPPLY_CAPACITY_LEVEL_FULL;
> - else if (di->cache.flags & BQ27Z561_FLAG_FDC)
> + else if (flags & BQ27Z561_FLAG_FDC)
> level = POWER_SUPPLY_CAPACITY_LEVEL_CRITICAL;
> else
> level = POWER_SUPPLY_CAPACITY_LEVEL_NORMAL;
> } else {
> - if (di->cache.flags & BQ27XXX_FLAG_FC)
> + if (flags & BQ27XXX_FLAG_FC)
> level = POWER_SUPPLY_CAPACITY_LEVEL_FULL;
> - else if (di->cache.flags & BQ27XXX_FLAG_SOCF)
> + else if (flags & BQ27XXX_FLAG_SOCF)
> level = POWER_SUPPLY_CAPACITY_LEVEL_CRITICAL;
> - else if (di->cache.flags & BQ27XXX_FLAG_SOC1)
> + else if (flags & BQ27XXX_FLAG_SOC1)
> level = POWER_SUPPLY_CAPACITY_LEVEL_LOW;
> else
> level = POWER_SUPPLY_CAPACITY_LEVEL_NORMAL;
> @@ -2004,13 +2080,12 @@ static int bq27xxx_battery_get_property(struct power_supply *psy,
> {
> int ret = 0;
> struct bq27xxx_device_info *di = power_supply_get_drvdata(psy);
> + int flags;
> + int cache;
>
> - mutex_lock(&di->lock);
> - if (time_is_before_jiffies(di->last_update + 5 * HZ))
> - bq27xxx_battery_update_unlocked(di);
> - mutex_unlock(&di->lock);
> + flags = bq27xxx_cached_reg_value(di, CACHE_REG_FLAGS);
>
> - if (psp != POWER_SUPPLY_PROP_PRESENT && di->cache.flags < 0)
> + if (psp != POWER_SUPPLY_PROP_PRESENT && flags < 0)
> return -ENODEV;
>
> switch (psp) {
> @@ -2021,30 +2096,40 @@ static int bq27xxx_battery_get_property(struct power_supply *psy,
> ret = bq27xxx_battery_voltage(di, val);
> break;
> case POWER_SUPPLY_PROP_PRESENT:
> - val->intval = di->cache.flags < 0 ? 0 : 1;
> + val->intval = flags < 0 ? 0 : 1;
> break;
> case POWER_SUPPLY_PROP_CURRENT_NOW:
> ret = bq27xxx_battery_current_and_status(di, val, NULL, NULL);
> break;
> case POWER_SUPPLY_PROP_CAPACITY:
> - ret = bq27xxx_simple_value(di->cache.capacity, val);
> + cache = bq27xxx_cached_reg_value(di, CACHE_REG_CAPACITY);
> +
> + ret = bq27xxx_simple_value(cache, val);
> break;
> case POWER_SUPPLY_PROP_CAPACITY_LEVEL:
> ret = bq27xxx_battery_capacity_level(di, val);
> break;
> case POWER_SUPPLY_PROP_TEMP:
> - ret = bq27xxx_simple_value(di->cache.temperature, val);
> + cache = bq27xxx_cached_reg_value(di, CACHE_REG_TEMPERATURE);
> +
> + ret = bq27xxx_simple_value(cache, val);
> if (ret == 0)
> val->intval -= 2731; /* convert decidegree k to c */
> break;
> case POWER_SUPPLY_PROP_TIME_TO_EMPTY_NOW:
> - ret = bq27xxx_simple_value(di->cache.time_to_empty, val);
> + cache = bq27xxx_cached_reg_value(di, CACHE_REG_TIME_TO_EMPTY);
> +
> + ret = bq27xxx_simple_value(cache, val);
> break;
> case POWER_SUPPLY_PROP_TIME_TO_EMPTY_AVG:
> - ret = bq27xxx_simple_value(di->cache.time_to_empty_avg, val);
> + cache = bq27xxx_cached_reg_value(di, CACHE_REG_TIME_TO_EMPTY_AVG);
> +
> + ret = bq27xxx_simple_value(cache, val);
> break;
> case POWER_SUPPLY_PROP_TIME_TO_FULL_NOW:
> - ret = bq27xxx_simple_value(di->cache.time_to_full, val);
> + cache = bq27xxx_cached_reg_value(di, CACHE_REG_TIME_TO_FULL);
> +
> + ret = bq27xxx_simple_value(cache, val);
> break;
> case POWER_SUPPLY_PROP_TECHNOLOGY:
> if (di->opts & BQ27XXX_O_MUL_CHEM)
> @@ -2059,7 +2144,9 @@ static int bq27xxx_battery_get_property(struct power_supply *psy,
> ret = bq27xxx_simple_value(bq27xxx_battery_read_rc(di), val);
> break;
> case POWER_SUPPLY_PROP_CHARGE_FULL:
> - ret = bq27xxx_simple_value(di->cache.charge_full, val);
> + cache = bq27xxx_cached_reg_value(di, CACHE_REG_CHARGE_FULL);
> +
> + ret = bq27xxx_simple_value(cache, val);
> break;
> case POWER_SUPPLY_PROP_CHARGE_FULL_DESIGN:
> ret = bq27xxx_simple_value(di->charge_design_full, val);
> @@ -2072,16 +2159,22 @@ static int bq27xxx_battery_get_property(struct power_supply *psy,
> case POWER_SUPPLY_PROP_VOLTAGE_MIN_DESIGN:
> return -EINVAL;
> case POWER_SUPPLY_PROP_CYCLE_COUNT:
> - ret = bq27xxx_simple_value(di->cache.cycle_count, val);
> + cache = bq27xxx_cached_reg_value(di, CACHE_REG_CYCLE_COUNT);
> +
> + ret = bq27xxx_simple_value(cache, val);
> break;
> case POWER_SUPPLY_PROP_ENERGY_NOW:
> - ret = bq27xxx_simple_value(di->cache.energy, val);
> + cache = bq27xxx_cached_reg_value(di, CACHE_REG_ENERGY);
> +
> + ret = bq27xxx_simple_value(cache, val);
> break;
> case POWER_SUPPLY_PROP_POWER_AVG:
> ret = bq27xxx_battery_pwr_avg(di, val);
> break;
> case POWER_SUPPLY_PROP_HEALTH:
> - ret = bq27xxx_simple_value(di->cache.health, val);
> + cache = bq27xxx_battery_read_health(di);
> +
> + ret = bq27xxx_simple_value(cache, val);
> break;
> case POWER_SUPPLY_PROP_MANUFACTURER:
> val->strval = BQ27XXX_MANUFACTURER;
> diff --git a/include/linux/power/bq27xxx_battery.h b/include/linux/power/bq27xxx_battery.h
> index 7d8025fb74b7..617c8409d80f 100644
> --- a/include/linux/power/bq27xxx_battery.h
> +++ b/include/linux/power/bq27xxx_battery.h
> @@ -46,17 +46,22 @@ struct bq27xxx_access_methods {
> int (*write_bulk)(struct bq27xxx_device_info *di, u8 reg, u8 *data, int len);
> };
>
> -struct bq27xxx_reg_cache {
> - int temperature;
> - int time_to_empty;
> - int time_to_empty_avg;
> - int time_to_full;
> - int charge_full;
> - int cycle_count;
> - int capacity;
> - int energy;
> - int flags;
> - int health;
> +struct bq27xxx_cache_reg {
> + int value;
> + unsigned long last_update;
> +};
> +
> +enum bq27xxx_cache_registers {
> + CACHE_REG_TEMPERATURE = 0,
> + CACHE_REG_TIME_TO_EMPTY,
> + CACHE_REG_TIME_TO_EMPTY_AVG,
> + CACHE_REG_TIME_TO_FULL,
> + CACHE_REG_CHARGE_FULL,
> + CACHE_REG_CYCLE_COUNT,
> + CACHE_REG_CAPACITY,
> + CACHE_REG_ENERGY,
> + CACHE_REG_FLAGS,
> + CACHE_REG_MAX,
> };
>
> struct bq27xxx_device_info {
> @@ -68,10 +73,9 @@ struct bq27xxx_device_info {
> struct bq27xxx_dm_reg *dm_regs;
> u32 unseal_key;
> struct bq27xxx_access_methods bus;
> - struct bq27xxx_reg_cache cache;
> + struct bq27xxx_cache_reg cache[CACHE_REG_MAX];
> int charge_design_full;
> bool removed;
> - unsigned long last_update;
> union power_supply_propval last_status;
> struct delayed_work work;
> struct power_supply *bat;
> --
> 2.39.2
>

Attachment: signature.asc
Description: PGP signature

Next message: Jarkko Sakkinen: "Re: [GIT PULL] tpmdd changes for v6.9-rc2"
Previous message: Shengjiu Wang: "[PATCH v2 2/2] ASoC: dt-bindings: fsl-asoc-card: convert to YAML"
Next in thread: Andrew Davis: "Re: [PATCH v2] power: supply: bq27xxx: Divide the reg cache to each register"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]