| Message ID | 20240222083515.1065025-1-quic_kshivnan@quicinc.com (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Headers | show |
| Series | cpufreq: Limit resolving a frequency to policy min/max | expand |
On Thu, Feb 22, 2024 at 9:35 AM Shivnandan Kumar <quic_kshivnan@quicinc.com> wrote: > > Resolving a frequency to an efficient one should not transgress policy->max > (which can be set for thermal reason) and policy->min. Currently there is > possibility where scaling_cur_freq can exceed scaling_max_freq when > scaling_max_freq is inefficient frequency. Add additional check to ensure > that resolving a frequency will respect policy->min/max. > > Fixes: 1f39fa0dccff ("cpufreq: Introducing CPUFREQ_RELATION_E") > Signed-off-by: Shivnandan Kumar <quic_kshivnan@quicinc.com> > --- > include/linux/cpufreq.h | 18 +++++++++++++++++- > 1 file changed, 17 insertions(+), 1 deletion(-) > > diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h > index afda5f24d3dd..42d98b576a36 100644 > --- a/include/linux/cpufreq.h > +++ b/include/linux/cpufreq.h > @@ -1021,6 +1021,19 @@ static inline int cpufreq_table_find_index_c(struct cpufreq_policy *policy, > efficiencies); > } > > +static inline bool cpufreq_table_index_is_in_limits(struct cpufreq_policy *policy, > + int idx) This is not really about the index only, but about the frequency at that index too, so I'd call the function differently. > +{ > + unsigned int freq; > + > + if (idx < 0) > + return false; > + > + freq = policy->freq_table[idx].frequency; > + > + return (freq == clamp_val(freq, policy->min, policy->max)); Redundant outer parens. > +} > + > static inline int cpufreq_frequency_table_target(struct cpufreq_policy *policy, > unsigned int target_freq, > unsigned int relation) > @@ -1054,7 +1067,10 @@ static inline int cpufreq_frequency_table_target(struct cpufreq_policy *policy, > return 0; > } > > - if (idx < 0 && efficiencies) { > + /* > + * Limit frequency index to honor policy->min/max > + */ This comment need not be multi-line. > + if (!cpufreq_table_index_is_in_limits(policy, idx) && efficiencies) { > efficiencies = false; > goto retry; > } > -- Thanks!
Hi Rafael, Thanks for reviewing the change. On 2/23/2024 12:52 AM, Rafael J. Wysocki wrote: > On Thu, Feb 22, 2024 at 9:35 AM Shivnandan Kumar > <quic_kshivnan@quicinc.com> wrote: >> >> Resolving a frequency to an efficient one should not transgress policy->max >> (which can be set for thermal reason) and policy->min. Currently there is >> possibility where scaling_cur_freq can exceed scaling_max_freq when >> scaling_max_freq is inefficient frequency. Add additional check to ensure >> that resolving a frequency will respect policy->min/max. >> >> Fixes: 1f39fa0dccff ("cpufreq: Introducing CPUFREQ_RELATION_E") >> Signed-off-by: Shivnandan Kumar <quic_kshivnan@quicinc.com> >> --- >> include/linux/cpufreq.h | 18 +++++++++++++++++- >> 1 file changed, 17 insertions(+), 1 deletion(-) >> >> diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h >> index afda5f24d3dd..42d98b576a36 100644 >> --- a/include/linux/cpufreq.h >> +++ b/include/linux/cpufreq.h >> @@ -1021,6 +1021,19 @@ static inline int cpufreq_table_find_index_c(struct cpufreq_policy *policy, >> efficiencies); >> } >> >> +static inline bool cpufreq_table_index_is_in_limits(struct cpufreq_policy *policy, >> + int idx) > > This is not really about the index only, but about the frequency at > that index too, so I'd call the function differently. > ACK >> +{ >> + unsigned int freq; >> + >> + if (idx < 0) >> + return false; >> + >> + freq = policy->freq_table[idx].frequency; >> + >> + return (freq == clamp_val(freq, policy->min, policy->max)); > > Redundant outer parens. > ACK >> +} >> + >> static inline int cpufreq_frequency_table_target(struct cpufreq_policy *policy, >> unsigned int target_freq, >> unsigned int relation) >> @@ -1054,7 +1067,10 @@ static inline int cpufreq_frequency_table_target(struct cpufreq_policy *policy, >> return 0; >> } >> >> - if (idx < 0 && efficiencies) { >> + /* >> + * Limit frequency index to honor policy->min/max >> + */ > > This comment need not be multi-line. > ACK I will make the changes in next patch set. Thanks Shivnandan >> + if (!cpufreq_table_index_is_in_limits(policy, idx) && efficiencies) { >> efficiencies = false; >> goto retry; >> } >> -- > > Thanks!
diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h index afda5f24d3dd..42d98b576a36 100644 --- a/include/linux/cpufreq.h +++ b/include/linux/cpufreq.h @@ -1021,6 +1021,19 @@ static inline int cpufreq_table_find_index_c(struct cpufreq_policy *policy, efficiencies); } +static inline bool cpufreq_table_index_is_in_limits(struct cpufreq_policy *policy, + int idx) +{ + unsigned int freq; + + if (idx < 0) + return false; + + freq = policy->freq_table[idx].frequency; + + return (freq == clamp_val(freq, policy->min, policy->max)); +} + static inline int cpufreq_frequency_table_target(struct cpufreq_policy *policy, unsigned int target_freq, unsigned int relation) @@ -1054,7 +1067,10 @@ static inline int cpufreq_frequency_table_target(struct cpufreq_policy *policy, return 0; } - if (idx < 0 && efficiencies) { + /* + * Limit frequency index to honor policy->min/max + */ + if (!cpufreq_table_index_is_in_limits(policy, idx) && efficiencies) { efficiencies = false; goto retry; }
Resolving a frequency to an efficient one should not transgress policy->max (which can be set for thermal reason) and policy->min. Currently there is possibility where scaling_cur_freq can exceed scaling_max_freq when scaling_max_freq is inefficient frequency. Add additional check to ensure that resolving a frequency will respect policy->min/max. Fixes: 1f39fa0dccff ("cpufreq: Introducing CPUFREQ_RELATION_E") Signed-off-by: Shivnandan Kumar <quic_kshivnan@quicinc.com> --- include/linux/cpufreq.h | 18 +++++++++++++++++- 1 file changed, 17 insertions(+), 1 deletion(-)