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

[Thomas Renninger <trenn@suse.com>]

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