[v4] amd-pstate cleanups

[PATCH v4 19/19] cpufreq/amd-pstate: Drop amd_pstate_epp_cpu_offline()

Posted by Mario Limonciello 10 months ago

From: Mario Limonciello <mario.limonciello@amd.com>

When the CPU goes offline there is no need to change the CPPC request
because the CPU will go into the deepest C-state it supports already.

Actually changing the CPPC request when it goes offline messes up the
cached values and can lead to the wrong values being restored when
it comes back.

Instead if the CPU comes back online let amd_pstate_epp_set_policy()
restore it to expected values.

Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
---
 drivers/cpufreq/amd-pstate.c | 9 +--------
 1 file changed, 1 insertion(+), 8 deletions(-)

diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c
index 408e63aff377a..5068778c1542a 100644
--- a/drivers/cpufreq/amd-pstate.c
+++ b/drivers/cpufreq/amd-pstate.c
@@ -1610,14 +1610,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;
-	union perf_cached perf = READ_ONCE(cpudata->perf);
-
-	if (cpudata->suspended)
-		return 0;
-
-	return amd_pstate_update_perf(policy, perf.lowest_perf, 0, perf.lowest_perf,
-				      AMD_CPPC_EPP_BALANCE_POWERSAVE, false);
+	return 0;
 }
 
 static int amd_pstate_epp_suspend(struct cpufreq_policy *policy)
-- 
2.43.0

Re: [PATCH v4 19/19] cpufreq/amd-pstate: Drop amd_pstate_epp_cpu_offline()

Posted by Dhananjay Ugwekar 9 months, 3 weeks ago

On 2/20/2025 2:33 AM, Mario Limonciello wrote:
> From: Mario Limonciello <mario.limonciello@amd.com>
> 
> When the CPU goes offline there is no need to change the CPPC request
> because the CPU will go into the deepest C-state it supports already.> 
> Actually changing the CPPC request when it goes offline messes up the
> cached values and can lead to the wrong values being restored when
> it comes back.
> 
> Instead if the CPU comes back online let amd_pstate_epp_set_policy()
> restore it to expected values.

Small suggestion below, apart from that LGTM

Reviewed-by: Dhananjay Ugwekar <dhananjay.ugwekar@amd.com>

> 
> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
> ---
>  drivers/cpufreq/amd-pstate.c | 9 +--------
>  1 file changed, 1 insertion(+), 8 deletions(-)
> 
> diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c
> index 408e63aff377a..5068778c1542a 100644
> --- a/drivers/cpufreq/amd-pstate.c
> +++ b/drivers/cpufreq/amd-pstate.c
> @@ -1610,14 +1610,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;
> -	union perf_cached perf = READ_ONCE(cpudata->perf);
> -
> -	if (cpudata->suspended)
> -		return 0;
> -
> -	return amd_pstate_update_perf(policy, perf.lowest_perf, 0, perf.lowest_perf,
> -				      AMD_CPPC_EPP_BALANCE_POWERSAVE, false);
> +	return 0;

Instead of making it an empty "return 0" function, can we remove this 
callback altogether? Didnt check if there are any constraints against 
removing it.

>  }
>  
>  static int amd_pstate_epp_suspend(struct cpufreq_policy *policy)

Re: [PATCH v4 19/19] cpufreq/amd-pstate: Drop amd_pstate_epp_cpu_offline()

Posted by Mario Limonciello 9 months, 3 weeks ago

On 2/24/2025 03:25, Dhananjay Ugwekar wrote:
> On 2/20/2025 2:33 AM, Mario Limonciello wrote:
>> From: Mario Limonciello <mario.limonciello@amd.com>
>>
>> When the CPU goes offline there is no need to change the CPPC request
>> because the CPU will go into the deepest C-state it supports already.>
>> Actually changing the CPPC request when it goes offline messes up the
>> cached values and can lead to the wrong values being restored when
>> it comes back.
>>
>> Instead if the CPU comes back online let amd_pstate_epp_set_policy()
>> restore it to expected values.
> 
> Small suggestion below, apart from that LGTM
> 
> Reviewed-by: Dhananjay Ugwekar <dhananjay.ugwekar@amd.com>

Thanks!

> 
>>
>> Signed-off-by: Mario Limonciello <mario.limonciello@amd.com>
>> ---
>>   drivers/cpufreq/amd-pstate.c | 9 +--------
>>   1 file changed, 1 insertion(+), 8 deletions(-)
>>
>> diff --git a/drivers/cpufreq/amd-pstate.c b/drivers/cpufreq/amd-pstate.c
>> index 408e63aff377a..5068778c1542a 100644
>> --- a/drivers/cpufreq/amd-pstate.c
>> +++ b/drivers/cpufreq/amd-pstate.c
>> @@ -1610,14 +1610,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;
>> -	union perf_cached perf = READ_ONCE(cpudata->perf);
>> -
>> -	if (cpudata->suspended)
>> -		return 0;
>> -
>> -	return amd_pstate_update_perf(policy, perf.lowest_perf, 0, perf.lowest_perf,
>> -				      AMD_CPPC_EPP_BALANCE_POWERSAVE, false);
>> +	return 0;
> 
> Instead of making it an empty "return 0" function, can we remove this
> callback altogether? Didnt check if there are any constraints against
> removing it.
> 
I originally had tried removing it, but the driver won't be able to 
setup properly unless the callback is setup.