Thread (3 messages) 3 messages, 2 authors, 2021-03-29

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

From: xufuhai <hidden>
Date: 2021-03-29 03:52:30
Also in: lkml

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: yes
	
	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 amd_pci_get_num_boost_states function:
-----------------------------------------------------
/linux/tools/power/cpupower/utils/helper/misc.c
 
	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;
		}
---------------------------------------------------

/linux/tools/power/cpupower/utils/helper/amd.c
amd_pci_get_num_boost_states:
	
	val = pci_read_byte(device, 0x15c);

 	if (val & 3)
 		*active = 1;
 	else
----------------------------------------------------

pci_read_byte will memset val to 0xff if caller has no permission to access to read from pci_dev
but for amd_pci_get_num_boost_states, active state will set 1 meaning "yes". I believe that active
state should return negative value to caller function meaning "have no permission" will showing "
Error while evaluating Boost Capabilities on CPU xx -- are you root?"  

----------------------------------------------------
static inline void
pci_read_data(struct pci_dev *d, void *buf, int pos, int len)
{
  if (pos & (len-1))
    d->access->error("Unaligned read: pos=%02x, len=%d", pos, len);
  if (pos + len <= d->cache_len)
    memcpy(buf, d->cache + pos, len);
  else if (!d->methods->read(d, pos, buf, len))
    memset(buf, 0xff, len);
}

byte
pci_read_byte(struct pci_dev *d, int pos)
{
  byte buf;
  pci_read_data(d, &buf, pos, 1);
  return buf;
}
----------------------------------------------------


在 2021/3/24 下午6:27, xufuhai 写道:
quoted hunk ↗ jump to hunk
From: xufuhai <redacted>

For the old  AMD processor (family < 0x17), cpupower will call the
amd_pci_get_num_boost_states function, but for the non-root user
pci_read_byte function (implementation comes from the psutil library),
val will be set to 0xff, indicating that there is no read function
callback. At this time, the original logic will set the cpupower turbo
active state to yes. This is an obvious issue~

Reproduce procedure:
	cpupower frequency-info

Signed-off-by: xufuhai <redacted>
Signed-off-by: chenguanqiao <redacted>
Signed-off-by: lishujin <redacted>
---
 tools/power/cpupower/utils/helpers/amd.c | 7 +++++++
 1 file changed, 7 insertions(+)
diff --git a/tools/power/cpupower/utils/helpers/amd.c b/tools/power/cpupower/utils/helpers/amd.c
index 97f2c857048e..6f9504906afa 100644
--- a/tools/power/cpupower/utils/helpers/amd.c
+++ b/tools/power/cpupower/utils/helpers/amd.c
@@ -137,6 +137,13 @@ int amd_pci_get_num_boost_states(int *active, int *states)
 		return -ENODEV;
 
 	val = pci_read_byte(device, 0x15c);
+
+	/* If val is 0xff, meaning has no permisson to
+	 * get the boost states, return -1
+	 */
+	if (val == 0xff)
+		return -1;
+
 	if (val & 3)
 		*active = 1;
 	else
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help