linux-kernel - Re: [PATCH v4 2/2] regulator: add QCOM RPMh regulator driver

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:   Tue, 29 May 2018 22:32:38 -0700
From:   Doug Anderson <dianders@...omium.org>
To:     David Collins <collinsd@...eaurora.org>
Cc:     Mark Brown <broonie@...nel.org>,
        Liam Girdwood <lgirdwood@...il.com>,
        Rob Herring <robh+dt@...nel.org>,
        Mark Rutland <mark.rutland@....com>,
        linux-arm-msm@...r.kernel.org,
        Linux ARM <linux-arm-kernel@...ts.infradead.org>,
        devicetree@...r.kernel.org, LKML <linux-kernel@...r.kernel.org>,
        Rajendra Nayak <rnayak@...eaurora.org>,
        Stephen Boyd <sboyd@...nel.org>
Subject: Re: [PATCH v4 2/2] regulator: add QCOM RPMh regulator driver

Hi,

On Tue, May 22, 2018 at 7:43 PM, David Collins <collinsd@...eaurora.org> wrote:
> + * @ever_enabled:              Boolean indicating that the regulator has been
> + *                             explicitly enabled at least once.  Voltage
> + *                             requests should be cached when this flag is not
> + *                             set.

Do you really need this extra boolean?  Can't you just check if
"enabled" is still "-EINVAL"?  If it is then you don't pass the
voltage along.

...this would mean that you'd also need to send the voltage vote when
the regulator core tries to disable unused regulators at the end of
bootup, but that should be OK right?  If we never touched a regulator
anywhere at probe time and we're about to vote to disable it, we know
there's nobody requiring it to still be on.  We can vote for the
voltage now without fear of messing up a vote that the BIOS left in
place.

In theory this should also allow you to assert your vote about the
voltage of a regulator that has never been enabled, which (if I
understand correctly) you consider to be a feature.

---

Other than that comment, everything else here looks good to go if Mark
is good with it.  As per the previous threads there are some things
that I don't like a ton, but I feel it is between you and Mark to
decide if you're good with it.  A summary of those issues are:

1. I still believe that when we disable a regulator in Linux we should
tell RPMh that we vote for the lowest voltage.  ...but if Mark is
happy with the way the driver works now I won't push it.

2. As per my comments in the bindings patch, I still believe that
"qcom,drms-mode-max-microamps" belongs in the core.  Again, up to
Mark.

3. As per my comments in the bindings patch, I still believe that
we're just adding lots of noise to the DT most of the time by
specifying both "qcom,regulator-drms-modes" and
"regulator-allowed-modes".  Again, up to Mark.

-Doug