Regression on cpufreq in v3.12-rc1

Regression on cpufreq in v3.12-rc1

[Linus Walleij]

Hi Rafael, Viresh,

I'm seeing this problem and maybe you can help me out fixing it
properly:

On some machines like the StrongARM SA1100 it seems that
cpufreq_get() can be called before the cpufreq driver and thus the
policy is set, resulting in a crash like this:

\x01------------[ cut here ]------------
\x01kernel BUG at /home/linus/linux/drivers/cpufreq/cpufreq.c:80!
\x01Internal error: Oops - BUG: 0 [#1] ARM
\x01Modules linked in:
\x01CPU: 0 PID: 1 Comm: swapper Not tainted 3.12.0-rc1-00001-g1266dae-dirty #17
\x01task: c1830000 ti: c1832000 task.ti: c1832000
(...)
Backtrace:
[<c01ea1a4>] (lock_policy_rwsem_read+0x0/0x48) from [<c01eb5c8>]
(cpufreq_get+0x34/0x68)
[<c01eb594>] (cpufreq_get+0x0/0x68) from [<c0185908>]
(sa1100fb_activate_var+0xdc/0x3ac)
 r5:00000003 r4:0000000a
[<c018582c>] (sa1100fb_activate_var+0x0/0x3ac) from [<c0185c78>]
(sa1100fb_set_par+0xa0/0xa8)
[<c0185bd8>] (sa1100fb_set_par+0x0/0xa8) from [<c0180418>]
(fbcon_init+0x444/0x4a8)
 r4:c1803200
[<c017ffd4>] (fbcon_init+0x0/0x4a8) from [<c019a8b4>] (visual_init+0x78/0xc8)
[<c019a83c>] (visual_init+0x0/0xc8) from [<c01a0010>]
(do_bind_con_driver+0x160/0x310)

This bug comes from the framebuffer but I first encountered it
in the PCMCIA driver, and both seem to cause the bug.

In the past I think things worked smoothly: the consumers
calling cpufreq_get() too early would just get 0 back.
(Or so it seems to me.)

The BUG() statement causing it is in the lock_policy_rwsem_##mode(int cpu)
macro.

Applying a patch like this seems to fix the issue:

diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index 43c24aa..4977b4b 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -70,7 +70,8 @@ static DEFINE_PER_CPU(struct rw_semaphore, cpu_policy_rwsem);
 static int lock_policy_rwsem_##mode(int cpu)                           \
 {                                                                      \
        struct cpufreq_policy *policy = per_cpu(cpufreq_cpu_data, cpu); \
-       BUG_ON(!policy);                                                \
+       if(!policy)                                                     \
+               return 0;                                               \
        down_##mode(&per_cpu(cpu_policy_rwsem, policy->cpu));           \
                                                                        \
        return 0;                                                       \
@@ -83,7 +84,8 @@ lock_policy_rwsem(write, cpu);
 static void unlock_policy_rwsem_##mode(int cpu)
         \
 {                                                                      \
        struct cpufreq_policy *policy = per_cpu(cpufreq_cpu_data, cpu); \
-       BUG_ON(!policy);                                                \
+       if(!policy)                                                     \
+               return;                                                 \
        up_##mode(&per_cpu(cpu_policy_rwsem, policy->cpu));             \
 }

@@ -1423,6 +1425,9 @@ static unsigned int __cpufreq_get(unsigned int cpu)
        struct cpufreq_policy *policy = per_cpu(cpufreq_cpu_data, cpu);
        unsigned int ret_freq = 0;

+       if (!policy)
+               return ret_freq;
+
        if (!cpufreq_driver->get)
                return ret_freq;

I don't really know if this is the right solution at all, so please
help me out here... if you want that patch I can send it once
I understand this properly.

Yours,
Linus Walleij

Re: Regression on cpufreq in v3.12-rc1

[Rafael J. Wysocki]

On Wednesday, September 18, 2013 11:21:45 PM Linus Walleij wrote:
> Hi Rafael, Viresh,
> 
> I'm seeing this problem and maybe you can help me out fixing it
> properly:
> 
> On some machines like the StrongARM SA1100 it seems that
> cpufreq_get() can be called before the cpufreq driver and thus the
> policy is set, resulting in a crash like this:

Did you try the current linux-next branch from my tree?

Rafael


> \x01------------[ cut here ]------------
> \x01kernel BUG at /home/linus/linux/drivers/cpufreq/cpufreq.c:80!
> \x01Internal error: Oops - BUG: 0 [#1] ARM
> \x01Modules linked in:
> \x01CPU: 0 PID: 1 Comm: swapper Not tainted 3.12.0-rc1-00001-g1266dae-dirty #17
> \x01task: c1830000 ti: c1832000 task.ti: c1832000
> (...)
> Backtrace:
> [<c01ea1a4>] (lock_policy_rwsem_read+0x0/0x48) from [<c01eb5c8>]
> (cpufreq_get+0x34/0x68)
> [<c01eb594>] (cpufreq_get+0x0/0x68) from [<c0185908>]
> (sa1100fb_activate_var+0xdc/0x3ac)
>  r5:00000003 r4:0000000a
> [<c018582c>] (sa1100fb_activate_var+0x0/0x3ac) from [<c0185c78>]
> (sa1100fb_set_par+0xa0/0xa8)
> [<c0185bd8>] (sa1100fb_set_par+0x0/0xa8) from [<c0180418>]
> (fbcon_init+0x444/0x4a8)
>  r4:c1803200
> [<c017ffd4>] (fbcon_init+0x0/0x4a8) from [<c019a8b4>] (visual_init+0x78/0xc8)
> [<c019a83c>] (visual_init+0x0/0xc8) from [<c01a0010>]
> (do_bind_con_driver+0x160/0x310)
> 
> This bug comes from the framebuffer but I first encountered it
> in the PCMCIA driver, and both seem to cause the bug.
> 
> In the past I think things worked smoothly: the consumers
> calling cpufreq_get() too early would just get 0 back.
> (Or so it seems to me.)
> 
> The BUG() statement causing it is in the lock_policy_rwsem_##mode(int cpu)
> macro.
> 
> Applying a patch like this seems to fix the issue:
> 
> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> index 43c24aa..4977b4b 100644
> --- a/drivers/cpufreq/cpufreq.c
> +++ b/drivers/cpufreq/cpufreq.c
> @@ -70,7 +70,8 @@ static DEFINE_PER_CPU(struct rw_semaphore, cpu_policy_rwsem);
>  static int lock_policy_rwsem_##mode(int cpu)                           \
>  {                                                                      \
>         struct cpufreq_policy *policy = per_cpu(cpufreq_cpu_data, cpu); \
> -       BUG_ON(!policy);                                                \
> +       if(!policy)                                                     \
> +               return 0;                                               \
>         down_##mode(&per_cpu(cpu_policy_rwsem, policy->cpu));           \
>                                                                         \
>         return 0;                                                       \
> @@ -83,7 +84,8 @@ lock_policy_rwsem(write, cpu);
>  static void unlock_policy_rwsem_##mode(int cpu)
>          \
>  {                                                                      \
>         struct cpufreq_policy *policy = per_cpu(cpufreq_cpu_data, cpu); \
> -       BUG_ON(!policy);                                                \
> +       if(!policy)                                                     \
> +               return;                                                 \
>         up_##mode(&per_cpu(cpu_policy_rwsem, policy->cpu));             \
>  }
> 
> @@ -1423,6 +1425,9 @@ static unsigned int __cpufreq_get(unsigned int cpu)
>         struct cpufreq_policy *policy = per_cpu(cpufreq_cpu_data, cpu);
>         unsigned int ret_freq = 0;
> 
> +       if (!policy)
> +               return ret_freq;
> +
>         if (!cpufreq_driver->get)
>                 return ret_freq;
> 
> I don't really know if this is the right solution at all, so please
> help me out here... if you want that patch I can send it once
> I understand this properly.
> 
> Yours,
> Linus Walleij
-- 
I speak only for myself.
Rafael J. Wysocki, Intel Open Source Technology Center.

Re: Regression on cpufreq in v3.12-rc1

[Linus Walleij]

On Thu, Sep 19, 2013 at 12:41 AM, Rafael J. Wysocki <rjw@rjwysocki.net> wrote:
> On Wednesday, September 18, 2013 11:21:45 PM Linus Walleij wrote:
>> Hi Rafael, Viresh,
>>
>> I'm seeing this problem and maybe you can help me out fixing it
>> properly:
>>
>> On some machines like the StrongARM SA1100 it seems that
>> cpufreq_get() can be called before the cpufreq driver and thus the
>> policy is set, resulting in a crash like this:
>
> Did you try the current linux-next branch from my tree?

Tested it now and the problem remains. :-(

Not that I can see that any of the fixes there would solve this
problem ... I mean it crashes as soon as you take the semaphore.

Switched to clocksource oscr
------------[ cut here ]------------
kernel BUG at /home/linus/linux-pm/drivers/cpufreq/cpufreq.c:79!
Internal error: Oops - BUG: 0 [#1] ARM
Modules linked in:
CPU: 0 PID: 1 Comm: swapper Not tainted 3.12.0-rc1-07601-g701ac96 #1
task: c1830000 ti: c1832000 task.ti: c1832000
PC is at lock_policy_rwsem_read+0x2c/0x3c
LR is at cpufreq_get+0x34/0x68
pc : [<c01df42c>]    lr : [<c01e0ae8>]    psr: 60000053
sp : c1833d00  ip : c1833d10  fp : c1833d0c
r10: c182d274  r9 : c182d290  r8 : c182d258
r7 : c182d2e4  r6 : c182d23c  r5 : 00000000  r4 : 00000000
r3 : c05d279c  r2 : 00000000  r1 : 60000053  r0 : 00000000
Flags: nZCv  IRQs on  FIQs off  Mode SVC_32  ISA ARM  Segment kernel
Control: c000717f  Table: c000717f  DAC: 00000017
Process swapper (pid: 1, stack limit = 0xc18321b0)
Stack: (0xc1833d00 to 0xc1834000)
3d00: c1833d24 c1833d10 c01e0ae8 c01df40c c182d008 00000000 c1833d38 c1833d28
3d20: c01d7050 c01e0ac0 c182d008 c1833d64 c1833d3c c01d6ba8 c01d7044 00000200
3d40: 2c000000 20000000 2fffffff 2bffffff 00000000 00000000 c1833d88 c1833d68
3d60: c01d70ec c01d6ab4 c182d008 00000000 c182d000 c182d000 00000002 c1833dbc
3d80: c1833d8c c01d71a0 c01d706c c05d2350 c05c56bc c05c56bc c05c56bc c05d2310
3da0: c05e4a94 00000000 00000000 00000000 c1833dcc c1833dc0 c01d7540 c01d7110
3dc0: c1833ddc c1833dd0 c01d732c c01d7530 c1833dec c1833de0 c01a2f14 c01d7324
3de0: c1833e10 c1833df0 c01a1970 c01a2f00 c05c56bc c05c56f0 c05d2310 c01a1ca8
3e00: 00000000 c1833e20 c1833e14 c01a1a5c c01a1874 c1833e3c c1833e24 c01a1d30
3e20: c01a1a38 00000000 c1833e40 c05d2310 c1833e64 c1833e40 c01a0104 c01a1cb4
3e40: c1829e8c c184e8d0 c18c1900 c05d2310 c05d0d1c 00000000 c1833e74 c1833e68
3e60: c01a1748 c01a009c c1833e98 c1833e78 c01a0f44 c01a1734 c031b9ac c05d2310
3e80: 00000000 00000006 c0374794 c1833eb0 c1833e9c c01a2288 c01a0e70 c1832000
3ea0: 00000000 c1833ec0 c1833eb4 c01a3208 c01a2234 c1833ed0 c1833ec4 c03747ac
3ec0: c01a31c4 c1833f34 c1833ed4 c00085a4 c03747a0 00000005 00000000 00000005
3ee0: c1833f34 c1833ef0 c0034ea8 c0034994 00000060 00000005 00000005 c035e6c0
3f00: c035bec4 c0002148 c0002140 00000005 00000005 00000006 c037bf28 00000005
3f20: 00000006 c0378d54 c1833f60 c1833f38 c035e590 c0008514 00000005 00000005
3f40: c035e6c0 00000006 c029dcd8 00000000 00000000 c1833f74 c1833f64 c035e5e4
3f60: c035e51c 00000000 c1833f84 c1833f78 c035e6bc c035e5cc c1833f98 c1833f88
3f80: c035ea98 c035e6a0 00000000 c1833fac c1833f9c c029dce8 c035ea58 00000000
3fa0: 00000000 c1833fb0 c000f9b0 c029dce4 00000000 00000000 00000000 00000000
3fc0: 00000000 00000000 00000000 00000000 00000000 00000000 00000000 00000000
3fe0: 00000000 00000000 00000000 00000000 00000013 00000000 7fc33b8e cc8610cc
Backtrace:
[<c01df400>] (lock_policy_rwsem_read+0x0/0x3c) from [<c01e0ae8>]
(cpufreq_get+0x34/0x68)
[<c01e0ab4>] (cpufreq_get+0x0/0x68) from [<c01d7050>]
(sa1100_pcmcia_set_timing+0x18/0x28)
 r5:00000000 r4:c182d008
[<c01d7038>] (sa1100_pcmcia_set_timing+0x0/0x28) from [<c01d6ba8>]
(soc_pcmcia_add_one+0x100/0x220)
 r4:c182d008
[<c01d6aa8>] (soc_pcmcia_add_one+0x0/0x220) from [<c01d70ec>]
(sa11xx_drv_pcmcia_add_one+0x8c/0xa4)
[<c01d7060>] (sa11xx_drv_pcmcia_add_one+0x0/0xa4) from [<c01d71a0>]
(sa11xx_drv_pcmcia_probe+0x9c/0xf8)
 r8:00000002 r7:c182d000 r6:c182d000 r5:00000000 r4:c182d008
[<c01d7104>] (sa11xx_drv_pcmcia_probe+0x0/0xf8) from [<c01d7540>]
(pcmcia_h3600_init+0x1c/0x24)
[<c01d7524>] (pcmcia_h3600_init+0x0/0x24) from [<c01d732c>]
(sa11x0_drv_pcmcia_probe+0x14/0x18)
[<c01d7318>] (sa11x0_drv_pcmcia_probe+0x0/0x18) from [<c01a2f14>]
(platform_drv_probe+0x20/0x24)
[<c01a2ef4>] (platform_drv_probe+0x0/0x24) from [<c01a1970>]
(really_probe+0x108/0x1c4)
[<c01a1868>] (really_probe+0x0/0x1c4) from [<c01a1a5c>]
(driver_probe_device+0x30/0x34)
 r8:00000000 r7:c01a1ca8 r6:c05d2310 r5:c05c56f0 r4:c05c56bc
[<c01a1a2c>] (driver_probe_device+0x0/0x34) from [<c01a1d30>]
(__driver_attach+0x88/0x8c)
[<c01a1ca8>] (__driver_attach+0x0/0x8c) from [<c01a0104>]
(bus_for_each_dev+0x74/0x98)
 r6:c05d2310 r5:c1833e40 r4:00000000
[<c01a0090>] (bus_for_each_dev+0x0/0x98) from [<c01a1748>]
(driver_attach+0x20/0x28)
 r7:00000000 r6:c05d0d1c r5:c05d2310 r4:c18c1900
[<c01a1728>] (driver_attach+0x0/0x28) from [<c01a0f44>]
(bus_add_driver+0xe0/0x1c8)
[<c01a0e64>] (bus_add_driver+0x0/0x1c8) from [<c01a2288>]
(driver_register+0x60/0x104)
 r7:c0374794 r6:00000006 r5:00000000 r4:c05d2310
[<c01a2228>] (driver_register+0x0/0x104) from [<c01a3208>]
(__platform_driver_register+0x50/0x64)
 r5:00000000 r4:c1832000
[<c01a31b8>] (__platform_driver_register+0x0/0x64) from [<c03747ac>]
(sa11x0_pcmcia_init+0x18/0x20)
[<c0374794>] (sa11x0_pcmcia_init+0x0/0x20) from [<c00085a4>]
(do_one_initcall+0x9c/0xfc)
[<c0008508>] (do_one_initcall+0x0/0xfc) from [<c035e590>]
(do_initcall_level+0x80/0xb0)
 r7:c0378d54 r6:00000006 r5:00000005 r4:c037bf28
[<c035e510>] (do_initcall_level+0x0/0xb0) from [<c035e5e4>]
(do_initcalls+0x24/0x30)
 r7:00000000 r6:00000000 r5:c029dcd8 r4:00000006
[<c035e5c0>] (do_initcalls+0x0/0x30) from [<c035e6bc>]
(do_basic_setup+0x28/0x2c)
 r4:00000000
[<c035e694>] (do_basic_setup+0x0/0x2c) from [<c035ea98>]
(kernel_init_freeable+0x4c/0xdc)
[<c035ea4c>] (kernel_init_freeable+0x0/0xdc) from [<c029dce8>]
(kernel_init+0x10/0xf0)
 r4:00000000
[<c029dcd8>] (kernel_init+0x0/0xf0) from [<c000f9b0>] (ret_from_fork+0x14/0x24)
 r4:00000000
Code: e59f0014 eb030036 e3a00000 e89da800 (e7f001f2)
---[ end trace a421e7362f5e31a5 ]---


Yours,
Linus Walleij

Re: Regression on cpufreq in v3.12-rc1

[Srivatsa S. Bhat]

On 09/19/2013 04:11 AM, Rafael J. Wysocki wrote:
> On Wednesday, September 18, 2013 11:21:45 PM Linus Walleij wrote:
>> Hi Rafael, Viresh,
>>
>> I'm seeing this problem and maybe you can help me out fixing it
>> properly:
>>
>> On some machines like the StrongARM SA1100 it seems that
>> cpufreq_get() can be called before the cpufreq driver and thus the
>> policy is set, resulting in a crash like this:
> 
> Did you try the current linux-next branch from my tree?
> 
> Rafael
> 
> 
>> \x01------------[ cut here ]------------
>> \x01kernel BUG at /home/linus/linux/drivers/cpufreq/cpufreq.c:80!
>> \x01Internal error: Oops - BUG: 0 [#1] ARM
>> \x01Modules linked in:
>> \x01CPU: 0 PID: 1 Comm: swapper Not tainted 3.12.0-rc1-00001-g1266dae-dirty #17
>> \x01task: c1830000 ti: c1832000 task.ti: c1832000
>> (...)
>> Backtrace:
>> [<c01ea1a4>] (lock_policy_rwsem_read+0x0/0x48) from [<c01eb5c8>]
>> (cpufreq_get+0x34/0x68)
>> [<c01eb594>] (cpufreq_get+0x0/0x68) from [<c0185908>]
>> (sa1100fb_activate_var+0xdc/0x3ac)
>>  r5:00000003 r4:0000000a
>> [<c018582c>] (sa1100fb_activate_var+0x0/0x3ac) from [<c0185c78>]
>> (sa1100fb_set_par+0xa0/0xa8)
>> [<c0185bd8>] (sa1100fb_set_par+0x0/0xa8) from [<c0180418>]
>> (fbcon_init+0x444/0x4a8)
>>  r4:c1803200
>> [<c017ffd4>] (fbcon_init+0x0/0x4a8) from [<c019a8b4>] (visual_init+0x78/0xc8)
>> [<c019a83c>] (visual_init+0x0/0xc8) from [<c01a0010>]
>> (do_bind_con_driver+0x160/0x310)
>>
>> This bug comes from the framebuffer but I first encountered it
>> in the PCMCIA driver, and both seem to cause the bug.
>>
>> In the past I think things worked smoothly: the consumers
>> calling cpufreq_get() too early would just get 0 back.
>> (Or so it seems to me.)
>>
>> The BUG() statement causing it is in the lock_policy_rwsem_##mode(int cpu)
>> macro.
>>
>> Applying a patch like this seems to fix the issue:
>>
>> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
>> index 43c24aa..4977b4b 100644
>> --- a/drivers/cpufreq/cpufreq.c
>> +++ b/drivers/cpufreq/cpufreq.c
>> @@ -70,7 +70,8 @@ static DEFINE_PER_CPU(struct rw_semaphore, cpu_policy_rwsem);
>>  static int lock_policy_rwsem_##mode(int cpu)                           \
>>  {                                                                      \
>>         struct cpufreq_policy *policy = per_cpu(cpufreq_cpu_data, cpu); \
>> -       BUG_ON(!policy);                                                \
>> +       if(!policy)                                                     \
>> +               return 0;                                               \
>>         down_##mode(&per_cpu(cpu_policy_rwsem, policy->cpu));           \
>>                                                                         \
>>         return 0;                                                       \
>> @@ -83,7 +84,8 @@ lock_policy_rwsem(write, cpu);
>>  static void unlock_policy_rwsem_##mode(int cpu)
>>          \
>>  {                                                                      \
>>         struct cpufreq_policy *policy = per_cpu(cpufreq_cpu_data, cpu); \
>> -       BUG_ON(!policy);                                                \
>> +       if(!policy)                                                     \
>> +               return;                                                 \
>>         up_##mode(&per_cpu(cpu_policy_rwsem, policy->cpu));             \
>>  }
>>
>> @@ -1423,6 +1425,9 @@ static unsigned int __cpufreq_get(unsigned int cpu)
>>         struct cpufreq_policy *policy = per_cpu(cpufreq_cpu_data, cpu);
>>         unsigned int ret_freq = 0;
>>
>> +       if (!policy)
>> +               return ret_freq;
>> +
>>         if (!cpufreq_driver->get)
>>                 return ret_freq;
>>
>> I don't really know if this is the right solution at all, so please
>> help me out here... if you want that patch I can send it once
>> I understand this properly.
>>

IIRC, recent kernels didn't return 0 or any error code when the !policy
condition was matched. So can you check whether this problem occurs with
3.11 or 3.10 as well?

In case the problem is unique to 3.12-rc1, I guess some recent commit changed
the ordering somewhere, which lead to premature invocations of cpufreq_get().
So I think we should first identify (bisect?) and understand what caused that
particular change and then we will be in a position to evaluate whether the
patch you proposed would be the right fix or not.

Regards,
Srivatsa S. Bhat

Re: Regression on cpufreq in v3.12-rc1

[Linus Walleij]

On Thu, Sep 19, 2013 at 2:46 PM, Srivatsa S. Bhat
<srivatsa.bhat@linux.vnet.ibm.com> wrote:

>>> I don't really know if this is the right solution at all, so please
>>> help me out here... if you want that patch I can send it once
>>> I understand this properly.
>
> IIRC, recent kernels didn't return 0 or any error code when the !policy
> condition was matched. So can you check whether this problem occurs with
> 3.11 or 3.10 as well?

v3.11 works fine.

The problem is not what it returns, the system seems to survive no matter
whether it returns 0 or 17 or whatever.

The problem is that sometimes in the v3.12 kernel cycle we got a
BUG() crash instead of some random value back for calling early.

> So I think we should first identify (bisect?) and understand what caused that
> particular change and then we will be in a position to evaluate whether the
> patch you proposed would be the right fix or not.

I'll see if I can get a bisect going, the problem is that I upload the
kernel over the serial port so this isn't a very quick procedure :-(

Yours,
Linus Walleij

Re: Regression on cpufreq in v3.12-rc1

[Srivatsa S. Bhat]

On 09/19/2013 06:25 PM, Linus Walleij wrote:
> On Thu, Sep 19, 2013 at 2:46 PM, Srivatsa S. Bhat
> <srivatsa.bhat@linux.vnet.ibm.com> wrote:
> 
>>>> I don't really know if this is the right solution at all, so please
>>>> help me out here... if you want that patch I can send it once
>>>> I understand this properly.
>>
>> IIRC, recent kernels didn't return 0 or any error code when the !policy
>> condition was matched. So can you check whether this problem occurs with
>> 3.11 or 3.10 as well?
> 
> v3.11 works fine.
> 
> The problem is not what it returns, the system seems to survive no matter
> whether it returns 0 or 17 or whatever.
> 

Of course. What I intended to say was that I don't recall recent kernels
returning _anything_ on !policy. So there wasn't any sudden change in _that_
piece of code, AFAIR.

> The problem is that sometimes in the v3.12 kernel cycle we got a
> BUG() crash instead of some random value back for calling early.
> 

Yep, and that's most likely due to some change in ordering of calls somewhere,
which makes calls to lock_policy_rwsem_read() before it is safe to do so,
rather than anything related to how lock_policy_rwsem_read() handles the call.

>> So I think we should first identify (bisect?) and understand what caused that
>> particular change and then we will be in a position to evaluate whether the
>> patch you proposed would be the right fix or not.
> 
> I'll see if I can get a bisect going, the problem is that I upload the
> kernel over the serial port so this isn't a very quick procedure :-(
> 

Hmmm.. :-/

Regards,
Srivatsa S. Bhat

Re: Regression on cpufreq in v3.12-rc1

[Viresh Kumar]

On 19 September 2013 18:28, Srivatsa S. Bhat
<srivatsa.bhat@linux.vnet.ibm.com> wrote:
> On 09/19/2013 06:25 PM, Linus Walleij wrote:

>> The problem is not what it returns, the system seems to survive no matter
>> whether it returns 0 or 17 or whatever.
>>
>
> Of course. What I intended to say was that I don't recall recent kernels
> returning _anything_ on !policy. So there wasn't any sudden change in _that_
> piece of code, AFAIR.
>
>> The problem is that sometimes in the v3.12 kernel cycle we got a
>> BUG() crash instead of some random value back for calling early.
>>
>
> Yep, and that's most likely due to some change in ordering of calls somewhere,
> which makes calls to lock_policy_rwsem_read() before it is safe to do so,
> rather than anything related to how lock_policy_rwsem_read() handles the call.

I don't see any problem in the order things are called from both the places
where you got the crash from.. Because cpufreq driver is registered with
arch_initcall() and both the crash locations gets initialized after that..

I believe the problem is, cpufreq driver failed to register for some reason
and so we are getting this problem...

Can you please confirm if cpufreq driver failed to register or not?

And, I have another solution in mind for getting this problem fixed,
will get that to you by then..

Re: Regression on cpufreq in v3.12-rc1

[Srivatsa S. Bhat]

On 09/19/2013 06:28 PM, Srivatsa S. Bhat wrote:
> On 09/19/2013 06:25 PM, Linus Walleij wrote:
>> On Thu, Sep 19, 2013 at 2:46 PM, Srivatsa S. Bhat
>> <srivatsa.bhat@linux.vnet.ibm.com> wrote:
>>
>>>>> I don't really know if this is the right solution at all, so please
>>>>> help me out here... if you want that patch I can send it once
>>>>> I understand this properly.
>>>
>>> IIRC, recent kernels didn't return 0 or any error code when the !policy
>>> condition was matched. So can you check whether this problem occurs with
>>> 3.11 or 3.10 as well?
>>
>> v3.11 works fine.
>>

Ok, now that I got a chance to look at the code and the git logs, I think
I have a clearer idea of what is happening.

Basically commit 474deff74 (cpufreq: remove cpufreq_policy_cpu per-cpu
variable) exposed a hidden issue. Before that commit, the code looked like
this:

static DEFINE_PER_CPU(int, cpufreq_policy_cpu);

[...]

static int lock_policy_rwsem_##mode(int cpu)                            \
{                                                                       \
        int policy_cpu = per_cpu(cpufreq_policy_cpu, cpu);              \
        BUG_ON(policy_cpu == -1);                                       \
        down_##mode(&per_cpu(cpu_policy_rwsem, policy_cpu));            \
                                                                        \
        return 0;                                                       \
}

But there was no code to set the per-cpu values to -1 to begin with. Since
the per-cpu variable was defined as static, it would have been initialized
to zero. Thus, we would never actually hit the BUG_ON() condition, since
policy_cpu didn't turn out to be -1.

(The per-cpu variable was set to -1 only during error in __cpufreq_add_dev
and during cpufreq_remove_dev, both of which are irrelevant to the scenario
we are dealing with here).

So, code ended up accessing and locking CPU 0's rwsemaphore. But how was
its initialization taken care of? The answer is the initcall sequence.
Cpufreq core code initialized itself at the core_initcall stage, and the
sa1100 cpufreq driver initialized itself at the arch_initcall stage, like
Viresh mentioned. And core-initcall comes before arch-initcall. So the
cpufreq core would have finished initializing the per-cpu rwsemaphores,
so that locking/unlocking them wouldn't crash the system or get complaints
from lockdep.

To summarize, I think before commit 474deff74, we were accessing CPU0's
rwsems (perhaps incorrectly) and due to the lacking initialization of the
per-cpu vars, the BUG_ON() would never fire.

To confirm this, you can perhaps try out one commit before 474deff74 and see
if it works for you. Or equivalently, you can apply the following patch
(revert of 474deff74) over mainline and see if it works.

That said, I'm still not sure how to fix the original issue. I'll do some
more digging later.

Regards,
Srivatsa S. Bhat

[The following is an untested plain revert of 474deff74, with a small
correction to account for the movement of update_policy_cpu() function.
If this doesn't work, please try a commit below 474deff74.]


diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index 82ecbe3..5164529 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -64,14 +64,15 @@ static DEFINE_PER_CPU(char[CPUFREQ_NAME_LEN], cpufreq_cpu_governor);
  * - Lock should not be held across
  *     __cpufreq_governor(data, CPUFREQ_GOV_STOP);
  */
+static DEFINE_PER_CPU(int, cpufreq_policy_cpu);
 static DEFINE_PER_CPU(struct rw_semaphore, cpu_policy_rwsem);
 
 #define lock_policy_rwsem(mode, cpu)					\
 static int 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));		\
+	int policy_cpu = per_cpu(cpufreq_policy_cpu, cpu);		\
+	BUG_ON(policy_cpu == -1);					\
+	down_##mode(&per_cpu(cpu_policy_rwsem, policy_cpu));		\
 									\
 	return 0;							\
 }
@@ -82,9 +83,9 @@ 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));		\
+	int policy_cpu = per_cpu(cpufreq_policy_cpu, cpu);		\
+	BUG_ON(policy_cpu == -1);					\
+	up_##mode(&per_cpu(cpu_policy_rwsem, policy_cpu));		\
 }
 
 unlock_policy_rwsem(read, cpu);
@@ -880,6 +881,7 @@ static int cpufreq_add_policy_cpu(struct cpufreq_policy *policy,
 	write_lock_irqsave(&cpufreq_driver_lock, flags);
 
 	cpumask_set_cpu(cpu, policy->cpus);
+	per_cpu(cpufreq_policy_cpu, cpu) = policy->cpu;
 	per_cpu(cpufreq_cpu_data, cpu) = policy;
 	write_unlock_irqrestore(&cpufreq_driver_lock, flags);
 
@@ -949,6 +951,9 @@ static void cpufreq_policy_free(struct cpufreq_policy *policy)
 
 static void update_policy_cpu(struct cpufreq_policy *policy, unsigned int cpu)
 {
+	int j;
+	unsigned long flags;
+
 	if (cpu == policy->cpu)
 		return;
 
@@ -966,6 +971,13 @@ static void update_policy_cpu(struct cpufreq_policy *policy, unsigned int cpu)
 
 	up_write(&per_cpu(cpu_policy_rwsem, policy->last_cpu));
 
+	write_lock_irqsave(&cpufreq_driver_lock, flags);
+
+	for_each_cpu(j, policy->cpus)
+		per_cpu(cpufreq_policy_cpu, j) = cpu;
+
+	write_unlock_irqrestore(&cpufreq_driver_lock, flags);
+
 #ifdef CONFIG_CPU_FREQ_TABLE
 	cpufreq_frequency_table_update_policy_cpu(policy);
 #endif
@@ -1041,6 +1053,9 @@ static int __cpufreq_add_dev(struct device *dev, struct subsys_interface *sif,
 	policy->governor = CPUFREQ_DEFAULT_GOVERNOR;
 	cpumask_copy(policy->cpus, cpumask_of(cpu));
 
+	/* Initially set CPU itself as the policy_cpu */
+	per_cpu(cpufreq_policy_cpu, cpu) = cpu;
+
 	init_completion(&policy->kobj_unregister);
 	INIT_WORK(&policy->update, handle_update);
 
@@ -1078,8 +1093,10 @@ static int __cpufreq_add_dev(struct device *dev, struct subsys_interface *sif,
 #endif
 
 	write_lock_irqsave(&cpufreq_driver_lock, flags);
-	for_each_cpu(j, policy->cpus)
+	for_each_cpu(j, policy->cpus) {
 		per_cpu(cpufreq_cpu_data, j) = policy;
+		per_cpu(cpufreq_policy_cpu, j) = policy->cpu;
+	}
 	write_unlock_irqrestore(&cpufreq_driver_lock, flags);
 
 	if (!frozen) {
@@ -1103,11 +1120,15 @@ static int __cpufreq_add_dev(struct device *dev, struct subsys_interface *sif,
 
 err_out_unregister:
 	write_lock_irqsave(&cpufreq_driver_lock, flags);
-	for_each_cpu(j, policy->cpus)
+	for_each_cpu(j, policy->cpus) {
 		per_cpu(cpufreq_cpu_data, j) = NULL;
+		if (j != cpu)
+			per_cpu(cpufreq_policy_cpu, j) = -1;
+	}
 	write_unlock_irqrestore(&cpufreq_driver_lock, flags);
 
 err_set_policy_cpu:
+	per_cpu(cpufreq_policy_cpu, cpu) = -1;
 	cpufreq_policy_free(policy);
 nomem_out:
 	up_read(&cpufreq_rwsem);
@@ -1133,6 +1154,7 @@ static int cpufreq_nominate_new_policy_cpu(struct cpufreq_policy *policy,
 					   unsigned int old_cpu, bool frozen)
 {
 	struct device *cpu_dev;
+	unsigned long flags;
 	int ret;
 
 	/* first sibling now owns the new sysfs dir */
@@ -1149,6 +1171,11 @@ static int cpufreq_nominate_new_policy_cpu(struct cpufreq_policy *policy,
 
 		WARN_ON(lock_policy_rwsem_write(old_cpu));
 		cpumask_set_cpu(old_cpu, policy->cpus);
+
+		write_lock_irqsave(&cpufreq_driver_lock, flags);
+		per_cpu(cpufreq_cpu_data, old_cpu) = policy;
+		write_unlock_irqrestore(&cpufreq_driver_lock, flags);
+
 		unlock_policy_rwsem_write(old_cpu);
 
 		ret = sysfs_create_link(&cpu_dev->kobj, &policy->kobj,
@@ -1174,6 +1201,7 @@ static int __cpufreq_remove_dev_prepare(struct device *dev,
 	write_lock_irqsave(&cpufreq_driver_lock, flags);
 
 	policy = per_cpu(cpufreq_cpu_data, cpu);
+	per_cpu(cpufreq_cpu_data, cpu) = NULL;
 
 	/* Save the policy somewhere when doing a light-weight tear-down */
 	if (frozen)
@@ -1305,7 +1333,7 @@ static int __cpufreq_remove_dev_finish(struct device *dev,
 		}
 	}
 
-	per_cpu(cpufreq_cpu_data, cpu) = NULL;
+	per_cpu(cpufreq_policy_cpu, cpu) = -1;
 	return 0;
 }
 
@@ -2185,8 +2213,10 @@ static int __init cpufreq_core_init(void)
 	if (cpufreq_disabled())
 		return -ENODEV;
 
-	for_each_possible_cpu(cpu)
+	for_each_possible_cpu(cpu) {
+		per_cpu(cpufreq_policy_cpu, cpu) = -1;
 		init_rwsem(&per_cpu(cpu_policy_rwsem, cpu));
+	}
 
 	cpufreq_global_kobject = kobject_create();
 	BUG_ON(!cpufreq_global_kobject);

Re: Regression on cpufreq in v3.12-rc1

[Srivatsa S. Bhat]

On 09/19/2013 11:41 PM, Srivatsa S. Bhat wrote:
> On 09/19/2013 06:28 PM, Srivatsa S. Bhat wrote:
>> On 09/19/2013 06:25 PM, Linus Walleij wrote:
>>> On Thu, Sep 19, 2013 at 2:46 PM, Srivatsa S. Bhat
>>> <srivatsa.bhat@linux.vnet.ibm.com> wrote:
>>>
>>>>>> I don't really know if this is the right solution at all, so please
>>>>>> help me out here... if you want that patch I can send it once
>>>>>> I understand this properly.
>>>>
>>>> IIRC, recent kernels didn't return 0 or any error code when the !policy
>>>> condition was matched. So can you check whether this problem occurs with
>>>> 3.11 or 3.10 as well?
>>>
>>> v3.11 works fine.
>>>
> 
> Ok, now that I got a chance to look at the code and the git logs, I think
> I have a clearer idea of what is happening.
> 
> Basically commit 474deff74 (cpufreq: remove cpufreq_policy_cpu per-cpu
> variable) exposed a hidden issue. Before that commit, the code looked like
> this:
> 
> static DEFINE_PER_CPU(int, cpufreq_policy_cpu);
> 
> [...]
> 
> static int lock_policy_rwsem_##mode(int cpu)                            \
> {                                                                       \
>         int policy_cpu = per_cpu(cpufreq_policy_cpu, cpu);              \
>         BUG_ON(policy_cpu == -1);                                       \
>         down_##mode(&per_cpu(cpu_policy_rwsem, policy_cpu));            \
>                                                                         \
>         return 0;                                                       \
> }
> 
> But there was no code to set the per-cpu values to -1 to begin with. Since
> the per-cpu variable was defined as static, it would have been initialized
> to zero. Thus, we would never actually hit the BUG_ON() condition, since
> policy_cpu didn't turn out to be -1.
> 
> (The per-cpu variable was set to -1 only during error in __cpufreq_add_dev
> and during cpufreq_remove_dev, both of which are irrelevant to the scenario
> we are dealing with here).
> 
> So, code ended up accessing and locking CPU 0's rwsemaphore. But how was
> its initialization taken care of? The answer is the initcall sequence.
> Cpufreq core code initialized itself at the core_initcall stage, and the
> sa1100 cpufreq driver initialized itself at the arch_initcall stage, like
> Viresh mentioned. And core-initcall comes before arch-initcall. So the
> cpufreq core would have finished initializing the per-cpu rwsemaphores,
> so that locking/unlocking them wouldn't crash the system or get complaints
> from lockdep.
> 
> To summarize, I think before commit 474deff74, we were accessing CPU0's
> rwsems (perhaps incorrectly) and due to the lacking initialization of the
> per-cpu vars, the BUG_ON() would never fire.
> 
> To confirm this, you can perhaps try out one commit before 474deff74 and see
> if it works for you. Or equivalently, you can apply the following patch
> (revert of 474deff74) over mainline and see if it works.

Just before sending, I rebased that patch on top of Rafael's linux-next
branch, but forgot to update the above sentence. However I think the same
patch might apply cleanly over mainline as well.

Regards,
Srivatsa S. Bhat

Re: Regression on cpufreq in v3.12-rc1

[Viresh Kumar]

On 19 September 2013 23:41, Srivatsa S. Bhat
<srivatsa.bhat@linux.vnet.ibm.com> wrote:
> But there was no code to set the per-cpu values to -1 to begin with. Since
> the per-cpu variable was defined as static, it would have been initialized
> to zero. Thus, we would never actually hit the BUG_ON() condition, since
> policy_cpu didn't turn out to be -1.

Really!! Or I have turned blind (and there is very strong chance of that,
considering the amount of silly mistakes I do :) )...

I picked it up from 474deff7 only:

@@ -2148,10 +2125,8 @@ static int __init cpufreq_core_init(void)
        if (cpufreq_disabled())
                return -ENODEV;

-       for_each_possible_cpu(cpu) {
-               per_cpu(cpufreq_policy_cpu, cpu) = -1;
+       for_each_possible_cpu(cpu)
                init_rwsem(&per_cpu(cpu_policy_rwsem, cpu));
-       }

        cpufreq_global_kobject = kobject_create();
        BUG_ON(!cpufreq_global_kobject);

Re: Regression on cpufreq in v3.12-rc1

[Linus Walleij]

On Thu, Sep 19, 2013 at 2:55 PM, Linus Walleij <linus.walleij@linaro.org> wrote:
> On Thu, Sep 19, 2013 at 2:46 PM, Srivatsa S. Bhat
> <srivatsa.bhat@linux.vnet.ibm.com> wrote:
>
>> So I think we should first identify (bisect?) and understand what caused that
>> particular change and then we will be in a position to evaluate whether the
>> patch you proposed would be the right fix or not.
>
> I'll see if I can get a bisect going, the problem is that I upload the
> kernel over the serial port so this isn't a very quick procedure :-(

Well I did a bisect anyway, it seems the problem appears due
at this commit:

commit 6eed9404ab3c4baea54ce4c7e862e69df1d39f38
Author: Viresh Kumar <viresh.kumar@linaro.org>
Date:   Tue Aug 6 22:53:11 2013 +0530

    cpufreq: Use rwsem for protecting critical sections

    Critical sections of the cpufreq core are protected with the help of
    the driver module owner's refcount, which isn't the correct approach,
    because it causes rmmod to return an error when some routine has
    updated that refcount.

    Let's use rwsem for this purpose instead.  Only
    cpufreq_unregister_driver() will use write sem
    and everybody else will use read sem.

    [rjw: Subject & changelog]
    Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
    Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>

I suspect this hunk from the patch may be the cause:

+       if (cpufreq_driver) {
+               /* get the CPU */
+               policy = per_cpu(cpufreq_cpu_data, cpu);
+               if (policy)
+                       kobject_get(&policy->kobj);
+       }

-       /* get the CPU */
-       policy = per_cpu(cpufreq_cpu_data, cpu);

As you see we *always* set a policy pointer before this patch,
but after this patch we only do it if we have a cpufreq driver
registered!

It's not trivial to fix this for me though, it seems there are some
other suggestions in this thread...

Yours,
Linus Walleij

Re: Regression on cpufreq in v3.12-rc1

[Viresh Kumar]

On 20 September 2013 14:03, Linus Walleij <linus.walleij@linaro.org> wrote:
> I suspect this hunk from the patch may be the cause:
>
> +       if (cpufreq_driver) {
> +               /* get the CPU */
> +               policy = per_cpu(cpufreq_cpu_data, cpu);
> +               if (policy)
> +                       kobject_get(&policy->kobj);
> +       }
>
> -       /* get the CPU */
> -       policy = per_cpu(cpufreq_cpu_data, cpu);
>
> As you see we *always* set a policy pointer before this patch,
> but after this patch we only do it if we have a cpufreq driver
> registered!

Not really!! See this few lines above:

-       if (!cpufreq_driver)
-               goto err_out_unlock;
-

Re: Regression on cpufreq in v3.12-rc1

[Linus Walleij]

On Fri, Sep 20, 2013 at 10:39 AM, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> On 20 September 2013 14:03, Linus Walleij <linus.walleij@linaro.org> wrote:
>> I suspect this hunk from the patch may be the cause:
>>
>> +       if (cpufreq_driver) {
>> +               /* get the CPU */
>> +               policy = per_cpu(cpufreq_cpu_data, cpu);
>> +               if (policy)
>> +                       kobject_get(&policy->kobj);
>> +       }
>>
>> -       /* get the CPU */
>> -       policy = per_cpu(cpufreq_cpu_data, cpu);
>>
>> As you see we *always* set a policy pointer before this patch,
>> but after this patch we only do it if we have a cpufreq driver
>> registered!
>
> Not really!! See this few lines above:
>
> -       if (!cpufreq_driver)
> -               goto err_out_unlock;

Hm that's true...

Any other idea why this patch is causing the issue?

It's not the same patch pointed out by Srivatsa...

Yours,
Linus Walleij

Re: Regression on cpufreq in v3.12-rc1

[Linus Walleij]

On Thu, Sep 19, 2013 at 8:11 PM, Srivatsa S. Bhat
<srivatsa.bhat@linux.vnet.ibm.com> wrote:

> To confirm this, you can perhaps try out one commit before 474deff74 and see
> if it works for you. Or equivalently, you can apply the following patch
> (revert of 474deff74) over mainline and see if it works.

Actually I did try to revert that patch before, during trial-and-error ...
However it doesn't help, as I realized after bisecting it's commit
6eed940 that is causing this somehow...

Yours,
Linus Walleij

Re: Regression on cpufreq in v3.12-rc1

[Viresh Kumar]

On 20 September 2013 14:11, Linus Walleij <linus.walleij@linaro.org> wrote:
> Any other idea why this patch is causing the issue?

I went into looking that patch in more detail
after my first reply, not as if I ran away from answering that :)

Probably yes.. I know what's causing it:

 unsigned int cpufreq_get(unsigned int cpu)
 {
        unsigned int ret_freq = 0;
-       struct cpufreq_policy *policy = cpufreq_cpu_get(cpu);

-       if (!policy)
-               goto out;
+       if (!down_read_trylock(&cpufreq_rwsem))
+               return 0;

        if (unlikely(lock_policy_rwsem_read(cpu)))
                goto out_policy;
@@ -1438,8 +1413,8 @@ unsigned int cpufreq_get(unsigned int cpu)
        unlock_policy_rwsem_read(cpu);

 out_policy:
-       cpufreq_cpu_put(policy);
-out:
+       up_read(&cpufreq_rwsem);
+
        return ret_freq;
 }

---------x---------------x--------------

We used to return early in case policy isn't found, but now we went
and took the lock..

Hmm... Remember I told you last time that I have another way of fixing
it up, probably we need that now..

I wanted to add another variable to reflect if a cpufreq_driver is registered
or not, and if not then return early from these routines..

I will get that in now, please see if you can give it a try..

But I am still surprised how are we reaching this place before your cpufreq
driver gets registered..

Re: Regression on cpufreq in v3.12-rc1

[Viresh Kumar]

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

On 20 September 2013 14:19, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> Hmm... Remember I told you last time that I have another way of fixing
> it up, probably we need that now..
>
> I wanted to add another variable to reflect if a cpufreq_driver is registered
> or not, and if not then return early from these routines..
>
> I will get that in now, please see if you can give it a try..
>
> But I am still surprised how are we reaching this place before your cpufreq
> driver gets registered..

Once we know what's going on in your system, please test attached patch
(Will send it separately once you have tested it):

commit 389fbc3c8ad7c339cd2d9572d73c355b7b967823
Author: Viresh Kumar <viresh.kumar@linaro.org>
Date:   Fri Sep 20 14:55:31 2013 +0530

    cpufreq: check cpufreq driver is valid and cpufreq isn't disabled
in cpufreq_get()

    cpufreq_get() can be called from external drivers which might not
be aware if
    cpufreq driver is registered or not. And so we should actually
check if cpufreq
    driver is registered or not and also if cpufreq is active or
disabled, at the
    beginning of cpufreq_get().

    Otherwise call to lock_policy_rwsem_read() might hit BUG_ON(!policy).

    Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/cpufreq/cpufreq.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index 82ecbe3..db004a8 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -1460,6 +1460,9 @@ unsigned int cpufreq_get(unsigned int cpu)
 {
        unsigned int ret_freq = 0;

+       if (cpufreq_disabled() || !cpufreq_driver)
+               return -ENOENT;
+
        if (!down_read_trylock(&cpufreq_rwsem))
                return 0;

[-- Attachment #2: 0001-cpufreq-check-cpufreq-driver-is-valid-and-cpufreq-is.patch --]
[-- Type: application/octet-stream, Size: 1224 bytes --]

From 389fbc3c8ad7c339cd2d9572d73c355b7b967823 Mon Sep 17 00:00:00 2001
Message-Id: <389fbc3c8ad7c339cd2d9572d73c355b7b967823.1379669625.git.viresh.kumar@linaro.org>
From: Viresh Kumar <viresh.kumar@linaro.org>
Date: Fri, 20 Sep 2013 14:55:31 +0530
Subject: [PATCH] cpufreq: check cpufreq driver is valid and cpufreq isn't
 disabled in cpufreq_get()

cpufreq_get() can be called from external drivers which might not be aware if
cpufreq driver is registered or not. And so we should actually check if cpufreq
driver is registered or not and also if cpufreq is active or disabled, at the
beginning of cpufreq_get().

Otherwise call to lock_policy_rwsem_read() might hit BUG_ON(!policy).

Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/cpufreq/cpufreq.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index 82ecbe3..db004a8 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -1460,6 +1460,9 @@ unsigned int cpufreq_get(unsigned int cpu)
 {
 	unsigned int ret_freq = 0;
 
+	if (cpufreq_disabled() || !cpufreq_driver)
+		return -ENOENT;
+
 	if (!down_read_trylock(&cpufreq_rwsem))
 		return 0;
 
-- 
1.7.12.rc2.18.g61b472e

Re: Regression on cpufreq in v3.12-rc1

[Srivatsa S. Bhat]

On 09/20/2013 09:49 AM, Viresh Kumar wrote:
> On 19 September 2013 23:41, Srivatsa S. Bhat
> <srivatsa.bhat@linux.vnet.ibm.com> wrote:
>> But there was no code to set the per-cpu values to -1 to begin with. Since
>> the per-cpu variable was defined as static, it would have been initialized
>> to zero. Thus, we would never actually hit the BUG_ON() condition, since
>> policy_cpu didn't turn out to be -1.
> 
> Really!! Or I have turned blind (and there is very strong chance of that,
> considering the amount of silly mistakes I do :) )...
> 
> I picked it up from 474deff7 only:
> 
> @@ -2148,10 +2125,8 @@ static int __init cpufreq_core_init(void)
>         if (cpufreq_disabled())
>                 return -ENODEV;
> 
> -       for_each_possible_cpu(cpu) {
> -               per_cpu(cpufreq_policy_cpu, cpu) = -1;
> +       for_each_possible_cpu(cpu)
>                 init_rwsem(&per_cpu(cpu_policy_rwsem, cpu));
> -       }
> 
>         cpufreq_global_kobject = kobject_create();
>         BUG_ON(!cpufreq_global_kobject);
> 


Heh, looks like it was me who was blind then :-/

Regards,
Srivatsa S. Bhat

Re: Regression on cpufreq in v3.12-rc1

[Srivatsa S. Bhat]

On 09/20/2013 03:05 PM, Viresh Kumar wrote:
> On 20 September 2013 14:19, Viresh Kumar <viresh.kumar@linaro.org> wrote:
>> Hmm... Remember I told you last time that I have another way of fixing
>> it up, probably we need that now..
>>
>> I wanted to add another variable to reflect if a cpufreq_driver is registered
>> or not, and if not then return early from these routines..
>>
>> I will get that in now, please see if you can give it a try..
>>
>> But I am still surprised how are we reaching this place before your cpufreq
>> driver gets registered..
> 
> Once we know what's going on in your system, please test attached patch
> (Will send it separately once you have tested it):
> 
> commit 389fbc3c8ad7c339cd2d9572d73c355b7b967823
> Author: Viresh Kumar <viresh.kumar@linaro.org>
> Date:   Fri Sep 20 14:55:31 2013 +0530
> 
>     cpufreq: check cpufreq driver is valid and cpufreq isn't disabled
> in cpufreq_get()
> 
>     cpufreq_get() can be called from external drivers which might not
> be aware if
>     cpufreq driver is registered or not. And so we should actually
> check if cpufreq
>     driver is registered or not and also if cpufreq is active or
> disabled, at the
>     beginning of cpufreq_get().
> 
>     Otherwise call to lock_policy_rwsem_read() might hit BUG_ON(!policy).
> 
>     Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>

The patch looks reasonable to me. Earlier cpufreq_cpu_get() used to do the
necessary checks, but commit 6eed9404 removed the call to that function in
cpufreq_get(). So adding those checks back seem like the right fix to me.
Also, looking at commit 6eed9404, I think show() and store() also suffer
from a similar fate. So do you think we need to add these checks there as well?
I'm not sure, since I can't think of a situation in which show() or store()
can be invoked before the cpufreq-driver is registered.. or, is such a
situation possible with cpufreq_disabled()?

Regards,
Srivatsa S. Bhat

> ---
>  drivers/cpufreq/cpufreq.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> index 82ecbe3..db004a8 100644
> --- a/drivers/cpufreq/cpufreq.c
> +++ b/drivers/cpufreq/cpufreq.c
> @@ -1460,6 +1460,9 @@ unsigned int cpufreq_get(unsigned int cpu)
>  {
>         unsigned int ret_freq = 0;
> 
> +       if (cpufreq_disabled() || !cpufreq_driver)
> +               return -ENOENT;
> +
>         if (!down_read_trylock(&cpufreq_rwsem))
>                 return 0;
> 

Re: Regression on cpufreq in v3.12-rc1

[Linus Walleij]

On Fri, Sep 20, 2013 at 10:49 AM, Viresh Kumar <viresh.kumar@linaro.org> wrote:

> We used to return early in case policy isn't found, but now we went
> and took the lock..

Aha!

> Hmm... Remember I told you last time that I have another way of fixing
> it up, probably we need that now..
>
> I wanted to add another variable to reflect if a cpufreq_driver is registered
> or not, and if not then return early from these routines..
>
> I will get that in now, please see if you can give it a try..

OK standing by.

> But I am still surprised how are we reaching this place before your cpufreq
> driver gets registered..

I have no idea really. The funny thing is that Russell has a similar
platform (Assabet) and he's not seeing the issue. It appears on this
h3600 iPAQ only.

So here is the backtrace:

[<c01db010>] (lock_policy_rwsem_read+0x0/0x3c) from [<c01dc6c8>]
(cpufreq_get+0x34/0x68)
[<c01dc694>] (cpufreq_get+0x0/0x68) from [<c01d2c94>]
(sa1100_pcmcia_set_timing+0x18/0x28)
 r5:00000000 r4:c182d008
[<c01d2c7c>] (sa1100_pcmcia_set_timing+0x0/0x28) from [<c01d27ec>]
(soc_pcmcia_add_one+0x100/0x220)
 r4:c182d008
[<c01d26ec>] (soc_pcmcia_add_one+0x0/0x220) from [<c01d2d30>]
(sa11xx_drv_pcmcia_add_one+0x8c/0xa4)
[<c01d2ca4>] (sa11xx_drv_pcmcia_add_one+0x0/0xa4) from [<c01d2de4>]
(sa11xx_drv_pcmcia_probe+0x9c/0xf8)
 r8:00000002 r7:c182d000 r6:c182d000 r5:00000000 r4:c182d008
[<c01d2d48>] (sa11xx_drv_pcmcia_probe+0x0/0xf8) from [<c01d3184>]
(pcmcia_h3600_init+0x1c/0x24)
[<c01d3168>] (pcmcia_h3600_init+0x0/0x24) from [<c01d2f70>]
(sa11x0_drv_pcmcia_probe+0x14/0x18)
[<c01d2f5c>] (sa11x0_drv_pcmcia_probe+0x0/0x18) from [<c019ec70>]
(platform_drv_probe+0x20/0x24)
[<c019ec50>] (platform_drv_probe+0x0/0x24) from [<c019d638>]
(really_probe+0x108/0x1c4)
[<c019d530>] (really_probe+0x0/0x1c4) from [<c019d724>]
(driver_probe_device+0x30/0x34)
 r8:00000000 r7:c019d970 r6:c05c81f0 r5:c05bb6b0 r4:c05bb67c
[<c019d6f4>] (driver_probe_device+0x0/0x34) from [<c019d9f8>]
(__driver_attach+0x88/0x8c)
[<c019d970>] (__driver_attach+0x0/0x8c) from [<c019bea0>]
(bus_for_each_dev+0x74/0x98)
 r6:c05c81f0 r5:c1833e40 r4:00000000
[<c019be2c>] (bus_for_each_dev+0x0/0x98) from [<c019d410>]
(driver_attach+0x20/0x28)
 r7:00000000 r6:c05c6c58 r5:c05c81f0 r4:c18c5900
[<c019d3f0>] (driver_attach+0x0/0x28) from [<c019cc6c>]
(bus_add_driver+0xe0/0x19c)
[<c019cb8c>] (bus_add_driver+0x0/0x19c) from [<c019dfe4>]
(driver_register+0x60/0x104)
 r7:c036b6ac r6:00000006 r5:00000000 r4:c05c81f0
[<c019df84>] (driver_register+0x0/0x104) from [<c019ef64>]
(__platform_driver_register+0x50/0x64)
 r5:00000000 r4:c1832000
[<c019ef14>] (__platform_driver_register+0x0/0x64) from [<c036b6c4>]
(sa11x0_pcmcia_init+0x18/0x20)
[<c036b6ac>] (sa11x0_pcmcia_init+0x0/0x20) from [<c00085a4>]
(do_one_initcall+0x9c/0xfc)
[<c0008508>] (do_one_initcall+0x0/0xfc) from [<c0355590>]
(do_initcall_level+0x80/0xb0)
 r7:c036fbec r6:00000006 r5:00000005 r4:c0372d9c
[<c0355510>] (do_initcall_level+0x0/0xb0) from [<c03555e4>]
(do_initcalls+0x24/0x30)
 r7:00000000 r6:00000000 r5:c0297820 r4:00000006
[<c03555c0>] (do_initcalls+0x0/0x30) from [<c03556bc>]
(do_basic_setup+0x28/0x2c)
 r4:00000000
[<c0355694>] (do_basic_setup+0x0/0x2c) from [<c0355a98>]
(kernel_init_freeable+0x4c/0xdc)
[<c0355a4c>] (kernel_init_freeable+0x0/0xdc) from [<c0297830>]
(kernel_init+0x10/0xf0)
 r4:00000000
[<c0297820>] (kernel_init+0x0/0xf0) from [<c000f950>] (ret_from_fork+0x14/0x24)
 r4:00000000
Code: e59f0014 eb02f800 e3a00000 e89da800 (e7f001f2)

sa11x0_pcmcia_init() which starts this chain of events is called as
an fs_initcall(), see drivers/pcmcia/sa1100_generic.c
if I comment out this one, I get the same thing in the frambuffer
driver instead, which is at module_init().

I don't have a trace so it's not like I know exactly what happened
before this point,
but the dmesg up to here reads:

Linux version 3.11.0-rc4-00024-g6eed940 (linus@localhost.localdomain)
(gcc version 4.2.1) #41 Fri Sep 20 10:50:04 CEST 2013
CPU: StrongARM-1110 [6901b118] revision 8 (ARMv4), cr=c000717f
CPU: VIVT data cache, VIVT instruction cache
Machine: Compaq iPAQ H3600
Ignoring tag cmdline (using the default kernel command line)
Memory policy: ECC disabled, Data cache writeback
Built 1 zonelists in Zone order, mobility grouping on.  Total pages: 8128
Kernel command line: console=ttySA0,115200n8
PID hash table entries: 128 (order: -3, 512 bytes)
Dentry cache hash table entries: 4096 (order: 2, 16384 bytes)
Inode-cache hash table entries: 2048 (order: 1, 8192 bytes)
Memory: 26412K/32768K available (2643K kernel code, 118K rwdata, 736K
rodata, 2411K init, 76K bss, 6356K reserved)
Virtual kernel memory layout:
    vector  : 0xffff0000 - 0xffff1000   (   4 kB)
    fixmap  : 0xfff00000 - 0xfffe0000   ( 896 kB)
    vmalloc : 0xc2800000 - 0xff000000   ( 968 MB)
    lowmem  : 0xc0000000 - 0xc2000000   (  32 MB)
    modules : 0xbf000000 - 0xc0000000   (  16 MB)
      .text : 0xc0008000 - 0xc0354f18   (3380 kB)
      .init : 0xc0355000 - 0xc05affb4   (2412 kB)
      .data : 0xc05b0000 - 0xc05cd840   ( 119 kB)
       .bss : 0xc05cd840 - 0xc05e0a68   (  77 kB)
SLUB: HWalign=32, Order=0-3, MinObjects=0, CPUs=1, Nodes=1
NR_IRQS:16 nr_irqs:49 49
sched_clock: 32 bits at 3686kHz, resolution 271ns, wraps every 1165084ms
Console: colour dummy device 80x30
console [ttySA0] enabled
Calibrating delay loop... 136.60 BogoMIPS (lpj=683008)
pid_max: default: 32768 minimum: 301
Mount-cache hash table entries: 512
CPU: Testing write buffer coherency: ok
Setting up static identity map for 0xc029a910 - 0xc029a968
NET: Registered protocol family 16
DMA: preallocated 256 KiB pool for atomic coherent allocations
bio: create slab <bio-0> at 0
Switched to clocksource oscr

Yours,
Linus Walleij

Re: Regression on cpufreq in v3.12-rc1

[Srivatsa S. Bhat]

On 09/20/2013 08:51 PM, Linus Walleij wrote:
> On Fri, Sep 20, 2013 at 10:49 AM, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> 
>> We used to return early in case policy isn't found, but now we went
>> and took the lock..
> 
> Aha!
> 
>> Hmm... Remember I told you last time that I have another way of fixing
>> it up, probably we need that now..
>>
>> I wanted to add another variable to reflect if a cpufreq_driver is registered
>> or not, and if not then return early from these routines..
>>
>> I will get that in now, please see if you can give it a try..
> 
> OK standing by.
> 
>> But I am still surprised how are we reaching this place before your cpufreq
>> driver gets registered..
> 
> I have no idea really. The funny thing is that Russell has a similar
> platform (Assabet) and he's not seeing the issue. It appears on this
> h3600 iPAQ only.
> 
> So here is the backtrace:
> 
> [<c01db010>] (lock_policy_rwsem_read+0x0/0x3c) from [<c01dc6c8>]
> (cpufreq_get+0x34/0x68)
> [<c01dc694>] (cpufreq_get+0x0/0x68) from [<c01d2c94>]
> (sa1100_pcmcia_set_timing+0x18/0x28)
>  r5:00000000 r4:c182d008
> [<c01d2c7c>] (sa1100_pcmcia_set_timing+0x0/0x28) from [<c01d27ec>]
> (soc_pcmcia_add_one+0x100/0x220)
>  r4:c182d008
> [<c01d26ec>] (soc_pcmcia_add_one+0x0/0x220) from [<c01d2d30>]
> (sa11xx_drv_pcmcia_add_one+0x8c/0xa4)
> [<c01d2ca4>] (sa11xx_drv_pcmcia_add_one+0x0/0xa4) from [<c01d2de4>]
> (sa11xx_drv_pcmcia_probe+0x9c/0xf8)
>  r8:00000002 r7:c182d000 r6:c182d000 r5:00000000 r4:c182d008
> [<c01d2d48>] (sa11xx_drv_pcmcia_probe+0x0/0xf8) from [<c01d3184>]
> (pcmcia_h3600_init+0x1c/0x24)
> [<c01d3168>] (pcmcia_h3600_init+0x0/0x24) from [<c01d2f70>]
> (sa11x0_drv_pcmcia_probe+0x14/0x18)
> [<c01d2f5c>] (sa11x0_drv_pcmcia_probe+0x0/0x18) from [<c019ec70>]
> (platform_drv_probe+0x20/0x24)
> [<c019ec50>] (platform_drv_probe+0x0/0x24) from [<c019d638>]
> (really_probe+0x108/0x1c4)
> [<c019d530>] (really_probe+0x0/0x1c4) from [<c019d724>]
> (driver_probe_device+0x30/0x34)
>  r8:00000000 r7:c019d970 r6:c05c81f0 r5:c05bb6b0 r4:c05bb67c
> [<c019d6f4>] (driver_probe_device+0x0/0x34) from [<c019d9f8>]
> (__driver_attach+0x88/0x8c)
> [<c019d970>] (__driver_attach+0x0/0x8c) from [<c019bea0>]
> (bus_for_each_dev+0x74/0x98)
>  r6:c05c81f0 r5:c1833e40 r4:00000000
> [<c019be2c>] (bus_for_each_dev+0x0/0x98) from [<c019d410>]
> (driver_attach+0x20/0x28)
>  r7:00000000 r6:c05c6c58 r5:c05c81f0 r4:c18c5900
> [<c019d3f0>] (driver_attach+0x0/0x28) from [<c019cc6c>]
> (bus_add_driver+0xe0/0x19c)
> [<c019cb8c>] (bus_add_driver+0x0/0x19c) from [<c019dfe4>]
> (driver_register+0x60/0x104)
>  r7:c036b6ac r6:00000006 r5:00000000 r4:c05c81f0
> [<c019df84>] (driver_register+0x0/0x104) from [<c019ef64>]
> (__platform_driver_register+0x50/0x64)
>  r5:00000000 r4:c1832000
> [<c019ef14>] (__platform_driver_register+0x0/0x64) from [<c036b6c4>]
> (sa11x0_pcmcia_init+0x18/0x20)
> [<c036b6ac>] (sa11x0_pcmcia_init+0x0/0x20) from [<c00085a4>]
> (do_one_initcall+0x9c/0xfc)
> [<c0008508>] (do_one_initcall+0x0/0xfc) from [<c0355590>]
> (do_initcall_level+0x80/0xb0)
>  r7:c036fbec r6:00000006 r5:00000005 r4:c0372d9c
> [<c0355510>] (do_initcall_level+0x0/0xb0) from [<c03555e4>]
> (do_initcalls+0x24/0x30)
>  r7:00000000 r6:00000000 r5:c0297820 r4:00000006
> [<c03555c0>] (do_initcalls+0x0/0x30) from [<c03556bc>]
> (do_basic_setup+0x28/0x2c)
>  r4:00000000
> [<c0355694>] (do_basic_setup+0x0/0x2c) from [<c0355a98>]
> (kernel_init_freeable+0x4c/0xdc)
> [<c0355a4c>] (kernel_init_freeable+0x0/0xdc) from [<c0297830>]
> (kernel_init+0x10/0xf0)
>  r4:00000000
> [<c0297820>] (kernel_init+0x0/0xf0) from [<c000f950>] (ret_from_fork+0x14/0x24)
>  r4:00000000
> Code: e59f0014 eb02f800 e3a00000 e89da800 (e7f001f2)
> 
> sa11x0_pcmcia_init() which starts this chain of events is called as
> an fs_initcall(), see drivers/pcmcia/sa1100_generic.c

But fs_initcall() comes after arch_initcall() right? (looking at
include/linux/init.h). Since the corresponding cpufreq-driver (drivers/
cpufreq/sa1100-cpufreq.c) uses arch_initcall() to register itself, it
is all the more surprising how this could happen...

Regards,
Srivatsa S. Bhat

> if I comment out this one, I get the same thing in the frambuffer
> driver instead, which is at module_init().
> 
> I don't have a trace so it's not like I know exactly what happened
> before this point,
> but the dmesg up to here reads:
> 
> Linux version 3.11.0-rc4-00024-g6eed940 (linus@localhost.localdomain)
> (gcc version 4.2.1) #41 Fri Sep 20 10:50:04 CEST 2013
> CPU: StrongARM-1110 [6901b118] revision 8 (ARMv4), cr=c000717f
> CPU: VIVT data cache, VIVT instruction cache
> Machine: Compaq iPAQ H3600
> Ignoring tag cmdline (using the default kernel command line)
> Memory policy: ECC disabled, Data cache writeback
> Built 1 zonelists in Zone order, mobility grouping on.  Total pages: 8128
> Kernel command line: console=ttySA0,115200n8
> PID hash table entries: 128 (order: -3, 512 bytes)
> Dentry cache hash table entries: 4096 (order: 2, 16384 bytes)
> Inode-cache hash table entries: 2048 (order: 1, 8192 bytes)
> Memory: 26412K/32768K available (2643K kernel code, 118K rwdata, 736K
> rodata, 2411K init, 76K bss, 6356K reserved)
> Virtual kernel memory layout:
>     vector  : 0xffff0000 - 0xffff1000   (   4 kB)
>     fixmap  : 0xfff00000 - 0xfffe0000   ( 896 kB)
>     vmalloc : 0xc2800000 - 0xff000000   ( 968 MB)
>     lowmem  : 0xc0000000 - 0xc2000000   (  32 MB)
>     modules : 0xbf000000 - 0xc0000000   (  16 MB)
>       .text : 0xc0008000 - 0xc0354f18   (3380 kB)
>       .init : 0xc0355000 - 0xc05affb4   (2412 kB)
>       .data : 0xc05b0000 - 0xc05cd840   ( 119 kB)
>        .bss : 0xc05cd840 - 0xc05e0a68   (  77 kB)
> SLUB: HWalign=32, Order=0-3, MinObjects=0, CPUs=1, Nodes=1
> NR_IRQS:16 nr_irqs:49 49
> sched_clock: 32 bits at 3686kHz, resolution 271ns, wraps every 1165084ms
> Console: colour dummy device 80x30
> console [ttySA0] enabled
> Calibrating delay loop... 136.60 BogoMIPS (lpj=683008)
> pid_max: default: 32768 minimum: 301
> Mount-cache hash table entries: 512
> CPU: Testing write buffer coherency: ok
> Setting up static identity map for 0xc029a910 - 0xc029a968
> NET: Registered protocol family 16
> DMA: preallocated 256 KiB pool for atomic coherent allocations
> bio: create slab <bio-0> at 0
> Switched to clocksource oscr
> 
> Yours,
> Linus Walleij
> 

Re: Regression on cpufreq in v3.12-rc1

[Linus Walleij]

On Fri, Sep 20, 2013 at 11:35 AM, Viresh Kumar <viresh.kumar@linaro.org> wrote:

> commit 389fbc3c8ad7c339cd2d9572d73c355b7b967823
> Author: Viresh Kumar <viresh.kumar@linaro.org>
> Date:   Fri Sep 20 14:55:31 2013 +0530
>
>     cpufreq: check cpufreq driver is valid and cpufreq isn't disabled
> in cpufreq_get()
>
>     cpufreq_get() can be called from external drivers which might not
> be aware if
>     cpufreq driver is registered or not. And so we should actually
> check if cpufreq
>     driver is registered or not and also if cpufreq is active or
> disabled, at the
>     beginning of cpufreq_get().
>
>     Otherwise call to lock_policy_rwsem_read() might hit BUG_ON(!policy).
>
>     Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
> ---
>  drivers/cpufreq/cpufreq.c | 3 +++
>  1 file changed, 3 insertions(+)
>
> diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
> index 82ecbe3..db004a8 100644
> --- a/drivers/cpufreq/cpufreq.c
> +++ b/drivers/cpufreq/cpufreq.c
> @@ -1460,6 +1460,9 @@ unsigned int cpufreq_get(unsigned int cpu)
>  {
>         unsigned int ret_freq = 0;
>
> +       if (cpufreq_disabled() || !cpufreq_driver)
> +               return -ENOENT;
> +
>         if (!down_read_trylock(&cpufreq_rwsem))
>                 return 0;

This works! My system boots without crashes after this.

Tested-by: Linus Walleij <linus.walleij@linaro.org>

Yours,
Linus Walleij

Re: Regression on cpufreq in v3.12-rc1

[Linus Walleij]

On Fri, Sep 20, 2013 at 11:35 AM, Viresh Kumar <viresh.kumar@linaro.org> wrote:

> +++ b/drivers/cpufreq/cpufreq.c
> @@ -1460,6 +1460,9 @@ unsigned int cpufreq_get(unsigned int cpu)
>  {
>         unsigned int ret_freq = 0;
>
> +       if (cpufreq_disabled() || !cpufreq_driver)
> +               return -ENOENT;
> +

But given that a cpufreq driver is just like any other driver, isn't the
proper thing to do to return -EPROBE_DEFER?

Yours,
Linus Walleij

Re: Regression on cpufreq in v3.12-rc1

[Linus Walleij]

On Fri, Sep 20, 2013 at 5:32 PM, Srivatsa S. Bhat
<srivatsa.bhat@linux.vnet.ibm.com> wrote:
> On 09/20/2013 08:51 PM, Linus Walleij wrote:

>> sa11x0_pcmcia_init() which starts this chain of events is called as
>> an fs_initcall(), see drivers/pcmcia/sa1100_generic.c
>
> But fs_initcall() comes after arch_initcall() right? (looking at
> include/linux/init.h). Since the corresponding cpufreq-driver (drivers/
> cpufreq/sa1100-cpufreq.c) uses arch_initcall() to register itself, it
> is all the more surprising how this could happen...

Hm it does happen if the cpufreq driver does not probe *at all*
right? I will look closer at this, maybe it just doesn't register
properly for some SA1100 variants...

Yours,
Linus Walleij

Re: Regression on cpufreq in v3.12-rc1

[Linus Walleij]

On Fri, Sep 20, 2013 at 5:40 PM, Linus Walleij <linus.walleij@linaro.org> wrote:
> On Fri, Sep 20, 2013 at 5:32 PM, Srivatsa S. Bhat
> <srivatsa.bhat@linux.vnet.ibm.com> wrote:
>> On 09/20/2013 08:51 PM, Linus Walleij wrote:
>
>>> sa11x0_pcmcia_init() which starts this chain of events is called as
>>> an fs_initcall(), see drivers/pcmcia/sa1100_generic.c
>>
>> But fs_initcall() comes after arch_initcall() right? (looking at
>> include/linux/init.h). Since the corresponding cpufreq-driver (drivers/
>> cpufreq/sa1100-cpufreq.c) uses arch_initcall() to register itself, it
>> is all the more surprising how this could happen...
>
> Hm it does happen if the cpufreq driver does not probe *at all*
> right? I will look closer at this, maybe it just doesn't register
> properly for some SA1100 variants...

Aha yeah this is it. The h3600 has an SA1110 and uses
drivers/cpufreq/sa1110-cpufreq.c.

This driver needs to know the RAM mounted on the board,
if it is this or that type, to perform properly.

This is passed from the command line or a default is selected
from the machine type:

    if (!name[0]) {
        if (machine_is_assabet())
            name = "TC59SM716-CL3";
        if (machine_is_pt_system3())
            name = "K4S641632D";
        if (machine_is_h3100())
            name = "KM416S4030CT";
        if (machine_is_jornada720())
            name = "K4S281632B-1H";
        if (machine_is_nanoengine())
            name = "MT48LC8M16A2TG-75";
    }

In this case we have neither. So the cpufreq driver fails to
register. But the kernel used to survive in any case, as you
could call cpufreq_get() without a cpufreq driver registered.

I think your suggested patch fixes the issue.

As a proper fix I'd like to add a default RAM type for the h3600
but that is for later...

Yours,
Linus Walleij

Re: Regression on cpufreq in v3.12-rc1

[Viresh Kumar]

On 20 September 2013 20:46, Srivatsa S. Bhat
<srivatsa.bhat@linux.vnet.ibm.com> wrote:
> I think show() and store() also suffer
> from a similar fate. So do you think we need to add these checks there as well?
> I'm not sure, since I can't think of a situation in which show() or store()
> can be invoked before the cpufreq-driver is registered.. or, is such a
> situation possible with cpufreq_disabled()?

cpufreq_disabled() is supposed to be called at boot time, so that
cpufreq_core_init() fails.. and so show/store wouldn't exist without
a driver..

Re: Regression on cpufreq in v3.12-rc1

[Viresh Kumar]

On 20 September 2013 22:04, Linus Walleij <linus.walleij@linaro.org> wrote:
> In this case we have neither. So the cpufreq driver fails to
> register. But the kernel used to survive in any case, as you
> could call cpufreq_get() without a cpufreq driver registered.

Aha, so exactly what I suspected in my first mail..

Re: Regression on cpufreq in v3.12-rc1

[Viresh Kumar]

On 20 September 2013 21:09, Linus Walleij <linus.walleij@linaro.org> wrote:
> On Fri, Sep 20, 2013 at 11:35 AM, Viresh Kumar <viresh.kumar@linaro.org> wrote:
>
>> +++ b/drivers/cpufreq/cpufreq.c
>> @@ -1460,6 +1460,9 @@ unsigned int cpufreq_get(unsigned int cpu)
>>  {
>>         unsigned int ret_freq = 0;
>>
>> +       if (cpufreq_disabled() || !cpufreq_driver)
>> +               return -ENOENT;
>> +
>
> But given that a cpufreq driver is just like any other driver, isn't the
> proper thing to do to return -EPROBE_DEFER?

Its not a probe and so that error type doesn't look correct to me..
Also, its only taking care of things when this routine is called without
a cpufreq driver and so it should be fine..

Re: Regression on cpufreq in v3.12-rc1

[Linus Walleij]

On Fri, Sep 20, 2013 at 7:05 PM, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> On 20 September 2013 21:09, Linus Walleij <linus.walleij@linaro.org> wrote:
>> On Fri, Sep 20, 2013 at 11:35 AM, Viresh Kumar <viresh.kumar@linaro.org> wrote:
>>
>>> +++ b/drivers/cpufreq/cpufreq.c
>>> @@ -1460,6 +1460,9 @@ unsigned int cpufreq_get(unsigned int cpu)
>>>  {
>>>         unsigned int ret_freq = 0;
>>>
>>> +       if (cpufreq_disabled() || !cpufreq_driver)
>>> +               return -ENOENT;
>>> +
>>
>> But given that a cpufreq driver is just like any other driver, isn't the
>> proper thing to do to return -EPROBE_DEFER?
>
> Its not a probe and so that error type doesn't look correct to me..
> Also, its only taking care of things when this routine is called without
> a cpufreq driver and so it should be fine..

Well given the use case here I agree, keep with the -ENOENT.

Yours,
Linus Walleij

Re: Regression on cpufreq in v3.12-rc1

[Srivatsa S. Bhat]

On 09/20/2013 10:24 PM, Viresh Kumar wrote:
> On 20 September 2013 20:46, Srivatsa S. Bhat
> <srivatsa.bhat@linux.vnet.ibm.com> wrote:
>> I think show() and store() also suffer
>> from a similar fate. So do you think we need to add these checks there as well?
>> I'm not sure, since I can't think of a situation in which show() or store()
>> can be invoked before the cpufreq-driver is registered.. or, is such a
>> situation possible with cpufreq_disabled()?
> 
> cpufreq_disabled() is supposed to be called at boot time, so that
> cpufreq_core_init() fails.. and so show/store wouldn't exist without
> a driver..
> 

Ah, ok..

Regards,
Srivatsa S. Bhat

Re: Regression on cpufreq in v3.12-rc1

[Srivatsa S. Bhat]

On 09/20/2013 10:31 PM, Viresh Kumar wrote:
> On 20 September 2013 22:04, Linus Walleij <linus.walleij@linaro.org> wrote:
>> In this case we have neither. So the cpufreq driver fails to
>> register. But the kernel used to survive in any case, as you
>> could call cpufreq_get() without a cpufreq driver registered.
> 
> Aha, so exactly what I suspected in my first mail..
> 

Yep, your analysis was perfect right from the beginning :-)

Regards,
Srivatsa S. Bhat