| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <2fd8c27d-7206-4af6-b30b-d8f786827d94@huawei.com>
Date: Sun, 27 Apr 2025 10:25:51 +0800
From: "zhenglifeng (A)" <zhenglifeng1@...wei.com>
To: "Rafael J. Wysocki" <rjw@...ysocki.net>
CC: Linux PM <linux-pm@...r.kernel.org>, Christian Loehle
<christian.loehle@....com>, LKML <linux-kernel@...r.kernel.org>, Viresh Kumar
<viresh.kumar@...aro.org>, Srinivas Pandruvada
<srinivas.pandruvada@...ux.intel.com>, Mario Limonciello
<mario.limonciello@....com>, Sultan Alsawaf <sultan@...neltoast.com>, Stephan
Gerhold <stephan.gerhold@...aro.org>, "Rafael J. Wysocki"
<rafael.j.wysocki@...el.com>
Subject: Re: [PATCH v2] cpufreq: Fix setting policy limits when frequency
tables are used
On 2025/4/25 19:36, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@...el.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@...aro.org>
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@...el.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;
> }
>
>
>
>
>
Powered by blists - more mailing lists