diff mbox series

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

Message ID 79f26f6b-f9f8-36ac-6f43-6329935ba9e4@gmail.com (mailing list archive)
State Superseded
Delegated to: Shuah Khan
Headers show
Series None | expand

Commit Message

徐福海 April 15, 2021, 7:02 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 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
>>>
>>>

Comments

Huang Rui April 15, 2021, 1:45 p.m. UTC | #1
On Thu, Apr 15, 2021 at 03:02:54PM +0800, 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 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
> +                        */

Looks good for me as well.

Acked-by: Huang Rui <ray.huang@amd.com>

> +                       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;
>  }
> 
> 在 2021/4/8 上午10:21, xufuhai 写道:
> > 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
> >>>
> >>>
Shuah Khan April 15, 2021, 11:07 p.m. UTC | #2
On 4/15/21 1:02 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 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>
> ---

As I mentioned on the previous version of this patch, please run
get_maintainers script and include me in the To: list.

The patch has to land in my Inbox for me to apply it.

thanks,
-- Shuah
Shuah Khan April 16, 2021, 3:04 p.m. UTC | #3
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
diff mbox series

Patch

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

在 2021/4/8 上午10:21, xufuhai 写道: