| Message ID | 79f26f6b-f9f8-36ac-6f43-6329935ba9e4@gmail.com (mailing list archive) |
|---|---|
| State | Changes Requested, archived |
| Delegated to: | Shuah Khan |
| Headers | show |
| Series | None | expand |
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 > >>> > >>>
On 4/15/21 1:02 AM, 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> > --- As I mentioned on the previous version of this patch, please run get_maintainers script and include me in the To: list. The patch has to land in my Inbox for me to apply it. thanks, -- Shuah
On 4/15/21 9:45 PM, 徐福海 wrote: > hi Shuah, > Thanks for applying my patch. > and another whether needing to email a new patch such as v3 to you for > merging into mainline？or v2 patch has been ok? I didn't apply your patch. I don't have it in my Inbox as I mentioned in my email yesterday. Please run get_maintainers.pl - refer to the following for information on how to submit patches: https://www.kernel.org/doc/html/latest/process/submitting-patches.html thanks, -- Shuah
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; } 在 2021/4/8 上午10:21, xufuhai 写道: