[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index] Re: [Xen-devel] [PATCH v2 8/9] x86/intel_pstate: support the use of intel_pstate in pmstat.c
>>> On 13.05.16 at 09:51, <wei.w.wang@xxxxxxxxx> wrote: > --- a/xen/drivers/acpi/pmstat.c > +++ b/xen/drivers/acpi/pmstat.c > @@ -167,7 +167,7 @@ int do_get_pm_info(struct xen_sysctl_get_pmstat *op) > * 2. Provide user PM control > */ > static int read_scaling_available_governors(char > *scaling_available_governors, > - unsigned int size) > + unsigned int size, unsigned int is_internal) Please don't break indentation aiming at suitable alignment. And is_internal looks like it wants to be bool_t (and probably doesn't need the is_ prefix). > @@ -175,12 +175,19 @@ static int read_scaling_available_governors(char > *scaling_available_governors, > if ( !scaling_available_governors ) > return -EINVAL; > > - list_for_each_entry(t, &cpufreq_governor_list, governor_list) > - { > - i += scnprintf(&scaling_available_governors[i], > - CPUFREQ_NAME_LEN, "%s ", t->name); > - if ( i > size ) > - return -EINVAL; > + if (is_internal) { Coding style (throughout the changes to this file). > + i += scnprintf(&scaling_available_governors[0], CPUFREQ_NAME_LEN, > "%s ", "performance"); > + i += scnprintf(&scaling_available_governors[i], CPUFREQ_NAME_LEN, > "%s ", "powersave"); > + i += scnprintf(&scaling_available_governors[i], CPUFREQ_NAME_LEN, > "%s ", "userspace"); > + i += scnprintf(&scaling_available_governors[i], CPUFREQ_NAME_LEN, > "%s ", "ondemand"); Long lines. > @@ -203,11 +210,15 @@ static int get_cpufreq_para(struct xen_sysctl_pm_op > *op) > policy = per_cpu(cpufreq_cpu_policy, op->cpuid); > > if ( !pmpt || !pmpt->perf.states || > - !policy || !policy->governor ) > + !policy || (!policy->governor && !policy->policy) ) > return -EINVAL; > > - list_for_each(pos, &cpufreq_governor_list) > - gov_num++; > + if (policy->policy) > + gov_num = 4; This literal number again needs a #define. > @@ -261,29 +272,47 @@ static int get_cpufreq_para(struct xen_sysctl_pm_op *op) > op->u.get_para.cpuinfo_max_freq = policy->cpuinfo.max_freq; > op->u.get_para.cpuinfo_min_freq = policy->cpuinfo.min_freq; > op->u.get_para.scaling_cur_freq = policy->cur; > - op->u.get_para.scaling_max_freq = policy->max; > - op->u.get_para.scaling_min_freq = policy->min; > + if (policy->policy) { > + op->u.get_para.scaling_max.max_perf_pct = policy->max_perf_pct; > + op->u.get_para.scaling_min.min_perf_pct = policy->min_perf_pct; > + op->u.get_para.scaling_turbo_pct = policy->turbo_pct; > + } else { > + op->u.get_para.scaling_max.max_freq = policy->max; > + op->u.get_para.scaling_min.min_freq = policy->min; > + } How does the caller then know which of the union member meanings apply? > - if ( policy->governor->name[0] ) > - strlcpy(op->u.get_para.scaling_governor, > + if (policy->policy == CPUFREQ_POLICY_PERFORMANCE) switch() again. > /* governor specific para */ > - if ( !strnicmp(op->u.get_para.scaling_governor, > + if ( !strnicmp(op->u.get_para.scaling_governor, > "userspace", CPUFREQ_NAME_LEN) ) > { > op->u.get_para.u.userspace.scaling_setspeed = policy->cur; > } > > - if ( !strnicmp(op->u.get_para.scaling_governor, > + if ( !strnicmp(op->u.get_para.scaling_governor, Please can you avoid such white space adjustments to otherwise untouched code in an already big patch? > @@ -348,6 +392,24 @@ static int set_cpufreq_para(struct xen_sysctl_pm_op *op) > break; > } > > + case SCALING_MAX_PCT: > + { > + struct cpufreq_policy new_policy; > + memcpy(&new_policy, policy, sizeof(struct cpufreq_policy)); Blank line between declarations and statements please. Also please use structure assignment (type safe) instead of memcpy() (not type safe), and then perhaps right as initializer for the variable. > --- a/xen/include/public/sysctl.h > +++ b/xen/include/public/sysctl.h > @@ -314,8 +314,18 @@ struct xen_get_cpufreq_para { > uint32_t scaling_cur_freq; > > char scaling_governor[CPUFREQ_NAME_LEN]; > - uint32_t scaling_max_freq; > - uint32_t scaling_min_freq; > + > + union { > + uint32_t max_freq; > + int32_t max_perf_pct; > + } scaling_max; > + > + union { > + uint32_t min_freq; > + int32_t min_perf_pct; > + } scaling_min; > + > + int32_t scaling_turbo_pct; Again all of these should presumably be uint32_t. Plus the max/min duplication should be avoided (i.e. drop the prefixes from the union fields). Nor do I think perf_pct is really all that useful - pct or percentage would do afaict. Jan _______________________________________________ Xen-devel mailing list Xen-devel@xxxxxxxxxxxxx http://lists.xen.org/xen-devel
|
Lists.xenproject.org is hosted with RackSpace, monitoring our |