[PATCH v1 0/2] cpufreq: Fix issues related to cached resolved frequency and index

[PATCH v1 0/2] cpufreq: Fix issues related to cached resolved frequency and index

[Rafael J. Wysocki]

Hi All,

This series addresses two issues related to the caching of the
resolved frequency and index in __resolve_freq().

The first one is that the cached resolved frequency value should be
the post-resolution one, not the pre-resolution one (which is the
case now).  Patch [1/2] fixes this.

The other issue is that __resolve_freq() is now called in multiple
places, some of which may race with the cached frequency and index
updates in the schedutil governor path and that may cause the cached
values to become invalid.  This is fixed by patch [2/2].

Please see the individual patch changelogs for details.

Thanks!

[PATCH v1 1/2] cpufreq: Cache post-resolution target frequency in __resolve_freq()

[Rafael J. Wysocki]

From: "Rafael J. Wysocki" <rafael.j.wysocki@intel.com>

get_next_freq() in the schedutil governor returns the resolved
frequency from cpufreq_driver_resolve_freq(), and that resolved value
is then passed to cpufreq_driver_fast_switch().  The only reader of
cached_target_freq, acpi_cpufreq_fast_switch(), compares it with the
post-resolution target frequency, but it holds the pre-resolution
clamped frequency, so any request that falls between table entries
misses the cache and falls back to a table scan.

Address this by making __resolve_freq() store the post-resolution
frequency in cached_target_freq.

Fixes: e3c062360870 ("cpufreq: add cpufreq_driver_resolve_freq()")
Cc: All applicable <stable@vger.kernel.org>
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/cpufreq/cpufreq.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -488,8 +488,8 @@ static unsigned int __resolve_freq(struc
 
 	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;
+	policy->cached_target_freq = policy->freq_table[idx].frequency;
+	return policy->cached_target_freq;
 }
 
 /**

Re: [PATCH v1 1/2] cpufreq: Cache post-resolution target frequency in __resolve_freq()

[Christian Loehle]

On 11/6/26 15:04, Rafael J. Wysocki wrote:
> From: "Rafael J. Wysocki" <rafael.j.wysocki@intel.com>
> 
> get_next_freq() in the schedutil governor returns the resolved
> frequency from cpufreq_driver_resolve_freq(), and that resolved value
> is then passed to cpufreq_driver_fast_switch().  The only reader of
> cached_target_freq, acpi_cpufreq_fast_switch(), compares it with the
> post-resolution target frequency, but it holds the pre-resolution
> clamped frequency, so any request that falls between table entries
> misses the cache and falls back to a table scan.
> 
> Address this by making __resolve_freq() store the post-resolution
> frequency in cached_target_freq.
> 
> Fixes: e3c062360870 ("cpufreq: add cpufreq_driver_resolve_freq()")
> 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 |    4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> --- a/drivers/cpufreq/cpufreq.c
> +++ b/drivers/cpufreq/cpufreq.c
> @@ -488,8 +488,8 @@ static unsigned int __resolve_freq(struc
>  
>  	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;
> +	policy->cached_target_freq = policy->freq_table[idx].frequency;
> +	return policy->cached_target_freq;
>  }
>  
>  /**
> 
> 
> 

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

[Rafael J. Wysocki]

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

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

[Christian Loehle]

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

Re: [PATCH v1 0/2] cpufreq: Fix issues related to cached resolved frequency and index

[Christian Loehle]

On 11/6/26 14:59, Rafael J. Wysocki wrote:
> Hi All,
> 
> This series addresses two issues related to the caching of the
> resolved frequency and index in __resolve_freq().
> 
> The first one is that the cached resolved frequency value should be
> the post-resolution one, not the pre-resolution one (which is the
> case now).  Patch [1/2] fixes this.
> 
> The other issue is that __resolve_freq() is now called in multiple
> places, some of which may race with the cached frequency and index
> updates in the schedutil governor path and that may cause the cached
> values to become invalid.  This is fixed by patch [2/2].
> 
> Please see the individual patch changelogs for details.
> 
> Thanks!
> 
> 
> 

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