* Re: [PATCH 1/2] cpupower: Fix amd cpu (family < 0x17) active state issue
[not found] ` <378e58d3-5300-1179-44bb-bc2b42a3beb0@gmail.com>
@ 2021-04-23 22:26 ` Shuah Khan
2021-04-25 2:41 ` 徐福海
0 siblings, 1 reply; 3+ messages in thread
From: Shuah Khan @ 2021-04-23 22:26 UTC (permalink / raw)
To: 徐福海, shuah, Thomas Renninger
Cc: linux-pm, linux-kernel, lishujin, xufuhai, Shuah Khan
On 4/19/21 8:27 PM, 徐福海 wrote:
> From: xufuhai<xufuhai@kuaishou.com>
>
> 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
>
> Reported-by: yangrui<yangrui@kuaishou.com>
> Signed-off-by: xufuhai<xufuhai@kuaishou.com>
Also your Signed-off-by should match the from address.
There is a mismatch between the two.
> 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/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
> --
> 2.24.3 (Apple Git-128)
>
I am seeing two patches with the same commit summary,
should these two be a singles patch?
https://patchwork.kernel.org/project/linux-pm/patch/6e35df20-753a-6c9c-8786-3fc87cdd17ba@gmail.com/
Please combine the two and send single patch if they fix the
same problem. If not, please change the commit log to reflect
the difference.
thanks,
-- Shuah
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH 1/2] cpupower: Fix amd cpu (family < 0x17) active state issue
2021-04-23 22:26 ` [PATCH 1/2] cpupower: Fix amd cpu (family < 0x17) active state issue Shuah Khan
@ 2021-04-25 2:41 ` 徐福海
0 siblings, 0 replies; 3+ messages in thread
From: 徐福海 @ 2021-04-25 2:41 UTC (permalink / raw)
To: Shuah Khan, shuah, Thomas Renninger
Cc: linux-pm, linux-kernel, lishujin, xufuhai
okay, I believe the two patches are for fixing defferent issue, I will update my patches as your mention
THANKS
在 2021/4/24 上午6:26, Shuah Khan 写道:
> On 4/19/21 8:27 PM, 徐福海 wrote:
>> From: xufuhai<xufuhai@kuaishou.com>
>>
>> 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
>>
>> Reported-by: yangrui<yangrui@kuaishou.com>
>> Signed-off-by: xufuhai<xufuhai@kuaishou.com>
>
> Also your Signed-off-by should match the from address.
> There is a mismatch between the two.
>
>> 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/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
>> --
>> 2.24.3 (Apple Git-128)
>>
>
> I am seeing two patches with the same commit summary,
> should these two be a singles patch?
>
> https://patchwork.kernel.org/project/linux-pm/patch/6e35df20-753a-6c9c-8786-3fc87cdd17ba@gmail.com/
>
> Please combine the two and send single patch if they fix the
> same problem. If not, please change the commit log to reflect
> the difference.
>
> thanks,
> -- Shuah
>
>
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH 1/2] cpupower: fix amd cpu (family < 0x17) active state issue
[not found] <35ab0d0a-f9ce-b4d7-cd85-3cd8a8638ab6@gmail.com>
@ 2021-03-29 3:51 ` xufuhai
0 siblings, 0 replies; 3+ messages in thread
From: xufuhai @ 2021-03-29 3:51 UTC (permalink / raw)
To: linux, sherry.hurwitz, trenn, linux-pm, rrichter,
nathan.fontenot, bp, latha, linux-kernel
Cc: lishujin
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 写道:
> From: xufuhai <xufuhai@kuaishou.com>
>
> 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 <xufuhai@kuaishou.com>
> Signed-off-by: chenguanqiao <chenguanqiao@kuaishou.com>
> Signed-off-by: lishujin <lishujin@kuaishou.com>
> ---
> 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
>
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2021-04-25 2:42 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
[not found] <bf312780-dda7-d08f-6098-1d8a7d4044e4@gmail.com>
[not found] ` <378e58d3-5300-1179-44bb-bc2b42a3beb0@gmail.com>
2021-04-23 22:26 ` [PATCH 1/2] cpupower: Fix amd cpu (family < 0x17) active state issue Shuah Khan
2021-04-25 2:41 ` 徐福海
[not found] <35ab0d0a-f9ce-b4d7-cd85-3cd8a8638ab6@gmail.com>
2021-03-29 3:51 ` [PATCH 1/2] cpupower: fix " xufuhai
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).