[PATCH V2 1/3] cpufreq: move call to __find_governor() to cpufreq_init_policy()

[PATCH V2 1/3] cpufreq: move call to __find_governor() to cpufreq_init_policy()

[Viresh Kumar]

We call __find_governor() during addition of first CPU of every policy from
__cpufreq_add_dev() 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.

There is no functional problem here with this code, but it is just that some of
the code required in cpufreq_init_policy() is being called by its caller, i.e.
__cpufreq_add_dev(). So, it would make more sense to get all these together at a
single place to make code more readable.

So, instead of doing this move the relevant parts to cpufreq_init_policy()
policy only and initialize policy->governor to NULL at the beginning.

In order to clean the code a bit more, some of the #ifdefs for
CONFIG_HOTPLUG_CPU are also removed which used to maintain the last governor
used for any CPU. Keeping that code wouldn't harm us much but would improve the
cleanliness of code.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
V1->V2:
- Improved commit logs
- Removed unrelated whitespace changes
- Removed some #ifdef CONFIG_HOTPLUG_CPU to clean code a bit.

 drivers/cpufreq/cpufreq.c | 38 ++++++++++++++------------------------
 1 file changed, 14 insertions(+), 24 deletions(-)

diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index 56b7b1b..fff2968 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -42,10 +42,8 @@ static DEFINE_RWLOCK(cpufreq_driver_lock);
 DEFINE_MUTEX(cpufreq_governor_lock);
 static LIST_HEAD(cpufreq_policy_list);
 
-#ifdef CONFIG_HOTPLUG_CPU
 /* This one keeps track of the previously set governor of a removed CPU */
 static DEFINE_PER_CPU(char[CPUFREQ_NAME_LEN], cpufreq_cpu_governor);
-#endif
 
 static inline bool has_target(void)
 {
@@ -879,18 +877,25 @@ 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 */
+	gov = __find_governor(per_cpu(cpufreq_cpu_governor, policy->cpu));
+	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);
@@ -949,6 +954,8 @@ static struct cpufreq_policy *cpufreq_policy_restore(unsigned int cpu)
 
 	read_unlock_irqrestore(&cpufreq_driver_lock, flags);
 
+	policy->governor = NULL;
+
 	return policy;
 }
 
@@ -1036,7 +1043,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 +1100,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 +1184,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)
@@ -1312,11 +1308,9 @@ static int __cpufreq_remove_dev_prepare(struct device *dev,
 		}
 	}
 
-#ifdef CONFIG_HOTPLUG_CPU
 	if (!cpufreq_driver->setpolicy)
 		strncpy(per_cpu(cpufreq_cpu_governor, cpu),
 			policy->governor->name, CPUFREQ_NAME_LEN);
-#endif
 
 	down_read(&policy->rwsem);
 	cpus = cpumask_weight(policy->cpus);
@@ -1955,9 +1949,7 @@ EXPORT_SYMBOL_GPL(cpufreq_register_governor);
 
 void cpufreq_unregister_governor(struct cpufreq_governor *governor)
 {
-#ifdef CONFIG_HOTPLUG_CPU
 	int cpu;
-#endif
 
 	if (!governor)
 		return;
@@ -1965,14 +1957,12 @@ void cpufreq_unregister_governor(struct cpufreq_governor *governor)
 	if (cpufreq_disabled())
 		return;
 
-#ifdef CONFIG_HOTPLUG_CPU
 	for_each_present_cpu(cpu) {
 		if (cpu_online(cpu))
 			continue;
 		if (!strcmp(per_cpu(cpufreq_cpu_governor, cpu), governor->name))
 			strcpy(per_cpu(cpufreq_cpu_governor, cpu), "\0");
 	}
-#endif
 
 	mutex_lock(&cpufreq_governor_mutex);
 	list_del(&governor->governor_list);
-- 
1.7.12.rc2.18.g61b472e

[PATCH V2 2/3] cpufreq: Initialize policy before making it available for others to use

[Viresh Kumar]

Policy must be fully initialized before it is being made available for use by
others. Otherwise cpufreq_cpu_get() would be able to grab a half initialized
policy structure that might not have affected_cpus (for example) filled. And so
anybody accessing those fields will get the wrong value and hence the results
would be unpredictable.

So, in order to fix this lets do all the necessary initialization before we make
policy structure available via cpufreq_cpu_get(). With this we can guarantee
that any code accessing fields of policy would be stable enough, as policy would
be completely initialized by now.

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
V1->V2:
- Improved commit logs

 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 fff2968..3c6f9a5 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -1114,6 +1114,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;
@@ -1167,20 +1181,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);
 
-- 
1.7.12.rc2.18.g61b472e

[PATCH V2 3/3] cpufreq: initialize governor for a new policy under policy->rwsem

