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

[Stephan Gerhold <stephan.gerhold@linaro.org>]

On Fri, Apr 25, 2025 at 01:36:21PM +0200, 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>

Thanks a lot for the quick fix! It works for me. After the CPU frequency
was throttled due to high temperature and the device has cooled down,
the CPU frequency goes back to maximum again.

Tested-by: Stephan Gerhold <stephan.gerhold@linaro.org>

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