Re: [PATCH] cpufreq: intel_pstate: Fix operation mode switching

[Srinivas Pandruvada]

On Tue, 2017-02-28 at 00:05 +0100, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki 
> 
> There is a problem with intel_pstate operation mode switching
> introduced by commit fb1fe1041c04 (cpufreq: intel_pstate: Operation
> mode control from sysfs), because the global sysfs limits are
> preserved across operation modes while per-policy limits are
> reinitialized from scratch and both sets of limits may get out of
> sync this wya.
> 
> Fix that by always reinitializing the global limits upon the
> registration of the driver.
> 
> Fixes: fb1fe1041c04 (cpufreq: intel_pstate: Operation mode control
> from sysfs)
> Signed-off-by: Rafael J. Wysocki 
Acked-by: Srinivas Pandruvada 

> ---
>  drivers/cpufreq/intel_pstate.c |   65 +-
> ---
>  1 file changed, 22 insertions(+), 43 deletions(-)
> 
> Index: linux-pm/drivers/cpufreq/intel_pstate.c
> ===
> --- linux-pm.orig/drivers/cpufreq/intel_pstate.c
> +++ linux-pm/drivers/cpufreq/intel_pstate.c
> @@ -364,37 +364,25 @@ static bool driver_registered __read_mos
>  static bool acpi_ppc;
>  #endif
>  
> -static struct perf_limits performance_limits = {
> - .no_turbo = 0,
> - .turbo_disabled = 0,
> - .max_perf_pct = 100,
> - .max_perf = int_ext_tofp(1),
> - .min_perf_pct = 100,
> - .min_perf = int_ext_tofp(1),
> - .max_policy_pct = 100,
> - .max_sysfs_pct = 100,
> - .min_policy_pct = 0,
> - .min_sysfs_pct = 0,
> -};
> +static struct perf_limits performance_limits;
> +static struct perf_limits powersave_limits;
> +static struct perf_limits *limits;
>  
> -static struct perf_limits powersave_limits = {
> - .no_turbo = 0,
> - .turbo_disabled = 0,
> - .max_perf_pct = 100,
> - .max_perf = int_ext_tofp(1),
> - .min_perf_pct = 0,
> - .min_perf = 0,
> - .max_policy_pct = 100,
> - .max_sysfs_pct = 100,
> - .min_policy_pct = 0,
> - .min_sysfs_pct = 0,
> -};
> +static void intel_pstate_init_limits(struct perf_limits *limits)
> +{
> + memset(limits, 0, sizeof(*limits));
> + limits->max_perf_pct = 100;
> + limits->max_perf = int_ext_tofp(1);
> + limits->max_policy_pct = 100;
> + limits->max_sysfs_pct = 100;
> +}
>  
> -#ifdef CONFIG_CPU_FREQ_DEFAULT_GOV_PERFORMANCE
> -static struct perf_limits *limits = _limits;
> -#else
> -static struct perf_limits *limits = _limits;
> -#endif
> +static void intel_pstate_set_performance_limits(struct perf_limits
> *limits)
> +{
> + intel_pstate_init_limits(limits);
> + limits->min_perf_pct = 100;
> + limits->min_perf = int_ext_tofp(1);
> +}
>  
>  static DEFINE_MUTEX(intel_pstate_driver_lock);
>  static DEFINE_MUTEX(intel_pstate_limits_lock);
> @@ -2084,20 +2072,6 @@ static void intel_pstate_clear_update_ut
>   synchronize_sched();
>  }
>  
> -static void intel_pstate_set_performance_limits(struct perf_limits
> *limits)
> -{
> - limits->no_turbo = 0;
> - limits->turbo_disabled = 0;
> - limits->max_perf_pct = 100;
> - limits->max_perf = int_ext_tofp(1);
> - limits->min_perf_pct = 100;
> - limits->min_perf = int_ext_tofp(1);
> - limits->max_policy_pct = 100;
> - limits->max_sysfs_pct = 100;
> - limits->min_policy_pct = 0;
> - limits->min_sysfs_pct = 0;
> -}
> -
>  static void intel_pstate_update_perf_limits(struct cpufreq_policy
> *policy,
>   struct perf_limits
> *limits)
>  {
> @@ -2466,6 +2440,11 @@ static int intel_pstate_register_driver(
>  {
>   int ret;
>  
> + intel_pstate_init_limits(_limits);
> + intel_pstate_set_performance_limits(_limits);
> + limits = IS_ENABLED(CONFIG_CPU_FREQ_DEFAULT_GOV_PERFORMANCE)
> ?
> + _limits : _limits;
> +
>   ret = cpufreq_register_driver(intel_pstate_driver);
>   if (ret) {
>   intel_pstate_driver_cleanup();
> 

Re: [PATCH] cpufreq: intel_pstate: Fix operation mode switching

[Srinivas Pandruvada]

On Tue, 2017-02-28 at 00:05 +0100, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki 
> 
> There is a problem with intel_pstate operation mode switching
> introduced by commit fb1fe1041c04 (cpufreq: intel_pstate: Operation
> mode control from sysfs), because the global sysfs limits are
> preserved across operation modes while per-policy limits are
> reinitialized from scratch and both sets of limits may get out of
> sync this wya.
> 
> Fix that by always reinitializing the global limits upon the
> registration of the driver.
> 
> Fixes: fb1fe1041c04 (cpufreq: intel_pstate: Operation mode control
> from sysfs)
> Signed-off-by: Rafael J. Wysocki 
Acked-by: Srinivas Pandruvada 

> ---
>  drivers/cpufreq/intel_pstate.c |   65 +-
> ---
>  1 file changed, 22 insertions(+), 43 deletions(-)
> 
> Index: linux-pm/drivers/cpufreq/intel_pstate.c
> ===
> --- linux-pm.orig/drivers/cpufreq/intel_pstate.c
> +++ linux-pm/drivers/cpufreq/intel_pstate.c
> @@ -364,37 +364,25 @@ static bool driver_registered __read_mos
>  static bool acpi_ppc;
>  #endif
>  
> -static struct perf_limits performance_limits = {
> - .no_turbo = 0,
> - .turbo_disabled = 0,
> - .max_perf_pct = 100,
> - .max_perf = int_ext_tofp(1),
> - .min_perf_pct = 100,
> - .min_perf = int_ext_tofp(1),
> - .max_policy_pct = 100,
> - .max_sysfs_pct = 100,
> - .min_policy_pct = 0,
> - .min_sysfs_pct = 0,
> -};
> +static struct perf_limits performance_limits;
> +static struct perf_limits powersave_limits;
> +static struct perf_limits *limits;
>  
> -static struct perf_limits powersave_limits = {
> - .no_turbo = 0,
> - .turbo_disabled = 0,
> - .max_perf_pct = 100,
> - .max_perf = int_ext_tofp(1),
> - .min_perf_pct = 0,
> - .min_perf = 0,
> - .max_policy_pct = 100,
> - .max_sysfs_pct = 100,
> - .min_policy_pct = 0,
> - .min_sysfs_pct = 0,
> -};
> +static void intel_pstate_init_limits(struct perf_limits *limits)
> +{
> + memset(limits, 0, sizeof(*limits));
> + limits->max_perf_pct = 100;
> + limits->max_perf = int_ext_tofp(1);
> + limits->max_policy_pct = 100;
> + limits->max_sysfs_pct = 100;
> +}
>  
> -#ifdef CONFIG_CPU_FREQ_DEFAULT_GOV_PERFORMANCE
> -static struct perf_limits *limits = _limits;
> -#else
> -static struct perf_limits *limits = _limits;
> -#endif
> +static void intel_pstate_set_performance_limits(struct perf_limits
> *limits)
> +{
> + intel_pstate_init_limits(limits);
> + limits->min_perf_pct = 100;
> + limits->min_perf = int_ext_tofp(1);
> +}
>  
>  static DEFINE_MUTEX(intel_pstate_driver_lock);
>  static DEFINE_MUTEX(intel_pstate_limits_lock);
> @@ -2084,20 +2072,6 @@ static void intel_pstate_clear_update_ut
>   synchronize_sched();
>  }
>  
> -static void intel_pstate_set_performance_limits(struct perf_limits
> *limits)
> -{
> - limits->no_turbo = 0;
> - limits->turbo_disabled = 0;
> - limits->max_perf_pct = 100;
> - limits->max_perf = int_ext_tofp(1);
> - limits->min_perf_pct = 100;
> - limits->min_perf = int_ext_tofp(1);
> - limits->max_policy_pct = 100;
> - limits->max_sysfs_pct = 100;
> - limits->min_policy_pct = 0;
> - limits->min_sysfs_pct = 0;
> -}
> -
>  static void intel_pstate_update_perf_limits(struct cpufreq_policy
> *policy,
>   struct perf_limits
> *limits)
>  {
> @@ -2466,6 +2440,11 @@ static int intel_pstate_register_driver(
>  {
>   int ret;
>  
> + intel_pstate_init_limits(_limits);
> + intel_pstate_set_performance_limits(_limits);
> + limits = IS_ENABLED(CONFIG_CPU_FREQ_DEFAULT_GOV_PERFORMANCE)
> ?
> + _limits : _limits;
> +
>   ret = cpufreq_register_driver(intel_pstate_driver);
>   if (ret) {
>   intel_pstate_driver_cleanup();
> 

[PATCH] cpufreq: intel_pstate: Fix operation mode switching

[Rafael J. Wysocki]

From: Rafael J. Wysocki 

There is a problem with intel_pstate operation mode switching
introduced by commit fb1fe1041c04 (cpufreq: intel_pstate: Operation
mode control from sysfs), because the global sysfs limits are
preserved across operation modes while per-policy limits are
reinitialized from scratch and both sets of limits may get out of
sync this wya.

Fix that by always reinitializing the global limits upon the
registration of the driver.

Fixes: fb1fe1041c04 (cpufreq: intel_pstate: Operation mode control from sysfs)
Signed-off-by: Rafael J. Wysocki 
---
 drivers/cpufreq/intel_pstate.c |   65 +
 1 file changed, 22 insertions(+), 43 deletions(-)

Index: linux-pm/drivers/cpufreq/intel_pstate.c
===
--- linux-pm.orig/drivers/cpufreq/intel_pstate.c
+++ linux-pm/drivers/cpufreq/intel_pstate.c
@@ -364,37 +364,25 @@ static bool driver_registered __read_mos
 static bool acpi_ppc;
 #endif
 
-static struct perf_limits performance_limits = {
-   .no_turbo = 0,
-   .turbo_disabled = 0,
-   .max_perf_pct = 100,
-   .max_perf = int_ext_tofp(1),
-   .min_perf_pct = 100,
-   .min_perf = int_ext_tofp(1),
-   .max_policy_pct = 100,
-   .max_sysfs_pct = 100,
-   .min_policy_pct = 0,
-   .min_sysfs_pct = 0,
-};
+static struct perf_limits performance_limits;
+static struct perf_limits powersave_limits;
+static struct perf_limits *limits;
 
-static struct perf_limits powersave_limits = {
-   .no_turbo = 0,
-   .turbo_disabled = 0,
-   .max_perf_pct = 100,
-   .max_perf = int_ext_tofp(1),
-   .min_perf_pct = 0,
-   .min_perf = 0,
-   .max_policy_pct = 100,
-   .max_sysfs_pct = 100,
-   .min_policy_pct = 0,
-   .min_sysfs_pct = 0,
-};
+static void intel_pstate_init_limits(struct perf_limits *limits)
+{
+   memset(limits, 0, sizeof(*limits));
+   limits->max_perf_pct = 100;
+   limits->max_perf = int_ext_tofp(1);
+   limits->max_policy_pct = 100;
+   limits->max_sysfs_pct = 100;
+}
 
-#ifdef CONFIG_CPU_FREQ_DEFAULT_GOV_PERFORMANCE
-static struct perf_limits *limits = _limits;
-#else
-static struct perf_limits *limits = _limits;
-#endif
+static void intel_pstate_set_performance_limits(struct perf_limits *limits)
+{
+   intel_pstate_init_limits(limits);
+   limits->min_perf_pct = 100;
+   limits->min_perf = int_ext_tofp(1);
+}
 
 static DEFINE_MUTEX(intel_pstate_driver_lock);
 static DEFINE_MUTEX(intel_pstate_limits_lock);
@@ -2084,20 +2072,6 @@ static void intel_pstate_clear_update_ut
synchronize_sched();
 }
 
-static void intel_pstate_set_performance_limits(struct perf_limits *limits)
-{
-   limits->no_turbo = 0;
-   limits->turbo_disabled = 0;
-   limits->max_perf_pct = 100;
-   limits->max_perf = int_ext_tofp(1);
-   limits->min_perf_pct = 100;
-   limits->min_perf = int_ext_tofp(1);
-   limits->max_policy_pct = 100;
-   limits->max_sysfs_pct = 100;
-   limits->min_policy_pct = 0;
-   limits->min_sysfs_pct = 0;
-}
-
 static void intel_pstate_update_perf_limits(struct cpufreq_policy *policy,
struct perf_limits *limits)
 {
@@ -2466,6 +2440,11 @@ static int intel_pstate_register_driver(
 {
int ret;
 
+   intel_pstate_init_limits(_limits);
+   intel_pstate_set_performance_limits(_limits);
+   limits = IS_ENABLED(CONFIG_CPU_FREQ_DEFAULT_GOV_PERFORMANCE) ?
+   _limits : _limits;
+
ret = cpufreq_register_driver(intel_pstate_driver);
if (ret) {
intel_pstate_driver_cleanup();

[PATCH] cpufreq: intel_pstate: Fix operation mode switching

[Rafael J. Wysocki]

From: Rafael J. Wysocki 

There is a problem with intel_pstate operation mode switching
introduced by commit fb1fe1041c04 (cpufreq: intel_pstate: Operation
mode control from sysfs), because the global sysfs limits are
preserved across operation modes while per-policy limits are
reinitialized from scratch and both sets of limits may get out of
sync this wya.

Fix that by always reinitializing the global limits upon the
registration of the driver.

Fixes: fb1fe1041c04 (cpufreq: intel_pstate: Operation mode control from sysfs)
Signed-off-by: Rafael J. Wysocki 
---
 drivers/cpufreq/intel_pstate.c |   65 +
 1 file changed, 22 insertions(+), 43 deletions(-)

Index: linux-pm/drivers/cpufreq/intel_pstate.c
===
--- linux-pm.orig/drivers/cpufreq/intel_pstate.c
+++ linux-pm/drivers/cpufreq/intel_pstate.c
@@ -364,37 +364,25 @@ static bool driver_registered __read_mos
 static bool acpi_ppc;
 #endif
 
-static struct perf_limits performance_limits = {
-   .no_turbo = 0,
-   .turbo_disabled = 0,
-   .max_perf_pct = 100,
-   .max_perf = int_ext_tofp(1),
-   .min_perf_pct = 100,
-   .min_perf = int_ext_tofp(1),
-   .max_policy_pct = 100,
-   .max_sysfs_pct = 100,
-   .min_policy_pct = 0,
-   .min_sysfs_pct = 0,
-};
+static struct perf_limits performance_limits;
+static struct perf_limits powersave_limits;
+static struct perf_limits *limits;
 
-static struct perf_limits powersave_limits = {
-   .no_turbo = 0,
-   .turbo_disabled = 0,
-   .max_perf_pct = 100,
-   .max_perf = int_ext_tofp(1),
-   .min_perf_pct = 0,
-   .min_perf = 0,
-   .max_policy_pct = 100,
-   .max_sysfs_pct = 100,
-   .min_policy_pct = 0,
-   .min_sysfs_pct = 0,
-};
+static void intel_pstate_init_limits(struct perf_limits *limits)
+{
+   memset(limits, 0, sizeof(*limits));
+   limits->max_perf_pct = 100;
+   limits->max_perf = int_ext_tofp(1);
+   limits->max_policy_pct = 100;
+   limits->max_sysfs_pct = 100;
+}
 
-#ifdef CONFIG_CPU_FREQ_DEFAULT_GOV_PERFORMANCE
-static struct perf_limits *limits = _limits;
-#else
-static struct perf_limits *limits = _limits;
-#endif
+static void intel_pstate_set_performance_limits(struct perf_limits *limits)
+{
+   intel_pstate_init_limits(limits);
+   limits->min_perf_pct = 100;
+   limits->min_perf = int_ext_tofp(1);
+}
 
 static DEFINE_MUTEX(intel_pstate_driver_lock);
 static DEFINE_MUTEX(intel_pstate_limits_lock);
@@ -2084,20 +2072,6 @@ static void intel_pstate_clear_update_ut
synchronize_sched();
 }
 
-static void intel_pstate_set_performance_limits(struct perf_limits *limits)
-{
-   limits->no_turbo = 0;
-   limits->turbo_disabled = 0;
-   limits->max_perf_pct = 100;
-   limits->max_perf = int_ext_tofp(1);
-   limits->min_perf_pct = 100;
-   limits->min_perf = int_ext_tofp(1);
-   limits->max_policy_pct = 100;
-   limits->max_sysfs_pct = 100;
-   limits->min_policy_pct = 0;
-   limits->min_sysfs_pct = 0;
-}
-
 static void intel_pstate_update_perf_limits(struct cpufreq_policy *policy,
struct perf_limits *limits)
 {
@@ -2466,6 +2440,11 @@ static int intel_pstate_register_driver(
 {
int ret;
 
+   intel_pstate_init_limits(_limits);
+   intel_pstate_set_performance_limits(_limits);
+   limits = IS_ENABLED(CONFIG_CPU_FREQ_DEFAULT_GOV_PERFORMANCE) ?
+   _limits : _limits;
+
ret = cpufreq_register_driver(intel_pstate_driver);
if (ret) {
intel_pstate_driver_cleanup();