Re: [PATCH 2/2] cpupower: fix amd cpu (family >= 0x17) active state issue

[xufuhai <xufuhai1992@gmail.com>]

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
> 
>