linux-kernel - Re: [PATCH] cpufreq: acpi: Don't enable boost on policy exit

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <c704850d-1fdd-4f25-8251-5bab03f055bb@huawei.com>
Date: Sat, 19 Apr 2025 17:35:51 +0800
From: "zhenglifeng (A)" <zhenglifeng1@...wei.com>
To: Viresh Kumar <viresh.kumar@...aro.org>, "Rafael J. Wysocki"
	<rafael@...nel.org>
CC: Nicholas Chin <nic.c3.14@...il.com>, <linux-kernel@...r.kernel.org>,
	<linux-pm@...r.kernel.org>, <rafael.j.wysocki@...el.com>,
	<vincent.guittot@...aro.org>
Subject: Re: [PATCH] cpufreq: acpi: Don't enable boost on policy exit

On 2025/4/19 15:54, Viresh Kumar wrote:

> On Sat, 19 Apr 2025 at 00:58, Rafael J. Wysocki <rafael@...nel.org> wrote:
> 
>> So it updates policy->boost_enabled in accordance with the current
>> setting in the MSR.
> 
> Yes.
> 
>> IMO it would be better to update the MSR in accordance with
>> policy->boost_enabled or users may get confused if their boost
>> settings change after a suspend-resume cycle.  Or have I got lost
>> completely?
> 
> I wrote this patch based on the sync that happens at the end of
> cpufreq_online():
> 
>         /* Let the per-policy boost flag mirror the cpufreq_driver
> boost during init */
>         if (cpufreq_driver->set_boost && policy->boost_supported &&
>             policy->boost_enabled != cpufreq_boost_enabled()) {
>                 policy->boost_enabled = cpufreq_boost_enabled();
>                 ret = cpufreq_driver->set_boost(policy, policy->boost_enabled);
>                 if (ret) {
>                         /* If the set_boost fails, the online
> operation is not affected */
>                         pr_info("%s: CPU%d: Cannot %s BOOST\n",
> __func__, policy->cpu,
>                                 str_enable_disable(policy->boost_enabled));
>                         policy->boost_enabled = !policy->boost_enabled;
>                 }
>         }
> 
> So my patch works as the cpufreq core force-syncs the state at init
> (pretty much what the driver was doing before).
> 
> Though I now wonder if this code (in cpufreq_online()) is really doing
> the right thing or not. So if global boost is enabled before suspend with
> policy boost being disabled, the policy boost will be enabled on resume.

Yes, the policy boost will be forcibly set to mirror the global boost. This
indicates that the global boost value is the default value of policy boost
each time the CPU goes online. Otherwise, we'll meet things like:

1. The global boost is set to disabled after a CPU going offline but the
policy boost is still be enabled after the CPU going online again.

2. The global boost is set to enabled after a CPU going offline and the
rest of the online CPUs are all boost enabled. However, the offline CPU
remains in the boost disabled state after it going online again. Users
have to set its boost state separately.

IMV, a user set the global boost means "I want all policy boost/unboost",
every CPU going online after that should follow this order. So I think
the code in cpufreq_online() is doing the right thing.

BTW, I think there is optimization can be done in
cpufreq_boost_trigger_state(). Currently, Nothing will happend if users set
global boost flag to true when this flag is already true. But I think it's
better to set all policies to boost in this situation. It might make more
sense to think of this as a refresh operation. This is just my idea. I'd
like to hear your opinion.

> 
> --
> Viresh