[PATCH v1 0/6] intel_pstate: Turbo disabled handling rework

[PATCH v1 0/6] intel_pstate: Turbo disabled handling rework

[Rafael J. Wysocki]

Hi Everyone,

This series reworks the handling of disabling turbo in intel_pstate
on top of the previous series of cleanups

https://lore.kernel.org/linux-pm/12409658.O9o76ZdvQC@kreacher/

The underlying problem is that disabling turbo is handled quite consistently
in intel_pstate and basically it can get disabled at any time (through
MSR_IA32_MISC_ENABLE_TURBO_DISABLE) without much coordination with the
cpufreq core or anything else.

Disabling turbo through the "no_turbo" sysfs attribute is more consistent,
but it has issues too (for example, if turbo is disabled via "no_turbo",
the frequency-invariance code gets notified on the turbo status change,
but the actual maximum frequency of the CPU is only updated if the
MSR_IA32_MISC_ENABLE_TURBO_DISABLE value changes either, which need not
happen at the same time or even at all).

The first patch is not really related to the rest of the series, it's
just a cleanup and can be applied separately.

Patch [2/6] uses the observation that it should be necessary to read
MSR_IA32_MISC_ENABLE_TURBO_DISABLE after driver initialization to remove
in-flight reads on that MSR and turbo state updates related to them.

Patch [3/6] builds on top of the previous one to adjust the "no_turbo"
attribute "store" and "show" callbacks.

Patch [4/6] adds READ_ONCE() annotations to global.no_turbo accesses and
makes some related simplifications.

Patch [5/6] replaces the cached MSR_IA32_MISC_ENABLE_TURBO_DISABLE
value in some checks with global.no_turbo for consistency.

Patch [6/6] makes all of the code paths where the maximum CPU frequency
can change to do that consistently by using the same set of functions.

Details are described in the individual patch changelogs.

Thanks!

[PATCH v1 1/6] cpufreq: intel_pstate: Fold intel_pstate_max_within_limits() into caller

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Fold intel_pstate_max_within_limits() into its only caller.

No functional impact.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/cpufreq/intel_pstate.c |   13 ++++---------
 1 file changed, 4 insertions(+), 9 deletions(-)

Index: linux-pm/drivers/cpufreq/intel_pstate.c
===================================================================
--- linux-pm.orig/drivers/cpufreq/intel_pstate.c
+++ linux-pm/drivers/cpufreq/intel_pstate.c
@@ -2012,14 +2012,6 @@ static void intel_pstate_set_min_pstate(
 	intel_pstate_set_pstate(cpu, cpu->pstate.min_pstate);
 }
 
