linux-kernel - Re: [PATCH RFC 2/2] regulator: qcom

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: <ZSRhTIqYX7hQ0_lc@gerhold.net>
Date:   Mon, 9 Oct 2023 22:23:40 +0200
From:   Stephan Gerhold <stephan@...hold.net>
To:     Konrad Dybcio <konrad.dybcio@...aro.org>
Cc:     Stephan Gerhold <stephan.gerhold@...nkonzept.com>,
        Mark Brown <broonie@...nel.org>,
        Liam Girdwood <lgirdwood@...il.com>,
        Andy Gross <agross@...nel.org>,
        Bjorn Andersson <andersson@...nel.org>,
        linux-kernel@...r.kernel.org, linux-arm-msm@...r.kernel.org
Subject: Re: [PATCH RFC 2/2] regulator: qcom_smd: Disable unused regulators

On Fri, Oct 06, 2023 at 11:15:40PM +0200, Konrad Dybcio wrote:
> On 4.10.2023 16:17, Stephan Gerhold wrote:
> > The RPM firmware on Qualcomm platforms does not provide a way to check
> > if a regulator is on during boot using the SMD interface. If the
> > regulators are already on during boot and Linux does not make use of
> > them they will currently stay enabled forever. The regulator core does
> > not know these regulators are on and cannot clean them up together with
> > the other unused regulators.
> > 
> > Fix this by setting the initial enable state to -EINVAL similar to
> > qcom-rpmh-regulator.c. The regulator core will then also explicitly
> > disable all unused regulators with unknown status.
> > 
> > Signed-off-by: Stephan Gerhold <stephan.gerhold@...nkonzept.com>
> > ---
> > NOTE: This has a slight potential of breaking boards that rely on having
> > unused regulators permanently enabled (without regulator-always-on).
> > However, this is always a mistake in the device tree so it's probably
> > better to risk some breakage now, add the missing regulators and avoid
> > this problem for all future boards.
> > ---
> >  drivers/regulator/qcom_smd-regulator.c | 5 +++--
> >  1 file changed, 3 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/regulator/qcom_smd-regulator.c b/drivers/regulator/qcom_smd-regulator.c
> > index f53ada076252..0bbfba2e17ff 100644
> > --- a/drivers/regulator/qcom_smd-regulator.c
> > +++ b/drivers/regulator/qcom_smd-regulator.c
> > @@ -53,14 +53,14 @@ static int rpm_reg_write_active(struct qcom_rpm_reg *vreg)
> >  		reqlen++;
> >  	}
> >  
> > -	if (vreg->uv_updated && vreg->is_enabled) {
> > +	if (vreg->uv_updated && vreg->is_enabled > 0) {
> At a quick glance, are there any states for this value, other
> than 0 and 1? This is not the regulator_ops->is_enabled, but
> qcom_rpm_reg->is_enabled.
> 

Yes, I initially assign vreg->is_enabled = -EINVAL (for use with PATCH
1/2). It's in the part of the patch that you trimmed in your reply. :D

Thanks,
Stephan

@@ -1377,6 +1377,7 @@ static int rpm_regulator_init_vreg(struct qcom_rpm_reg *vreg, struct device *dev
 	vreg->rpm	= rpm;
 	vreg->type	= rpm_data->type;
 	vreg->id	= rpm_data->id;
+	vreg->is_enabled = -EINVAL;
 
 	memcpy(&vreg->desc, rpm_data->desc, sizeof(vreg->desc));
 	vreg->desc.name = rpm_data->name;