Hi,
This series is based on the observation that the policy rwsem is used (or rather not
used) inconsistently in cpufreq_remove_dev() (in summary, it does things without
holding the policy rwsem that are done under that rwsem elsewhere).
The first two patches are preparatory (but patch [1/3] is a good enough improvement
by itself IMO) and patch [3/3] makes the essential change.
Please refer to the patch changelogs for details.
Thanks!
From: Rafael J. Wysocki <[email protected]>
Currently, cpufreq_remove_dev() invokes the ->exit() driver callback
without holding the policy rwsem which is inconsistent with what
happens if ->exit() is invoked directly from cpufreq_offline().
It also manipulates the real_cpus mask and removes the CPU device
symlink without holding the policy rwsem, but cpufreq_offline() holds
the rwsem around the modifications thereof.
For consistency, modify cpufreq_remove_dev() to hold the policy rwsem
until the ->exit() callback has been called (or it has been determined
that it is not necessary to call it).
Signed-off-by: Rafael J. Wysocki <[email protected]>
---
drivers/cpufreq/cpufreq.c | 21 ++++++++++++++-------
1 file changed, 14 insertions(+), 7 deletions(-)
Index: linux-pm/drivers/cpufreq/cpufreq.c
===================================================================
--- linux-pm.orig/drivers/cpufreq/cpufreq.c
+++ linux-pm/drivers/cpufreq/cpufreq.c
@@ -1659,19 +1659,26 @@ static void cpufreq_remove_dev(struct de
if (!policy)
return;
+ down_write(&policy->rwsem);
+
if (cpu_online(cpu))
- cpufreq_offline(cpu);
+ __cpufreq_offline(cpu, policy);
cpumask_clear_cpu(cpu, policy->real_cpus);
remove_cpu_dev_symlink(policy, dev);
- if (cpumask_empty(policy->real_cpus)) {
- /* We did light-weight exit earlier, do full tear down now */
- if (cpufreq_driver->offline)
- cpufreq_driver->exit(policy);
-
- cpufreq_policy_free(policy);
+ if (!cpumask_empty(policy->real_cpus)) {
+ up_write(&policy->rwsem);
+ return;
}
+
+ /* We did light-weight exit earlier, do full tear down now */
+ if (cpufreq_driver->offline)
+ cpufreq_driver->exit(policy);
+
+ up_write(&policy->rwsem);
+
+ cpufreq_policy_free(policy);
}
/**
From: Rafael J. Wysocki <[email protected]>
Notice that cpufreq_offline() only needs to check policy_is_inactive()
once and rearrange the code in there to make that happen.
No expected functional impact.
Signed-off-by: Rafael J. Wysocki <[email protected]>
---
drivers/cpufreq/cpufreq.c | 24 ++++++++++++------------
1 file changed, 12 insertions(+), 12 deletions(-)
Index: linux-pm/drivers/cpufreq/cpufreq.c
===================================================================
--- linux-pm.orig/drivers/cpufreq/cpufreq.c
+++ linux-pm/drivers/cpufreq/cpufreq.c
@@ -1590,24 +1590,18 @@ static int cpufreq_offline(unsigned int
}
down_write(&policy->rwsem);
+
if (has_target())
cpufreq_stop_governor(policy);
cpumask_clear_cpu(cpu, policy->cpus);
- if (policy_is_inactive(policy)) {
- if (has_target())
- strncpy(policy->last_governor, policy->governor->name,
- CPUFREQ_NAME_LEN);
- else
- policy->last_policy = policy->policy;
- } else if (cpu == policy->cpu) {
- /* Nominate new CPU */
- policy->cpu = cpumask_any(policy->cpus);
- }
-
- /* Start governor again for active policy */
if (!policy_is_inactive(policy)) {
+ /* Nominate a new CPU if necessary. */
+ if (cpu == policy->cpu)
+ policy->cpu = cpumask_any(policy->cpus);
+
+ /* Start the governor again for the active policy. */
if (has_target()) {
ret = cpufreq_start_governor(policy);
if (ret)
@@ -1617,6 +1611,12 @@ static int cpufreq_offline(unsigned int
goto unlock;
}
+ if (has_target())
+ strncpy(policy->last_governor, policy->governor->name,
+ CPUFREQ_NAME_LEN);
+ else
+ policy->last_policy = policy->policy;
+
if (cpufreq_thermal_control_enabled(cpufreq_driver)) {
cpufreq_cooling_unregister(policy->cdev);
policy->cdev = NULL;
From: Rafael J. Wysocki <[email protected]>
Split the "core" part running under the policy rwsem out of
cpufreq_offline() to allow the locking in cpufreq_remove_dev() to be
rearranged more easily.
As a side-effect this eliminates the unlock label that's not needed
any more.
No expected functional impact.
Signed-off-by: Rafael J. Wysocki <[email protected]>
---
drivers/cpufreq/cpufreq.c | 33 +++++++++++++++++++--------------
1 file changed, 19 insertions(+), 14 deletions(-)
Index: linux-pm/drivers/cpufreq/cpufreq.c
===================================================================
--- linux-pm.orig/drivers/cpufreq/cpufreq.c
+++ linux-pm/drivers/cpufreq/cpufreq.c
@@ -1576,21 +1576,10 @@ static int cpufreq_add_dev(struct device
return 0;
}
-static int cpufreq_offline(unsigned int cpu)
+static void __cpufreq_offline(unsigned int cpu, struct cpufreq_policy *policy)
{
- struct cpufreq_policy *policy;
int ret;
- pr_debug("%s: unregistering CPU %u\n", __func__, cpu);
-
- policy = cpufreq_cpu_get_raw(cpu);
- if (!policy) {
- pr_debug("%s: No cpu_data found\n", __func__);
- return 0;
- }
-
- down_write(&policy->rwsem);
-
if (has_target())
cpufreq_stop_governor(policy);
@@ -1608,7 +1597,7 @@ static int cpufreq_offline(unsigned int
pr_err("%s: Failed to start governor\n", __func__);
}
- goto unlock;
+ return;
}
if (has_target())
@@ -1635,8 +1624,24 @@ static int cpufreq_offline(unsigned int
cpufreq_driver->exit(policy);
policy->freq_table = NULL;
}
+}
+
+static int cpufreq_offline(unsigned int cpu)
+{
+ struct cpufreq_policy *policy;
+
+ pr_debug("%s: unregistering CPU %u\n", __func__, cpu);
+
+ policy = cpufreq_cpu_get_raw(cpu);
+ if (!policy) {
+ pr_debug("%s: No cpu_data found\n", __func__);
+ return 0;
+ }
+
+ down_write(&policy->rwsem);
+
+ __cpufreq_offline(cpu, policy);
-unlock:
up_write(&policy->rwsem);
return 0;
}
On 11-05-22, 17:50, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <[email protected]>
>
> Split the "core" part running under the policy rwsem out of
> cpufreq_offline() to allow the locking in cpufreq_remove_dev() to be
> rearranged more easily.
>
> As a side-effect this eliminates the unlock label that's not needed
> any more.
>
> No expected functional impact.
>
> Signed-off-by: Rafael J. Wysocki <[email protected]>
> ---
> drivers/cpufreq/cpufreq.c | 33 +++++++++++++++++++--------------
> 1 file changed, 19 insertions(+), 14 deletions(-)
Acked-by: Viresh Kumar <[email protected]>
--
viresh
On 11-05-22, 17:51, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <[email protected]>
>
> Currently, cpufreq_remove_dev() invokes the ->exit() driver callback
> without holding the policy rwsem which is inconsistent with what
> happens if ->exit() is invoked directly from cpufreq_offline().
>
> It also manipulates the real_cpus mask and removes the CPU device
> symlink without holding the policy rwsem, but cpufreq_offline() holds
> the rwsem around the modifications thereof.
>
> For consistency, modify cpufreq_remove_dev() to hold the policy rwsem
> until the ->exit() callback has been called (or it has been determined
> that it is not necessary to call it).
>
> Signed-off-by: Rafael J. Wysocki <[email protected]>
> ---
> drivers/cpufreq/cpufreq.c | 21 ++++++++++++++-------
> 1 file changed, 14 insertions(+), 7 deletions(-)
Acked-by: Viresh Kumar <[email protected]>
--
viresh
On 11-05-22, 17:48, Rafael J. Wysocki wrote:
> From: Rafael J. Wysocki <[email protected]>
>
> Notice that cpufreq_offline() only needs to check policy_is_inactive()
> once and rearrange the code in there to make that happen.
>
> No expected functional impact.
>
> Signed-off-by: Rafael J. Wysocki <[email protected]>
> ---
> drivers/cpufreq/cpufreq.c | 24 ++++++++++++------------
> 1 file changed, 12 insertions(+), 12 deletions(-)
Acked-by: Viresh Kumar <[email protected]>
--
viresh