[PATCH 00/12] cpufreq/amd-pstate: Fixes and optimizations

[PATCH 00/12] cpufreq/amd-pstate: Fixes and optimizations

[Dhananjay Ugwekar]

Patch 1: 
* Remove the goto label in amd_pstate_update_limits, as the mutex
  has been converted to a scope based (guard) cleanup.

Patches 2-4 (adjust_perf changes):
* In amd_state_adjust_perf, max_perf was being set to highest_perf which
  was leading to max_limit_perf value not being honored, fix that.
* The min_perf has a hard lower limit at lowest_nonlinear_perf,
  instead set the hard limit at min_limit_perf. This gives the user the
  freedom to set min_perf lower than lowest_nonlinear_perf with schedutil.
* Pass the correct perf limits to amd_pstate_update() in adjust_perf.

Patches 5-6 (Modularize perf<->freq conversion):
* Convert all perf values to u8 datatype.
* Create two helper functions which carry out all the perf<->freq 
  conversions throughout the code.

Patches 7-12 (Fix amd_pstate_update_limits() and add scope based cleanup 
for cpufreq_policy ref counting):
* Fix a missed cpufreq_cpu_put in one of the exit paths and remove
  unnecessary cpufreq_update_policy call.
* Add a missing nullptr check in amd_pstate_update.
* Add a scope based cleanup macro for cpufreq_policy ref counting.
* Remove the unnecessary driver_lock in update_limits.

Base: superm1/bleeding-edge

Dhananjay Ugwekar (12):
  cpufreq/amd-pstate: Remove the goto label in amd_pstate_update_limits
  cpufreq/amd-pstate: Fix max_perf updation with schedutil
  cpufreq/amd-pstate: Modify the min_perf calculation in adjust_perf
    callback
  cpufreq/amd-pstate: Remove the redundant des_perf clamping in
    adjust_perf
  cpufreq/amd-pstate: Pass min/max_limit_perf as min/max_perf to
    amd_pstate_update
  cpufreq/amd-pstate: Convert all perf values to u8
  cpufreq/amd-pstate: Modularize perf<->freq conversion
  cpufreq/amd-pstate: Remove the unnecessary cpufreq_update_policy call
  cpufreq/amd-pstate: Fix cpufreq_policy ref counting
  cpufreq/amd-pstate: Add missing NULL ptr check in amd_pstate_update
  cpufreq/amd-pstate: Use scope based cleanup for cpufreq_policy refs
  cpufreq/amd-pstate: Remove the unncecessary driver_lock in
    amd_pstate_update_limits

 drivers/cpufreq/amd-pstate-trace.h |  46 ++++----
 drivers/cpufreq/amd-pstate.c       | 167 ++++++++++++++---------------
 drivers/cpufreq/amd-pstate.h       |  18 ++--
 include/linux/cpufreq.h            |   3 +
 4 files changed, 113 insertions(+), 121 deletions(-)

-- 
2.34.1

[PATCH 01/12] cpufreq/amd-pstate: Remove the goto label in amd_pstate_update_limits

[Dhananjay Ugwekar]

Scope based guard/cleanup macros should not be used together with goto
labels. Hence, remove the goto label.

Fixes: 6c093d5a5b73 ("cpufreq/amd-pstate: convert mutex use to guard()")
Signed-off-by: Dhananjay Ugwekar <dhananjay.ugwekar@amd.com>
---
 drivers/cpufreq/amd-pstate.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c
index 7120f035c0be..b163c1699821 100644
--- a/drivers/cpufreq/amd-pstate.c
+++ b/drivers/cpufreq/amd-pstate.c
@@ -838,8 +838,10 @@ static void amd_pstate_update_limits(unsigned int cpu)
 	guard(mutex)(&amd_pstate_driver_lock);
 
 	ret = amd_get_highest_perf(cpu, &cur_high);
-	if (ret)
-		goto free_cpufreq_put;
+	if (ret) {
+		cpufreq_cpu_put(policy);
+		return;
+	}
 
 	prev_high = READ_ONCE(cpudata->prefcore_ranking);
 	highest_perf_changed = (prev_high != cur_high);
@@ -849,8 +851,6 @@ static void amd_pstate_update_limits(unsigned int cpu)
 		if (cur_high < CPPC_MAX_PERF)
 			sched_set_itmt_core_prio((int)cur_high, cpu);
 	}
-
-free_cpufreq_put:
 	cpufreq_cpu_put(policy);
 
 	if (!highest_perf_changed)
-- 
2.34.1

[PATCH 02/12] cpufreq/amd-pstate: Fix max_perf updation with schedutil

[Dhananjay Ugwekar]

In adjust_perf() callback, we are setting the max_perf to highest_perf,
as opposed to the correct limit value i.e. max_limit_perf. Fix that.

Fixes: 3f7b835fa4d0 ("cpufreq/amd-pstate: Move limit updating code")
Signed-off-by: Dhananjay Ugwekar <dhananjay.ugwekar@amd.com>
---
 drivers/cpufreq/amd-pstate.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c
