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

[Hermes Zhang]

On 2024/4/1 21:57, Andrew Davis wrote:
> On 4/1/24 8:15 AM, Sebastian Reichel wrote:
> > [+cc Andrew Davis]
> >
> > Hello Hermes,
> >
> > 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.
>
> The original BQ27000 device did not have an interrupt pin to signal
> when important updates to the battery occurred, so early devices
> using it would have userspace poll those values. As the kernel driver
> added its own polling for updates, it seems the early driver authors
> simply decided to cache the values between kernel reads and return
> those to userspace when it reads.
>
> This is a problem though for two reasons.
> 1. If no one is interested in these values the kernel will still poll
>    them anyway, wasting I2C bus time and power.
> 2. If userspace is actually interested in some value and so checks it
>    often, it will only get real updated values when the kernel next 
> polls,
>    which might not be often enough for some use-cases.
Agree! Also good to know the history.


> > 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.
>
> Preventing a DoS of the I2C bus is not the responsibility of a
> given driver. Userspace has plenty of other ways to spam the
> I2C bus if it really wants to, no need to try to predict what a
> system's admin would want and block users from those actions.
>
> If we really do want to reduce I2C accesses for registers we know
> cannot change often (which are few), then we should use the
> existing regmap_cache mechanism, not roll our own. This driver
> is complex enough without re-inventing the wheel and adding
> our own custom register caching scheme.
Yes, actually for our case, we have already reviewed the access from 
userspace and optimise it, then we found the driver also poll the 
registers quite offten and some of them are not really needed. Now if we 
think it's OK to remove some of the registers from the cache group, then 
it's great and much simple solution than mine. Thanks for the patches.


Best Regards,

Hermes