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

["zhenglifeng (A)" <zhenglifeng1@huawei.com>]

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();
>