We call __find_governor() during addition of first CPU of every policy to find the last governor used for this CPU before it was hotplugged-out.
After that we call cpufreq_parse_governor() in cpufreq_init_policy() either with this governor or default governor. And right after that policy->governor is set to NULL.
So, instead of doing this move the relevant parts to cpufreq_init_policy() policy only and initialize policy->governor to NULL at the beginning.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org ---
Hi Saravana,
I hope only the first two patches would fix things for you but probably you can test all three.
@Rafael: We might need to get these in next rc only as these are more or less fixes.
drivers/cpufreq/cpufreq.c | 34 ++++++++++++++++------------------ 1 file changed, 16 insertions(+), 18 deletions(-)
diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c index c755b5f..cc4f244 100644 --- a/drivers/cpufreq/cpufreq.c +++ b/drivers/cpufreq/cpufreq.c @@ -879,18 +879,27 @@ err_out_kobj_put:
static void cpufreq_init_policy(struct cpufreq_policy *policy) { + struct cpufreq_governor *gov = NULL; struct cpufreq_policy new_policy; int ret = 0;
memcpy(&new_policy, policy, sizeof(*policy));
+ /* Update governor of new_policy to the governor used before hotplug */ +#ifdef CONFIG_HOTPLUG_CPU + gov = __find_governor(per_cpu(cpufreq_cpu_governor, policy->cpu)); +#endif + if (gov) + pr_debug("Restoring governor %s for cpu %d\n", + policy->governor->name, policy->cpu); + else + gov = CPUFREQ_DEFAULT_GOVERNOR; + + new_policy.governor = gov; + /* Use the default policy if its valid. */ if (cpufreq_driver->setpolicy) - cpufreq_parse_governor(policy->governor->name, - &new_policy.policy, NULL); - - /* assure that the starting sequence is run in cpufreq_set_policy */ - policy->governor = NULL; + cpufreq_parse_governor(gov->name, &new_policy.policy, NULL);
/* set default policy */ ret = cpufreq_set_policy(policy, &new_policy); @@ -944,11 +953,11 @@ static struct cpufreq_policy *cpufreq_policy_restore(unsigned int cpu) unsigned long flags;
read_lock_irqsave(&cpufreq_driver_lock, flags); - policy = per_cpu(cpufreq_cpu_data_fallback, cpu); - read_unlock_irqrestore(&cpufreq_driver_lock, flags);
+ policy->governor = NULL; + return policy; }
@@ -1036,7 +1045,6 @@ static int __cpufreq_add_dev(struct device *dev, struct subsys_interface *sif, unsigned long flags; #ifdef CONFIG_HOTPLUG_CPU struct cpufreq_policy *tpolicy; - struct cpufreq_governor *gov; #endif
if (cpu_is_offline(cpu)) @@ -1094,7 +1102,6 @@ static int __cpufreq_add_dev(struct device *dev, struct subsys_interface *sif, else policy->cpu = cpu;
- policy->governor = CPUFREQ_DEFAULT_GOVERNOR; cpumask_copy(policy->cpus, cpumask_of(cpu));
init_completion(&policy->kobj_unregister); @@ -1179,15 +1186,6 @@ static int __cpufreq_add_dev(struct device *dev, struct subsys_interface *sif, blocking_notifier_call_chain(&cpufreq_policy_notifier_list, CPUFREQ_START, policy);
-#ifdef CONFIG_HOTPLUG_CPU - gov = __find_governor(per_cpu(cpufreq_cpu_governor, cpu)); - if (gov) { - policy->governor = gov; - pr_debug("Restoring governor %s for cpu %d\n", - policy->governor->name, cpu); - } -#endif - if (!frozen) { ret = cpufreq_add_dev_interface(policy, dev); if (ret)
Policy must be fully initialized before it is being made available for use by others. This patch moves some initialization code before making policy available for others.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org --- drivers/cpufreq/cpufreq.c | 28 ++++++++++++++-------------- 1 file changed, 14 insertions(+), 14 deletions(-)
diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c index cc4f244..110c0cd 100644 --- a/drivers/cpufreq/cpufreq.c +++ b/drivers/cpufreq/cpufreq.c @@ -1116,6 +1116,20 @@ static int __cpufreq_add_dev(struct device *dev, struct subsys_interface *sif, goto err_set_policy_cpu; }
+ /* related cpus should atleast have policy->cpus */ + cpumask_or(policy->related_cpus, policy->related_cpus, policy->cpus); + + /* + * affected cpus must always be the one, which are online. We aren't + * managing offline cpus here. + */ + cpumask_and(policy->cpus, policy->cpus, cpu_online_mask); + + if (!frozen) { + policy->user_policy.min = policy->min; + policy->user_policy.max = policy->max; + } + write_lock_irqsave(&cpufreq_driver_lock, flags); for_each_cpu(j, policy->cpus) per_cpu(cpufreq_cpu_data, j) = policy; @@ -1169,20 +1183,6 @@ static int __cpufreq_add_dev(struct device *dev, struct subsys_interface *sif, } }
- /* related cpus should atleast have policy->cpus */ - cpumask_or(policy->related_cpus, policy->related_cpus, policy->cpus); - - /* - * affected cpus must always be the one, which are online. We aren't - * managing offline cpus here. - */ - cpumask_and(policy->cpus, policy->cpus, cpu_online_mask); - - if (!frozen) { - policy->user_policy.min = policy->min; - policy->user_policy.max = policy->max; - } - blocking_notifier_call_chain(&cpufreq_policy_notifier_list, CPUFREQ_START, policy);
On Tuesday, February 25, 2014 02:20:10 PM Viresh Kumar wrote:
Policy must be fully initialized before it is being made available for use by others.
True enough. And the problem is?
This patch moves some initialization code before making policy available for others.
So why/how exactly does this fix the problem?
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org
drivers/cpufreq/cpufreq.c | 28 ++++++++++++++-------------- 1 file changed, 14 insertions(+), 14 deletions(-)
diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c index cc4f244..110c0cd 100644 --- a/drivers/cpufreq/cpufreq.c +++ b/drivers/cpufreq/cpufreq.c @@ -1116,6 +1116,20 @@ static int __cpufreq_add_dev(struct device *dev, struct subsys_interface *sif, goto err_set_policy_cpu; }
- /* related cpus should atleast have policy->cpus */
- cpumask_or(policy->related_cpus, policy->related_cpus, policy->cpus);
- /*
* affected cpus must always be the one, which are online. We aren't* managing offline cpus here.*/- cpumask_and(policy->cpus, policy->cpus, cpu_online_mask);
- if (!frozen) {
policy->user_policy.min = policy->min;policy->user_policy.max = policy->max;- }
- write_lock_irqsave(&cpufreq_driver_lock, flags); for_each_cpu(j, policy->cpus) per_cpu(cpufreq_cpu_data, j) = policy;
@@ -1169,20 +1183,6 @@ static int __cpufreq_add_dev(struct device *dev, struct subsys_interface *sif, } }
- /* related cpus should atleast have policy->cpus */
- cpumask_or(policy->related_cpus, policy->related_cpus, policy->cpus);
- /*
* affected cpus must always be the one, which are online. We aren't* managing offline cpus here.*/- cpumask_and(policy->cpus, policy->cpus, cpu_online_mask);
- if (!frozen) {
policy->user_policy.min = policy->min;policy->user_policy.max = policy->max;- }
- blocking_notifier_call_chain(&cpufreq_policy_notifier_list, CPUFREQ_START, policy);
policy->rwsem is used to lock access to all parts of code modifying struct cpufreq_policy but wasn't used on a new policy created from __cpufreq_add_dev().
Because of which if we call cpufreq_update_policy() repeatedly on one CPU and do offline/online of another CPU then we might see these crashes:
Unable to handle kernel NULL pointer dereference at virtual address 00000020 pgd = c0003000 [00000020] *pgd=80000000004003, *pmd=00000000 Internal error: Oops: 206 [#1] PREEMPT SMP ARM
PC is at __cpufreq_governor+0x10/0x1ac LR is at cpufreq_update_policy+0x114/0x150
---[ end trace f23a8defea6cd706 ]--- Kernel panic - not syncing: Fatal exception CPU0: stopping CPU: 0 PID: 7136 Comm: mpdecision Tainted: G D W 3.10.0-gd727407-00074-g979ede8 #396
[<c0afe180>] (notifier_call_chain+0x40/0x68) from [<c02a23ac>] (__blocking_notifier_call_chain+0x40/0x58) [<c02a23ac>] (__blocking_notifier_call_chain+0x40/0x58) from [<c02a23d8>] (blocking_notifier_call_chain+0x14/0x1c) [<c02a23d8>] (blocking_notifier_call_chain+0x14/0x1c) from [<c0803c68>] (cpufreq_set_policy+0xd4/0x2b8) [<c0803c68>] (cpufreq_set_policy+0xd4/0x2b8) from [<c0803e7c>] (cpufreq_init_policy+0x30/0x98) [<c0803e7c>] (cpufreq_init_policy+0x30/0x98) from [<c0805a18>] (__cpufreq_add_dev.isra.17+0x4dc/0x7a4) [<c0805a18>] (__cpufreq_add_dev.isra.17+0x4dc/0x7a4) from [<c0805d38>] (cpufreq_cpu_callback+0x58/0x84) [<c0805d38>] (cpufreq_cpu_callback+0x58/0x84) from [<c0afe180>] (notifier_call_chain+0x40/0x68) [<c0afe180>] (notifier_call_chain+0x40/0x68) from [<c02812dc>] (__cpu_notify+0x28/0x44) [<c02812dc>] (__cpu_notify+0x28/0x44) from [<c0aeed90>] (_cpu_up+0xf4/0x1dc) [<c0aeed90>] (_cpu_up+0xf4/0x1dc) from [<c0aeeed4>] (cpu_up+0x5c/0x78) [<c0aeeed4>] (cpu_up+0x5c/0x78) from [<c0aec808>] (store_online+0x44/0x74) [<c0aec808>] (store_online+0x44/0x74) from [<c03a40f4>] (sysfs_write_file+0x108/0x14c) [<c03a40f4>] (sysfs_write_file+0x108/0x14c) from [<c03517d4>] (vfs_write+0xd0/0x180) [<c03517d4>] (vfs_write+0xd0/0x180) from [<c0351ca8>] (SyS_write+0x38/0x68) [<c0351ca8>] (SyS_write+0x38/0x68) from [<c0205de0>] (ret_fast_syscall+0x0/0x30)
Fix these by taking locks at appropriate places in __cpufreq_add_dev() as well.
Reported-by: Saravana Kannan skannan@codeaurora.org Suggested-by: Srivatsa S. Bhat srivatsa.bhat@linux.vnet.ibm.com Signed-off-by: Viresh Kumar viresh.kumar@linaro.org --- drivers/cpufreq/cpufreq.c | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c index 110c0cd..513d1d4 100644 --- a/drivers/cpufreq/cpufreq.c +++ b/drivers/cpufreq/cpufreq.c @@ -1130,6 +1130,7 @@ static int __cpufreq_add_dev(struct device *dev, struct subsys_interface *sif, policy->user_policy.max = policy->max; }
+ down_write(&policy->rwsem); write_lock_irqsave(&cpufreq_driver_lock, flags); for_each_cpu(j, policy->cpus) per_cpu(cpufreq_cpu_data, j) = policy; @@ -1204,6 +1205,7 @@ static int __cpufreq_add_dev(struct device *dev, struct subsys_interface *sif, policy->user_policy.policy = policy->policy; policy->user_policy.governor = policy->governor; } + up_write(&policy->rwsem);
kobject_uevent(&policy->kobj, KOBJ_ADD); up_read(&cpufreq_rwsem);
On Tuesday, February 25, 2014 02:20:09 PM Viresh Kumar wrote:
We call __find_governor() during addition of first CPU of every policy to find the last governor used for this CPU before it was hotplugged-out.
After that we call cpufreq_parse_governor() in cpufreq_init_policy() either with this governor or default governor. And right after that policy->governor is set to NULL.
This is a problem, right? So care to write *why* it is a problem here?
So, instead of doing this move the relevant parts to cpufreq_init_policy() policy only and initialize policy->governor to NULL at the beginning.
And this change is supposed to fix that problem, right? You're not moving stuff around just for the fun of it?
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org
Hi Saravana,
I hope only the first two patches would fix things for you but probably you can test all three.
@Rafael: We might need to get these in next rc only as these are more or less fixes.
drivers/cpufreq/cpufreq.c | 34 ++++++++++++++++------------------ 1 file changed, 16 insertions(+), 18 deletions(-)
diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c index c755b5f..cc4f244 100644 --- a/drivers/cpufreq/cpufreq.c +++ b/drivers/cpufreq/cpufreq.c @@ -879,18 +879,27 @@ err_out_kobj_put: static void cpufreq_init_policy(struct cpufreq_policy *policy) {
- struct cpufreq_governor *gov = NULL; struct cpufreq_policy new_policy; int ret = 0;
memcpy(&new_policy, policy, sizeof(*policy));
- /* Update governor of new_policy to the governor used before hotplug */
+#ifdef CONFIG_HOTPLUG_CPU
- gov = __find_governor(per_cpu(cpufreq_cpu_governor, policy->cpu));
+#endif
- if (gov)
pr_debug("Restoring governor %s for cpu %d\n",policy->governor->name, policy->cpu);- else
gov = CPUFREQ_DEFAULT_GOVERNOR;- new_policy.governor = gov;
- /* Use the default policy if its valid. */ if (cpufreq_driver->setpolicy)
cpufreq_parse_governor(policy->governor->name,&new_policy.policy, NULL);- /* assure that the starting sequence is run in cpufreq_set_policy */
- policy->governor = NULL;
cpufreq_parse_governor(gov->name, &new_policy.policy, NULL);/* set default policy */ ret = cpufreq_set_policy(policy, &new_policy); @@ -944,11 +953,11 @@ static struct cpufreq_policy *cpufreq_policy_restore(unsigned int cpu) unsigned long flags; read_lock_irqsave(&cpufreq_driver_lock, flags);
- policy = per_cpu(cpufreq_cpu_data_fallback, cpu);
- read_unlock_irqrestore(&cpufreq_driver_lock, flags);
- policy->governor = NULL;
- return policy;
} @@ -1036,7 +1045,6 @@ static int __cpufreq_add_dev(struct device *dev, struct subsys_interface *sif, unsigned long flags; #ifdef CONFIG_HOTPLUG_CPU struct cpufreq_policy *tpolicy;
- struct cpufreq_governor *gov;
#endif if (cpu_is_offline(cpu)) @@ -1094,7 +1102,6 @@ static int __cpufreq_add_dev(struct device *dev, struct subsys_interface *sif, else policy->cpu = cpu;
- policy->governor = CPUFREQ_DEFAULT_GOVERNOR; cpumask_copy(policy->cpus, cpumask_of(cpu));
init_completion(&policy->kobj_unregister); @@ -1179,15 +1186,6 @@ static int __cpufreq_add_dev(struct device *dev, struct subsys_interface *sif, blocking_notifier_call_chain(&cpufreq_policy_notifier_list, CPUFREQ_START, policy); -#ifdef CONFIG_HOTPLUG_CPU
- gov = __find_governor(per_cpu(cpufreq_cpu_governor, cpu));
- if (gov) {
policy->governor = gov;pr_debug("Restoring governor %s for cpu %d\n",policy->governor->name, cpu);- }
-#endif
- if (!frozen) { ret = cpufreq_add_dev_interface(policy, dev); if (ret)
On Tuesday, February 25, 2014 02:20:09 PM Viresh Kumar wrote:
We call __find_governor() during addition of first CPU of every policy to find the last governor used for this CPU before it was hotplugged-out.
After that we call cpufreq_parse_governor() in cpufreq_init_policy() either with this governor or default governor. And right after that policy->governor is set to NULL.
So, instead of doing this move the relevant parts to cpufreq_init_policy() policy only and initialize policy->governor to NULL at the beginning.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org
Hi Saravana,
I hope only the first two patches would fix things for you but probably you can test all three.
@Rafael: We might need to get these in next rc only as these are more or less fixes.
drivers/cpufreq/cpufreq.c | 34 ++++++++++++++++------------------ 1 file changed, 16 insertions(+), 18 deletions(-)
diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c index c755b5f..cc4f244 100644 --- a/drivers/cpufreq/cpufreq.c +++ b/drivers/cpufreq/cpufreq.c @@ -879,18 +879,27 @@ err_out_kobj_put: static void cpufreq_init_policy(struct cpufreq_policy *policy) {
- struct cpufreq_governor *gov = NULL; struct cpufreq_policy new_policy; int ret = 0;
memcpy(&new_policy, policy, sizeof(*policy));
And while I'm at it, can we *please* avoid adding new #ifdef blocks into function bodies?
Please introduce a wrapper around __find_governor() returning NULL for CONFIG_HOTPLUG_CPU unset.
- /* Update governor of new_policy to the governor used before hotplug */
+#ifdef CONFIG_HOTPLUG_CPU
- gov = __find_governor(per_cpu(cpufreq_cpu_governor, policy->cpu));
+#endif
- if (gov)
pr_debug("Restoring governor %s for cpu %d\n",policy->governor->name, policy->cpu);- else
gov = CPUFREQ_DEFAULT_GOVERNOR;- new_policy.governor = gov;
- /* Use the default policy if its valid. */ if (cpufreq_driver->setpolicy)
cpufreq_parse_governor(policy->governor->name,&new_policy.policy, NULL);- /* assure that the starting sequence is run in cpufreq_set_policy */
- policy->governor = NULL;
cpufreq_parse_governor(gov->name, &new_policy.policy, NULL);/* set default policy */ ret = cpufreq_set_policy(policy, &new_policy); @@ -944,11 +953,11 @@ static struct cpufreq_policy *cpufreq_policy_restore(unsigned int cpu) unsigned long flags; read_lock_irqsave(&cpufreq_driver_lock, flags);
- policy = per_cpu(cpufreq_cpu_data_fallback, cpu);
Why do these whitespace changes belong to this patch?
read_unlock_irqrestore(&cpufreq_driver_lock, flags);
- policy->governor = NULL;
- return policy;
} @@ -1036,7 +1045,6 @@ static int __cpufreq_add_dev(struct device *dev, struct subsys_interface *sif, unsigned long flags; #ifdef CONFIG_HOTPLUG_CPU struct cpufreq_policy *tpolicy;
- struct cpufreq_governor *gov;
#endif if (cpu_is_offline(cpu)) @@ -1094,7 +1102,6 @@ static int __cpufreq_add_dev(struct device *dev, struct subsys_interface *sif, else policy->cpu = cpu;
- policy->governor = CPUFREQ_DEFAULT_GOVERNOR; cpumask_copy(policy->cpus, cpumask_of(cpu));
init_completion(&policy->kobj_unregister); @@ -1179,15 +1186,6 @@ static int __cpufreq_add_dev(struct device *dev, struct subsys_interface *sif, blocking_notifier_call_chain(&cpufreq_policy_notifier_list, CPUFREQ_START, policy); -#ifdef CONFIG_HOTPLUG_CPU
- gov = __find_governor(per_cpu(cpufreq_cpu_governor, cpu));
- if (gov) {
policy->governor = gov;pr_debug("Restoring governor %s for cpu %d\n",policy->governor->name, cpu);- }
-#endif
- if (!frozen) { ret = cpufreq_add_dev_interface(policy, dev); if (ret)
On 1 March 2014 06:39, Rafael J. Wysocki rjw@rjwysocki.net wrote:
And while I'm at it, can we *please* avoid adding new #ifdef blocks into function bodies?
Please introduce a wrapper around __find_governor() returning NULL for CONFIG_HOTPLUG_CPU unset.
I have tried fixing all the suggestions you gave for this series. Please have a look at V2 set and let me know if I am still missing anything.
linaro-kernel@lists.linaro.org
-
Rafael J. Wysocki -
Viresh Kumar