On 24.01.2024 21:59, Jason Andryuk wrote: > xenpm get-cpufreq-states currently just prints no output when cpufreq is > disabled or HWP is running. Have it print an appropriate message. The > cpufreq disabled one mirros the cpuidle disabled one. > > cpufreq disabled: > $ xenpm get-cpufreq-states > Either Xen cpufreq is disabled or no valid information is registered! > > Under HWP: > $ xenpm get-cpufreq-states > P-State information not supported. Try get-cpufreq-average or start. > > Also allow xenpm to handle EOPNOTSUPP from the pmstat hypercalls. > EOPNOTSUPP is returned when HWP is active in some cases and allows the > differentiation from cpufreq being disabled. > > Signed-off-by: Jason Andryuk <jandr...@gmail.com>
Largely okay, but a number of cosmetic remarks / nits: > --- a/tools/misc/xenpm.c > +++ b/tools/misc/xenpm.c > @@ -362,7 +362,15 @@ static int show_pxstat_by_cpuid(xc_interface *xc_handle, > int cpuid) > > ret = get_pxstat_by_cpuid(xc_handle, cpuid, &pxstatinfo); > if ( ret ) > + { > + if ( ret == -ENODEV ) > + fprintf(stderr, > + "Either Xen cpufreq is disabled or no valid information > is registered!\n"); > + else if ( ret == -EOPNOTSUPP ) One too few blanks for indentation. > + fprintf(stderr, > + "P-State information not supported. Try > get-cpufreq-average or start.\n"); Especially the "or start" part reads odd without any quotation. > @@ -382,9 +390,11 @@ void pxstat_func(int argc, char *argv[]) > { > /* show pxstates on all cpus */ > int i; > - for ( i = 0; i < max_cpu_nr; i++ ) > - if ( show_pxstat_by_cpuid(xc_handle, i) == -ENODEV ) > + for ( i = 0; i < max_cpu_nr; i++ ) { This file tries to follow hypervisor style, so the brace ought to go on its own line. While there may I ask that you also add the missing blank line (separating declaration from statements)? This then also applies ... > + int ret = show_pxstat_by_cpuid(xc_handle, i); > + if ( ret == -ENODEV || ret == -EOPNOTSUPP ) ... between these two new lines. > break; > + } Hard tab? > @@ -473,7 +483,8 @@ static void signal_int_handler(int signo) > } > } > > - if ( get_pxstat_by_cpuid(xc_handle, 0, NULL) != -ENODEV ) > + ret = get_pxstat_by_cpuid(xc_handle, 0, NULL); > + if ( ret != -ENODEV && ret != -EOPNOTSUPP ) While looking odd, I can see why it wants to be this way. Jan