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

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

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