linux-kernel - Re: [PATCHv1 02/11] power: supply: core: auto-exposure of simple-battery data

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: <1fdf00a0-4830-465a-801c-147472fdcd22@mercury.local>
Date:   Sun, 12 Mar 2023 18:07:42 +0100
From:   Sebastian Reichel <sre@...nel.org>
To:     Linus Walleij <linus.walleij@...aro.org>
Cc:     Matti Vaittinen <mazziesaccount@...il.com>,
        Rob Herring <robh+dt@...nel.org>,
        Krzysztof Kozlowski <krzysztof.kozlowski+dt@...aro.org>,
        linux-kernel@...r.kernel.org, linux-pm@...r.kernel.org,
        devicetree@...r.kernel.org
Subject: Re: [PATCHv1 02/11] power: supply: core: auto-exposure of
 simple-battery data

Hi,

On Fri, Mar 10, 2023 at 09:20:09AM +0100, Linus Walleij wrote:
> On Thu, Mar 9, 2023 at 11:50 PM Sebastian Reichel <sre@...nel.org> wrote:
> 
> > +       /*
> > +        * Set if constant battery information from firmware should be
> > +        * exposed automatically. No driver specific code is required
> > +        * in that case. If the driver also handles a property provided
> > +        * by constant firmware data, the driver's handler is preferred.
> > +        */
> > +       bool expose_battery_info;
> 
> Playing it safe with opt-in I see! But I would probably invert it and
> add a hide_battery_info for those that don't wanna expose it. It seems
> pretty useful to just expose this in general.

I just did not yet spend the time to understand if there are any
issues. I guess I can do it now and then remove the opt-in part.

> However I have no insight in what happens on laptops etc for this
> so I guess you have your reasons, either way:

ACPI based systems should be fine, since battery info does not
yet support ACPI and thus nothing changes for them.

> Reviewed-by: Linus Walleij <linus.walleij@...aro.org>
> 
> > +extern bool power_supply_battery_info_has_prop(struct power_supply_battery_info *info,
> > +                                              enum power_supply_property psp);
> > +extern int power_supply_battery_info_get_prop(struct power_supply_battery_info *info,
> > +                                             enum power_supply_property psp,
> > +                                             union power_supply_propval *val);
> 
> I think the build robots complain because you need to add some stubs
> for the not enabled case.

I don't think so. They are only used from code needing POWER_SUPPLY
being enabled.

One reported error is about the array of battery_info properties not
being used when POWER_SUPPLY is disabled. I will move that array to a
better place.

The other error is about power_supply_get_property(), because I
accidently removed the EXPORT_SYMBOL_GPL(power_supply_get_property).
I did not notice myself, because I compiled a monolithic kernel for
the thermal camera for easy deployment.

Thanks for the review, much appreciated!

-- Sebastian

Download attachment "signature.asc" of type "application/pgp-signature" (834 bytes)