power: supply: bq27xxx_battery: do not update cached flags prematurely

[PATCH] power: supply: bq27xxx_battery: do not update cached flags prematurely

Posted by Sicelo A. Mhlongo 1 year, 2 months ago

Commit 243f8ffc883a1 ("power: supply: bq27xxx_battery: Notify also about
status changes") intended to notify userspace when the status changes,
based on the flags register. However, the cached state is updated too
early, before the flags are tested for any changes. Remove the premature
update.

Fixes: 243f8ffc883a1 ("power: supply: bq27xxx_battery: Notify also about status changes")
Signed-off-by: Sicelo A. Mhlongo <absicsz@gmail.com>
---
 drivers/power/supply/bq27xxx_battery.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/power/supply/bq27xxx_battery.c b/drivers/power/supply/bq27xxx_battery.c
index 40c5ac7a1118..b2c65fe43d5c 100644
--- a/drivers/power/supply/bq27xxx_battery.c
+++ b/drivers/power/supply/bq27xxx_battery.c
@@ -1913,7 +1913,6 @@ static void bq27xxx_battery_update_unlocked(struct bq27xxx_device_info *di)
 		cache.flags = -1; /* read error */
 	if (cache.flags >= 0) {
 		cache.capacity = bq27xxx_battery_read_soc(di);
-		di->cache.flags = cache.flags;
 
 		/*
 		 * On gauges with signed current reporting the current must be
-- 
2.45.2

Re: [PATCH] power: supply: bq27xxx_battery: do not update cached flags prematurely

Posted by Sebastian Reichel 11 months, 3 weeks ago

From: Sebastian Reichel <sebastian.reichel@collabora.com>


On Mon, 25 Nov 2024 17:29:30 +0200, Sicelo A. Mhlongo wrote:
> Commit 243f8ffc883a1 ("power: supply: bq27xxx_battery: Notify also about
> status changes") intended to notify userspace when the status changes,
> based on the flags register. However, the cached state is updated too
> early, before the flags are tested for any changes. Remove the premature
> update.
> 
> 
> [...]

Applied, thanks!

[1/1] power: supply: bq27xxx_battery: do not update cached flags prematurely
      commit: 45291874a762dbb12a619dc2efaf84598859007a

Best regards,
-- 
Sebastian Reichel <sebastian.reichel@collabora.com>

Re: [PATCH] power: supply: bq27xxx_battery: do not update cached flags prematurely

Posted by Pali Rohár 1 year, 2 months ago

cc Krzysztof, author of the mentioned commit.

On Monday 25 November 2024 17:29:30 Sicelo A. Mhlongo wrote:
> Commit 243f8ffc883a1 ("power: supply: bq27xxx_battery: Notify also about
> status changes") intended to notify userspace when the status changes,
> based on the flags register. However, the cached state is updated too
> early, before the flags are tested for any changes. Remove the premature
> update.
> 
> Fixes: 243f8ffc883a1 ("power: supply: bq27xxx_battery: Notify also about status changes")
> Signed-off-by: Sicelo A. Mhlongo <absicsz@gmail.com>
> ---
>  drivers/power/supply/bq27xxx_battery.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/drivers/power/supply/bq27xxx_battery.c b/drivers/power/supply/bq27xxx_battery.c
> index 40c5ac7a1118..b2c65fe43d5c 100644
> --- a/drivers/power/supply/bq27xxx_battery.c
> +++ b/drivers/power/supply/bq27xxx_battery.c
> @@ -1913,7 +1913,6 @@ static void bq27xxx_battery_update_unlocked(struct bq27xxx_device_info *di)
>  		cache.flags = -1; /* read error */
>  	if (cache.flags >= 0) {
>  		cache.capacity = bq27xxx_battery_read_soc(di);
> -		di->cache.flags = cache.flags;
>  
>  		/*
>  		 * On gauges with signed current reporting the current must be
> -- 
> 2.45.2
>

Re: [PATCH] power: supply: bq27xxx_battery: do not update cached flags prematurely

Posted by Sicelo 1 year, 1 month ago

Hi 

On Mon, Nov 25, 2024 at 06:59:57PM +0100, Pali Rohár wrote:
> cc Krzysztof, author of the mentioned commit.

Yes, apologies for overlooking to cc him.

> On Monday 25 November 2024 17:29:30 Sicelo A. Mhlongo wrote:
> > Commit 243f8ffc883a1 ("power: supply: bq27xxx_battery: Notify also about
> > status changes") intended to notify userspace when the status changes,
> > based on the flags register. However, the cached state is updated too
> > early, before the flags are tested for any changes. Remove the premature
> > update.
> > 
> > Fixes: 243f8ffc883a1 ("power: supply: bq27xxx_battery: Notify also about status changes")
> > Signed-off-by: Sicelo A. Mhlongo <absicsz@gmail.com>
> > ---
> >  drivers/power/supply/bq27xxx_battery.c | 1 -
> >  1 file changed, 1 deletion(-)
> > 
> > diff --git a/drivers/power/supply/bq27xxx_battery.c b/drivers/power/supply/bq27xxx_battery.c
> > index 40c5ac7a1118..b2c65fe43d5c 100644
> > --- a/drivers/power/supply/bq27xxx_battery.c
> > +++ b/drivers/power/supply/bq27xxx_battery.c
> > @@ -1913,7 +1913,6 @@ static void bq27xxx_battery_update_unlocked(struct bq27xxx_device_info *di)
> >  		cache.flags = -1; /* read error */
> >  	if (cache.flags >= 0) {
> >  		cache.capacity = bq27xxx_battery_read_soc(di);
> > -		di->cache.flags = cache.flags;
> >  
> >  		/*
> >  		 * On gauges with signed current reporting the current must be
> > -- 
> > 2.45.2

Is there anything that still needs to be done to move this patch along?

Perhaps for clarification, in the middle of bq27xxx_battery_update_unlocked,
there is:

	if ((di->cache.capacity != cache.capacity) ||
	    (di->cache.flags != cache.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;

which compares the new value of cache.flags with the previously cached
values in di->cache, and calls power_supply_changed() if they are
different. For the flags, this test is currently not working, because by
the time it is run, di->cache.flags has already been updated with the
value in cache.flags, so the flags condition is always short-circuited
in the test. Hence my patch ensures di->cache.flags is not updated
before the test.

The flags are correctly updated after the test, in the 

        di->cache = cache;

line, so the removed line is simply not needed.

If there is any adjustment necessary, I will happily do so.

Kind Regards
Sicelo A. Mhlongo