diff mbox series

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

Message ID e3f8c0f1-63dc-9b25-7129-d0a4ee87f62a@gmail.com (mailing list archive)
State Changes Requested
Delegated to: Shuah Khan
Headers show
Series [1/2] cpupower: fix amd cpu (family < 0x17) active state issue | expand

Commit Message

徐福海 March 24, 2021, 10:28 a.m. UTC
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.

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>
---
 tools/power/cpupower/utils/helpers/misc.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

Comments

Shuah Khan March 26, 2021, 8:13 p.m. UTC | #1
On 3/24/21 4:28 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 turbo active status as 0.
> 
> Reproduce procedure:
>          cpupower frequency-info
> 

Please run get_maintainer.pl and send patch maintainers
and others suggested by the tool. I don't see this in my
Inbox for me to review/accept and send it to pm maintainer.

Please include before and after the patch when you run
cpupower frequency-info

thanks,
-- Shuah
徐福海 March 29, 2021, 4:11 a.m. UTC | #2
yeah Shuah~ thanks for your reply

For this issue, not meaning "current CPU frequency" but "boost state support--->Active" during 
"cpupower frequency-info" command as below:

	boost state support:
    		Supported: yes
    		Active: yes/no

I think the state returned from the command for amd cpu (family >= 0x17) should be like as below:

	as non-root Active state:
		Active: Error while evaluating Boost Capabilities on CPU xx -- are you root?
	
	as root Active state:
		Active: yes (if supported)
			no  (if not supprted)

I don't wanna see the state returned like below:
	
	as non-root Active state:
		Active: no
	
	as root Active state:
		Active: yes (if supported)
			no  (if not supprted)
	
I will paste the related code by detailed for showing the issue:
	
	if amd cpu family >= 0x17 , will run read_msr function via read /dev/cpu/%d/msr. For non-root
caller can not open msr node due to having no permission, but cpufreq_has_boost_support will return 0 to 
upper caller function that not means caller no permission to read /dev/cpu/%d/msr. I believe we should
return negative value for the condition:

-----------------------------------------------------
/linux/tools/power/cpupower/utils/helper/misc.c
cpufreq_has_boost_support:
 
	if (cpupower_cpu_info.caps & CPUPOWER_CAP_AMD_CPB) {
		*support = 1;

		/* AMD Family 0x17 does not utilize PCI D18F4 like prior
		 * families and has no fixed discrete boost states but
		 * has Hardware determined variable increments instead.
		 */

		if (cpupower_cpu_info.caps & CPUPOWER_CAP_AMD_CPB_MSR) {
			if (!read_msr(cpu, MSR_AMD_HWCR, &val)) {
				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;
---------------------------------------------------



在 2021/3/27 上午4:13, Shuah Khan 写道:
> On 3/24/21 4:28 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 turbo active status as 0.
>>
>> Reproduce procedure:
>>          cpupower frequency-info
>>
> 
> Please run get_maintainer.pl and send patch maintainers
> and others suggested by the tool. I don't see this in my
> Inbox for me to review/accept and send it to pm maintainer.
> 
> Please include before and after the patch when you run
> cpupower frequency-info
> 
> thanks,
> -- Shuah
>
diff mbox series

Patch

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) {
 				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;
 		} else {
 			ret = amd_pci_get_num_boost_states(active, states);
 			if (ret)