Re: [Xen-devel] [PATCH v2 4/9] x86/intel_pstate: add new policy fields and a new driver interface

["Jan Beulich" <JBeulich@xxxxxxxx>]

>>> On 13.05.16 at 09:50, <wei.w.wang@xxxxxxxxx> wrote:
> --- a/xen/drivers/cpufreq/utility.c
> +++ b/xen/drivers/cpufreq/utility.c
> @@ -456,6 +456,11 @@ int __cpufreq_set_policy(struct cpufreq_policy *data,
>  
>      data->min = policy->min;
>      data->max = policy->max;
> +    data->min_perf_pct = policy->min_perf_pct;
> +    data->max_perf_pct = policy->max_perf_pct;

Shouldn't you do similar sanity checks on them as are being done on
->min and ->max? (Admittedly they look a little strange, but I'm not
asking you to copy what is there without sanity checking the original).

> +    if (cpufreq_driver->setpolicy)

Also I wonder whether the additions shouldn't move into this if()'s
body.

> --- a/xen/include/acpi/cpufreq/cpufreq.h
> +++ b/xen/include/acpi/cpufreq/cpufreq.h
> @@ -52,6 +52,10 @@ struct cpufreq_policy {
>      unsigned int        max;    /* in kHz */
>      unsigned int        cur;    /* in kHz, only needed if cpufreq
>                                   * governors are used */
> +    int                 min_perf_pct; /* min performance in percentage */
> +    int                 max_perf_pct; /* max performance in percentage */
> +    int                 turbo_pct;

Can any of these legitimately be negative? If not, they should be
of unsigned int type.

> @@ -87,6 +91,12 @@ struct cpufreq_freqs {
>   *                          CPUFREQ GOVERNORS                        *
>   *********************************************************************/
>  
> +/* the four internal governors used in intel_pstate */
> +#define CPUFREQ_POLICY_POWERSAVE        (1)
> +#define CPUFREQ_POLICY_PERFORMANCE      (2)
> +#define CPUFREQ_POLICY_USERSPACE        (3)
> +#define CPUFREQ_POLICY_ONDEMAND         (4)

Why are they being added in this header and at this point in the
sequence of patches? If they're local to intel_pstate, they should
go there. If they're needed by multiple files, they should be added
when needed, so that one can easily judge whether their addition
is indeed necessary.

Jan


_______________________________________________
Xen-devel mailing list
Xen-devel@xxxxxxxxxxxxx
http://lists.xen.org/xen-devel