linux-kernel - Re: [PATCH v2] clk: qcom: smd: Disable unused clocks

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:   Mon, 9 Oct 2023 22:15:47 +0200
From:   Stephan Gerhold <stephan@...hold.net>
To:     Konrad Dybcio <konrad.dybcio@...aro.org>
Cc:     Stephan Gerhold <stephan.gerhold@...nkonzept.com>,
        Bjorn Andersson <andersson@...nel.org>,
        Andy Gross <agross@...nel.org>,
        Michael Turquette <mturquette@...libre.com>,
        Stephen Boyd <sboyd@...nel.org>, linux-arm-msm@...r.kernel.org,
        linux-clk@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] clk: qcom: smd: Disable unused clocks

On Fri, Oct 06, 2023 at 11:08:39PM +0200, Konrad Dybcio wrote:
> On 4.10.2023 14:10, Stephan Gerhold wrote:
> > At the moment, clk-smd-rpm forces all clocks on at probe time (for
> > "handoff"). However, it does not make the clk core aware of that.
> > 
> > This means that the clocks stay enabled forever if they are not used
> > by anything. We can easily disable them again after bootup has been
> > completed, by making the clk core aware of the state. This is
> > implemented by returning the current state of the clock in
> > is_prepared().
> > 
> > Checking the SPMI clock registers reveals that this allows the RPM to
> > disable unused BB/RF clocks. This reduces the power consumption quite
> > significantly and is also needed to allow entering low-power states.
> > 
> > As of commit d6edc31f3a68 ("clk: qcom: smd-rpm: Separate out
> > interconnect bus clocks") the interconnect-related clocks are no longer
> > managed/exposed by clk-smd-rpm. Also the BI_TCXO_AO clock is now
> > critical (and never disabled).
> > 
> > There is still a slight chance that this change will break boot on some
> > devices. However, this will be most likely caused by actual mistakes in
> > the device tree (where required clocks were not actually specified).
> Precisely this, and solely as a consequence of the interconnect driver
> not covering all the required clocks (usually named GCC_SOME_NOC_XYZ_CLK,
> but there's quite a lot more).
> 
> For platforms without an interconnect driver, breaking stuff this **MOST
> LIKELY** means that Linux uses some hw that isn't voted for (e.g. missing
> crypto clock under scm or something).
> 
> For those with an interconnect driver, this will uncover issues that were
> previously hidden because of the smd-rpm interconnect being essentially
> broken for most of its existence. I can smell 660 breaking from however
> many miles you are away from me, but it's "good", as we were relying on
> (board specific) magic..
> 
> I've been carrying an equivalent patch in my tree for over half a year now
> and IIRC 8996 was mostly fine. It's also a good idea to test suspend
> (echo mem > /sys/power/state) and wakeup.
> 

I didn't notice any problems on 8916 and 8909 either. :-)

> For reasons that I don't fully recall, I do have both .is_prepared and
> .is_enabled though..
> 

clk-smd-rpm doesn't have any .enable()/.disable() ops (only .prepare()
and .unprepare()) so I don't think is_enabled is needed. For the unused
clock cleanup in drivers/clk/clk.c (clk_disable_unused()) we just care
about the clk_unprepare_unused_subtree() part. That part is run when the
clock reports true in .is_prepared(). The equivalent for .is_enabled()
would just be a no-op because there are no .enable()/.disable() ops.

Thanks,
Stephan