index b163c1699821..9dc3933bc326 100644
--- a/drivers/cpufreq/amd-pstate.c
+++ b/drivers/cpufreq/amd-pstate.c
@@ -699,7 +699,7 @@ static void amd_pstate_adjust_perf(unsigned int cpu,
 	if (min_perf < lowest_nonlinear_perf)
 		min_perf = lowest_nonlinear_perf;
 
-	max_perf = cap_perf;
+	max_perf = cpudata->max_limit_perf;
 	if (max_perf < min_perf)
 		max_perf = min_perf;
 
-- 
2.34.1

[PATCH 03/12] cpufreq/amd-pstate: Modify the min_perf calculation in adjust_perf callback

[Dhananjay Ugwekar]

Instead of setting a fixed floor at lowest_nonlinear_perf, use the
min_limit_perf value, so that it gives the user the freedom to lower the
floor further.

There are two minimum frequency/perf limits that we need to consider in
the adjust_perf callback. One provided by schedutil i.e. the sg_cpu->bw_min
value passed in _min_perf arg, another is the effective value of
min_freq_qos request that is updated in cpudata->min_limit_perf. Modify the
code to use the bigger of these two values.

Signed-off-by: Dhananjay Ugwekar <dhananjay.ugwekar@amd.com>
---
 drivers/cpufreq/amd-pstate.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c
index 9dc3933bc326..a23fb78a442b 100644
--- a/drivers/cpufreq/amd-pstate.c
+++ b/drivers/cpufreq/amd-pstate.c
@@ -672,7 +672,7 @@ static void amd_pstate_adjust_perf(unsigned int cpu,
 				   unsigned long capacity)
 {
 	unsigned long max_perf, min_perf, des_perf,
-		      cap_perf, lowest_nonlinear_perf;
+		      cap_perf, min_limit_perf;
 	struct cpufreq_policy *policy = cpufreq_cpu_get(cpu);
 	struct amd_cpudata *cpudata;
 
@@ -684,20 +684,20 @@ static void amd_pstate_adjust_perf(unsigned int cpu,
 	if (policy->min != cpudata->min_limit_freq || policy->max != cpudata->max_limit_freq)
 		amd_pstate_update_min_max_limit(policy);
 
-
 	cap_perf = READ_ONCE(cpudata->highest_perf);
-	lowest_nonlinear_perf = READ_ONCE(cpudata->lowest_nonlinear_perf);
+	min_limit_perf = READ_ONCE(cpudata->min_limit_perf);
 
 	des_perf = cap_perf;
 	if (target_perf < capacity)
 		des_perf = DIV_ROUND_UP(cap_perf * target_perf, capacity);
 
-	min_perf = READ_ONCE(cpudata->lowest_perf);
 	if (_min_perf < capacity)
 		min_perf = DIV_ROUND_UP(cap_perf * _min_perf, capacity);
+	else
+		min_perf = cap_perf;
 
-	if (min_perf < lowest_nonlinear_perf)
-		min_perf = lowest_nonlinear_perf;
+	if (min_perf < min_limit_perf)
+		min_perf = min_limit_perf;
 
 	max_perf = cpudata->max_limit_perf;
 	if (max_perf < min_perf)
-- 
2.34.1

[PATCH 04/12] cpufreq/amd-pstate: Remove the redundant des_perf clamping in adjust_perf

[Dhananjay Ugwekar]

des_perf is later on clamped between min_perf and max_perf in
amd_pstate_update. So, remove the redundant clamping from
amd_pstate_adjust_perf.

Signed-off-by: Dhananjay Ugwekar <dhananjay.ugwekar@amd.com>
---
 drivers/cpufreq/amd-pstate.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c
index a23fb78a442b..2926f292be7b 100644
--- a/drivers/cpufreq/amd-pstate.c
+++ b/drivers/cpufreq/amd-pstate.c
@@ -703,8 +703,6 @@ static void amd_pstate_adjust_perf(unsigned int cpu,
 	if (max_perf < min_perf)
 		max_perf = min_perf;
 
-	des_perf = clamp_t(unsigned long, des_perf, min_perf, max_perf);
-
 	amd_pstate_update(cpudata, min_perf, des_perf, max_perf, true,
 			policy->governor->flags);
 	cpufreq_cpu_put(policy);
-- 
2.34.1

[PATCH 05/12] cpufreq/amd-pstate: Pass min/max_limit_perf as min/max_perf to amd_pstate_update

[Dhananjay Ugwekar]

Currently, amd_pstate_update_freq passes the hardware perf limits as
min/max_perf to amd_pstate_update, which eventually gets programmed into
the min/max_perf fields of the CPPC_REQ register.

Instead pass the effective perf limits i.e. min/max_limit_perf values to
amd_pstate_update as min/max_perf.

Signed-off-by: Dhananjay Ugwekar <dhananjay.ugwekar@amd.com>
---
 drivers/cpufreq/amd-pstate.c | 9 ++++-----
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c
index 2926f292be7b..e179e929b941 100644
--- a/drivers/cpufreq/amd-pstate.c
+++ b/drivers/cpufreq/amd-pstate.c
@@ -615,7 +615,7 @@ static int amd_pstate_update_freq(struct cpufreq_policy *policy,
 {
 	struct cpufreq_freqs freqs;
 	struct amd_cpudata *cpudata = policy->driver_data;
-	unsigned long max_perf, min_perf, des_perf, cap_perf;
+	unsigned long des_perf, cap_perf;
 
 	if (!cpudata->max_freq)
 		return -ENODEV;
@@ -624,8 +624,6 @@ static int amd_pstate_update_freq(struct cpufreq_policy *policy,
 		amd_pstate_update_min_max_limit(policy);
 
 	cap_perf = READ_ONCE(cpudata->highest_perf);
-	min_perf = READ_ONCE(cpudata->lowest_perf);
-	max_perf = cap_perf;
 
 	freqs.old = policy->cur;
 	freqs.new = target_freq;
@@ -642,8 +640,9 @@ static int amd_pstate_update_freq(struct cpufreq_policy *policy,
 	if (!fast_switch)
 		cpufreq_freq_transition_begin(policy, &freqs);
 
-	amd_pstate_update(cpudata, min_perf, des_perf,
-			max_perf, fast_switch, policy->governor->flags);
+	amd_pstate_update(cpudata, cpudata->min_limit_perf, des_perf,
+			  cpudata->max_limit_perf, fast_switch,
+			  policy->governor->flags);
 
 	if (!fast_switch)
 		cpufreq_freq_transition_end(policy, &freqs, false);
-- 
2.34.1

[PATCH 06/12] cpufreq/amd-pstate: Convert all perf values to u8

[Dhananjay Ugwekar]

All perf values are always within 0-255 range, hence convert their
datatype to u8 everywhere.

Signed-off-by: Dhananjay Ugwekar <dhananjay.ugwekar@amd.com>
---
 drivers/cpufreq/amd-pstate-trace.h | 46 +++++++++++------------
 drivers/cpufreq/amd-pstate.c       | 60 +++++++++++++++---------------
 drivers/cpufreq/amd-pstate.h       | 18 ++++-----
 3 files changed, 62 insertions(+), 62 deletions(-)

diff --git a/drivers/cpufreq/amd-pstate-trace.h b/drivers/cpufreq/amd-pstate-trace.h
index 8d692415d905..f457d4af2c62 100644
--- a/drivers/cpufreq/amd-pstate-trace.h
+++ b/drivers/cpufreq/amd-pstate-trace.h
@@ -24,9 +24,9 @@
 
 TRACE_EVENT(amd_pstate_perf,
 
-	TP_PROTO(unsigned long min_perf,
-		 unsigned long target_perf,
-		 unsigned long capacity,
+	TP_PROTO(u8 min_perf,
+		 u8 target_perf,
+		 u8 capacity,
 		 u64 freq,
 		 u64 mperf,
 		 u64 aperf,
@@ -47,9 +47,9 @@ TRACE_EVENT(amd_pstate_perf,
 		),
 
 	TP_STRUCT__entry(
-		__field(unsigned long, min_perf)
-		__field(unsigned long, target_perf)
-		__field(unsigned long, capacity)
+		__field(u8, min_perf)
+		__field(u8, target_perf)
+		__field(u8, capacity)
 		__field(unsigned long long, freq)
 		__field(unsigned long long, mperf)
 		__field(unsigned long long, aperf)
@@ -70,10 +70,10 @@ TRACE_EVENT(amd_pstate_perf,
 		__entry->fast_switch = fast_switch;
 		),
 
-	TP_printk("amd_min_perf=%lu amd_des_perf=%lu amd_max_perf=%lu freq=%llu mperf=%llu aperf=%llu tsc=%llu cpu_id=%u fast_switch=%s",
-		  (unsigned long)__entry->min_perf,
-		  (unsigned long)__entry->target_perf,
-		  (unsigned long)__entry->capacity,
+	TP_printk("amd_min_perf=%hhu amd_des_perf=%hhu amd_max_perf=%hhu freq=%llu mperf=%llu aperf=%llu tsc=%llu cpu_id=%u fast_switch=%s",
+		  (u8)__entry->min_perf,
+		  (u8)__entry->target_perf,
+		  (u8)__entry->capacity,
 		  (unsigned long long)__entry->freq,
 		  (unsigned long long)__entry->mperf,
 		  (unsigned long long)__entry->aperf,
@@ -86,10 +86,10 @@ TRACE_EVENT(amd_pstate_perf,
 TRACE_EVENT(amd_pstate_epp_perf,
 
 	TP_PROTO(unsigned int cpu_id,
-		 unsigned int highest_perf,
-		 unsigned int epp,
-		 unsigned int min_perf,
-		 unsigned int max_perf,
+		 u8 highest_perf,
+		 u8 epp,
+		 u8 min_perf,
+		 u8 max_perf,
 		 bool boost
 		 ),
 
@@ -102,10 +102,10 @@ TRACE_EVENT(amd_pstate_epp_perf,
 
 	TP_STRUCT__entry(
 		__field(unsigned int, cpu_id)
-		__field(unsigned int, highest_perf)
-		__field(unsigned int, epp)
-		__field(unsigned int, min_perf)
-		__field(unsigned int, max_perf)
+		__field(u8, highest_perf)
+		__field(u8, epp)
+		__field(u8, min_perf)
+		__field(u8, max_perf)
 		__field(bool, boost)
 		),
 
@@ -118,12 +118,12 @@ TRACE_EVENT(amd_pstate_epp_perf,
 		__entry->boost = boost;
 		),
 
-	TP_printk("cpu%u: [%u<->%u]/%u, epp=%u, boost=%u",
+	TP_printk("cpu%u: [%hhu<->%hhu]/%hhu, epp=%hhu, boost=%u",
 		  (unsigned int)__entry->cpu_id,
-		  (unsigned int)__entry->min_perf,
-		  (unsigned int)__entry->max_perf,
-		  (unsigned int)__entry->highest_perf,
-		  (unsigned int)__entry->epp,
+		  (u8)__entry->min_perf,
+		  (u8)__entry->max_perf,
+		  (u8)__entry->highest_perf,
+		  (u8)__entry->epp,
 		  (bool)__entry->boost
 		 )
 );
diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c
index e179e929b941..dd4f23fa2587 100644
--- a/drivers/cpufreq/amd-pstate.c
+++ b/drivers/cpufreq/amd-pstate.c
@@ -186,7 +186,7 @@ static inline int get_mode_idx_from_str(const char *str, size_t size)
 static DEFINE_MUTEX(amd_pstate_limits_lock);
 static DEFINE_MUTEX(amd_pstate_driver_lock);
 
-static s16 msr_get_epp(struct amd_cpudata *cpudata)
+static u8 msr_get_epp(struct amd_cpudata *cpudata)
 {
 	u64 value;
 	int ret;
@@ -207,7 +207,7 @@ static inline s16 amd_pstate_get_epp(struct amd_cpudata *cpudata)
 	return static_call(amd_pstate_get_epp)(cpudata);
 }
 
-static s16 shmem_get_epp(struct amd_cpudata *cpudata)
+static u8 shmem_get_epp(struct amd_cpudata *cpudata)
 {
 	u64 epp;
 	int ret;
@@ -218,11 +218,11 @@ static s16 shmem_get_epp(struct amd_cpudata *cpudata)
 		return ret;
 	}
 
-	return (s16)(epp & 0xff);
+	return FIELD_GET(AMD_CPPC_EPP_PERF_MASK, epp);
 }
 
-static int msr_update_perf(struct amd_cpudata *cpudata, u32 min_perf,
-			   u32 des_perf, u32 max_perf, u32 epp, bool fast_switch)
+static int msr_update_perf(struct amd_cpudata *cpudata, u8 min_perf,
+			   u8 des_perf, u8 max_perf, u8 epp, bool fast_switch)
 {
 	u64 value, prev;
 
@@ -257,15 +257,15 @@ static int msr_update_perf(struct amd_cpudata *cpudata, u32 min_perf,
 DEFINE_STATIC_CALL(amd_pstate_update_perf, msr_update_perf);
 
 static inline int amd_pstate_update_perf(struct amd_cpudata *cpudata,
-					  u32 min_perf, u32 des_perf,
-					  u32 max_perf, u32 epp,
+					  u8 min_perf, u8 des_perf,
+					  u8 max_perf, u8 epp,
 					  bool fast_switch)
 {
 	return static_call(amd_pstate_update_perf)(cpudata, min_perf, des_perf,
 						   max_perf, epp, fast_switch);
 }
 
-static int msr_set_epp(struct amd_cpudata *cpudata, u32 epp)
+static int msr_set_epp(struct amd_cpudata *cpudata, u8 epp)
 {
 	u64 value, prev;
 	int ret;
@@ -292,12 +292,12 @@ static int msr_set_epp(struct amd_cpudata *cpudata, u32 epp)
 
 DEFINE_STATIC_CALL(amd_pstate_set_epp, msr_set_epp);
 
-static inline int amd_pstate_set_epp(struct amd_cpudata *cpudata, u32 epp)
+static inline int amd_pstate_set_epp(struct amd_cpudata *cpudata, u8 epp)
 {
 	return static_call(amd_pstate_set_epp)(cpudata, epp);
 }
 
-static int shmem_set_epp(struct amd_cpudata *cpudata, u32 epp)
+static int shmem_set_epp(struct amd_cpudata *cpudata, u8 epp)
 {
 	int ret;
 	struct cppc_perf_ctrls perf_ctrls;
@@ -320,7 +320,7 @@ static int amd_pstate_set_energy_pref_index(struct cpufreq_policy *policy,
 					    int pref_index)
 {
 	struct amd_cpudata *cpudata = policy->driver_data;
-	int epp;
+	u8 epp;
 
 	if (!pref_index)
 		epp = cpudata->epp_default;
@@ -479,8 +479,8 @@ static inline int amd_pstate_init_perf(struct amd_cpudata *cpudata)
 	return static_call(amd_pstate_init_perf)(cpudata);
 }
 
-static int shmem_update_perf(struct amd_cpudata *cpudata, u32 min_perf,
-			     u32 des_perf, u32 max_perf, u32 epp, bool fast_switch)
+static int shmem_update_perf(struct amd_cpudata *cpudata, u8 min_perf,
+			     u8 des_perf, u8 max_perf, u8 epp, bool fast_switch)
 {
 	struct cppc_perf_ctrls perf_ctrls;
 
@@ -531,14 +531,14 @@ static inline bool amd_pstate_sample(struct amd_cpudata *cpudata)
 	return true;
 }
 
-static void amd_pstate_update(struct amd_cpudata *cpudata, u32 min_perf,
-			      u32 des_perf, u32 max_perf, bool fast_switch, int gov_flags)
+static void amd_pstate_update(struct amd_cpudata *cpudata, u8 min_perf,
+			      u8 des_perf, u8 max_perf, bool fast_switch, int gov_flags)
 {
 	unsigned long max_freq;
 	struct cpufreq_policy *policy = cpufreq_cpu_get(cpudata->cpu);
-	u32 nominal_perf = READ_ONCE(cpudata->nominal_perf);
+	u8 nominal_perf = READ_ONCE(cpudata->nominal_perf);
 
-	des_perf = clamp_t(unsigned long, des_perf, min_perf, max_perf);
+	des_perf = clamp_t(u8, des_perf, min_perf, max_perf);
 
 	max_freq = READ_ONCE(cpudata->max_limit_freq);
 	policy->cur = div_u64(des_perf * max_freq, max_perf);
@@ -550,7 +550,7 @@ static void amd_pstate_update(struct amd_cpudata *cpudata, u32 min_perf,
 
 	/* limit the max perf when core performance boost feature is disabled */
 	if (!cpudata->boost_supported)
-		max_perf = min_t(unsigned long, nominal_perf, max_perf);
+		max_perf = min_t(u8, nominal_perf, max_perf);
 
 	if (trace_amd_pstate_perf_enabled() && amd_pstate_sample(cpudata)) {
 		trace_amd_pstate_perf(min_perf, des_perf, max_perf, cpudata->freq,
@@ -591,7 +591,8 @@ static int amd_pstate_verify(struct cpufreq_policy_data *policy_data)
 
 static int amd_pstate_update_min_max_limit(struct cpufreq_policy *policy)
 {
-	u32 max_limit_perf, min_limit_perf, max_perf, max_freq;
+	u8 max_limit_perf, min_limit_perf, max_perf;
+	u32 max_freq;
 	struct amd_cpudata *cpudata = policy->driver_data;
 
 	max_perf = READ_ONCE(cpudata->highest_perf);
@@ -615,7 +616,7 @@ static int amd_pstate_update_freq(struct cpufreq_policy *policy,
 {
 	struct cpufreq_freqs freqs;
 	struct amd_cpudata *cpudata = policy->driver_data;
-	unsigned long des_perf, cap_perf;
+	u8 des_perf, cap_perf;
 
 	if (!cpudata->max_freq)
 		return -ENODEV;
@@ -670,8 +671,7 @@ static void amd_pstate_adjust_perf(unsigned int cpu,
 				   unsigned long target_perf,
 				   unsigned long capacity)
 {
-	unsigned long max_perf, min_perf, des_perf,
-		      cap_perf, min_limit_perf;
+	u8 max_perf, min_perf, des_perf, cap_perf, min_limit_perf;
 	struct cpufreq_policy *policy = cpufreq_cpu_get(cpu);
 	struct amd_cpudata *cpudata;
 
@@ -904,8 +904,8 @@ static int amd_pstate_init_freq(struct amd_cpudata *cpudata)
 {
 	int ret;
 	u32 min_freq, max_freq;
-	u32 highest_perf, nominal_perf, nominal_freq;
-	u32 lowest_nonlinear_perf, lowest_nonlinear_freq;
+	u8 highest_perf, nominal_perf, lowest_nonlinear_perf;
+	u32 nominal_freq, lowest_nonlinear_freq;
 	struct cppc_perf_caps cppc_perf;
 
 	ret = cppc_get_perf_caps(cpudata->cpu, &cppc_perf);
@@ -1112,7 +1112,7 @@ static ssize_t show_amd_pstate_lowest_nonlinear_freq(struct cpufreq_policy *poli
 static ssize_t show_amd_pstate_highest_perf(struct cpufreq_policy *policy,
 					    char *buf)
 {
-	u32 perf;
+	u8 perf;
 	struct amd_cpudata *cpudata = policy->driver_data;
 
 	perf = READ_ONCE(cpudata->highest_perf);
@@ -1123,7 +1123,7 @@ static ssize_t show_amd_pstate_highest_perf(struct cpufreq_policy *policy,
 static ssize_t show_amd_pstate_prefcore_ranking(struct cpufreq_policy *policy,
 						char *buf)
 {
-	u32 perf;
+	u8 perf;
 	struct amd_cpudata *cpudata = policy->driver_data;
 
 	perf = READ_ONCE(cpudata->prefcore_ranking);
@@ -1186,7 +1186,7 @@ static ssize_t show_energy_performance_preference(
 				struct cpufreq_policy *policy, char *buf)
 {
 	struct amd_cpudata *cpudata = policy->driver_data;
-	int preference;
+	u8 preference;
 
 	switch (cpudata->epp_cached) {
 	case AMD_CPPC_EPP_PERFORMANCE:
@@ -1548,7 +1548,7 @@ static void amd_pstate_epp_cpu_exit(struct cpufreq_policy *policy)
 static int amd_pstate_epp_update_limit(struct cpufreq_policy *policy)
 {
 	struct amd_cpudata *cpudata = policy->driver_data;
-	u32 epp;
+	u8 epp;
 
 	amd_pstate_update_min_max_limit(policy);
 
@@ -1597,7 +1597,7 @@ static int amd_pstate_epp_set_policy(struct cpufreq_policy *policy)
 static int amd_pstate_epp_reenable(struct cpufreq_policy *policy)
 {
 	struct amd_cpudata *cpudata = policy->driver_data;
-	u64 max_perf;
+	u8 max_perf;
 	int ret;
 
 	ret = amd_pstate_cppc_enable(true);
@@ -1634,7 +1634,7 @@ static int amd_pstate_epp_cpu_online(struct cpufreq_policy *policy)
 static int amd_pstate_epp_cpu_offline(struct cpufreq_policy *policy)
 {
 	struct amd_cpudata *cpudata = policy->driver_data;
-	int min_perf;
+	u8 min_perf;
 
 	if (cpudata->suspended)
 		return 0;
diff --git a/drivers/cpufreq/amd-pstate.h b/drivers/cpufreq/amd-pstate.h
index 9747e3be6cee..19d405c6d805 100644
--- a/drivers/cpufreq/amd-pstate.h
+++ b/drivers/cpufreq/amd-pstate.h
@@ -70,13 +70,13 @@ struct amd_cpudata {
 	struct	freq_qos_request req[2];
 	u64	cppc_req_cached;
 
-	u32	highest_perf;
-	u32	nominal_perf;
-	u32	lowest_nonlinear_perf;
-	u32	lowest_perf;
-	u32     prefcore_ranking;
-	u32     min_limit_perf;
-	u32     max_limit_perf;
+	u8	highest_perf;
+	u8	nominal_perf;
+	u8	lowest_nonlinear_perf;
+	u8	lowest_perf;
+	u8	prefcore_ranking;
+	u8	min_limit_perf;
+	u8	max_limit_perf;
 	u32     min_limit_freq;
 	u32     max_limit_freq;
 
@@ -93,11 +93,11 @@ struct amd_cpudata {
 	bool	hw_prefcore;
 
 	/* EPP feature related attributes*/
-	s16	epp_cached;
+	u8	epp_cached;
 	u32	policy;
 	u64	cppc_cap1_cached;
 	bool	suspended;
-	s16	epp_default;
+	u8	epp_default;
 };
 
 /*
-- 
2.34.1

[PATCH 07/12] cpufreq/amd-pstate: Modularize perf<->freq conversion

[Dhananjay Ugwekar]

Delegate the perf<->frequency conversion to helper functions to reduce
code duplication, and improve readability.

Signed-off-by: Dhananjay Ugwekar <dhananjay.ugwekar@amd.com>
---
 drivers/cpufreq/amd-pstate.c | 57 +++++++++++++++++++-----------------
 1 file changed, 30 insertions(+), 27 deletions(-)

diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c
index dd4f23fa2587..346fac646eba 100644
--- a/drivers/cpufreq/amd-pstate.c
+++ b/drivers/cpufreq/amd-pstate.c
@@ -142,6 +142,20 @@ static struct quirk_entry quirk_amd_7k62 = {
 	.lowest_freq = 550,
 };
 
+static inline u8 freq_to_perf(struct amd_cpudata *cpudata, unsigned int freq_val)
+{
+	u8 perf_val = DIV_ROUND_UP_ULL((u64)freq_val * cpudata->nominal_perf,
+					cpudata->nominal_freq);
+
+	return clamp_t(u8, perf_val, cpudata->lowest_perf, cpudata->highest_perf);
+}
+
+static inline u32 perf_to_freq(struct amd_cpudata *cpudata, u8 perf_val)
+{
+	return DIV_ROUND_UP_ULL((u64)cpudata->nominal_freq * perf_val,
+				cpudata->nominal_perf);
+}
+
 static int __init dmi_matched_7k62_bios_bug(const struct dmi_system_id *dmi)
 {
 	/**
@@ -534,14 +548,12 @@ static inline bool amd_pstate_sample(struct amd_cpudata *cpudata)
 static void amd_pstate_update(struct amd_cpudata *cpudata, u8 min_perf,
 			      u8 des_perf, u8 max_perf, bool fast_switch, int gov_flags)
 {
-	unsigned long max_freq;
 	struct cpufreq_policy *policy = cpufreq_cpu_get(cpudata->cpu);
 	u8 nominal_perf = READ_ONCE(cpudata->nominal_perf);
 
 	des_perf = clamp_t(u8, des_perf, min_perf, max_perf);
 
-	max_freq = READ_ONCE(cpudata->max_limit_freq);
-	policy->cur = div_u64(des_perf * max_freq, max_perf);
+	policy->cur = perf_to_freq(cpudata, des_perf);
 
 	if ((cppc_state == AMD_PSTATE_GUIDED) && (gov_flags & CPUFREQ_GOV_DYNAMIC_SWITCHING)) {
 		min_perf = des_perf;
@@ -591,14 +603,11 @@ static int amd_pstate_verify(struct cpufreq_policy_data *policy_data)
 
 static int amd_pstate_update_min_max_limit(struct cpufreq_policy *policy)
 {
-	u8 max_limit_perf, min_limit_perf, max_perf;
-	u32 max_freq;
+	u8 max_limit_perf, min_limit_perf;
 	struct amd_cpudata *cpudata = policy->driver_data;
 
-	max_perf = READ_ONCE(cpudata->highest_perf);
-	max_freq = READ_ONCE(cpudata->max_freq);
-	max_limit_perf = div_u64(policy->max * max_perf, max_freq);
-	min_limit_perf = div_u64(policy->min * max_perf, max_freq);
+	max_limit_perf = freq_to_perf(cpudata, policy->max);
+	min_limit_perf = freq_to_perf(cpudata, policy->min);
 
 	if (cpudata->policy == CPUFREQ_POLICY_PERFORMANCE)
 		min_limit_perf = min(cpudata->nominal_perf, max_limit_perf);
@@ -616,21 +625,15 @@ static int amd_pstate_update_freq(struct cpufreq_policy *policy,
 {
 	struct cpufreq_freqs freqs;
 	struct amd_cpudata *cpudata = policy->driver_data;
-	u8 des_perf, cap_perf;
-
-	if (!cpudata->max_freq)
-		return -ENODEV;
+	u8 des_perf;
 
 	if (policy->min != cpudata->min_limit_freq || policy->max != cpudata->max_limit_freq)
 		amd_pstate_update_min_max_limit(policy);
 
-	cap_perf = READ_ONCE(cpudata->highest_perf);
-
 	freqs.old = policy->cur;
 	freqs.new = target_freq;
 
-	des_perf = DIV_ROUND_CLOSEST(target_freq * cap_perf,
-				     cpudata->max_freq);
+	des_perf = freq_to_perf(cpudata, target_freq);
 
 	WARN_ON(fast_switch && !policy->fast_switch_enabled);
 	/*
@@ -904,7 +907,6 @@ static int amd_pstate_init_freq(struct amd_cpudata *cpudata)
 {
 	int ret;
 	u32 min_freq, max_freq;
-	u8 highest_perf, nominal_perf, lowest_nonlinear_perf;
 	u32 nominal_freq, lowest_nonlinear_freq;
 	struct cppc_perf_caps cppc_perf;
 
@@ -922,16 +924,17 @@ static int amd_pstate_init_freq(struct amd_cpudata *cpudata)
 	else
 		nominal_freq = cppc_perf.nominal_freq;
 
-	highest_perf = READ_ONCE(cpudata->highest_perf);
-	nominal_perf = READ_ONCE(cpudata->nominal_perf);
-	max_freq = div_u64((u64)highest_perf * nominal_freq, nominal_perf);
+	min_freq *= 1000;
+	nominal_freq *= 1000;
+
+	WRITE_ONCE(cpudata->nominal_freq, nominal_freq);
+	WRITE_ONCE(cpudata->min_freq, min_freq);
+
+	max_freq = perf_to_freq(cpudata, cpudata->highest_perf);
+	lowest_nonlinear_freq = perf_to_freq(cpudata, cpudata->lowest_nonlinear_perf);
 
-	lowest_nonlinear_perf = READ_ONCE(cpudata->lowest_nonlinear_perf);
-	lowest_nonlinear_freq = div_u64((u64)nominal_freq * lowest_nonlinear_perf, nominal_perf);
-	WRITE_ONCE(cpudata->min_freq, min_freq * 1000);
-	WRITE_ONCE(cpudata->lowest_nonlinear_freq, lowest_nonlinear_freq * 1000);
-	WRITE_ONCE(cpudata->nominal_freq, nominal_freq * 1000);
-	WRITE_ONCE(cpudata->max_freq, max_freq * 1000);
+	WRITE_ONCE(cpudata->lowest_nonlinear_freq, lowest_nonlinear_freq);
+	WRITE_ONCE(cpudata->max_freq, max_freq);
 
 	/**
 	 * Below values need to be initialized correctly, otherwise driver will fail to load
-- 
2.34.1

[PATCH 08/12] cpufreq/amd-pstate: Remove the unnecessary cpufreq_update_policy call

[Dhananjay Ugwekar]

The update_limits callback is only called in two conditions.

* When the preferred core rankings change. In which case, we just need to
change the prefcore ranking in the cpudata struct. As there are no changes
to any of the perf values, there is no need to call cpufreq_update_policy()

* When the _PPC ACPI object changes, i.e. the highest allowed Pstate
changes. The _PPC object is only used for a table based cpufreq driver
like acpi-cpufreq, hence is irrelevant for CPPC based amd-pstate.

Hence, the cpufreq_update_policy() call becomes unnecessary and can be
removed.

Signed-off-by: Dhananjay Ugwekar <dhananjay.ugwekar@amd.com>
---
 drivers/cpufreq/amd-pstate.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c
index 346fac646eba..107ad953ce43 100644
--- a/drivers/cpufreq/amd-pstate.c
+++ b/drivers/cpufreq/amd-pstate.c
@@ -852,10 +852,6 @@ static void amd_pstate_update_limits(unsigned int cpu)
 			sched_set_itmt_core_prio((int)cur_high, cpu);
 	}
 	cpufreq_cpu_put(policy);
-
-	if (!highest_perf_changed)
-		cpufreq_update_policy(cpu);
-
 }
 
 /*
-- 
2.34.1

[PATCH 09/12] cpufreq/amd-pstate: Fix cpufreq_policy ref counting

[Dhananjay Ugwekar]

amd_pstate_update_limits() takes a cpufreq_policy reference but doesn't
decrement the refcount in one of the exit paths, fix that.

Fixes: 45722e777fd9 ("cpufreq: amd-pstate: Optimize amd_pstate_update_limits()")
Signed-off-by: Dhananjay Ugwekar <dhananjay.ugwekar@amd.com>
---
 drivers/cpufreq/amd-pstate.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c
index 107ad953ce43..9c939be59042 100644
--- a/drivers/cpufreq/amd-pstate.c
+++ b/drivers/cpufreq/amd-pstate.c
@@ -821,20 +821,21 @@ static void amd_pstate_init_prefcore(struct amd_cpudata *cpudata)
 
 static void amd_pstate_update_limits(unsigned int cpu)
 {
-	struct cpufreq_policy *policy = cpufreq_cpu_get(cpu);
+	struct cpufreq_policy *policy = NULL;
 	struct amd_cpudata *cpudata;
 	u32 prev_high = 0, cur_high = 0;
 	int ret;
 	bool highest_perf_changed = false;
 
+	if (!amd_pstate_prefcore)
+		return;
+
+	policy = cpufreq_cpu_get(cpu);
 	if (!policy)
 		return;
 
 	cpudata = policy->driver_data;
 
-	if (!amd_pstate_prefcore)
-		return;
-
 	guard(mutex)(&amd_pstate_driver_lock);
 
 	ret = amd_get_highest_perf(cpu, &cur_high);
-- 
2.34.1

[PATCH 10/12] cpufreq/amd-pstate: Add missing NULL ptr check in amd_pstate_update

[Dhananjay Ugwekar]

Check if policy is NULL before dereferencing it in amd_pstate_update.

Fixes: e8f555daacd3 ("cpufreq/amd-pstate: fix setting policy current frequency value")
Signed-off-by: Dhananjay Ugwekar <dhananjay.ugwekar@amd.com>
---
 drivers/cpufreq/amd-pstate.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c
index 9c939be59042..6a604f0797d9 100644
--- a/drivers/cpufreq/amd-pstate.c
+++ b/drivers/cpufreq/amd-pstate.c
@@ -551,6 +551,9 @@ static void amd_pstate_update(struct amd_cpudata *cpudata, u8 min_perf,
 	struct cpufreq_policy *policy = cpufreq_cpu_get(cpudata->cpu);
 	u8 nominal_perf = READ_ONCE(cpudata->nominal_perf);
 
+	if (!policy)
+		return;
+
 	des_perf = clamp_t(u8, des_perf, min_perf, max_perf);
 
 	policy->cur = perf_to_freq(cpudata, des_perf);
-- 
2.34.1

[PATCH 11/12] cpufreq/amd-pstate: Use scope based cleanup for cpufreq_policy refs

[Dhananjay Ugwekar]

There have been instances in past where refcount decrementing is missed
while exiting a function. Use automatic scope based cleanup to avoid
such errors.

Signed-off-by: Dhananjay Ugwekar <dhananjay.ugwekar@amd.com>
---
 drivers/cpufreq/amd-pstate.c | 25 ++++++++-----------------
 include/linux/cpufreq.h      |  3 +++
 2 files changed, 11 insertions(+), 17 deletions(-)

diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c
index 6a604f0797d9..ee7e3f0a4c0a 100644
--- a/drivers/cpufreq/amd-pstate.c
+++ b/drivers/cpufreq/amd-pstate.c
@@ -548,7 +548,7 @@ static inline bool amd_pstate_sample(struct amd_cpudata *cpudata)
 static void amd_pstate_update(struct amd_cpudata *cpudata, u8 min_perf,
 			      u8 des_perf, u8 max_perf, bool fast_switch, int gov_flags)
 {
-	struct cpufreq_policy *policy = cpufreq_cpu_get(cpudata->cpu);
+	struct cpufreq_policy *policy __free(put_cpufreq_policy) = cpufreq_cpu_get(cpudata->cpu);
 	u8 nominal_perf = READ_ONCE(cpudata->nominal_perf);
 
 	if (!policy)
@@ -574,8 +574,6 @@ static void amd_pstate_update(struct amd_cpudata *cpudata, u8 min_perf,
 	}
 
 	amd_pstate_update_perf(cpudata, min_perf, des_perf, max_perf, 0, fast_switch);
-
-	cpufreq_cpu_put(policy);
 }
 
 static int amd_pstate_verify(struct cpufreq_policy_data *policy_data)
@@ -587,7 +585,8 @@ static int amd_pstate_verify(struct cpufreq_policy_data *policy_data)
 	 * amd-pstate qos_requests.
 	 */
 	if (policy_data->min == FREQ_QOS_MIN_DEFAULT_VALUE) {
-		struct cpufreq_policy *policy = cpufreq_cpu_get(policy_data->cpu);
+		struct cpufreq_policy *policy __free(put_cpufreq_policy) =
+					      cpufreq_cpu_get(policy_data->cpu);
 		struct amd_cpudata *cpudata;
 
 		if (!policy)
@@ -595,7 +594,6 @@ static int amd_pstate_verify(struct cpufreq_policy_data *policy_data)
 
 		cpudata = policy->driver_data;
 		policy_data->min = cpudata->lowest_nonlinear_freq;
-		cpufreq_cpu_put(policy);
 	}
 
 	cpufreq_verify_within_cpu_limits(policy_data);
@@ -678,7 +676,7 @@ static void amd_pstate_adjust_perf(unsigned int cpu,
 				   unsigned long capacity)
 {
 	u8 max_perf, min_perf, des_perf, cap_perf, min_limit_perf;
-	struct cpufreq_policy *policy = cpufreq_cpu_get(cpu);
+	struct cpufreq_policy *policy __free(put_cpufreq_policy) = cpufreq_cpu_get(cpu);
 	struct amd_cpudata *cpudata;
 
 	if (!policy)
@@ -710,7 +708,6 @@ static void amd_pstate_adjust_perf(unsigned int cpu,
 
 	amd_pstate_update(cpudata, min_perf, des_perf, max_perf, true,
 			policy->governor->flags);
-	cpufreq_cpu_put(policy);
 }
 
 static int amd_pstate_cpu_boost_update(struct cpufreq_policy *policy, bool on)
@@ -824,28 +821,23 @@ static void amd_pstate_init_prefcore(struct amd_cpudata *cpudata)
 
 static void amd_pstate_update_limits(unsigned int cpu)
 {
-	struct cpufreq_policy *policy = NULL;
+	struct cpufreq_policy *policy __free(put_cpufreq_policy) = cpufreq_cpu_get(cpu);
 	struct amd_cpudata *cpudata;
 	u32 prev_high = 0, cur_high = 0;
-	int ret;
 	bool highest_perf_changed = false;
 
 	if (!amd_pstate_prefcore)
 		return;
 
-	policy = cpufreq_cpu_get(cpu);
 	if (!policy)
 		return;
 
-	cpudata = policy->driver_data;
-
 	guard(mutex)(&amd_pstate_driver_lock);
 
-	ret = amd_get_highest_perf(cpu, &cur_high);
-	if (ret) {
-		cpufreq_cpu_put(policy);
+	if (amd_get_highest_perf(cpu, &cur_high))
 		return;
-	}
+
+	cpudata = policy->driver_data;
 
 	prev_high = READ_ONCE(cpudata->prefcore_ranking);
 	highest_perf_changed = (prev_high != cur_high);
@@ -855,7 +847,6 @@ static void amd_pstate_update_limits(unsigned int cpu)
 		if (cur_high < CPPC_MAX_PERF)
 			sched_set_itmt_core_prio((int)cur_high, cpu);
 	}
-	cpufreq_cpu_put(policy);
 }
 
 /*
diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h
index 7fe0981a7e46..dde5212d256c 100644
--- a/include/linux/cpufreq.h
+++ b/include/linux/cpufreq.h
@@ -210,6 +210,9 @@ static inline struct cpufreq_policy *cpufreq_cpu_get(unsigned int cpu)
 static inline void cpufreq_cpu_put(struct cpufreq_policy *policy) { }
 #endif
 
+/* Scope based cleanup macro for cpufreq_policy kobject reference counting */
+DEFINE_FREE(put_cpufreq_policy, struct cpufreq_policy *, if (_T) cpufreq_cpu_put(_T))
+
 static inline bool policy_is_inactive(struct cpufreq_policy *policy)
 {
 	return cpumask_empty(policy->cpus);
-- 
2.34.1

[PATCH 12/12] cpufreq/amd-pstate: Remove the unncecessary driver_lock in amd_pstate_update_limits

[Dhananjay Ugwekar]

There is no need to take a driver wide lock while updating the
highest_perf value in the percpu cpudata struct. Hence remove it.

Signed-off-by: Dhananjay Ugwekar <dhananjay.ugwekar@amd.com>
---
 drivers/cpufreq/amd-pstate.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c
index ee7e3f0a4c0a..08ae48076812 100644
--- a/drivers/cpufreq/amd-pstate.c
+++ b/drivers/cpufreq/amd-pstate.c
@@ -832,8 +832,6 @@ static void amd_pstate_update_limits(unsigned int cpu)
 	if (!policy)
 		return;
 
-	guard(mutex)(&amd_pstate_driver_lock);
-
 	if (amd_get_highest_perf(cpu, &cur_high))
 		return;
 
-- 
2.34.1

Re: [PATCH 01/12] cpufreq/amd-pstate: Remove the goto label in amd_pstate_update_limits

[Mario Limonciello]

On 2/5/2025 05:25, Dhananjay Ugwekar wrote:
> Scope based guard/cleanup macros should not be used together with goto
> labels. Hence, remove the goto label.
> 
> Fixes: 6c093d5a5b73 ("cpufreq/amd-pstate: convert mutex use to guard()")
> Signed-off-by: Dhananjay Ugwekar <dhananjay.ugwekar@amd.com>

Thanks.

Reviewed-by: Mario Limonciello <mario.limonciello@amd.com>

I'll queue this fix for 6.14-rc.

> ---
>   drivers/cpufreq/amd-pstate.c | 8 ++++----
>   1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c
> index 7120f035c0be..b163c1699821 100644
> --- a/drivers/cpufreq/amd-pstate.c
> +++ b/drivers/cpufreq/amd-pstate.c
> @@ -838,8 +838,10 @@ static void amd_pstate_update_limits(unsigned int cpu)
>   	guard(mutex)(&amd_pstate_driver_lock);
>   
>   	ret = amd_get_highest_perf(cpu, &cur_high);
> -	if (ret)
> -		goto free_cpufreq_put;
> +	if (ret) {
> +		cpufreq_cpu_put(policy);
> +		return;
> +	}
>   
>   	prev_high = READ_ONCE(cpudata->prefcore_ranking);
>   	highest_perf_changed = (prev_high != cur_high);
> @@ -849,8 +851,6 @@ static void amd_pstate_update_limits(unsigned int cpu)
>   		if (cur_high < CPPC_MAX_PERF)
>   			sched_set_itmt_core_prio((int)cur_high, cpu);
>   	}
> -
> -free_cpufreq_put:
>   	cpufreq_cpu_put(policy);
>   
>   	if (!highest_perf_changed)

Re: [PATCH 02/12] cpufreq/amd-pstate: Fix max_perf updation with schedutil

[Mario Limonciello]

On 2/5/2025 05:25, Dhananjay Ugwekar wrote:
> In adjust_perf() callback, we are setting the max_perf to highest_perf,
> as opposed to the correct limit value i.e. max_limit_perf. Fix that.
> 
> Fixes: 3f7b835fa4d0 ("cpufreq/amd-pstate: Move limit updating code")
> Signed-off-by: Dhananjay Ugwekar <dhananjay.ugwekar@amd.com>

Thanks.

Reviewed-by: Mario Limonciello <mario.limonciello@amd.com>

I'll queue this fix for 6.14-rc.
> ---
>   drivers/cpufreq/amd-pstate.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c
> index b163c1699821..9dc3933bc326 100644
> --- a/drivers/cpufreq/amd-pstate.c
> +++ b/drivers/cpufreq/amd-pstate.c
> @@ -699,7 +699,7 @@ static void amd_pstate_adjust_perf(unsigned int cpu,
>   	if (min_perf < lowest_nonlinear_perf)
>   		min_perf = lowest_nonlinear_perf;
>   
> -	max_perf = cap_perf;
> +	max_perf = cpudata->max_limit_perf;
>   	if (max_perf < min_perf)
>   		max_perf = min_perf;
>   

Re: [PATCH 03/12] cpufreq/amd-pstate: Modify the min_perf calculation in adjust_perf callback

[Mario Limonciello]

On 2/5/2025 05:25, Dhananjay Ugwekar wrote:
> Instead of setting a fixed floor at lowest_nonlinear_perf, use the
> min_limit_perf value, so that it gives the user the freedom to lower the
> floor further.
> 
> There are two minimum frequency/perf limits that we need to consider in
> the adjust_perf callback. One provided by schedutil i.e. the sg_cpu->bw_min
> value passed in _min_perf arg, another is the effective value of
> min_freq_qos request that is updated in cpudata->min_limit_perf. Modify the
> code to use the bigger of these two values.
> 
> Signed-off-by: Dhananjay Ugwekar <dhananjay.ugwekar@amd.com>

Reviewed-by: Mario Limonciello <mario.limonciello@amd.com>

I'll queue this for 6.15.

> ---
>   drivers/cpufreq/amd-pstate.c | 12 ++++++------
>   1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c
> index 9dc3933bc326..a23fb78a442b 100644
> --- a/drivers/cpufreq/amd-pstate.c
> +++ b/drivers/cpufreq/amd-pstate.c
> @@ -672,7 +672,7 @@ static void amd_pstate_adjust_perf(unsigned int cpu,
>   				   unsigned long capacity)
>   {
>   	unsigned long max_perf, min_perf, des_perf,
> -		      cap_perf, lowest_nonlinear_perf;
> +		      cap_perf, min_limit_perf;
>   	struct cpufreq_policy *policy = cpufreq_cpu_get(cpu);
>   	struct amd_cpudata *cpudata;
>   
> @@ -684,20 +684,20 @@ static void amd_pstate_adjust_perf(unsigned int cpu,
>   	if (policy->min != cpudata->min_limit_freq || policy->max != cpudata->max_limit_freq)
>   		amd_pstate_update_min_max_limit(policy);
>   
> -
>   	cap_perf = READ_ONCE(cpudata->highest_perf);
> -	lowest_nonlinear_perf = READ_ONCE(cpudata->lowest_nonlinear_perf);
> +	min_limit_perf = READ_ONCE(cpudata->min_limit_perf);
>   
>   	des_perf = cap_perf;
>   	if (target_perf < capacity)
>   		des_perf = DIV_ROUND_UP(cap_perf * target_perf, capacity);
>   
> -	min_perf = READ_ONCE(cpudata->lowest_perf);
>   	if (_min_perf < capacity)
>   		min_perf = DIV_ROUND_UP(cap_perf * _min_perf, capacity);
> +	else
> +		min_perf = cap_perf;
>   
> -	if (min_perf < lowest_nonlinear_perf)
> -		min_perf = lowest_nonlinear_perf;
> +	if (min_perf < min_limit_perf)
> +		min_perf = min_limit_perf;
>   
>   	max_perf = cpudata->max_limit_perf;
>   	if (max_perf < min_perf)

Re: [PATCH 04/12] cpufreq/amd-pstate: Remove the redundant des_perf clamping in adjust_perf

[Mario Limonciello]

On 2/5/2025 05:25, Dhananjay Ugwekar wrote:
> des_perf is later on clamped between min_perf and max_perf in
> amd_pstate_update. So, remove the redundant clamping from
> amd_pstate_adjust_perf.
> 
> Signed-off-by: Dhananjay Ugwekar <dhananjay.ugwekar@amd.com>

Reviewed-by: Mario Limonciello <mario.limonciello@amd.com>

I'll queue this for 6.15.

> ---
>   drivers/cpufreq/amd-pstate.c | 2 --
>   1 file changed, 2 deletions(-)
> 
> diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c
> index a23fb78a442b..2926f292be7b 100644
> --- a/drivers/cpufreq/amd-pstate.c
> +++ b/drivers/cpufreq/amd-pstate.c
> @@ -703,8 +703,6 @@ static void amd_pstate_adjust_perf(unsigned int cpu,
>   	if (max_perf < min_perf)
>   		max_perf = min_perf;
>   
> -	des_perf = clamp_t(unsigned long, des_perf, min_perf, max_perf);
> -
>   	amd_pstate_update(cpudata, min_perf, des_perf, max_perf, true,
>   			policy->governor->flags);
>   	cpufreq_cpu_put(policy);

Re: [PATCH 05/12] cpufreq/amd-pstate: Pass min/max_limit_perf as min/max_perf to amd_pstate_update

[Mario Limonciello]

On 2/5/2025 05:25, Dhananjay Ugwekar wrote:
> Currently, amd_pstate_update_freq passes the hardware perf limits as
> min/max_perf to amd_pstate_update, which eventually gets programmed into
> the min/max_perf fields of the CPPC_REQ register.
> 
> Instead pass the effective perf limits i.e. min/max_limit_perf values to
> amd_pstate_update as min/max_perf.
> 
> Signed-off-by: Dhananjay Ugwekar <dhananjay.ugwekar@amd.com>

Reviewed-by: Mario Limonciello <mario.limonciello@amd.com>

I'll queue this for 6.15.

> ---
>   drivers/cpufreq/amd-pstate.c | 9 ++++-----
>   1 file changed, 4 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c
> index 2926f292be7b..e179e929b941 100644
> --- a/drivers/cpufreq/amd-pstate.c
> +++ b/drivers/cpufreq/amd-pstate.c
> @@ -615,7 +615,7 @@ static int amd_pstate_update_freq(struct cpufreq_policy *policy,
>   {
>   	struct cpufreq_freqs freqs;
>   	struct amd_cpudata *cpudata = policy->driver_data;
> -	unsigned long max_perf, min_perf, des_perf, cap_perf;
> +	unsigned long des_perf, cap_perf;
>   
>   	if (!cpudata->max_freq)
>   		return -ENODEV;
> @@ -624,8 +624,6 @@ static int amd_pstate_update_freq(struct cpufreq_policy *policy,
>   		amd_pstate_update_min_max_limit(policy);
>   
>   	cap_perf = READ_ONCE(cpudata->highest_perf);
> -	min_perf = READ_ONCE(cpudata->lowest_perf);
> -	max_perf = cap_perf;
>   
>   	freqs.old = policy->cur;
>   	freqs.new = target_freq;
> @@ -642,8 +640,9 @@ static int amd_pstate_update_freq(struct cpufreq_policy *policy,
>   	if (!fast_switch)
>   		cpufreq_freq_transition_begin(policy, &freqs);
>   
> -	amd_pstate_update(cpudata, min_perf, des_perf,
> -			max_perf, fast_switch, policy->governor->flags);
> +	amd_pstate_update(cpudata, cpudata->min_limit_perf, des_perf,
> +			  cpudata->max_limit_perf, fast_switch,
> +			  policy->governor->flags);
>   
>   	if (!fast_switch)
>   		cpufreq_freq_transition_end(policy, &freqs, false);

Re: [PATCH 06/12] cpufreq/amd-pstate: Convert all perf values to u8

[Mario Limonciello]

On 2/5/2025 05:25, Dhananjay Ugwekar wrote:
> All perf values are always within 0-255 range, hence convert their
> datatype to u8 everywhere.
> 
> Signed-off-by: Dhananjay Ugwekar <dhananjay.ugwekar@amd.com>

Reviewed-by: Mario Limonciello <mario.limonciello@amd.com>

I'll queue this for 6.15.

> ---
>   drivers/cpufreq/amd-pstate-trace.h | 46 +++++++++++------------
>   drivers/cpufreq/amd-pstate.c       | 60 +++++++++++++++---------------
>   drivers/cpufreq/amd-pstate.h       | 18 ++++-----
>   3 files changed, 62 insertions(+), 62 deletions(-)
> 
> diff --git a/drivers/cpufreq/amd-pstate-trace.h b/drivers/cpufreq/amd-pstate-trace.h
> index 8d692415d905..f457d4af2c62 100644
> --- a/drivers/cpufreq/amd-pstate-trace.h
> +++ b/drivers/cpufreq/amd-pstate-trace.h
> @@ -24,9 +24,9 @@
>   
>   TRACE_EVENT(amd_pstate_perf,
>   
> -	TP_PROTO(unsigned long min_perf,
> -		 unsigned long target_perf,
> -		 unsigned long capacity,
> +	TP_PROTO(u8 min_perf,
> +		 u8 target_perf,
> +		 u8 capacity,
>   		 u64 freq,
>   		 u64 mperf,
>   		 u64 aperf,
> @@ -47,9 +47,9 @@ TRACE_EVENT(amd_pstate_perf,
>   		),
>   
>   	TP_STRUCT__entry(
> -		__field(unsigned long, min_perf)
> -		__field(unsigned long, target_perf)
> -		__field(unsigned long, capacity)
> +		__field(u8, min_perf)
> +		__field(u8, target_perf)
> +		__field(u8, capacity)
>   		__field(unsigned long long, freq)
>   		__field(unsigned long long, mperf)
>   		__field(unsigned long long, aperf)
> @@ -70,10 +70,10 @@ TRACE_EVENT(amd_pstate_perf,
>   		__entry->fast_switch = fast_switch;
>   		),
>   
> -	TP_printk("amd_min_perf=%lu amd_des_perf=%lu amd_max_perf=%lu freq=%llu mperf=%llu aperf=%llu tsc=%llu cpu_id=%u fast_switch=%s",
> -		  (unsigned long)__entry->min_perf,
> -		  (unsigned long)__entry->target_perf,
> -		  (unsigned long)__entry->capacity,
> +	TP_printk("amd_min_perf=%hhu amd_des_perf=%hhu amd_max_perf=%hhu freq=%llu mperf=%llu aperf=%llu tsc=%llu cpu_id=%u fast_switch=%s",
> +		  (u8)__entry->min_perf,
> +		  (u8)__entry->target_perf,
> +		  (u8)__entry->capacity,
>   		  (unsigned long long)__entry->freq,
>   		  (unsigned long long)__entry->mperf,
>   		  (unsigned long long)__entry->aperf,
> @@ -86,10 +86,10 @@ TRACE_EVENT(amd_pstate_perf,
>   TRACE_EVENT(amd_pstate_epp_perf,
>   
>   	TP_PROTO(unsigned int cpu_id,
> -		 unsigned int highest_perf,
> -		 unsigned int epp,
> -		 unsigned int min_perf,
> -		 unsigned int max_perf,
> +		 u8 highest_perf,
> +		 u8 epp,
> +		 u8 min_perf,
> +		 u8 max_perf,
>   		 bool boost
>   		 ),
>   
> @@ -102,10 +102,10 @@ TRACE_EVENT(amd_pstate_epp_perf,
>   
>   	TP_STRUCT__entry(
>   		__field(unsigned int, cpu_id)
> -		__field(unsigned int, highest_perf)
> -		__field(unsigned int, epp)
> -		__field(unsigned int, min_perf)
> -		__field(unsigned int, max_perf)
> +		__field(u8, highest_perf)
> +		__field(u8, epp)
> +		__field(u8, min_perf)
> +		__field(u8, max_perf)
>   		__field(bool, boost)
>   		),
>   
> @@ -118,12 +118,12 @@ TRACE_EVENT(amd_pstate_epp_perf,
>   		__entry->boost = boost;
>   		),
>   
> -	TP_printk("cpu%u: [%u<->%u]/%u, epp=%u, boost=%u",
> +	TP_printk("cpu%u: [%hhu<->%hhu]/%hhu, epp=%hhu, boost=%u",
>   		  (unsigned int)__entry->cpu_id,
> -		  (unsigned int)__entry->min_perf,
> -		  (unsigned int)__entry->max_perf,
> -		  (unsigned int)__entry->highest_perf,
> -		  (unsigned int)__entry->epp,
> +		  (u8)__entry->min_perf,
> +		  (u8)__entry->max_perf,
> +		  (u8)__entry->highest_perf,
> +		  (u8)__entry->epp,
>   		  (bool)__entry->boost
>   		 )
>   );
> diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c
> index e179e929b941..dd4f23fa2587 100644
> --- a/drivers/cpufreq/amd-pstate.c
> +++ b/drivers/cpufreq/amd-pstate.c
> @@ -186,7 +186,7 @@ static inline int get_mode_idx_from_str(const char *str, size_t size)
>   static DEFINE_MUTEX(amd_pstate_limits_lock);
>   static DEFINE_MUTEX(amd_pstate_driver_lock);
>   
> -static s16 msr_get_epp(struct amd_cpudata *cpudata)
> +static u8 msr_get_epp(struct amd_cpudata *cpudata)
>   {
>   	u64 value;
>   	int ret;
> @@ -207,7 +207,7 @@ static inline s16 amd_pstate_get_epp(struct amd_cpudata *cpudata)
>   	return static_call(amd_pstate_get_epp)(cpudata);
>   }
>   
> -static s16 shmem_get_epp(struct amd_cpudata *cpudata)
> +static u8 shmem_get_epp(struct amd_cpudata *cpudata)
>   {
>   	u64 epp;
>   	int ret;
> @@ -218,11 +218,11 @@ static s16 shmem_get_epp(struct amd_cpudata *cpudata)
>   		return ret;
>   	}
>   
> -	return (s16)(epp & 0xff);
> +	return FIELD_GET(AMD_CPPC_EPP_PERF_MASK, epp);
>   }
>   
> -static int msr_update_perf(struct amd_cpudata *cpudata, u32 min_perf,
> -			   u32 des_perf, u32 max_perf, u32 epp, bool fast_switch)
> +static int msr_update_perf(struct amd_cpudata *cpudata, u8 min_perf,
> +			   u8 des_perf, u8 max_perf, u8 epp, bool fast_switch)
>   {
>   	u64 value, prev;
>   
> @@ -257,15 +257,15 @@ static int msr_update_perf(struct amd_cpudata *cpudata, u32 min_perf,
>   DEFINE_STATIC_CALL(amd_pstate_update_perf, msr_update_perf);
>   
>   static inline int amd_pstate_update_perf(struct amd_cpudata *cpudata,
> -					  u32 min_perf, u32 des_perf,
> -					  u32 max_perf, u32 epp,
> +					  u8 min_perf, u8 des_perf,
> +					  u8 max_perf, u8 epp,
>   					  bool fast_switch)
>   {
>   	return static_call(amd_pstate_update_perf)(cpudata, min_perf, des_perf,
>   						   max_perf, epp, fast_switch);
>   }
>   
> -static int msr_set_epp(struct amd_cpudata *cpudata, u32 epp)
> +static int msr_set_epp(struct amd_cpudata *cpudata, u8 epp)
>   {
>   	u64 value, prev;
>   	int ret;
> @@ -292,12 +292,12 @@ static int msr_set_epp(struct amd_cpudata *cpudata, u32 epp)
>   
>   DEFINE_STATIC_CALL(amd_pstate_set_epp, msr_set_epp);
>   
> -static inline int amd_pstate_set_epp(struct amd_cpudata *cpudata, u32 epp)
> +static inline int amd_pstate_set_epp(struct amd_cpudata *cpudata, u8 epp)
>   {
>   	return static_call(amd_pstate_set_epp)(cpudata, epp);
>   }
>   
> -static int shmem_set_epp(struct amd_cpudata *cpudata, u32 epp)
> +static int shmem_set_epp(struct amd_cpudata *cpudata, u8 epp)
>   {
>   	int ret;
>   	struct cppc_perf_ctrls perf_ctrls;
> @@ -320,7 +320,7 @@ static int amd_pstate_set_energy_pref_index(struct cpufreq_policy *policy,
>   					    int pref_index)
>   {
>   	struct amd_cpudata *cpudata = policy->driver_data;
> -	int epp;
> +	u8 epp;
>   
>   	if (!pref_index)
>   		epp = cpudata->epp_default;
> @@ -479,8 +479,8 @@ static inline int amd_pstate_init_perf(struct amd_cpudata *cpudata)
>   	return static_call(amd_pstate_init_perf)(cpudata);
>   }
>   
> -static int shmem_update_perf(struct amd_cpudata *cpudata, u32 min_perf,
> -			     u32 des_perf, u32 max_perf, u32 epp, bool fast_switch)
> +static int shmem_update_perf(struct amd_cpudata *cpudata, u8 min_perf,
> +			     u8 des_perf, u8 max_perf, u8 epp, bool fast_switch)
>   {
>   	struct cppc_perf_ctrls perf_ctrls;
>   
> @@ -531,14 +531,14 @@ static inline bool amd_pstate_sample(struct amd_cpudata *cpudata)
>   	return true;
>   }
>   
> -static void amd_pstate_update(struct amd_cpudata *cpudata, u32 min_perf,
> -			      u32 des_perf, u32 max_perf, bool fast_switch, int gov_flags)
> +static void amd_pstate_update(struct amd_cpudata *cpudata, u8 min_perf,
> +			      u8 des_perf, u8 max_perf, bool fast_switch, int gov_flags)
>   {
>   	unsigned long max_freq;
>   	struct cpufreq_policy *policy = cpufreq_cpu_get(cpudata->cpu);
> -	u32 nominal_perf = READ_ONCE(cpudata->nominal_perf);
> +	u8 nominal_perf = READ_ONCE(cpudata->nominal_perf);
>   
> -	des_perf = clamp_t(unsigned long, des_perf, min_perf, max_perf);
> +	des_perf = clamp_t(u8, des_perf, min_perf, max_perf);
>   
>   	max_freq = READ_ONCE(cpudata->max_limit_freq);
>   	policy->cur = div_u64(des_perf * max_freq, max_perf);
> @@ -550,7 +550,7 @@ static void amd_pstate_update(struct amd_cpudata *cpudata, u32 min_perf,
>   
>   	/* limit the max perf when core performance boost feature is disabled */
>   	if (!cpudata->boost_supported)
> -		max_perf = min_t(unsigned long, nominal_perf, max_perf);
> +		max_perf = min_t(u8, nominal_perf, max_perf);
>   
>   	if (trace_amd_pstate_perf_enabled() && amd_pstate_sample(cpudata)) {
>   		trace_amd_pstate_perf(min_perf, des_perf, max_perf, cpudata->freq,
> @@ -591,7 +591,8 @@ static int amd_pstate_verify(struct cpufreq_policy_data *policy_data)
>   
>   static int amd_pstate_update_min_max_limit(struct cpufreq_policy *policy)
>   {
> -	u32 max_limit_perf, min_limit_perf, max_perf, max_freq;
> +	u8 max_limit_perf, min_limit_perf, max_perf;
> +	u32 max_freq;
>   	struct amd_cpudata *cpudata = policy->driver_data;
>   
>   	max_perf = READ_ONCE(cpudata->highest_perf);
> @@ -615,7 +616,7 @@ static int amd_pstate_update_freq(struct cpufreq_policy *policy,
>   {
>   	struct cpufreq_freqs freqs;
>   	struct amd_cpudata *cpudata = policy->driver_data;
> -	unsigned long des_perf, cap_perf;
> +	u8 des_perf, cap_perf;
>   
>   	if (!cpudata->max_freq)
>   		return -ENODEV;
> @@ -670,8 +671,7 @@ static void amd_pstate_adjust_perf(unsigned int cpu,
>   				   unsigned long target_perf,
>   				   unsigned long capacity)
>   {
> -	unsigned long max_perf, min_perf, des_perf,
> -		      cap_perf, min_limit_perf;
> +	u8 max_perf, min_perf, des_perf, cap_perf, min_limit_perf;
>   	struct cpufreq_policy *policy = cpufreq_cpu_get(cpu);
>   	struct amd_cpudata *cpudata;
>   
> @@ -904,8 +904,8 @@ static int amd_pstate_init_freq(struct amd_cpudata *cpudata)
>   {
>   	int ret;
>   	u32 min_freq, max_freq;
> -	u32 highest_perf, nominal_perf, nominal_freq;
> -	u32 lowest_nonlinear_perf, lowest_nonlinear_freq;
> +	u8 highest_perf, nominal_perf, lowest_nonlinear_perf;
> +	u32 nominal_freq, lowest_nonlinear_freq;
>   	struct cppc_perf_caps cppc_perf;
>   
>   	ret = cppc_get_perf_caps(cpudata->cpu, &cppc_perf);
> @@ -1112,7 +1112,7 @@ static ssize_t show_amd_pstate_lowest_nonlinear_freq(struct cpufreq_policy *poli
>   static ssize_t show_amd_pstate_highest_perf(struct cpufreq_policy *policy,
>   					    char *buf)
>   {
> -	u32 perf;
> +	u8 perf;
>   	struct amd_cpudata *cpudata = policy->driver_data;
>   
>   	perf = READ_ONCE(cpudata->highest_perf);
> @@ -1123,7 +1123,7 @@ static ssize_t show_amd_pstate_highest_perf(struct cpufreq_policy *policy,
>   static ssize_t show_amd_pstate_prefcore_ranking(struct cpufreq_policy *policy,
>   						char *buf)
>   {
> -	u32 perf;
> +	u8 perf;
>   	struct amd_cpudata *cpudata = policy->driver_data;
>   
>   	perf = READ_ONCE(cpudata->prefcore_ranking);
> @@ -1186,7 +1186,7 @@ static ssize_t show_energy_performance_preference(
>   				struct cpufreq_policy *policy, char *buf)
>   {
>   	struct amd_cpudata *cpudata = policy->driver_data;
> -	int preference;
> +	u8 preference;
>   
>   	switch (cpudata->epp_cached) {
>   	case AMD_CPPC_EPP_PERFORMANCE:
> @@ -1548,7 +1548,7 @@ static void amd_pstate_epp_cpu_exit(struct cpufreq_policy *policy)
>   static int amd_pstate_epp_update_limit(struct cpufreq_policy *policy)
>   {
>   	struct amd_cpudata *cpudata = policy->driver_data;
> -	u32 epp;
> +	u8 epp;
>   
>   	amd_pstate_update_min_max_limit(policy);
>   
> @@ -1597,7 +1597,7 @@ static int amd_pstate_epp_set_policy(struct cpufreq_policy *policy)
>   static int amd_pstate_epp_reenable(struct cpufreq_policy *policy)
>   {
>   	struct amd_cpudata *cpudata = policy->driver_data;
> -	u64 max_perf;
> +	u8 max_perf;
>   	int ret;
>   
>   	ret = amd_pstate_cppc_enable(true);
> @@ -1634,7 +1634,7 @@ static int amd_pstate_epp_cpu_online(struct cpufreq_policy *policy)
>   static int amd_pstate_epp_cpu_offline(struct cpufreq_policy *policy)
>   {
>   	struct amd_cpudata *cpudata = policy->driver_data;
> -	int min_perf;
> +	u8 min_perf;
>   
>   	if (cpudata->suspended)
>   		return 0;
> diff --git a/drivers/cpufreq/amd-pstate.h b/drivers/cpufreq/amd-pstate.h
> index 9747e3be6cee..19d405c6d805 100644
> --- a/drivers/cpufreq/amd-pstate.h
> +++ b/drivers/cpufreq/amd-pstate.h
> @@ -70,13 +70,13 @@ struct amd_cpudata {
>   	struct	freq_qos_request req[2];
>   	u64	cppc_req_cached;
>   
> -	u32	highest_perf;
> -	u32	nominal_perf;
> -	u32	lowest_nonlinear_perf;
> -	u32	lowest_perf;
> -	u32     prefcore_ranking;
> -	u32     min_limit_perf;
> -	u32     max_limit_perf;
> +	u8	highest_perf;
> +	u8	nominal_perf;
> +	u8	lowest_nonlinear_perf;
> +	u8	lowest_perf;
> +	u8	prefcore_ranking;
> +	u8	min_limit_perf;
> +	u8	max_limit_perf;
>   	u32     min_limit_freq;
>   	u32     max_limit_freq;
>   
> @@ -93,11 +93,11 @@ struct amd_cpudata {
>   	bool	hw_prefcore;
>   
>   	/* EPP feature related attributes*/
> -	s16	epp_cached;
> +	u8	epp_cached;
>   	u32	policy;
>   	u64	cppc_cap1_cached;
>   	bool	suspended;
> -	s16	epp_default;
> +	u8	epp_default;
>   };
>   
>   /*

Re: [PATCH 07/12] cpufreq/amd-pstate: Modularize perf<->freq conversion

[Mario Limonciello]

On 2/5/2025 05:25, Dhananjay Ugwekar wrote:
> Delegate the perf<->frequency conversion to helper functions to reduce
> code duplication, and improve readability.
> 
> Signed-off-by: Dhananjay Ugwekar <dhananjay.ugwekar@amd.com>

Reviewed-by: Mario Limonciello <mario.limonciello@amd.com>

I'll queue this for 6.15.

> ---
>   drivers/cpufreq/amd-pstate.c | 57 +++++++++++++++++++-----------------
>   1 file changed, 30 insertions(+), 27 deletions(-)
> 
> diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c
> index dd4f23fa2587..346fac646eba 100644
> --- a/drivers/cpufreq/amd-pstate.c
> +++ b/drivers/cpufreq/amd-pstate.c
> @@ -142,6 +142,20 @@ static struct quirk_entry quirk_amd_7k62 = {
>   	.lowest_freq = 550,
>   };
>   
> +static inline u8 freq_to_perf(struct amd_cpudata *cpudata, unsigned int freq_val)
> +{
> +	u8 perf_val = DIV_ROUND_UP_ULL((u64)freq_val * cpudata->nominal_perf,
> +					cpudata->nominal_freq);
> +
> +	return clamp_t(u8, perf_val, cpudata->lowest_perf, cpudata->highest_perf);
> +}
> +
> +static inline u32 perf_to_freq(struct amd_cpudata *cpudata, u8 perf_val)
> +{
> +	return DIV_ROUND_UP_ULL((u64)cpudata->nominal_freq * perf_val,
> +				cpudata->nominal_perf);
> +}
> +
>   static int __init dmi_matched_7k62_bios_bug(const struct dmi_system_id *dmi)
>   {
>   	/**
> @@ -534,14 +548,12 @@ static inline bool amd_pstate_sample(struct amd_cpudata *cpudata)
>   static void amd_pstate_update(struct amd_cpudata *cpudata, u8 min_perf,
>   			      u8 des_perf, u8 max_perf, bool fast_switch, int gov_flags)
>   {
> -	unsigned long max_freq;
>   	struct cpufreq_policy *policy = cpufreq_cpu_get(cpudata->cpu);
>   	u8 nominal_perf = READ_ONCE(cpudata->nominal_perf);
>   
>   	des_perf = clamp_t(u8, des_perf, min_perf, max_perf);
>   
> -	max_freq = READ_ONCE(cpudata->max_limit_freq);
> -	policy->cur = div_u64(des_perf * max_freq, max_perf);
> +	policy->cur = perf_to_freq(cpudata, des_perf);
>   
>   	if ((cppc_state == AMD_PSTATE_GUIDED) && (gov_flags & CPUFREQ_GOV_DYNAMIC_SWITCHING)) {
>   		min_perf = des_perf;
> @@ -591,14 +603,11 @@ static int amd_pstate_verify(struct cpufreq_policy_data *policy_data)
>   
>   static int amd_pstate_update_min_max_limit(struct cpufreq_policy *policy)
>   {
> -	u8 max_limit_perf, min_limit_perf, max_perf;
> -	u32 max_freq;
> +	u8 max_limit_perf, min_limit_perf;
>   	struct amd_cpudata *cpudata = policy->driver_data;
>   
> -	max_perf = READ_ONCE(cpudata->highest_perf);
> -	max_freq = READ_ONCE(cpudata->max_freq);
> -	max_limit_perf = div_u64(policy->max * max_perf, max_freq);
> -	min_limit_perf = div_u64(policy->min * max_perf, max_freq);
> +	max_limit_perf = freq_to_perf(cpudata, policy->max);
> +	min_limit_perf = freq_to_perf(cpudata, policy->min);
>   
>   	if (cpudata->policy == CPUFREQ_POLICY_PERFORMANCE)
>   		min_limit_perf = min(cpudata->nominal_perf, max_limit_perf);
> @@ -616,21 +625,15 @@ static int amd_pstate_update_freq(struct cpufreq_policy *policy,
>   {
>   	struct cpufreq_freqs freqs;
>   	struct amd_cpudata *cpudata = policy->driver_data;
> -	u8 des_perf, cap_perf;
> -
> -	if (!cpudata->max_freq)
> -		return -ENODEV;
> +	u8 des_perf;
>   
>   	if (policy->min != cpudata->min_limit_freq || policy->max != cpudata->max_limit_freq)
>   		amd_pstate_update_min_max_limit(policy);
>   
> -	cap_perf = READ_ONCE(cpudata->highest_perf);
> -
>   	freqs.old = policy->cur;
>   	freqs.new = target_freq;
>   
> -	des_perf = DIV_ROUND_CLOSEST(target_freq * cap_perf,
> -				     cpudata->max_freq);
> +	des_perf = freq_to_perf(cpudata, target_freq);
>   
>   	WARN_ON(fast_switch && !policy->fast_switch_enabled);
>   	/*
> @@ -904,7 +907,6 @@ static int amd_pstate_init_freq(struct amd_cpudata *cpudata)
>   {
>   	int ret;
>   	u32 min_freq, max_freq;
> -	u8 highest_perf, nominal_perf, lowest_nonlinear_perf;
>   	u32 nominal_freq, lowest_nonlinear_freq;
>   	struct cppc_perf_caps cppc_perf;
>   
> @@ -922,16 +924,17 @@ static int amd_pstate_init_freq(struct amd_cpudata *cpudata)
>   	else
>   		nominal_freq = cppc_perf.nominal_freq;
>   
> -	highest_perf = READ_ONCE(cpudata->highest_perf);
> -	nominal_perf = READ_ONCE(cpudata->nominal_perf);
> -	max_freq = div_u64((u64)highest_perf * nominal_freq, nominal_perf);
> +	min_freq *= 1000;
> +	nominal_freq *= 1000;
> +
> +	WRITE_ONCE(cpudata->nominal_freq, nominal_freq);
> +	WRITE_ONCE(cpudata->min_freq, min_freq);
> +
> +	max_freq = perf_to_freq(cpudata, cpudata->highest_perf);
> +	lowest_nonlinear_freq = perf_to_freq(cpudata, cpudata->lowest_nonlinear_perf);
>   
> -	lowest_nonlinear_perf = READ_ONCE(cpudata->lowest_nonlinear_perf);
> -	lowest_nonlinear_freq = div_u64((u64)nominal_freq * lowest_nonlinear_perf, nominal_perf);
> -	WRITE_ONCE(cpudata->min_freq, min_freq * 1000);
> -	WRITE_ONCE(cpudata->lowest_nonlinear_freq, lowest_nonlinear_freq * 1000);
> -	WRITE_ONCE(cpudata->nominal_freq, nominal_freq * 1000);
> -	WRITE_ONCE(cpudata->max_freq, max_freq * 1000);
> +	WRITE_ONCE(cpudata->lowest_nonlinear_freq, lowest_nonlinear_freq);
> +	WRITE_ONCE(cpudata->max_freq, max_freq);
>   
>   	/**
>   	 * Below values need to be initialized correctly, otherwise driver will fail to load

Re: [PATCH 08/12] cpufreq/amd-pstate: Remove the unnecessary cpufreq_update_policy call

[Mario Limonciello]

On 2/5/2025 05:25, Dhananjay Ugwekar wrote:
> The update_limits callback is only called in two conditions.
> 
> * When the preferred core rankings change. In which case, we just need to
> change the prefcore ranking in the cpudata struct. As there are no changes
> to any of the perf values, there is no need to call cpufreq_update_policy()
> 
> * When the _PPC ACPI object changes, i.e. the highest allowed Pstate
> changes. The _PPC object is only used for a table based cpufreq driver
> like acpi-cpufreq, hence is irrelevant for CPPC based amd-pstate.
> 
> Hence, the cpufreq_update_policy() call becomes unnecessary and can be
> removed.
> 
> Signed-off-by: Dhananjay Ugwekar <dhananjay.ugwekar@amd.com>

Reviewed-by: Mario Limonciello <mario.limonciello@amd.com>

I'll queue this for 6.15.

> ---
>   drivers/cpufreq/amd-pstate.c | 4 ----
>   1 file changed, 4 deletions(-)
> 
> diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c
> index 346fac646eba..107ad953ce43 100644
> --- a/drivers/cpufreq/amd-pstate.c
> +++ b/drivers/cpufreq/amd-pstate.c
> @@ -852,10 +852,6 @@ static void amd_pstate_update_limits(unsigned int cpu)
>   			sched_set_itmt_core_prio((int)cur_high, cpu);
>   	}
>   	cpufreq_cpu_put(policy);
> -
> -	if (!highest_perf_changed)
> -		cpufreq_update_policy(cpu);
> -
>   }
>   
>   /*

Re: [PATCH 09/12] cpufreq/amd-pstate: Fix cpufreq_policy ref counting

[Mario Limonciello]

On 2/5/2025 05:25, Dhananjay Ugwekar wrote:
> amd_pstate_update_limits() takes a cpufreq_policy reference but doesn't
> decrement the refcount in one of the exit paths, fix that.
> 
> Fixes: 45722e777fd9 ("cpufreq: amd-pstate: Optimize amd_pstate_update_limits()")
> Signed-off-by: Dhananjay Ugwekar <dhananjay.ugwekar@amd.com>

Thanks.

Reviewed-by: Mario Limonciello <mario.limonciello@amd.com>

I'll queue this fix for 6.14-rc.
> ---
>   drivers/cpufreq/amd-pstate.c | 9 +++++----
>   1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c
> index 107ad953ce43..9c939be59042 100644
> --- a/drivers/cpufreq/amd-pstate.c
> +++ b/drivers/cpufreq/amd-pstate.c
> @@ -821,20 +821,21 @@ static void amd_pstate_init_prefcore(struct amd_cpudata *cpudata)
>   
>   static void amd_pstate_update_limits(unsigned int cpu)
>   {
> -	struct cpufreq_policy *policy = cpufreq_cpu_get(cpu);
> +	struct cpufreq_policy *policy = NULL;
>   	struct amd_cpudata *cpudata;
>   	u32 prev_high = 0, cur_high = 0;
>   	int ret;
>   	bool highest_perf_changed = false;
>   
> +	if (!amd_pstate_prefcore)
> +		return;
> +
> +	policy = cpufreq_cpu_get(cpu);
>   	if (!policy)
>   		return;
>   
>   	cpudata = policy->driver_data;
>   
> -	if (!amd_pstate_prefcore)
> -		return;
> -
>   	guard(mutex)(&amd_pstate_driver_lock);
>   
>   	ret = amd_get_highest_perf(cpu, &cur_high);

Re: [PATCH 10/12] cpufreq/amd-pstate: Add missing NULL ptr check in amd_pstate_update

[Mario Limonciello]

On 2/5/2025 05:25, Dhananjay Ugwekar wrote:
> Check if policy is NULL before dereferencing it in amd_pstate_update.
> 
> Fixes: e8f555daacd3 ("cpufreq/amd-pstate: fix setting policy current frequency value")
> Signed-off-by: Dhananjay Ugwekar <dhananjay.ugwekar@amd.com>
> ---

Thanks.

Reviewed-by: Mario Limonciello <mario.limonciello@amd.com>

I'll queue this fix for 6.14-rc.

>   drivers/cpufreq/amd-pstate.c | 3 +++
>   1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c
> index 9c939be59042..6a604f0797d9 100644
> --- a/drivers/cpufreq/amd-pstate.c
> +++ b/drivers/cpufreq/amd-pstate.c
> @@ -551,6 +551,9 @@ static void amd_pstate_update(struct amd_cpudata *cpudata, u8 min_perf,
>   	struct cpufreq_policy *policy = cpufreq_cpu_get(cpudata->cpu);
>   	u8 nominal_perf = READ_ONCE(cpudata->nominal_perf);
>   
> +	if (!policy)
> +		return;
> +
>   	des_perf = clamp_t(u8, des_perf, min_perf, max_perf);
>   
>   	policy->cur = perf_to_freq(cpudata, des_perf);

Re: [PATCH 12/12] cpufreq/amd-pstate: Remove the unncecessary driver_lock in amd_pstate_update_limits

[Mario Limonciello]

On 2/5/2025 05:25, Dhananjay Ugwekar wrote:
> There is no need to take a driver wide lock while updating the
> highest_perf value in the percpu cpudata struct. Hence remove it.
> 
> Signed-off-by: Dhananjay Ugwekar <dhananjay.ugwekar@amd.com>

Reviewed-by: Mario Limonciello <mario.limonciello@amd.com>

> ---
>   drivers/cpufreq/amd-pstate.c | 2 --
>   1 file changed, 2 deletions(-)
> 
> diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c
> index ee7e3f0a4c0a..08ae48076812 100644
> --- a/drivers/cpufreq/amd-pstate.c
> +++ b/drivers/cpufreq/amd-pstate.c
> @@ -832,8 +832,6 @@ static void amd_pstate_update_limits(unsigned int cpu)
>   	if (!policy)
>   		return;
>   
> -	guard(mutex)(&amd_pstate_driver_lock);
> -
>   	if (amd_get_highest_perf(cpu, &cur_high))
>   		return;
>   

Re: [PATCH 11/12] cpufreq/amd-pstate: Use scope based cleanup for cpufreq_policy refs

[Mario Limonciello]

On 2/5/2025 05:25, Dhananjay Ugwekar wrote:
> There have been instances in past where refcount decrementing is missed
> while exiting a function. Use automatic scope based cleanup to avoid
> such errors.
> 
> Signed-off-by: Dhananjay Ugwekar <dhananjay.ugwekar@amd.com>

Reviewed-by: Mario Limonciello <mario.limonciello@amd.com>

> ---
>   drivers/cpufreq/amd-pstate.c | 25 ++++++++-----------------
>   include/linux/cpufreq.h      |  3 +++
>   2 files changed, 11 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c
> index 6a604f0797d9..ee7e3f0a4c0a 100644
> --- a/drivers/cpufreq/amd-pstate.c
> +++ b/drivers/cpufreq/amd-pstate.c
> @@ -548,7 +548,7 @@ static inline bool amd_pstate_sample(struct amd_cpudata *cpudata)
>   static void amd_pstate_update(struct amd_cpudata *cpudata, u8 min_perf,
>   			      u8 des_perf, u8 max_perf, bool fast_switch, int gov_flags)
>   {
> -	struct cpufreq_policy *policy = cpufreq_cpu_get(cpudata->cpu);
> +	struct cpufreq_policy *policy __free(put_cpufreq_policy) = cpufreq_cpu_get(cpudata->cpu);
>   	u8 nominal_perf = READ_ONCE(cpudata->nominal_perf);
>   
>   	if (!policy)
> @@ -574,8 +574,6 @@ static void amd_pstate_update(struct amd_cpudata *cpudata, u8 min_perf,
>   	}
>   
>   	amd_pstate_update_perf(cpudata, min_perf, des_perf, max_perf, 0, fast_switch);
> -
> -	cpufreq_cpu_put(policy);
>   }
>   
>   static int amd_pstate_verify(struct cpufreq_policy_data *policy_data)
> @@ -587,7 +585,8 @@ static int amd_pstate_verify(struct cpufreq_policy_data *policy_data)
>   	 * amd-pstate qos_requests.
>   	 */
>   	if (policy_data->min == FREQ_QOS_MIN_DEFAULT_VALUE) {
> -		struct cpufreq_policy *policy = cpufreq_cpu_get(policy_data->cpu);
> +		struct cpufreq_policy *policy __free(put_cpufreq_policy) =
> +					      cpufreq_cpu_get(policy_data->cpu);
>   		struct amd_cpudata *cpudata;
>   
>   		if (!policy)
> @@ -595,7 +594,6 @@ static int amd_pstate_verify(struct cpufreq_policy_data *policy_data)
>   
>   		cpudata = policy->driver_data;
>   		policy_data->min = cpudata->lowest_nonlinear_freq;
> -		cpufreq_cpu_put(policy);
>   	}
>   
>   	cpufreq_verify_within_cpu_limits(policy_data);
> @@ -678,7 +676,7 @@ static void amd_pstate_adjust_perf(unsigned int cpu,
>   				   unsigned long capacity)
>   {
>   	u8 max_perf, min_perf, des_perf, cap_perf, min_limit_perf;
> -	struct cpufreq_policy *policy = cpufreq_cpu_get(cpu);
> +	struct cpufreq_policy *policy __free(put_cpufreq_policy) = cpufreq_cpu_get(cpu);
>   	struct amd_cpudata *cpudata;
>   
>   	if (!policy)
> @@ -710,7 +708,6 @@ static void amd_pstate_adjust_perf(unsigned int cpu,
>   
>   	amd_pstate_update(cpudata, min_perf, des_perf, max_perf, true,
>   			policy->governor->flags);
> -	cpufreq_cpu_put(policy);
>   }
>   
>   static int amd_pstate_cpu_boost_update(struct cpufreq_policy *policy, bool on)
> @@ -824,28 +821,23 @@ static void amd_pstate_init_prefcore(struct amd_cpudata *cpudata)
>   
>   static void amd_pstate_update_limits(unsigned int cpu)
>   {
> -	struct cpufreq_policy *policy = NULL;
> +	struct cpufreq_policy *policy __free(put_cpufreq_policy) = cpufreq_cpu_get(cpu);
>   	struct amd_cpudata *cpudata;
>   	u32 prev_high = 0, cur_high = 0;
> -	int ret;
>   	bool highest_perf_changed = false;
>   
>   	if (!amd_pstate_prefcore)
>   		return;
>   
> -	policy = cpufreq_cpu_get(cpu);
>   	if (!policy)
>   		return;
>   
> -	cpudata = policy->driver_data;
> -
>   	guard(mutex)(&amd_pstate_driver_lock);
>   
> -	ret = amd_get_highest_perf(cpu, &cur_high);
> -	if (ret) {
> -		cpufreq_cpu_put(policy);
> +	if (amd_get_highest_perf(cpu, &cur_high))
>   		return;
> -	}
> +
> +	cpudata = policy->driver_data;
>   
>   	prev_high = READ_ONCE(cpudata->prefcore_ranking);
>   	highest_perf_changed = (prev_high != cur_high);
> @@ -855,7 +847,6 @@ static void amd_pstate_update_limits(unsigned int cpu)
>   		if (cur_high < CPPC_MAX_PERF)
>   			sched_set_itmt_core_prio((int)cur_high, cpu);
>   	}
> -	cpufreq_cpu_put(policy);
>   }
>   
>   /*
> diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h
> index 7fe0981a7e46..dde5212d256c 100644
> --- a/include/linux/cpufreq.h
> +++ b/include/linux/cpufreq.h
> @@ -210,6 +210,9 @@ static inline struct cpufreq_policy *cpufreq_cpu_get(unsigned int cpu)
>   static inline void cpufreq_cpu_put(struct cpufreq_policy *policy) { }
>   #endif
>   
> +/* Scope based cleanup macro for cpufreq_policy kobject reference counting */
> +DEFINE_FREE(put_cpufreq_policy, struct cpufreq_policy *, if (_T) cpufreq_cpu_put(_T))
> +
>   static inline bool policy_is_inactive(struct cpufreq_policy *policy)
>   {
>   	return cpumask_empty(policy->cpus);

Re: [PATCH 01/12] cpufreq/amd-pstate: Remove the goto label in amd_pstate_update_limits

[Gautham R. Shenoy]

On Wed, Feb 05, 2025 at 11:25:12AM +0000, Dhananjay Ugwekar wrote:
> Scope based guard/cleanup macros should not be used together with goto
> labels. Hence, remove the goto label.
> 
> Fixes: 6c093d5a5b73 ("cpufreq/amd-pstate: convert mutex use to guard()")
> Signed-off-by: Dhananjay Ugwekar <dhananjay.ugwekar@amd.com>


Reviewed-by: Gautham R. Shenoy <gautham.shenoy@amd.com>

-- 
Thanks and Regards
gautham.

> ---
>  drivers/cpufreq/amd-pstate.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c
> index 7120f035c0be..b163c1699821 100644
> --- a/drivers/cpufreq/amd-pstate.c
> +++ b/drivers/cpufreq/amd-pstate.c
> @@ -838,8 +838,10 @@ static void amd_pstate_update_limits(unsigned int cpu)
>  	guard(mutex)(&amd_pstate_driver_lock);
>  
>  	ret = amd_get_highest_perf(cpu, &cur_high);
> -	if (ret)
> -		goto free_cpufreq_put;
> +	if (ret) {
> +		cpufreq_cpu_put(policy);
> +		return;
> +	}
>  
>  	prev_high = READ_ONCE(cpudata->prefcore_ranking);
>  	highest_perf_changed = (prev_high != cur_high);
> @@ -849,8 +851,6 @@ static void amd_pstate_update_limits(unsigned int cpu)
>  		if (cur_high < CPPC_MAX_PERF)
>  			sched_set_itmt_core_prio((int)cur_high, cpu);
>  	}
> -
> -free_cpufreq_put:
>  	cpufreq_cpu_put(policy);
>  
>  	if (!highest_perf_changed)
> -- 
> 2.34.1
> 

Re: [PATCH 02/12] cpufreq/amd-pstate: Fix max_perf updation with schedutil

[Gautham R. Shenoy]

On Wed, Feb 05, 2025 at 11:25:13AM +0000, Dhananjay Ugwekar wrote:
> In adjust_perf() callback, we are setting the max_perf to highest_perf,
> as opposed to the correct limit value i.e. max_limit_perf. Fix that.
> 
> Fixes: 3f7b835fa4d0 ("cpufreq/amd-pstate: Move limit updating code")
> Signed-off-by: Dhananjay Ugwekar <dhananjay.ugwekar@amd.com>

Reviewed-by: Gautham R. Shenoy <gautham.shenoy@amd.com>

-- 
Thanks and Regards
gautham.
> ---
>  drivers/cpufreq/amd-pstate.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c
> index b163c1699821..9dc3933bc326 100644
> --- a/drivers/cpufreq/amd-pstate.c
> +++ b/drivers/cpufreq/amd-pstate.c
> @@ -699,7 +699,7 @@ static void amd_pstate_adjust_perf(unsigned int cpu,
>  	if (min_perf < lowest_nonlinear_perf)
>  		min_perf = lowest_nonlinear_perf;
>  
> -	max_perf = cap_perf;
> +	max_perf = cpudata->max_limit_perf;
>  	if (max_perf < min_perf)
>  		max_perf = min_perf;
>  
> -- 
> 2.34.1
> 

Re: [PATCH 03/12] cpufreq/amd-pstate: Modify the min_perf calculation in adjust_perf callback

[Gautham R. Shenoy]

On Wed, Feb 05, 2025 at 11:25:14AM +0000, Dhananjay Ugwekar wrote:
> Instead of setting a fixed floor at lowest_nonlinear_perf, use the
> min_limit_perf value, so that it gives the user the freedom to lower the
> floor further.
> 
> There are two minimum frequency/perf limits that we need to consider in
> the adjust_perf callback. One provided by schedutil i.e. the sg_cpu->bw_min
> value passed in _min_perf arg, another is the effective value of
> min_freq_qos request that is updated in cpudata->min_limit_perf. Modify the
> code to use the bigger of these two values.
> 
> Signed-off-by: Dhananjay Ugwekar <dhananjay.ugwekar@amd.com>

Reviewed-by: Gautham R. Shenoy <gautham.shenoy@amd.com>

-- 
Thanks and Regards
gautham.
> ---
>  drivers/cpufreq/amd-pstate.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c
> index 9dc3933bc326..a23fb78a442b 100644
> --- a/drivers/cpufreq/amd-pstate.c
> +++ b/drivers/cpufreq/amd-pstate.c
> @@ -672,7 +672,7 @@ static void amd_pstate_adjust_perf(unsigned int cpu,
>  				   unsigned long capacity)
>  {
>  	unsigned long max_perf, min_perf, des_perf,
> -		      cap_perf, lowest_nonlinear_perf;
> +		      cap_perf, min_limit_perf;
>  	struct cpufreq_policy *policy = cpufreq_cpu_get(cpu);
>  	struct amd_cpudata *cpudata;
>  
> @@ -684,20 +684,20 @@ static void amd_pstate_adjust_perf(unsigned int cpu,
>  	if (policy->min != cpudata->min_limit_freq || policy->max != cpudata->max_limit_freq)
>  		amd_pstate_update_min_max_limit(policy);
>  
> -
>  	cap_perf = READ_ONCE(cpudata->highest_perf);
> -	lowest_nonlinear_perf = READ_ONCE(cpudata->lowest_nonlinear_perf);
> +	min_limit_perf = READ_ONCE(cpudata->min_limit_perf);
>  
>  	des_perf = cap_perf;
>  	if (target_perf < capacity)
>  		des_perf = DIV_ROUND_UP(cap_perf * target_perf, capacity);
>  
> -	min_perf = READ_ONCE(cpudata->lowest_perf);
>  	if (_min_perf < capacity)
>  		min_perf = DIV_ROUND_UP(cap_perf * _min_perf, capacity);
> +	else
> +		min_perf = cap_perf;
>  
> -	if (min_perf < lowest_nonlinear_perf)
> -		min_perf = lowest_nonlinear_perf;
> +	if (min_perf < min_limit_perf)
> +		min_perf = min_limit_perf;
>  
>  	max_perf = cpudata->max_limit_perf;
>  	if (max_perf < min_perf)
> -- 
> 2.34.1
> 

Re: [PATCH 04/12] cpufreq/amd-pstate: Remove the redundant des_perf clamping in adjust_perf

[Gautham R. Shenoy]

On Wed, Feb 05, 2025 at 11:25:15AM +0000, Dhananjay Ugwekar wrote:
> des_perf is later on clamped between min_perf and max_perf in
> amd_pstate_update. So, remove the redundant clamping from
> amd_pstate_adjust_perf.
> 
> Signed-off-by: Dhananjay Ugwekar <dhananjay.ugwekar@amd.com>

Nice catch.

Reviewed-by: Gautham R. Shenoy <gautham.shenoy@amd.com>

-- 
Thanks and Regards
gautham.
> ---
>  drivers/cpufreq/amd-pstate.c | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c
> index a23fb78a442b..2926f292be7b 100644
> --- a/drivers/cpufreq/amd-pstate.c
> +++ b/drivers/cpufreq/amd-pstate.c
> @@ -703,8 +703,6 @@ static void amd_pstate_adjust_perf(unsigned int cpu,
>  	if (max_perf < min_perf)
>  		max_perf = min_perf;
>  
> -	des_perf = clamp_t(unsigned long, des_perf, min_perf, max_perf);
> -
>  	amd_pstate_update(cpudata, min_perf, des_perf, max_perf, true,
>  			policy->governor->flags);
>  	cpufreq_cpu_put(policy);
> -- 
> 2.34.1
> 

Re: [PATCH 05/12] cpufreq/amd-pstate: Pass min/max_limit_perf as min/max_perf to amd_pstate_update

[Gautham R. Shenoy]

On Wed, Feb 05, 2025 at 11:25:16AM +0000, Dhananjay Ugwekar wrote:
> Currently, amd_pstate_update_freq passes the hardware perf limits as
> min/max_perf to amd_pstate_update, which eventually gets programmed into
> the min/max_perf fields of the CPPC_REQ register.
> 
> Instead pass the effective perf limits i.e. min/max_limit_perf values to
> amd_pstate_update as min/max_perf.

Reviewed-by: Gautham R. Shenoy <gautham.shenoy@amd.com>

-- 
Thanks and Regards
gautham.
> 
> Signed-off-by: Dhananjay Ugwekar <dhananjay.ugwekar@amd.com>
> ---
>  drivers/cpufreq/amd-pstate.c | 9 ++++-----
>  1 file changed, 4 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c
> index 2926f292be7b..e179e929b941 100644
> --- a/drivers/cpufreq/amd-pstate.c
> +++ b/drivers/cpufreq/amd-pstate.c
> @@ -615,7 +615,7 @@ static int amd_pstate_update_freq(struct cpufreq_policy *policy,
>  {
>  	struct cpufreq_freqs freqs;
>  	struct amd_cpudata *cpudata = policy->driver_data;
> -	unsigned long max_perf, min_perf, des_perf, cap_perf;
> +	unsigned long des_perf, cap_perf;
>  
>  	if (!cpudata->max_freq)
>  		return -ENODEV;
> @@ -624,8 +624,6 @@ static int amd_pstate_update_freq(struct cpufreq_policy *policy,
>  		amd_pstate_update_min_max_limit(policy);
>  
>  	cap_perf = READ_ONCE(cpudata->highest_perf);
> -	min_perf = READ_ONCE(cpudata->lowest_perf);
> -	max_perf = cap_perf;
>  
>  	freqs.old = policy->cur;
>  	freqs.new = target_freq;
> @@ -642,8 +640,9 @@ static int amd_pstate_update_freq(struct cpufreq_policy *policy,
>  	if (!fast_switch)
>  		cpufreq_freq_transition_begin(policy, &freqs);
>  
> -	amd_pstate_update(cpudata, min_perf, des_perf,
> -			max_perf, fast_switch, policy->governor->flags);
> +	amd_pstate_update(cpudata, cpudata->min_limit_perf, des_perf,
> +			  cpudata->max_limit_perf, fast_switch,
> +			  policy->governor->flags);
>  
>  	if (!fast_switch)
>  		cpufreq_freq_transition_end(policy, &freqs, false);
> -- 
> 2.34.1
> 

Re: [PATCH 06/12] cpufreq/amd-pstate: Convert all perf values to u8

[Gautham R. Shenoy]

On Wed, Feb 05, 2025 at 11:25:17AM +0000, Dhananjay Ugwekar wrote:
> All perf values are always within 0-255 range, hence convert their
> datatype to u8 everywhere.
> 
> Signed-off-by: Dhananjay Ugwekar <dhananjay.ugwekar@amd.com>

Reviewed-by: Gautham R. Shenoy <gautham.shenoy@amd.com>

-- 
Thanks and Regards
gautham.


> ---
>  drivers/cpufreq/amd-pstate-trace.h | 46 +++++++++++------------
>  drivers/cpufreq/amd-pstate.c       | 60 +++++++++++++++---------------
>  drivers/cpufreq/amd-pstate.h       | 18 ++++-----
>  3 files changed, 62 insertions(+), 62 deletions(-)
> 
> diff --git a/drivers/cpufreq/amd-pstate-trace.h b/drivers/cpufreq/amd-pstate-trace.h
> index 8d692415d905..f457d4af2c62 100644
> --- a/drivers/cpufreq/amd-pstate-trace.h
> +++ b/drivers/cpufreq/amd-pstate-trace.h
> @@ -24,9 +24,9 @@
>  
>  TRACE_EVENT(amd_pstate_perf,
>  
> -	TP_PROTO(unsigned long min_perf,
> -		 unsigned long target_perf,
> -		 unsigned long capacity,
> +	TP_PROTO(u8 min_perf,
> +		 u8 target_perf,
> +		 u8 capacity,
>  		 u64 freq,
>  		 u64 mperf,
>  		 u64 aperf,
> @@ -47,9 +47,9 @@ TRACE_EVENT(amd_pstate_perf,
>  		),
>  
>  	TP_STRUCT__entry(
> -		__field(unsigned long, min_perf)
> -		__field(unsigned long, target_perf)
> -		__field(unsigned long, capacity)
> +		__field(u8, min_perf)
> +		__field(u8, target_perf)
> +		__field(u8, capacity)
>  		__field(unsigned long long, freq)
>  		__field(unsigned long long, mperf)
>  		__field(unsigned long long, aperf)
> @@ -70,10 +70,10 @@ TRACE_EVENT(amd_pstate_perf,
>  		__entry->fast_switch = fast_switch;
>  		),
>  
> -	TP_printk("amd_min_perf=%lu amd_des_perf=%lu amd_max_perf=%lu freq=%llu mperf=%llu aperf=%llu tsc=%llu cpu_id=%u fast_switch=%s",
> -		  (unsigned long)__entry->min_perf,
> -		  (unsigned long)__entry->target_perf,
> -		  (unsigned long)__entry->capacity,
> +	TP_printk("amd_min_perf=%hhu amd_des_perf=%hhu amd_max_perf=%hhu freq=%llu mperf=%llu aperf=%llu tsc=%llu cpu_id=%u fast_switch=%s",
> +		  (u8)__entry->min_perf,
> +		  (u8)__entry->target_perf,
> +		  (u8)__entry->capacity,
>  		  (unsigned long long)__entry->freq,
>  		  (unsigned long long)__entry->mperf,
>  		  (unsigned long long)__entry->aperf,
> @@ -86,10 +86,10 @@ TRACE_EVENT(amd_pstate_perf,
>  TRACE_EVENT(amd_pstate_epp_perf,
>  
>  	TP_PROTO(unsigned int cpu_id,
> -		 unsigned int highest_perf,
> -		 unsigned int epp,
> -		 unsigned int min_perf,
> -		 unsigned int max_perf,
> +		 u8 highest_perf,
> +		 u8 epp,
> +		 u8 min_perf,
> +		 u8 max_perf,
>  		 bool boost
>  		 ),
>  
> @@ -102,10 +102,10 @@ TRACE_EVENT(amd_pstate_epp_perf,
>  
>  	TP_STRUCT__entry(
>  		__field(unsigned int, cpu_id)
> -		__field(unsigned int, highest_perf)
> -		__field(unsigned int, epp)
> -		__field(unsigned int, min_perf)
> -		__field(unsigned int, max_perf)
> +		__field(u8, highest_perf)
> +		__field(u8, epp)
> +		__field(u8, min_perf)
> +		__field(u8, max_perf)
>  		__field(bool, boost)
>  		),
>  
> @@ -118,12 +118,12 @@ TRACE_EVENT(amd_pstate_epp_perf,
>  		__entry->boost = boost;
>  		),
>  
> -	TP_printk("cpu%u: [%u<->%u]/%u, epp=%u, boost=%u",
> +	TP_printk("cpu%u: [%hhu<->%hhu]/%hhu, epp=%hhu, boost=%u",
>  		  (unsigned int)__entry->cpu_id,
> -		  (unsigned int)__entry->min_perf,
> -		  (unsigned int)__entry->max_perf,
> -		  (unsigned int)__entry->highest_perf,
> -		  (unsigned int)__entry->epp,
> +		  (u8)__entry->min_perf,
> +		  (u8)__entry->max_perf,
> +		  (u8)__entry->highest_perf,
> +		  (u8)__entry->epp,
>  		  (bool)__entry->boost
>  		 )
>  );
> diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c
> index e179e929b941..dd4f23fa2587 100644
> --- a/drivers/cpufreq/amd-pstate.c
> +++ b/drivers/cpufreq/amd-pstate.c
> @@ -186,7 +186,7 @@ static inline int get_mode_idx_from_str(const char *str, size_t size)
>  static DEFINE_MUTEX(amd_pstate_limits_lock);
>  static DEFINE_MUTEX(amd_pstate_driver_lock);
>  
> -static s16 msr_get_epp(struct amd_cpudata *cpudata)
> +static u8 msr_get_epp(struct amd_cpudata *cpudata)
>  {
>  	u64 value;
>  	int ret;
> @@ -207,7 +207,7 @@ static inline s16 amd_pstate_get_epp(struct amd_cpudata *cpudata)
>  	return static_call(amd_pstate_get_epp)(cpudata);
>  }
>  
> -static s16 shmem_get_epp(struct amd_cpudata *cpudata)
> +static u8 shmem_get_epp(struct amd_cpudata *cpudata)
>  {
>  	u64 epp;
>  	int ret;
> @@ -218,11 +218,11 @@ static s16 shmem_get_epp(struct amd_cpudata *cpudata)
>  		return ret;
>  	}
>  
> -	return (s16)(epp & 0xff);
> +	return FIELD_GET(AMD_CPPC_EPP_PERF_MASK, epp);
>  }
>  
> -static int msr_update_perf(struct amd_cpudata *cpudata, u32 min_perf,
> -			   u32 des_perf, u32 max_perf, u32 epp, bool fast_switch)
> +static int msr_update_perf(struct amd_cpudata *cpudata, u8 min_perf,
> +			   u8 des_perf, u8 max_perf, u8 epp, bool fast_switch)
>  {
>  	u64 value, prev;
>  
> @@ -257,15 +257,15 @@ static int msr_update_perf(struct amd_cpudata *cpudata, u32 min_perf,
>  DEFINE_STATIC_CALL(amd_pstate_update_perf, msr_update_perf);
>  
>  static inline int amd_pstate_update_perf(struct amd_cpudata *cpudata,
> -					  u32 min_perf, u32 des_perf,
> -					  u32 max_perf, u32 epp,
> +					  u8 min_perf, u8 des_perf,
> +					  u8 max_perf, u8 epp,
>  					  bool fast_switch)
>  {
>  	return static_call(amd_pstate_update_perf)(cpudata, min_perf, des_perf,
>  						   max_perf, epp, fast_switch);
>  }
>  
> -static int msr_set_epp(struct amd_cpudata *cpudata, u32 epp)
> +static int msr_set_epp(struct amd_cpudata *cpudata, u8 epp)
>  {
>  	u64 value, prev;
>  	int ret;
> @@ -292,12 +292,12 @@ static int msr_set_epp(struct amd_cpudata *cpudata, u32 epp)
>  
>  DEFINE_STATIC_CALL(amd_pstate_set_epp, msr_set_epp);
>  
> -static inline int amd_pstate_set_epp(struct amd_cpudata *cpudata, u32 epp)
> +static inline int amd_pstate_set_epp(struct amd_cpudata *cpudata, u8 epp)
>  {
>  	return static_call(amd_pstate_set_epp)(cpudata, epp);
>  }
>  
> -static int shmem_set_epp(struct amd_cpudata *cpudata, u32 epp)
> +static int shmem_set_epp(struct amd_cpudata *cpudata, u8 epp)
>  {
>  	int ret;
>  	struct cppc_perf_ctrls perf_ctrls;
> @@ -320,7 +320,7 @@ static int amd_pstate_set_energy_pref_index(struct cpufreq_policy *policy,
>  					    int pref_index)
>  {
>  	struct amd_cpudata *cpudata = policy->driver_data;
> -	int epp;
> +	u8 epp;
>  
>  	if (!pref_index)
>  		epp = cpudata->epp_default;
> @@ -479,8 +479,8 @@ static inline int amd_pstate_init_perf(struct amd_cpudata *cpudata)
>  	return static_call(amd_pstate_init_perf)(cpudata);
>  }
>  
> -static int shmem_update_perf(struct amd_cpudata *cpudata, u32 min_perf,
> -			     u32 des_perf, u32 max_perf, u32 epp, bool fast_switch)
> +static int shmem_update_perf(struct amd_cpudata *cpudata, u8 min_perf,
> +			     u8 des_perf, u8 max_perf, u8 epp, bool fast_switch)
>  {
>  	struct cppc_perf_ctrls perf_ctrls;
>  
> @@ -531,14 +531,14 @@ static inline bool amd_pstate_sample(struct amd_cpudata *cpudata)
>  	return true;
>  }
>  
> -static void amd_pstate_update(struct amd_cpudata *cpudata, u32 min_perf,
> -			      u32 des_perf, u32 max_perf, bool fast_switch, int gov_flags)
> +static void amd_pstate_update(struct amd_cpudata *cpudata, u8 min_perf,
> +			      u8 des_perf, u8 max_perf, bool fast_switch, int gov_flags)
>  {
>  	unsigned long max_freq;
>  	struct cpufreq_policy *policy = cpufreq_cpu_get(cpudata->cpu);
> -	u32 nominal_perf = READ_ONCE(cpudata->nominal_perf);
> +	u8 nominal_perf = READ_ONCE(cpudata->nominal_perf);
>  
> -	des_perf = clamp_t(unsigned long, des_perf, min_perf, max_perf);
> +	des_perf = clamp_t(u8, des_perf, min_perf, max_perf);
>  
>  	max_freq = READ_ONCE(cpudata->max_limit_freq);
>  	policy->cur = div_u64(des_perf * max_freq, max_perf);
> @@ -550,7 +550,7 @@ static void amd_pstate_update(struct amd_cpudata *cpudata, u32 min_perf,
>  
>  	/* limit the max perf when core performance boost feature is disabled */
>  	if (!cpudata->boost_supported)
> -		max_perf = min_t(unsigned long, nominal_perf, max_perf);
> +		max_perf = min_t(u8, nominal_perf, max_perf);
>  
>  	if (trace_amd_pstate_perf_enabled() && amd_pstate_sample(cpudata)) {
>  		trace_amd_pstate_perf(min_perf, des_perf, max_perf, cpudata->freq,
> @@ -591,7 +591,8 @@ static int amd_pstate_verify(struct cpufreq_policy_data *policy_data)
>  
>  static int amd_pstate_update_min_max_limit(struct cpufreq_policy *policy)
>  {
> -	u32 max_limit_perf, min_limit_perf, max_perf, max_freq;
> +	u8 max_limit_perf, min_limit_perf, max_perf;
> +	u32 max_freq;
>  	struct amd_cpudata *cpudata = policy->driver_data;
>  
>  	max_perf = READ_ONCE(cpudata->highest_perf);
> @@ -615,7 +616,7 @@ static int amd_pstate_update_freq(struct cpufreq_policy *policy,
>  {
>  	struct cpufreq_freqs freqs;
>  	struct amd_cpudata *cpudata = policy->driver_data;
> -	unsigned long des_perf, cap_perf;
> +	u8 des_perf, cap_perf;
>  
>  	if (!cpudata->max_freq)
>  		return -ENODEV;
> @@ -670,8 +671,7 @@ static void amd_pstate_adjust_perf(unsigned int cpu,
>  				   unsigned long target_perf,
>  				   unsigned long capacity)
>  {
> -	unsigned long max_perf, min_perf, des_perf,
> -		      cap_perf, min_limit_perf;
> +	u8 max_perf, min_perf, des_perf, cap_perf, min_limit_perf;
>  	struct cpufreq_policy *policy = cpufreq_cpu_get(cpu);
>  	struct amd_cpudata *cpudata;
>  
> @@ -904,8 +904,8 @@ static int amd_pstate_init_freq(struct amd_cpudata *cpudata)
>  {
>  	int ret;
>  	u32 min_freq, max_freq;
> -	u32 highest_perf, nominal_perf, nominal_freq;
> -	u32 lowest_nonlinear_perf, lowest_nonlinear_freq;
> +	u8 highest_perf, nominal_perf, lowest_nonlinear_perf;
> +	u32 nominal_freq, lowest_nonlinear_freq;
>  	struct cppc_perf_caps cppc_perf;
>  
>  	ret = cppc_get_perf_caps(cpudata->cpu, &cppc_perf);
> @@ -1112,7 +1112,7 @@ static ssize_t show_amd_pstate_lowest_nonlinear_freq(struct cpufreq_policy *poli
>  static ssize_t show_amd_pstate_highest_perf(struct cpufreq_policy *policy,
>  					    char *buf)
>  {
> -	u32 perf;
> +	u8 perf;
>  	struct amd_cpudata *cpudata = policy->driver_data;
>  
>  	perf = READ_ONCE(cpudata->highest_perf);
> @@ -1123,7 +1123,7 @@ static ssize_t show_amd_pstate_highest_perf(struct cpufreq_policy *policy,
>  static ssize_t show_amd_pstate_prefcore_ranking(struct cpufreq_policy *policy,
>  						char *buf)
>  {
> -	u32 perf;
> +	u8 perf;
>  	struct amd_cpudata *cpudata = policy->driver_data;
>  
>  	perf = READ_ONCE(cpudata->prefcore_ranking);
> @@ -1186,7 +1186,7 @@ static ssize_t show_energy_performance_preference(
>  				struct cpufreq_policy *policy, char *buf)
>  {
>  	struct amd_cpudata *cpudata = policy->driver_data;
> -	int preference;
> +	u8 preference;
>  
>  	switch (cpudata->epp_cached) {
>  	case AMD_CPPC_EPP_PERFORMANCE:
> @@ -1548,7 +1548,7 @@ static void amd_pstate_epp_cpu_exit(struct cpufreq_policy *policy)
>  static int amd_pstate_epp_update_limit(struct cpufreq_policy *policy)
>  {
>  	struct amd_cpudata *cpudata = policy->driver_data;
> -	u32 epp;
> +	u8 epp;
>  
>  	amd_pstate_update_min_max_limit(policy);
>  
> @@ -1597,7 +1597,7 @@ static int amd_pstate_epp_set_policy(struct cpufreq_policy *policy)
>  static int amd_pstate_epp_reenable(struct cpufreq_policy *policy)
>  {
>  	struct amd_cpudata *cpudata = policy->driver_data;
> -	u64 max_perf;
> +	u8 max_perf;
>  	int ret;
>  
>  	ret = amd_pstate_cppc_enable(true);
> @@ -1634,7 +1634,7 @@ static int amd_pstate_epp_cpu_online(struct cpufreq_policy *policy)
>  static int amd_pstate_epp_cpu_offline(struct cpufreq_policy *policy)
>  {
>  	struct amd_cpudata *cpudata = policy->driver_data;
> -	int min_perf;
> +	u8 min_perf;
>  
>  	if (cpudata->suspended)
>  		return 0;
> diff --git a/drivers/cpufreq/amd-pstate.h b/drivers/cpufreq/amd-pstate.h
> index 9747e3be6cee..19d405c6d805 100644
> --- a/drivers/cpufreq/amd-pstate.h
> +++ b/drivers/cpufreq/amd-pstate.h
> @@ -70,13 +70,13 @@ struct amd_cpudata {
>  	struct	freq_qos_request req[2];
>  	u64	cppc_req_cached;
>  
> -	u32	highest_perf;
> -	u32	nominal_perf;
> -	u32	lowest_nonlinear_perf;
> -	u32	lowest_perf;
> -	u32     prefcore_ranking;
> -	u32     min_limit_perf;
> -	u32     max_limit_perf;
> +	u8	highest_perf;
> +	u8	nominal_perf;
> +	u8	lowest_nonlinear_perf;
> +	u8	lowest_perf;
> +	u8	prefcore_ranking;
> +	u8	min_limit_perf;
> +	u8	max_limit_perf;
>  	u32     min_limit_freq;
>  	u32     max_limit_freq;
>  
> @@ -93,11 +93,11 @@ struct amd_cpudata {
>  	bool	hw_prefcore;
>  
>  	/* EPP feature related attributes*/
> -	s16	epp_cached;
> +	u8	epp_cached;
>  	u32	policy;
>  	u64	cppc_cap1_cached;
>  	bool	suspended;
> -	s16	epp_default;
> +	u8	epp_default;
>  };
>  
>  /*
> -- 
> 2.34.1
> 

Re: [PATCH 07/12] cpufreq/amd-pstate: Modularize perf<->freq conversion

[Gautham R. Shenoy]

Hello Dhananjay,

On Wed, Feb 05, 2025 at 11:25:18AM +0000, Dhananjay Ugwekar wrote:
> Delegate the perf<->frequency conversion to helper functions to reduce
> code duplication, and improve readability.


This is better than using the max_freq/max_perf as the baseline since
these values can change when boost is enabled/disabled.

Reviewed-by: Gautham R. Shenoy <gautham.shenoy@amd.com>

-- 
Thanks and Regards
gautham.

> 
> Signed-off-by: Dhananjay Ugwekar <dhananjay.ugwekar@amd.com>
> ---
>  drivers/cpufreq/amd-pstate.c | 57 +++++++++++++++++++-----------------
>  1 file changed, 30 insertions(+), 27 deletions(-)
> 
> diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c
> index dd4f23fa2587..346fac646eba 100644
> --- a/drivers/cpufreq/amd-pstate.c
> +++ b/drivers/cpufreq/amd-pstate.c
> @@ -142,6 +142,20 @@ static struct quirk_entry quirk_amd_7k62 = {
>  	.lowest_freq = 550,
>  };
>  
> +static inline u8 freq_to_perf(struct amd_cpudata *cpudata, unsigned int freq_val)
> +{
> +	u8 perf_val = DIV_ROUND_UP_ULL((u64)freq_val * cpudata->nominal_perf,
> +					cpudata->nominal_freq);
> +
> +	return clamp_t(u8, perf_val, cpudata->lowest_perf, cpudata->highest_perf);
> +}
> +
> +static inline u32 perf_to_freq(struct amd_cpudata *cpudata, u8 perf_val)
> +{
> +	return DIV_ROUND_UP_ULL((u64)cpudata->nominal_freq * perf_val,
> +				cpudata->nominal_perf);
> +}
> +
>  static int __init dmi_matched_7k62_bios_bug(const struct dmi_system_id *dmi)
>  {
>  	/**
> @@ -534,14 +548,12 @@ static inline bool amd_pstate_sample(struct amd_cpudata *cpudata)
>  static void amd_pstate_update(struct amd_cpudata *cpudata, u8 min_perf,
>  			      u8 des_perf, u8 max_perf, bool fast_switch, int gov_flags)
>  {
> -	unsigned long max_freq;
>  	struct cpufreq_policy *policy = cpufreq_cpu_get(cpudata->cpu);
>  	u8 nominal_perf = READ_ONCE(cpudata->nominal_perf);
>  
>  	des_perf = clamp_t(u8, des_perf, min_perf, max_perf);
>  
> -	max_freq = READ_ONCE(cpudata->max_limit_freq);
> -	policy->cur = div_u64(des_perf * max_freq, max_perf);
> +	policy->cur = perf_to_freq(cpudata, des_perf);
>  
>  	if ((cppc_state == AMD_PSTATE_GUIDED) && (gov_flags & CPUFREQ_GOV_DYNAMIC_SWITCHING)) {
>  		min_perf = des_perf;
> @@ -591,14 +603,11 @@ static int amd_pstate_verify(struct cpufreq_policy_data *policy_data)
>  
>  static int amd_pstate_update_min_max_limit(struct cpufreq_policy *policy)
>  {
> -	u8 max_limit_perf, min_limit_perf, max_perf;
> -	u32 max_freq;
> +	u8 max_limit_perf, min_limit_perf;
>  	struct amd_cpudata *cpudata = policy->driver_data;
>  
> -	max_perf = READ_ONCE(cpudata->highest_perf);
> -	max_freq = READ_ONCE(cpudata->max_freq);
> -	max_limit_perf = div_u64(policy->max * max_perf, max_freq);
> -	min_limit_perf = div_u64(policy->min * max_perf, max_freq);
> +	max_limit_perf = freq_to_perf(cpudata, policy->max);
> +	min_limit_perf = freq_to_perf(cpudata, policy->min);
>  
>  	if (cpudata->policy == CPUFREQ_POLICY_PERFORMANCE)
>  		min_limit_perf = min(cpudata->nominal_perf, max_limit_perf);
> @@ -616,21 +625,15 @@ static int amd_pstate_update_freq(struct cpufreq_policy *policy,
>  {
>  	struct cpufreq_freqs freqs;
>  	struct amd_cpudata *cpudata = policy->driver_data;
> -	u8 des_perf, cap_perf;
> -
> -	if (!cpudata->max_freq)
> -		return -ENODEV;
> +	u8 des_perf;
>  
>  	if (policy->min != cpudata->min_limit_freq || policy->max != cpudata->max_limit_freq)
>  		amd_pstate_update_min_max_limit(policy);
>  
> -	cap_perf = READ_ONCE(cpudata->highest_perf);
> -
>  	freqs.old = policy->cur;
>  	freqs.new = target_freq;
>  
> -	des_perf = DIV_ROUND_CLOSEST(target_freq * cap_perf,
> -				     cpudata->max_freq);
> +	des_perf = freq_to_perf(cpudata, target_freq);
>  
>  	WARN_ON(fast_switch && !policy->fast_switch_enabled);
>  	/*
> @@ -904,7 +907,6 @@ static int amd_pstate_init_freq(struct amd_cpudata *cpudata)
>  {
>  	int ret;
>  	u32 min_freq, max_freq;
> -	u8 highest_perf, nominal_perf, lowest_nonlinear_perf;
>  	u32 nominal_freq, lowest_nonlinear_freq;
>  	struct cppc_perf_caps cppc_perf;
>  
> @@ -922,16 +924,17 @@ static int amd_pstate_init_freq(struct amd_cpudata *cpudata)
>  	else
>  		nominal_freq = cppc_perf.nominal_freq;
>  
> -	highest_perf = READ_ONCE(cpudata->highest_perf);
> -	nominal_perf = READ_ONCE(cpudata->nominal_perf);
> -	max_freq = div_u64((u64)highest_perf * nominal_freq, nominal_perf);
> +	min_freq *= 1000;
> +	nominal_freq *= 1000;
> +
> +	WRITE_ONCE(cpudata->nominal_freq, nominal_freq);
> +	WRITE_ONCE(cpudata->min_freq, min_freq);
> +
> +	max_freq = perf_to_freq(cpudata, cpudata->highest_perf);
> +	lowest_nonlinear_freq = perf_to_freq(cpudata, cpudata->lowest_nonlinear_perf);
>  
> -	lowest_nonlinear_perf = READ_ONCE(cpudata->lowest_nonlinear_perf);
> -	lowest_nonlinear_freq = div_u64((u64)nominal_freq * lowest_nonlinear_perf, nominal_perf);
> -	WRITE_ONCE(cpudata->min_freq, min_freq * 1000);
> -	WRITE_ONCE(cpudata->lowest_nonlinear_freq, lowest_nonlinear_freq * 1000);
> -	WRITE_ONCE(cpudata->nominal_freq, nominal_freq * 1000);
> -	WRITE_ONCE(cpudata->max_freq, max_freq * 1000);
> +	WRITE_ONCE(cpudata->lowest_nonlinear_freq, lowest_nonlinear_freq);
> +	WRITE_ONCE(cpudata->max_freq, max_freq);
>  
>  	/**
>  	 * Below values need to be initialized correctly, otherwise driver will fail to load
> -- 
> 2.34.1
> 

Re: [PATCH 08/12] cpufreq/amd-pstate: Remove the unnecessary cpufreq_update_policy call

[Gautham R. Shenoy]

On Wed, Feb 05, 2025 at 11:25:19AM +0000, Dhananjay Ugwekar wrote:
> The update_limits callback is only called in two conditions.
> 
> * When the preferred core rankings change. In which case, we just need to
> change the prefcore ranking in the cpudata struct. As there are no changes
> to any of the perf values, there is no need to call cpufreq_update_policy()
> 
> * When the _PPC ACPI object changes, i.e. the highest allowed Pstate
> changes. The _PPC object is only used for a table based cpufreq driver
> like acpi-cpufreq, hence is irrelevant for CPPC based amd-pstate.
> 
> Hence, the cpufreq_update_policy() call becomes unnecessary and can be
> removed.

Thanks for the cleanup.

Reviewed-by: Gautham R. Shenoy <gautham.shenoy@amd.com>

-- 
Thanks and Regards
gautham.
> 
> Signed-off-by: Dhananjay Ugwekar <dhananjay.ugwekar@amd.com>
> ---
>  drivers/cpufreq/amd-pstate.c | 4 ----
>  1 file changed, 4 deletions(-)
> 
> diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c
> index 346fac646eba..107ad953ce43 100644
> --- a/drivers/cpufreq/amd-pstate.c
> +++ b/drivers/cpufreq/amd-pstate.c
> @@ -852,10 +852,6 @@ static void amd_pstate_update_limits(unsigned int cpu)
>  			sched_set_itmt_core_prio((int)cur_high, cpu);
>  	}
>  	cpufreq_cpu_put(policy);
> -
> -	if (!highest_perf_changed)
> -		cpufreq_update_policy(cpu);
> -
>  }
>  
>  /*
> -- 
> 2.34.1
> 

Re: [PATCH 09/12] cpufreq/amd-pstate: Fix cpufreq_policy ref counting

[Gautham R. Shenoy]

On Wed, Feb 05, 2025 at 11:25:20AM +0000, Dhananjay Ugwekar wrote:
> amd_pstate_update_limits() takes a cpufreq_policy reference but doesn't
> decrement the refcount in one of the exit paths, fix that.
> 
> Fixes: 45722e777fd9 ("cpufreq: amd-pstate: Optimize amd_pstate_update_limits()")
> Signed-off-by: Dhananjay Ugwekar <dhananjay.ugwekar@amd.com>

Thanks for fixing this.

Reviewed-by: Gautham R. Shenoy <gautham.shenoy@amd.com>

-- 
Thanks and Regards
gautham.

> ---
>  drivers/cpufreq/amd-pstate.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c
> index 107ad953ce43..9c939be59042 100644
> --- a/drivers/cpufreq/amd-pstate.c
> +++ b/drivers/cpufreq/amd-pstate.c
> @@ -821,20 +821,21 @@ static void amd_pstate_init_prefcore(struct amd_cpudata *cpudata)
>  
>  static void amd_pstate_update_limits(unsigned int cpu)
>  {
> -	struct cpufreq_policy *policy = cpufreq_cpu_get(cpu);
> +	struct cpufreq_policy *policy = NULL;
>  	struct amd_cpudata *cpudata;
>  	u32 prev_high = 0, cur_high = 0;
>  	int ret;
>  	bool highest_perf_changed = false;
>  
> +	if (!amd_pstate_prefcore)
> +		return;
> +
> +	policy = cpufreq_cpu_get(cpu);
>  	if (!policy)
>  		return;
>  
>  	cpudata = policy->driver_data;
>  
> -	if (!amd_pstate_prefcore)
> -		return;
> -
>  	guard(mutex)(&amd_pstate_driver_lock);
>  
>  	ret = amd_get_highest_perf(cpu, &cur_high);
> -- 
> 2.34.1
> 

Re: [PATCH 10/12] cpufreq/amd-pstate: Add missing NULL ptr check in amd_pstate_update

[Gautham R. Shenoy]

On Wed, Feb 05, 2025 at 11:25:21AM +0000, Dhananjay Ugwekar wrote:
> Check if policy is NULL before dereferencing it in amd_pstate_update.

Reviewed-by: Gautham R. Shenoy <gautham.shenoy@amd.com>

-- 
Thanks and Regards
gautham.


> 
> Fixes: e8f555daacd3 ("cpufreq/amd-pstate: fix setting policy current frequency value")
> Signed-off-by: Dhananjay Ugwekar <dhananjay.ugwekar@amd.com>
> ---
>  drivers/cpufreq/amd-pstate.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c
> index 9c939be59042..6a604f0797d9 100644
> --- a/drivers/cpufreq/amd-pstate.c
> +++ b/drivers/cpufreq/amd-pstate.c
> @@ -551,6 +551,9 @@ static void amd_pstate_update(struct amd_cpudata *cpudata, u8 min_perf,
>  	struct cpufreq_policy *policy = cpufreq_cpu_get(cpudata->cpu);
>  	u8 nominal_perf = READ_ONCE(cpudata->nominal_perf);
>  
> +	if (!policy)
> +		return;
> +
>  	des_perf = clamp_t(u8, des_perf, min_perf, max_perf);
>  
>  	policy->cur = perf_to_freq(cpudata, des_perf);
> -- 
> 2.34.1
> 

Re: [PATCH 11/12] cpufreq/amd-pstate: Use scope based cleanup for cpufreq_policy refs

[Gautham R. Shenoy]

On Wed, Feb 05, 2025 at 11:25:22AM +0000, Dhananjay Ugwekar wrote:
> There have been instances in past where refcount decrementing is missed
> while exiting a function. Use automatic scope based cleanup to avoid
> such errors.

This is a nice fix! I wonder if this can be used in other parts
of cpufreq. Did you check if there are other cpufreq drivers/governors
where we can use this destructor?

Other than that,

Reviewed-by: Gautham R. Shenoy <gautham.shenoy@amd.com>

-- 
Thanks and Regards
gautham.


> 
> Signed-off-by: Dhananjay Ugwekar <dhananjay.ugwekar@amd.com>
> ---
>  drivers/cpufreq/amd-pstate.c | 25 ++++++++-----------------
>  include/linux/cpufreq.h      |  3 +++
>  2 files changed, 11 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c
> index 6a604f0797d9..ee7e3f0a4c0a 100644
> --- a/drivers/cpufreq/amd-pstate.c
> +++ b/drivers/cpufreq/amd-pstate.c
> @@ -548,7 +548,7 @@ static inline bool amd_pstate_sample(struct amd_cpudata *cpudata)
>  static void amd_pstate_update(struct amd_cpudata *cpudata, u8 min_perf,
>  			      u8 des_perf, u8 max_perf, bool fast_switch, int gov_flags)
>  {
> -	struct cpufreq_policy *policy = cpufreq_cpu_get(cpudata->cpu);
> +	struct cpufreq_policy *policy __free(put_cpufreq_policy) = cpufreq_cpu_get(cpudata->cpu);
>  	u8 nominal_perf = READ_ONCE(cpudata->nominal_perf);
>  
>  	if (!policy)
> @@ -574,8 +574,6 @@ static void amd_pstate_update(struct amd_cpudata *cpudata, u8 min_perf,
>  	}
>  
>  	amd_pstate_update_perf(cpudata, min_perf, des_perf, max_perf, 0, fast_switch);
> -
> -	cpufreq_cpu_put(policy);
>  }
>  
>  static int amd_pstate_verify(struct cpufreq_policy_data *policy_data)
> @@ -587,7 +585,8 @@ static int amd_pstate_verify(struct cpufreq_policy_data *policy_data)
>  	 * amd-pstate qos_requests.
>  	 */
>  	if (policy_data->min == FREQ_QOS_MIN_DEFAULT_VALUE) {
> -		struct cpufreq_policy *policy = cpufreq_cpu_get(policy_data->cpu);
> +		struct cpufreq_policy *policy __free(put_cpufreq_policy) =
> +					      cpufreq_cpu_get(policy_data->cpu);
>  		struct amd_cpudata *cpudata;
>  
>  		if (!policy)
> @@ -595,7 +594,6 @@ static int amd_pstate_verify(struct cpufreq_policy_data *policy_data)
>  
>  		cpudata = policy->driver_data;
>  		policy_data->min = cpudata->lowest_nonlinear_freq;
> -		cpufreq_cpu_put(policy);
>  	}
>  
>  	cpufreq_verify_within_cpu_limits(policy_data);
> @@ -678,7 +676,7 @@ static void amd_pstate_adjust_perf(unsigned int cpu,
>  				   unsigned long capacity)
>  {
>  	u8 max_perf, min_perf, des_perf, cap_perf, min_limit_perf;
> -	struct cpufreq_policy *policy = cpufreq_cpu_get(cpu);
> +	struct cpufreq_policy *policy __free(put_cpufreq_policy) = cpufreq_cpu_get(cpu);
>  	struct amd_cpudata *cpudata;
>  
>  	if (!policy)
> @@ -710,7 +708,6 @@ static void amd_pstate_adjust_perf(unsigned int cpu,
>  
>  	amd_pstate_update(cpudata, min_perf, des_perf, max_perf, true,
>  			policy->governor->flags);
> -	cpufreq_cpu_put(policy);
>  }
>  
>  static int amd_pstate_cpu_boost_update(struct cpufreq_policy *policy, bool on)
> @@ -824,28 +821,23 @@ static void amd_pstate_init_prefcore(struct amd_cpudata *cpudata)
>  
>  static void amd_pstate_update_limits(unsigned int cpu)
>  {
> -	struct cpufreq_policy *policy = NULL;
> +	struct cpufreq_policy *policy __free(put_cpufreq_policy) = cpufreq_cpu_get(cpu);
>  	struct amd_cpudata *cpudata;
>  	u32 prev_high = 0, cur_high = 0;
> -	int ret;
>  	bool highest_perf_changed = false;
>  
>  	if (!amd_pstate_prefcore)
>  		return;
>  
> -	policy = cpufreq_cpu_get(cpu);
>  	if (!policy)
>  		return;
>  
> -	cpudata = policy->driver_data;
> -
>  	guard(mutex)(&amd_pstate_driver_lock);
>  
> -	ret = amd_get_highest_perf(cpu, &cur_high);
> -	if (ret) {
> -		cpufreq_cpu_put(policy);
> +	if (amd_get_highest_perf(cpu, &cur_high))
>  		return;
> -	}
> +
> +	cpudata = policy->driver_data;
>  
>  	prev_high = READ_ONCE(cpudata->prefcore_ranking);
>  	highest_perf_changed = (prev_high != cur_high);
> @@ -855,7 +847,6 @@ static void amd_pstate_update_limits(unsigned int cpu)
>  		if (cur_high < CPPC_MAX_PERF)
>  			sched_set_itmt_core_prio((int)cur_high, cpu);
>  	}
> -	cpufreq_cpu_put(policy);
>  }
>  
>  /*
> diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h
> index 7fe0981a7e46..dde5212d256c 100644
> --- a/include/linux/cpufreq.h
> +++ b/include/linux/cpufreq.h
> @@ -210,6 +210,9 @@ static inline struct cpufreq_policy *cpufreq_cpu_get(unsigned int cpu)
>  static inline void cpufreq_cpu_put(struct cpufreq_policy *policy) { }
>  #endif
>  
> +/* Scope based cleanup macro for cpufreq_policy kobject reference counting */
> +DEFINE_FREE(put_cpufreq_policy, struct cpufreq_policy *, if (_T) cpufreq_cpu_put(_T))
> +
>  static inline bool policy_is_inactive(struct cpufreq_policy *policy)
>  {
>  	return cpumask_empty(policy->cpus);
> -- 
> 2.34.1
> 

Re: [PATCH 12/12] cpufreq/amd-pstate: Remove the unncecessary driver_lock in amd_pstate_update_limits

[Gautham R. Shenoy]

On Wed, Feb 05, 2025 at 11:25:23AM +0000, Dhananjay Ugwekar wrote:
> There is no need to take a driver wide lock while updating the
> highest_perf value in the percpu cpudata struct. Hence remove it.

Yup!

Reviewed-by: Gautham R. Shenoy <gautham.shenoy@amd.com>

-- 
Thanks and Regards
gautham.
> 
> Signed-off-by: Dhananjay Ugwekar <dhananjay.ugwekar@amd.com>
> ---
>  drivers/cpufreq/amd-pstate.c | 2 --
>  1 file changed, 2 deletions(-)
> 
> diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c
> index ee7e3f0a4c0a..08ae48076812 100644
> --- a/drivers/cpufreq/amd-pstate.c
> +++ b/drivers/cpufreq/amd-pstate.c
> @@ -832,8 +832,6 @@ static void amd_pstate_update_limits(unsigned int cpu)
>  	if (!policy)
>  		return;
>  
> -	guard(mutex)(&amd_pstate_driver_lock);
> -
>  	if (amd_get_highest_perf(cpu, &cur_high))
>  		return;
>  
> -- 
> 2.34.1
>