linux-kernel - Re: [PATCH] power: supply: sbs-battery: Handle unsupported PROP_TIME_TO_EMPTY

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]

Date: Fri, 8 Mar 2024 08:11:53 -0500
From: Nícolas F. R. A. Prado <nfraprado@...labora.com>
To: AngeloGioacchino Del Regno <angelogioacchino.delregno@...labora.com>
Cc: Sebastian Reichel <sre@...nel.org>, kernel@...labora.com,
	linux-pm@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] power: supply: sbs-battery: Handle unsupported
 PROP_TIME_TO_EMPTY_NOW

On Fri, Mar 08, 2024 at 10:26:29AM +0100, AngeloGioacchino Del Regno wrote:
> Il 07/03/24 23:05, Nícolas F. R. A. Prado ha scritto:
> > Despite the RunTimeToEmpty() (0x11) function being defined in the SBS
> > specification as required, it seems that not all batteries implement it.
> > On platforms with such batteries, reading the property will cause an
> > error to be printed:
> > 
> > power_supply sbs-8-000b: driver failed to report `time_to_empty_now' property: -5
> > 
> > This not only pollutes the log, distracting from real problems on the
> > device, but also prevents the uevent file from being read since it
> > contains all properties, including the faulty one.
> > 
> > The following table summarizes the findings for a handful of platforms:
> > 
> > Platform                                Status  Manufacturer    Model
> > ------------------------------------------------------------------------
> > mt8186-corsola-steelix-sku131072        OK      BYD             L22B3PG0
> > mt8195-cherry-tomato-r2                 NOT OK  PANASON         AP16L5J
> > mt8192-asurada-spherion-r0              NOT OK  PANASON         AP15O5L
> > mt8183-kukui-jacuzzi-juniper-sku16      NOT OK  LGC KT0         AP16L8J
> > mt8173-elm-hana                         OK      Sunwoda         L18D3PG1
> > sc7180-trogdor-lazor-limozeen-nots-r5   NOT OK  Murata          AP18C4K
> > sc7180-trogdor-kingoftown               NOT OK  333-AC-0D-A     GG02047XL
> > rk3399-gru-kevin                        OK      SDI             4352D51
> > 
> > Identify during probe, based on the battery model, if this is one of the
> > quirky batteries, and if so, don't register the
> > POWER_SUPPLY_PROP_TIME_TO_EMPTY_NOW property.
> > 
> > Signed-off-by: Nícolas F. R. A. Prado <nfraprado@...labora.com>
> > ---
> >   drivers/power/supply/sbs-battery.c | 45 ++++++++++++++++++++++++++++++++++++++
> >   1 file changed, 45 insertions(+)
> > 
> > diff --git a/drivers/power/supply/sbs-battery.c b/drivers/power/supply/sbs-battery.c
> > index a6c204c08232..85ff331cf87a 100644
> > --- a/drivers/power/supply/sbs-battery.c
> > +++ b/drivers/power/supply/sbs-battery.c
> > @@ -1112,6 +1112,49 @@ static const struct power_supply_desc sbs_default_desc = {
> >   	.external_power_changed = sbs_external_power_changed,
> >   };
> > +static const char * const unsupported_time_to_empty_now_models[] = {
> > +	"AP16L5J", "AP15O5L", "AP16L8J", "AP18C4K", "GG02047XL"
> > +};
> 
> I think that you must make sure that this is seen as a quirk, rather than
> "something normal" because - as you said - the SBS specification says that
> the TIME_TO_EMPTY_NOW is *required*, so, this is a *deviation* from the spec
> (so, the SBS implementation in those devices is *out of spec*!).
> 
> static const char * const quirk_remove_time_to_empty_now_models
> or                        quirk_unsupported_time_to_empty_now_models
> 
> ...the former, if you want to avoid having a variable name that is 5000 characters
> long (:-P); the latter, if you don't care about that (there's no rule anyway).

(and I just noticed I forgot the usual sbs_ prefix here, so it'll get a few more
characters ;P)

> 
> 
> Besides that, since I didn't like what I just saw, I looked for different
> alternatives; the only other one is to de-constify the sbs_properties[] array,
> which is something that I also dislike anyway.
> 
> I'm not sure if deconstifying that could be acceptable, but if it would, you
> would be able to remove-reorder without copies of this and that.

Personally I don't see an issue with creating a new struct and copying things
over - this will only happen during probe and for the quirky batteries anyway -
and it's nice to keep things const for the common case.

> 
> Anyway - the only thing I really want here is to make sure that this is seen
> as a quirk and a clear deviation from the specification - everything else is
> a plus, and not really a blocker for me.

Yep, and you're right on that. I'll make sure to slap the "quirk" sticker on the
variable and function for next version.

Thanks,
Nícolas