[PATCH 0/3] CPUFreq Fixes for 3.10-rc2

[PATCH 0/3] CPUFreq Fixes for 3.10-rc2

[Viresh Kumar]

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(-)

-- 
1.7.12.rc2.18.g61b472e

[PATCH 1/3] cpufreq: Add EXPORT_SYMBOL_GPL for have_governor_per_policy

[Viresh Kumar]

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)
 {
-- 
1.7.12.rc2.18.g61b472e

[PATCH 2/3] cpufreq: governors: Move get_governor_parent_kobj() to cpufreq.c

[Viresh Kumar]

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 */
-- 
1.7.12.rc2.18.g61b472e

[PATCH 3/3] cpufreq: Drop rwsem lock around CPUFREQ_GOV_POLICY_EXIT

[Viresh Kumar]

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) {
-- 
1.7.12.rc2.18.g61b472e

Re: [PATCH 0/3] CPUFreq Fixes for 3.10-rc2

[Rafael J. Wysocki]

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


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

Re: [PATCH 0/3] CPUFreq Fixes for 3.10-rc2

[Viresh Kumar]

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.

Re: [PATCH 0/3] CPUFreq Fixes for 3.10-rc2

[Rafael J. Wysocki]

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


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

Re: [PATCH 0/3] CPUFreq Fixes for 3.10-rc2

[Viresh Kumar]

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.

Re: [PATCH 0/3] CPUFreq Fixes for 3.10-rc2

[Rafael J. Wysocki]

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


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

Re: [PATCH 0/3] CPUFreq Fixes for 3.10-rc2

[Viresh Kumar]

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.

Re: [PATCH 0/3] CPUFreq Fixes for 3.10-rc2

[Rafael J. Wysocki]

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


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

Re: [PATCH 0/3] CPUFreq Fixes for 3.10-rc2

[Viresh Kumar]

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 :)

Re: [PATCH 0/3] CPUFreq Fixes for 3.10-rc2

[Rafael J. Wysocki]

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


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

Re: [PATCH 0/3] CPUFreq Fixes for 3.10-rc2

[Viresh Kumar]

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