Re: [PATCH V4 2/4] cpufreq: intel_pstate: Migrate to ->offline() instead of ->stop_cpu()

On Thu, Jun 24, 2021 at 3:52 AM Viresh Kumar [off-list ref] wrote:

On 23-06-21, 17:13, Rafael J. Wysocki wrote:

quoted

As mentioned already in

https://lore.kernel.org/linux-pm/CAJZ5v0g2tCZptcqh+c55YYiO7rDHmZivMLsmpq_7005zNPN1xg@mail.gmail.com/ (local)

Sorry about failing to reply over that, I got confused somehow..

quoted

this isn't particularly clean, because intel_pstate_cpu_offline() is
also used in the passive mode where the above call is not needed.

intel_pstate_clear_update_util_hook() returns early if the hook was never
registered, and so calling it was safe, but yes not very clean.

quoted

From: Rafael J. Wysocki <redacted>
Subject: [PATCH] cpufreq: intel_pstate: Combine ->stop_cpu() and ->offline()

Combine the ->stop_cpu() and ->offline() callback routines for the
active mode of intel_pstate so as to avoid setting the ->stop_cpu
callback pointer which is going to be dropped from the framework.

Signed-off-by: Rafael J. Wysocki <redacted>
---
 drivers/cpufreq/intel_pstate.c |    7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

Index: linux-pm/drivers/cpufreq/intel_pstate.c
===================================================================

--- linux-pm.orig/drivers/cpufreq/intel_pstate.c
+++ linux-pm/drivers/cpufreq/intel_pstate.c

@@ -2577,11 +2577,13 @@ static int intel_pstate_cpu_online(struc
      return 0;
 }

-static void intel_pstate_stop_cpu(struct cpufreq_policy *policy)
+static int intel_pstate_stop_cpu(struct cpufreq_policy *policy)
 {
      pr_debug("CPU %d stopping\n", policy->cpu);

      intel_pstate_clear_update_util_hook(policy->cpu);
+
+     return intel_pstate_cpu_offline(policy);
 }

 static int intel_pstate_cpu_exit(struct cpufreq_policy *policy)

@@ -2654,8 +2656,7 @@ static struct cpufreq_driver intel_pstat
      .resume         = intel_pstate_resume,
      .init           = intel_pstate_cpu_init,
      .exit           = intel_pstate_cpu_exit,
-     .stop_cpu       = intel_pstate_stop_cpu,
-     .offline        = intel_pstate_cpu_offline,
+     .offline        = intel_pstate_stop_cpu,

I would suggest to rename intel_pstate_cpu_offline() as
intel_cpufreq_cpu_offline() and intel_pstate_stop_cpu() as
intel_pstate_cpu_offline(), so we remove the stop-cpu terminology completely.

I have followed the above suggestion and applied the modified patch
along with the rest of this series.

Either way:
Acked-by: Viresh Kumar <viresh.kumar@linaro.org>

Thanks!

quoted

      .online         = intel_pstate_cpu_online,
      .update_limits  = intel_pstate_update_limits,
      .name           = "intel_pstate",