linux-kernel - Re: [PATCH] regulator: core: Clean enabling always-on regulators + their supplies

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: <20181207013101.GA27539@basecamp>
Date:   Thu, 6 Dec 2018 20:31:01 -0500
From:   Brian Masney <masneyb@...tation.org>
To:     Douglas Anderson <dianders@...omium.org>
Cc:     Mark Brown <broonie@...nel.org>, linux-arm-msm@...r.kernel.org,
        Dmitry Osipenko <digetx@...il.com>, evgreen@...omium.org,
        Liam Girdwood <lgirdwood@...il.com>,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH] regulator: core: Clean enabling always-on regulators +
 their supplies

On Thu, Dec 06, 2018 at 02:23:18PM -0800, Douglas Anderson wrote:
> At the end of regulator_resolve_supply() we have historically turned
> on our supply in some cases.  This could be for one of two reasons:
> 
> 1. If resolving supplies was happening before the call to
>    set_machine_constraints() we needed to predict if
>    set_machine_constraints() was going to turn the regulator on and we
>    needed to preemptively turn the supply on.
> 2. Maybe set_machine_constraints() happened before we could resolve
>    supplies (because we failed the first time to resolve) and thus we
>    might need to propagate an enable that already happened up to our
>    supply.
> 
> Historically regulator_resolve_supply() used _regulator_is_enabled()
> to decide whether to turn on the supply.
> 
> Let's change things a little bit.  Specifically:
> 
> 1. Let's try to enable the supply and the regulator in the same place,
>    both in set_machine_constraints().  This means that we have exactly
>    the same logic for enabling the supply and the regulator.
> 2. Let's properly set use_count when we enable always-on or boot-on
>    regulators even for those that don't have supplies.  The previous
>    commit 1fc12b05895e ("regulator: core: Avoid propagating to
>    supplies when possible") only did this right for regulators with
>    supplies.
> 3. Let's make it clear that the only time we need to enable the supply
>    in regulator_resolve_supply() is if the main regulator is currently
>    in use.  By using use_count (like the rest of the code) to decide
>    if we're going to enable our supply we keep everything consistent.
> 
> Overall the new scheme should be cleaner and easier to reason about.
> In addition to fixing regulator_summary to be more correct (because of
> the more correct use_count), this change also has the effect of no
> longer using _regulator_is_enabled() in this code path.
> _regulator_is_enabled() could return an error code for some regulators
> at bootup (like RPMh) that can't read their initial state.  While one
> can argue that the design of those regulators is sub-optimal, the new
> logic sidesteps this brokenness.  This fix in particular fixes
> observed problems on Qualcomm sdm845 boards which use the
> above-mentioned RPMh regulator.  Those problems were made worse by
> commit 1fc12b05895e ("regulator: core: Avoid propagating to supplies
> when possible") because now we'd think at bootup that the SD
> regulators were already enabled and we'd never try them again.
> 
> Fixes: 1fc12b05895e ("regulator: core: Avoid propagating to supplies when possible")
> Reported-by: Evan Green <evgreen@...omium.org>
> Signed-off-by: Douglas Anderson <dianders@...omium.org>

Looks good on the Nexus 5 (qcom msm8974).

Tested-by: Brian Masney <masneyb@...tation.org>