From: Viresh Kumar <viresh.kumar@linaro.org>
To: TungChen Shih <tung-chen.shih@mediatek.com>
Cc: rjw@rjwysocki.net, matthias.bgg@gmail.com,
wsd_upstream@mediatek.com, linux-pm@vger.kernel.org,
linux-kernel@vger.kernel.org,
linux-arm-kernel@lists.infradead.org,
linux-mediatek@lists.infradead.org
Subject: Re: [PATCH v2 1/1] cpufreq: fix the target freq not in the range of policy->min & max
Date: Tue, 29 Jun 2021 11:47:58 +0530 [thread overview]
Message-ID: <20210629061758.wdavb2a4bpklmqi3@vireshk-i7> (raw)
In-Reply-To: <20210626162324.8236-1-tung-chen.shih@mediatek.com>
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
next prev parent reply other threads:[~2021-06-29 6:18 UTC|newest]
Thread overview: 3+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-06-26 16:23 [PATCH v2 1/1] cpufreq: fix the target freq not in the range of policy->min & max TungChen Shih
2021-06-29 6:17 ` Viresh Kumar [this message]
2021-06-30 16:32 ` Rafael J. Wysocki
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20210629061758.wdavb2a4bpklmqi3@vireshk-i7 \
--to=viresh.kumar@linaro.org \
--cc=linux-arm-kernel@lists.infradead.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-mediatek@lists.infradead.org \
--cc=linux-pm@vger.kernel.org \
--cc=matthias.bgg@gmail.com \
--cc=rjw@rjwysocki.net \
--cc=tung-chen.shih@mediatek.com \
--cc=wsd_upstream@mediatek.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).