Messages in this thread | | | Date | Mon, 1 Apr 2024 15:15:54 +0200 | From | Sebastian Reichel <> | Subject | Re: [PATCH v2] power: supply: bq27xxx: Divide the reg cache to each register |
| |
[+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@axis.com> > ---
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 > [unhandled content-type:application/pgp-signature] | |