Re: [PATCH v1 2/2] cpufreq: Fix updates of cached resolved frequency and index

[Christian Loehle <christian.loehle@arm.com>]

On 11/6/26 15:05, Rafael J. Wysocki wrote:
> From: "Rafael J. Wysocki" <rafael.j.wysocki@intel.com>
> 
> The cached resolved frequency and index, cached_target_freq and
> cached_resolved_idx, respectively, may be updated concurrently if
> cpufreq_driver_resolve_freq() is called by the schedutil governor
> while cpufreq_set_policy() is running for the same policy.  Those
> concurrent updates are not synchronized in any way and the values
> written by cpufreq_set_policy(), if it wins the race, may confuse
> drivers using those policy fields [1].
> 
> To address this, use the observation that the only place where
> cached_target_freq and cached_resolved_idx need to be updated is
> cpufreq_driver_resolve_freq() because the only legitimate users of
> them are .fast_switch() driver callbacks.  Although cached_resolved_idx
> is also used in __cpufreq_driver_target(), that is in fact unnecessary
> and it is better to use a local variable for storing the frequency
> table index in that function.
> 
> Accordingly, rearrange __resolve_freq() and its callers so that
> cached_target_freq and cached_resolved_idx are only updated in
> cpufreq_driver_resolve_freq().
> 
> The cached_target_freq initialization in cpufreq_set_policy() is still
> needed, but move it after stopping the old governor and before starting
> the new one to avoid racing with cpufreq_driver_resolve_freq().
> 
> Fixes: f9ccdec24d91 ("cpufreq: Reuse cpufreq_driver_resolve_freq() in __cpufreq_driver_target()")
> Fixes: 151717690694 ("cpufreq: Make policy min/max hard requirements")
> Reported-by: Zhongqiu Han <zhongqiu.han@oss.qualcomm.com>
> Closes: https://lore.kernel.org/linux-pm/20261105140357.1331894-1-zhongqiu.han@oss.qualcomm.com/ [1]
> Co-developed-by: Zhongqiu Han <zhongqiu.han@oss.qualcomm.com>
> Signed-off-by: Zhongqiu Han <zhongqiu.han@oss.qualcomm.com>
> Cc: All applicable <stable@vger.kernel.org>
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Reviewed-by: Christian Loehle <christian.loehle@arm.com>

> ---
>  drivers/cpufreq/cpufreq.c |   31 ++++++++++++++++++++-----------
>  1 file changed, 20 insertions(+), 11 deletions(-)
> 
> --- a/drivers/cpufreq/cpufreq.c
> +++ b/drivers/cpufreq/cpufreq.c
> @@ -477,7 +477,7 @@ EXPORT_SYMBOL_GPL(cpufreq_disable_fast_s
>  static unsigned int __resolve_freq(struct cpufreq_policy *policy,
>  				   unsigned int target_freq,
>  				   unsigned int min, unsigned int max,
> -				   unsigned int relation)
> +				   unsigned int relation, unsigned int *idx_p)
>  {
>  	unsigned int idx;
>  
> @@ -487,9 +487,10 @@ static unsigned int __resolve_freq(struc
>  		return target_freq;
>  
>  	idx = cpufreq_frequency_table_target(policy, target_freq, min, max, relation);
> -	policy->cached_resolved_idx = idx;
> -	policy->cached_target_freq = policy->freq_table[idx].frequency;
> -	return policy->cached_target_freq;
> +	if (idx_p)
> +		*idx_p = idx;
> +
> +	return policy->freq_table[idx].frequency;
>  }
>  
>  /**
> @@ -520,7 +521,10 @@ unsigned int cpufreq_driver_resolve_freq
>  	if (unlikely(min > max))
>  		min = max;
>  
> -	return __resolve_freq(policy, target_freq, min, max, CPUFREQ_RELATION_LE);
> +	policy->cached_target_freq = __resolve_freq(policy, target_freq, min, max,
> +						    CPUFREQ_RELATION_LE,
> +						    &policy->cached_resolved_idx);
> +	return policy->cached_target_freq;
>  }
>  EXPORT_SYMBOL_GPL(cpufreq_driver_resolve_freq);
>  
> @@ -2398,12 +2402,13 @@ int __cpufreq_driver_target(struct cpufr
>  			    unsigned int relation)
>  {
>  	unsigned int old_target_freq = target_freq;
> +	unsigned int idx = 0;
>  
>  	if (cpufreq_disabled())
>  		return -ENODEV;
>  
>  	target_freq = __resolve_freq(policy, target_freq, policy->min,
> -				     policy->max, relation);
> +				     policy->max, relation, &idx);
>  
>  	pr_debug("CPU %u: cur %u kHz -> target %u kHz (req %u kHz, rel %u)\n",
>  		policy->cpu, policy->cur, target_freq, old_target_freq, relation);
> @@ -2433,7 +2438,7 @@ int __cpufreq_driver_target(struct cpufr
>  	if (!cpufreq_driver->target_index)
>  		return -EINVAL;
>  
> -	return __target_index(policy, policy->cached_resolved_idx);
> +	return __target_index(policy, idx);
>  }
>  EXPORT_SYMBOL_GPL(__cpufreq_driver_target);
>  
> @@ -2712,7 +2717,7 @@ static int cpufreq_set_policy(struct cpu
>  	 * concurrently by cpufreq_driver_resolve_freq() during the update.
>  	 */
>  	freq = __resolve_freq(policy, new_data.max, new_data.min, new_data.max,
> -			      CPUFREQ_RELATION_H);
> +			      CPUFREQ_RELATION_H, NULL);
>  	if (freq != policy->max) {
>  		WRITE_ONCE(policy->max, freq);
>  		if (need_update_limits)
> @@ -2720,7 +2725,7 @@ static int cpufreq_set_policy(struct cpu
>  	}
>  
>  	freq = __resolve_freq(policy, new_data.min, new_data.min, new_data.max,
> -			      CPUFREQ_RELATION_L);
> +			      CPUFREQ_RELATION_L, NULL);
>  	freq = min(freq, policy->max);
>  	if (freq != policy->min) {
>  		WRITE_ONCE(policy->min, freq);
> @@ -2732,8 +2737,6 @@ static int cpufreq_set_policy(struct cpu
>  
>  	cpufreq_update_pressure(policy);
>  
> -	policy->cached_target_freq = UINT_MAX;
> -
>  	pr_debug("new min and max freqs are %u - %u kHz\n",
>  		 policy->min, policy->max);
>  
> @@ -2759,6 +2762,12 @@ static int cpufreq_set_policy(struct cpu
>  		cpufreq_exit_governor(policy);
>  	}
>  
> +	/*
> +	 * Initialize cached_target_freq while no governor is running to avoid
> +	 * racing with cpufreq_driver_resolve_freq(), which updates it.
> +	 */
> +	policy->cached_target_freq = UINT_MAX;
> +
>  	/* start new governor */
>  	policy->governor = new_gov;
>  	ret = cpufreq_init_governor(policy);
> 
> 
>