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] [thread-next>] [day] [month] [year] [list]
Message-ID: <20250421113753.lwukxhi45bnmqbpq@vireshk-i7>
Date: Mon, 21 Apr 2025 17:07:53 +0530
From: Viresh Kumar <viresh.kumar@...aro.org>
To: "zhenglifeng (A)" <zhenglifeng1@...wei.com>
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

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.

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;
+
+       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);
                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);
+               if (ret)
                        goto err_reset_state;
-               }
        }
        cpus_read_unlock();

-- 
viresh