diff mbox series

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

Message ID 5896780.DvuYhMxLoT@rjwysocki.net
State New
Headers show
Series [v2] cpufreq: Fix setting policy limits when frequency tables are used | expand

Commit Message

Rafael J. Wysocki April 25, 2025, 11:36 a.m. UTC
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(-)

Comments

zhenglifeng (A) April 27, 2025, 2:25 a.m. UTC | #1
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));

>  
>  	trace_cpu_frequency_limits(policy);
> --- a/drivers/cpufreq/cpufreq_ondemand.c
> +++ b/drivers/cpufreq/cpufreq_ondemand.c
> @@ -76,7 +76,8 @@
>  		return freq_next;
>  	}
>  
> -	index = cpufreq_frequency_table_target(policy, freq_next, relation);
> +	index = cpufreq_frequency_table_target(policy, freq_next, policy->min,
> +					       policy->max, relation);
>  	freq_req = freq_table[index].frequency;
>  	freq_reduc = freq_req * od_tuners->powersave_bias / 1000;
>  	freq_avg = freq_req - freq_reduc;
> --- a/drivers/cpufreq/freq_table.c
> +++ b/drivers/cpufreq/freq_table.c
> @@ -115,8 +115,8 @@
>  EXPORT_SYMBOL_GPL(cpufreq_generic_frequency_table_verify);
>  
>  int cpufreq_table_index_unsorted(struct cpufreq_policy *policy,
> -				 unsigned int target_freq,
> -				 unsigned int relation)
> +				 unsigned int target_freq, unsigned int min,
> +				 unsigned int max, unsigned int relation)
>  {
>  	struct cpufreq_frequency_table optimal = {
>  		.driver_data = ~0,
> @@ -147,7 +147,7 @@
>  	cpufreq_for_each_valid_entry_idx(pos, table, i) {
>  		freq = pos->frequency;
>  
> -		if ((freq < policy->min) || (freq > policy->max))
> +		if (freq < min || freq > max)
>  			continue;
>  		if (freq == target_freq) {
>  			optimal.driver_data = i;
> --- a/include/linux/cpufreq.h
> +++ b/include/linux/cpufreq.h
> @@ -788,8 +788,8 @@
>  int cpufreq_generic_frequency_table_verify(struct cpufreq_policy_data *policy);
>  
>  int cpufreq_table_index_unsorted(struct cpufreq_policy *policy,
> -				 unsigned int target_freq,
> -				 unsigned int relation);
> +				 unsigned int target_freq, unsigned int min,
> +				 unsigned int max, unsigned int relation);
>  int cpufreq_frequency_table_get_index(struct cpufreq_policy *policy,
>  		unsigned int freq);
>  
> @@ -852,12 +852,12 @@
>  	return best;
>  }
>  
> -/* Works only on sorted freq-tables */
> -static inline int cpufreq_table_find_index_l(struct cpufreq_policy *policy,
> -					     unsigned int target_freq,
> -					     bool efficiencies)
> +static inline int find_index_l(struct cpufreq_policy *policy,
> +			       unsigned int target_freq,
> +			       unsigned int min, unsigned int max,
> +			       bool efficiencies)
>  {
> -	target_freq = clamp_val(target_freq, policy->min, policy->max);
> +	target_freq = clamp_val(target_freq, min, max);
>  
>  	if (policy->freq_table_sorted == CPUFREQ_TABLE_SORTED_ASCENDING)
>  		return cpufreq_table_find_index_al(policy, target_freq,
> @@ -867,6 +867,14 @@
>  						   efficiencies);
>  }
>  
> +/* Works only on sorted freq-tables */
> +static inline int cpufreq_table_find_index_l(struct cpufreq_policy *policy,
> +					     unsigned int target_freq,
> +					     bool efficiencies)
> +{
> +	return find_index_l(policy, target_freq, policy->min, policy->max, efficiencies);
> +}
> +
>  /* Find highest freq at or below target in a table in ascending order */
>  static inline int cpufreq_table_find_index_ah(struct cpufreq_policy *policy,
>  					      unsigned int target_freq,
> @@ -920,12 +928,12 @@
>  	return best;
>  }
>  
> -/* Works only on sorted freq-tables */
> -static inline int cpufreq_table_find_index_h(struct cpufreq_policy *policy,
> -					     unsigned int target_freq,
> -					     bool efficiencies)
> +static inline int find_index_h(struct cpufreq_policy *policy,
> +			       unsigned int target_freq,
> +			       unsigned int min, unsigned int max,
> +			       bool efficiencies)
>  {
> -	target_freq = clamp_val(target_freq, policy->min, policy->max);
> +	target_freq = clamp_val(target_freq, min, max);
>  
>  	if (policy->freq_table_sorted == CPUFREQ_TABLE_SORTED_ASCENDING)
>  		return cpufreq_table_find_index_ah(policy, target_freq,
> @@ -935,6 +943,14 @@
>  						   efficiencies);
>  }
>  
> +/* Works only on sorted freq-tables */
> +static inline int cpufreq_table_find_index_h(struct cpufreq_policy *policy,
> +					     unsigned int target_freq,
> +					     bool efficiencies)
> +{
> +	return find_index_h(policy, target_freq, policy->min, policy->max, efficiencies);
> +}
> +
>  /* Find closest freq to target in a table in ascending order */
>  static inline int cpufreq_table_find_index_ac(struct cpufreq_policy *policy,
>  					      unsigned int target_freq,
> @@ -1005,12 +1021,12 @@
>  	return best;
>  }
>  
> -/* Works only on sorted freq-tables */
> -static inline int cpufreq_table_find_index_c(struct cpufreq_policy *policy,
> -					     unsigned int target_freq,
> -					     bool efficiencies)
> +static inline int find_index_c(struct cpufreq_policy *policy,
> +			       unsigned int target_freq,
> +			       unsigned int min, unsigned int max,
> +			       bool efficiencies)
>  {
> -	target_freq = clamp_val(target_freq, policy->min, policy->max);
> +	target_freq = clamp_val(target_freq, min, max);
>  
>  	if (policy->freq_table_sorted == CPUFREQ_TABLE_SORTED_ASCENDING)
>  		return cpufreq_table_find_index_ac(policy, target_freq,
> @@ -1020,7 +1036,17 @@
>  						   efficiencies);
>  }
>  
> -static inline bool cpufreq_is_in_limits(struct cpufreq_policy *policy, int idx)
> +/* Works only on sorted freq-tables */
> +static inline int cpufreq_table_find_index_c(struct cpufreq_policy *policy,
> +					     unsigned int target_freq,
> +					     bool efficiencies)
> +{
> +	return find_index_c(policy, target_freq, policy->min, policy->max, efficiencies);
> +}
> +
> +static inline bool cpufreq_is_in_limits(struct cpufreq_policy *policy,
> +					unsigned int min, unsigned int max,
> +					int idx)
>  {
>  	unsigned int freq;
>  
> @@ -1029,11 +1055,13 @@
>  
>  	freq = policy->freq_table[idx].frequency;
>  
> -	return freq == clamp_val(freq, policy->min, policy->max);
> +	return freq == clamp_val(freq, min, max);
>  }
>  
>  static inline int cpufreq_frequency_table_target(struct cpufreq_policy *policy,
>  						 unsigned int target_freq,
> +						 unsigned int min,
> +						 unsigned int max,
>  						 unsigned int relation)
>  {
>  	bool efficiencies = policy->efficiencies_available &&
> @@ -1044,29 +1072,26 @@
>  	relation &= ~CPUFREQ_RELATION_E;
>  
>  	if (unlikely(policy->freq_table_sorted == CPUFREQ_TABLE_UNSORTED))
> -		return cpufreq_table_index_unsorted(policy, target_freq,
> -						    relation);
> +		return cpufreq_table_index_unsorted(policy, target_freq, min,
> +						    max, relation);
>  retry:
>  	switch (relation) {
>  	case CPUFREQ_RELATION_L:
> -		idx = cpufreq_table_find_index_l(policy, target_freq,
> -						 efficiencies);
> +		idx = find_index_l(policy, target_freq, min, max, efficiencies);
>  		break;
>  	case CPUFREQ_RELATION_H:
> -		idx = cpufreq_table_find_index_h(policy, target_freq,
> -						 efficiencies);
> +		idx = find_index_h(policy, target_freq, min, max, efficiencies);
>  		break;
>  	case CPUFREQ_RELATION_C:
> -		idx = cpufreq_table_find_index_c(policy, target_freq,
> -						 efficiencies);
> +		idx = find_index_c(policy, target_freq, min, max, efficiencies);
>  		break;
>  	default:
>  		WARN_ON_ONCE(1);
>  		return 0;
>  	}
>  
> -	/* Limit frequency index to honor policy->min/max */
> -	if (!cpufreq_is_in_limits(policy, idx) && efficiencies) {
> +	/* Limit frequency index to honor min and max */
> +	if (!cpufreq_is_in_limits(policy, min, max, idx) && efficiencies) {
>  		efficiencies = false;
>  		goto retry;
>  	}
> 
> 
> 
> 
>
Rafael J. Wysocki April 27, 2025, 11:41 a.m. UTC | #2
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.
zhenglifeng (A) April 28, 2025, 1:44 a.m. UTC | #3
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>
diff mbox series

Patch

--- 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);
 
 	trace_cpu_frequency_limits(policy);
--- a/drivers/cpufreq/cpufreq_ondemand.c
+++ b/drivers/cpufreq/cpufreq_ondemand.c
@@ -76,7 +76,8 @@ 
 		return freq_next;
 	}
 
-	index = cpufreq_frequency_table_target(policy, freq_next, relation);
+	index = cpufreq_frequency_table_target(policy, freq_next, policy->min,
+					       policy->max, relation);
 	freq_req = freq_table[index].frequency;
 	freq_reduc = freq_req * od_tuners->powersave_bias / 1000;
 	freq_avg = freq_req - freq_reduc;
--- a/drivers/cpufreq/freq_table.c
+++ b/drivers/cpufreq/freq_table.c
@@ -115,8 +115,8 @@ 
 EXPORT_SYMBOL_GPL(cpufreq_generic_frequency_table_verify);
 
 int cpufreq_table_index_unsorted(struct cpufreq_policy *policy,
-				 unsigned int target_freq,
-				 unsigned int relation)
+				 unsigned int target_freq, unsigned int min,
+				 unsigned int max, unsigned int relation)
 {
 	struct cpufreq_frequency_table optimal = {
 		.driver_data = ~0,
@@ -147,7 +147,7 @@ 
 	cpufreq_for_each_valid_entry_idx(pos, table, i) {
 		freq = pos->frequency;
 
-		if ((freq < policy->min) || (freq > policy->max))
+		if (freq < min || freq > max)
 			continue;
 		if (freq == target_freq) {
 			optimal.driver_data = i;
--- a/include/linux/cpufreq.h
+++ b/include/linux/cpufreq.h
@@ -788,8 +788,8 @@ 
 int cpufreq_generic_frequency_table_verify(struct cpufreq_policy_data *policy);
 
 int cpufreq_table_index_unsorted(struct cpufreq_policy *policy,
-				 unsigned int target_freq,
-				 unsigned int relation);
+				 unsigned int target_freq, unsigned int min,
+				 unsigned int max, unsigned int relation);
 int cpufreq_frequency_table_get_index(struct cpufreq_policy *policy,
 		unsigned int freq);
 
@@ -852,12 +852,12 @@ 
 	return best;
 }
 
-/* Works only on sorted freq-tables */
-static inline int cpufreq_table_find_index_l(struct cpufreq_policy *policy,
-					     unsigned int target_freq,
-					     bool efficiencies)
+static inline int find_index_l(struct cpufreq_policy *policy,
+			       unsigned int target_freq,
+			       unsigned int min, unsigned int max,
+			       bool efficiencies)
 {
-	target_freq = clamp_val(target_freq, policy->min, policy->max);
+	target_freq = clamp_val(target_freq, min, max);
 
 	if (policy->freq_table_sorted == CPUFREQ_TABLE_SORTED_ASCENDING)
 		return cpufreq_table_find_index_al(policy, target_freq,
@@ -867,6 +867,14 @@ 
 						   efficiencies);
 }
 
+/* Works only on sorted freq-tables */
+static inline int cpufreq_table_find_index_l(struct cpufreq_policy *policy,
+					     unsigned int target_freq,
+					     bool efficiencies)
+{
+	return find_index_l(policy, target_freq, policy->min, policy->max, efficiencies);
+}
+
 /* Find highest freq at or below target in a table in ascending order */
 static inline int cpufreq_table_find_index_ah(struct cpufreq_policy *policy,
 					      unsigned int target_freq,
@@ -920,12 +928,12 @@ 
 	return best;
 }
 
-/* Works only on sorted freq-tables */
-static inline int cpufreq_table_find_index_h(struct cpufreq_policy *policy,
-					     unsigned int target_freq,
-					     bool efficiencies)
+static inline int find_index_h(struct cpufreq_policy *policy,
+			       unsigned int target_freq,
+			       unsigned int min, unsigned int max,
+			       bool efficiencies)
 {
-	target_freq = clamp_val(target_freq, policy->min, policy->max);
+	target_freq = clamp_val(target_freq, min, max);
 
 	if (policy->freq_table_sorted == CPUFREQ_TABLE_SORTED_ASCENDING)
 		return cpufreq_table_find_index_ah(policy, target_freq,
@@ -935,6 +943,14 @@ 
 						   efficiencies);
 }
 
+/* Works only on sorted freq-tables */
+static inline int cpufreq_table_find_index_h(struct cpufreq_policy *policy,
+					     unsigned int target_freq,
+					     bool efficiencies)
+{
+	return find_index_h(policy, target_freq, policy->min, policy->max, efficiencies);
+}
+
 /* Find closest freq to target in a table in ascending order */
 static inline int cpufreq_table_find_index_ac(struct cpufreq_policy *policy,
 					      unsigned int target_freq,
@@ -1005,12 +1021,12 @@ 
 	return best;
 }
 
-/* Works only on sorted freq-tables */
-static inline int cpufreq_table_find_index_c(struct cpufreq_policy *policy,
-					     unsigned int target_freq,
-					     bool efficiencies)
+static inline int find_index_c(struct cpufreq_policy *policy,
+			       unsigned int target_freq,
+			       unsigned int min, unsigned int max,
+			       bool efficiencies)
 {
-	target_freq = clamp_val(target_freq, policy->min, policy->max);
+	target_freq = clamp_val(target_freq, min, max);
 
 	if (policy->freq_table_sorted == CPUFREQ_TABLE_SORTED_ASCENDING)
 		return cpufreq_table_find_index_ac(policy, target_freq,
@@ -1020,7 +1036,17 @@ 
 						   efficiencies);
 }
 
-static inline bool cpufreq_is_in_limits(struct cpufreq_policy *policy, int idx)
+/* Works only on sorted freq-tables */
+static inline int cpufreq_table_find_index_c(struct cpufreq_policy *policy,
+					     unsigned int target_freq,
+					     bool efficiencies)
+{
+	return find_index_c(policy, target_freq, policy->min, policy->max, efficiencies);
+}
+
+static inline bool cpufreq_is_in_limits(struct cpufreq_policy *policy,
+					unsigned int min, unsigned int max,
+					int idx)
 {
 	unsigned int freq;
 
@@ -1029,11 +1055,13 @@ 
 
 	freq = policy->freq_table[idx].frequency;
 
-	return freq == clamp_val(freq, policy->min, policy->max);
+	return freq == clamp_val(freq, min, max);
 }
 
 static inline int cpufreq_frequency_table_target(struct cpufreq_policy *policy,
 						 unsigned int target_freq,
+						 unsigned int min,
+						 unsigned int max,
 						 unsigned int relation)
 {
 	bool efficiencies = policy->efficiencies_available &&
@@ -1044,29 +1072,26 @@ 
 	relation &= ~CPUFREQ_RELATION_E;
 
 	if (unlikely(policy->freq_table_sorted == CPUFREQ_TABLE_UNSORTED))
-		return cpufreq_table_index_unsorted(policy, target_freq,
-						    relation);
+		return cpufreq_table_index_unsorted(policy, target_freq, min,
+						    max, relation);
 retry:
 	switch (relation) {
 	case CPUFREQ_RELATION_L:
-		idx = cpufreq_table_find_index_l(policy, target_freq,
-						 efficiencies);
+		idx = find_index_l(policy, target_freq, min, max, efficiencies);
 		break;
 	case CPUFREQ_RELATION_H:
-		idx = cpufreq_table_find_index_h(policy, target_freq,
-						 efficiencies);
+		idx = find_index_h(policy, target_freq, min, max, efficiencies);
 		break;
 	case CPUFREQ_RELATION_C:
-		idx = cpufreq_table_find_index_c(policy, target_freq,
-						 efficiencies);
+		idx = find_index_c(policy, target_freq, min, max, efficiencies);
 		break;
 	default:
 		WARN_ON_ONCE(1);
 		return 0;
 	}
 
-	/* Limit frequency index to honor policy->min/max */
-	if (!cpufreq_is_in_limits(policy, idx) && efficiencies) {
+	/* Limit frequency index to honor min and max */
+	if (!cpufreq_is_in_limits(policy, min, max, idx) && efficiencies) {
 		efficiencies = false;
 		goto retry;
 	}