Hi Rafael,
Here are few more fixes for 3.10-rc2.
Viresh Kumar (3): cpufreq: Add EXPORT_SYMBOL_GPL for have_governor_per_policy cpufreq: governors: Move get_governor_parent_kobj() to cpufreq.c cpufreq: Drop rwsem lock around CPUFREQ_GOV_POLICY_EXIT
drivers/cpufreq/cpufreq.c | 19 +++++++++++++++++-- drivers/cpufreq/cpufreq_governor.c | 8 -------- include/linux/cpufreq.h | 1 + 3 files changed, 18 insertions(+), 10 deletions(-)
This patch adds: EXPORT_SYMBOL_GPL(have_governor_per_policy), so that this routine can be used by modules too.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org --- drivers/cpufreq/cpufreq.c | 1 + 1 file changed, 1 insertion(+)
diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c index b7acfd1..21a7fb0 100644 --- a/drivers/cpufreq/cpufreq.c +++ b/drivers/cpufreq/cpufreq.c @@ -132,6 +132,7 @@ bool have_governor_per_policy(void) { return cpufreq_driver->have_governor_per_policy; } +EXPORT_SYMBOL_GPL(have_governor_per_policy);
static struct cpufreq_policy *__cpufreq_cpu_get(unsigned int cpu, bool sysfs) {
get_governor_parent_kobj() can be used by any governor, generic cpufreq governors or platform specific ones and so must be present in cpufreq.c instead of cpufreq_governor.c.
This patch moves it to cpufreq.c. This also adds EXPORT_SYMBOL_GPL(get_governor_parent_kobj) so that modules can use this function too.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org --- drivers/cpufreq/cpufreq.c | 9 +++++++++ drivers/cpufreq/cpufreq_governor.c | 8 -------- include/linux/cpufreq.h | 1 + 3 files changed, 10 insertions(+), 8 deletions(-)
diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c index 21a7fb0..cb0f723 100644 --- a/drivers/cpufreq/cpufreq.c +++ b/drivers/cpufreq/cpufreq.c @@ -134,6 +134,15 @@ bool have_governor_per_policy(void) } EXPORT_SYMBOL_GPL(have_governor_per_policy);
+struct kobject *get_governor_parent_kobj(struct cpufreq_policy *policy) +{ + if (have_governor_per_policy()) + return &policy->kobj; + else + return cpufreq_global_kobject; +} +EXPORT_SYMBOL_GPL(get_governor_parent_kobj); + static struct cpufreq_policy *__cpufreq_cpu_get(unsigned int cpu, bool sysfs) { struct cpufreq_policy *data; diff --git a/drivers/cpufreq/cpufreq_governor.c b/drivers/cpufreq/cpufreq_governor.c index 5af40ad..d1421b4 100644 --- a/drivers/cpufreq/cpufreq_governor.c +++ b/drivers/cpufreq/cpufreq_governor.c @@ -29,14 +29,6 @@
#include "cpufreq_governor.h"
-static struct kobject *get_governor_parent_kobj(struct cpufreq_policy *policy) -{ - if (have_governor_per_policy()) - return &policy->kobj; - else - return cpufreq_global_kobject; -} - static struct attribute_group *get_sysfs_attr(struct dbs_data *dbs_data) { if (have_governor_per_policy()) diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h index aa0c2a3..7ffb4d5 100644 --- a/include/linux/cpufreq.h +++ b/include/linux/cpufreq.h @@ -340,6 +340,7 @@ const char *cpufreq_get_current_driver(void); int cpufreq_get_policy(struct cpufreq_policy *policy, unsigned int cpu); int cpufreq_update_policy(unsigned int cpu); bool have_governor_per_policy(void); +struct kobject *get_governor_parent_kobj(struct cpufreq_policy *policy);
#ifdef CONFIG_CPU_FREQ /* query the current CPU frequency (in kHz). If zero, cpufreq couldn't detect it */
With this lock around __cpufreq_governor(policy, CPUFREQ_GOV_POLICY_EXIT), we get circular dependency when we call sysfs_remove_group().
[ 195.319079] ====================================================== [ 195.337653] [ INFO: possible circular locking dependency detected ] [ 195.356497] 3.9.0-rc7+ #15 Not tainted [ 195.367758] ------------------------------------------------------- [ 195.386613] cat/2387 is trying to acquire lock: [ 195.400176] (&per_cpu(cpu_policy_rwsem, cpu)){+++++.}, at: [<c02f6179>] lock_policy_rwsem_read+0x25/0x34 [ 195.428920] [ 195.428920] but task is already holding lock: [ 195.446393] (s_active#41){++++.+}, at: [<c00f9bf7>] sysfs_read_file+0x4f/0xcc [ 195.468305] [ 195.468305] which lock already depends on the new lock. [ 195.468305] [ 195.492830] [ 195.492830] the existing dependency chain (in reverse order) is: [ 195.515250] -> #1 (s_active#41){++++.+}: [ 195.527647] [<c0055a79>] lock_acquire+0x61/0xbc [ 195.543129] [<c00fabf1>] sysfs_addrm_finish+0xc1/0x128 [ 195.560362] [<c00f9819>] sysfs_hash_and_remove+0x35/0x64 [ 195.578119] [<c00fbe6f>] remove_files.isra.0+0x1b/0x24 [ 195.595497] [<c00fbea5>] sysfs_remove_group+0x2d/0xa8 [ 195.612469] [<c02f9a0b>] cpufreq_governor_interactive+0x13b/0x35c [ 195.632668] [<c02f61df>] __cpufreq_governor+0x2b/0x8c [ 195.649644] [<c02f6579>] __cpufreq_set_policy+0xa9/0xf8 [ 195.667132] [<c02f6b75>] store_scaling_governor+0x61/0x100 [ 195.685404] [<c02f6f4d>] store+0x39/0x60 [ 195.698989] [<c00f9b81>] sysfs_write_file+0xed/0x114 [ 195.715694] [<c00b3fd1>] vfs_write+0x65/0xd8 [ 195.730320] [<c00b424b>] sys_write+0x2f/0x50 [ 195.744943] [<c000cdc1>] ret_fast_syscall+0x1/0x52 [ 195.761135] -> #0 (&per_cpu(cpu_policy_rwsem, cpu)){+++++.}: [ 195.778665] [<c0055253>] __lock_acquire+0xef3/0x13dc [ 195.795371] [<c0055a79>] lock_acquire+0x61/0xbc [ 195.810776] [<c03ee1f5>] down_read+0x25/0x30 [ 195.825398] [<c02f6179>] lock_policy_rwsem_read+0x25/0x34 [ 195.843410] [<c02f6edd>] show+0x21/0x58 [ 195.856731] [<c00f9c0f>] sysfs_read_file+0x67/0xcc [ 195.872919] [<c00b40a7>] vfs_read+0x63/0xd8 [ 195.887282] [<c00b41fb>] sys_read+0x2f/0x50 [ 195.901645] [<c000cdc1>] ret_fast_syscall+0x1/0x52 [ 195.917863] [ 195.917863] other info that might help us debug this: [ 195.917863] [ 195.941853] Possible unsafe locking scenario: [ 195.941853] [ 195.959586] CPU0 CPU1 [ 195.973149] ---- ---- [ 195.986712] lock(s_active#41); [ 195.996407] lock(&per_cpu(cpu_policy_rwsem, cpu)); [ 196.018912] lock(s_active#41); [ 196.036161] lock(&per_cpu(cpu_policy_rwsem, cpu)); [ 196.051051] [ 196.051051] *** DEADLOCK *** [ 196.051051] [ 196.068792] 2 locks held by cat/2387: [ 196.079750] #0: (&buffer->mutex){+.+.+.}, at: [<c00f9bcd>] sysfs_read_file+0x25/0xcc [ 196.103546] #1: (s_active#41){++++.+}, at: [<c00f9bf7>] sysfs_read_file+0x4f/0xcc [ 196.126577] [ 196.126577] stack backtrace: [ 196.139644] [<c0011d55>] (unwind_backtrace+0x1/0x9c) from [<c03e9a09>] (print_circular_bug+0x19d/0x1e8) [ 196.167857] [<c03e9a09>] (print_circular_bug+0x19d/0x1e8) from [<c0055253>] (__lock_acquire+0xef3/0x13dc) [ 196.196542] [<c0055253>] (__lock_acquire+0xef3/0x13dc) from [<c0055a79>] (lock_acquire+0x61/0xbc) [ 196.223139] [<c0055a79>] (lock_acquire+0x61/0xbc) from [<c03ee1f5>] (down_read+0x25/0x30) [ 196.247722] [<c03ee1f5>] (down_read+0x25/0x30) from [<c02f6179>] (lock_policy_rwsem_read+0x25/0x34) [ 196.274905] [<c02f6179>] (lock_policy_rwsem_read+0x25/0x34) from [<c02f6edd>] (show+0x21/0x58) [ 196.300724] [<c02f6edd>] (show+0x21/0x58) from [<c00f9c0f>] (sysfs_read_file+0x67/0xcc) [ 196.324719] [<c00f9c0f>] (sysfs_read_file+0x67/0xcc) from [<c00b40a7>] (vfs_read+0x63/0xd8) [ 196.349756] [<c00b40a7>] (vfs_read+0x63/0xd8) from [<c00b41fb>] (sys_read+0x2f/0x50) [ 196.372970] [<c00b41fb>] (sys_read+0x2f/0x50) from [<c000cdc1>] (ret_fast_syscall+0x1/0x52)
This lock isn't required while calling __cpufreq_governor(policy, CPUFREQ_GOV_POLICY_EXIT). Remove it.
Signed-off-by: Viresh Kumar viresh.kumar@linaro.org --- drivers/cpufreq/cpufreq.c | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-)
diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c index cb0f723..2d5a829 100644 --- a/drivers/cpufreq/cpufreq.c +++ b/drivers/cpufreq/cpufreq.c @@ -1739,18 +1739,23 @@ static int __cpufreq_set_policy(struct cpufreq_policy *data, /* end old governor */ if (data->governor) { __cpufreq_governor(data, CPUFREQ_GOV_STOP); + unlock_policy_rwsem_write(policy->cpu); __cpufreq_governor(data, CPUFREQ_GOV_POLICY_EXIT); + lock_policy_rwsem_write(policy->cpu); }
/* start new governor */ data->governor = policy->governor; if (!__cpufreq_governor(data, CPUFREQ_GOV_POLICY_INIT)) { - if (!__cpufreq_governor(data, CPUFREQ_GOV_START)) + if (!__cpufreq_governor(data, CPUFREQ_GOV_START)) { failed = 0; - else + } else { + unlock_policy_rwsem_write(policy->cpu); __cpufreq_governor(data, CPUFREQ_GOV_POLICY_EXIT); + lock_policy_rwsem_write(policy->cpu); + } }
if (failed) {
On Thursday, May 16, 2013 10:39:55 AM Viresh Kumar wrote:
Hi Rafael,
Hi,
Here are few more fixes for 3.10-rc2.
I'm actually going to sent the for-rc2 request today, so these are for -rc3.
Viresh Kumar (3): cpufreq: Add EXPORT_SYMBOL_GPL for have_governor_per_policy cpufreq: governors: Move get_governor_parent_kobj() to cpufreq.c cpufreq: Drop rwsem lock around CPUFREQ_GOV_POLICY_EXIT
Thanks, Rafael
On 16 May 2013 14:16, Rafael J. Wysocki rjw@sisk.pl wrote:
On Thursday, May 16, 2013 10:39:55 AM Viresh Kumar wrote:
Hi Rafael,
Hi,
Here are few more fixes for 3.10-rc2.
I'm actually going to sent the for-rc2 request today, so these are for -rc3.
Okay. No issues.
On Thursday, May 16, 2013 02:09:46 PM Viresh Kumar wrote:
On 16 May 2013 14:16, Rafael J. Wysocki rjw@sisk.pl wrote:
On Thursday, May 16, 2013 10:39:55 AM Viresh Kumar wrote:
Hi Rafael,
Hi,
Here are few more fixes for 3.10-rc2.
I'm actually going to sent the for-rc2 request today, so these are for -rc3.
Okay. No issues.
While I kind of understand why you want [3/3] to go into 3.10, I'm wondering about the other two patches. Why exactly are they needed now?
And speaking of patch [3/3], if you paste kernel logs into changelogs, please remove the time stamps (unless they are relevant to the problem being fixed).
Thanks, Rafael
On 17 May 2013 02:21, Rafael J. Wysocki rjw@sisk.pl wrote:
While I kind of understand why you want [3/3] to go into 3.10, I'm wondering about the other two patches. Why exactly are they needed now?
First one:
cpufreq: Add EXPORT_SYMBOL_GPL for have_governor_per_policy
is required so that governors can be compiled as module. Otherwise they may break.. I haven't tried that but I believe that is the case.
For second one:
cpufreq: governors: Move get_governor_parent_kobj() to cpufreq.c
I was actually working on Android's Interactive governor (Where I enabled same per-policy-instance of governor support), And i required this patch for its working. Then I thought other platform specific governors present in mainline might also use this routine and so must be pushed to cpufreq.c.
Now, because this patch is around due to something which got added in 3.10-rc1, its better to fix it in rcs only rather than fixing it in 3.11. I will trust your judgement here, push it for 3.10-rc if it looks okay to you.
And speaking of patch [3/3], if you paste kernel logs into changelogs, please remove the time stamps (unless they are relevant to the problem being fixed).
Okay.. Will keep that in mind.
On Friday, May 17, 2013 10:13:37 AM Viresh Kumar wrote:
On 17 May 2013 02:21, Rafael J. Wysocki rjw@sisk.pl wrote:
While I kind of understand why you want [3/3] to go into 3.10, I'm wondering about the other two patches. Why exactly are they needed now?
First one:
cpufreq: Add EXPORT_SYMBOL_GPL for have_governor_per_policy
is required so that governors can be compiled as module. Otherwise they may break.. I haven't tried that but I believe that is the case.
Did you try to build them as modules?
For second one:
cpufreq: governors: Move get_governor_parent_kobj() to cpufreq.c
I was actually working on Android's Interactive governor (Where I enabled same per-policy-instance of governor support), And i required this patch for its working. Then I thought other platform specific governors present in mainline might also use this routine and so must be pushed to cpufreq.c.
Now, because this patch is around due to something which got added in 3.10-rc1, its better to fix it in rcs only rather than fixing it in 3.11.
OK, thanks for the explanation.
I will trust your judgement here, push it for 3.10-rc if it looks okay to you.
And speaking of patch [3/3], if you paste kernel logs into changelogs, please remove the time stamps (unless they are relevant to the problem being fixed).
Okay.. Will keep that in mind.
Thanks!
Rafael
On 17 May 2013 17:46, Rafael J. Wysocki rjw@sisk.pl wrote:
On Friday, May 17, 2013 10:13:37 AM Viresh Kumar wrote:
On 17 May 2013 02:21, Rafael J. Wysocki rjw@sisk.pl wrote:
While I kind of understand why you want [3/3] to go into 3.10, I'm wondering about the other two patches. Why exactly are they needed now?
First one:
cpufreq: Add EXPORT_SYMBOL_GPL for have_governor_per_policy
is required so that governors can be compiled as module. Otherwise they may break.. I haven't tried that but I believe that is the case.
Did you try to build them as modules?
That's what: "I haven't tried that but I believe that is the case.".. Modules need variables to be exported with EXPORT_SYMBOL_GPL to be used by them.. And I thought this is pretty straight forward.
On Friday, May 17, 2013 07:22:05 PM Viresh Kumar wrote:
On 17 May 2013 17:46, Rafael J. Wysocki rjw@sisk.pl wrote:
On Friday, May 17, 2013 10:13:37 AM Viresh Kumar wrote:
On 17 May 2013 02:21, Rafael J. Wysocki rjw@sisk.pl wrote:
While I kind of understand why you want [3/3] to go into 3.10, I'm wondering about the other two patches. Why exactly are they needed now?
First one:
cpufreq: Add EXPORT_SYMBOL_GPL for have_governor_per_policy
is required so that governors can be compiled as module. Otherwise they may break.. I haven't tried that but I believe that is the case.
Did you try to build them as modules?
That's what: "I haven't tried that but I believe that is the case.".. Modules need variables to be exported with EXPORT_SYMBOL_GPL to be used by them.. And I thought this is pretty straight forward.
Well, I actually meant "can you please verify your belief?". :-)
And that's because I'm wondering why the zero-day build testing doesn't catch this problem. Apparently, it doesn't build .configs with cpufreq governors configured as modules, although I believe it does test "make allmodconfig" for a couple of architectures at least. What gives?
Rafael
On 18 May 2013 05:10, Rafael J. Wysocki rjw@sisk.pl wrote:
Well, I actually meant "can you please verify your belief?". :-)
And that's because I'm wondering why the zero-day build testing doesn't catch this problem. Apparently, it doesn't build .configs with cpufreq governors configured as modules, although I believe it does test "make allmodconfig" for a couple of architectures at least. What gives?
My assumption was wrong. Actually cpufreq_governor.c is never compiled as module, but cpufreq_ondemand is...
And this routine isn't used from cpufreq_ondemand but cpufreq_governor..
But we were lucky that we didn't get a error here and EXPORT_SYMBOL is still required :)
On Saturday, May 18, 2013 07:45:45 AM Viresh Kumar wrote:
On 18 May 2013 05:10, Rafael J. Wysocki rjw@sisk.pl wrote:
Well, I actually meant "can you please verify your belief?". :-)
And that's because I'm wondering why the zero-day build testing doesn't catch this problem. Apparently, it doesn't build .configs with cpufreq governors configured as modules, although I believe it does test "make allmodconfig" for a couple of architectures at least. What gives?
My assumption was wrong. Actually cpufreq_governor.c is never compiled as module, but cpufreq_ondemand is...
And this routine isn't used from cpufreq_ondemand but cpufreq_governor..
But we were lucky that we didn't get a error here and EXPORT_SYMBOL is still required :)
Although not necessarily 3.10 material I suppose?
Rafael
On 18 May 2013 14:46, Rafael J. Wysocki rjw@sisk.pl wrote:
Although not necessarily 3.10 material I suppose?
Yes.. Pick only 3/3 for 3.10 and others for later part..
linaro-kernel@lists.linaro.org
-
Rafael J. Wysocki -
Viresh Kumar