[Viresh Kumar]

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>
---
V1->V2: No change

 drivers/cpufreq/cpufreq.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index 3c6f9a5..e2a1e67 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -1128,6 +1128,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;
@@ -1202,6 +1203,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);
-- 
1.7.12.rc2.18.g61b472e

Re: [PATCH V2 3/3] cpufreq: initialize governor for a new policy under policy->rwsem

[Rafael J. Wysocki]

On Tuesday, March 04, 2014 11:44:01 AM Viresh Kumar wrote:
> 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>

I've rebased this one on top of 3.14-rc5 and queued it up for 3.14-rc6.

Please check the bleeding-edge branch for the result.

> ---
> V1->V2: No change
> 
>  drivers/cpufreq/cpufreq.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> index 3c6f9a5..e2a1e67 100644
> --- a/drivers/cpufreq/cpufreq.c
> +++ b/drivers/cpufreq/cpufreq.c
> @@ -1128,6 +1128,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;
> @@ -1202,6 +1203,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);
> 

-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [PATCH V2 3/3] cpufreq: initialize governor for a new policy under policy->rwsem

[Rafael J. Wysocki]

On Thursday, March 06, 2014 02:04:39 AM Rafael J. Wysocki wrote:
> On Tuesday, March 04, 2014 11:44:01 AM Viresh Kumar wrote:
> > 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>
> 
> I've rebased this one on top of 3.14-rc5 and queued it up for 3.14-rc6.
> 
> Please check the bleeding-edge branch for the result.

Actually, I think I'll queue up [2-3/3] for 3.14-rc6 instead.

> 
> > ---
> > V1->V2: No change
> > 
> >  drivers/cpufreq/cpufreq.c | 2 ++
> >  1 file changed, 2 insertions(+)
> > 
> > diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> > index 3c6f9a5..e2a1e67 100644
> > --- a/drivers/cpufreq/cpufreq.c
> > +++ b/drivers/cpufreq/cpufreq.c
> > @@ -1128,6 +1128,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;
> > @@ -1202,6 +1203,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);
> > 
> 
> 

-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [PATCH V2 3/3] cpufreq: initialize governor for a new policy under policy->rwsem

[Saravana Kannan]

On 03/05/2014 05:06 PM, Rafael J. Wysocki wrote:
> On Thursday, March 06, 2014 02:04:39 AM Rafael J. Wysocki wrote:
>> On Tuesday, March 04, 2014 11:44:01 AM Viresh Kumar wrote:
>>> 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>
>>
>> I've rebased this one on top of 3.14-rc5 and queued it up for 3.14-rc6.
>>
>> Please check the bleeding-edge branch for the result.
>
> Actually, I think I'll queue up [2-3/3] for 3.14-rc6 instead.
>

Pretty close to having this tested and reported back. So, if you can 
wait, that would be better. Should probably see an email by Fri evening PST.

-Saravana


-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
hosted by The Linux Foundation

Re: [PATCH V2 3/3] cpufreq: initialize governor for a new policy under policy->rwsem

[Rafael J. Wysocki]

On Wednesday, March 05, 2014 05:10:01 PM Saravana Kannan wrote:
> On 03/05/2014 05:06 PM, Rafael J. Wysocki wrote:
> > On Thursday, March 06, 2014 02:04:39 AM Rafael J. Wysocki wrote:
> >> On Tuesday, March 04, 2014 11:44:01 AM Viresh Kumar wrote:
> >>> 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>
> >>
> >> I've rebased this one on top of 3.14-rc5 and queued it up for 3.14-rc6.
> >>
> >> Please check the bleeding-edge branch for the result.
> >
> > Actually, I think I'll queue up [2-3/3] for 3.14-rc6 instead.
> >
> 
> Pretty close to having this tested and reported back. So, if you can 
> wait, that would be better. Should probably see an email by Fri evening PST.

OK

It won't hurt if they stay in bleeding-edge/linux-next till then, though.

Thanks!

-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: [PATCH V2 3/3] cpufreq: initialize governor for a new policy under policy->rwsem

[Viresh Kumar]

On 6 March 2014 09:04, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> I've rebased this one on top of 3.14-rc5 and queued it up for 3.14-rc6.
>
> Please check the bleeding-edge branch for the result.

Yeah, it looks fine. And I assume that you are planning to take 1/3 in 3.15?
Or going to drop it?

Re: [PATCH V2 3/3] cpufreq: initialize governor for a new policy under policy->rwsem

[Rafael J. Wysocki]

On Thursday, March 06, 2014 10:24:38 AM Viresh Kumar wrote:
> On 6 March 2014 09:04, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> > I've rebased this one on top of 3.14-rc5 and queued it up for 3.14-rc6.
> >
> > Please check the bleeding-edge branch for the result.
> 
> Yeah, it looks fine. And I assume that you are planning to take 1/3 in 3.15?
> Or going to drop it?

I'm going to queue it up for 3.15.

Thanks!

-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.