linux-kernel - Re: [PATCH] regulator: core: Skip balancing of the enabled regulators in regulator

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: <20191008115025.GF4382@sirena.co.uk>
Date:   Tue, 8 Oct 2019 12:50:25 +0100
From:   Mark Brown <broonie@...nel.org>
To:     Marek Szyprowski <m.szyprowski@...sung.com>
Cc:     linux-kernel@...r.kernel.org, Dmitry Osipenko <digetx@...il.com>,
        Liam Girdwood <lgirdwood@...il.com>,
        Lucas Stach <l.stach@...gutronix.de>,
        Bartlomiej Zolnierkiewicz <b.zolnierkie@...sung.com>,
        Krzysztof Kozlowski <krzk@...nel.org>,
        linux-samsung-soc@...r.kernel.org
Subject: Re: [PATCH] regulator: core: Skip balancing of the enabled
 regulators in regulator_enable()

On Tue, Oct 08, 2019 at 12:17:09PM +0200, Marek Szyprowski wrote:
> Commit f8702f9e4aa7 ("regulator: core: Use ww_mutex for regulators
> locking"), regardless of the subject, added additional call to
> regulator_balance_voltage() during regulator_enable(). This is basically
> a good idea, however it causes some issue for the regulators which are
> already enabled at boot and are critical for system operation (for example
> provides supply to the CPU).

If regulators are essential to system operation they should be marked as
always-on...

> CPUfreq or other drivers typically call regulator_enable() on such
> regulators during their probe, although the regulators are already enabled
> by bootloader. The mentioned patch however added a call to
> regulator_balance_voltage(), what in case of system boot, where no
> additional requirements are set yet, typically causes to limit the voltage
> to the minimal value defined at regulator constraints. This causes a crash
> of the system when voltage on the CPU regulator is set to the lowest
> possible value without adjusting the operation frequency. Fix this by
> adding a check if regulator is already enabled - if so, then skip the
> balancing procedure. The voltage will be balanced later anyway once the
> required voltage value is requested.

This then means that for users that might legitimately enable and
disable regulators that need to be constrained are forced to change the
voltage when they enable the regualtors in order to have their
constraints take effect which seems bad.  I'd rather change the the
cpufreq consumers to either not do the enable (since there really should
be an always-on constraint this should be redundant, we might need to
fix the core to take account of their settings though I think we lost
that) or to set the voltage to whatever they need prior to doing their
first enable, that seems more robust.

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