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: Windows password security audit tool. GUI, reports in PDF.

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

Message-ID: <6f0ae81a-78a1-4584-b2dd-2f88aff598ef@huawei.com>
Date: Tue, 22 Apr 2025 20:05:39 +0800
From: "zhenglifeng (A)" <zhenglifeng1@...wei.com>
To: Viresh Kumar <viresh.kumar@...aro.org>
CC: "Rafael J. Wysocki" <rafael@...nel.org>, 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/22 17:41, Viresh Kumar wrote:

> On 21-04-25, 21:36, zhenglifeng (A) wrote:
>> On 2025/4/21 19:37, Viresh Kumar wrote:
>>> +static int policy_set_boost(struct cpufreq_policy *policy, bool enable, bool forced)
>>> +{
>>> +       if (!forced && (policy->boost_enabled == enable))
>>> +               return 0;
>>> +
>>> +       policy->boost_enabled = enable;
>>> +
>>> +       ret = cpufreq_driver->set_boost(policy, enable);
>>> +       if (ret)
>>> +               policy->boost_enabled = !policy->boost_enabled;
>>
>> This may cause boost_enabled becomes false but actually boosted when forced
>> is true and trying to set boost_enabled from true to true.
> 
> Can't do much in case of failure. And this is the current behavior
> anyway. This was just some code cleanup, doesn't change the behavior
> of the code.

If forced is true, this may change the behavior. But I see you gave up this
parameter in new version, so I think it's OK now.

> 
>>>  static struct freq_attr local_boost = __ATTR(boost, 0644, show_local_boost, store_local_boost);
>>> @@ -1617,16 +1624,17 @@ static int cpufreq_online(unsigned int cpu)
>>>         if (new_policy && cpufreq_thermal_control_enabled(cpufreq_driver))
>>>                 policy->cdev = of_cpufreq_cooling_register(policy);
>>>
>>> -       /* Let the per-policy boost flag mirror the cpufreq_driver boost during init */
>>> +       /*
>>> +        * Let the per-policy boost flag mirror the cpufreq_driver boost during
>>> +        * initialization for a new policy. For an existing policy, maintain the
>>> +        * previous boost value unless global boost is disabled now.
>>> +        */
>>>         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);
>>> +           (new_policy || !cpufreq_boost_enabled())) {
>>> +               ret = policy_set_boost(policy, cpufreq_boost_enabled(), false);
>>
>> I think forced here should be true. If new_policy and
>> !cpufreq_boost_enabled() but the cpu is actually boosted by some other
>> reason (like what we met in acpi-cpufreq), set_boost() should be forcibly
>> executed to make the cpu unboost.
> 
> The problem is that setting boost may be time consuming for some
> platforms and we may not want to do that unnecessarily. ACPI cpufreq
> should be fixed separately for that.

Makes sense.

> 
> I am sending a series now to fix them all, please review that.
>