linux-kernel - Re: [PATCH] power: supply: bq27xxx: Return the value instead of -ENODATA

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <20210401085024.r3u26yftibbxvhnr@pali>
Date:   Thu, 1 Apr 2021 10:50:24 +0200
From:   Pali Rohár <pali@...nel.org>
To:     Hermes Zhang <Hermes.Zhang@...s.com>
Cc:     "lars@...afoo.de" <lars@...afoo.de>,
        Sebastian Reichel <sre@...nel.org>, kernel <kernel@...s.com>,
        "linux-pm@...r.kernel.org" <linux-pm@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] power: supply: bq27xxx: Return the value instead of
 -ENODATA

On Thursday 01 April 2021 01:51:28 Hermes Zhang wrote:
> On 3/31/21 10:02 PM, Pali Rohár wrote:
> > @@ -1655,9 +1655,6 @@ static int bq27xxx_battery_read_time(struct bq27xxx_device_info *di, u8 reg)
> >  		return tval;
> >  	}
> >  
> > -	if (tval == 65535)
> > -		return -ENODATA;
> > -
> >  	return tval * 60;
> > I'm not sure if this is correct change. If value 65535 is special which
> > indicates that data are not available then driver should not return
> > (converted) value 65535*60. If -ENODATA is there to indicate that data
> > are not available then -ENODATA should not be used.
> >
> > And if there is application which does not handle -ENODATA for state
> > when data are not available then it is a bug in application.
> 
> Yeah, I just have a feeling return -ENODATA for time_to_full/empty is
> not good here. Because:
> 
> 1. From chip datasheet, it mentioned return 65535 when it's not
> available (e.g. read time_to_full when discharging), but the driver
> changes behavior here.

So if chip reports that value is not available (by special value 65535)
then driver should not report that data are available with value 3932100s.

> 2. There is other case will return -ENODATA (e.g. the gauge not
> calibrated), so it will confuse application which is real failure.

In both cases, driver does not have (meaningful) value, so it reports
-ENODATA. If you think that some other error code is better for 2. then
we can discuss about it.

But -ENODATA seems a correct error code for reporting when data are not
available.

> Could we change the value in minute instead of seconds in
> bq27xxx_battery_read_time(), so that means driver do nothing but only
> pass the value from the chip?

No, according to API, driver must report value in seconds, not in
minutes, see documentation: Documentation/power/power_supply_class.rst

  All voltages, currents, charges, energies, time and temperatures in µV,
  µA, µAh, µWh, seconds and tenths of degree Celsius unless otherwise
  stated. It's driver's job to convert its raw values to units in which
  this class operates.

There is no API for reporting value in minutes.