linux-kernel - Re: [PATCH 5/5] power: supply: bq27xxx: Correct supply status with current draw

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] [thread-next>] [day] [month] [year] [list]
Message-ID: <1496819733.927.16.camel@paulk.fr>
Date:   Wed, 07 Jun 2017 10:15:33 +0300
From:   Paul Kocialkowski <contact@...lk.fr>
To:     Pavel Machek <pavel@....cz>
Cc:     linux-pm@...r.kernel.org, linux-kernel@...r.kernel.org,
        Pali Rohár <pali.rohar@...il.com>,
        "Andrew F . Davis" <afd@...com>,
        Sebastian Reichel <sre@...nel.org>,
        Chris Lapa <chris@...a.com.au>,
        Matt Ranostay <mranostay@...il.com>
Subject: Re: [PATCH 5/5] power: supply: bq27xxx: Correct supply status with
 current draw

Le mercredi 31 mai 2017 à 21:28 +0200, Paul Kocialkowski a écrit :
> Hi,
> 
> Le mercredi 31 mai 2017 à 19:32 +0200, Pavel Machek a écrit :
> > The status reported directly by the battery controller is not always
> > > > > reliable and should be corrected based on the current draw
> > > > > information.
> > > > > 
> > > > > This implements such a correction with a dedicated function, called
> > > > > when retrieving the supply status.
> > > > > @@ -1182,6 +1196,8 @@ static int bq27xxx_battery_status(struct
> > > > > bq27xxx_device_info *di,
> > > > >  		else
> > > > >  			status = POWER_SUPPLY_STATUS_DISCHARGING;
> > > > >  	} else {
> > > > > +		curr = (int)((s16)curr) * 1000;
> > > > 
> > > > Umm.
> > 
> > As in "two casts in one expression -- too ugly to live".
> 
> Oh, I had skipped that comment, sorry about that. Yeah I understand your
> concern. However, this line was mostly inspired by another part of the code,
> below the following comment: /* Other gauges return signed value */
> 
> I think we should fix the first occurence first and then used the fixed syntax
> in v2 of this patch. What do you think?
> 
> > > > > @@ -1190,6 +1206,18 @@ static int bq27xxx_battery_status(struct
> > > > > bq27xxx_device_info *di,
> > > > >  			status = POWER_SUPPLY_STATUS_CHARGING;
> > > > >  	}
> > > > >  
> > > > > +
> > > > > +	if (curr == 0 && status != POWER_SUPPLY_STATUS_NOT_CHARGING)
> > > > > +		status = POWER_SUPPLY_STATUS_FULL;
> > > > > +
> > > > > +	if (status == POWER_SUPPLY_STATUS_FULL) {
> > > > > +		/* Drawing or providing current when full */
> > > > > +		if (curr > 0)
> > > > > +			status = POWER_SUPPLY_STATUS_CHARGING;
> > > > > +		else if (curr < 0)
> > > > > +			status = POWER_SUPPLY_STATUS_DISCHARGING;
> > > > > +	}
> > > > 
> > > > Are you sure this works? On N900, we normally see small currents to/from
> > > > "full" battery.
> > > 
> > > In my case, this works perfectly and I am quite surprised of what you're
> > > describing. Is it the case when the battery has a PSU connected?
> > 
> > "PSU"? This is cellphone. It has USB connection and charges from that.
> > 
> > It has been charging for long while now, and current_now fluctuates
> > between 20706 and -2856. USB has limitted current, so I guess "draw
> > current from battery if we need more than USB can provide" is quite common.
> 
> Ah right, I had forgotten about the USB current limitation thing. In this
> case,
> I guess the battery is never actually full and IMO, it should be reported as
> such.
> 
> > pavel@...0:~$ cat /sys/class/power_supply/bq27200-0/current_now
> > 5355
> > pavel@...0:~$ cat /sys/class/power_supply/bq27200-0/current_now
> > 5355
> > pavel@...0:~$ cat /sys/class/power_supply/bq27200-0/current_now
> > -4105
> > pavel@...0:~$ cat /sys/class/power_supply/bq27200-0/current_now
> > -4105
> > pavel@...0:~$ cat /sys/class/power_supply/bq27200-0/current_now
> > -7675
> > pavel@...0:~$ cat /sys/class/power_supply/bq27200-0/current_now
> > -5712
> > pavel@...0:~$ #screen on
> > pavel@...0:~$ cat /sys/class/power_supply/bq27200-0/current_now
> > 4641
> > pavel@...0:~$ cat /sys/class/power_supply/bq27200-0/current_now
> > 4641
> > pavel@...0:~$ cat /sys/class/power_supply/bq27200-0/current_now
> > 37842
> > pavel@...0:~$ cat /sys/class/power_supply/bq27200-0/current_now
> > 16600
> > pavel@...0:~$
> > 
> > > I guess I would consider this a hardware issue (leak currents) and we
> > > could
> > > definitely set some range (in device-tree) to distinguish between full +
> > > leak
> > > currents and bad reporting from the fuel gauge. That would work well in my
> > > case
> > > too.
> > 
> > I'd pass to userspace what the controller reports. Yes, I seldom see
> > "STATUS_FULL" but that may be a problem we need to track down.
> 
> The controller is known, from my experience, to not be reliable in that
> regard,
> so I don't think it makes sense to pass a state that doesn't reflect the
> actual
> state of charging just because the chip tells us so.
> 
> Worst case, we could also have a dt property to enable that kind of fixup
> workaround and let every device maintainer decide whether it is relevant for
> their device.

Actually, since a similar fix[0] was accepted in sbs-battery, I'd rather not
make this optional but rather make it the default and perhaps have a dt prop to
disable it.

> What do you think?

[0]: https://git.kernel.org/pub/scm/linux/kernel/git/torvalds/linux.git/commit/?
h=v4.12-rc4&id=7f93e1fa032bb5ee19b868b9649bc98c82553003
-- 
Paul Kocialkowski, developer of free digital technology and hardware support

Website: https://www.paulk.fr/
Coding blog: https://code.paulk.fr/
Git repositories: https://git.paulk.fr/ https://git.code.paulk.fr/
Download attachment "signature.asc" of type "application/pgp-signature" (834 bytes)