From: Huang Rui <ray.huang@amd.com>
To: xufuhai <xufuhai1992@gmail.com>
Cc: Thomas Renninger <trenn@suse.com>,
linux@dominikbrodowski.net, sherry.hurwitz@amd.com,
linux-pm@vger.kernel.org, xufuhai <xufuhai@kuaishou.com>,
lishujin@kuaishou.com
Subject: Re: [PATCH v2 2/2] cpupower: fix amd cpu (family >= 0x17) active state issue
Date: Thu, 15 Apr 2021 21:45:28 +0800 [thread overview]
Message-ID: <20210415134528.GA2671822@hr-amd> (raw)
In-Reply-To: <79f26f6b-f9f8-36ac-6f43-6329935ba9e4@gmail.com>
On Thu, Apr 15, 2021 at 03:02:54PM +0800, xufuhai wrote:
> From: xufuhai <xufuhai@kuaishou.com>
>
> If the read_msr function is executed by a non-root user, the function returns
> -1, which means that there is no permission to access /dev/cpu/%d/msr, but
> cpufreq_has_boost_support should also return -1 immediately, and should not
> follow the original logic to return 0, which will cause amd The cpupower tool
> returns the boost active state as 0.
>
> Reproduce procedure:
> cpupower frequency-info
>
> Signed-off-by: xufuhai <xufuhai@kuaishou.com>
> Signed-off-by: chenguanqiao <chenguanqiao@kuaishou.com>
> Signed-off-by: lishujin <lishujin@kuaishou.com>
> Reviewed-by: Thomas Renninger <trenn@suse.com>
> ---
> tools/power/cpupower/utils/helpers/misc.c | 13 ++++++++-----
> 1 file changed, 8 insertions(+), 5 deletions(-)
>
> diff --git a/tools/power/cpupower/utils/helpers/misc.c b/tools/power/cpupower/utils/helpers/misc.c
> index fc6e34511721..565f8c414396 100644
> --- a/tools/power/cpupower/utils/helpers/misc.c
> +++ b/tools/power/cpupower/utils/helpers/misc.c
> @@ -16,7 +16,7 @@
> int cpufreq_has_boost_support(unsigned int cpu, int *support, int *active,
> int *states)
> {
> - int ret;
> + int ret = 0;
> unsigned long long val;
>
> *support = *active = *states = 0;
> @@ -30,18 +30,21 @@ int cpufreq_has_boost_support(unsigned int cpu, int *support, int *active,
> */
>
> if (cpupower_cpu_info.caps & CPUPOWER_CAP_AMD_CPB_MSR) {
> - if (!read_msr(cpu, MSR_AMD_HWCR, &val)) {
> + /*
> + * no permission to access /dev/cpu/%d/msr, return -1 immediately,
> + * and should not follow the original logic to return 0
> + */
Looks good for me as well.
Acked-by: Huang Rui <ray.huang@amd.com>
> + ret = read_msr(cpu, MSR_AMD_HWCR, &val);
> + if (!ret) {
> if (!(val & CPUPOWER_AMD_CPBDIS))
> *active = 1;
> }
> } else {
> ret = amd_pci_get_num_boost_states(active, states);
> - if (ret)
> - return ret;
> }
> } else if (cpupower_cpu_info.caps & CPUPOWER_CAP_INTEL_IDA)
> *support = *active = 1;
> - return 0;
> + return ret;
> }
>
> 在 2021/4/8 上午10:21, xufuhai 写道:
> > Any reply? Thomas
> >
> > 在 2021/3/30 上午10:46, xufuhai 写道:
> >> Thanks for your review, Thomas～
> >> as you suggested, I have updated my patch as your advice.
> >> Please help me review the patch again. thanks
> >>
> >>
> >> ----------------------------------------------------------------------------------------------------
> >>
> >> From: xufuhai <xufuhai@kuaishou.com>
> >>
> >> If the read_msr function is executed by a non-root user, the function returns
> >> -1, which means that there is no permission to access /dev/cpu/%d/msr, but
> >> cpufreq_has_boost_support should also return -1 immediately, and should not
> >> follow the original logic to return 0, which will cause amd The cpupower tool
> >> returns the boost active state as 0.
> >>
> >> Reproduce procedure:
> >> cpupower frequency-info
> >>
> >> Signed-off-by: xufuhai <xufuhai@kuaishou.com>
> >> Signed-off-by: chenguanqiao <chenguanqiao@kuaishou.com>
> >> Signed-off-by: lishujin <lishujin@kuaishou.com>
> >> Reviewed-by: Thomas Renninger <trenn@suse.com>
> >> ---
> >> tools/power/cpupower/utils/helpers/misc.c | 13 ++++++++-----
> >> 1 file changed, 8 insertions(+), 5 deletions(-)
> >>
> >> diff --git a/tools/power/cpupower/utils/helpers/misc.c b/tools/power/cpupower/utils/helpers/misc.c
> >> index fc6e34511721..565f8c414396 100644
> >> --- a/tools/power/cpupower/utils/helpers/misc.c
> >> +++ b/tools/power/cpupower/utils/helpers/misc.c
> >> @@ -16,7 +16,7 @@
> >> int cpufreq_has_boost_support(unsigned int cpu, int *support, int *active,
> >> int *states)
> >> {
> >> - int ret;
> >> + int ret = 0;
> >> unsigned long long val;
> >>
> >> *support = *active = *states = 0;
> >> @@ -30,18 +30,21 @@ int cpufreq_has_boost_support(unsigned int cpu, int *support, int *active,
> >> */
> >>
> >> if (cpupower_cpu_info.caps & CPUPOWER_CAP_AMD_CPB_MSR) {
> >> - if (!read_msr(cpu, MSR_AMD_HWCR, &val)) {
> >> + /*
> >> + * no permission to access /dev/cpu/%d/msr, return -1 immediately,
> >> + * and should not follow the original logic to return 0
> >> + */
> >> + ret = read_msr(cpu, MSR_AMD_HWCR, &val);
> >> + if (!ret) {
> >> if (!(val & CPUPOWER_AMD_CPBDIS))
> >> *active = 1;
> >> }
> >> } else {
> >> ret = amd_pci_get_num_boost_states(active, states);
> >> - if (ret)
> >> - return ret;
> >> }
> >> } else if (cpupower_cpu_info.caps & CPUPOWER_CAP_INTEL_IDA)
> >> *support = *active = 1;
> >> - return 0;
> >> + return ret;
> >> }
> >>
> >> int cpupower_intel_get_perf_bias(unsigned int cpu)
> >> --
> >> 2.24.3 (Apple Git-128)
> >>
> >> 在 2021/3/29 下午6:58, Thomas Renninger 写道:
> >>> Hi,
> >>>
> >>> Am Mittwoch, 24. März 2021, 09:28:38 CEST schrieb xufuhai:
> >>>> From: xufuhai <xufuhai@kuaishou.com>
> >>>>
> >>>> If the read_msr function is executed by a non-root user, the function
> >>>> returns -1, which means that there is no permission to access
> >>>> /dev/cpu/%d/msr, but cpufreq_has_boost_support should also return -1
> >>>> immediately, and should not follow the original logic to return 0, which
> >>>> will cause amd The cpupower tool returns the turbo active status as 0.
> >>>
> >>> Yes, this seem to be buggy.
> >>> Can you clean this up a bit more, please:
> >>>
> >>>> Reproduce procedure:
> >>>> cpupower frequency-info
> >>>>
> >>>> Signed-off-by: xufuhai <xufuhai@kuaishou.com>
> >>>> ---
> >>>> tools/power/cpupower/utils/helpers/misc.c | 9 +++++++--
> >>>> 1 file changed, 7 insertions(+), 2 deletions(-)
> >>>>
> >>>> diff --git a/tools/power/cpupower/utils/helpers/misc.c
> >>>> b/tools/power/cpupower/utils/helpers/misc.c index
> >>>> fc6e34511721..be96f9ce18eb 100644
> >>>> --- a/tools/power/cpupower/utils/helpers/misc.c
> >>>> +++ b/tools/power/cpupower/utils/helpers/misc.c
> >>>> @@ -30,10 +30,15 @@ int cpufreq_has_boost_support(unsigned int cpu, int
> >>>> *support, int *active, */
> >>>>
> >>>> if (cpupower_cpu_info.caps & CPUPOWER_CAP_AMD_CPB_MSR) {
> >>>> - if (!read_msr(cpu, MSR_AMD_HWCR, &val)) {
> >>>> + ret = read_msr(cpu, MSR_AMD_HWCR, &val);
> >>>> + if (!ret) {
> >>> ret should be initialized. I would initialize it with -1, but as Intel case
> >>> is always "good"/zero, it may make sense here to set:
> >>>
> >>> ret = 0
> >>> at the beginning of the func already.
> >>> At the end of the func, unconditionally returning zero:
> >>> return 0;
> >>> should be replace by:
> >>> return ret;
> >>>
> >>>> if (!(val & CPUPOWER_AMD_CPBDIS))
> >>>> *active = 1;
> >>>> - }
> >>>> + } else
> >>>> + /* no permission to access /dev/cpu/%d/msr, return -1 immediately,
> >>>> + * and should not follow the original logic to return 0
> >>>> + */
> >>>> + return ret;
> >>>
> >>> Then this part is not needed anymore, right?
> >>> Still the comment would be nice to show up, maybe slightly modified
> >>> in the if condition?
> >>> Afaik 100% correct comment would be:
> >>> /* ... */
> >>> for one line comment and:
> >>> /*
> >>> * ...
> >>> * ...
> >>> */
> >>> for multiline comment (one more line..).
> >>>
> >>>> } else {
> >>>> ret = amd_pci_get_num_boost_states(active, states);
> >>>> if (ret)
> >>> and these 2 lines can vanish as well at this point:
> >>> if (ret)
> >>> return ret;
> >>>
> >>> What do you think?
> >>>
> >>> Thanks for spotting this,
> >>>
> >>> Thomas
> >>>
> >>>
next prev parent reply other threads:[~2021-04-15 13:46 UTC|newest]
Thread overview: 10+ messages / expand[flat|nested] mbox.gz Atom feed top
[not found] <20210324082838.41462-1-xufuhai1992@gmail.com>
[not found] ` <20210324082838.41462-2-xufuhai1992@gmail.com>
2021-03-29 10:58 ` [PATCH 2/2] cpupower: fix amd cpu (family >= 0x17) active state issue Thomas Renninger
2021-03-30 2:46 ` xufuhai
2021-04-08 2:21 ` xufuhai
2021-04-15 7:02 ` [PATCH v2 " xufuhai
2021-04-15 13:45 ` Huang Rui [this message]
2021-04-15 23:07 ` Shuah Khan
[not found] ` <CAOkq_BcdvS1NtKJ9=peRWHc00kGjfvdE+Cyz7vqvH0+kermfhQ@mail.gmail.com>
2021-04-16 15:04 ` Shuah Khan
2021-03-29 11:10 ` [PATCH 1/2] cpupower: fix amd cpu (family < " Thomas Renninger
2021-03-30 3:47 ` xufuhai
2021-04-08 2:22 ` xufuhai
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=20210415134528.GA2671822@hr-amd \
--to=ray.huang@amd.com \
--cc=linux-pm@vger.kernel.org \
--cc=linux@dominikbrodowski.net \
--cc=lishujin@kuaishou.com \
--cc=sherry.hurwitz@amd.com \
--cc=trenn@suse.com \
--cc=xufuhai1992@gmail.com \
--cc=xufuhai@kuaishou.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