-static void intel_pstate_max_within_limits(struct cpudata *cpu)
-{
-	int pstate = max(cpu->pstate.min_pstate, cpu->max_perf_ratio);
-
-	update_turbo_state();
-	intel_pstate_set_pstate(cpu, pstate);
-}
-
 static void intel_pstate_get_cpu_pstates(struct cpudata *cpu)
 {
 	int perf_ctl_max_phys = pstate_funcs.get_max_physical(cpu->cpu);
@@ -2594,12 +2586,15 @@ static int intel_pstate_set_policy(struc
 	intel_pstate_update_perf_limits(cpu, policy->min, policy->max);
 
 	if (cpu->policy == CPUFREQ_POLICY_PERFORMANCE) {
+		int pstate = max(cpu->pstate.min_pstate, cpu->max_perf_ratio);
+
 		/*
 		 * NOHZ_FULL CPUs need this as the governor callback may not
 		 * be invoked on them.
 		 */
 		intel_pstate_clear_update_util_hook(policy->cpu);
-		intel_pstate_max_within_limits(cpu);
+		update_turbo_state();
+		intel_pstate_set_pstate(cpu, pstate);
 	} else {
 		intel_pstate_set_update_util_hook(policy->cpu);
 	}

[PATCH v1 2/6] cpufreq: intel_pstate: Do not update global.turbo_disabled after initialization

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

The global.turbo_disabled is updated quite often, especially in the
passive mode in which case it is updated every time the scheduler calls
into the driver.  However, this is generally not necessary and it adds
MSR read overhead to scheduler code paths (and that particular MSR is
slow to read).

For this reason, make the driver read MSR_IA32_MISC_ENABLE_TURBO_DISABLE
just once at the cpufreq driver registration time and remove all of the
in-flight updates of global.turbo_disabled.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/cpufreq/intel_pstate.c |   51 ++++++-----------------------------------
 1 file changed, 8 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
@@ -173,7 +173,6 @@ struct vid_data {
  *			based on the MSR_IA32_MISC_ENABLE value and whether or
  *			not the maximum reported turbo P-state is different from
  *			the maximum reported non-turbo one.
- * @turbo_disabled_mf:	The @turbo_disabled value reflected by cpuinfo.max_freq.
  * @min_perf_pct:	Minimum capacity limit in percent of the maximum turbo
  *			P-state capacity.
  * @max_perf_pct:	Maximum capacity limit in percent of the maximum turbo
@@ -182,7 +181,6 @@ struct vid_data {
 struct global_params {
 	bool no_turbo;
 	bool turbo_disabled;
-	bool turbo_disabled_mf;
 	int max_perf_pct;
 	int min_perf_pct;
 };
@@ -594,12 +592,13 @@ static void intel_pstate_hybrid_hwp_adju
 	cpu->pstate.min_pstate = intel_pstate_freq_to_hwp(cpu, freq);
 }
 
-static inline void update_turbo_state(void)
+static bool turbo_is_disabled(void)
 {
 	u64 misc_en;
 
 	rdmsrl(MSR_IA32_MISC_ENABLE, misc_en);
-	global.turbo_disabled = misc_en & MSR_IA32_MISC_ENABLE_TURBO_DISABLE;
+
+	return !!(misc_en & MSR_IA32_MISC_ENABLE_TURBO_DISABLE);
 }
 
 static int min_perf_pct_min(void)
@@ -1154,40 +1153,16 @@ static void intel_pstate_update_policies
 static void __intel_pstate_update_max_freq(struct cpudata *cpudata,
 					   struct cpufreq_policy *policy)
 {
-	policy->cpuinfo.max_freq = global.turbo_disabled_mf ?
+	policy->cpuinfo.max_freq = global.turbo_disabled ?
 			cpudata->pstate.max_freq : cpudata->pstate.turbo_freq;
 	refresh_frequency_limits(policy);
 }
 
-static void intel_pstate_update_max_freq(unsigned int cpu)
-{
-	struct cpufreq_policy *policy = cpufreq_cpu_acquire(cpu);
-
-	if (!policy)
-		return;
-
-	__intel_pstate_update_max_freq(all_cpu_data[cpu], policy);
-
-	cpufreq_cpu_release(policy);
-}
-
 static void intel_pstate_update_limits(unsigned int cpu)
 {
 	mutex_lock(&intel_pstate_driver_lock);
 
-	update_turbo_state();
-	/*
-	 * If turbo has been turned on or off globally, policy limits for
-	 * all CPUs need to be updated to reflect that.
-	 */
-	if (global.turbo_disabled_mf != global.turbo_disabled) {
-		global.turbo_disabled_mf = global.turbo_disabled;
-		arch_set_max_freq_ratio(global.turbo_disabled);
-		for_each_possible_cpu(cpu)
-			intel_pstate_update_max_freq(cpu);
-	} else {
-		cpufreq_update_policy(cpu);
-	}
+	cpufreq_update_policy(cpu);
 
 	mutex_unlock(&intel_pstate_driver_lock);
 }
@@ -1287,7 +1262,6 @@ static ssize_t show_no_turbo(struct kobj
 		return -EAGAIN;
 	}
 
-	update_turbo_state();
 	if (global.turbo_disabled)
 		ret = sprintf(buf, "%u\n", global.turbo_disabled);
 	else
@@ -1317,7 +1291,6 @@ static ssize_t store_no_turbo(struct kob
 
 	mutex_lock(&intel_pstate_limits_lock);
 
-	update_turbo_state();
 	if (global.turbo_disabled) {
 		pr_notice_once("Turbo disabled by BIOS or unavailable on processor\n");
 		mutex_unlock(&intel_pstate_limits_lock);
@@ -2281,8 +2254,6 @@ static void intel_pstate_adjust_pstate(s
 	struct sample *sample;
 	int target_pstate;
 
-	update_turbo_state();
-
 	target_pstate = get_target_pstate(cpu);
 	target_pstate = intel_pstate_prepare_request(cpu, target_pstate);
 	trace_cpu_frequency(target_pstate * cpu->pstate.scaling, cpu->cpu);
@@ -2593,7 +2564,6 @@ static int intel_pstate_set_policy(struc
 		 * be invoked on them.
 		 */
 		intel_pstate_clear_update_util_hook(policy->cpu);
-		update_turbo_state();
 		intel_pstate_set_pstate(cpu, pstate);
 	} else {
 		intel_pstate_set_update_util_hook(policy->cpu);
@@ -2637,7 +2607,6 @@ static void intel_pstate_verify_cpu_poli
 {
 	int max_freq;
 
-	update_turbo_state();
 	if (hwp_active) {
 		intel_pstate_get_hwp_cap(cpu);
 		max_freq = global.no_turbo || global.turbo_disabled ?
@@ -2734,8 +2703,6 @@ static int __intel_pstate_cpu_init(struc
 
 	/* cpuinfo and default policy values */
 	policy->cpuinfo.min_freq = cpu->pstate.min_freq;
-	update_turbo_state();
-	global.turbo_disabled_mf = global.turbo_disabled;
 	policy->cpuinfo.max_freq = global.turbo_disabled ?
 			cpu->pstate.max_freq : cpu->pstate.turbo_freq;
 
@@ -2901,8 +2868,6 @@ static int intel_cpufreq_target(struct c
 	struct cpufreq_freqs freqs;
 	int target_pstate;
 
-	update_turbo_state();
-
 	freqs.old = policy->cur;
 	freqs.new = target_freq;
 
@@ -2924,8 +2889,6 @@ static unsigned int intel_cpufreq_fast_s
 	struct cpudata *cpu = all_cpu_data[policy->cpu];
 	int target_pstate;
 
-	update_turbo_state();
-
 	target_pstate = intel_pstate_freq_to_hwp(cpu, target_freq);
 
 	target_pstate = intel_cpufreq_update_pstate(policy, target_pstate, true);
@@ -2943,7 +2906,6 @@ static void intel_cpufreq_adjust_perf(un
 	int old_pstate = cpu->pstate.current_pstate;
 	int cap_pstate, min_pstate, max_pstate, target_pstate;
 
-	update_turbo_state();
 	cap_pstate = global.turbo_disabled ? HWP_GUARANTEED_PERF(hwp_cap) :
 					     HWP_HIGHEST_PERF(hwp_cap);
 
@@ -3131,6 +3093,9 @@ static int intel_pstate_register_driver(
 
 	memset(&global, 0, sizeof(global));
 	global.max_perf_pct = 100;
+	global.turbo_disabled = turbo_is_disabled();
+
+	arch_set_max_freq_ratio(global.turbo_disabled);
 
 	intel_pstate_driver = driver;
 	ret = cpufreq_register_driver(intel_pstate_driver);

[PATCH v1 3/6] cpufreq: intel_pstate: Rearrange show_no_turbo() and store_no_turbo()

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Now that global.turbo_disabled can only change at the cpufreq driver
registration time, initialize global.no_turbo at that time too so they
are in sync to start with (if the former is set, the latter cannot be
updated later anyway).

That allows show_no_turbo() to be simlified because it does not need
to check global.turbo_disabled and store_no_turbo() can be rearranged
to avoid doing anything if the new value of global.no_turbo is equal
to the current one and only return an error on attempts to clear
global.no_turbo when global.turbo_disabled.

While at it, eliminate the redundant ret variable from store_no_turbo().

No intentional functional impact.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/cpufreq/intel_pstate.c |   34 ++++++++++++++++++----------------
 1 file changed, 18 insertions(+), 16 deletions(-)

Index: linux-pm/drivers/cpufreq/intel_pstate.c
===================================================================
--- linux-pm.orig/drivers/cpufreq/intel_pstate.c
+++ linux-pm/drivers/cpufreq/intel_pstate.c
@@ -1262,10 +1262,7 @@ static ssize_t show_no_turbo(struct kobj
 		return -EAGAIN;
 	}
 
-	if (global.turbo_disabled)
-		ret = sprintf(buf, "%u\n", global.turbo_disabled);
-	else
-		ret = sprintf(buf, "%u\n", global.no_turbo);
+	ret = sprintf(buf, "%u\n", global.no_turbo);
 
 	mutex_unlock(&intel_pstate_driver_lock);
 
@@ -1276,31 +1273,34 @@ static ssize_t store_no_turbo(struct kob
 			      const char *buf, size_t count)
 {
 	unsigned int input;
-	int ret;
+	bool no_turbo;
 
-	ret = sscanf(buf, "%u", &input);
-	if (ret != 1)
+	if (sscanf(buf, "%u", &input) != 1)
 		return -EINVAL;
 
 	mutex_lock(&intel_pstate_driver_lock);
 
 	if (!intel_pstate_driver) {
-		mutex_unlock(&intel_pstate_driver_lock);
-		return -EAGAIN;
+		count = -EAGAIN;
+		goto unlock_driver;
 	}
 
-	mutex_lock(&intel_pstate_limits_lock);
+	no_turbo = !!clamp_t(int, input, 0, 1);
+
+	if (no_turbo == global.no_turbo)
+		goto unlock_driver;
 
 	if (global.turbo_disabled) {
 		pr_notice_once("Turbo disabled by BIOS or unavailable on processor\n");
-		mutex_unlock(&intel_pstate_limits_lock);
-		mutex_unlock(&intel_pstate_driver_lock);
-		return -EPERM;
+		count = -EPERM;
+		goto unlock_driver;
 	}
 
-	global.no_turbo = clamp_t(int, input, 0, 1);
+	global.no_turbo = no_turbo;
+
+	mutex_lock(&intel_pstate_limits_lock);
 
-	if (global.no_turbo) {
+	if (no_turbo) {
 		struct cpudata *cpu = all_cpu_data[0];
 		int pct = cpu->pstate.max_pstate * 100 / cpu->pstate.turbo_pstate;
 
@@ -1312,8 +1312,9 @@ static ssize_t store_no_turbo(struct kob
 	mutex_unlock(&intel_pstate_limits_lock);
 
 	intel_pstate_update_policies();
-	arch_set_max_freq_ratio(global.no_turbo);
+	arch_set_max_freq_ratio(no_turbo);
 
+unlock_driver:
 	mutex_unlock(&intel_pstate_driver_lock);
 
 	return count;
@@ -3094,6 +3095,7 @@ static int intel_pstate_register_driver(
 	memset(&global, 0, sizeof(global));
 	global.max_perf_pct = 100;
 	global.turbo_disabled = turbo_is_disabled();
+	global.no_turbo = global.turbo_disabled;
 
 	arch_set_max_freq_ratio(global.turbo_disabled);
 

[PATCH v1 4/6] cpufreq: intel_pstate: Read global.no_turbo under READ_ONCE()

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Because global.no_turbo is generally not read under intel_pstate_driver_lock
make store_no_turbo() use WRITE_ONCE() for updating it (this is the only
place at which it is updated except for the initialization) and make the
majority of places reading it use READ_ONCE().

Also remove redundant global.turbo_disabled checks from places that
depend on the 'true' value of global.no_turbo because it can only be
'true' if global.turbo_disabled is also 'true'.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/cpufreq/intel_pstate.c |   12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

Index: linux-pm/drivers/cpufreq/intel_pstate.c
===================================================================
--- linux-pm.orig/drivers/cpufreq/intel_pstate.c
+++ linux-pm/drivers/cpufreq/intel_pstate.c
@@ -1296,7 +1296,7 @@ static ssize_t store_no_turbo(struct kob
 		goto unlock_driver;
 	}
 
-	global.no_turbo = no_turbo;
+	WRITE_ONCE(global.no_turbo, no_turbo);
 
 	mutex_lock(&intel_pstate_limits_lock);
 
@@ -1748,7 +1748,7 @@ static u64 atom_get_val(struct cpudata *
 	u32 vid;
 
 	val = (u64)pstate << 8;
-	if (global.no_turbo && !global.turbo_disabled)
+	if (READ_ONCE(global.no_turbo) && !global.turbo_disabled)
 		val |= (u64)1 << 32;
 
 	vid_fp = cpudata->vid.min + mul_fp(
@@ -1913,7 +1913,7 @@ static u64 core_get_val(struct cpudata *
 	u64 val;
 
 	val = (u64)pstate << 8;
-	if (global.no_turbo && !global.turbo_disabled)
+	if (READ_ONCE(global.no_turbo) && !global.turbo_disabled)
 		val |= (u64)1 << 32;
 
 	return val;
@@ -2211,7 +2211,7 @@ static inline int32_t get_target_pstate(
 
 	sample->busy_scaled = busy_frac * 100;
 
-	target = global.no_turbo || global.turbo_disabled ?
+	target = READ_ONCE(global.no_turbo) ?
 			cpu->pstate.max_pstate : cpu->pstate.turbo_pstate;
 	target += target >> 2;
 	target = mul_fp(target, busy_frac);
@@ -2473,7 +2473,7 @@ static void intel_pstate_clear_update_ut
 
 static int intel_pstate_get_max_freq(struct cpudata *cpu)
 {
-	return global.turbo_disabled || global.no_turbo ?
+	return READ_ONCE(global.no_turbo) ?
 			cpu->pstate.max_freq : cpu->pstate.turbo_freq;
 }
 
@@ -2610,7 +2610,7 @@ static void intel_pstate_verify_cpu_poli
 
 	if (hwp_active) {
 		intel_pstate_get_hwp_cap(cpu);
-		max_freq = global.no_turbo || global.turbo_disabled ?
+		max_freq = READ_ONCE(global.no_turbo) ?
 				cpu->pstate.max_freq : cpu->pstate.turbo_freq;
 	} else {
 		max_freq = intel_pstate_get_max_freq(cpu);

[PATCH v1 5/6] cpufreq: intel_pstate: Replace three global.turbo_disabled checks

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

Replace the global.turbo_disabled in __intel_pstate_update_max_freq() with
a global.no_turbo one to make store_no_turbo() actually update the maximum
CPU frequency on the trubo preference changes, which needs to be consistent
with arch_set_max_freq_ratio() called from there.

For more consistency, replace the global.turbo_disabled checks in
__intel_pstate_cpu_init() and intel_cpufreq_adjust_perf() with
global.no_turbo checks either.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/cpufreq/intel_pstate.c |    9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

Index: linux-pm/drivers/cpufreq/intel_pstate.c
===================================================================
--- linux-pm.orig/drivers/cpufreq/intel_pstate.c
+++ linux-pm/drivers/cpufreq/intel_pstate.c
@@ -1153,7 +1153,7 @@ static void intel_pstate_update_policies
 static void __intel_pstate_update_max_freq(struct cpudata *cpudata,
 					   struct cpufreq_policy *policy)
 {
-	policy->cpuinfo.max_freq = global.turbo_disabled ?
+	policy->cpuinfo.max_freq = READ_ONCE(global.no_turbo) ?
 			cpudata->pstate.max_freq : cpudata->pstate.turbo_freq;
 	refresh_frequency_limits(policy);
 }
@@ -2704,7 +2704,7 @@ static int __intel_pstate_cpu_init(struc
 
 	/* cpuinfo and default policy values */
 	policy->cpuinfo.min_freq = cpu->pstate.min_freq;
-	policy->cpuinfo.max_freq = global.turbo_disabled ?
+	policy->cpuinfo.max_freq = READ_ONCE(global.no_turbo) ?
 			cpu->pstate.max_freq : cpu->pstate.turbo_freq;
 
 	policy->min = policy->cpuinfo.min_freq;
@@ -2907,8 +2907,9 @@ static void intel_cpufreq_adjust_perf(un
 	int old_pstate = cpu->pstate.current_pstate;
 	int cap_pstate, min_pstate, max_pstate, target_pstate;
 
-	cap_pstate = global.turbo_disabled ? HWP_GUARANTEED_PERF(hwp_cap) :
-					     HWP_HIGHEST_PERF(hwp_cap);
+	cap_pstate = READ_ONCE(global.no_turbo) ?
+					HWP_GUARANTEED_PERF(hwp_cap) :
+					HWP_HIGHEST_PERF(hwp_cap);
 
 	/* Optimization: Avoid unnecessary divisions. */
 

[PATCH v1 6/6] cpufreq: intel_pstate: Update the maximum CPU frequency consistently

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

There are 3 places at which the maximum CPU frequency may change,
store_no_turbo(), intel_pstate_update_limits() (when called by the
cpufreq core) and intel_pstate_notify_work() (when handling a HWP
change notification).  Currently, cpuinfo.max_freq is only updated by
store_no_turbo(), although it principle it may be necessary to update
it at the other 2 places too.

Make all of them mutually consistent.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/cpufreq/intel_pstate.c |   23 ++++++++++++++++++-----
 1 file changed, 18 insertions(+), 5 deletions(-)

Index: linux-pm/drivers/cpufreq/intel_pstate.c
===================================================================
--- linux-pm.orig/drivers/cpufreq/intel_pstate.c
+++ linux-pm/drivers/cpufreq/intel_pstate.c
@@ -1153,18 +1153,32 @@ static void intel_pstate_update_policies
 static void __intel_pstate_update_max_freq(struct cpudata *cpudata,
 					   struct cpufreq_policy *policy)
 {
+	intel_pstate_get_hwp_cap(cpudata);
+
 	policy->cpuinfo.max_freq = READ_ONCE(global.no_turbo) ?
 			cpudata->pstate.max_freq : cpudata->pstate.turbo_freq;
+
 	refresh_frequency_limits(policy);
 }
 
 static void intel_pstate_update_limits(unsigned int cpu)
 {
-	mutex_lock(&intel_pstate_driver_lock);
+	struct cpufreq_policy *policy = cpufreq_cpu_acquire(cpu);
 
-	cpufreq_update_policy(cpu);
+	if (!policy)
+		return;
 
-	mutex_unlock(&intel_pstate_driver_lock);
+	__intel_pstate_update_max_freq(all_cpu_data[cpu], policy);
+
+	cpufreq_cpu_release(policy);
+}
+
+static void intel_pstate_update_limits_for_all(void)
+{
+	int cpu;
+
+	for_each_possible_cpu(cpu)
+		intel_pstate_update_limits(cpu);
 }
 
 /************************** sysfs begin ************************/
@@ -1311,7 +1325,7 @@ static ssize_t store_no_turbo(struct kob
 
 	mutex_unlock(&intel_pstate_limits_lock);
 
-	intel_pstate_update_policies();
+	intel_pstate_update_limits_for_all();
 	arch_set_max_freq_ratio(no_turbo);
 
 unlock_driver:
@@ -1595,7 +1609,6 @@ static void intel_pstate_notify_work(str
 	struct cpufreq_policy *policy = cpufreq_cpu_acquire(cpudata->cpu);
 
 	if (policy) {
-		intel_pstate_get_hwp_cap(cpudata);
 		__intel_pstate_update_max_freq(cpudata, policy);
 
 		cpufreq_cpu_release(policy);

Re: [PATCH v1 6/6] cpufreq: intel_pstate: Update the maximum CPU frequency consistently

[srinivas pandruvada]

On Mon, 2024-03-25 at 18:06 +0100, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> There are 3 places at which the maximum CPU frequency may change,
> store_no_turbo(), intel_pstate_update_limits() (when called by the
> cpufreq core) and intel_pstate_notify_work() (when handling a HWP
> change notification).  Currently, cpuinfo.max_freq is only updated by
> store_no_turbo(), although it principle it may be necessary to update
> it at the other 2 places too.

It also works for intel_pstate_notify_work() path as is without this
change.

To start with:

$ sudo rdmsr 0x771
6080c14
$ cat /sys/devices/system/cpu/cpu0/cpufreq/cpuinfo_*
2000000
800000
0

Now trigger a max frequency change via SST. intel_pstate_notify_work()
called because guaranteed also changed. We didn't subscribe for max
change only (although we should).

Max changed from 2GHz to 1.9 GHz.

$ sudo rdmsr 0x771
6080e13
[labuser@gnr-bkc ~]$ cat /sys/devices/system/cpu/cpu0/cpufreq/cpuinfo_*
1900000
800000
0

Now trigger SST to change to max frequency to 2GHz.

sudo rdmsr 0x771
6080c14

cat /sys/devices/system/cpu/cpu0/cpufreq/cpuinfo_*
2000000
800000
0

May be you mean something else.

Thanks,
Srinivas


> 
> Make all of them mutually consistent.
> 
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> ---
>  drivers/cpufreq/intel_pstate.c |   23 ++++++++++++++++++-----
>  1 file changed, 18 insertions(+), 5 deletions(-)
> 
> Index: linux-pm/drivers/cpufreq/intel_pstate.c
> ===================================================================
> --- linux-pm.orig/drivers/cpufreq/intel_pstate.c
> +++ linux-pm/drivers/cpufreq/intel_pstate.c
> @@ -1153,18 +1153,32 @@ static void intel_pstate_update_policies
>  static void __intel_pstate_update_max_freq(struct cpudata *cpudata,
>                                            struct cpufreq_policy
> *policy)
>  {
> +       intel_pstate_get_hwp_cap(cpudata);
> +
>         policy->cpuinfo.max_freq = READ_ONCE(global.no_turbo) ?
>                         cpudata->pstate.max_freq : cpudata-
> >pstate.turbo_freq;
> +
>         refresh_frequency_limits(policy);
>  }
>  
>  static void intel_pstate_update_limits(unsigned int cpu)
>  {
> -       mutex_lock(&intel_pstate_driver_lock);
> +       struct cpufreq_policy *policy = cpufreq_cpu_acquire(cpu);
>  
> -       cpufreq_update_policy(cpu);
> +       if (!policy)
> +               return;
>  
> -       mutex_unlock(&intel_pstate_driver_lock);
> +       __intel_pstate_update_max_freq(all_cpu_data[cpu], policy);
> +
> +       cpufreq_cpu_release(policy);
> +}
> +
> +static void intel_pstate_update_limits_for_all(void)
> +{
> +       int cpu;
> +
> +       for_each_possible_cpu(cpu)
> +               intel_pstate_update_limits(cpu);
>  }
>  
>  /************************** sysfs begin ************************/
> @@ -1311,7 +1325,7 @@ static ssize_t store_no_turbo(struct kob
>  
>         mutex_unlock(&intel_pstate_limits_lock);
>  
> -       intel_pstate_update_policies();
> +       intel_pstate_update_limits_for_all();
>         arch_set_max_freq_ratio(no_turbo);
>  
>  unlock_driver:
> @@ -1595,7 +1609,6 @@ static void intel_pstate_notify_work(str
>         struct cpufreq_policy *policy = cpufreq_cpu_acquire(cpudata-
> >cpu);
>  
>         if (policy) {
> -               intel_pstate_get_hwp_cap(cpudata);
>                 __intel_pstate_update_max_freq(cpudata, policy);
>  
>                 cpufreq_cpu_release(policy);
> 
> 
> 

Re: [PATCH v1 6/6] cpufreq: intel_pstate: Update the maximum CPU frequency consistently

[Rafael J. Wysocki]

On Wed, Mar 27, 2024 at 7:08 PM srinivas pandruvada
<srinivas.pandruvada@linux.intel.com> wrote:
>
> On Mon, 2024-03-25 at 18:06 +0100, Rafael J. Wysocki wrote:
> > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> >
> > There are 3 places at which the maximum CPU frequency may change,
> > store_no_turbo(), intel_pstate_update_limits() (when called by the
> > cpufreq core) and intel_pstate_notify_work() (when handling a HWP
> > change notification).  Currently, cpuinfo.max_freq is only updated by
> > store_no_turbo(), although it principle it may be necessary to update
> > it at the other 2 places too.
>
> It also works for intel_pstate_notify_work() path as is without this
> change.
>
> To start with:
>
> $ sudo rdmsr 0x771
> 6080c14
> $ cat /sys/devices/system/cpu/cpu0/cpufreq/cpuinfo_*
> 2000000
> 800000
> 0
>
> Now trigger a max frequency change via SST. intel_pstate_notify_work()
> called because guaranteed also changed. We didn't subscribe for max
> change only (although we should).
>
> Max changed from 2GHz to 1.9 GHz.
>
> $ sudo rdmsr 0x771
> 6080e13
> [labuser@gnr-bkc ~]$ cat /sys/devices/system/cpu/cpu0/cpufreq/cpuinfo_*
> 1900000
> 800000
> 0
>
> Now trigger SST to change to max frequency to 2GHz.
>
> sudo rdmsr 0x771
> 6080c14
>
> cat /sys/devices/system/cpu/cpu0/cpufreq/cpuinfo_*
> 2000000
> 800000
> 0
>
> May be you mean something else.

No, I don't, and you are right.

When I was writing the changelog, I somehow forgot that
intel_pstate_notify_work() called __intel_pstate_update_max_freq(),
even though the code changes in the patch obviously take that into
account (I can't really explain what happened :-/).

I'll fix the changelog.

Cheers,
Rafael

[PATCH v1.1 6/6] cpufreq: intel_pstate: Update the maximum CPU frequency consistently

[Rafael J. Wysocki]

From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

There are 3 places at which the maximum CPU frequency may change,
store_no_turbo(), intel_pstate_update_limits() (when called by the
cpufreq core) and intel_pstate_notify_work() (when handling a HWP
change notification).  Currently, cpuinfo.max_freq is only updated by
store_no_turbo() and intel_pstate_notify_work(), although it principle
it may be necessary to update it in intel_pstate_update_limits() either.

Make all of them mutually consistent.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---

v1 -> v1.1: Fix changelog (Srinivas).

---
 drivers/cpufreq/intel_pstate.c |   23 ++++++++++++++++++-----
 1 file changed, 18 insertions(+), 5 deletions(-)

Index: linux-pm/drivers/cpufreq/intel_pstate.c
===================================================================
--- linux-pm.orig/drivers/cpufreq/intel_pstate.c
+++ linux-pm/drivers/cpufreq/intel_pstate.c
@@ -1153,18 +1153,32 @@ static void intel_pstate_update_policies
 static void __intel_pstate_update_max_freq(struct cpudata *cpudata,
 					   struct cpufreq_policy *policy)
 {
+	intel_pstate_get_hwp_cap(cpudata);
+
 	policy->cpuinfo.max_freq = READ_ONCE(global.no_turbo) ?
 			cpudata->pstate.max_freq : cpudata->pstate.turbo_freq;
+
 	refresh_frequency_limits(policy);
 }
 
 static void intel_pstate_update_limits(unsigned int cpu)
 {
-	mutex_lock(&intel_pstate_driver_lock);
+	struct cpufreq_policy *policy = cpufreq_cpu_acquire(cpu);
 
-	cpufreq_update_policy(cpu);
+	if (!policy)
+		return;
 
-	mutex_unlock(&intel_pstate_driver_lock);
+	__intel_pstate_update_max_freq(all_cpu_data[cpu], policy);
+
+	cpufreq_cpu_release(policy);
+}
+
+static void intel_pstate_update_limits_for_all(void)
+{
+	int cpu;
+
+	for_each_possible_cpu(cpu)
+		intel_pstate_update_limits(cpu);
 }
 
 /************************** sysfs begin ************************/
@@ -1311,7 +1325,7 @@ static ssize_t store_no_turbo(struct kob
 
 	mutex_unlock(&intel_pstate_limits_lock);
 
-	intel_pstate_update_policies();
+	intel_pstate_update_limits_for_all();
 	arch_set_max_freq_ratio(no_turbo);
 
 unlock_driver:
@@ -1595,7 +1609,6 @@ static void intel_pstate_notify_work(str
 	struct cpufreq_policy *policy = cpufreq_cpu_acquire(cpudata->cpu);
 
 	if (policy) {
-		intel_pstate_get_hwp_cap(cpudata);
 		__intel_pstate_update_max_freq(cpudata, policy);
 
 		cpufreq_cpu_release(policy);

Re: [PATCH v1 1/6] cpufreq: intel_pstate: Fold intel_pstate_max_within_limits() into caller

[srinivas pandruvada]

On Mon, 2024-03-25 at 18:01 +0100, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> Fold intel_pstate_max_within_limits() into its only caller.
> 
> No functional impact.
> 
> Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Acked-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>

> ---
>  drivers/cpufreq/intel_pstate.c |   13 ++++---------
>  1 file changed, 4 insertions(+), 9 deletions(-)
> 
> Index: linux-pm/drivers/cpufreq/intel_pstate.c
> ===================================================================
> --- linux-pm.orig/drivers/cpufreq/intel_pstate.c
> +++ linux-pm/drivers/cpufreq/intel_pstate.c
> @@ -2012,14 +2012,6 @@ static void intel_pstate_set_min_pstate(
>         intel_pstate_set_pstate(cpu, cpu->pstate.min_pstate);
>  }
>  
> -static void intel_pstate_max_within_limits(struct cpudata *cpu)
> -{
> -       int pstate = max(cpu->pstate.min_pstate, cpu-
> >max_perf_ratio);
> -
> -       update_turbo_state();
> -       intel_pstate_set_pstate(cpu, pstate);
> -}
> -
>  static void intel_pstate_get_cpu_pstates(struct cpudata *cpu)
>  {
>         int perf_ctl_max_phys = pstate_funcs.get_max_physical(cpu-
> >cpu);
> @@ -2594,12 +2586,15 @@ static int intel_pstate_set_policy(struc
>         intel_pstate_update_perf_limits(cpu, policy->min, policy-
> >max);
>  
>         if (cpu->policy == CPUFREQ_POLICY_PERFORMANCE) {
> +               int pstate = max(cpu->pstate.min_pstate, cpu-
> >max_perf_ratio);
> +
>                 /*
>                  * NOHZ_FULL CPUs need this as the governor callback
> may not
>                  * be invoked on them.
>                  */
>                 intel_pstate_clear_update_util_hook(policy->cpu);
> -               intel_pstate_max_within_limits(cpu);
> +               update_turbo_state();
> +               intel_pstate_set_pstate(cpu, pstate);
>         } else {
>                 intel_pstate_set_update_util_hook(policy->cpu);
>         }
> 
> 
> 

Re: [PATCH v1 0/6] intel_pstate: Turbo disabled handling rework

[srinivas pandruvada]

On Mon, 2024-03-25 at 18:00 +0100, Rafael J. Wysocki wrote:
> Hi Everyone,
> 
> This series reworks the handling of disabling turbo in intel_pstate
> on top of the previous series of cleanups
> 
> https://lore.kernel.org/linux-pm/12409658.O9o76ZdvQC@kreacher/
> 
> The underlying problem is that disabling turbo is handled quite
> consistently
> in intel_pstate and basically it can get disabled at any time
> (through
> MSR_IA32_MISC_ENABLE_TURBO_DISABLE) without much coordination with
> the
> cpufreq core or anything else.
> 
> Disabling turbo through the "no_turbo" sysfs attribute is more
> consistent,
> but it has issues too (for example, if turbo is disabled via
> "no_turbo",
> the frequency-invariance code gets notified on the turbo status
> change,
> but the actual maximum frequency of the CPU is only updated if the
> MSR_IA32_MISC_ENABLE_TURBO_DISABLE value changes either, which need
> not
> happen at the same time or even at all).
> 
> The first patch is not really related to the rest of the series, it's
> just a cleanup and can be applied separately.
> 
> Patch [2/6] uses the observation that it should be necessary to read
> MSR_IA32_MISC_ENABLE_TURBO_DISABLE after driver initialization to
> remove
> in-flight reads on that MSR and turbo state updates related to them.
> 
> Patch [3/6] builds on top of the previous one to adjust the
> "no_turbo"
> attribute "store" and "show" callbacks.
> 
> Patch [4/6] adds READ_ONCE() annotations to global.no_turbo accesses
> and
> makes some related simplifications.
> 
> Patch [5/6] replaces the cached MSR_IA32_MISC_ENABLE_TURBO_DISABLE
> value in some checks with global.no_turbo for consistency.
> 
> Patch [6/6] makes all of the code paths where the maximum CPU
> frequency
> can change to do that consistently by using the same set of
> functions.
> 
> Details are described in the individual patch changelogs.
> 

Acked-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>

Thanks,
Srinivas

> Thanks!
> 
> 
> 
> 

Re: [PATCH v1 2/6] cpufreq: intel_pstate: Do not update global.turbo_disabled after initialization

[Xi Ruoyao]

On Mon, 2024-03-25 at 18:02 +0100, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> 
> The global.turbo_disabled is updated quite often, especially in the
> passive mode in which case it is updated every time the scheduler calls
> into the driver.  However, this is generally not necessary and it adds
> MSR read overhead to scheduler code paths (and that particular MSR is
> slow to read).
> 
> For this reason, make the driver read MSR_IA32_MISC_ENABLE_TURBO_DISABLE
> just once at the cpufreq driver registration time and remove all of the
> in-flight updates of global.turbo_disabled.

Hi Rafael and Srinivas,

Thanks for the clean up, but unfortunately on one of my laptops (based
on i5-11300H) MSR_IA32_MISC_ENABLE_TURBO_DISABLE is mysteriously
changing from 1 to 0 in about one minute after system boot.  I've no
idea why this is happening (firmware is doing some stupid thing?)

I've noticed the issue before and "hacked it around"
(https://bugzilla.kernel.org/show_bug.cgi?id=218702). But after this
change I can no longer hack it around and the system is much slower.

Is it possible to hack it around again?

-- 
Xi Ruoyao <xry111@xry111.site>
School of Aerospace Science and Technology, Xidian University

Re: [PATCH v1 2/6] cpufreq: intel_pstate: Do not update global.turbo_disabled after initialization

[srinivas pandruvada]

[-- Attachment #1: Type: text/plain, Size: 1469 bytes --]

Hi Xi,

On Sun, 2024-06-02 at 11:21 +0800, Xi Ruoyao wrote:
> On Mon, 2024-03-25 at 18:02 +0100, Rafael J. Wysocki wrote:
> > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > 
> > The global.turbo_disabled is updated quite often, especially in the
> > passive mode in which case it is updated every time the scheduler
> > calls
> > into the driver.  However, this is generally not necessary and it
> > adds
> > MSR read overhead to scheduler code paths (and that particular MSR
> > is
> > slow to read).
> > 
> > For this reason, make the driver read
> > MSR_IA32_MISC_ENABLE_TURBO_DISABLE
> > just once at the cpufreq driver registration time and remove all of
> > the
> > in-flight updates of global.turbo_disabled.
> 
> Hi Rafael and Srinivas,
> 
> Thanks for the clean up, but unfortunately on one of my laptops
> (based
> on i5-11300H) MSR_IA32_MISC_ENABLE_TURBO_DISABLE is mysteriously
> changing from 1 to 0 in about one minute after system boot.  I've no
> idea why this is happening (firmware is doing some stupid thing?)
> 
> I've noticed the issue before and "hacked it around"
> (https://bugzilla.kernel.org/show_bug.cgi?id=218702). But after this
> change I can no longer hack it around and the system is much slower.
> 
> Is it possible to hack it around again?
> 
Please try the attached diff and build kernel and try.

git apply update_max_freq.diff

Then build kernel and install.

Thanks,
Srinivas

[-- Attachment #2: update_max_freq.diff --]
[-- Type: text/x-patch, Size: 578 bytes --]

diff --git a/drivers/cpufreq/intel_pstate.c b/drivers/cpufreq/intel_pstate.c
index 4b986c044741..deab96d8d4bf 100644
--- a/drivers/cpufreq/intel_pstate.c
+++ b/drivers/cpufreq/intel_pstate.c
@@ -1168,6 +1168,12 @@ static void intel_pstate_update_limits(unsigned int cpu)
 	if (!policy)
 		return;
 
+	pr_info("%s cpu:%d\n", __func__, cpu);
+
+	global.turbo_disabled = turbo_is_disabled();
+	global.no_turbo = global.turbo_disabled;
+	arch_set_max_freq_ratio(global.turbo_disabled);
+
 	__intel_pstate_update_max_freq(all_cpu_data[cpu], policy);
 
 	cpufreq_cpu_release(policy);

Re: [PATCH v1 2/6] cpufreq: intel_pstate: Do not update global.turbo_disabled after initialization

[Xi Ruoyao]

On Sat, 2024-06-01 at 21:03 -0700, srinivas pandruvada wrote:
> Hi Xi,
> 
> On Sun, 2024-06-02 at 11:21 +0800, Xi Ruoyao wrote:
> > On Mon, 2024-03-25 at 18:02 +0100, Rafael J. Wysocki wrote:
> > > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > > 
> > > The global.turbo_disabled is updated quite often, especially in the
> > > passive mode in which case it is updated every time the scheduler
> > > calls
> > > into the driver.  However, this is generally not necessary and it
> > > adds
> > > MSR read overhead to scheduler code paths (and that particular MSR
> > > is
> > > slow to read).
> > > 
> > > For this reason, make the driver read
> > > MSR_IA32_MISC_ENABLE_TURBO_DISABLE
> > > just once at the cpufreq driver registration time and remove all of
> > > the
> > > in-flight updates of global.turbo_disabled.
> > 
> > Hi Rafael and Srinivas,
> > 
> > Thanks for the clean up, but unfortunately on one of my laptops
> > (based
> > on i5-11300H) MSR_IA32_MISC_ENABLE_TURBO_DISABLE is mysteriously
> > changing from 1 to 0 in about one minute after system boot.  I've no
> > idea why this is happening (firmware is doing some stupid thing?)
> > 
> > I've noticed the issue before and "hacked it around"
> > (https://bugzilla.kernel.org/show_bug.cgi?id=218702). But after this
> > change I can no longer hack it around and the system is much slower.
> > 
> > Is it possible to hack it around again?
> > 
> Please try the attached diff and build kernel and try.
> 
> git apply update_max_freq.diff
> 
> Then build kernel and install.

Unfortunately it didn't work.  Then I tried:

@@ -1304,6 +1310,10 @@ static ssize_t store_no_turbo(struct kobject *a, struct kobj_attribute *b,
 	if (no_turbo == global.no_turbo)
 		goto unlock_driver;
 
+	global.turbo_disabled = turbo_is_disabled();
+	global.no_turbo = global.turbo_disabled;
+	arch_set_max_freq_ratio(global.turbo_disabled);
+
 	if (global.turbo_disabled) {
 		pr_notice_once("Turbo disabled by BIOS or unavailable on processor\n");
 		count = -EPERM;

and my old hack worked again.  Curiously after I writing 0 to
/sys/devices/system/cpu/intel_pstate/no_turbo successfully, your code is
triggered.

$ dmesg | grep intel_pstate
[    0.554425] intel_pstate: Intel P-state driver initializing
[    0.554877] intel_pstate: HWP enabled
[    1.780021] intel_pstate: Turbo disabled by BIOS or unavailable on processor
[   21.789044] intel_pstate: intel_pstate_update_limits cpu:0
[   21.789053] intel_pstate: intel_pstate_update_limits cpu:1
[   21.789060] intel_pstate: intel_pstate_update_limits cpu:2
[   21.789189] intel_pstate: intel_pstate_update_limits cpu:3
[   21.789198] intel_pstate: intel_pstate_update_limits cpu:4
[   21.789203] intel_pstate: intel_pstate_update_limits cpu:5
[   21.789209] intel_pstate: intel_pstate_update_limits cpu:6
[   21.789276] intel_pstate: intel_pstate_update_limits cpu:7

The message at [1.780021] is from the first attempt writing 0 to
/sys/devices/system/cpu/intel_pstate/no_turbo when
MSR_IA32_MISC_ENABLE_TURBO_DISABLE is still 1.

-- 
Xi Ruoyao <xry111@xry111.site>
School of Aerospace Science and Technology, Xidian University

Re: [PATCH v1 2/6] cpufreq: intel_pstate: Do not update global.turbo_disabled after initialization

[srinivas pandruvada]

On Sun, 2024-06-02 at 12:25 +0800, Xi Ruoyao wrote:
> On Sat, 2024-06-01 at 21:03 -0700, srinivas pandruvada wrote:
> > Hi Xi,
> > 
> > On Sun, 2024-06-02 at 11:21 +0800, Xi Ruoyao wrote:
> > > On Mon, 2024-03-25 at 18:02 +0100, Rafael J. Wysocki wrote:
> > > > From: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
> > > > 
> > > > The global.turbo_disabled is updated quite often, especially in
> > > > the
> > > > passive mode in which case it is updated every time the
> > > > scheduler
> > > > calls
> > > > into the driver.  However, this is generally not necessary and
> > > > it
> > > > adds
> > > > MSR read overhead to scheduler code paths (and that particular
> > > > MSR
> > > > is
> > > > slow to read).
> > > > 
> > > > For this reason, make the driver read
> > > > MSR_IA32_MISC_ENABLE_TURBO_DISABLE
> > > > just once at the cpufreq driver registration time and remove
> > > > all of
> > > > the
> > > > in-flight updates of global.turbo_disabled.
> > > 
> > > Hi Rafael and Srinivas,
> > > 
> > > Thanks for the clean up, but unfortunately on one of my laptops
> > > (based
> > > on i5-11300H) MSR_IA32_MISC_ENABLE_TURBO_DISABLE is mysteriously
> > > changing from 1 to 0 in about one minute after system boot.  I've
> > > no
> > > idea why this is happening (firmware is doing some stupid thing?)
> > > 
> > > I've noticed the issue before and "hacked it around"
> > > (https://bugzilla.kernel.org/show_bug.cgi?id=218702). But after
> > > this
> > > change I can no longer hack it around and the system is much
> > > slower.
> > > 
> > > Is it possible to hack it around again?
> > > 
> > Please try the attached diff and build kernel and try.
> > 
> > git apply update_max_freq.diff
> > 
> > Then build kernel and install.
> 
> Unfortunately it didn't work.  Then I tried:
> 
> @@ -1304,6 +1310,10 @@ static ssize_t store_no_turbo(struct kobject
> *a, struct kobj_attribute *b,
>         if (no_turbo == global.no_turbo)
>                 goto unlock_driver;
>  
> +       global.turbo_disabled = turbo_is_disabled();
> +       global.no_turbo = global.turbo_disabled;
> +       arch_set_max_freq_ratio(global.turbo_disabled);
> +
>         if (global.turbo_disabled) {
>                 pr_notice_once("Turbo disabled by BIOS or unavailable
> on processor\n");
>                 count = -EPERM;
> 
> and my old hack worked again.  Curiously after I writing 0 to
> /sys/devices/system/cpu/intel_pstate/no_turbo successfully, your code
> is
> triggered.
> 
> $ dmesg | grep intel_pstate
> [    0.554425] intel_pstate: Intel P-state driver initializing
> [    0.554877] intel_pstate: HWP enabled
> [    1.780021] intel_pstate: Turbo disabled by BIOS or unavailable on
> processor
> [   21.789044] intel_pstate: intel_pstate_update_limits cpu:0
> [   21.789053] intel_pstate: intel_pstate_update_limits cpu:1
> [   21.789060] intel_pstate: intel_pstate_update_limits cpu:2
> [   21.789189] intel_pstate: intel_pstate_update_limits cpu:3
> [   21.789198] intel_pstate: intel_pstate_update_limits cpu:4
> [   21.789203] intel_pstate: intel_pstate_update_limits cpu:5
> [   21.789209] intel_pstate: intel_pstate_update_limits cpu:6
> [   21.789276] intel_pstate: intel_pstate_update_limits cpu:7
> 
> The message at [1.780021] is from the first attempt writing 0 to
> /sys/devices/system/cpu/intel_pstate/no_turbo when
> MSR_IA32_MISC_ENABLE_TURBO_DISABLE is still 1

This requires user action,
Please add a
pr_info() to
https://elixir.bootlin.com/linux/v6.10-rc1/C/ident/acpi_processor_notify

Check if you got any message

Also what is 
cat /proc/cpuinfo
and
cpuid -1

Thanks,
Srinivas

Re: [PATCH v1 2/6] cpufreq: intel_pstate: Do not update global.turbo_disabled after initialization

[Xi Ruoyao]

[-- Attachment #1: Type: text/plain, Size: 899 bytes --]

On Sun, 2024-06-02 at 06:40 -0700, srinivas pandruvada wrote:

/* snip */

> This requires user action,
> Please add a
> pr_info() to
> https://elixir.bootlin.com/linux/v6.10-rc1/C/ident/acpi_processor_notify
> 
> Check if you got any message

With

diff --git a/drivers/acpi/processor_driver.c b/drivers/acpi/processor_driver.c
index 67db60eda370..4585eb6566c8 100644
--- a/drivers/acpi/processor_driver.c
+++ b/drivers/acpi/processor_driver.c
@@ -57,6 +57,8 @@ static void acpi_processor_notify(acpi_handle handle, u32 event, void *data)
 	struct acpi_processor *pr;
 	int saved;
 
+	pr_info("acpi_processor_notify: %d\n", event);
+
 	if (device->handle != handle)
 		return;


I get nothing.

> Also what is 
> cat /proc/cpuinfo
> and
> cpuid -1

Attached.


-- 
Xi Ruoyao <xry111@xry111.site>
School of Aerospace Science and Technology, Xidian University

[-- Attachment #2: cpuid.gz --]
[-- Type: application/gzip, Size: 4390 bytes --]

[-- Attachment #3: cpuinfo.gz --]
[-- Type: application/gzip, Size: 1311 bytes --]

Re: [PATCH v1 2/6] cpufreq: intel_pstate: Do not update global.turbo_disabled after initialization

[srinivas pandruvada]

[-- Attachment #1: Type: text/plain, Size: 1213 bytes --]

On Mon, 2024-06-03 at 00:07 +0800, Xi Ruoyao wrote:
> On Sun, 2024-06-02 at 06:40 -0700, srinivas pandruvada wrote:
> 
> /* snip */
> 
> > This requires user action,
> > Please add a
> > pr_info() to
> > https://elixir.bootlin.com/linux/v6.10-rc1/C/ident/acpi_processor_notify
> > 
> > Check if you got any message
> 
> With
> 
> diff --git a/drivers/acpi/processor_driver.c
> b/drivers/acpi/processor_driver.c
> index 67db60eda370..4585eb6566c8 100644
> --- a/drivers/acpi/processor_driver.c
> +++ b/drivers/acpi/processor_driver.c
> @@ -57,6 +57,8 @@ static void acpi_processor_notify(acpi_handle
> handle, u32 event, void *data)
>  	struct acpi_processor *pr;
>  	int saved;
>  
> +	pr_info("acpi_processor_notify: %d\n", event);
> +
>  	if (device->handle != handle)
>  		return;
> 
> 
> I get nothing.
> 

What is the output of:
grep . /sys/devices/system/cpu/intel_pstate/*

Also 
rdmsr 0x771
rdmsr 0x774


Try these three patches. Don't worry about the commit description for
this issue.

Please send me full dmesg after you see the issue.

Thanks,
Srinivas

> > Also what is 
> > cat /proc/cpuinfo
> > and
> > cpuid -1
> 
> Attached.
> 
> 


[-- Attachment #2: 0003-cpufreq-intel_pstate-Update-turbo-flag-on-HWP-perf-c.patch --]
[-- Type: text/x-patch, Size: 2188 bytes --]

From f5708683877570e2993c9364b84267255f016e7c Mon Sep 17 00:00:00 2001
From: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
Date: Sun, 2 Jun 2024 16:00:37 -0700
Subject: [PATCH 3/3] cpufreq: intel_pstate: Update turbo flag on HWP perf
 change

When HWP performance change, update tuebo flag.

Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
---
 drivers/cpufreq/intel_pstate.c | 12 ++++++++++++
 1 file changed, 12 insertions(+)

diff --git a/drivers/cpufreq/intel_pstate.c b/drivers/cpufreq/intel_pstate.c
index 998b2e8fc2d7..f641c9ed31a3 100644
--- a/drivers/cpufreq/intel_pstate.c
+++ b/drivers/cpufreq/intel_pstate.c
@@ -1168,6 +1168,12 @@ static void intel_pstate_update_limits(unsigned int cpu)
 	if (!policy)
 		return;
 
+	pr_info("turbo status before:%d\n", global.turbo_disabled);
+	global.turbo_disabled = turbo_is_disabled();
+	pr_info("turbo status after:%d\n", global.turbo_disabled);
+	global.no_turbo = global.turbo_disabled;
+	arch_set_max_freq_ratio(global.turbo_disabled);
+
 	__intel_pstate_update_max_freq(all_cpu_data[cpu], policy);
 
 	cpufreq_cpu_release(policy);
@@ -1629,6 +1635,8 @@ void notify_hwp_interrupt(void)
 	u64 value, status_mask;
 	unsigned long flags;
 
+	pr_info("%s HWP interrupt\n", __func__);
+
 	if (!hwp_active || !boot_cpu_has(X86_FEATURE_HWP_NOTIFY))
 		return;
 
@@ -1637,9 +1645,11 @@ void notify_hwp_interrupt(void)
 		status_mask |= HWP_HIGHEST_PERF_CHANGE_STATUS;
 
 	rdmsrl_safe(MSR_HWP_STATUS, &value);
+	pr_info("%s HWP interrupt status:%llx\n", __func__, value);
 	if (!(value & status_mask))
 		return;
 
+
 	spin_lock_irqsave(&hwp_notify_lock, flags);
 
 	if (!cpumask_test_cpu(this_cpu, &hwp_intr_enable_mask))
@@ -1692,6 +1702,8 @@ static void intel_pstate_enable_hwp_interrupt(struct cpudata *cpudata)
 		if (boot_cpu_has(X86_FEATURE_HWP_HIGHEST_PERF_CHANGE))
 			interrupt_mask |= HWP_HIGHEST_PERF_CHANGE_REQ;
 
+		pr_info("%s HWP interrupt mask:%llx\n", __func__, interrupt_mask);
+
 		/* wrmsrl_on_cpu has to be outside spinlock as this can result in IPC */
 		wrmsrl_on_cpu(cpudata->cpu, MSR_HWP_INTERRUPT, interrupt_mask);
 		wrmsrl_on_cpu(cpudata->cpu, MSR_HWP_STATUS, 0);
-- 
2.40.1


[-- Attachment #3: 0002-cpufreq-intel_pstate-Support-highest-performance-cha.patch --]
[-- Type: text/x-patch, Size: 4810 bytes --]

From eaf30416cab758fefcc65cb089ec0796a6730c7d Mon Sep 17 00:00:00 2001
From: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
Date: Wed, 5 Jul 2023 13:02:41 -0700
Subject: [PATCH 2/3] cpufreq: intel_pstate: Support highest performance change
 interrupt

On some systems, HWP highest performance can change from the boot up
value. It leads to two issues:
- cpufreq sysfs cpuinfo_max_freq will not show the highest performance
of the CPU.
- Even if the highest performance of the CPU is increased after boot,
CPU will not reach the full expected performance.

The change in the highest performance can be triggered by Intel Speed
Select Technology-Performance profile feature. Each performance profile
can have different base and max turbo (highest) frequency. When admin
switches to a new performance profile, the firmware sends HWP interrupt
for any change in the guaranteed or highest performance. Admin can also
switch to a new performance profile via BMC (Board management Controller)
from a remote management controller. For more details about technology
refer to:
https://docs.kernel.org/admin-guide/pm/intel-speed-select.html

There are other triggers like over-clocking or dynamic adjustment of
performance limits remotely via BMC to manage power, thermal and
performance.

The support of this feature depends on CPUID[6].EAX[15] = 1. When
supported, MSR_HWP_INTERRUPT BIT(2) enables notification of the highest
performance change. As part of enabling HWP interrupt, also set Bit(2)
of MSR MSR_HWP_INTERRUPT, when this feature is supported.

On highest performance change a new HWP interrupt is generated with
MSR_HWP_STATUS BIT(3) set and MSR_HWP_CAPABILITIES is updated with
a new highest performance limit.

The processing of the interrupt is the same as the guaranteed performance
change. Notify change to cpufreq core and update MSR_HWP_REQUEST with new
performance limits.

The current driver implementation already takes care of the highest
performance change as part of:
commit dfeeedc1bf57 ("cpufreq: intel_pstate: Update cpuinfo.max_freq
on HWP_CAP changes")

For example:
Before highest performance change interrupt:
cat /sys/devices/system/cpu/cpu0/cpufreq/scaling_max_freq
3700000
cat /sys/devices/system/cpu/cpu0/cpufreq/cpuinfo_max_freq
3700000

After highest performance changes interrupt:
cat /sys/devices/system/cpu/cpu0/cpufreq/cpuinfo_max_freq
3900000
cat /sys/devices/system/cpu/cpu0/cpufreq/scaling_max_freq
3900000

Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
---
 drivers/cpufreq/intel_pstate.c | 23 +++++++++++++++++++----
 1 file changed, 19 insertions(+), 4 deletions(-)

diff --git a/drivers/cpufreq/intel_pstate.c b/drivers/cpufreq/intel_pstate.c
index 4b986c044741..998b2e8fc2d7 100644
--- a/drivers/cpufreq/intel_pstate.c
+++ b/drivers/cpufreq/intel_pstate.c
@@ -1620,17 +1620,24 @@ static void intel_pstate_notify_work(struct work_struct *work)
 static DEFINE_SPINLOCK(hwp_notify_lock);
 static cpumask_t hwp_intr_enable_mask;
 
+#define HWP_GUARANTEED_PERF_CHANGE_STATUS      BIT(0)
+#define HWP_HIGHEST_PERF_CHANGE_STATUS         BIT(3)
+
 void notify_hwp_interrupt(void)
 {
 	unsigned int this_cpu = smp_processor_id();
+	u64 value, status_mask;
 	unsigned long flags;
-	u64 value;
 
 	if (!hwp_active || !boot_cpu_has(X86_FEATURE_HWP_NOTIFY))
 		return;
 
+	status_mask = HWP_GUARANTEED_PERF_CHANGE_STATUS;
+	if (boot_cpu_has(X86_FEATURE_HWP_HIGHEST_PERF_CHANGE))
+		status_mask |= HWP_HIGHEST_PERF_CHANGE_STATUS;
+
 	rdmsrl_safe(MSR_HWP_STATUS, &value);
-	if (!(value & 0x01))
+	if (!(value & status_mask))
 		return;
 
 	spin_lock_irqsave(&hwp_notify_lock, flags);
@@ -1668,17 +1675,25 @@ static void intel_pstate_disable_hwp_interrupt(struct cpudata *cpudata)
 		cancel_delayed_work_sync(&cpudata->hwp_notify_work);
 }
 
+#define HWP_GUARANTEED_PERF_CHANGE_REQ BIT(0)
+#define HWP_HIGHEST_PERF_CHANGE_REQ    BIT(2)
+
 static void intel_pstate_enable_hwp_interrupt(struct cpudata *cpudata)
 {
-	/* Enable HWP notification interrupt for guaranteed performance change */
+	/* Enable HWP notification interrupt for performance change */
 	if (boot_cpu_has(X86_FEATURE_HWP_NOTIFY)) {
+		u64 interrupt_mask = HWP_GUARANTEED_PERF_CHANGE_REQ;
+
 		spin_lock_irq(&hwp_notify_lock);
 		INIT_DELAYED_WORK(&cpudata->hwp_notify_work, intel_pstate_notify_work);
 		cpumask_set_cpu(cpudata->cpu, &hwp_intr_enable_mask);
 		spin_unlock_irq(&hwp_notify_lock);
 
+		if (boot_cpu_has(X86_FEATURE_HWP_HIGHEST_PERF_CHANGE))
+			interrupt_mask |= HWP_HIGHEST_PERF_CHANGE_REQ;
+
 		/* wrmsrl_on_cpu has to be outside spinlock as this can result in IPC */
-		wrmsrl_on_cpu(cpudata->cpu, MSR_HWP_INTERRUPT, 0x01);
+		wrmsrl_on_cpu(cpudata->cpu, MSR_HWP_INTERRUPT, interrupt_mask);
 		wrmsrl_on_cpu(cpudata->cpu, MSR_HWP_STATUS, 0);
 	}
 }
-- 
2.40.1


[-- Attachment #4: 0001-x86-cpufeatures-Add-HWP-highest-perf-change-feature-.patch --]
[-- Type: text/x-patch, Size: 1309 bytes --]

From 8043c1f6a4cd6ce5bbbcb1f720065044bb706e13 Mon Sep 17 00:00:00 2001
From: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
Date: Wed, 5 Jul 2023 12:12:49 -0700
Subject: [PATCH 1/3] x86/cpufeatures: Add HWP highest perf change feature flag

When CPUID[6].EAX[15] is set to 1, this CPU supports notification for
HWP (Hardware P-states) highest performance change.

Add a feature flag to check if the CPU supports HWP highest performance
change.

Signed-off-by: Srinivas Pandruvada <srinivas.pandruvada@linux.intel.com>
---
 arch/x86/include/asm/cpufeatures.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/x86/include/asm/cpufeatures.h b/arch/x86/include/asm/cpufeatures.h
index 3c7434329661..4674ba5310b2 100644
--- a/arch/x86/include/asm/cpufeatures.h
+++ b/arch/x86/include/asm/cpufeatures.h
@@ -361,6 +361,7 @@
 #define X86_FEATURE_HWP_ACT_WINDOW	(14*32+ 9) /* HWP Activity Window */
 #define X86_FEATURE_HWP_EPP		(14*32+10) /* HWP Energy Perf. Preference */
 #define X86_FEATURE_HWP_PKG_REQ		(14*32+11) /* HWP Package Level Request */
+#define X86_FEATURE_HWP_HIGHEST_PERF_CHANGE (14*32+15) /* HWP Highest perf change */
 #define X86_FEATURE_HFI			(14*32+19) /* Hardware Feedback Interface */
 
 /* AMD SVM Feature Identification, CPUID level 0x8000000a (EDX), word 15 */
-- 
2.40.1

Re: [PATCH v1 2/6] cpufreq: intel_pstate: Do not update global.turbo_disabled after initialization

[Xi Ruoyao]

[-- Attachment #1: Type: text/plain, Size: 1570 bytes --]

On Sun, 2024-06-02 at 16:11 -0700, srinivas pandruvada wrote:

/* snip */

> What is the output of:
> grep . /sys/devices/system/cpu/intel_pstate/*
> 
> Also 
> rdmsr 0x771
> rdmsr 0x774
> 
> 
> Try these three patches. Don't worry about the commit description for
> this issue.

Unfortunately they still do not fix the issue for me.

The outputs of grep and rdmsr commands are initially:

/sys/devices/system/cpu/intel_pstate/hwp_dynamic_boost:0
/sys/devices/system/cpu/intel_pstate/max_perf_pct:100
/sys/devices/system/cpu/intel_pstate/min_perf_pct:9
/sys/devices/system/cpu/intel_pstate/no_turbo:1
/sys/devices/system/cpu/intel_pstate/num_pstates:41
/sys/devices/system/cpu/intel_pstate/status:active
/sys/devices/system/cpu/intel_pstate/turbo_pct:33
rdmsr 0x771: 10d1f2c
rdmsr 0x774: 1f04

But it then changes to:

/sys/devices/system/cpu/intel_pstate/hwp_dynamic_boost:0
/sys/devices/system/cpu/intel_pstate/max_perf_pct:100
/sys/devices/system/cpu/intel_pstate/min_perf_pct:9
/sys/devices/system/cpu/intel_pstate/no_turbo:1
/sys/devices/system/cpu/intel_pstate/num_pstates:41
/sys/devices/system/cpu/intel_pstate/status:active
/sys/devices/system/cpu/intel_pstate/turbo_pct:33
rdmsr 0x771: 10c1f2c
rdmsr 0x774: 1f04

It seems only the output of rdmsr 0x771 has changed.  And if I read the
SDM correctly it's a "Most_Efficient_Performance" change.

> Please send me full dmesg after you see the issue.

Attached.  

-- 
Xi Ruoyao <xry111@xry111.site>
School of Aerospace Science and Technology, Xidian University

[-- Attachment #2: dmesg.gz --]
[-- Type: application/gzip, Size: 20460 bytes --]

Re: [PATCH v1 2/6] cpufreq: intel_pstate: Do not update global.turbo_disabled after initialization

[srinivas pandruvada]

On Mon, 2024-06-03 at 21:12 +0800, Xi Ruoyao wrote:
> On Sun, 2024-06-02 at 16:11 -0700, srinivas pandruvada wrote:
> 
> /* snip */
> 
> > What is the output of:
> > grep . /sys/devices/system/cpu/intel_pstate/*
> > 
> > Also 
> > rdmsr 0x771
> > rdmsr 0x774
> > 
> > 
> > Try these three patches. Don't worry about the commit description
> > for
> > this issue.
> 
> Unfortunately they still do not fix the issue for me.
> 
> The outputs of grep and rdmsr commands are initially:
> 
> /sys/devices/system/cpu/intel_pstate/hwp_dynamic_boost:0
> /sys/devices/system/cpu/intel_pstate/max_perf_pct:100
> /sys/devices/system/cpu/intel_pstate/min_perf_pct:9
> /sys/devices/system/cpu/intel_pstate/no_turbo:1
> /sys/devices/system/cpu/intel_pstate/num_pstates:41
> /sys/devices/system/cpu/intel_pstate/status:active
> /sys/devices/system/cpu/intel_pstate/turbo_pct:33
> rdmsr 0x771: 10d1f2c
> rdmsr 0x774: 1f04
> 
> But it then changes to:
> 
> /sys/devices/system/cpu/intel_pstate/hwp_dynamic_boost:0
> /sys/devices/system/cpu/intel_pstate/max_perf_pct:100
> /sys/devices/system/cpu/intel_pstate/min_perf_pct:9
> /sys/devices/system/cpu/intel_pstate/no_turbo:1
> /sys/devices/system/cpu/intel_pstate/num_pstates:41
> /sys/devices/system/cpu/intel_pstate/status:active
> /sys/devices/system/cpu/intel_pstate/turbo_pct:33
> rdmsr 0x771: 10c1f2c
> rdmsr 0x774: 1f04
> 
> It seems only the output of rdmsr 0x771 has changed.  And if I read
> the
> SDM correctly it's a "Most_Efficient_Performance" change.
That is fine.

We don't have any notifications either via ACPI or via HWP interrupt.
I think it was working by chance before this change as by the cpufreq
core is trying to set policy, the turbo is enabled by the firmware.

What is this laptop make and model?

Thanks,
Srinivas

> 
> > Please send me full dmesg after you see the issue.
> 
> Attached.  
> 

Re: [PATCH v1 2/6] cpufreq: intel_pstate: Do not update global.turbo_disabled after initialization

[Xi Ruoyao]

On Mon, 2024-06-03 at 10:11 -0700, srinivas pandruvada wrote:
> On Mon, 2024-06-03 at 21:12 +0800, Xi Ruoyao wrote:
> > On Sun, 2024-06-02 at 16:11 -0700, srinivas pandruvada wrote:
> > 
> > /* snip */
> > 
> > > What is the output of:
> > > grep . /sys/devices/system/cpu/intel_pstate/*
> > > 
> > > Also 
> > > rdmsr 0x771
> > > rdmsr 0x774
> > > 
> > > 
> > > Try these three patches. Don't worry about the commit description
> > > for
> > > this issue.
> > 
> > Unfortunately they still do not fix the issue for me.
> > 
> > The outputs of grep and rdmsr commands are initially:
> > 
> > /sys/devices/system/cpu/intel_pstate/hwp_dynamic_boost:0
> > /sys/devices/system/cpu/intel_pstate/max_perf_pct:100
> > /sys/devices/system/cpu/intel_pstate/min_perf_pct:9
> > /sys/devices/system/cpu/intel_pstate/no_turbo:1
> > /sys/devices/system/cpu/intel_pstate/num_pstates:41
> > /sys/devices/system/cpu/intel_pstate/status:active
> > /sys/devices/system/cpu/intel_pstate/turbo_pct:33
> > rdmsr 0x771: 10d1f2c
> > rdmsr 0x774: 1f04
> > 
> > But it then changes to:
> > 
> > /sys/devices/system/cpu/intel_pstate/hwp_dynamic_boost:0
> > /sys/devices/system/cpu/intel_pstate/max_perf_pct:100
> > /sys/devices/system/cpu/intel_pstate/min_perf_pct:9
> > /sys/devices/system/cpu/intel_pstate/no_turbo:1
> > /sys/devices/system/cpu/intel_pstate/num_pstates:41
> > /sys/devices/system/cpu/intel_pstate/status:active
> > /sys/devices/system/cpu/intel_pstate/turbo_pct:33
> > rdmsr 0x771: 10c1f2c
> > rdmsr 0x774: 1f04
> > 
> > It seems only the output of rdmsr 0x771 has changed.  And if I read
> > the
> > SDM correctly it's a "Most_Efficient_Performance" change.
> That is fine.
> 
> We don't have any notifications either via ACPI or via HWP interrupt.
> I think it was working by chance before this change as by the cpufreq
> core is trying to set policy, the turbo is enabled by the firmware.
> 
> What is this laptop make and model?

It's a Hasee X5-2021S5H.

Hasee is known for producing some laptops very cheap but often having
"minor" issues.  So I guess the firmware is doing some stupid thing.

But turbo works just fine on Windows 11 so it'd be better if we could
make it work for Linux too.


-- 
Xi Ruoyao <xry111@xry111.site>
School of Aerospace Science and Technology, Xidian University

Re: [PATCH v1 2/6] cpufreq: intel_pstate: Do not update global.turbo_disabled after initialization

[Rafael J. Wysocki]

On Mon, Jun 3, 2024 at 7:44 PM Xi Ruoyao <xry111@xry111.site> wrote:
>
> On Mon, 2024-06-03 at 10:11 -0700, srinivas pandruvada wrote:
> > On Mon, 2024-06-03 at 21:12 +0800, Xi Ruoyao wrote:
> > > On Sun, 2024-06-02 at 16:11 -0700, srinivas pandruvada wrote:
> > >
> > > /* snip */
> > >
> > > > What is the output of:
> > > > grep . /sys/devices/system/cpu/intel_pstate/*
> > > >
> > > > Also
> > > > rdmsr 0x771
> > > > rdmsr 0x774
> > > >
> > > >
> > > > Try these three patches. Don't worry about the commit description
> > > > for
> > > > this issue.
> > >
> > > Unfortunately they still do not fix the issue for me.
> > >
> > > The outputs of grep and rdmsr commands are initially:
> > >
> > > /sys/devices/system/cpu/intel_pstate/hwp_dynamic_boost:0
> > > /sys/devices/system/cpu/intel_pstate/max_perf_pct:100
> > > /sys/devices/system/cpu/intel_pstate/min_perf_pct:9
> > > /sys/devices/system/cpu/intel_pstate/no_turbo:1
> > > /sys/devices/system/cpu/intel_pstate/num_pstates:41
> > > /sys/devices/system/cpu/intel_pstate/status:active
> > > /sys/devices/system/cpu/intel_pstate/turbo_pct:33
> > > rdmsr 0x771: 10d1f2c
> > > rdmsr 0x774: 1f04
> > >
> > > But it then changes to:
> > >
> > > /sys/devices/system/cpu/intel_pstate/hwp_dynamic_boost:0
> > > /sys/devices/system/cpu/intel_pstate/max_perf_pct:100
> > > /sys/devices/system/cpu/intel_pstate/min_perf_pct:9
> > > /sys/devices/system/cpu/intel_pstate/no_turbo:1
> > > /sys/devices/system/cpu/intel_pstate/num_pstates:41
> > > /sys/devices/system/cpu/intel_pstate/status:active
> > > /sys/devices/system/cpu/intel_pstate/turbo_pct:33
> > > rdmsr 0x771: 10c1f2c
> > > rdmsr 0x774: 1f04
> > >
> > > It seems only the output of rdmsr 0x771 has changed.  And if I read
> > > the
> > > SDM correctly it's a "Most_Efficient_Performance" change.
> > That is fine.
> >
> > We don't have any notifications either via ACPI or via HWP interrupt.
> > I think it was working by chance before this change as by the cpufreq
> > core is trying to set policy, the turbo is enabled by the firmware.
> >
> > What is this laptop make and model?
>
> It's a Hasee X5-2021S5H.
>
> Hasee is known for producing some laptops very cheap but often having
> "minor" issues.  So I guess the firmware is doing some stupid thing.
>
> But turbo works just fine on Windows 11 so it'd be better if we could
> make it work for Linux too.

In principle, there are two things that can be done about this.

First, MSR_IA32_MISC_ENABLE_TURBO_DISABLE on this system can be
ignored altogether, but that would require adding a quirk.

Second, a delayed work can be added to check the MSR long enough after
initialization and update global.turbo_disabled if it is 1.  However,
that would require some code surgery.

Re: [PATCH v1 2/6] cpufreq: intel_pstate: Do not update global.turbo_disabled after initialization

[srinivas pandruvada]

On Mon, 2024-06-03 at 20:32 +0200, Rafael J. Wysocki wrote:
> On Mon, Jun 3, 2024 at 7:44 PM Xi Ruoyao <xry111@xry111.site> wrote:
> > 
> > On Mon, 2024-06-03 at 10:11 -0700, srinivas pandruvada wrote:
> > > On Mon, 2024-06-03 at 21:12 +0800, Xi Ruoyao wrote:
> > > > On Sun, 2024-06-02 at 16:11 -0700, srinivas pandruvada wrote:
> > > > 
> > > > /* snip */
> > > > 
> > > > > What is the output of:
> > > > > grep . /sys/devices/system/cpu/intel_pstate/*
> > > > > 
> > > > > Also
> > > > > rdmsr 0x771
> > > > > rdmsr 0x774
> > > > > 
> > > > > 
> > > > > Try these three patches. Don't worry about the commit
> > > > > description
> > > > > for
> > > > > this issue.
> > > > 
> > > > Unfortunately they still do not fix the issue for me.
> > > > 
> > > > The outputs of grep and rdmsr commands are initially:
> > > > 
> > > > /sys/devices/system/cpu/intel_pstate/hwp_dynamic_boost:0
> > > > /sys/devices/system/cpu/intel_pstate/max_perf_pct:100
> > > > /sys/devices/system/cpu/intel_pstate/min_perf_pct:9
> > > > /sys/devices/system/cpu/intel_pstate/no_turbo:1
> > > > /sys/devices/system/cpu/intel_pstate/num_pstates:41
> > > > /sys/devices/system/cpu/intel_pstate/status:active
> > > > /sys/devices/system/cpu/intel_pstate/turbo_pct:33
> > > > rdmsr 0x771: 10d1f2c
> > > > rdmsr 0x774: 1f04
> > > > 
> > > > But it then changes to:
> > > > 
> > > > /sys/devices/system/cpu/intel_pstate/hwp_dynamic_boost:0
> > > > /sys/devices/system/cpu/intel_pstate/max_perf_pct:100
> > > > /sys/devices/system/cpu/intel_pstate/min_perf_pct:9
> > > > /sys/devices/system/cpu/intel_pstate/no_turbo:1
> > > > /sys/devices/system/cpu/intel_pstate/num_pstates:41
> > > > /sys/devices/system/cpu/intel_pstate/status:active
> > > > /sys/devices/system/cpu/intel_pstate/turbo_pct:33
> > > > rdmsr 0x771: 10c1f2c
> > > > rdmsr 0x774: 1f04
> > > > 
> > > > It seems only the output of rdmsr 0x771 has changed.  And if I
> > > > read
> > > > the
> > > > SDM correctly it's a "Most_Efficient_Performance" change.
> > > That is fine.
> > > 
> > > We don't have any notifications either via ACPI or via HWP
> > > interrupt.
> > > I think it was working by chance before this change as by the
> > > cpufreq
> > > core is trying to set policy, the turbo is enabled by the
> > > firmware.
> > > 
> > > What is this laptop make and model?
> > 
> > It's a Hasee X5-2021S5H.
> > 
> > Hasee is known for producing some laptops very cheap but often
> > having
> > "minor" issues.  So I guess the firmware is doing some stupid
> > thing.
> > 
> > But turbo works just fine on Windows 11 so it'd be better if we
> > could
> > make it work for Linux too.
> 
> In principle, there are two things that can be done about this.
> 
> First, MSR_IA32_MISC_ENABLE_TURBO_DISABLE on this system can be
> ignored altogether, but that would require adding a quirk.
> 
> Second, a delayed work can be added to check the MSR long enough
> after
> initialization and update global.turbo_disabled if it is 1.  However,
> that would require some code surgery.

I was about to send this suggestion.

For the first one we can always program the HWP_REQ.max to HWP_CAP.max
and  let hardware do the clipping. But this is not friendly to passive
mode. But display of scalig_max_freq still should reflect the reality.


Thanks,
Srinivas

> 

Re: [PATCH v1 2/6] cpufreq: intel_pstate: Do not update global.turbo_disabled after initialization

[srinivas pandruvada]

[-- Attachment #1: Type: text/plain, Size: 3740 bytes --]

On Mon, 2024-06-03 at 11:47 -0700, srinivas pandruvada wrote:
> On Mon, 2024-06-03 at 20:32 +0200, Rafael J. Wysocki wrote:
> > On Mon, Jun 3, 2024 at 7:44 PM Xi Ruoyao <xry111@xry111.site>
> > wrote:
> > > 
> > > On Mon, 2024-06-03 at 10:11 -0700, srinivas pandruvada wrote:
> > > > On Mon, 2024-06-03 at 21:12 +0800, Xi Ruoyao wrote:
> > > > > On Sun, 2024-06-02 at 16:11 -0700, srinivas pandruvada wrote:
> > > > > 
> > > > > /* snip */
> > > > > 
> > > > > > What is the output of:
> > > > > > grep . /sys/devices/system/cpu/intel_pstate/*
> > > > > > 
> > > > > > Also
> > > > > > rdmsr 0x771
> > > > > > rdmsr 0x774
> > > > > > 
> > > > > > 
> > > > > > Try these three patches. Don't worry about the commit
> > > > > > description
> > > > > > for
> > > > > > this issue.
> > > > > 
> > > > > Unfortunately they still do not fix the issue for me.
> > > > > 
> > > > > The outputs of grep and rdmsr commands are initially:
> > > > > 
> > > > > /sys/devices/system/cpu/intel_pstate/hwp_dynamic_boost:0
> > > > > /sys/devices/system/cpu/intel_pstate/max_perf_pct:100
> > > > > /sys/devices/system/cpu/intel_pstate/min_perf_pct:9
> > > > > /sys/devices/system/cpu/intel_pstate/no_turbo:1
> > > > > /sys/devices/system/cpu/intel_pstate/num_pstates:41
> > > > > /sys/devices/system/cpu/intel_pstate/status:active
> > > > > /sys/devices/system/cpu/intel_pstate/turbo_pct:33
> > > > > rdmsr 0x771: 10d1f2c
> > > > > rdmsr 0x774: 1f04
> > > > > 
> > > > > But it then changes to:
> > > > > 
> > > > > /sys/devices/system/cpu/intel_pstate/hwp_dynamic_boost:0
> > > > > /sys/devices/system/cpu/intel_pstate/max_perf_pct:100
> > > > > /sys/devices/system/cpu/intel_pstate/min_perf_pct:9
> > > > > /sys/devices/system/cpu/intel_pstate/no_turbo:1
> > > > > /sys/devices/system/cpu/intel_pstate/num_pstates:41
> > > > > /sys/devices/system/cpu/intel_pstate/status:active
> > > > > /sys/devices/system/cpu/intel_pstate/turbo_pct:33
> > > > > rdmsr 0x771: 10c1f2c
> > > > > rdmsr 0x774: 1f04
> > > > > 
> > > > > It seems only the output of rdmsr 0x771 has changed.  And if
> > > > > I
> > > > > read
> > > > > the
> > > > > SDM correctly it's a "Most_Efficient_Performance" change.
> > > > That is fine.
> > > > 
> > > > We don't have any notifications either via ACPI or via HWP
> > > > interrupt.
> > > > I think it was working by chance before this change as by the
> > > > cpufreq
> > > > core is trying to set policy, the turbo is enabled by the
> > > > firmware.
> > > > 
> > > > What is this laptop make and model?
> > > 
> > > It's a Hasee X5-2021S5H.
> > > 
> > > Hasee is known for producing some laptops very cheap but often
> > > having
> > > "minor" issues.  So I guess the firmware is doing some stupid
> > > thing.
> > > 
> > > But turbo works just fine on Windows 11 so it'd be better if we
> > > could
> > > make it work for Linux too.
> > 
> > In principle, there are two things that can be done about this.
> > 
> > First, MSR_IA32_MISC_ENABLE_TURBO_DISABLE on this system can be
> > ignored altogether, but that would require adding a quirk.
> > 
> > Second, a delayed work can be added to check the MSR long enough
> > after
> > initialization and update global.turbo_disabled if it is 1. 
> > However,
> > that would require some code surgery.
> 
Try the attached diff for this case.

Thanks,
Srinivas

> I was about to send this suggestion.
> 
> For the first one we can always program the HWP_REQ.max to
> HWP_CAP.max
> and  let hardware do the clipping. But this is not friendly to
> passive
> mode. But display of scalig_max_freq still should reflect the
> reality.
> 
> 
> Thanks,
> Srinivas
> 
> > 
> 
> 


[-- Attachment #2: delayed_turbo_check.diff --]
[-- Type: text/x-patch, Size: 1067 bytes --]

diff --git a/drivers/cpufreq/intel_pstate.c b/drivers/cpufreq/intel_pstate.c
index 65d3f79104bd..40a8f9d86b59 100644
--- a/drivers/cpufreq/intel_pstate.c
+++ b/drivers/cpufreq/intel_pstate.c
@@ -3102,6 +3102,20 @@ static void intel_pstate_driver_cleanup(void)
 	intel_pstate_driver = NULL;
 }
 
+static void check_turbo_work_handler(struct work_struct *work)
+{
+	global.turbo_disabled = turbo_is_disabled();
+
+	if (global.turbo_disabled)
+		return;
+
+	global.no_turbo = global.turbo_disabled;
+	intel_pstate_update_limits_for_all();
+	arch_set_max_freq_ratio(false);
+}
+
+DECLARE_DELAYED_WORK(turbo_work, check_turbo_work_handler);
+
 static int intel_pstate_register_driver(struct cpufreq_driver *driver)
 {
 	int ret;
@@ -3114,6 +3128,9 @@ static int intel_pstate_register_driver(struct cpufreq_driver *driver)
 	global.turbo_disabled = turbo_is_disabled();
 	global.no_turbo = global.turbo_disabled;
 
+	if (global.turbo_disabled)
+		schedule_delayed_work(&turbo_work, HZ);
+
 	arch_set_max_freq_ratio(global.turbo_disabled);
 
 	intel_pstate_driver = driver;

Re: [PATCH v1 2/6] cpufreq: intel_pstate: Do not update global.turbo_disabled after initialization

[srinivas pandruvada]

[-- Attachment #1: Type: text/plain, Size: 3783 bytes --]

On Mon, 2024-06-03 at 11:47 -0700, srinivas pandruvada wrote:
> On Mon, 2024-06-03 at 20:32 +0200, Rafael J. Wysocki wrote:
> > On Mon, Jun 3, 2024 at 7:44 PM Xi Ruoyao <xry111@xry111.site>
> > wrote:
> > > 
> > > On Mon, 2024-06-03 at 10:11 -0700, srinivas pandruvada wrote:
> > > > On Mon, 2024-06-03 at 21:12 +0800, Xi Ruoyao wrote:
> > > > > On Sun, 2024-06-02 at 16:11 -0700, srinivas pandruvada wrote:
> > > > > 
> > > > > /* snip */
> > > > > 
> > > > > > What is the output of:
> > > > > > grep . /sys/devices/system/cpu/intel_pstate/*
> > > > > > 
> > > > > > Also
> > > > > > rdmsr 0x771
> > > > > > rdmsr 0x774
> > > > > > 
> > > > > > 
> > > > > > Try these three patches. Don't worry about the commit
> > > > > > description
> > > > > > for
> > > > > > this issue.
> > > > > 
> > > > > Unfortunately they still do not fix the issue for me.
> > > > > 
> > > > > The outputs of grep and rdmsr commands are initially:
> > > > > 
> > > > > /sys/devices/system/cpu/intel_pstate/hwp_dynamic_boost:0
> > > > > /sys/devices/system/cpu/intel_pstate/max_perf_pct:100
> > > > > /sys/devices/system/cpu/intel_pstate/min_perf_pct:9
> > > > > /sys/devices/system/cpu/intel_pstate/no_turbo:1
> > > > > /sys/devices/system/cpu/intel_pstate/num_pstates:41
> > > > > /sys/devices/system/cpu/intel_pstate/status:active
> > > > > /sys/devices/system/cpu/intel_pstate/turbo_pct:33
> > > > > rdmsr 0x771: 10d1f2c
> > > > > rdmsr 0x774: 1f04
> > > > > 
> > > > > But it then changes to:
> > > > > 
> > > > > /sys/devices/system/cpu/intel_pstate/hwp_dynamic_boost:0
> > > > > /sys/devices/system/cpu/intel_pstate/max_perf_pct:100
> > > > > /sys/devices/system/cpu/intel_pstate/min_perf_pct:9
> > > > > /sys/devices/system/cpu/intel_pstate/no_turbo:1
> > > > > /sys/devices/system/cpu/intel_pstate/num_pstates:41
> > > > > /sys/devices/system/cpu/intel_pstate/status:active
> > > > > /sys/devices/system/cpu/intel_pstate/turbo_pct:33
> > > > > rdmsr 0x771: 10c1f2c
> > > > > rdmsr 0x774: 1f04
> > > > > 
> > > > > It seems only the output of rdmsr 0x771 has changed.  And if
> > > > > I
> > > > > read
> > > > > the
> > > > > SDM correctly it's a "Most_Efficient_Performance" change.
> > > > That is fine.
> > > > 
> > > > We don't have any notifications either via ACPI or via HWP
> > > > interrupt.
> > > > I think it was working by chance before this change as by the
> > > > cpufreq
> > > > core is trying to set policy, the turbo is enabled by the
> > > > firmware.
> > > > 
> > > > What is this laptop make and model?
> > > 
> > > It's a Hasee X5-2021S5H.
> > > 
> > > Hasee is known for producing some laptops very cheap but often
> > > having
> > > "minor" issues.  So I guess the firmware is doing some stupid
> > > thing.
> > > 
> > > But turbo works just fine on Windows 11 so it'd be better if we
> > > could
> > > make it work for Linux too.
> > 
> > In principle, there are two things that can be done about this.
> > 
> > First, MSR_IA32_MISC_ENABLE_TURBO_DISABLE on this system can be
> > ignored altogether, but that would require adding a quirk.
> > 
> > Second, a delayed work can be added to check the MSR long enough
> > after
> > initialization and update global.turbo_disabled if it is 1. 
> > However,
> > that would require some code surgery.
> 
Something like the attached which does same way as user space no_turbo
update.

Thanks,
Srinivas

> I was about to send this suggestion.
> 
> For the first one we can always program the HWP_REQ.max to
> HWP_CAP.max
> and  let hardware do the clipping. But this is not friendly to
> passive
> mode. But display of scalig_max_freq still should reflect the
> reality.
> 
> 
> Thanks,
> Srinivas
> 
> > 
> 
> 


[-- Attachment #2: delayed_turbo_check.diff --]
[-- Type: text/x-patch, Size: 2202 bytes --]

diff --git a/drivers/cpufreq/intel_pstate.c b/drivers/cpufreq/intel_pstate.c
index 65d3f79104bd..8d61ff261c61 100644
--- a/drivers/cpufreq/intel_pstate.c
+++ b/drivers/cpufreq/intel_pstate.c
@@ -1305,7 +1305,7 @@ static ssize_t store_no_turbo(struct kobject *a, struct kobj_attribute *b,
 	if (no_turbo == global.no_turbo)
 		goto unlock_driver;
 
-	if (global.turbo_disabled) {
+	if (READ_ONCE(global.turbo_disabled)) {
 		pr_notice_once("Turbo disabled by BIOS or unavailable on processor\n");
 		count = -EPERM;
 		goto unlock_driver;
@@ -1762,7 +1762,7 @@ static u64 atom_get_val(struct cpudata *cpudata, int pstate)
 	u32 vid;
 
 	val = (u64)pstate << 8;
-	if (READ_ONCE(global.no_turbo) && !global.turbo_disabled)
+	if (READ_ONCE(global.no_turbo) && !READ_ONCE(global.turbo_disabled))
 		val |= (u64)1 << 32;
 
 	vid_fp = cpudata->vid.min + mul_fp(
@@ -1927,7 +1927,7 @@ static u64 core_get_val(struct cpudata *cpudata, int pstate)
 	u64 val;
 
 	val = (u64)pstate << 8;
-	if (READ_ONCE(global.no_turbo) && !global.turbo_disabled)
+	if (READ_ONCE(global.no_turbo) && !READ_ONCE(global.turbo_disabled))
 		val |= (u64)1 << 32;
 
 	return val;
@@ -3102,6 +3102,29 @@ static void intel_pstate_driver_cleanup(void)
 	intel_pstate_driver = NULL;
 }
 
+static void check_turbo_work_handler(struct work_struct *work)
+{
+	bool no_turbo;
+	
+	no_turbo = turbo_is_disabled();
+
+	if (no_turbo)
+		return;
+
+	/* Same processing as sysfs no_turbo update */
+	mutex_lock(&intel_pstate_driver_lock);
+
+	WRITE_ONCE(global.turbo_disabled, no_turbo);
+	WRITE_ONCE(global.no_turbo, no_turbo);
+
+	intel_pstate_update_limits_for_all();
+	arch_set_max_freq_ratio(false);
+
+	mutex_unlock(&intel_pstate_driver_lock);
+}
+
+DECLARE_DELAYED_WORK(turbo_work, check_turbo_work_handler);
+
 static int intel_pstate_register_driver(struct cpufreq_driver *driver)
 {
 	int ret;
@@ -3114,6 +3137,9 @@ static int intel_pstate_register_driver(struct cpufreq_driver *driver)
 	global.turbo_disabled = turbo_is_disabled();
 	global.no_turbo = global.turbo_disabled;
 
+	if (global.turbo_disabled)
+		schedule_delayed_work(&turbo_work, HZ);
+
 	arch_set_max_freq_ratio(global.turbo_disabled);
 
 	intel_pstate_driver = driver;

Re: [PATCH v1 2/6] cpufreq: intel_pstate: Do not update global.turbo_disabled after initialization

[Xi Ruoyao]

On Mon, 2024-06-03 at 21:31 -0700, srinivas pandruvada wrote:

> > > Second, a delayed work can be added to check the MSR long enough
> > > after
> > > initialization and update global.turbo_disabled if it is 1. 
> > > However,
> > > that would require some code surgery.
> >
> Something like the attached which does same way as user space no_turbo
> update.

>  static int intel_pstate_register_driver(struct cpufreq_driver *driver)
>  {
>  	int ret;
> @@ -3114,6 +3137,9 @@ static int intel_pstate_register_driver(struct cpufreq_driver *driver)
>  	global.turbo_disabled = turbo_is_disabled();
>  	global.no_turbo = global.turbo_disabled;
>  
> +	if (global.turbo_disabled)
> +		schedule_delayed_work(&turbo_work, HZ);
> +

I have to change it to 20 * HZ to make it work for me.  15 * HZ does not
work.

-- 
Xi Ruoyao <xry111@xry111.site>
School of Aerospace Science and Technology, Xidian University

Re: [PATCH v1 2/6] cpufreq: intel_pstate: Do not update global.turbo_disabled after initialization

[srinivas pandruvada]

On Tue, 2024-06-04 at 17:30 +0800, Xi Ruoyao wrote:
> On Mon, 2024-06-03 at 21:31 -0700, srinivas pandruvada wrote:
> 
> > > > Second, a delayed work can be added to check the MSR long
> > > > enough
> > > > after
> > > > initialization and update global.turbo_disabled if it is 1. 
> > > > However,
> > > > that would require some code surgery.
> > > 
> > Something like the attached which does same way as user space
> > no_turbo
> > update.
> 
> >  static int intel_pstate_register_driver(struct cpufreq_driver
> > *driver)
> >  {
> >         int ret;
> > @@ -3114,6 +3137,9 @@ static int
> > intel_pstate_register_driver(struct cpufreq_driver *driver)
> >         global.turbo_disabled = turbo_is_disabled();
> >         global.no_turbo = global.turbo_disabled;
> >  
> > +       if (global.turbo_disabled)
> > +               schedule_delayed_work(&turbo_work, HZ);
> > +
> 
> I have to change it to 20 * HZ to make it work for me.  15 * HZ does
> not
> work.

Is there any consistency or it is changing every time?

Thanks,
Srinivas

> 

Re: [PATCH v1 2/6] cpufreq: intel_pstate: Do not update global.turbo_disabled after initialization

[Xi Ruoyao]

On Tue, 2024-06-04 at 03:29 -0700, srinivas pandruvada wrote:
> On Tue, 2024-06-04 at 17:30 +0800, Xi Ruoyao wrote:
> > On Mon, 2024-06-03 at 21:31 -0700, srinivas pandruvada wrote:
> > 
> > > > > Second, a delayed work can be added to check the MSR long
> > > > > enough
> > > > > after
> > > > > initialization and update global.turbo_disabled if it is 1. 
> > > > > However,
> > > > > that would require some code surgery.
> > > > 
> > > Something like the attached which does same way as user space
> > > no_turbo
> > > update.
> > 
> > >  static int intel_pstate_register_driver(struct cpufreq_driver
> > > *driver)
> > >  {
> > >         int ret;
> > > @@ -3114,6 +3137,9 @@ static int
> > > intel_pstate_register_driver(struct cpufreq_driver *driver)
> > >         global.turbo_disabled = turbo_is_disabled();
> > >         global.no_turbo = global.turbo_disabled;
> > >  
> > > +       if (global.turbo_disabled)
> > > +               schedule_delayed_work(&turbo_work, HZ);
> > > +
> > 
> > I have to change it to 20 * HZ to make it work for me.  15 * HZ does
> > not
> > work.
> 
> Is there any consistency or it is changing every time?

It seems consistent.


-- 
Xi Ruoyao <xry111@xry111.site>
School of Aerospace Science and Technology, Xidian University

Re: [PATCH v1 2/6] cpufreq: intel_pstate: Do not update global.turbo_disabled after initialization

[srinivas pandruvada]

On Tue, 2024-06-04 at 18:32 +0800, Xi Ruoyao wrote:
> On Tue, 2024-06-04 at 03:29 -0700, srinivas pandruvada wrote:
> > On Tue, 2024-06-04 at 17:30 +0800, Xi Ruoyao wrote:
> > > On Mon, 2024-06-03 at 21:31 -0700, srinivas pandruvada wrote:
> > > 
> > > > > > Second, a delayed work can be added to check the MSR long
> > > > > > enough
> > > > > > after
> > > > > > initialization and update global.turbo_disabled if it is 1.
> > > > > > However,
> > > > > > that would require some code surgery.
> > > > > 
> > > > Something like the attached which does same way as user space
> > > > no_turbo
> > > > update.
> > > 
> > > >  static int intel_pstate_register_driver(struct cpufreq_driver
> > > > *driver)
> > > >  {
> > > >         int ret;
> > > > @@ -3114,6 +3137,9 @@ static int
> > > > intel_pstate_register_driver(struct cpufreq_driver *driver)
> > > >         global.turbo_disabled = turbo_is_disabled();
> > > >         global.no_turbo = global.turbo_disabled;
> > > >  
> > > > +       if (global.turbo_disabled)
> > > > +               schedule_delayed_work(&turbo_work, HZ);
> > > > +
> > > 
> > > I have to change it to 20 * HZ to make it work for me.  15 * HZ
> > > does
> > > not
> > > work.
> > 
> > Is there any consistency or it is changing every time?
> 
> It seems consistent.
With such a delay, I am not sure how this even worked before.
Can you revert the patch in question and use kernel dynamic debug
dyndbg="file intel_pstate.c +p" kernel command line and collect log for
30 seconds?

Thanks,
Srinivas

> 
> 

Re: [PATCH v1 2/6] cpufreq: intel_pstate: Do not update global.turbo_disabled after initialization

[Rafael J. Wysocki]

On Tue, Jun 4, 2024 at 6:41 PM srinivas pandruvada
<srinivas.pandruvada@linux.intel.com> wrote:
>
> On Tue, 2024-06-04 at 18:32 +0800, Xi Ruoyao wrote:
> > On Tue, 2024-06-04 at 03:29 -0700, srinivas pandruvada wrote:
> > > On Tue, 2024-06-04 at 17:30 +0800, Xi Ruoyao wrote:
> > > > On Mon, 2024-06-03 at 21:31 -0700, srinivas pandruvada wrote:
> > > >
> > > > > > > Second, a delayed work can be added to check the MSR long
> > > > > > > enough
> > > > > > > after
> > > > > > > initialization and update global.turbo_disabled if it is 1.
> > > > > > > However,
> > > > > > > that would require some code surgery.
> > > > > >
> > > > > Something like the attached which does same way as user space
> > > > > no_turbo
> > > > > update.
> > > >
> > > > >  static int intel_pstate_register_driver(struct cpufreq_driver
> > > > > *driver)
> > > > >  {
> > > > >         int ret;
> > > > > @@ -3114,6 +3137,9 @@ static int
> > > > > intel_pstate_register_driver(struct cpufreq_driver *driver)
> > > > >         global.turbo_disabled = turbo_is_disabled();
> > > > >         global.no_turbo = global.turbo_disabled;
> > > > >
> > > > > +       if (global.turbo_disabled)
> > > > > +               schedule_delayed_work(&turbo_work, HZ);
> > > > > +
> > > >
> > > > I have to change it to 20 * HZ to make it work for me.  15 * HZ
> > > > does
> > > > not
> > > > work.
> > >
> > > Is there any consistency or it is changing every time?
> >
> > It seems consistent.
> With such a delay, I am not sure how this even worked before.
> Can you revert the patch in question and use kernel dynamic debug
> dyndbg="file intel_pstate.c +p" kernel command line and collect log for
> 30 seconds?

I think that it worked because the MSR was read every time
intel_pstate ran, so it got updated at one point and stayed that way.

Re: [PATCH v1 2/6] cpufreq: intel_pstate: Do not update global.turbo_disabled after initialization

[srinivas pandruvada]

On Tue, 2024-06-04 at 18:46 +0200, Rafael J. Wysocki wrote:
> On Tue, Jun 4, 2024 at 6:41 PM srinivas pandruvada
> <srinivas.pandruvada@linux.intel.com> wrote:
> > 
> > On Tue, 2024-06-04 at 18:32 +0800, Xi Ruoyao wrote:
> > > On Tue, 2024-06-04 at 03:29 -0700, srinivas pandruvada wrote:
> > > > On Tue, 2024-06-04 at 17:30 +0800, Xi Ruoyao wrote:
> > > > > On Mon, 2024-06-03 at 21:31 -0700, srinivas pandruvada wrote:
> > > > > 
> > > > > > > > Second, a delayed work can be added to check the MSR
> > > > > > > > long
> > > > > > > > enough
> > > > > > > > after
> > > > > > > > initialization and update global.turbo_disabled if it
> > > > > > > > is 1.
> > > > > > > > However,
> > > > > > > > that would require some code surgery.
> > > > > > > 
> > > > > > Something like the attached which does same way as user
> > > > > > space
> > > > > > no_turbo
> > > > > > update.
> > > > > 
> > > > > >  static int intel_pstate_register_driver(struct
> > > > > > cpufreq_driver
> > > > > > *driver)
> > > > > >  {
> > > > > >         int ret;
> > > > > > @@ -3114,6 +3137,9 @@ static int
> > > > > > intel_pstate_register_driver(struct cpufreq_driver *driver)
> > > > > >         global.turbo_disabled = turbo_is_disabled();
> > > > > >         global.no_turbo = global.turbo_disabled;
> > > > > > 
> > > > > > +       if (global.turbo_disabled)
> > > > > > +               schedule_delayed_work(&turbo_work, HZ);
> > > > > > +
> > > > > 
> > > > > I have to change it to 20 * HZ to make it work for me.  15 *
> > > > > HZ
> > > > > does
> > > > > not
> > > > > work.
> > > > 
> > > > Is there any consistency or it is changing every time?
> > > 
> > > It seems consistent.
> > With such a delay, I am not sure how this even worked before.
> > Can you revert the patch in question and use kernel dynamic debug
> > dyndbg="file intel_pstate.c +p" kernel command line and collect log
> > for
> > 30 seconds?
> 
> I think that it worked because the MSR was read every time
> intel_pstate ran, so it got updated at one point and stayed that way.

But here HWP in active mode is getting used. So it should have fewer
request calls to set accept via cpufreq set_policy()

 callback or with some HWP interrupt.

Re: [PATCH v1 2/6] cpufreq: intel_pstate: Do not update global.turbo_disabled after initialization

[Xi Ruoyao]

[-- Attachment #1: Type: text/plain, Size: 6035 bytes --]

On Tue, 2024-06-04 at 09:56 -0700, srinivas pandruvada wrote:
> > > With such a delay, I am not sure how this even worked before.

It didn't work out of box but it worked after manually writing 0 to
no_turbo after 20 seconds, see
https://bugzilla.kernel.org/show_bug.cgi?id=218702.

> > > Can you revert the patch in question and use kernel dynamic debug
> > > dyndbg="file intel_pstate.c +p" kernel command line and collect log
> > > for
> > > 30 seconds?

Attached.  The related part seems:

[    0.553606] intel_pstate: Intel P-state driver initializing
[    0.553630] intel_pstate: controlling: cpu 0
[    0.553640] intel_pstate: set_policy cpuinfo.max 3100000 policy->max 3100000
[    0.553642] intel_pstate: cpu:0 min_policy_perf:4 max_policy_perf:31
[    0.553643] intel_pstate: cpu:0 global_min:0 global_max:44
[    0.553644] intel_pstate: cpu:0 max_perf_ratio:31 min_perf_ratio:4
[    0.553680] intel_pstate: controlling: cpu 1
[    0.553702] intel_pstate: set_policy cpuinfo.max 3100000 policy->max 3100000
[    0.553703] intel_pstate: cpu:1 min_policy_perf:4 max_policy_perf:31
[    0.553704] intel_pstate: cpu:1 global_min:0 global_max:44
[    0.553705] intel_pstate: cpu:1 max_perf_ratio:31 min_perf_ratio:4
[    0.553742] intel_pstate: controlling: cpu 2
[    0.553763] intel_pstate: set_policy cpuinfo.max 3100000 policy->max 3100000
[    0.553764] intel_pstate: cpu:2 min_policy_perf:4 max_policy_perf:31
[    0.553765] intel_pstate: cpu:2 global_min:0 global_max:44
[    0.553766] intel_pstate: cpu:2 max_perf_ratio:31 min_perf_ratio:4
[    0.553809] intel_pstate: controlling: cpu 3
[    0.553830] intel_pstate: set_policy cpuinfo.max 3100000 policy->max 3100000
[    0.553831] intel_pstate: cpu:3 min_policy_perf:4 max_policy_perf:31
[    0.553831] intel_pstate: cpu:3 global_min:0 global_max:44
[    0.553832] intel_pstate: cpu:3 max_perf_ratio:31 min_perf_ratio:4
[    0.553868] intel_pstate: controlling: cpu 4
[    0.553892] intel_pstate: set_policy cpuinfo.max 3100000 policy->max 3100000
[    0.553893] intel_pstate: cpu:4 min_policy_perf:4 max_policy_perf:31
[    0.553894] intel_pstate: cpu:4 global_min:0 global_max:44
[    0.553895] intel_pstate: cpu:4 max_perf_ratio:31 min_perf_ratio:4
[    0.553943] intel_pstate: controlling: cpu 5
[    0.553967] intel_pstate: set_policy cpuinfo.max 3100000 policy->max 3100000
[    0.553968] intel_pstate: cpu:5 min_policy_perf:4 max_policy_perf:31
[    0.553968] intel_pstate: cpu:5 global_min:0 global_max:44
[    0.553969] intel_pstate: cpu:5 max_perf_ratio:31 min_perf_ratio:4
[    0.554009] intel_pstate: controlling: cpu 6
[    0.554034] intel_pstate: set_policy cpuinfo.max 3100000 policy->max 3100000
[    0.554035] intel_pstate: cpu:6 min_policy_perf:4 max_policy_perf:31
[    0.554036] intel_pstate: cpu:6 global_min:0 global_max:44
[    0.554037] intel_pstate: cpu:6 max_perf_ratio:31 min_perf_ratio:4
[    0.554077] intel_pstate: controlling: cpu 7
[    0.554098] intel_pstate: set_policy cpuinfo.max 3100000 policy->max 3100000
[    0.554099] intel_pstate: cpu:7 min_policy_perf:4 max_policy_perf:31
[    0.554099] intel_pstate: cpu:7 global_min:0 global_max:44
[    0.554100] intel_pstate: cpu:7 max_perf_ratio:31 min_perf_ratio:4
[    0.554104] intel_pstate: HWP enabled
[    2.183669] intel_pstate: set_policy cpuinfo.max 3100000 policy->max 3100000
[    2.183675] intel_pstate: cpu:6 min_policy_perf:4 max_policy_perf:31
[    2.183677] intel_pstate: cpu:6 global_min:4 global_max:44
[    2.183679] intel_pstate: cpu:6 max_perf_ratio:31 min_perf_ratio:4
[    2.183710] intel_pstate: set_policy cpuinfo.max 3100000 policy->max 3100000
[    2.183711] intel_pstate: cpu:4 min_policy_perf:4 max_policy_perf:31
[    2.183713] intel_pstate: cpu:4 global_min:4 global_max:44
[    2.183715] intel_pstate: cpu:4 max_perf_ratio:31 min_perf_ratio:4
[    2.183742] intel_pstate: set_policy cpuinfo.max 3100000 policy->max 3100000
[    2.183744] intel_pstate: cpu:2 min_policy_perf:4 max_policy_perf:31
[    2.183745] intel_pstate: cpu:2 global_min:4 global_max:44
[    2.183747] intel_pstate: cpu:2 max_perf_ratio:31 min_perf_ratio:4
[    2.183773] intel_pstate: set_policy cpuinfo.max 3100000 policy->max 3100000
[    2.183775] intel_pstate: cpu:0 min_policy_perf:4 max_policy_perf:31
[    2.183776] intel_pstate: cpu:0 global_min:4 global_max:44
[    2.183777] intel_pstate: cpu:0 max_perf_ratio:31 min_perf_ratio:4
[    2.183803] intel_pstate: set_policy cpuinfo.max 3100000 policy->max 3100000
[    2.183805] intel_pstate: cpu:7 min_policy_perf:4 max_policy_perf:31
[    2.183806] intel_pstate: cpu:7 global_min:4 global_max:44
[    2.183807] intel_pstate: cpu:7 max_perf_ratio:31 min_perf_ratio:4
[    2.183831] intel_pstate: set_policy cpuinfo.max 3100000 policy->max 3100000
[    2.183833] intel_pstate: cpu:5 min_policy_perf:4 max_policy_perf:31
[    2.183834] intel_pstate: cpu:5 global_min:4 global_max:44
[    2.183836] intel_pstate: cpu:5 max_perf_ratio:31 min_perf_ratio:4
[    2.183862] intel_pstate: set_policy cpuinfo.max 3100000 policy->max 3100000
[    2.183864] intel_pstate: cpu:3 min_policy_perf:4 max_policy_perf:31
[    2.183865] intel_pstate: cpu:3 global_min:4 global_max:44
[    2.183867] intel_pstate: cpu:3 max_perf_ratio:31 min_perf_ratio:4
[    2.183893] intel_pstate: set_policy cpuinfo.max 3100000 policy->max 3100000
[    2.183895] intel_pstate: cpu:1 min_policy_perf:4 max_policy_perf:31
[    2.183896] intel_pstate: cpu:1 global_min:4 global_max:44
[    2.183898] intel_pstate: cpu:1 max_perf_ratio:31 min_perf_ratio:4

> > I think that it worked because the MSR was read every time
> > intel_pstate ran, so it got updated at one point and stayed that way.
> 
> But here HWP in active mode is getting used. So it should have fewer
> request calls to set accept via cpufreq set_policy()
> 
>  callback or with some HWP interrupt.

-- 
Xi Ruoyao <xry111@xry111.site>
School of Aerospace Science and Technology, Xidian University

[-- Attachment #2: dmesg.gz --]
[-- Type: application/gzip, Size: 21506 bytes --]

Re: [PATCH v1 2/6] cpufreq: intel_pstate: Do not update global.turbo_disabled after initialization

[srinivas pandruvada]

On Wed, 2024-06-05 at 13:21 +0800, Xi Ruoyao wrote:
> On Tue, 2024-06-04 at 09:56 -0700, srinivas pandruvada wrote:
> > > > With such a delay, I am not sure how this even worked before.
> 
> It didn't work out of box but it worked after manually writing 0 to
> no_turbo after 20 seconds, see
> https://bugzilla.kernel.org/show_bug.cgi?id=218702.

That make sense. So it never worked out of box. The store_no_turbo()
has additional read for turbo flag before, which is removed now. I
think adding that back will will restore old behavior.

diff --git a/drivers/cpufreq/intel_pstate.c
b/drivers/cpufreq/intel_pstate.c
index 4b986c044741..0d5330e5b96b 100644
--- a/drivers/cpufreq/intel_pstate.c
+++ b/drivers/cpufreq/intel_pstate.c
@@ -1301,6 +1301,8 @@ static ssize_t store_no_turbo(struct kobject *a,
struct kobj_attribute *b,
 
        no_turbo = !!clamp_t(int, input, 0, 1);
 
+       global.turbo_disabled = turbo_is_disabled();
+
        if (no_turbo == global.no_turbo)
                goto unlock_driver;


Need to adjust the mutex around it also.

Regarding the bugzilla, I disabled turbo and no_turbo shows 1. Not sure
how it shows "0" to you.

Thanks,
Srinivas


> 
> > > > Can you revert the patch in question and use kernel dynamic
> > > > debug
> > > > dyndbg="file intel_pstate.c +p" kernel command line and collect
> > > > log
> > > > for
> > > > 30 seconds?
> 
> Attached.  The related part seems:
> 
> [    0.553606] intel_pstate: Intel P-state driver initializing
> [    0.553630] intel_pstate: controlling: cpu 0
> [    0.553640] intel_pstate: set_policy cpuinfo.max 3100000 policy-
> >max 3100000
> [    0.553642] intel_pstate: cpu:0 min_policy_perf:4
> max_policy_perf:31
> [    0.553643] intel_pstate: cpu:0 global_min:0 global_max:44
> [    0.553644] intel_pstate: cpu:0 max_perf_ratio:31 min_perf_ratio:4
> [    0.553680] intel_pstate: controlling: cpu 1
> [    0.553702] intel_pstate: set_policy cpuinfo.max 3100000 policy-
> >max 3100000
> [    0.553703] intel_pstate: cpu:1 min_policy_perf:4
> max_policy_perf:31
> [    0.553704] intel_pstate: cpu:1 global_min:0 global_max:44
> [    0.553705] intel_pstate: cpu:1 max_perf_ratio:31 min_perf_ratio:4
> [    0.553742] intel_pstate: controlling: cpu 2
> [    0.553763] intel_pstate: set_policy cpuinfo.max 3100000 policy-
> >max 3100000
> [    0.553764] intel_pstate: cpu:2 min_policy_perf:4
> max_policy_perf:31
> [    0.553765] intel_pstate: cpu:2 global_min:0 global_max:44
> [    0.553766] intel_pstate: cpu:2 max_perf_ratio:31 min_perf_ratio:4
> [    0.553809] intel_pstate: controlling: cpu 3
> [    0.553830] intel_pstate: set_policy cpuinfo.max 3100000 policy-
> >max 3100000
> [    0.553831] intel_pstate: cpu:3 min_policy_perf:4
> max_policy_perf:31
> [    0.553831] intel_pstate: cpu:3 global_min:0 global_max:44
> [    0.553832] intel_pstate: cpu:3 max_perf_ratio:31 min_perf_ratio:4
> [    0.553868] intel_pstate: controlling: cpu 4
> [    0.553892] intel_pstate: set_policy cpuinfo.max 3100000 policy-
> >max 3100000
> [    0.553893] intel_pstate: cpu:4 min_policy_perf:4
> max_policy_perf:31
> [    0.553894] intel_pstate: cpu:4 global_min:0 global_max:44
> [    0.553895] intel_pstate: cpu:4 max_perf_ratio:31 min_perf_ratio:4
> [    0.553943] intel_pstate: controlling: cpu 5
> [    0.553967] intel_pstate: set_policy cpuinfo.max 3100000 policy-
> >max 3100000
> [    0.553968] intel_pstate: cpu:5 min_policy_perf:4
> max_policy_perf:31
> [    0.553968] intel_pstate: cpu:5 global_min:0 global_max:44
> [    0.553969] intel_pstate: cpu:5 max_perf_ratio:31 min_perf_ratio:4
> [    0.554009] intel_pstate: controlling: cpu 6
> [    0.554034] intel_pstate: set_policy cpuinfo.max 3100000 policy-
> >max 3100000
> [    0.554035] intel_pstate: cpu:6 min_policy_perf:4
> max_policy_perf:31
> [    0.554036] intel_pstate: cpu:6 global_min:0 global_max:44
> [    0.554037] intel_pstate: cpu:6 max_perf_ratio:31 min_perf_ratio:4
> [    0.554077] intel_pstate: controlling: cpu 7
> [    0.554098] intel_pstate: set_policy cpuinfo.max 3100000 policy-
> >max 3100000
> [    0.554099] intel_pstate: cpu:7 min_policy_perf:4
> max_policy_perf:31
> [    0.554099] intel_pstate: cpu:7 global_min:0 global_max:44
> [    0.554100] intel_pstate: cpu:7 max_perf_ratio:31 min_perf_ratio:4
> [    0.554104] intel_pstate: HWP enabled
> [    2.183669] intel_pstate: set_policy cpuinfo.max 3100000 policy-
> >max 3100000
> [    2.183675] intel_pstate: cpu:6 min_policy_perf:4
> max_policy_perf:31
> [    2.183677] intel_pstate: cpu:6 global_min:4 global_max:44
> [    2.183679] intel_pstate: cpu:6 max_perf_ratio:31 min_perf_ratio:4
> [    2.183710] intel_pstate: set_policy cpuinfo.max 3100000 policy-
> >max 3100000
> [    2.183711] intel_pstate: cpu:4 min_policy_perf:4
> max_policy_perf:31
> [    2.183713] intel_pstate: cpu:4 global_min:4 global_max:44
> [    2.183715] intel_pstate: cpu:4 max_perf_ratio:31 min_perf_ratio:4
> [    2.183742] intel_pstate: set_policy cpuinfo.max 3100000 policy-
> >max 3100000
> [    2.183744] intel_pstate: cpu:2 min_policy_perf:4
> max_policy_perf:31
> [    2.183745] intel_pstate: cpu:2 global_min:4 global_max:44
> [    2.183747] intel_pstate: cpu:2 max_perf_ratio:31 min_perf_ratio:4
> [    2.183773] intel_pstate: set_policy cpuinfo.max 3100000 policy-
> >max 3100000
> [    2.183775] intel_pstate: cpu:0 min_policy_perf:4
> max_policy_perf:31
> [    2.183776] intel_pstate: cpu:0 global_min:4 global_max:44
> [    2.183777] intel_pstate: cpu:0 max_perf_ratio:31 min_perf_ratio:4
> [    2.183803] intel_pstate: set_policy cpuinfo.max 3100000 policy-
> >max 3100000
> [    2.183805] intel_pstate: cpu:7 min_policy_perf:4
> max_policy_perf:31
> [    2.183806] intel_pstate: cpu:7 global_min:4 global_max:44
> [    2.183807] intel_pstate: cpu:7 max_perf_ratio:31 min_perf_ratio:4
> [    2.183831] intel_pstate: set_policy cpuinfo.max 3100000 policy-
> >max 3100000
> [    2.183833] intel_pstate: cpu:5 min_policy_perf:4
> max_policy_perf:31
> [    2.183834] intel_pstate: cpu:5 global_min:4 global_max:44
> [    2.183836] intel_pstate: cpu:5 max_perf_ratio:31 min_perf_ratio:4
> [    2.183862] intel_pstate: set_policy cpuinfo.max 3100000 policy-
> >max 3100000
> [    2.183864] intel_pstate: cpu:3 min_policy_perf:4
> max_policy_perf:31
> [    2.183865] intel_pstate: cpu:3 global_min:4 global_max:44
> [    2.183867] intel_pstate: cpu:3 max_perf_ratio:31 min_perf_ratio:4
> [    2.183893] intel_pstate: set_policy cpuinfo.max 3100000 policy-
> >max 3100000
> [    2.183895] intel_pstate: cpu:1 min_policy_perf:4
> max_policy_perf:31
> [    2.183896] intel_pstate: cpu:1 global_min:4 global_max:44
> [    2.183898] intel_pstate: cpu:1 max_perf_ratio:31 min_perf_ratio:4
> 
> > > I think that it worked because the MSR was read every time
> > > intel_pstate ran, so it got updated at one point and stayed that
> > > way.
> > 
> > But here HWP in active mode is getting used. So it should have
> > fewer
> > request calls to set accept via cpufreq set_policy()
> > 
> >  callback or with some HWP interrupt.
> 

Re: [PATCH v1 2/6] cpufreq: intel_pstate: Do not update global.turbo_disabled after initialization

[Rafael J. Wysocki]

[-- Attachment #1: Type: text/plain, Size: 1527 bytes --]

On Wed, Jun 5, 2024 at 2:05 PM srinivas pandruvada
<srinivas.pandruvada@linux.intel.com> wrote:
>
> On Wed, 2024-06-05 at 13:21 +0800, Xi Ruoyao wrote:
> > On Tue, 2024-06-04 at 09:56 -0700, srinivas pandruvada wrote:
> > > > > With such a delay, I am not sure how this even worked before.
> >
> > It didn't work out of box but it worked after manually writing 0 to
> > no_turbo after 20 seconds, see
> > https://bugzilla.kernel.org/show_bug.cgi?id=218702.
>
> That make sense. So it never worked out of box. The store_no_turbo()
> has additional read for turbo flag before, which is removed now. I
> think adding that back will will restore old behavior.
>
> diff --git a/drivers/cpufreq/intel_pstate.c
> b/drivers/cpufreq/intel_pstate.c
> index 4b986c044741..0d5330e5b96b 100644
> --- a/drivers/cpufreq/intel_pstate.c
> +++ b/drivers/cpufreq/intel_pstate.c
> @@ -1301,6 +1301,8 @@ static ssize_t store_no_turbo(struct kobject *a,
> struct kobj_attribute *b,
>
>         no_turbo = !!clamp_t(int, input, 0, 1);
>
> +       global.turbo_disabled = turbo_is_disabled();
> +
>         if (no_turbo == global.no_turbo)
>                 goto unlock_driver;
>
>
> Need to adjust the mutex around it also.

Anyhow, it can be made work.

global.turbo_disabled can be updated right before it is checked in
store_no_turbo(), so if 0 is written to no_turbo (and global.no_turbo
is 1), it will succeed if global.turbo_disabled changes from 1 to 0.

Something like the attached (untested) patch.

[-- Attachment #2: intel_pstate-turbo_disabled.patch --]
[-- Type: text/x-patch, Size: 1506 bytes --]

---
 drivers/cpufreq/intel_pstate.c |   19 ++++++++++++-------
 1 file changed, 12 insertions(+), 7 deletions(-)

Index: linux-pm/drivers/cpufreq/intel_pstate.c
===================================================================
--- linux-pm.orig/drivers/cpufreq/intel_pstate.c
+++ linux-pm/drivers/cpufreq/intel_pstate.c
@@ -1302,12 +1302,17 @@ static ssize_t store_no_turbo(struct kob
 
 	no_turbo = !!clamp_t(int, input, 0, 1);
 
-	if (no_turbo == global.no_turbo)
-		goto unlock_driver;
-
-	if (global.turbo_disabled) {
-		pr_notice_once("Turbo disabled by BIOS or unavailable on processor\n");
+	WRITE_ONCE(global.turbo_disabled, turbo_is_disabled());
+	if (global.turbo_disabled && !no_turbo) {
+		pr_notice("Turbo disabled by BIOS or unavailable on processor\n");
 		count = -EPERM;
+		if (global.no_turbo)
+			goto unlock_driver;
+		else
+			no_turbo = 1;
+	}
+
+	if (no_turbo == global.no_turbo) {
 		goto unlock_driver;
 	}
 
@@ -1762,7 +1767,7 @@ static u64 atom_get_val(struct cpudata *
 	u32 vid;
 
 	val = (u64)pstate << 8;
-	if (READ_ONCE(global.no_turbo) && !global.turbo_disabled)
+	if (READ_ONCE(global.no_turbo) && !READ_ONCE(global.turbo_disabled))
 		val |= (u64)1 << 32;
 
 	vid_fp = cpudata->vid.min + mul_fp(
@@ -1927,7 +1932,7 @@ static u64 core_get_val(struct cpudata *
 	u64 val;
 
 	val = (u64)pstate << 8;
-	if (READ_ONCE(global.no_turbo) && !global.turbo_disabled)
+	if (READ_ONCE(global.no_turbo) && !READ_ONCE(global.turbo_disabled))
 		val |= (u64)1 << 32;
 
 	return val;

Re: [PATCH v1 2/6] cpufreq: intel_pstate: Do not update global.turbo_disabled after initialization

[srinivas pandruvada]

On Fri, 2024-06-07 at 17:04 +0200, Rafael J. Wysocki wrote:
> On Wed, Jun 5, 2024 at 2:05 PM srinivas pandruvada
> <srinivas.pandruvada@linux.intel.com> wrote:
> > 
> > On Wed, 2024-06-05 at 13:21 +0800, Xi Ruoyao wrote:
> > > On Tue, 2024-06-04 at 09:56 -0700, srinivas pandruvada wrote:
> > > > > > With such a delay, I am not sure how this even worked
> > > > > > before.
> > > 
> > > It didn't work out of box but it worked after manually writing 0
> > > to
> > > no_turbo after 20 seconds, see
> > > https://bugzilla.kernel.org/show_bug.cgi?id=218702.
> > 
> > That make sense. So it never worked out of box. The
> > store_no_turbo()
> > has additional read for turbo flag before, which is removed now. I
> > think adding that back will will restore old behavior.
> > 
> > diff --git a/drivers/cpufreq/intel_pstate.c
> > b/drivers/cpufreq/intel_pstate.c
> > index 4b986c044741..0d5330e5b96b 100644
> > --- a/drivers/cpufreq/intel_pstate.c
> > +++ b/drivers/cpufreq/intel_pstate.c
> > @@ -1301,6 +1301,8 @@ static ssize_t store_no_turbo(struct kobject
> > *a,
> > struct kobj_attribute *b,
> > 
> >         no_turbo = !!clamp_t(int, input, 0, 1);
> > 
> > +       global.turbo_disabled = turbo_is_disabled();
> > +
> >         if (no_turbo == global.no_turbo)
> >                 goto unlock_driver;
> > 
> > 
> > Need to adjust the mutex around it also.
> 
> Anyhow, it can be made work.
> 
> global.turbo_disabled can be updated right before it is checked in
> store_no_turbo(), so if 0 is written to no_turbo (and global.no_turbo
> is 1), it will succeed if global.turbo_disabled changes from 1 to 0.
> 
> Something like the attached (untested) patch.

Should work.

Xi,
Please test so that we can close this issue.

Thanks,
Srinivas

Re: [PATCH v1 2/6] cpufreq: intel_pstate: Do not update global.turbo_disabled after initialization

[Xi Ruoyao]

On Fri, 2024-06-07 at 08:18 -0700, srinivas pandruvada wrote:
> On Fri, 2024-06-07 at 17:04 +0200, Rafael J. Wysocki wrote:
> > On Wed, Jun 5, 2024 at 2:05 PM srinivas pandruvada
> > <srinivas.pandruvada@linux.intel.com> wrote:
> > > 
> > > On Wed, 2024-06-05 at 13:21 +0800, Xi Ruoyao wrote:
> > > > On Tue, 2024-06-04 at 09:56 -0700, srinivas pandruvada wrote:
> > > > > > > With such a delay, I am not sure how this even worked
> > > > > > > before.
> > > > 
> > > > It didn't work out of box but it worked after manually writing 0
> > > > to
> > > > no_turbo after 20 seconds, see
> > > > https://bugzilla.kernel.org/show_bug.cgi?id=218702.
> > > 
> > > That make sense. So it never worked out of box. The
> > > store_no_turbo()
> > > has additional read for turbo flag before, which is removed now. I
> > > think adding that back will will restore old behavior.
> > > 
> > > diff --git a/drivers/cpufreq/intel_pstate.c
> > > b/drivers/cpufreq/intel_pstate.c
> > > index 4b986c044741..0d5330e5b96b 100644
> > > --- a/drivers/cpufreq/intel_pstate.c
> > > +++ b/drivers/cpufreq/intel_pstate.c
> > > @@ -1301,6 +1301,8 @@ static ssize_t store_no_turbo(struct kobject
> > > *a,
> > > struct kobj_attribute *b,
> > > 
> > >         no_turbo = !!clamp_t(int, input, 0, 1);
> > > 
> > > +       global.turbo_disabled = turbo_is_disabled();
> > > +
> > >         if (no_turbo == global.no_turbo)
> > >                 goto unlock_driver;
> > > 
> > > 
> > > Need to adjust the mutex around it also.
> > 
> > Anyhow, it can be made work.
> > 
> > global.turbo_disabled can be updated right before it is checked in
> > store_no_turbo(), so if 0 is written to no_turbo (and global.no_turbo
> > is 1), it will succeed if global.turbo_disabled changes from 1 to 0.
> > 
> > Something like the attached (untested) patch.
> 
> Should work.
> 
> Xi,
> Please test so that we can close this issue.

Yes it restores the old behavior for me.

Tested-by: Xi Ruoyao <xry111@xry111.site>

-- 
Xi Ruoyao <xry111@xry111.site>
School of Aerospace Science and Technology, Xidian University