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

[Rafael J. Wysocki]

On Wed, Jun 5, 2024 at 2:05 PM srinivas pandruvada
<srinivas.pandruvada@xxxxxxxxxxxxxxx> 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.
---
 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;