cpufreq-next opps with cpufreq-bench

cpufreq-next opps with cpufreq-bench

[Andrew Lunn]

Hi Viresh

I said off list last week i has having problems with your cpufreq-next
branch with cpufreq-bench.

It is simple to reproduce, just run cpufreq-bench and within a few
seconds you get:

Unable to handle kernel NULL pointer dereference at virtual address 00000000
pgd = cfa60000
[00000000] *pgd=0d7c9831, *pte=00000000, *ppte=00000000
Internal error: Oops: 17 [#1] PREEMPT ARM
Modules linked in:
CPU: 0 PID: 2427 Comm: cpufreq-bench Not tainted 3.12.0-rc4-00708-g1546af5 #86
task: cfa98e60 ti: ce58a000 task.ti: ce58a000
PC is at wake_up_process+0xc/0x48
LR is at __up_write+0x158/0x164
pc : [<c0040a14>]    lr : [<c0209630>]    psr: 60000093
sp : ce58bd90  ip : ce58bda8  fp : ce58bda4
r10: cfb3aa28  r9 : ce58bea8  r8 : ce58beb8
r7 : ce58beac  r6 : 00000000  r5 : cfb3a9c0  r4 : ce58be10
r3 : cfb3aa5c  r2 : cfb3aa5c  r1 : 00000000  r0 : 00000000
Flags: nZCv  IRQs off  FIQs on  Mode SVC_32  ISA ARM  Segment user
Control: 0005397f  Table: 0fa60000  DAC: 00000015
Process cpufreq-bench (pid: 2427, stack limit = 0xce58a1c0)
Stack: (0xce58bd90 to 0xce58c000)
bd80:                                     ce58be10 cfb3a9c0 ce58bddc ce58bda8
bda0: c0209630 c0040a18 0000000b 80000013 ce58bdcc ce58be10 cfb3a9c0 c064c2a8
bdc0: ce58bea8 ce58beb8 0000000b cfb3aa28 ce58bdec ce58bde0 c0039fe8 c02094e8
bde0: ce58be0c ce58bdf0 c03578a0 c0039fe8 cfb3a9c0 ce58be10 00000000 0000000b
be00: ce58beec ce58be10 c0358efc c0357790 00000001 00000001 00000000 00000000
be20: 00000000 00000000 00186a00 00082355 00001388 00082355 00186a00 00082355
be40: 00000000 c064c278 cfae7220 00000001 ffffffe0 cfb3aa04 cfb3aa04 c0358dec
be60: 00082355 00186a00 00000000 c064c2a8 c064c108 c064c108 cfae71c0 cfb3aa2c
be80: cfb3aa2c c0622808 00000000 c064c0c4 cfae6880 00000002 00000003 00000000
bea0: cfb3aa50 cfb3aa50 00000000 cfb3aa5c cfb3aa5c 00000000 66726570 616d726f
bec0: 0065636e ce58bed0 c04ab470 cfb3aa58 cfb3a9c0 ce58bf78 cfae6a80 c04d6e3c
bee0: ce58bf14 ce58bef0 c03576e8 c0358e60 c0082cd4 0000000b cfb43000 c064c1b0
bf00: cfa12f40 cfa12f58 ce58bf44 ce58bf18 c011a078 c0357670 ce58bf78 cf8f6a00
bf20: 0000000b bedcfaf0 ce58bf78 0000000b ce58a000 00000000 ce58bf74 ce58bf48
bf40: c00bb340 c0119f80 c00b8b10 c00c97f8 cf8f6a00 00000000 00000000 00000000
bf60: 0000000b 00000000 ce58bfa4 ce58bf78 c00bb4f0 c00bb284 00000000 00000000
bf80: c00095e4 0000000b 00000006 bedcfaf0 00000004 c00095e4 00000000 ce58bfa8
bfa0: c0009480 c00bb4b4 0000000b 00000006 00000006 bedcfaf0 0000000b bedcfa15
bfc0: 0000000b 00000006 bedcfaf0 00000004 000139b0 0000b08c 0000b0c0 00014008
bfe0: 00000000 bedcf9d4 b6ebba68 b6e4186c 60000010 00000006 00000000 00000000
Backtrace: 
[<c0040a08>] (wake_up_process+0x0/0x48) from [<c0209630>] (__up_write+0x158/0x164)
 r5:cfb3a9c0 r4:ce58be10
[<c02094d8>] (__up_write+0x0/0x164) from [<c0039fe8>] (up_write+0x10/0x14)
[<c0039fd8>] (up_write+0x0/0x14) from [<c03578a0>] (cpufreq_set_policy+0x120/0x1cc)
[<c0357780>] (cpufreq_set_policy+0x0/0x1cc) from [<c0358efc>] (store_scaling_governor+0xac/0x1a4)
 r7:0000000b r6:00000000 r5:ce58be10 r4:cfb3a9c0
[<c0358e50>] (store_scaling_governor+0x0/0x1a4) from [<c03576e8>] (store+0x88/0xa4)
 r8:c04d6e3c r7:cfae6a80 r6:ce58bf78 r5:cfb3a9c0 r4:cfb3aa58
[<c0357660>] (store+0x0/0xa4) from [<c011a078>] (sysfs_write_file+0x108/0x188)
 r5:cfa12f58 r4:cfa12f40
[<c0119f70>] (sysfs_write_file+0x0/0x188) from [<c00bb340>] (vfs_write+0xcc/0x198)
[<c00bb274>] (vfs_write+0x0/0x198) from [<c00bb4f0>] (SyS_write+0x4c/0x78)
[<c00bb4a4>] (SyS_write+0x0/0x78) from [<c0009480>] (ret_fast_syscall+0x0/0x2c)
 r8:c00095e4 r7:00000004 r6:bedcfaf0 r5:00000006 r4:0000000b
Code: e89da800 e1a0c00d e92dd830 e24cb004 (e5903000) 
---[ end trace a412cb5cfd5edab9 ]---
note: cpufreq-bench[2427] exited with preempt_count 1

I did a bisect, and the problem is in the patch:

cpufreq: create per policy rwsem instead of per cpu cpu_policy_rwsem
e4762ba2d652dbc4c496c5abdcc80702544ca1fc

Thanks
	Andrew

Re: cpufreq-next opps with cpufreq-bench

[Rafael J. Wysocki]

On Sunday, October 13, 2013 11:01:59 AM Andrew Lunn wrote:
> Hi Viresh
> 
> I said off list last week i has having problems with your cpufreq-next
> branch with cpufreq-bench.
> 
> It is simple to reproduce, just run cpufreq-bench and within a few
> seconds you get:
> 
> Unable to handle kernel NULL pointer dereference at virtual address 00000000
> pgd = cfa60000
> [00000000] *pgd=0d7c9831, *pte=00000000, *ppte=00000000
> Internal error: Oops: 17 [#1] PREEMPT ARM
> Modules linked in:
> CPU: 0 PID: 2427 Comm: cpufreq-bench Not tainted 3.12.0-rc4-00708-g1546af5 #86
> task: cfa98e60 ti: ce58a000 task.ti: ce58a000
> PC is at wake_up_process+0xc/0x48
> LR is at __up_write+0x158/0x164
> pc : [<c0040a14>]    lr : [<c0209630>]    psr: 60000093
> sp : ce58bd90  ip : ce58bda8  fp : ce58bda4
> r10: cfb3aa28  r9 : ce58bea8  r8 : ce58beb8
> r7 : ce58beac  r6 : 00000000  r5 : cfb3a9c0  r4 : ce58be10
> r3 : cfb3aa5c  r2 : cfb3aa5c  r1 : 00000000  r0 : 00000000
> Flags: nZCv  IRQs off  FIQs on  Mode SVC_32  ISA ARM  Segment user
> Control: 0005397f  Table: 0fa60000  DAC: 00000015
> Process cpufreq-bench (pid: 2427, stack limit = 0xce58a1c0)
> Stack: (0xce58bd90 to 0xce58c000)
> bd80:                                     ce58be10 cfb3a9c0 ce58bddc ce58bda8
> bda0: c0209630 c0040a18 0000000b 80000013 ce58bdcc ce58be10 cfb3a9c0 c064c2a8
> bdc0: ce58bea8 ce58beb8 0000000b cfb3aa28 ce58bdec ce58bde0 c0039fe8 c02094e8
> bde0: ce58be0c ce58bdf0 c03578a0 c0039fe8 cfb3a9c0 ce58be10 00000000 0000000b
> be00: ce58beec ce58be10 c0358efc c0357790 00000001 00000001 00000000 00000000
> be20: 00000000 00000000 00186a00 00082355 00001388 00082355 00186a00 00082355
> be40: 00000000 c064c278 cfae7220 00000001 ffffffe0 cfb3aa04 cfb3aa04 c0358dec
> be60: 00082355 00186a00 00000000 c064c2a8 c064c108 c064c108 cfae71c0 cfb3aa2c
> be80: cfb3aa2c c0622808 00000000 c064c0c4 cfae6880 00000002 00000003 00000000
> bea0: cfb3aa50 cfb3aa50 00000000 cfb3aa5c cfb3aa5c 00000000 66726570 616d726f
> bec0: 0065636e ce58bed0 c04ab470 cfb3aa58 cfb3a9c0 ce58bf78 cfae6a80 c04d6e3c
> bee0: ce58bf14 ce58bef0 c03576e8 c0358e60 c0082cd4 0000000b cfb43000 c064c1b0
> bf00: cfa12f40 cfa12f58 ce58bf44 ce58bf18 c011a078 c0357670 ce58bf78 cf8f6a00
> bf20: 0000000b bedcfaf0 ce58bf78 0000000b ce58a000 00000000 ce58bf74 ce58bf48
> bf40: c00bb340 c0119f80 c00b8b10 c00c97f8 cf8f6a00 00000000 00000000 00000000
> bf60: 0000000b 00000000 ce58bfa4 ce58bf78 c00bb4f0 c00bb284 00000000 00000000
> bf80: c00095e4 0000000b 00000006 bedcfaf0 00000004 c00095e4 00000000 ce58bfa8
> bfa0: c0009480 c00bb4b4 0000000b 00000006 00000006 bedcfaf0 0000000b bedcfa15
> bfc0: 0000000b 00000006 bedcfaf0 00000004 000139b0 0000b08c 0000b0c0 00014008
> bfe0: 00000000 bedcf9d4 b6ebba68 b6e4186c 60000010 00000006 00000000 00000000
> Backtrace: 
> [<c0040a08>] (wake_up_process+0x0/0x48) from [<c0209630>] (__up_write+0x158/0x164)
>  r5:cfb3a9c0 r4:ce58be10
> [<c02094d8>] (__up_write+0x0/0x164) from [<c0039fe8>] (up_write+0x10/0x14)
> [<c0039fd8>] (up_write+0x0/0x14) from [<c03578a0>] (cpufreq_set_policy+0x120/0x1cc)
> [<c0357780>] (cpufreq_set_policy+0x0/0x1cc) from [<c0358efc>] (store_scaling_governor+0xac/0x1a4)
>  r7:0000000b r6:00000000 r5:ce58be10 r4:cfb3a9c0
> [<c0358e50>] (store_scaling_governor+0x0/0x1a4) from [<c03576e8>] (store+0x88/0xa4)
>  r8:c04d6e3c r7:cfae6a80 r6:ce58bf78 r5:cfb3a9c0 r4:cfb3aa58
> [<c0357660>] (store+0x0/0xa4) from [<c011a078>] (sysfs_write_file+0x108/0x188)
>  r5:cfa12f58 r4:cfa12f40
> [<c0119f70>] (sysfs_write_file+0x0/0x188) from [<c00bb340>] (vfs_write+0xcc/0x198)
> [<c00bb274>] (vfs_write+0x0/0x198) from [<c00bb4f0>] (SyS_write+0x4c/0x78)
> [<c00bb4a4>] (SyS_write+0x0/0x78) from [<c0009480>] (ret_fast_syscall+0x0/0x2c)
>  r8:c00095e4 r7:00000004 r6:bedcfaf0 r5:00000006 r4:0000000b
> Code: e89da800 e1a0c00d e92dd830 e24cb004 (e5903000) 
> ---[ end trace a412cb5cfd5edab9 ]---
> note: cpufreq-bench[2427] exited with preempt_count 1
> 
> I did a bisect, and the problem is in the patch:
> 
> cpufreq: create per policy rwsem instead of per cpu cpu_policy_rwsem
> e4762ba2d652dbc4c496c5abdcc80702544ca1fc

Is this bug present in current linux-next?

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

Re: cpufreq-next opps with cpufreq-bench

[Andrew Lunn]

> > I did a bisect, and the problem is in the patch:
> > 
> > cpufreq: create per policy rwsem instead of per cpu cpu_policy_rwsem
> > e4762ba2d652dbc4c496c5abdcc80702544ca1fc
> 
> Is this bug present in current linux-next?

The current linux-next seems to be next-20130927, since Stephen
Rothwell is away to the moment. The patch causing the problem is not
in linux-next.

However, i will build and test, tomorrow.

	 Andrew

Re: cpufreq-next opps with cpufreq-bench

[Andrew Lunn]

On Sun, Oct 13, 2013 at 11:09:15PM +0200, Andrew Lunn wrote:
> > > I did a bisect, and the problem is in the patch:
> > > 
> > > cpufreq: create per policy rwsem instead of per cpu cpu_policy_rwsem
> > > e4762ba2d652dbc4c496c5abdcc80702544ca1fc
> > 
> > Is this bug present in current linux-next?
> 
> The current linux-next seems to be next-20130927, since Stephen
> Rothwell is away to the moment. The patch causing the problem is not
> in linux-next.

Hi Rafael

It is however in your linux-next branch:

http://git.kernel.org/cgit/linux/kernel/git/rafael/linux-pm.git/commit/drivers/cpufreq/cpufreq.c?h=linux-next&id=e4762ba2d652dbc4c496c5abdcc80702544ca1fc

I will build your tree and test...

  Andrew

Re: cpufreq-next opps with cpufreq-bench

[Rafael J. Wysocki]

On Sunday, October 13, 2013 11:09:15 PM Andrew Lunn wrote:
> > > I did a bisect, and the problem is in the patch:
> > > 
> > > cpufreq: create per policy rwsem instead of per cpu cpu_policy_rwsem
> > > e4762ba2d652dbc4c496c5abdcc80702544ca1fc
> > 
> > Is this bug present in current linux-next?
> 
> The current linux-next seems to be next-20130927, since Stephen
> Rothwell is away to the moment. The patch causing the problem is not
> in linux-next.
> 
> However, i will build and test, tomorrow.

Well, I was talking about the current linux-next branch of the tree at:

git://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm.git

Thanks!

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

Re: cpufreq-next opps with cpufreq-bench

[Andrew Lunn]

On Sun, Oct 13, 2013 at 11:14:36PM +0200, Andrew Lunn wrote:
> On Sun, Oct 13, 2013 at 11:09:15PM +0200, Andrew Lunn wrote:
> > > > I did a bisect, and the problem is in the patch:
> > > > 
> > > > cpufreq: create per policy rwsem instead of per cpu cpu_policy_rwsem
> > > > e4762ba2d652dbc4c496c5abdcc80702544ca1fc
> > > 
> > > Is this bug present in current linux-next?
> > 
> > The current linux-next seems to be next-20130927, since Stephen
> > Rothwell is away to the moment. The patch causing the problem is not
> > in linux-next.
> 
> Hi Rafael
> 
> It is however in your linux-next branch:
> 
> http://git.kernel.org/cgit/linux/kernel/git/rafael/linux-pm.git/commit/drivers/cpufreq/cpufreq.c?h=linux-next&id=e4762ba2d652dbc4c496c5abdcc80702544ca1fc
> 
> I will build your tree and test...

Done.

It also has the bug.

   Andrew

Re: cpufreq-next opps with cpufreq-bench

[Rafael J. Wysocki]

On Sunday, October 13, 2013 11:45:47 PM Andrew Lunn wrote:
> On Sun, Oct 13, 2013 at 11:14:36PM +0200, Andrew Lunn wrote:
> > On Sun, Oct 13, 2013 at 11:09:15PM +0200, Andrew Lunn wrote:
> > > > > I did a bisect, and the problem is in the patch:
> > > > > 
> > > > > cpufreq: create per policy rwsem instead of per cpu cpu_policy_rwsem
> > > > > e4762ba2d652dbc4c496c5abdcc80702544ca1fc
> > > > 
> > > > Is this bug present in current linux-next?
> > > 
> > > The current linux-next seems to be next-20130927, since Stephen
> > > Rothwell is away to the moment. The patch causing the problem is not
> > > in linux-next.
> > 
> > Hi Rafael
> > 
> > It is however in your linux-next branch:
> > 
> > http://git.kernel.org/cgit/linux/kernel/git/rafael/linux-pm.git/commit/drivers/cpufreq/cpufreq.c?h=linux-next&id=e4762ba2d652dbc4c496c5abdcc80702544ca1fc
> > 
> > I will build your tree and test...
> 
> Done.
> 
> It also has the bug.

Well, I've dropped the offending commit from my queue.  Hopefully, it still
builds on all targets it used to build on.

Thanks!

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

Re: cpufreq-next opps with cpufreq-bench

[Viresh Kumar]

[-- Attachment #1: Type: text/plain, Size: 5029 bytes --]

On 16/10/2013, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> Well, I've dropped the offending commit from my queue.  Hopefully, it still
> builds on all targets it used to build on.

Thanks!!

I tried to reproduce it 2 days back and my laptop was almost completely
discharged and I have left my charger at a friends place last week :(
So couldn't really do anything until then.. Got my charger today and so
coming back on this..

I couldn't run and find cpufreq-bench on ubuntu (Is it available?)..
But I think I have found the problem..

@Andrew: Can you give this a try please? (Use attached commit)..

commit 0c5d6493fe02606512e94fad3ea075bc4f56b7e7
Author: Viresh Kumar <viresh.kumar@linaro.org>
Date:   Wed Oct 16 09:39:18 2013 +0530

    cpufreq: Use correct rwsem in cpufreq_set_policy()

    Recent commit "cpufreq: create per policy rwsem instead of per CPU
    cpu_policy_rwsem" introduced a bug where kernel crashes when we run
    cpufreq-bench. Crash looks like this:

    Unable to handle kernel NULL pointer dereference at virtual address 00000000
    pgd = cfa60000
    [00000000] *pgd=0d7c9831, *pte=00000000, *ppte=00000000
    Internal error: Oops: 17 [#1] PREEMPT ARM
    Modules linked in:
    CPU: 0 PID: 2427 Comm: cpufreq-bench Not tainted
3.12.0-rc4-00708-g1546af5 #86
    task: cfa98e60 ti: ce58a000 task.ti: ce58a000
    PC is at wake_up_process+0xc/0x48
    LR is at __up_write+0x158/0x164
    pc : [<c0040a14>]    lr : [<c0209630>]    psr: 60000093
    sp : ce58bd90  ip : ce58bda8  fp : ce58bda4
    r10: cfb3aa28  r9 : ce58bea8  r8 : ce58beb8
    r7 : ce58beac  r6 : 00000000  r5 : cfb3a9c0  r4 : ce58be10
    r3 : cfb3aa5c  r2 : cfb3aa5c  r1 : 00000000  r0 : 00000000
    Flags: nZCv  IRQs off  FIQs on  Mode SVC_32  ISA ARM  Segment user
    Control: 0005397f  Table: 0fa60000  DAC: 00000015
    Process cpufreq-bench (pid: 2427, stack limit = 0xce58a1c0)
    Stack: (0xce58bd90 to 0xce58c000)

    <snip>

    Backtrace:
    [<c0040a08>] (wake_up_process+0x0/0x48) from [<c0209630>]
(__up_write+0x158/0x164)
     r5:cfb3a9c0 r4:ce58be10
    [<c02094d8>] (__up_write+0x0/0x164) from [<c0039fe8>] (up_write+0x10/0x14)
    [<c0039fd8>] (up_write+0x0/0x14) from [<c03578a0>]
(cpufreq_set_policy+0x120/0x1cc)
    [<c0357780>] (cpufreq_set_policy+0x0/0x1cc) from [<c0358efc>]
(store_scaling_governor+0xac/0x1a4)
     r7:0000000b r6:00000000 r5:ce58be10 r4:cfb3a9c0
    [<c0358e50>] (store_scaling_governor+0x0/0x1a4) from [<c03576e8>]
(store+0x88/0xa4)
     r8:c04d6e3c r7:cfae6a80 r6:ce58bf78 r5:cfb3a9c0 r4:cfb3aa58
    [<c0357660>] (store+0x0/0xa4) from [<c011a078>]
(sysfs_write_file+0x108/0x188)
     r5:cfa12f58 r4:cfa12f40
    [<c0119f70>] (sysfs_write_file+0x0/0x188) from [<c00bb340>]
(vfs_write+0xcc/0x198)
    [<c00bb274>] (vfs_write+0x0/0x198) from [<c00bb4f0>] (SyS_write+0x4c/0x78)
    [<c00bb4a4>] (SyS_write+0x0/0x78) from [<c0009480>]
(ret_fast_syscall+0x0/0x2c)
     r8:c00095e4 r7:00000004 r6:bedcfaf0 r5:00000006 r4:0000000b
    Code: e89da800 e1a0c00d e92dd830 e24cb004 (e5903000)
    ---[ end trace a412cb5cfd5edab9 ]---
    note: cpufreq-bench[2427] exited with preempt_count 1

    This happened because we used rwsem of new policy structure instead of the
    existing one. And that one was never initialized.

    Reported-by: Andrew Lunn <andrew@lunn.ch>
    Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/cpufreq/cpufreq.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index 8a3914b..5c9be08 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -1902,10 +1902,10 @@ static int cpufreq_set_policy(struct
cpufreq_policy *policy,
                        /* end old governor */
                        if (policy->governor) {
                                __cpufreq_governor(policy, CPUFREQ_GOV_STOP);
-                               up_write(&new_policy->rwsem);
+                               up_write(&policy->rwsem);
                                __cpufreq_governor(policy,
                                                CPUFREQ_GOV_POLICY_EXIT);
-                               down_write(&new_policy->rwsem);
+                               down_write(&policy->rwsem);
                        }

                        /* start new governor */
@@ -1914,10 +1914,10 @@ static int cpufreq_set_policy(struct
cpufreq_policy *policy,
                                if (!__cpufreq_governor(policy,
CPUFREQ_GOV_START)) {
                                        failed = 0;
                                } else {
-                                       up_write(&new_policy->rwsem);
+                                       up_write(&policy->rwsem);
                                        __cpufreq_governor(policy,

CPUFREQ_GOV_POLICY_EXIT);
-                                       down_write(&new_policy->rwsem);
+                                       down_write(&policy->rwsem);
                                }
                        }

[-- Attachment #2: 0001-cpufreq-Use-correct-rwsem-in-cpufreq_set_policy.patch --]
[-- Type: text/x-patch, Size: 3823 bytes --]

From 0c5d6493fe02606512e94fad3ea075bc4f56b7e7 Mon Sep 17 00:00:00 2001
Message-Id: <0c5d6493fe02606512e94fad3ea075bc4f56b7e7.1381896884.git.viresh.kumar@linaro.org>
From: Viresh Kumar <viresh.kumar@linaro.org>
Date: Wed, 16 Oct 2013 09:39:18 +0530
Subject: [PATCH] cpufreq: Use correct rwsem in cpufreq_set_policy()

Recent commit "cpufreq: create per policy rwsem instead of per CPU
cpu_policy_rwsem" introduced a bug where kernel crashes when we run
cpufreq-bench. Crash looks like this:

Unable to handle kernel NULL pointer dereference at virtual address 00000000
pgd = cfa60000
[00000000] *pgd=0d7c9831, *pte=00000000, *ppte=00000000
Internal error: Oops: 17 [#1] PREEMPT ARM
Modules linked in:
CPU: 0 PID: 2427 Comm: cpufreq-bench Not tainted 3.12.0-rc4-00708-g1546af5 #86
task: cfa98e60 ti: ce58a000 task.ti: ce58a000
PC is at wake_up_process+0xc/0x48
LR is at __up_write+0x158/0x164
pc : [<c0040a14>]    lr : [<c0209630>]    psr: 60000093
sp : ce58bd90  ip : ce58bda8  fp : ce58bda4
r10: cfb3aa28  r9 : ce58bea8  r8 : ce58beb8
r7 : ce58beac  r6 : 00000000  r5 : cfb3a9c0  r4 : ce58be10
r3 : cfb3aa5c  r2 : cfb3aa5c  r1 : 00000000  r0 : 00000000
Flags: nZCv  IRQs off  FIQs on  Mode SVC_32  ISA ARM  Segment user
Control: 0005397f  Table: 0fa60000  DAC: 00000015
Process cpufreq-bench (pid: 2427, stack limit = 0xce58a1c0)
Stack: (0xce58bd90 to 0xce58c000)

<snip>

Backtrace:
[<c0040a08>] (wake_up_process+0x0/0x48) from [<c0209630>] (__up_write+0x158/0x164)
 r5:cfb3a9c0 r4:ce58be10
[<c02094d8>] (__up_write+0x0/0x164) from [<c0039fe8>] (up_write+0x10/0x14)
[<c0039fd8>] (up_write+0x0/0x14) from [<c03578a0>] (cpufreq_set_policy+0x120/0x1cc)
[<c0357780>] (cpufreq_set_policy+0x0/0x1cc) from [<c0358efc>] (store_scaling_governor+0xac/0x1a4)
 r7:0000000b r6:00000000 r5:ce58be10 r4:cfb3a9c0
[<c0358e50>] (store_scaling_governor+0x0/0x1a4) from [<c03576e8>] (store+0x88/0xa4)
 r8:c04d6e3c r7:cfae6a80 r6:ce58bf78 r5:cfb3a9c0 r4:cfb3aa58
[<c0357660>] (store+0x0/0xa4) from [<c011a078>] (sysfs_write_file+0x108/0x188)
 r5:cfa12f58 r4:cfa12f40
[<c0119f70>] (sysfs_write_file+0x0/0x188) from [<c00bb340>] (vfs_write+0xcc/0x198)
[<c00bb274>] (vfs_write+0x0/0x198) from [<c00bb4f0>] (SyS_write+0x4c/0x78)
[<c00bb4a4>] (SyS_write+0x0/0x78) from [<c0009480>] (ret_fast_syscall+0x0/0x2c)
 r8:c00095e4 r7:00000004 r6:bedcfaf0 r5:00000006 r4:0000000b
Code: e89da800 e1a0c00d e92dd830 e24cb004 (e5903000)
---[ end trace a412cb5cfd5edab9 ]---
note: cpufreq-bench[2427] exited with preempt_count 1

This happened because we used rwsem of new policy structure instead of the
existing one. And that one was never initialized.

Reported-by: Andrew Lunn <andrew@lunn.ch>
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/cpufreq/cpufreq.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index 8a3914b..5c9be08 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -1902,10 +1902,10 @@ static int cpufreq_set_policy(struct cpufreq_policy *policy,
 			/* end old governor */
 			if (policy->governor) {
 				__cpufreq_governor(policy, CPUFREQ_GOV_STOP);
-				up_write(&new_policy->rwsem);
+				up_write(&policy->rwsem);
 				__cpufreq_governor(policy,
 						CPUFREQ_GOV_POLICY_EXIT);
-				down_write(&new_policy->rwsem);
+				down_write(&policy->rwsem);
 			}
 
 			/* start new governor */
@@ -1914,10 +1914,10 @@ static int cpufreq_set_policy(struct cpufreq_policy *policy,
 				if (!__cpufreq_governor(policy, CPUFREQ_GOV_START)) {
 					failed = 0;
 				} else {
-					up_write(&new_policy->rwsem);
+					up_write(&policy->rwsem);
 					__cpufreq_governor(policy,
 							CPUFREQ_GOV_POLICY_EXIT);
-					down_write(&new_policy->rwsem);
+					down_write(&policy->rwsem);
 				}
 			}
 
-- 
1.7.12.rc2.18.g61b472e

Re: cpufreq-next opps with cpufreq-bench

[Srivatsa S. Bhat]

On 10/16/2013 09:49 AM, Viresh Kumar wrote:
> On 16/10/2013, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
>> Well, I've dropped the offending commit from my queue.  Hopefully, it still
>> builds on all targets it used to build on.
> 
> Thanks!!
> 
> I tried to reproduce it 2 days back and my laptop was almost completely
> discharged and I have left my charger at a friends place last week :(
> So couldn't really do anything until then.. Got my charger today and so
> coming back on this..
> 
> I couldn't run and find cpufreq-bench on ubuntu (Is it available?)..
> But I think I have found the problem..
> 
> @Andrew: Can you give this a try please? (Use attached commit)..
> 
> commit 0c5d6493fe02606512e94fad3ea075bc4f56b7e7
> Author: Viresh Kumar <viresh.kumar@linaro.org>
> Date:   Wed Oct 16 09:39:18 2013 +0530
> 
>     cpufreq: Use correct rwsem in cpufreq_set_policy()
> 
>     Recent commit "cpufreq: create per policy rwsem instead of per CPU
>     cpu_policy_rwsem" introduced a bug where kernel crashes when we run
>     cpufreq-bench. Crash looks like this:
> 
>     Unable to handle kernel NULL pointer dereference at virtual address 00000000
>     pgd = cfa60000
>     [00000000] *pgd=0d7c9831, *pte=00000000, *ppte=00000000
>     Internal error: Oops: 17 [#1] PREEMPT ARM
>     Modules linked in:
>     CPU: 0 PID: 2427 Comm: cpufreq-bench Not tainted
> 3.12.0-rc4-00708-g1546af5 #86
>     task: cfa98e60 ti: ce58a000 task.ti: ce58a000
>     PC is at wake_up_process+0xc/0x48
>     LR is at __up_write+0x158/0x164
>     pc : [<c0040a14>]    lr : [<c0209630>]    psr: 60000093
>     sp : ce58bd90  ip : ce58bda8  fp : ce58bda4
>     r10: cfb3aa28  r9 : ce58bea8  r8 : ce58beb8
>     r7 : ce58beac  r6 : 00000000  r5 : cfb3a9c0  r4 : ce58be10
>     r3 : cfb3aa5c  r2 : cfb3aa5c  r1 : 00000000  r0 : 00000000
>     Flags: nZCv  IRQs off  FIQs on  Mode SVC_32  ISA ARM  Segment user
>     Control: 0005397f  Table: 0fa60000  DAC: 00000015
>     Process cpufreq-bench (pid: 2427, stack limit = 0xce58a1c0)
>     Stack: (0xce58bd90 to 0xce58c000)
> 
>     <snip>
> 
>     Backtrace:
>     [<c0040a08>] (wake_up_process+0x0/0x48) from [<c0209630>]
> (__up_write+0x158/0x164)
>      r5:cfb3a9c0 r4:ce58be10
>     [<c02094d8>] (__up_write+0x0/0x164) from [<c0039fe8>] (up_write+0x10/0x14)
>     [<c0039fd8>] (up_write+0x0/0x14) from [<c03578a0>]
> (cpufreq_set_policy+0x120/0x1cc)
>     [<c0357780>] (cpufreq_set_policy+0x0/0x1cc) from [<c0358efc>]
> (store_scaling_governor+0xac/0x1a4)
>      r7:0000000b r6:00000000 r5:ce58be10 r4:cfb3a9c0
>     [<c0358e50>] (store_scaling_governor+0x0/0x1a4) from [<c03576e8>]
> (store+0x88/0xa4)
>      r8:c04d6e3c r7:cfae6a80 r6:ce58bf78 r5:cfb3a9c0 r4:cfb3aa58
>     [<c0357660>] (store+0x0/0xa4) from [<c011a078>]
> (sysfs_write_file+0x108/0x188)
>      r5:cfa12f58 r4:cfa12f40
>     [<c0119f70>] (sysfs_write_file+0x0/0x188) from [<c00bb340>]
> (vfs_write+0xcc/0x198)
>     [<c00bb274>] (vfs_write+0x0/0x198) from [<c00bb4f0>] (SyS_write+0x4c/0x78)
>     [<c00bb4a4>] (SyS_write+0x0/0x78) from [<c0009480>]
> (ret_fast_syscall+0x0/0x2c)
>      r8:c00095e4 r7:00000004 r6:bedcfaf0 r5:00000006 r4:0000000b
>     Code: e89da800 e1a0c00d e92dd830 e24cb004 (e5903000)
>     ---[ end trace a412cb5cfd5edab9 ]---
>     note: cpufreq-bench[2427] exited with preempt_count 1
> 
>     This happened because we used rwsem of new policy structure instead of the
>     existing one. And that one was never initialized.
>

Hmm, so the problem is that we lock one policy structure in store() whereas we
unlock a totally different one in __cpufreq_set_policy(). I would have expected
lockdep to complain about this before we hit the NULL pointer dereference.
Andrew, can you enable lockdep on your system and confirm this please, just to
be sure?

And in the previous locking design, since the policy structure was per-cpu,
it wouldn't cause this problem because, as long as the cpu parameter passed
was the same, both the lock and unlock operations would happen on the same
policy structure.

The fix looks good to me.

Regards,
Srivatsa S. Bhat

 
>     Reported-by: Andrew Lunn <andrew@lunn.ch>
>     Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---
>  drivers/cpufreq/cpufreq.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> index 8a3914b..5c9be08 100644
> --- a/drivers/cpufreq/cpufreq.c
> +++ b/drivers/cpufreq/cpufreq.c
> @@ -1902,10 +1902,10 @@ static int cpufreq_set_policy(struct
> cpufreq_policy *policy,
>                         /* end old governor */
>                         if (policy->governor) {
>                                 __cpufreq_governor(policy, CPUFREQ_GOV_STOP);
> -                               up_write(&new_policy->rwsem);
> +                               up_write(&policy->rwsem);
>                                 __cpufreq_governor(policy,
>                                                 CPUFREQ_GOV_POLICY_EXIT);
> -                               down_write(&new_policy->rwsem);
> +                               down_write(&policy->rwsem);
>                         }
> 
>                         /* start new governor */
> @@ -1914,10 +1914,10 @@ static int cpufreq_set_policy(struct
> cpufreq_policy *policy,
>                                 if (!__cpufreq_governor(policy,
> CPUFREQ_GOV_START)) {
>                                         failed = 0;
>                                 } else {
> -                                       up_write(&new_policy->rwsem);
> +                                       up_write(&policy->rwsem);
>                                         __cpufreq_governor(policy,
> 
> CPUFREQ_GOV_POLICY_EXIT);
> -                                       down_write(&new_policy->rwsem);
> +                                       down_write(&policy->rwsem);
>                                 }
>                         }
> 

Re: cpufreq-next opps with cpufreq-bench

[Rafael J. Wysocki]

On Wednesday, October 16, 2013 09:49:02 AM Viresh Kumar wrote:
> 
> --047d7b6d91a050552f04e8d40024
> Content-Type: text/plain; charset=ISO-8859-1
> 
> On 16/10/2013, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> > Well, I've dropped the offending commit from my queue.  Hopefully, it still
> > builds on all targets it used to build on.
> 
> Thanks!!
> 
> I tried to reproduce it 2 days back and my laptop was almost completely
> discharged and I have left my charger at a friends place last week :(
> So couldn't really do anything until then.. Got my charger today and so
> coming back on this..
> 
> I couldn't run and find cpufreq-bench on ubuntu (Is it available?)..
> But I think I have found the problem..
> 
> @Andrew: Can you give this a try please? (Use attached commit)..

No.

Please resumbit the original commit *with* the fix folded into it.

Since I have dropped the broken one from linux-next, this "fixup" doesn't
apply to it any more.

Thanks!

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

Re: cpufreq-next opps with cpufreq-bench

[Viresh Kumar]

[-- Attachment #1: Type: text/plain, Size: 815 bytes --]

On 16/10/2013, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> On Wednesday, October 16, 2013 09:49:02 AM Viresh Kumar wrote:

>> @Andrew: Can you give this a try please? (Use attached commit)..
>
> No.
>
> Please resumbit the original commit *with* the fix folded into it.
>
> Since I have dropped the broken one from linux-next, this "fixup" doesn't
> apply to it any more.

I thought of adding following lines to my last email and then dropped it
for some reason:

"After you have tested it, I will resend a single patch for both these for
Rafael to include as he has already dropped this offending commit"

:)

I have merged both the commits now and rebased over your latest
linux-next..

@Andrew: Can you please test this now?

I will then resend it formally to the list again with your Tested-by :)

--
viresh

[-- Attachment #2: 0001-cpufreq-create-per-policy-rwsem-instead-of-per-CPU-c.patch --]
[-- Type: text/x-patch, Size: 10332 bytes --]

From 80a9e5a13bd5e2cca793e4acd4981db6d4ca4bd5 Mon Sep 17 00:00:00 2001
Message-Id: <80a9e5a13bd5e2cca793e4acd4981db6d4ca4bd5.1381986049.git.viresh.kumar@linaro.org>
From: Viresh Kumar <viresh.kumar@linaro.org>
Date: Wed, 2 Oct 2013 14:13:10 +0530
Subject: [PATCH] cpufreq: create per policy rwsem instead of per CPU
 cpu_policy_rwsem

We have per-CPU cpu_policy_rwsem for cpufreq core, but we never use
all of them. We always use rwsem of policy->cpu and so we can
actually make this rwsem per policy instead.

This patch does this change. With this change other tricky situations
are also avoided now, like which lock to take while we are changing
policy->cpu, etc.

Suggested-by: Srivatsa S. Bhat <srivatsa.bhat@linux.vnet.ibm.com>
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
Reviewed-by: Srivatsa S. Bhat <srivatsa.bhat@linux.vnet.ibm.com>
Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
---
 drivers/cpufreq/cpufreq.c | 110 +++++++++++++---------------------------------
 include/linux/cpufreq.h   |  14 ++++++
 2 files changed, 45 insertions(+), 79 deletions(-)

diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index f0897c7..5c9be08 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -48,47 +48,6 @@ static DEFINE_PER_CPU(char[CPUFREQ_NAME_LEN], cpufreq_cpu_governor);
 #endif
 
 /*
- * cpu_policy_rwsem is a per CPU reader-writer semaphore designed to cure
- * all cpufreq/hotplug/workqueue/etc related lock issues.
- *
- * The rules for this semaphore:
- * - Any routine that wants to read from the policy structure will
- *   do a down_read on this semaphore.
- * - Any routine that will write to the policy structure and/or may take away
- *   the policy altogether (eg. CPU hotplug), will hold this lock in write
- *   mode before doing so.
- *
- * Additional rules:
- * - Governor routines that can be called in cpufreq hotplug path should not
- *   take this sem as top level hotplug notifier handler takes this.
- * - Lock should not be held across
- *     __cpufreq_governor(data, CPUFREQ_GOV_STOP);
- */
-static DEFINE_PER_CPU(struct rw_semaphore, cpu_policy_rwsem);
-
-#define lock_policy_rwsem(mode, cpu)					\
-static void lock_policy_rwsem_##mode(int cpu)				\
-{									\
-	struct cpufreq_policy *policy = per_cpu(cpufreq_cpu_data, cpu);	\
-	BUG_ON(!policy);						\
-	down_##mode(&per_cpu(cpu_policy_rwsem, policy->cpu));		\
-}
-
-lock_policy_rwsem(read, cpu);
-lock_policy_rwsem(write, cpu);
-
-#define unlock_policy_rwsem(mode, cpu)					\
-static void unlock_policy_rwsem_##mode(int cpu)				\
-{									\
-	struct cpufreq_policy *policy = per_cpu(cpufreq_cpu_data, cpu);	\
-	BUG_ON(!policy);						\
-	up_##mode(&per_cpu(cpu_policy_rwsem, policy->cpu));		\
-}
-
-unlock_policy_rwsem(read, cpu);
-unlock_policy_rwsem(write, cpu);
-
-/*
  * rwsem to guarantee that cpufreq driver module doesn't unload during critical
  * sections
  */
@@ -683,14 +642,14 @@ static ssize_t show(struct kobject *kobj, struct attribute *attr, char *buf)
 	if (!down_read_trylock(&cpufreq_rwsem))
 		return -EINVAL;
 
-	lock_policy_rwsem_read(policy->cpu);
+	down_read(&policy->rwsem);
 
 	if (fattr->show)
 		ret = fattr->show(policy, buf);
 	else
 		ret = -EIO;
 
-	unlock_policy_rwsem_read(policy->cpu);
+	up_read(&policy->rwsem);
 	up_read(&cpufreq_rwsem);
 
 	return ret;
@@ -711,14 +670,14 @@ static ssize_t store(struct kobject *kobj, struct attribute *attr,
 	if (!down_read_trylock(&cpufreq_rwsem))
 		goto unlock;
 
-	lock_policy_rwsem_write(policy->cpu);
+	down_write(&policy->rwsem);
 
 	if (fattr->store)
 		ret = fattr->store(policy, buf, count);
 	else
 		ret = -EIO;
 
-	unlock_policy_rwsem_write(policy->cpu);
+	up_write(&policy->rwsem);
 
 	up_read(&cpufreq_rwsem);
 unlock:
@@ -895,7 +854,7 @@ static int cpufreq_add_policy_cpu(struct cpufreq_policy *policy,
 		}
 	}
 
-	lock_policy_rwsem_write(policy->cpu);
+	down_write(&policy->rwsem);
 
 	write_lock_irqsave(&cpufreq_driver_lock, flags);
 
@@ -903,7 +862,7 @@ static int cpufreq_add_policy_cpu(struct cpufreq_policy *policy,
 	per_cpu(cpufreq_cpu_data, cpu) = policy;
 	write_unlock_irqrestore(&cpufreq_driver_lock, flags);
 
-	unlock_policy_rwsem_write(policy->cpu);
+	up_write(&policy->rwsem);
 
 	if (has_target) {
 		if ((ret = __cpufreq_governor(policy, CPUFREQ_GOV_START)) ||
@@ -950,6 +909,8 @@ static struct cpufreq_policy *cpufreq_policy_alloc(void)
 		goto err_free_cpumask;
 
 	INIT_LIST_HEAD(&policy->policy_list);
+	init_rwsem(&policy->rwsem);
+
 	return policy;
 
 err_free_cpumask:
@@ -972,19 +933,12 @@ static void update_policy_cpu(struct cpufreq_policy *policy, unsigned int cpu)
 	if (cpu == policy->cpu)
 		return;
 
-	/*
-	 * Take direct locks as lock_policy_rwsem_write wouldn't work here.
-	 * Also lock for last cpu is enough here as contention will happen only
-	 * after policy->cpu is changed and after it is changed, other threads
-	 * will try to acquire lock for new cpu. And policy is already updated
-	 * by then.
-	 */
-	down_write(&per_cpu(cpu_policy_rwsem, policy->cpu));
+	down_write(&policy->rwsem);
 
 	policy->last_cpu = policy->cpu;
 	policy->cpu = cpu;
 
-	up_write(&per_cpu(cpu_policy_rwsem, policy->last_cpu));
+	up_write(&policy->rwsem);
 
 	cpufreq_frequency_table_update_policy_cpu(policy);
 	blocking_notifier_call_chain(&cpufreq_policy_notifier_list,
@@ -1176,9 +1130,9 @@ static int cpufreq_nominate_new_policy_cpu(struct cpufreq_policy *policy,
 	if (ret) {
 		pr_err("%s: Failed to move kobj: %d", __func__, ret);
 
-		lock_policy_rwsem_write(old_cpu);
+		down_write(&policy->rwsem);
 		cpumask_set_cpu(old_cpu, policy->cpus);
-		unlock_policy_rwsem_write(old_cpu);
+		up_write(&policy->rwsem);
 
 		ret = sysfs_create_link(&cpu_dev->kobj, &policy->kobj,
 					"cpufreq");
@@ -1229,9 +1183,9 @@ static int __cpufreq_remove_dev_prepare(struct device *dev,
 			policy->governor->name, CPUFREQ_NAME_LEN);
 #endif
 
-	lock_policy_rwsem_read(cpu);
+	down_read(&policy->rwsem);
 	cpus = cpumask_weight(policy->cpus);
-	unlock_policy_rwsem_read(cpu);
+	up_read(&policy->rwsem);
 
 	if (cpu != policy->cpu) {
 		if (!frozen)
@@ -1271,12 +1225,12 @@ static int __cpufreq_remove_dev_finish(struct device *dev,
 		return -EINVAL;
 	}
 
-	lock_policy_rwsem_write(cpu);
+	down_write(&policy->rwsem);
 	cpus = cpumask_weight(policy->cpus);
 
 	if (cpus > 1)
 		cpumask_clear_cpu(cpu, policy->cpus);
-	unlock_policy_rwsem_write(cpu);
+	up_write(&policy->rwsem);
 
 	/* If cpu is last user of policy, free policy */
 	if (cpus == 1) {
@@ -1291,10 +1245,10 @@ static int __cpufreq_remove_dev_finish(struct device *dev,
 		}
 
 		if (!frozen) {
-			lock_policy_rwsem_read(cpu);
+			down_read(&policy->rwsem);
 			kobj = &policy->kobj;
 			cmp = &policy->kobj_unregister;
-			unlock_policy_rwsem_read(cpu);
+			up_read(&policy->rwsem);
 			kobject_put(kobj);
 
 			/*
@@ -1474,19 +1428,22 @@ static unsigned int __cpufreq_get(unsigned int cpu)
  */
 unsigned int cpufreq_get(unsigned int cpu)
 {
+	struct cpufreq_policy *policy = per_cpu(cpufreq_cpu_data, cpu);
 	unsigned int ret_freq = 0;
 
 	if (cpufreq_disabled() || !cpufreq_driver)
 		return -ENOENT;
 
+	BUG_ON(!policy);
+
 	if (!down_read_trylock(&cpufreq_rwsem))
 		return 0;
 
-	lock_policy_rwsem_read(cpu);
+	down_read(&policy->rwsem);
 
 	ret_freq = __cpufreq_get(cpu);
 
-	unlock_policy_rwsem_read(cpu);
+	up_read(&policy->rwsem);
 	up_read(&cpufreq_rwsem);
 
 	return ret_freq;
@@ -1710,11 +1667,11 @@ int cpufreq_driver_target(struct cpufreq_policy *policy,
 {
 	int ret = -EINVAL;
 
-	lock_policy_rwsem_write(policy->cpu);
+	down_write(&policy->rwsem);
 
 	ret = __cpufreq_driver_target(policy, target_freq, relation);
 
-	unlock_policy_rwsem_write(policy->cpu);
+	up_write(&policy->rwsem);
 
 	return ret;
 }
@@ -1945,10 +1902,10 @@ static int cpufreq_set_policy(struct cpufreq_policy *policy,
 			/* end old governor */
 			if (policy->governor) {
 				__cpufreq_governor(policy, CPUFREQ_GOV_STOP);
-				unlock_policy_rwsem_write(new_policy->cpu);
+				up_write(&policy->rwsem);
 				__cpufreq_governor(policy,
 						CPUFREQ_GOV_POLICY_EXIT);
-				lock_policy_rwsem_write(new_policy->cpu);
+				down_write(&policy->rwsem);
 			}
 
 			/* start new governor */
@@ -1957,10 +1914,10 @@ static int cpufreq_set_policy(struct cpufreq_policy *policy,
 				if (!__cpufreq_governor(policy, CPUFREQ_GOV_START)) {
 					failed = 0;
 				} else {
-					unlock_policy_rwsem_write(new_policy->cpu);
+					up_write(&policy->rwsem);
 					__cpufreq_governor(policy,
 							CPUFREQ_GOV_POLICY_EXIT);
-					lock_policy_rwsem_write(new_policy->cpu);
+					down_write(&policy->rwsem);
 				}
 			}
 
@@ -2006,7 +1963,7 @@ int cpufreq_update_policy(unsigned int cpu)
 		goto no_policy;
 	}
 
-	lock_policy_rwsem_write(cpu);
+	down_write(&policy->rwsem);
 
 	pr_debug("updating policy for CPU %u\n", cpu);
 	memcpy(&new_policy, policy, sizeof(*policy));
@@ -2033,7 +1990,7 @@ int cpufreq_update_policy(unsigned int cpu)
 
 	ret = cpufreq_set_policy(policy, &new_policy);
 
-	unlock_policy_rwsem_write(cpu);
+	up_write(&policy->rwsem);
 
 	cpufreq_cpu_put(policy);
 no_policy:
@@ -2190,14 +2147,9 @@ EXPORT_SYMBOL_GPL(cpufreq_unregister_driver);
 
 static int __init cpufreq_core_init(void)
 {
-	int cpu;
-
 	if (cpufreq_disabled())
 		return -ENODEV;
 
-	for_each_possible_cpu(cpu)
-		init_rwsem(&per_cpu(cpu_policy_rwsem, cpu));
-
 	cpufreq_global_kobject = kobject_create();
 	BUG_ON(!cpufreq_global_kobject);
 	register_syscore_ops(&cpufreq_syscore_ops);
diff --git a/include/linux/cpufreq.h b/include/linux/cpufreq.h
index 0aba2a6c..6b457d0 100644
--- a/include/linux/cpufreq.h
+++ b/include/linux/cpufreq.h
@@ -85,6 +85,20 @@ struct cpufreq_policy {
 	struct list_head        policy_list;
 	struct kobject		kobj;
 	struct completion	kobj_unregister;
+
+	/*
+	 * The rules for this semaphore:
+	 * - Any routine that wants to read from the policy structure will
+	 *   do a down_read on this semaphore.
+	 * - Any routine that will write to the policy structure and/or may take away
+	 *   the policy altogether (eg. CPU hotplug), will hold this lock in write
+	 *   mode before doing so.
+	 *
+	 * Additional rules:
+	 * - Lock should not be held across
+	 *     __cpufreq_governor(data, CPUFREQ_GOV_POLICY_EXIT);
+	 */
+	struct rw_semaphore	rwsem;
 };
 
 /* Only for ACPI */
-- 
1.7.12.rc2.18.g61b472e

Re: cpufreq-next opps with cpufreq-bench

[Rafael J. Wysocki]

On Thursday, October 17, 2013 10:34:03 AM Viresh Kumar wrote:
> On 16/10/2013, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> > On Wednesday, October 16, 2013 09:49:02 AM Viresh Kumar wrote:
> 
> >> @Andrew: Can you give this a try please? (Use attached commit)..
> >
> > No.
> >
> > Please resumbit the original commit *with* the fix folded into it.
> >
> > Since I have dropped the broken one from linux-next, this "fixup" doesn't
> > apply to it any more.
> 
> I thought of adding following lines to my last email and then dropped it
> for some reason:
> 
> "After you have tested it, I will resend a single patch for both these for
> Rafael to include as he has already dropped this offending commit"
> 
> :)

That really isn't helpful, because even if the fixup is tested, you can still
make a mistake when folding it into the original patch (while I believe that
that is unlikely, things like that have happened in the kernel history).  So,
it is always better to test the final patch altogether.

Thanks!

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