| Message ID | 20210626162324.8236-1-tung-chen.shih@mediatek.com |
|---|---|
| State | New |
| Headers | show |
| Series | [v2,1/1] cpufreq: fix the target freq not in the range of policy->min & max | expand |
On 27-06-21, 00:23, TungChen Shih wrote: > The function cpufreq_driver_resolve_freq() should return the lowest Don't add extra spaces at the beginning of paragraphs here. > supported freq greater than or equal to the given target_freq, subject > to policy (min/max) and driver limitations. However, the index returned > by cpufreq_frequency_table_target() won't subject to policy min/max in > some cases. > > In cpufreq_frequency_table_target(), this function will try to find > an index for @target_freq in freq_table, and the frequency of selected > index should be in the range [policy->min, policy->max], which means: > > policy->min <= policy->freq_table[idx].frequency <= policy->max > > Though "clamp_val(target_freq, policy->min, policy->max);" would > have been called to check this condition, when policy->max or min is > not exactly one of the frequency in the frequency table, > policy->freq_table[idx].frequency may still go out of the range > > For example, if our sorted freq_table is [3000, 2000, 1000], and > suppose we have: > > @target_freq = 2500 > @policy->min = 2000 > @policy->max = 2200 > @relation = CPUFREQ_RELATION_L > > 1. After clamp_val(target_freq, policy->min, policy->max); @target_freq > becomes 2200 > 2. Since we use CPUFREQ_REALTION_L, final selected freq will be 3000 which > beyonds policy->max Right so the problem does exist, and not only with cpufreq_driver_resolve_freq(), but __cpufreq_driver_target() as well. I have a sent a patchset to update both of these to start sharing some code and we need to fix this for both now. > Signed-off-by: TungChen Shih <tung-chen.shih@mediatek.com> > --- > drivers/cpufreq/cpufreq.c | 15 +++++++++++++++ > 1 file changed, 15 insertions(+) > > diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c > index 802abc925b2a..8e3a17781618 100644 > --- a/drivers/cpufreq/cpufreq.c > +++ b/drivers/cpufreq/cpufreq.c > @@ -544,8 +544,23 @@ unsigned int cpufreq_driver_resolve_freq(struct cpufreq_policy *policy, > if (cpufreq_driver->target_index) { > unsigned int idx; > > + /* to find the frequency >= target_freq */ > idx = cpufreq_frequency_table_target(policy, target_freq, > CPUFREQ_RELATION_L); > + > + /* frequency should subject to policy (min/max) */ > + if (policy->freq_table[idx].frequency > policy->max) { > + if (policy->freq_table_sorted == CPUFREQ_TABLE_SORTED_ASCENDING) > + idx--; > + else > + idx++; > + } else if (policy->freq_table[idx].frequency < policy->min) { > + if (policy->freq_table_sorted == CPUFREQ_TABLE_SORTED_ASCENDING) > + idx++; > + else > + idx--; > + } This doesn't look clean to be honest. Rafael, does it make sense to update cpufreq_frequency_table_target() (and its internal routines) to take policy bounds in consideration, or something else ? -- viresh
On Tue, Jun 29, 2021 at 8:18 AM Viresh Kumar <viresh.kumar@linaro.org> wrote: > > On 27-06-21, 00:23, TungChen Shih wrote: > > The function cpufreq_driver_resolve_freq() should return the lowest > > Don't add extra spaces at the beginning of paragraphs here. > > > supported freq greater than or equal to the given target_freq, subject > > to policy (min/max) and driver limitations. However, the index returned > > by cpufreq_frequency_table_target() won't subject to policy min/max in > > some cases. > > > > In cpufreq_frequency_table_target(), this function will try to find > > an index for @target_freq in freq_table, and the frequency of selected > > index should be in the range [policy->min, policy->max], which means: > > > > policy->min <= policy->freq_table[idx].frequency <= policy->max > > > > Though "clamp_val(target_freq, policy->min, policy->max);" would > > have been called to check this condition, when policy->max or min is > > not exactly one of the frequency in the frequency table, > > policy->freq_table[idx].frequency may still go out of the range > > > > For example, if our sorted freq_table is [3000, 2000, 1000], and > > suppose we have: > > > > @target_freq = 2500 > > @policy->min = 2000 > > @policy->max = 2200 > > @relation = CPUFREQ_RELATION_L > > > > 1. After clamp_val(target_freq, policy->min, policy->max); @target_freq > > becomes 2200 > > 2. Since we use CPUFREQ_REALTION_L, final selected freq will be 3000 which > > beyonds policy->max > > Right so the problem does exist, That IMO is a matter for discussion and the patch author seems to have decided to ignore my previous comments. > and not only with > cpufreq_driver_resolve_freq(), but __cpufreq_driver_target() as well. That all depends on what the policy min and max limits are expected to mean and so far the interpretation has been that they are applied to the target frequency coming from the governor. Drivers have never been expected to ensure that the final effective frequency will always be between the policy min and max and, indeed, they may not even be able to ensure that. Now, because RELATION_L is defined as "the closest frequency equal to or above the target", running at a frequency below the target is questionable even if the max limit gets in the way. IOW, RELATION_L takes precedence over the policy max limit. Accordingly, I'm not going to apply this patch or anything similar to it until I'm given a really convincing argument otherwise. > I have a sent a patchset to update both of these to start sharing some > code and we need to fix this for both now. > > > Signed-off-by: TungChen Shih <tung-chen.shih@mediatek.com> > > --- > > drivers/cpufreq/cpufreq.c | 15 +++++++++++++++ > > 1 file changed, 15 insertions(+) > > > > diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c > > index 802abc925b2a..8e3a17781618 100644 > > --- a/drivers/cpufreq/cpufreq.c > > +++ b/drivers/cpufreq/cpufreq.c > > @@ -544,8 +544,23 @@ unsigned int cpufreq_driver_resolve_freq(struct cpufreq_policy *policy, > > if (cpufreq_driver->target_index) { > > unsigned int idx; > > > > + /* to find the frequency >= target_freq */ > > idx = cpufreq_frequency_table_target(policy, target_freq, > > CPUFREQ_RELATION_L); > > + > > + /* frequency should subject to policy (min/max) */ > > + if (policy->freq_table[idx].frequency > policy->max) { > > + if (policy->freq_table_sorted == CPUFREQ_TABLE_SORTED_ASCENDING) > > + idx--; > > + else > > + idx++; > > + } else if (policy->freq_table[idx].frequency < policy->min) { > > + if (policy->freq_table_sorted == CPUFREQ_TABLE_SORTED_ASCENDING) > > + idx++; > > + else > > + idx--; > > + } > > This doesn't look clean to be honest. > > Rafael, does it make sense to update cpufreq_frequency_table_target() > (and its internal routines) to take policy bounds in consideration, or > something else ? > > -- > viresh
diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c index 802abc925b2a..8e3a17781618 100644 --- a/drivers/cpufreq/cpufreq.c +++ b/drivers/cpufreq/cpufreq.c @@ -544,8 +544,23 @@ unsigned int cpufreq_driver_resolve_freq(struct cpufreq_policy *policy, if (cpufreq_driver->target_index) { unsigned int idx; + /* to find the frequency >= target_freq */ idx = cpufreq_frequency_table_target(policy, target_freq, CPUFREQ_RELATION_L); + + /* frequency should subject to policy (min/max) */ + if (policy->freq_table[idx].frequency > policy->max) { + if (policy->freq_table_sorted == CPUFREQ_TABLE_SORTED_ASCENDING) + idx--; + else + idx++; + } else if (policy->freq_table[idx].frequency < policy->min) { + if (policy->freq_table_sorted == CPUFREQ_TABLE_SORTED_ASCENDING) + idx++; + else + idx--; + } + policy->cached_resolved_idx = idx; return policy->freq_table[idx].frequency; }
The function cpufreq_driver_resolve_freq() should return the lowest supported freq greater than or equal to the given target_freq, subject to policy (min/max) and driver limitations. However, the index returned by cpufreq_frequency_table_target() won't subject to policy min/max in some cases. In cpufreq_frequency_table_target(), this function will try to find an index for @target_freq in freq_table, and the frequency of selected index should be in the range [policy->min, policy->max], which means: policy->min <= policy->freq_table[idx].frequency <= policy->max Though "clamp_val(target_freq, policy->min, policy->max);" would have been called to check this condition, when policy->max or min is not exactly one of the frequency in the frequency table, policy->freq_table[idx].frequency may still go out of the range For example, if our sorted freq_table is [3000, 2000, 1000], and suppose we have: @target_freq = 2500 @policy->min = 2000 @policy->max = 2200 @relation = CPUFREQ_RELATION_L 1. After clamp_val(target_freq, policy->min, policy->max); @target_freq becomes 2200 2. Since we use CPUFREQ_REALTION_L, final selected freq will be 3000 which beyonds policy->max Signed-off-by: TungChen Shih <tung-chen.shih@mediatek.com> --- drivers/cpufreq/cpufreq.c | 15 +++++++++++++++ 1 file changed, 15 insertions(+)