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

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

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.
>