Re: [PATCH v2] cpufreq: Fix setting policy limits when frequency tables are used

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

On 2025/4/27 19:41, Rafael J. Wysocki wrote:
> On Sun, Apr 27, 2025 at 4:26 AM zhenglifeng (A) <zhenglifeng1@huawei.com> wrote:
>>
>> On 2025/4/25 19:36, Rafael J. Wysocki wrote:
>>
>>> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>>>
>>> Commit 7491cdf46b5c ("cpufreq: Avoid using inconsistent policy->min and
>>> policy->max") overlooked the fact that policy->min and policy->max were
>>> accessed directly in cpufreq_frequency_table_target() and in the
>>> functions called by it.  Consequently, the changes made by that commit
>>> led to problems with setting policy limits.
>>>
>>> Address this by passing the target frequency limits to __resolve_freq()
>>> and cpufreq_frequency_table_target() and propagating them to the
>>> functions called by the latter.
>>>
>>> Fixes: 7491cdf46b5c ("cpufreq: Avoid using inconsistent policy->min and policy->max")
>>> Link: https://lore.kernel.org/linux-pm/aAplED3IA_J0eZN0@linaro.org/
>>> Reported-by: Stephan Gerhold <stephan.gerhold@linaro.org>
>>> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
>>> ---
>>>
>>> The v1 is here: https://lore.kernel.org/linux-pm/12665363.O9o76ZdvQC@rjwysocki.net/
>>>
>>> v1 -> v2:
>>>    * Do clamp_val(target_freq, min, max) before checking freq_table against
>>>      NULL in __resolve_freq().
>>>    * Update comment in cpufreq_frequency_table_target() to match the new code.
>>>
>>> ---
>>>  drivers/cpufreq/cpufreq.c          |   22 ++++++---
>>>  drivers/cpufreq/cpufreq_ondemand.c |    3 -
>>>  drivers/cpufreq/freq_table.c       |    6 +-
>>>  include/linux/cpufreq.h            |   83 ++++++++++++++++++++++++-------------
>>>  4 files changed, 73 insertions(+), 41 deletions(-)
>>>
>>> --- a/drivers/cpufreq/cpufreq.c
>>> +++ b/drivers/cpufreq/cpufreq.c
>>> @@ -491,14 +491,18 @@
>>>  EXPORT_SYMBOL_GPL(cpufreq_disable_fast_switch);
>>>
>>>  static unsigned int __resolve_freq(struct cpufreq_policy *policy,
>>> -             unsigned int target_freq, unsigned int relation)
>>> +                                unsigned int target_freq,
>>> +                                unsigned int min, unsigned int max,
>>> +                                unsigned int relation)
>>>  {
>>>       unsigned int idx;
>>>
>>> +     target_freq = clamp_val(target_freq, min, max);
>>> +
>>>       if (!policy->freq_table)
>>>               return target_freq;
>>>
>>> -     idx = cpufreq_frequency_table_target(policy, target_freq, relation);
>>> +     idx = cpufreq_frequency_table_target(policy, target_freq, min, max, relation);
>>>       policy->cached_resolved_idx = idx;
>>>       policy->cached_target_freq = target_freq;
>>>       return policy->freq_table[idx].frequency;
>>> @@ -532,8 +536,7 @@
>>>       if (unlikely(min > max))
>>>               min = max;
>>>
>>> -     return __resolve_freq(policy, clamp_val(target_freq, min, max),
>>> -                           CPUFREQ_RELATION_LE);
>>> +     return __resolve_freq(policy, target_freq, min, max, CPUFREQ_RELATION_LE);
>>>  }
>>>  EXPORT_SYMBOL_GPL(cpufreq_driver_resolve_freq);
>>>
>>> @@ -2351,8 +2354,8 @@
>>>       if (cpufreq_disabled())
>>>               return -ENODEV;
>>>
>>> -     target_freq = clamp_val(target_freq, policy->min, policy->max);
>>> -     target_freq = __resolve_freq(policy, target_freq, relation);
>>> +     target_freq = __resolve_freq(policy, target_freq, policy->min,
>>> +                                  policy->max, relation);
>>>
>>>       pr_debug("target for CPU %u: %u kHz, relation %u, requested %u kHz\n",
>>>                policy->cpu, target_freq, relation, old_target_freq);
>>> @@ -2650,8 +2653,11 @@
>>>        * compiler optimizations around them because they may be accessed
>>>        * concurrently by cpufreq_driver_resolve_freq() during the update.
>>>        */
>>> -     WRITE_ONCE(policy->max, __resolve_freq(policy, new_data.max, CPUFREQ_RELATION_H));
>>> -     new_data.min = __resolve_freq(policy, new_data.min, CPUFREQ_RELATION_L);
>>> +     WRITE_ONCE(policy->max, __resolve_freq(policy, new_data.max,
>>> +                                            new_data.min, new_data.max,
>>> +                                            CPUFREQ_RELATION_H));
>>> +     new_data.min = __resolve_freq(policy, new_data.min, new_data.min,
>>> +                                   new_data.max, CPUFREQ_RELATION_L);
>>>       WRITE_ONCE(policy->min, new_data.min > policy->max ? policy->max : new_data.min);
>>
>> It might be better like:
>>
>> -       WRITE_ONCE(policy->max, __resolve_freq(policy, new_data.max, CPUFREQ_RELATION_H));
>> -       new_data.min = __resolve_freq(policy, new_data.min, CPUFREQ_RELATION_L);
>> -       WRITE_ONCE(policy->min, new_data.min > policy->max ? policy->max : new_data.min);
>> +       WRITE_ONCE(policy->max, __resolve_freq(policy, new_data.max,
>> +                                              new_data.min, new_data.max,
>> +                                              CPUFREQ_RELATION_H));
>> +       WRITE_ONCE(policy->min, __resolve_freq(policy, new_data.min,
>> +                                              new_data.min, policy->max,
>> +                                              CPUFREQ_RELATION_L));
>>
> 
> Not really because policy->max may be less than new_data.min at this
> point AFAICS.

I see.

Reviewed-by: Lifeng Zheng <zhenglifeng1@huawei.com>