Commit e3c062360870 ("cpufreq: add cpufreq_driver_resolve_freq()") added policy->cached_resolved_idx so that "a subsequent fast_switch operation can avoid repeating the same lookup", and made cpufreq_set_policy() invalidate the cache instead of filling it. That is, the cached index is only meant to be used by the frequency switch immediately following the resolution that has produced it. However, two other places now access the cache without synchronizing with its other users: * Since commit f9ccdec24d91 ("cpufreq: Reuse cpufreq_driver_resolve_freq() in __cpufreq_driver_target()"), __cpufreq_driver_target() reads the resolved index back from the cache before calling ->target_index(). sugov_work() calls it without sg_policy->update_lock, so the scheduler path (a tick on the policy CPU or a remote update) can overwrite the index via cpufreq_driver_resolve_freq() meanwhile. This matters when sugov_hold_freq() discards the lower frequency resolved there, leaving the CPU below the frequency schedutil has kept for it. CPU0 (sugov_work()) CPU0 (tick interrupt) __cpufreq_driver_target(f_i) __resolve_freq() cached_resolved_idx = i sugov_update_single_freq() get_next_freq() cpufreq_driver_resolve_freq(f_k) cached_resolved_idx = k __target_index(cached_resolved_idx) ->target_index(policy, k) * Since commit 151717690694 ("cpufreq: Make policy min/max hard requirements"), cpufreq_set_policy() updates the cache as a side effect of __resolve_freq(), which can race with governors switching frequencies without policy->rwsem (schedutil, ondemand, conservative), and a similar race was addressed by commit 7491cdf46b5c ("cpufreq: Avoid using inconsistent policy->min and policy->max"). qcom-cpufreq-hw and apple-soc-cpufreq ->fast_switch() read the cached index directly too. CPU0 (sugov_work(), work_lock held) CPU1 (handle_update(), rwsem held) __cpufreq_driver_target(f_i) __resolve_freq() cached_resolved_idx = i cpufreq_set_policy() __resolve_freq(min) cached_resolved_idx = j_min __target_index(cached_resolved_idx) ->target_index(policy, j_min) In both cases, schedutil does not call the driver again until its selection changes, so the CPU can get stuck at a frequency it has not selected. All drivers implementing ->target_index() are affected. The cache is only consumed by ->fast_switch() right after cpufreq_driver_resolve_freq(), so only update it there and use a local index in __cpufreq_driver_target(). Fixes: f9ccdec24d91 ("cpufreq: Reuse cpufreq_driver_resolve_freq() in __cpufreq_driver_target()") Fixes: 151717690694 ("cpufreq: Make policy min/max hard requirements") Cc: stable@vger.kernel.org # 5.14+ Signed-off-by: Zhongqiu Han --- drivers/cpufreq/cpufreq.c | 38 ++++++++++++++++++++++++++++++-------- 1 file changed, 30 insertions(+), 8 deletions(-) diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c index 54dde8419bdc..ffde75dab91d 100644 --- a/drivers/cpufreq/cpufreq.c +++ b/drivers/cpufreq/cpufreq.c @@ -471,10 +471,17 @@ void cpufreq_disable_fast_switch(struct cpufreq_policy *policy) } EXPORT_SYMBOL_GPL(cpufreq_disable_fast_switch); +/* + * Note that this does not update the cached frequency lookup in @policy, + * because it may be running in parallel with a frequency switch carried out by + * the governor that uses the cached index. If @resolved_idx is not NULL, the + * index of the resolved frequency in the frequency table is stored in the + * location pointed to by it instead. + */ 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 *resolved_idx) { unsigned int idx; @@ -484,8 +491,8 @@ static unsigned int __resolve_freq(struct cpufreq_policy *policy, return target_freq; idx = cpufreq_frequency_table_target(policy, target_freq, min, max, relation); - policy->cached_resolved_idx = idx; - policy->cached_target_freq = target_freq; + if (resolved_idx) + *resolved_idx = idx; return policy->freq_table[idx].frequency; } @@ -505,6 +512,7 @@ unsigned int cpufreq_driver_resolve_freq(struct cpufreq_policy *policy, { unsigned int min = READ_ONCE(policy->min); unsigned int max = READ_ONCE(policy->max); + unsigned int freq, idx; /* * If this function runs in parallel with cpufreq_set_policy(), it may @@ -517,7 +525,20 @@ unsigned int cpufreq_driver_resolve_freq(struct cpufreq_policy *policy, if (unlikely(min > max)) min = max; - return __resolve_freq(policy, target_freq, min, max, CPUFREQ_RELATION_LE); + target_freq = clamp_val(target_freq, min, max); + freq = __resolve_freq(policy, target_freq, min, max, CPUFREQ_RELATION_LE, + &idx); + + /* + * Cache the lookup result for the subsequent ->fast_switch() call, which + * is the only user of it. + */ + if (policy->freq_table) { + policy->cached_resolved_idx = idx; + policy->cached_target_freq = target_freq; + } + + return freq; } EXPORT_SYMBOL_GPL(cpufreq_driver_resolve_freq); @@ -2355,12 +2376,13 @@ int __cpufreq_driver_target(struct cpufreq_policy *policy, unsigned int relation) { unsigned int old_target_freq = target_freq; + unsigned int idx; 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); @@ -2393,7 +2415,7 @@ int __cpufreq_driver_target(struct cpufreq_policy *policy, 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); @@ -2666,14 +2688,14 @@ static int cpufreq_set_policy(struct cpufreq_policy *policy, * 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); policy->update_limits = true; } 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); -- 2.43.0