| Message ID | 20250219210302.442954-20-superm1@kernel.org |
|---|---|
| State | Superseded |
| Headers | show |
| Series | amd-pstate cleanups | expand |
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)
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.
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)