| 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
| ||
|
Message-ID: <794278e8-633d-4fd7-affa-9e89ba9719bd@huawei.com>
Date: Mon, 21 Apr 2025 21:36:00 +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/21 19:37, Viresh Kumar wrote:
> Coming back to this response again:
>
> On 19-04-25, 17:35, zhenglifeng (A) wrote:
>> 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.
>
> This is surely a valid case, we must not enable policy boost when
> global boost is disabled.
>
>> 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.
>
> I now this this is the right behavior. The policy wasn't present when
> the global boost was enabled and so the action doesn't necessarily
> apply to it.
OK. I just think that in this case the users would generally want it to be
true. But if you think this is the right behavior, I'll accept it.
>
> This is how I think this should be fixed, we may still need to fix
> acpi driver's bug separately though:
>
> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> index 3841c9da6cac..7ac8b4c28658 100644
> --- a/drivers/cpufreq/cpufreq.c
> +++ b/drivers/cpufreq/cpufreq.c
> @@ -620,6 +620,20 @@ static ssize_t show_local_boost(struct cpufreq_policy *policy, char *buf)
> return sysfs_emit(buf, "%d\n", policy->boost_enabled);
> }
>
> +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.
> +
> + return ret;
> +}
> +
> static ssize_t store_local_boost(struct cpufreq_policy *policy,
> const char *buf, size_t count)
> {
> @@ -635,21 +649,14 @@ static ssize_t store_local_boost(struct cpufreq_policy *policy,
> if (!policy->boost_supported)
> return -EINVAL;
>
> - if (policy->boost_enabled == enable)
> - return count;
> -
> - policy->boost_enabled = enable;
> -
> cpus_read_lock();
> - ret = cpufreq_driver->set_boost(policy, enable);
> + ret = policy_set_boost(policy, enable, false);
> cpus_read_unlock();
>
> - if (ret) {
> - policy->boost_enabled = !policy->boost_enabled;
> - return ret;
> - }
> + if (!ret)
> + return count;
>
> - return count;
> + return ret;
> }
>
> 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.
> 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;
> + pr_info("%s: CPU%d: Cannot %s BOOST\n", __func__,
> + policy->cpu, str_enable_disable(cpufreq_boost_enabled()));
> }
> }
>
> @@ -2864,12 +2872,9 @@ static int cpufreq_boost_trigger_state(int state)
> if (!policy->boost_supported)
> continue;
>
> - policy->boost_enabled = state;
> - ret = cpufreq_driver->set_boost(policy, state);
> - if (ret) {
> - policy->boost_enabled = !policy->boost_enabled;
> + ret = policy_set_boost(policy, state, true);
Sorry, I can't see why forced need to be true here but false in other
places. Actually, the optimization I mentioned earlier is like:
@@ -2870,16 +2870,13 @@ static int cpufreq_boost_trigger_state(int state)
unsigned long flags;
int ret = 0;
- if (cpufreq_driver->boost_enabled == state)
- return 0;
-
write_lock_irqsave(&cpufreq_driver_lock, flags);
cpufreq_driver->boost_enabled = state;
write_unlock_irqrestore(&cpufreq_driver_lock, flags);
cpus_read_lock();
for_each_active_policy(policy) {
- if (!policy->boost_supported)
+ if (!policy->boost_supported || (policy->boost_enabled == state))
continue;
policy->boost_enabled = state;
> + if (ret)
> goto err_reset_state;
> - }
> }
> cpus_read_unlock();
>
Powered by blists - more mailing lists