LinuxLists.cc - [PATCH V4 0/4] cpufreq: Migrate away from ->stop

2021-06-23 04:25:38

Subject: [PATCH V4 0/4] cpufreq: Migrate away from ->stop_cpu() callback

Hi Rafael,

These are based on your patch [1] now.

commit 367dc4aa932b ("cpufreq: Add stop CPU callback to cpufreq_driver
interface") added the stop_cpu() callback to allow the drivers to do
clean up before the CPU is completely down and its state can't be
modified.

At that time the CPU hotplug framework used to call the cpufreq core's
registered notifier for different events like CPU_DOWN_PREPARE and
CPU_POST_DEAD. The stop_cpu() callback was called during the
CPU_DOWN_PREPARE event.

This is no longer the case, cpuhp_cpufreq_offline() is called only once
by the CPU hotplug core now and we don't really need two separate
callbacks for cpufreq drivers, i.e. stop_cpu() and exit(), as everything
can be done from the exit() callback itself.

Migrate to using the offline() or exit() callback instead of stop_cpu().

V3->V4:
- Based on a cleanup patch [1] from Rafael, apart from 5.13-rc7.
- No need to update exit() for intel pstate anymore.
- Remove the stop_cpu() callback completely.

--
Viresh

[1] https://lore.kernel.org/linux-pm/5490292.DvuYhMxLoT@kreacher/

Viresh Kumar (4):
cpufreq: cppc: Migrate to ->exit() callback instead of ->stop_cpu()
cpufreq: intel_pstate: Migrate to ->offline() instead of ->stop_cpu()
cpufreq: powerenv: Migrate to ->exit() callback instead of
->stop_cpu()
cpufreq: Remove stop_cpu() callback

Documentation/cpu-freq/cpu-drivers.rst | 3 --
.../zh_CN/cpu-freq/cpu-drivers.rst | 3 --
drivers/cpufreq/cppc_cpufreq.c | 46 ++++++++++---------
drivers/cpufreq/cpufreq.c | 3 --
drivers/cpufreq/intel_pstate.c | 10 +---
drivers/cpufreq/powernv-cpufreq.c | 23 ++++------
include/linux/cpufreq.h | 1 -
7 files changed, 35 insertions(+), 54 deletions(-)

--
2.31.1.272.g89b43f80a514

2021-06-23 04:25:45

by Viresh Kumar

[permalink] [raw]

Subject: [PATCH V4 1/4] cpufreq: cppc: Migrate to ->exit() callback instead of ->stop_cpu()

commit 367dc4aa932b ("cpufreq: Add stop CPU callback to cpufreq_driver
interface") added the stop_cpu() callback to allow the drivers to do
clean up before the CPU is completely down and its state can't be
modified.

At that time the CPU hotplug framework used to call the cpufreq core's
registered notifier for different events like CPU_DOWN_PREPARE and
CPU_POST_DEAD. The stop_cpu() callback was called during the
CPU_DOWN_PREPARE event.

This is no longer the case, cpuhp_cpufreq_offline() is called only once
by the CPU hotplug core now and we don't really need two separate
callbacks for cpufreq drivers, i.e. stop_cpu() and exit(), as everything
can be done from the exit() callback itself.

Migrate to using the exit() callback instead of stop_cpu().

Signed-off-by: Viresh Kumar <[email protected]>
---
drivers/cpufreq/cppc_cpufreq.c | 46 ++++++++++++++++++----------------
1 file changed, 24 insertions(+), 22 deletions(-)

diff --git a/drivers/cpufreq/cppc_cpufreq.c b/drivers/cpufreq/cppc_cpufreq.c
index 2f769b1630c5..be4f62e2c5f1 100644
--- a/drivers/cpufreq/cppc_cpufreq.c
+++ b/drivers/cpufreq/cppc_cpufreq.c
@@ -182,27 +182,6 @@ static int cppc_verify_policy(struct cpufreq_policy_data *policy)
return 0;
}

-static void cppc_cpufreq_stop_cpu(struct cpufreq_policy *policy)
-{
- struct cppc_cpudata *cpu_data = policy->driver_data;
- struct cppc_perf_caps *caps = &cpu_data->perf_caps;
- unsigned int cpu = policy->cpu;
- int ret;
-
- cpu_data->perf_ctrls.desired_perf = caps->lowest_perf;
-
- ret = cppc_set_perf(cpu, &cpu_data->perf_ctrls);
- if (ret)
- pr_debug("Err setting perf value:%d on CPU:%d. ret:%d\n",
- caps->lowest_perf, cpu, ret);
-
- /* Remove CPU node from list and free driver data for policy */
- free_cpumask_var(cpu_data->shared_cpu_map);
- list_del(&cpu_data->node);
- kfree(policy->driver_data);
- policy->driver_data = NULL;
-}
-
/*
* The PCC subspace describes the rate at which platform can accept commands
* on the shared PCC channel (including READs which do not count towards freq
@@ -352,6 +331,29 @@ static int cppc_cpufreq_cpu_init(struct cpufreq_policy *policy)
return ret;
}

+static int cppc_cpufreq_cpu_exit(struct cpufreq_policy *policy)
+{
+ struct cppc_cpudata *cpu_data = policy->driver_data;
+ struct cppc_perf_caps *caps = &cpu_data->perf_caps;
+ unsigned int cpu = policy->cpu;
+ int ret;
+
+ cpu_data->perf_ctrls.desired_perf = caps->lowest_perf;
+
+ ret = cppc_set_perf(cpu, &cpu_data->perf_ctrls);
+ if (ret)
+ pr_debug("Err setting perf value:%d on CPU:%d. ret:%d\n",
+ caps->lowest_perf, cpu, ret);
+
+ /* Remove CPU node from list and free driver data for policy */
+ free_cpumask_var(cpu_data->shared_cpu_map);
+ list_del(&cpu_data->node);
+ kfree(policy->driver_data);
+ policy->driver_data = NULL;
+
+ return 0;
+}
+
static inline u64 get_delta(u64 t1, u64 t0)
{
if (t1 > t0 || t0 > ~(u32)0)
@@ -451,7 +453,7 @@ static struct cpufreq_driver cppc_cpufreq_driver = {
.target = cppc_cpufreq_set_target,
.get = cppc_cpufreq_get_rate,
.init = cppc_cpufreq_cpu_init,
- .stop_cpu = cppc_cpufreq_stop_cpu,
+ .exit = cppc_cpufreq_cpu_exit,
.set_boost = cppc_cpufreq_set_boost,
.attr = cppc_cpufreq_attr,
.name = "cppc_cpufreq",
--
2.31.1.272.g89b43f80a514

2021-06-23 04:25:47

by Viresh Kumar

[permalink] [raw]

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

2021-06-23 04:26:04

by Viresh Kumar

[permalink] [raw]

Subject: [PATCH V4 3/4] cpufreq: powerenv: Migrate to ->exit() callback instead of ->stop_cpu()

commit 367dc4aa932b ("cpufreq: Add stop CPU callback to cpufreq_driver
interface") added the stop_cpu() callback to allow the drivers to do
clean up before the CPU is completely down and its state can't be
modified.

At that time the CPU hotplug framework used to call the cpufreq core's
registered notifier for different events like CPU_DOWN_PREPARE and
CPU_POST_DEAD. The stop_cpu() callback was called during the
CPU_DOWN_PREPARE event.

This is no longer the case, cpuhp_cpufreq_offline() is called only once
by the CPU hotplug core now and we don't really need two separate
callbacks for cpufreq drivers, i.e. stop_cpu() and exit(), as everything
can be done from the exit() callback itself.

Migrate to using the exit() callback instead of stop_cpu().

Signed-off-by: Viresh Kumar <[email protected]>
---
drivers/cpufreq/powernv-cpufreq.c | 23 +++++++++--------------
1 file changed, 9 insertions(+), 14 deletions(-)

diff --git a/drivers/cpufreq/powernv-cpufreq.c b/drivers/cpufreq/powernv-cpufreq.c
index e439b43c19eb..005600cef273 100644
--- a/drivers/cpufreq/powernv-cpufreq.c
+++ b/drivers/cpufreq/powernv-cpufreq.c
@@ -875,7 +875,15 @@ static int powernv_cpufreq_cpu_init(struct cpufreq_policy *policy)

static int powernv_cpufreq_cpu_exit(struct cpufreq_policy *policy)
{
- /* timer is deleted in cpufreq_cpu_stop() */
+ struct powernv_smp_call_data freq_data;
+ struct global_pstate_info *gpstates = policy->driver_data;
+
+ freq_data.pstate_id = idx_to_pstate(powernv_pstate_info.min);
+ freq_data.gpstate_id = idx_to_pstate(powernv_pstate_info.min);
+ smp_call_function_single(policy->cpu, set_pstate, &freq_data, 1);
+ if (gpstates)
+ del_timer_sync(&gpstates->timer);
+
kfree(policy->driver_data);

return 0;
@@ -1007,18 +1015,6 @@ static struct notifier_block powernv_cpufreq_opal_nb = {
.priority = 0,
};

-static void powernv_cpufreq_stop_cpu(struct cpufreq_policy *policy)
-{
- struct powernv_smp_call_data freq_data;
- struct global_pstate_info *gpstates = policy->driver_data;
-
- freq_data.pstate_id = idx_to_pstate(powernv_pstate_info.min);
- freq_data.gpstate_id = idx_to_pstate(powernv_pstate_info.min);
- smp_call_function_single(policy->cpu, set_pstate, &freq_data, 1);
- if (gpstates)
- del_timer_sync(&gpstates->timer);
-}
-
static unsigned int powernv_fast_switch(struct cpufreq_policy *policy,
unsigned int target_freq)
{
@@ -1042,7 +1038,6 @@ static struct cpufreq_driver powernv_cpufreq_driver = {
.target_index = powernv_cpufreq_target_index,
.fast_switch = powernv_fast_switch,
.get = powernv_cpufreq_get,
- .stop_cpu = powernv_cpufreq_stop_cpu,
.attr = powernv_cpu_freq_attr,
};

--
2.31.1.272.g89b43f80a514

2021-06-23 04:27:16

by Viresh Kumar

[permalink] [raw]

Subject: [PATCH V4 4/4] cpufreq: Remove stop_cpu() callback

Now that all users of stop_cpu() are migrated to use other callbacks,
lets remove its support from the core.

Signed-off-by: Viresh Kumar <[email protected]>
---
Documentation/cpu-freq/cpu-drivers.rst | 3 ---
Documentation/translations/zh_CN/cpu-freq/cpu-drivers.rst | 3 ---
drivers/cpufreq/cpufreq.c | 3 ---
include/linux/cpufreq.h | 1 -
4 files changed, 10 deletions(-)

diff --git a/Documentation/cpu-freq/cpu-drivers.rst b/Documentation/cpu-freq/cpu-drivers.rst
index a697278ce190..74fac797c396 100644
--- a/Documentation/cpu-freq/cpu-drivers.rst
+++ b/Documentation/cpu-freq/cpu-drivers.rst
@@ -71,9 +71,6 @@ And optionally
.exit - A pointer to a per-policy cleanup function called during
CPU_POST_DEAD phase of cpu hotplug process.

- .stop_cpu - A pointer to a per-policy stop function called during
- CPU_DOWN_PREPARE phase of cpu hotplug process.
-
.suspend - A pointer to a per-policy suspend function which is called
with interrupts disabled and _after_ the governor is stopped for the
policy.
diff --git a/Documentation/translations/zh_CN/cpu-freq/cpu-drivers.rst b/Documentation/translations/zh_CN/cpu-freq/cpu-drivers.rst
index 0ca2cb646666..9570e9c9e939 100644
--- a/Documentation/translations/zh_CN/cpu-freq/cpu-drivers.rst
+++ b/Documentation/translations/zh_CN/cpu-freq/cpu-drivers.rst
@@ -76,9 +76,6 @@ 并且可选择
.exit - 一个指向per-policy清理函数的指针，该函数在cpu热插拔过程的CPU_POST_DEAD
阶段被调用。

- .stop_cpu - 一个指向per-policy停止函数的指针，该函数在cpu热插拔过程的CPU_DOWN_PREPARE
- 阶段被调用。
-
.suspend - 一个指向per-policy暂停函数的指针，该函数在关中断且在该策略的调节器停止
后被调用。

diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index cbab834c37a0..5e4b5316d254 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -1606,9 +1606,6 @@ static int cpufreq_offline(unsigned int cpu)
policy->cdev = NULL;
}

- if (cpufreq_driver->stop_cpu)
- cpufreq_driver->stop_cpu(policy);
-
if (has_target())
cpufreq_exit_governor(policy);

diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h
index 353969c7acd3..2e2267a36502 100644
--- a/include/linux/cpufreq.h
+++ b/include/linux/cpufreq.h
@@ -371,7 +371,6 @@ struct cpufreq_driver {
int (*online)(struct cpufreq_policy *policy);
int (*offline)(struct cpufreq_policy *policy);
int (*exit)(struct cpufreq_policy *policy);
- void (*stop_cpu)(struct cpufreq_policy *policy);
int (*suspend)(struct cpufreq_policy *policy);
int (*resume)(struct cpufreq_policy *policy);

--
2.31.1.272.g89b43f80a514

2021-06-23 05:46:55

by Michael Ellerman

[permalink] [raw]

Subject: Re: [PATCH V4 3/4] cpufreq: powerenv: Migrate to ->exit() callback instead of ->stop_cpu()

Viresh Kumar <[email protected]> writes:
>
> Subject: Re: [PATCH V4 3/4] cpufreq: powerenv: Migrate to ->exit() callback instead of ->stop_cpu()

Typo in subject should be "powernv".

cheers

2021-06-23 06:02:45

by Viresh Kumar

[permalink] [raw]

Subject: [PATCH V4.1 3/4] cpufreq: powernv: Migrate to ->exit() callback instead of ->stop_cpu()

commit 367dc4aa932b ("cpufreq: Add stop CPU callback to cpufreq_driver
interface") added the stop_cpu() callback to allow the drivers to do
clean up before the CPU is completely down and its state can't be
modified.

At that time the CPU hotplug framework used to call the cpufreq core's
registered notifier for different events like CPU_DOWN_PREPARE and
CPU_POST_DEAD. The stop_cpu() callback was called during the
CPU_DOWN_PREPARE event.

This is no longer the case, cpuhp_cpufreq_offline() is called only once
by the CPU hotplug core now and we don't really need two separate
callbacks for cpufreq drivers, i.e. stop_cpu() and exit(), as everything
can be done from the exit() callback itself.

Migrate to using the exit() callback instead of stop_cpu().

Signed-off-by: Viresh Kumar <[email protected]>
---
V4->V4.1:
- s/powerenv/powernv/

drivers/cpufreq/powernv-cpufreq.c | 23 +++++++++--------------
1 file changed, 9 insertions(+), 14 deletions(-)

diff --git a/drivers/cpufreq/powernv-cpufreq.c b/drivers/cpufreq/powernv-cpufreq.c
index e439b43c19eb..005600cef273 100644
--- a/drivers/cpufreq/powernv-cpufreq.c
+++ b/drivers/cpufreq/powernv-cpufreq.c
@@ -875,7 +875,15 @@ static int powernv_cpufreq_cpu_init(struct cpufreq_policy *policy)

static int powernv_cpufreq_cpu_exit(struct cpufreq_policy *policy)
{
- /* timer is deleted in cpufreq_cpu_stop() */
+ struct powernv_smp_call_data freq_data;
+ struct global_pstate_info *gpstates = policy->driver_data;
+
+ freq_data.pstate_id = idx_to_pstate(powernv_pstate_info.min);
+ freq_data.gpstate_id = idx_to_pstate(powernv_pstate_info.min);
+ smp_call_function_single(policy->cpu, set_pstate, &freq_data, 1);
+ if (gpstates)
+ del_timer_sync(&gpstates->timer);
+
kfree(policy->driver_data);

return 0;
@@ -1007,18 +1015,6 @@ static struct notifier_block powernv_cpufreq_opal_nb = {
.priority = 0,
};

-static void powernv_cpufreq_stop_cpu(struct cpufreq_policy *policy)
-{
- struct powernv_smp_call_data freq_data;
- struct global_pstate_info *gpstates = policy->driver_data;
-
- freq_data.pstate_id = idx_to_pstate(powernv_pstate_info.min);
- freq_data.gpstate_id = idx_to_pstate(powernv_pstate_info.min);
- smp_call_function_single(policy->cpu, set_pstate, &freq_data, 1);
- if (gpstates)
- del_timer_sync(&gpstates->timer);
-}
-
static unsigned int powernv_fast_switch(struct cpufreq_policy *policy,
unsigned int target_freq)
{
@@ -1042,7 +1038,6 @@ static struct cpufreq_driver powernv_cpufreq_driver = {
.target_index = powernv_cpufreq_target_index,
.fast_switch = powernv_fast_switch,
.get = powernv_cpufreq_get,
- .stop_cpu = powernv_cpufreq_stop_cpu,
.attr = powernv_cpu_freq_attr,
};

--
2.31.1.272.g89b43f80a514

2021-06-23 06:02:57

by Viresh Kumar

[permalink] [raw]

Subject: Re: [PATCH V4 3/4] cpufreq: powerenv: Migrate to ->exit() callback instead of ->stop_cpu()

On 23-06-21, 15:45, Michael Ellerman wrote:
> Viresh Kumar <[email protected]> writes:
> >
> > Subject: Re: [PATCH V4 3/4] cpufreq: powerenv: Migrate to ->exit() callback instead of ->stop_cpu()
>
> Typo in subject should be "powernv".

Thanks for noticing it :)

--
viresh

2021-06-23 15:15:22

by Rafael J. Wysocki

[permalink] [raw]

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

On Wednesday, June 23, 2021 6:24:40 AM CEST Viresh Kumar wrote:
> commit 367dc4aa932b ("cpufreq: Add stop CPU callback to cpufreq_driver
> interface") added the stop_cpu() callback to allow the drivers to do
> clean up before the CPU is completely down and its state can't be
> modified.
>
> At that time the CPU hotplug framework used to call the cpufreq core's
> registered notifier for different events like CPU_DOWN_PREPARE and
> CPU_POST_DEAD. The stop_cpu() callback was called during the
> CPU_DOWN_PREPARE event.
>
> This is no longer the case, cpuhp_cpufreq_offline() is called only once
> by the CPU hotplug core now and we don't really need to separately
> call stop_cpu() for cpufreq drivers.
>
> Migrate to using the offline() callbacks instead of stop_cpu().
>
> Cc: Dirk Brandewie <[email protected]>
> Signed-off-by: Viresh Kumar <[email protected]>
> ---
> drivers/cpufreq/intel_pstate.c | 10 ++--------
> 1 file changed, 2 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/cpufreq/intel_pstate.c b/drivers/cpufreq/intel_pstate.c
> index 0e69dffd5a76..b4c0ff7f5b71 100644
> --- a/drivers/cpufreq/intel_pstate.c
> +++ b/drivers/cpufreq/intel_pstate.c
> @@ -2335,6 +2335,8 @@ static int intel_pstate_cpu_offline(struct cpufreq_policy *policy)
>
> pr_debug("CPU %d going offline\n", cpu->cpu);
>
> + intel_pstate_clear_update_util_hook(policy->cpu);

As mentioned already in

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

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

I would make a change like in the patch below instead.

> +
> if (cpu->suspended)
> return 0;

---
From: Rafael J. Wysocki <[email protected]>
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 <[email protected]>
---
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,
.online = intel_pstate_cpu_online,
.update_limits = intel_pstate_update_limits,
.name = "intel_pstate",

2021-06-24 01:58:55

by Viresh Kumar

[permalink] [raw]

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

On 23-06-21, 17:13, Rafael J. Wysocki wrote:
> As mentioned already in
>
> https://lore.kernel.org/linux-pm/CAJZ5v0g2tCZptcqh+c55YYiO7rDHmZivMLsmpq_7005zNPN1xg@mail.gmail.com/

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

> 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.

> From: Rafael J. Wysocki <[email protected]>
> 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 <[email protected]>
> ---
> 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.

Either way:
Acked-by: Viresh Kumar <[email protected]>

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

--
viresh

2021-06-30 17:01:51

by Rafael J. Wysocki

[permalink] [raw]

Subject: 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 <[email protected]> wrote:
>
> On 23-06-21, 17:13, Rafael J. Wysocki wrote:
> > As mentioned already in
> >
> > https://lore.kernel.org/linux-pm/CAJZ5v0g2tCZptcqh+c55YYiO7rDHmZivMLsmpq_7005zNPN1xg@mail.gmail.com/
>
> Sorry about failing to reply over that, I got confused somehow..
>
> > 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.
>
> > From: Rafael J. Wysocki <[email protected]>
> > 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 <[email protected]>
> > ---
> > 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 <[email protected]>

Thanks!

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