Kernel warning in cpufreq_add_dev()

Kernel warning in cpufreq_add_dev()

[Russell King - ARM Linux]

While checking the kernel on SA1110 Assabet, CPUFREQ issues a warning:

------------[ cut here ]------------
WARNING: CPU: 0 PID: 1 at /home/rmk/git/linux-rmk/drivers/cpufreq/cpufreq.c:1080 cpufreq_add_dev+0x140/0x62c
Modules linked in:
CPU: 0 PID: 1 Comm: swapper Not tainted 4.8.0-rc2+ #883
Hardware name: Intel-Assabet
Backtrace:
[<c0212190>] (dump_backtrace) from [<c021249c>] (show_stack+0x18/0x1c)
 r6:00000000 r5:c05e87c3 r4:00000000
[<c0212484>] (show_stack) from [<c037260c>] (dump_stack+0x20/0x28)
[<c03725ec>] (dump_stack) from [<c021f4cc>] (__warn+0xd0/0xfc)
[<c021f3fc>] (__warn) from [<c021f520>] (warn_slowpath_null+0x28/0x30)
 r10:00000000 r8:00000000 r7:00000000 r6:c064525c r5:00000000 r4:00000000
[<c021f4f8>] (warn_slowpath_null) from [<c04343a8>] (cpufreq_add_dev+0x140/0x62c)
[<c0434268>] (cpufreq_add_dev) from [<c03d83f4>] (bus_probe_device+0x5c/0x84)
 r10:00000000 r8:00000000 r7:00000000 r6:c064525c r5:c0657d60 r4:c065a9f8
[<c03d8398>] (bus_probe_device) from [<c03d677c>] (device_add+0x390/0x520)
 r6:c0645264 r5:00000000 r4:c064525c
[<c03d63ec>] (device_add) from [<c03d6a90>] (device_register+0x1c/0x20)
 r10:c0639848 r8:c061e524 r7:00000001 r6:00000000 r5:c064525c r4:c064525c
[<c03d6a74>] (device_register) from [<c03db5a0>] (register_cpu+0x88/0xac)
 r4:c0645254
[<c03db518>] (register_cpu) from [<c061e544>] (topology_init+0x20/0x2c)
 r7:c0660b20 r6:c063f4a0 r5:c0639834 r4:00000000
[<c061e524>] (topology_init) from [<c020974c>] (do_one_initcall+0xc0/0x178)
 r4:00000004
[<c020968c>] (do_one_initcall) from [<c061be70>] (kernel_init_freeable+0xfc/0x1c4)
 r10:c0639848 r9:00000000 r8:00000088 r7:c0660b20 r6:c063f4a0 r5:c0639834
 r4:00000004
[<c061bd74>] (kernel_init_freeable) from [<c050d730>] (kernel_init+0x10/0xf4)
 r10:00000000 r8:00000000 r7:00000000 r6:00000000 r5:c050d720 r4:00000000
[<c050d720>] (kernel_init) from [<c020fcf0>] (ret_from_fork+0x14/0x24)
 r4:00000000
---[ end trace df94656649275917 ]---

This is because of an incompatibility between the expectations of cpufreq
and how register_cpu() works:

int register_cpu(struct cpu *cpu, int num)
{
...
        error = device_register(&cpu->dev);
        if (!error)
                per_cpu(cpu_sys_devices, num) = &cpu->dev;

When the device is registered via device_register(), any subsystems
registered for the cpu_subsys will have their "add_dev" method called.

The cpufreq add_dev, via cpufreq_online() and cpufreq_policy_alloc(),
tries to get the CPU device:

static struct cpufreq_policy *cpufreq_policy_alloc(unsigned int cpu)
{
        struct device *dev = get_cpu_device(cpu);
        if (WARN_ON(!dev))
                return NULL;

but this fails:

struct device *get_cpu_device(unsigned cpu)
{
        if (cpu < nr_cpu_ids && cpu_possible(cpu))
                return per_cpu(cpu_sys_devices, cpu);

because the percpu data has not yet been written - it'll be written
after a successful device registration.  So, using get_cpu_device()
from within cpufreq_add_dev() is broken, and results in the above
kernel warning.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

Kernel warning in cpufreq_add_dev()

[Russell King - ARM Linux]

While checking the kernel on SA1110 Assabet, CPUFREQ issues a warning:

------------[ cut here ]------------
WARNING: CPU: 0 PID: 1 at /home/rmk/git/linux-rmk/drivers/cpufreq/cpufreq.c:1080 cpufreq_add_dev+0x140/0x62c
Modules linked in:
CPU: 0 PID: 1 Comm: swapper Not tainted 4.8.0-rc2+ #883
Hardware name: Intel-Assabet
Backtrace:
[<c0212190>] (dump_backtrace) from [<c021249c>] (show_stack+0x18/0x1c)
 r6:00000000 r5:c05e87c3 r4:00000000
[<c0212484>] (show_stack) from [<c037260c>] (dump_stack+0x20/0x28)
[<c03725ec>] (dump_stack) from [<c021f4cc>] (__warn+0xd0/0xfc)
[<c021f3fc>] (__warn) from [<c021f520>] (warn_slowpath_null+0x28/0x30)
 r10:00000000 r8:00000000 r7:00000000 r6:c064525c r5:00000000 r4:00000000
[<c021f4f8>] (warn_slowpath_null) from [<c04343a8>] (cpufreq_add_dev+0x140/0x62c)
[<c0434268>] (cpufreq_add_dev) from [<c03d83f4>] (bus_probe_device+0x5c/0x84)
 r10:00000000 r8:00000000 r7:00000000 r6:c064525c r5:c0657d60 r4:c065a9f8
[<c03d8398>] (bus_probe_device) from [<c03d677c>] (device_add+0x390/0x520)
 r6:c0645264 r5:00000000 r4:c064525c
[<c03d63ec>] (device_add) from [<c03d6a90>] (device_register+0x1c/0x20)
 r10:c0639848 r8:c061e524 r7:00000001 r6:00000000 r5:c064525c r4:c064525c
[<c03d6a74>] (device_register) from [<c03db5a0>] (register_cpu+0x88/0xac)
 r4:c0645254
[<c03db518>] (register_cpu) from [<c061e544>] (topology_init+0x20/0x2c)
 r7:c0660b20 r6:c063f4a0 r5:c0639834 r4:00000000
[<c061e524>] (topology_init) from [<c020974c>] (do_one_initcall+0xc0/0x178)
 r4:00000004
[<c020968c>] (do_one_initcall) from [<c061be70>] (kernel_init_freeable+0xfc/0x1c4)
 r10:c0639848 r9:00000000 r8:00000088 r7:c0660b20 r6:c063f4a0 r5:c0639834
 r4:00000004
[<c061bd74>] (kernel_init_freeable) from [<c050d730>] (kernel_init+0x10/0xf4)
 r10:00000000 r8:00000000 r7:00000000 r6:00000000 r5:c050d720 r4:00000000
[<c050d720>] (kernel_init) from [<c020fcf0>] (ret_from_fork+0x14/0x24)
 r4:00000000
---[ end trace df94656649275917 ]---

This is because of an incompatibility between the expectations of cpufreq
and how register_cpu() works:

int register_cpu(struct cpu *cpu, int num)
{
...
        error = device_register(&cpu->dev);
        if (!error)
                per_cpu(cpu_sys_devices, num) = &cpu->dev;

When the device is registered via device_register(), any subsystems
registered for the cpu_subsys will have their "add_dev" method called.

The cpufreq add_dev, via cpufreq_online() and cpufreq_policy_alloc(),
tries to get the CPU device:

static struct cpufreq_policy *cpufreq_policy_alloc(unsigned int cpu)
{
        struct device *dev = get_cpu_device(cpu);
        if (WARN_ON(!dev))
                return NULL;

but this fails:

struct device *get_cpu_device(unsigned cpu)
{
        if (cpu < nr_cpu_ids && cpu_possible(cpu))
                return per_cpu(cpu_sys_devices, cpu);

because the percpu data has not yet been written - it'll be written
after a successful device registration.  So, using get_cpu_device()
from within cpufreq_add_dev() is broken, and results in the above
kernel warning.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently@9.6Mbps down 400kbps up
according to speedtest.net.

Re: Kernel warning in cpufreq_add_dev()

[Rafael J. Wysocki]

On Friday, August 19, 2016 12:00:32 PM Russell King - ARM Linux wrote:
> While checking the kernel on SA1110 Assabet, CPUFREQ issues a warning:
> 
> ------------[ cut here ]------------
> WARNING: CPU: 0 PID: 1 at /home/rmk/git/linux-rmk/drivers/cpufreq/cpufreq.c:1080 cpufreq_add_dev+0x140/0x62c
> Modules linked in:
> CPU: 0 PID: 1 Comm: swapper Not tainted 4.8.0-rc2+ #883
> Hardware name: Intel-Assabet
> Backtrace:
> [<c0212190>] (dump_backtrace) from [<c021249c>] (show_stack+0x18/0x1c)
>  r6:00000000 r5:c05e87c3 r4:00000000
> [<c0212484>] (show_stack) from [<c037260c>] (dump_stack+0x20/0x28)
> [<c03725ec>] (dump_stack) from [<c021f4cc>] (__warn+0xd0/0xfc)
> [<c021f3fc>] (__warn) from [<c021f520>] (warn_slowpath_null+0x28/0x30)
>  r10:00000000 r8:00000000 r7:00000000 r6:c064525c r5:00000000 r4:00000000
> [<c021f4f8>] (warn_slowpath_null) from [<c04343a8>] (cpufreq_add_dev+0x140/0x62c)
> [<c0434268>] (cpufreq_add_dev) from [<c03d83f4>] (bus_probe_device+0x5c/0x84)
>  r10:00000000 r8:00000000 r7:00000000 r6:c064525c r5:c0657d60 r4:c065a9f8
> [<c03d8398>] (bus_probe_device) from [<c03d677c>] (device_add+0x390/0x520)
>  r6:c0645264 r5:00000000 r4:c064525c
> [<c03d63ec>] (device_add) from [<c03d6a90>] (device_register+0x1c/0x20)
>  r10:c0639848 r8:c061e524 r7:00000001 r6:00000000 r5:c064525c r4:c064525c
> [<c03d6a74>] (device_register) from [<c03db5a0>] (register_cpu+0x88/0xac)
>  r4:c0645254
> [<c03db518>] (register_cpu) from [<c061e544>] (topology_init+0x20/0x2c)
>  r7:c0660b20 r6:c063f4a0 r5:c0639834 r4:00000000
> [<c061e524>] (topology_init) from [<c020974c>] (do_one_initcall+0xc0/0x178)
>  r4:00000004
> [<c020968c>] (do_one_initcall) from [<c061be70>] (kernel_init_freeable+0xfc/0x1c4)
>  r10:c0639848 r9:00000000 r8:00000088 r7:c0660b20 r6:c063f4a0 r5:c0639834
>  r4:00000004
> [<c061bd74>] (kernel_init_freeable) from [<c050d730>] (kernel_init+0x10/0xf4)
>  r10:00000000 r8:00000000 r7:00000000 r6:00000000 r5:c050d720 r4:00000000
> [<c050d720>] (kernel_init) from [<c020fcf0>] (ret_from_fork+0x14/0x24)
>  r4:00000000
> ---[ end trace df94656649275917 ]---
> 
> This is because of an incompatibility between the expectations of cpufreq
> and how register_cpu() works:
> 
> int register_cpu(struct cpu *cpu, int num)
> {
> ...
>         error = device_register(&cpu->dev);
>         if (!error)
>                 per_cpu(cpu_sys_devices, num) = &cpu->dev;
> 
> When the device is registered via device_register(), any subsystems
> registered for the cpu_subsys will have their "add_dev" method called.
> 
> The cpufreq add_dev, via cpufreq_online() and cpufreq_policy_alloc(),
> tries to get the CPU device:
> 
> static struct cpufreq_policy *cpufreq_policy_alloc(unsigned int cpu)
> {
>         struct device *dev = get_cpu_device(cpu);
>         if (WARN_ON(!dev))
>                 return NULL;
> 
> but this fails:
> 
> struct device *get_cpu_device(unsigned cpu)
> {
>         if (cpu < nr_cpu_ids && cpu_possible(cpu))
>                 return per_cpu(cpu_sys_devices, cpu);
> 
> because the percpu data has not yet been written - it'll be written
> after a successful device registration.  So, using get_cpu_device()
> from within cpufreq_add_dev() is broken, and results in the above
> kernel warning.

Ironically enough, cpufreq_policy_alloc() doesn't even use the value of dev
for anything other than the check, so we can simply get rid of it (as per the
appended patch).

add_cpu_dev_symlink() will still be problematic, though, if I'm not mistaken.

I wonder how that works on other platforms.

---
 drivers/cpufreq/cpufreq.c |    4 ----
 1 file changed, 4 deletions(-)

Index: linux-pm/drivers/cpufreq/cpufreq.c
===================================================================
--- linux-pm.orig/drivers/cpufreq/cpufreq.c
+++ linux-pm/drivers/cpufreq/cpufreq.c
@@ -1073,13 +1073,9 @@ static void handle_update(struct work_st
 
 static struct cpufreq_policy *cpufreq_policy_alloc(unsigned int cpu)
 {
-	struct device *dev = get_cpu_device(cpu);
 	struct cpufreq_policy *policy;
 	int ret;
 
-	if (WARN_ON(!dev))
-		return NULL;
-
 	policy = kzalloc(sizeof(*policy), GFP_KERNEL);
 	if (!policy)
 		return NULL;

Kernel warning in cpufreq_add_dev()

[Rafael J. Wysocki]

On Friday, August 19, 2016 12:00:32 PM Russell King - ARM Linux wrote:
> While checking the kernel on SA1110 Assabet, CPUFREQ issues a warning:
> 
> ------------[ cut here ]------------
> WARNING: CPU: 0 PID: 1 at /home/rmk/git/linux-rmk/drivers/cpufreq/cpufreq.c:1080 cpufreq_add_dev+0x140/0x62c
> Modules linked in:
> CPU: 0 PID: 1 Comm: swapper Not tainted 4.8.0-rc2+ #883
> Hardware name: Intel-Assabet
> Backtrace:
> [<c0212190>] (dump_backtrace) from [<c021249c>] (show_stack+0x18/0x1c)
>  r6:00000000 r5:c05e87c3 r4:00000000
> [<c0212484>] (show_stack) from [<c037260c>] (dump_stack+0x20/0x28)
> [<c03725ec>] (dump_stack) from [<c021f4cc>] (__warn+0xd0/0xfc)
> [<c021f3fc>] (__warn) from [<c021f520>] (warn_slowpath_null+0x28/0x30)
>  r10:00000000 r8:00000000 r7:00000000 r6:c064525c r5:00000000 r4:00000000
> [<c021f4f8>] (warn_slowpath_null) from [<c04343a8>] (cpufreq_add_dev+0x140/0x62c)
> [<c0434268>] (cpufreq_add_dev) from [<c03d83f4>] (bus_probe_device+0x5c/0x84)
>  r10:00000000 r8:00000000 r7:00000000 r6:c064525c r5:c0657d60 r4:c065a9f8
> [<c03d8398>] (bus_probe_device) from [<c03d677c>] (device_add+0x390/0x520)
>  r6:c0645264 r5:00000000 r4:c064525c
> [<c03d63ec>] (device_add) from [<c03d6a90>] (device_register+0x1c/0x20)
>  r10:c0639848 r8:c061e524 r7:00000001 r6:00000000 r5:c064525c r4:c064525c
> [<c03d6a74>] (device_register) from [<c03db5a0>] (register_cpu+0x88/0xac)
>  r4:c0645254
> [<c03db518>] (register_cpu) from [<c061e544>] (topology_init+0x20/0x2c)
>  r7:c0660b20 r6:c063f4a0 r5:c0639834 r4:00000000
> [<c061e524>] (topology_init) from [<c020974c>] (do_one_initcall+0xc0/0x178)
>  r4:00000004
> [<c020968c>] (do_one_initcall) from [<c061be70>] (kernel_init_freeable+0xfc/0x1c4)
>  r10:c0639848 r9:00000000 r8:00000088 r7:c0660b20 r6:c063f4a0 r5:c0639834
>  r4:00000004
> [<c061bd74>] (kernel_init_freeable) from [<c050d730>] (kernel_init+0x10/0xf4)
>  r10:00000000 r8:00000000 r7:00000000 r6:00000000 r5:c050d720 r4:00000000
> [<c050d720>] (kernel_init) from [<c020fcf0>] (ret_from_fork+0x14/0x24)
>  r4:00000000
> ---[ end trace df94656649275917 ]---
> 
> This is because of an incompatibility between the expectations of cpufreq
> and how register_cpu() works:
> 
> int register_cpu(struct cpu *cpu, int num)
> {
> ...
>         error = device_register(&cpu->dev);
>         if (!error)
>                 per_cpu(cpu_sys_devices, num) = &cpu->dev;
> 
> When the device is registered via device_register(), any subsystems
> registered for the cpu_subsys will have their "add_dev" method called.
> 
> The cpufreq add_dev, via cpufreq_online() and cpufreq_policy_alloc(),
> tries to get the CPU device:
> 
> static struct cpufreq_policy *cpufreq_policy_alloc(unsigned int cpu)
> {
>         struct device *dev = get_cpu_device(cpu);
>         if (WARN_ON(!dev))
>                 return NULL;
> 
> but this fails:
> 
> struct device *get_cpu_device(unsigned cpu)
> {
>         if (cpu < nr_cpu_ids && cpu_possible(cpu))
>                 return per_cpu(cpu_sys_devices, cpu);
> 
> because the percpu data has not yet been written - it'll be written
> after a successful device registration.  So, using get_cpu_device()
> from within cpufreq_add_dev() is broken, and results in the above
> kernel warning.

Ironically enough, cpufreq_policy_alloc() doesn't even use the value of dev
for anything other than the check, so we can simply get rid of it (as per the
appended patch).

add_cpu_dev_symlink() will still be problematic, though, if I'm not mistaken.

I wonder how that works on other platforms.

---
 drivers/cpufreq/cpufreq.c |    4 ----
 1 file changed, 4 deletions(-)

Index: linux-pm/drivers/cpufreq/cpufreq.c
===================================================================
--- linux-pm.orig/drivers/cpufreq/cpufreq.c
+++ linux-pm/drivers/cpufreq/cpufreq.c
@@ -1073,13 +1073,9 @@ static void handle_update(struct work_st
 
 static struct cpufreq_policy *cpufreq_policy_alloc(unsigned int cpu)
 {
-	struct device *dev = get_cpu_device(cpu);
 	struct cpufreq_policy *policy;
 	int ret;
 
-	if (WARN_ON(!dev))
-		return NULL;
-
 	policy = kzalloc(sizeof(*policy), GFP_KERNEL);
 	if (!policy)
 		return NULL;

Re: Kernel warning in cpufreq_add_dev()

[Viresh Kumar]

On 20-08-16, 03:29, Rafael J. Wysocki wrote:
> On Friday, August 19, 2016 12:00:32 PM Russell King - ARM Linux wrote:
> > While checking the kernel on SA1110 Assabet, CPUFREQ issues a warning:
> > 
> > ------------[ cut here ]------------
> > WARNING: CPU: 0 PID: 1 at /home/rmk/git/linux-rmk/drivers/cpufreq/cpufreq.c:1080 cpufreq_add_dev+0x140/0x62c
> > Modules linked in:
> > CPU: 0 PID: 1 Comm: swapper Not tainted 4.8.0-rc2+ #883
> > Hardware name: Intel-Assabet
> > Backtrace:
> > [<c0212190>] (dump_backtrace) from [<c021249c>] (show_stack+0x18/0x1c)
> >  r6:00000000 r5:c05e87c3 r4:00000000
> > [<c0212484>] (show_stack) from [<c037260c>] (dump_stack+0x20/0x28)
> > [<c03725ec>] (dump_stack) from [<c021f4cc>] (__warn+0xd0/0xfc)
> > [<c021f3fc>] (__warn) from [<c021f520>] (warn_slowpath_null+0x28/0x30)
> >  r10:00000000 r8:00000000 r7:00000000 r6:c064525c r5:00000000 r4:00000000
> > [<c021f4f8>] (warn_slowpath_null) from [<c04343a8>] (cpufreq_add_dev+0x140/0x62c)
> > [<c0434268>] (cpufreq_add_dev) from [<c03d83f4>] (bus_probe_device+0x5c/0x84)
> >  r10:00000000 r8:00000000 r7:00000000 r6:c064525c r5:c0657d60 r4:c065a9f8
> > [<c03d8398>] (bus_probe_device) from [<c03d677c>] (device_add+0x390/0x520)
> >  r6:c0645264 r5:00000000 r4:c064525c
> > [<c03d63ec>] (device_add) from [<c03d6a90>] (device_register+0x1c/0x20)
> >  r10:c0639848 r8:c061e524 r7:00000001 r6:00000000 r5:c064525c r4:c064525c
> > [<c03d6a74>] (device_register) from [<c03db5a0>] (register_cpu+0x88/0xac)
> >  r4:c0645254
> > [<c03db518>] (register_cpu) from [<c061e544>] (topology_init+0x20/0x2c)
> >  r7:c0660b20 r6:c063f4a0 r5:c0639834 r4:00000000
> > [<c061e524>] (topology_init) from [<c020974c>] (do_one_initcall+0xc0/0x178)
> >  r4:00000004
> > [<c020968c>] (do_one_initcall) from [<c061be70>] (kernel_init_freeable+0xfc/0x1c4)
> >  r10:c0639848 r9:00000000 r8:00000088 r7:c0660b20 r6:c063f4a0 r5:c0639834
> >  r4:00000004
> > [<c061bd74>] (kernel_init_freeable) from [<c050d730>] (kernel_init+0x10/0xf4)
> >  r10:00000000 r8:00000000 r7:00000000 r6:00000000 r5:c050d720 r4:00000000
> > [<c050d720>] (kernel_init) from [<c020fcf0>] (ret_from_fork+0x14/0x24)
> >  r4:00000000
> > ---[ end trace df94656649275917 ]---
> > 
> > This is because of an incompatibility between the expectations of cpufreq
> > and how register_cpu() works:
> > 
> > int register_cpu(struct cpu *cpu, int num)
> > {
> > ...
> >         error = device_register(&cpu->dev);
> >         if (!error)
> >                 per_cpu(cpu_sys_devices, num) = &cpu->dev;
> > 
> > When the device is registered via device_register(), any subsystems
> > registered for the cpu_subsys will have their "add_dev" method called.
> > 
> > The cpufreq add_dev, via cpufreq_online() and cpufreq_policy_alloc(),
> > tries to get the CPU device:
> > 
> > static struct cpufreq_policy *cpufreq_policy_alloc(unsigned int cpu)
> > {
> >         struct device *dev = get_cpu_device(cpu);
> >         if (WARN_ON(!dev))
> >                 return NULL;
> > 
> > but this fails:
> > 
> > struct device *get_cpu_device(unsigned cpu)
> > {
> >         if (cpu < nr_cpu_ids && cpu_possible(cpu))
> >                 return per_cpu(cpu_sys_devices, cpu);
> > 
> > because the percpu data has not yet been written - it'll be written
> > after a successful device registration.  So, using get_cpu_device()
> > from within cpufreq_add_dev() is broken, and results in the above
> > kernel warning.

Hmm, I am wondering why is your case special here and why we never saw the same
behavior ? Is this because the driver is registered as arch_initcall() ?

In all the cases that I have seen at least, cpufreq_add_dev() doesn't get called
via the path you mentioned, but only during the cpufreq driver is registered.

> Ironically enough, cpufreq_policy_alloc() doesn't even use the value of dev
> for anything other than the check, so we can simply get rid of it (as per the
> appended patch).
> 
> add_cpu_dev_symlink() will still be problematic, though, if I'm not mistaken.

Right, it will be. We try to create links for all the *real* (currently plugged
in) CPUs on policy creation and that needs the kobjects of those devices.

-- 
viresh

Kernel warning in cpufreq_add_dev()

[Viresh Kumar]

On 20-08-16, 03:29, Rafael J. Wysocki wrote:
> On Friday, August 19, 2016 12:00:32 PM Russell King - ARM Linux wrote:
> > While checking the kernel on SA1110 Assabet, CPUFREQ issues a warning:
> > 
> > ------------[ cut here ]------------
> > WARNING: CPU: 0 PID: 1 at /home/rmk/git/linux-rmk/drivers/cpufreq/cpufreq.c:1080 cpufreq_add_dev+0x140/0x62c
> > Modules linked in:
> > CPU: 0 PID: 1 Comm: swapper Not tainted 4.8.0-rc2+ #883
> > Hardware name: Intel-Assabet
> > Backtrace:
> > [<c0212190>] (dump_backtrace) from [<c021249c>] (show_stack+0x18/0x1c)
> >  r6:00000000 r5:c05e87c3 r4:00000000
> > [<c0212484>] (show_stack) from [<c037260c>] (dump_stack+0x20/0x28)
> > [<c03725ec>] (dump_stack) from [<c021f4cc>] (__warn+0xd0/0xfc)
> > [<c021f3fc>] (__warn) from [<c021f520>] (warn_slowpath_null+0x28/0x30)
> >  r10:00000000 r8:00000000 r7:00000000 r6:c064525c r5:00000000 r4:00000000
> > [<c021f4f8>] (warn_slowpath_null) from [<c04343a8>] (cpufreq_add_dev+0x140/0x62c)
> > [<c0434268>] (cpufreq_add_dev) from [<c03d83f4>] (bus_probe_device+0x5c/0x84)
> >  r10:00000000 r8:00000000 r7:00000000 r6:c064525c r5:c0657d60 r4:c065a9f8
> > [<c03d8398>] (bus_probe_device) from [<c03d677c>] (device_add+0x390/0x520)
> >  r6:c0645264 r5:00000000 r4:c064525c
> > [<c03d63ec>] (device_add) from [<c03d6a90>] (device_register+0x1c/0x20)
> >  r10:c0639848 r8:c061e524 r7:00000001 r6:00000000 r5:c064525c r4:c064525c
> > [<c03d6a74>] (device_register) from [<c03db5a0>] (register_cpu+0x88/0xac)
> >  r4:c0645254
> > [<c03db518>] (register_cpu) from [<c061e544>] (topology_init+0x20/0x2c)
> >  r7:c0660b20 r6:c063f4a0 r5:c0639834 r4:00000000
> > [<c061e524>] (topology_init) from [<c020974c>] (do_one_initcall+0xc0/0x178)
> >  r4:00000004
> > [<c020968c>] (do_one_initcall) from [<c061be70>] (kernel_init_freeable+0xfc/0x1c4)
> >  r10:c0639848 r9:00000000 r8:00000088 r7:c0660b20 r6:c063f4a0 r5:c0639834
> >  r4:00000004
> > [<c061bd74>] (kernel_init_freeable) from [<c050d730>] (kernel_init+0x10/0xf4)
> >  r10:00000000 r8:00000000 r7:00000000 r6:00000000 r5:c050d720 r4:00000000
> > [<c050d720>] (kernel_init) from [<c020fcf0>] (ret_from_fork+0x14/0x24)
> >  r4:00000000
> > ---[ end trace df94656649275917 ]---
> > 
> > This is because of an incompatibility between the expectations of cpufreq
> > and how register_cpu() works:
> > 
> > int register_cpu(struct cpu *cpu, int num)
> > {
> > ...
> >         error = device_register(&cpu->dev);
> >         if (!error)
> >                 per_cpu(cpu_sys_devices, num) = &cpu->dev;
> > 
> > When the device is registered via device_register(), any subsystems
> > registered for the cpu_subsys will have their "add_dev" method called.
> > 
> > The cpufreq add_dev, via cpufreq_online() and cpufreq_policy_alloc(),
> > tries to get the CPU device:
> > 
> > static struct cpufreq_policy *cpufreq_policy_alloc(unsigned int cpu)
> > {
> >         struct device *dev = get_cpu_device(cpu);
> >         if (WARN_ON(!dev))
> >                 return NULL;
> > 
> > but this fails:
> > 
> > struct device *get_cpu_device(unsigned cpu)
> > {
> >         if (cpu < nr_cpu_ids && cpu_possible(cpu))
> >                 return per_cpu(cpu_sys_devices, cpu);
> > 
> > because the percpu data has not yet been written - it'll be written
> > after a successful device registration.  So, using get_cpu_device()
> > from within cpufreq_add_dev() is broken, and results in the above
> > kernel warning.

Hmm, I am wondering why is your case special here and why we never saw the same
behavior ? Is this because the driver is registered as arch_initcall() ?

In all the cases that I have seen at least, cpufreq_add_dev() doesn't get called
via the path you mentioned, but only during the cpufreq driver is registered.

> Ironically enough, cpufreq_policy_alloc() doesn't even use the value of dev
> for anything other than the check, so we can simply get rid of it (as per the
> appended patch).
> 
> add_cpu_dev_symlink() will still be problematic, though, if I'm not mistaken.

Right, it will be. We try to create links for all the *real* (currently plugged
in) CPUs on policy creation and that needs the kobjects of those devices.

-- 
viresh

Re: Kernel warning in cpufreq_add_dev()

[Rafael J. Wysocki]

On Saturday, August 20, 2016 07:16:34 AM Viresh Kumar wrote:
> On 20-08-16, 03:29, Rafael J. Wysocki wrote:
> > On Friday, August 19, 2016 12:00:32 PM Russell King - ARM Linux wrote:
> > > While checking the kernel on SA1110 Assabet, CPUFREQ issues a warning:
> > > 
> > > ------------[ cut here ]------------
> > > WARNING: CPU: 0 PID: 1 at /home/rmk/git/linux-rmk/drivers/cpufreq/cpufreq.c:1080 cpufreq_add_dev+0x140/0x62c
> > > Modules linked in:
> > > CPU: 0 PID: 1 Comm: swapper Not tainted 4.8.0-rc2+ #883
> > > Hardware name: Intel-Assabet
> > > Backtrace:
> > > [<c0212190>] (dump_backtrace) from [<c021249c>] (show_stack+0x18/0x1c)
> > >  r6:00000000 r5:c05e87c3 r4:00000000
> > > [<c0212484>] (show_stack) from [<c037260c>] (dump_stack+0x20/0x28)
> > > [<c03725ec>] (dump_stack) from [<c021f4cc>] (__warn+0xd0/0xfc)
> > > [<c021f3fc>] (__warn) from [<c021f520>] (warn_slowpath_null+0x28/0x30)
> > >  r10:00000000 r8:00000000 r7:00000000 r6:c064525c r5:00000000 r4:00000000
> > > [<c021f4f8>] (warn_slowpath_null) from [<c04343a8>] (cpufreq_add_dev+0x140/0x62c)
> > > [<c0434268>] (cpufreq_add_dev) from [<c03d83f4>] (bus_probe_device+0x5c/0x84)
> > >  r10:00000000 r8:00000000 r7:00000000 r6:c064525c r5:c0657d60 r4:c065a9f8
> > > [<c03d8398>] (bus_probe_device) from [<c03d677c>] (device_add+0x390/0x520)
> > >  r6:c0645264 r5:00000000 r4:c064525c
> > > [<c03d63ec>] (device_add) from [<c03d6a90>] (device_register+0x1c/0x20)
> > >  r10:c0639848 r8:c061e524 r7:00000001 r6:00000000 r5:c064525c r4:c064525c
> > > [<c03d6a74>] (device_register) from [<c03db5a0>] (register_cpu+0x88/0xac)
> > >  r4:c0645254
> > > [<c03db518>] (register_cpu) from [<c061e544>] (topology_init+0x20/0x2c)
> > >  r7:c0660b20 r6:c063f4a0 r5:c0639834 r4:00000000
> > > [<c061e524>] (topology_init) from [<c020974c>] (do_one_initcall+0xc0/0x178)
> > >  r4:00000004
> > > [<c020968c>] (do_one_initcall) from [<c061be70>] (kernel_init_freeable+0xfc/0x1c4)
> > >  r10:c0639848 r9:00000000 r8:00000088 r7:c0660b20 r6:c063f4a0 r5:c0639834
> > >  r4:00000004
> > > [<c061bd74>] (kernel_init_freeable) from [<c050d730>] (kernel_init+0x10/0xf4)
> > >  r10:00000000 r8:00000000 r7:00000000 r6:00000000 r5:c050d720 r4:00000000
> > > [<c050d720>] (kernel_init) from [<c020fcf0>] (ret_from_fork+0x14/0x24)
> > >  r4:00000000
> > > ---[ end trace df94656649275917 ]---
> > > 
> > > This is because of an incompatibility between the expectations of cpufreq
> > > and how register_cpu() works:
> > > 
> > > int register_cpu(struct cpu *cpu, int num)
> > > {
> > > ...
> > >         error = device_register(&cpu->dev);
> > >         if (!error)
> > >                 per_cpu(cpu_sys_devices, num) = &cpu->dev;
> > > 
> > > When the device is registered via device_register(), any subsystems
> > > registered for the cpu_subsys will have their "add_dev" method called.
> > > 
> > > The cpufreq add_dev, via cpufreq_online() and cpufreq_policy_alloc(),
> > > tries to get the CPU device:
> > > 
> > > static struct cpufreq_policy *cpufreq_policy_alloc(unsigned int cpu)
> > > {
> > >         struct device *dev = get_cpu_device(cpu);
> > >         if (WARN_ON(!dev))
> > >                 return NULL;
> > > 
> > > but this fails:
> > > 
> > > struct device *get_cpu_device(unsigned cpu)
> > > {
> > >         if (cpu < nr_cpu_ids && cpu_possible(cpu))
> > >                 return per_cpu(cpu_sys_devices, cpu);
> > > 
> > > because the percpu data has not yet been written - it'll be written
> > > after a successful device registration.  So, using get_cpu_device()
> > > from within cpufreq_add_dev() is broken, and results in the above
> > > kernel warning.
> 
> Hmm, I am wondering why is your case special here and why we never saw the same
> behavior ? Is this because the driver is registered as arch_initcall() ?
> 
> In all the cases that I have seen at least, cpufreq_add_dev() doesn't get called
> via the path you mentioned, but only during the cpufreq driver is registered.

But it will be called in that path during physical CPU hot-add, won't it?

Thanks,
Rafael

Kernel warning in cpufreq_add_dev()

[Rafael J. Wysocki]

On Saturday, August 20, 2016 07:16:34 AM Viresh Kumar wrote:
> On 20-08-16, 03:29, Rafael J. Wysocki wrote:
> > On Friday, August 19, 2016 12:00:32 PM Russell King - ARM Linux wrote:
> > > While checking the kernel on SA1110 Assabet, CPUFREQ issues a warning:
> > > 
> > > ------------[ cut here ]------------
> > > WARNING: CPU: 0 PID: 1 at /home/rmk/git/linux-rmk/drivers/cpufreq/cpufreq.c:1080 cpufreq_add_dev+0x140/0x62c
> > > Modules linked in:
> > > CPU: 0 PID: 1 Comm: swapper Not tainted 4.8.0-rc2+ #883
> > > Hardware name: Intel-Assabet
> > > Backtrace:
> > > [<c0212190>] (dump_backtrace) from [<c021249c>] (show_stack+0x18/0x1c)
> > >  r6:00000000 r5:c05e87c3 r4:00000000
> > > [<c0212484>] (show_stack) from [<c037260c>] (dump_stack+0x20/0x28)
> > > [<c03725ec>] (dump_stack) from [<c021f4cc>] (__warn+0xd0/0xfc)
> > > [<c021f3fc>] (__warn) from [<c021f520>] (warn_slowpath_null+0x28/0x30)
> > >  r10:00000000 r8:00000000 r7:00000000 r6:c064525c r5:00000000 r4:00000000
> > > [<c021f4f8>] (warn_slowpath_null) from [<c04343a8>] (cpufreq_add_dev+0x140/0x62c)
> > > [<c0434268>] (cpufreq_add_dev) from [<c03d83f4>] (bus_probe_device+0x5c/0x84)
> > >  r10:00000000 r8:00000000 r7:00000000 r6:c064525c r5:c0657d60 r4:c065a9f8
> > > [<c03d8398>] (bus_probe_device) from [<c03d677c>] (device_add+0x390/0x520)
> > >  r6:c0645264 r5:00000000 r4:c064525c
> > > [<c03d63ec>] (device_add) from [<c03d6a90>] (device_register+0x1c/0x20)
> > >  r10:c0639848 r8:c061e524 r7:00000001 r6:00000000 r5:c064525c r4:c064525c
> > > [<c03d6a74>] (device_register) from [<c03db5a0>] (register_cpu+0x88/0xac)
> > >  r4:c0645254
> > > [<c03db518>] (register_cpu) from [<c061e544>] (topology_init+0x20/0x2c)
> > >  r7:c0660b20 r6:c063f4a0 r5:c0639834 r4:00000000
> > > [<c061e524>] (topology_init) from [<c020974c>] (do_one_initcall+0xc0/0x178)
> > >  r4:00000004
> > > [<c020968c>] (do_one_initcall) from [<c061be70>] (kernel_init_freeable+0xfc/0x1c4)
> > >  r10:c0639848 r9:00000000 r8:00000088 r7:c0660b20 r6:c063f4a0 r5:c0639834
> > >  r4:00000004
> > > [<c061bd74>] (kernel_init_freeable) from [<c050d730>] (kernel_init+0x10/0xf4)
> > >  r10:00000000 r8:00000000 r7:00000000 r6:00000000 r5:c050d720 r4:00000000
> > > [<c050d720>] (kernel_init) from [<c020fcf0>] (ret_from_fork+0x14/0x24)
> > >  r4:00000000
> > > ---[ end trace df94656649275917 ]---
> > > 
> > > This is because of an incompatibility between the expectations of cpufreq
> > > and how register_cpu() works:
> > > 
> > > int register_cpu(struct cpu *cpu, int num)
> > > {
> > > ...
> > >         error = device_register(&cpu->dev);
> > >         if (!error)
> > >                 per_cpu(cpu_sys_devices, num) = &cpu->dev;
> > > 
> > > When the device is registered via device_register(), any subsystems
> > > registered for the cpu_subsys will have their "add_dev" method called.
> > > 
> > > The cpufreq add_dev, via cpufreq_online() and cpufreq_policy_alloc(),
> > > tries to get the CPU device:
> > > 
> > > static struct cpufreq_policy *cpufreq_policy_alloc(unsigned int cpu)
> > > {
> > >         struct device *dev = get_cpu_device(cpu);
> > >         if (WARN_ON(!dev))
> > >                 return NULL;
> > > 
> > > but this fails:
> > > 
> > > struct device *get_cpu_device(unsigned cpu)
> > > {
> > >         if (cpu < nr_cpu_ids && cpu_possible(cpu))
> > >                 return per_cpu(cpu_sys_devices, cpu);
> > > 
> > > because the percpu data has not yet been written - it'll be written
> > > after a successful device registration.  So, using get_cpu_device()
> > > from within cpufreq_add_dev() is broken, and results in the above
> > > kernel warning.
> 
> Hmm, I am wondering why is your case special here and why we never saw the same
> behavior ? Is this because the driver is registered as arch_initcall() ?
> 
> In all the cases that I have seen at least, cpufreq_add_dev() doesn't get called
> via the path you mentioned, but only during the cpufreq driver is registered.

But it will be called in that path during physical CPU hot-add, won't it?

Thanks,
Rafael

Re: Kernel warning in cpufreq_add_dev()

[Viresh Kumar]

On 22-08-16, 19:32, Rafael J. Wysocki wrote:
> But it will be called in that path during physical CPU hot-add, won't it?

What about something like this instead (completely untested) ?

@Russell: Can you please try this ??

-- 
viresh

-------------------------8<-------------------------

diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index 3dd4884c6f9e..a702d6246385 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -916,20 +916,11 @@ static struct kobj_type ktype_cpufreq = {
 	.release	= cpufreq_sysfs_release,
 };
 
-static int add_cpu_dev_symlink(struct cpufreq_policy *policy, int cpu)
+static int add_cpu_dev_symlink(struct cpufreq_policy *policy,
+			       struct device *dev)
 {
-	struct device *cpu_dev;
-
-	pr_debug("%s: Adding symlink for CPU: %u\n", __func__, cpu);
-
-	if (!policy)
-		return 0;
-
-	cpu_dev = get_cpu_device(cpu);
-	if (WARN_ON(!cpu_dev))
-		return 0;
-
-	return sysfs_create_link(&cpu_dev->kobj, &policy->kobj, "cpufreq");
+	dev_dbg(dev, "%s: Adding symlink\n", __func__);
+	return sysfs_create_link(&dev->kobj, &policy->kobj, "cpufreq");
 }
 
 static void remove_cpu_dev_symlink(struct cpufreq_policy *policy, int cpu)
@@ -948,12 +939,17 @@ static void remove_cpu_dev_symlink(struct cpufreq_policy *policy, int cpu)
 /* Add/remove symlinks for all related CPUs */
 static int cpufreq_add_dev_symlink(struct cpufreq_policy *policy)
 {
+	struct device *cpu_dev;
 	unsigned int j;
 	int ret = 0;
 
 	/* Some related CPUs might not be present (physically hotplugged) */
 	for_each_cpu(j, policy->real_cpus) {
-		ret = add_cpu_dev_symlink(policy, j);
+		cpu_dev = get_cpu_device(j);
+		if (WARN_ON(!cpu_dev))
+			continue;
+
+		ret = add_cpu_dev_symlink(policy, cpu_dev);
 		if (ret)
 			break;
 	}
@@ -1073,13 +1069,9 @@ static void handle_update(struct work_struct *work)
 
 static struct cpufreq_policy *cpufreq_policy_alloc(unsigned int cpu)
 {
-	struct device *dev = get_cpu_device(cpu);
 	struct cpufreq_policy *policy;
 	int ret;
 
-	if (WARN_ON(!dev))
-		return NULL;
-
 	policy = kzalloc(sizeof(*policy), GFP_KERNEL);
 	if (!policy)
 		return NULL;
@@ -1355,7 +1347,7 @@ static int cpufreq_add_dev(struct device *dev, struct subsys_interface *sif)
 	if (!policy || cpumask_test_and_set_cpu(cpu, policy->real_cpus))
 		return 0;
 
-	return add_cpu_dev_symlink(policy, cpu);
+	return add_cpu_dev_symlink(policy, dev);
 }
 
 static void cpufreq_offline(unsigned int cpu)

Kernel warning in cpufreq_add_dev()

[Viresh Kumar]

On 22-08-16, 19:32, Rafael J. Wysocki wrote:
> But it will be called in that path during physical CPU hot-add, won't it?

What about something like this instead (completely untested) ?

@Russell: Can you please try this ??

-- 
viresh

-------------------------8<-------------------------

diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index 3dd4884c6f9e..a702d6246385 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -916,20 +916,11 @@ static struct kobj_type ktype_cpufreq = {
 	.release	= cpufreq_sysfs_release,
 };
 
-static int add_cpu_dev_symlink(struct cpufreq_policy *policy, int cpu)
+static int add_cpu_dev_symlink(struct cpufreq_policy *policy,
+			       struct device *dev)
 {
-	struct device *cpu_dev;
-
-	pr_debug("%s: Adding symlink for CPU: %u\n", __func__, cpu);
-
-	if (!policy)
-		return 0;
-
-	cpu_dev = get_cpu_device(cpu);
-	if (WARN_ON(!cpu_dev))
-		return 0;
-
-	return sysfs_create_link(&cpu_dev->kobj, &policy->kobj, "cpufreq");
+	dev_dbg(dev, "%s: Adding symlink\n", __func__);
+	return sysfs_create_link(&dev->kobj, &policy->kobj, "cpufreq");
 }
 
 static void remove_cpu_dev_symlink(struct cpufreq_policy *policy, int cpu)
@@ -948,12 +939,17 @@ static void remove_cpu_dev_symlink(struct cpufreq_policy *policy, int cpu)
 /* Add/remove symlinks for all related CPUs */
 static int cpufreq_add_dev_symlink(struct cpufreq_policy *policy)
 {
+	struct device *cpu_dev;
 	unsigned int j;
 	int ret = 0;
 
 	/* Some related CPUs might not be present (physically hotplugged) */
 	for_each_cpu(j, policy->real_cpus) {
-		ret = add_cpu_dev_symlink(policy, j);
+		cpu_dev = get_cpu_device(j);
+		if (WARN_ON(!cpu_dev))
+			continue;
+
+		ret = add_cpu_dev_symlink(policy, cpu_dev);
 		if (ret)
 			break;
 	}
@@ -1073,13 +1069,9 @@ static void handle_update(struct work_struct *work)
 
 static struct cpufreq_policy *cpufreq_policy_alloc(unsigned int cpu)
 {
-	struct device *dev = get_cpu_device(cpu);
 	struct cpufreq_policy *policy;
 	int ret;
 
-	if (WARN_ON(!dev))
-		return NULL;
-
 	policy = kzalloc(sizeof(*policy), GFP_KERNEL);
 	if (!policy)
 		return NULL;
@@ -1355,7 +1347,7 @@ static int cpufreq_add_dev(struct device *dev, struct subsys_interface *sif)
 	if (!policy || cpumask_test_and_set_cpu(cpu, policy->real_cpus))
 		return 0;
 
-	return add_cpu_dev_symlink(policy, cpu);
+	return add_cpu_dev_symlink(policy, dev);
 }
 
 static void cpufreq_offline(unsigned int cpu)

Re: Kernel warning in cpufreq_add_dev()

[Rafael J. Wysocki]

On Wednesday, August 24, 2016 06:43:16 PM Viresh Kumar wrote:
> On 22-08-16, 19:32, Rafael J. Wysocki wrote:
> > But it will be called in that path during physical CPU hot-add, won't it?
> 
> What about something like this instead (completely untested) ?

Inline, please?

> @Russell: Can you please try this ??

I was thinking about something similar, but won't the WARN_ON()s in
cpufreq_add/remove_dev_symlink() still trigger, say if there's more
than one CPU in a policy and both happen to be online initially?

Thanks,
Rafael

Kernel warning in cpufreq_add_dev()

[Rafael J. Wysocki]

On Wednesday, August 24, 2016 06:43:16 PM Viresh Kumar wrote:
> On 22-08-16, 19:32, Rafael J. Wysocki wrote:
> > But it will be called in that path during physical CPU hot-add, won't it?
> 
> What about something like this instead (completely untested) ?

Inline, please?

> @Russell: Can you please try this ??

I was thinking about something similar, but won't the WARN_ON()s in
cpufreq_add/remove_dev_symlink() still trigger, say if there's more
than one CPU in a policy and both happen to be online initially?

Thanks,
Rafael

Re: Kernel warning in cpufreq_add_dev()

[Viresh Kumar]

On 31-08-16, 03:26, Rafael J. Wysocki wrote:
> On Wednesday, August 24, 2016 06:43:16 PM Viresh Kumar wrote:
> > On 22-08-16, 19:32, Rafael J. Wysocki wrote:
> > > But it will be called in that path during physical CPU hot-add, won't it?
> > 
> > What about something like this instead (completely untested) ?
> 
> Inline, please?

I am not sure what that means. I pasted that inline in my previous mail only.

> > @Russell: Can you please try this ??
> 
> I was thinking about something similar, but won't the WARN_ON()s in
> cpufreq_add/remove_dev_symlink() still trigger, say if there's more
> than one CPU in a policy and both happen to be online initially?

real CPUs should already be online and their device structure should be
available, isn't it ?

-- 
viresh

Kernel warning in cpufreq_add_dev()

[Viresh Kumar]

On 31-08-16, 03:26, Rafael J. Wysocki wrote:
> On Wednesday, August 24, 2016 06:43:16 PM Viresh Kumar wrote:
> > On 22-08-16, 19:32, Rafael J. Wysocki wrote:
> > > But it will be called in that path during physical CPU hot-add, won't it?
> > 
> > What about something like this instead (completely untested) ?
> 
> Inline, please?

I am not sure what that means. I pasted that inline in my previous mail only.

> > @Russell: Can you please try this ??
> 
> I was thinking about something similar, but won't the WARN_ON()s in
> cpufreq_add/remove_dev_symlink() still trigger, say if there's more
> than one CPU in a policy and both happen to be online initially?

real CPUs should already be online and their device structure should be
available, isn't it ?

-- 
viresh

Re: Kernel warning in cpufreq_add_dev()

[Rafael J. Wysocki]

On Wed, Aug 31, 2016 at 6:11 AM, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> On 31-08-16, 03:26, Rafael J. Wysocki wrote:
>> On Wednesday, August 24, 2016 06:43:16 PM Viresh Kumar wrote:
>> > On 22-08-16, 19:32, Rafael J. Wysocki wrote:
>> > > But it will be called in that path during physical CPU hot-add, won't it?
>> >
>> > What about something like this instead (completely untested) ?
>>
>> Inline, please?
>
> I am not sure what that means. I pasted that inline in my previous mail only.

I wanted to say that I'd prefer patches to be sent in the message
proper instead of as inline attachments.  It is easier to respond to
them this way (to me at least).

>> > @Russell: Can you please try this ??
>>
>> I was thinking about something similar, but won't the WARN_ON()s in
>> cpufreq_add/remove_dev_symlink() still trigger, say if there's more
>> than one CPU in a policy and both happen to be online initially?
>
> real CPUs should already be online and their device structure should be
> available, isn't it ?

Not if the driver has already been registered when cpufreq_add_dev()
runs AFAICS (which is the case in question).

Thanks,
Rafael

Kernel warning in cpufreq_add_dev()

[Rafael J. Wysocki]

On Wed, Aug 31, 2016 at 6:11 AM, Viresh Kumar <viresh.kumar@linaro.org> wrote:
> On 31-08-16, 03:26, Rafael J. Wysocki wrote:
>> On Wednesday, August 24, 2016 06:43:16 PM Viresh Kumar wrote:
>> > On 22-08-16, 19:32, Rafael J. Wysocki wrote:
>> > > But it will be called in that path during physical CPU hot-add, won't it?
>> >
>> > What about something like this instead (completely untested) ?
>>
>> Inline, please?
>
> I am not sure what that means. I pasted that inline in my previous mail only.

I wanted to say that I'd prefer patches to be sent in the message
proper instead of as inline attachments.  It is easier to respond to
them this way (to me at least).

>> > @Russell: Can you please try this ??
>>
>> I was thinking about something similar, but won't the WARN_ON()s in
>> cpufreq_add/remove_dev_symlink() still trigger, say if there's more
>> than one CPU in a policy and both happen to be online initially?
>
> real CPUs should already be online and their device structure should be
> available, isn't it ?

Not if the driver has already been registered when cpufreq_add_dev()
runs AFAICS (which is the case in question).

Thanks,
Rafael

[PATCH] cpufreq: create link to policy only for registered CPUs

[Viresh Kumar]

If a cpufreq driver is registered very early in the boot stage (e.g.
registered from postcore_initcall()), then cpufreq core may generate
kernel warnings for it.

In this case, the CPUs are registered as devices with the kernel only
after the cpufreq driver is registered, while the CPUs were brought
online way before that. And by the time cpufreq_add_dev() gets called,
the cpu device isn't stored in the per-cpu variable (cpu_sys_devices,)
which is read by get_cpu_device().

And so cpufreq core fails to get device for the CPU, for which
cpufreq_add_dev() was called in the first place and we will hit a
WARN_ON(!cpu_dev).

Even if we reuse the 'dev' parameter passed to cpufreq_add_dev() to
avoid that warning, there might be other CPUs online that share the
policy with the cpu for which cpufreq_add_dev() is called. And
eventually get_cpu_device() will return NULL for them as well, and we
will hit the same WARN_ON() again.

In order to fix these issues, change cpufreq core to create links to the
policy for a cpu only when cpufreq_add_dev() is called for that CPU.

Reuse the 'real_cpus' mask to track that as well.

Note that cpufreq_remove_dev() already handles removal of the links for
individual CPUs and cpufreq_add_dev() has aligned with that now.

Reported-by: Russell King <linux@armlinux.org.uk>
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/cpufreq/cpufreq.c | 89 +++++++++++++++--------------------------------
 1 file changed, 28 insertions(+), 61 deletions(-)

diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index 13fb589b6d2c..3a64136bf21b 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -916,58 +916,18 @@ static struct kobj_type ktype_cpufreq = {
 	.release	= cpufreq_sysfs_release,
 };
 
-static int add_cpu_dev_symlink(struct cpufreq_policy *policy, int cpu)
+static int add_cpu_dev_symlink(struct cpufreq_policy *policy,
+			       struct device *dev)
 {
-	struct device *cpu_dev;
-
-	pr_debug("%s: Adding symlink for CPU: %u\n", __func__, cpu);
-
-	if (!policy)
-		return 0;
-
-	cpu_dev = get_cpu_device(cpu);
-	if (WARN_ON(!cpu_dev))
-		return 0;
-
-	return sysfs_create_link(&cpu_dev->kobj, &policy->kobj, "cpufreq");
+	dev_dbg(dev, "%s: Adding symlink\n", __func__);
+	return sysfs_create_link(&dev->kobj, &policy->kobj, "cpufreq");
 }
 
-static void remove_cpu_dev_symlink(struct cpufreq_policy *policy, int cpu)
+static void remove_cpu_dev_symlink(struct cpufreq_policy *policy,
+				   struct device *dev)
 {
-	struct device *cpu_dev;
-
-	pr_debug("%s: Removing symlink for CPU: %u\n", __func__, cpu);
-
-	cpu_dev = get_cpu_device(cpu);
-	if (WARN_ON(!cpu_dev))
-		return;
-
-	sysfs_remove_link(&cpu_dev->kobj, "cpufreq");
-}
-
-/* Add/remove symlinks for all related CPUs */
-static int cpufreq_add_dev_symlink(struct cpufreq_policy *policy)
-{
-	unsigned int j;
-	int ret = 0;
-
-	/* Some related CPUs might not be present (physically hotplugged) */
-	for_each_cpu(j, policy->real_cpus) {
-		ret = add_cpu_dev_symlink(policy, j);
-		if (ret)
-			break;
-	}
-
-	return ret;
-}
-
-static void cpufreq_remove_dev_symlink(struct cpufreq_policy *policy)
-{
-	unsigned int j;
-
-	/* Some related CPUs might not be present (physically hotplugged) */
-	for_each_cpu(j, policy->real_cpus)
-		remove_cpu_dev_symlink(policy, j);
+	dev_dbg(dev, "%s: Removing symlink\n", __func__);
+	sysfs_remove_link(&dev->kobj, "cpufreq");
 }
 
 static int cpufreq_add_dev_interface(struct cpufreq_policy *policy)
@@ -999,7 +959,7 @@ static int cpufreq_add_dev_interface(struct cpufreq_policy *policy)
 			return ret;
 	}
 
-	return cpufreq_add_dev_symlink(policy);
+	return 0;
 }
 
 __weak struct cpufreq_governor *cpufreq_default_governor(void)
@@ -1129,7 +1089,6 @@ static void cpufreq_policy_put_kobj(struct cpufreq_policy *policy, bool notify)
 
 	down_write(&policy->rwsem);
 	cpufreq_stats_free_table(policy);
-	cpufreq_remove_dev_symlink(policy);
 	kobj = &policy->kobj;
 	cmp = &policy->kobj_unregister;
 	up_write(&policy->rwsem);
@@ -1211,8 +1170,8 @@ static int cpufreq_online(unsigned int cpu)
 	if (new_policy) {
 		/* related_cpus should at least include policy->cpus. */
 		cpumask_copy(policy->related_cpus, policy->cpus);
-		/* Remember CPUs present at the policy creation time. */
-		cpumask_and(policy->real_cpus, policy->cpus, cpu_present_mask);
+		/* Clear mask of registered CPUs */
+		cpumask_clear(policy->real_cpus);
 	}
 
 	/*
@@ -1327,6 +1286,8 @@ static int cpufreq_online(unsigned int cpu)
 	return ret;
 }
 
+static void cpufreq_offline(unsigned int cpu);
+
 /**
  * cpufreq_add_dev - the cpufreq interface for a CPU device.
  * @dev: CPU device.
@@ -1336,22 +1297,28 @@ static int cpufreq_add_dev(struct device *dev, struct subsys_interface *sif)
 {
 	struct cpufreq_policy *policy;
 	unsigned cpu = dev->id;
+	int ret;
 
 	dev_dbg(dev, "%s: adding CPU%u\n", __func__, cpu);
 
-	if (cpu_online(cpu))
-		return cpufreq_online(cpu);
+	if (cpu_online(cpu)) {
+		ret = cpufreq_online(cpu);
+		if (ret)
+			return ret;
+	}
 
-	/*
-	 * A hotplug notifier will follow and we will handle it as CPU online
-	 * then.  For now, just create the sysfs link, unless there is no policy
-	 * or the link is already present.
-	 */
+	/* Create sysfs link on CPU registration */
 	policy = per_cpu(cpufreq_cpu_data, cpu);
 	if (!policy || cpumask_test_and_set_cpu(cpu, policy->real_cpus))
 		return 0;
 
-	return add_cpu_dev_symlink(policy, cpu);
+	ret = add_cpu_dev_symlink(policy, dev);
+	if (ret) {
+		cpumask_clear_cpu(cpu, policy->real_cpus);
+		cpufreq_offline(cpu);
+	}
+
+	return ret;
 }
 
 static void cpufreq_offline(unsigned int cpu)
@@ -1432,7 +1399,7 @@ static void cpufreq_remove_dev(struct device *dev, struct subsys_interface *sif)
 		cpufreq_offline(cpu);
 
 	cpumask_clear_cpu(cpu, policy->real_cpus);
-	remove_cpu_dev_symlink(policy, cpu);
+	remove_cpu_dev_symlink(policy, dev);
 
 	if (cpumask_empty(policy->real_cpus))
 		cpufreq_policy_free(policy, true);
-- 
2.7.1.410.g6faf27b

[PATCH] cpufreq: create link to policy only for registered CPUs

[Viresh Kumar]

If a cpufreq driver is registered very early in the boot stage (e.g.
registered from postcore_initcall()), then cpufreq core may generate
kernel warnings for it.

In this case, the CPUs are registered as devices with the kernel only
after the cpufreq driver is registered, while the CPUs were brought
online way before that. And by the time cpufreq_add_dev() gets called,
the cpu device isn't stored in the per-cpu variable (cpu_sys_devices,)
which is read by get_cpu_device().

And so cpufreq core fails to get device for the CPU, for which
cpufreq_add_dev() was called in the first place and we will hit a
WARN_ON(!cpu_dev).

Even if we reuse the 'dev' parameter passed to cpufreq_add_dev() to
avoid that warning, there might be other CPUs online that share the
policy with the cpu for which cpufreq_add_dev() is called. And
eventually get_cpu_device() will return NULL for them as well, and we
will hit the same WARN_ON() again.

In order to fix these issues, change cpufreq core to create links to the
policy for a cpu only when cpufreq_add_dev() is called for that CPU.

Reuse the 'real_cpus' mask to track that as well.

Note that cpufreq_remove_dev() already handles removal of the links for
individual CPUs and cpufreq_add_dev() has aligned with that now.

Reported-by: Russell King <linux@armlinux.org.uk>
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/cpufreq/cpufreq.c | 89 +++++++++++++++--------------------------------
 1 file changed, 28 insertions(+), 61 deletions(-)

diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index 13fb589b6d2c..3a64136bf21b 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -916,58 +916,18 @@ static struct kobj_type ktype_cpufreq = {
 	.release	= cpufreq_sysfs_release,
 };
 
-static int add_cpu_dev_symlink(struct cpufreq_policy *policy, int cpu)
+static int add_cpu_dev_symlink(struct cpufreq_policy *policy,
+			       struct device *dev)
 {
-	struct device *cpu_dev;
-
-	pr_debug("%s: Adding symlink for CPU: %u\n", __func__, cpu);
-
-	if (!policy)
-		return 0;
-
-	cpu_dev = get_cpu_device(cpu);
-	if (WARN_ON(!cpu_dev))
-		return 0;
-
-	return sysfs_create_link(&cpu_dev->kobj, &policy->kobj, "cpufreq");
+	dev_dbg(dev, "%s: Adding symlink\n", __func__);
+	return sysfs_create_link(&dev->kobj, &policy->kobj, "cpufreq");
 }
 
-static void remove_cpu_dev_symlink(struct cpufreq_policy *policy, int cpu)
+static void remove_cpu_dev_symlink(struct cpufreq_policy *policy,
+				   struct device *dev)
 {
-	struct device *cpu_dev;
-
-	pr_debug("%s: Removing symlink for CPU: %u\n", __func__, cpu);
-
-	cpu_dev = get_cpu_device(cpu);
-	if (WARN_ON(!cpu_dev))
-		return;
-
-	sysfs_remove_link(&cpu_dev->kobj, "cpufreq");
-}
-
-/* Add/remove symlinks for all related CPUs */
-static int cpufreq_add_dev_symlink(struct cpufreq_policy *policy)
-{
-	unsigned int j;
-	int ret = 0;
-
-	/* Some related CPUs might not be present (physically hotplugged) */
-	for_each_cpu(j, policy->real_cpus) {
-		ret = add_cpu_dev_symlink(policy, j);
-		if (ret)
-			break;
-	}
-
-	return ret;
-}
-
-static void cpufreq_remove_dev_symlink(struct cpufreq_policy *policy)
-{
-	unsigned int j;
-
-	/* Some related CPUs might not be present (physically hotplugged) */
-	for_each_cpu(j, policy->real_cpus)
-		remove_cpu_dev_symlink(policy, j);
+	dev_dbg(dev, "%s: Removing symlink\n", __func__);
+	sysfs_remove_link(&dev->kobj, "cpufreq");
 }
 
 static int cpufreq_add_dev_interface(struct cpufreq_policy *policy)
@@ -999,7 +959,7 @@ static int cpufreq_add_dev_interface(struct cpufreq_policy *policy)
 			return ret;
 	}
 
-	return cpufreq_add_dev_symlink(policy);
+	return 0;
 }
 
 __weak struct cpufreq_governor *cpufreq_default_governor(void)
@@ -1129,7 +1089,6 @@ static void cpufreq_policy_put_kobj(struct cpufreq_policy *policy, bool notify)
 
 	down_write(&policy->rwsem);
 	cpufreq_stats_free_table(policy);
-	cpufreq_remove_dev_symlink(policy);
 	kobj = &policy->kobj;
 	cmp = &policy->kobj_unregister;
 	up_write(&policy->rwsem);
@@ -1211,8 +1170,8 @@ static int cpufreq_online(unsigned int cpu)
 	if (new_policy) {
 		/* related_cpus should at least include policy->cpus. */
 		cpumask_copy(policy->related_cpus, policy->cpus);
-		/* Remember CPUs present at the policy creation time. */
-		cpumask_and(policy->real_cpus, policy->cpus, cpu_present_mask);
+		/* Clear mask of registered CPUs */
+		cpumask_clear(policy->real_cpus);
 	}
 
 	/*
@@ -1327,6 +1286,8 @@ static int cpufreq_online(unsigned int cpu)
 	return ret;
 }
 
+static void cpufreq_offline(unsigned int cpu);
+
 /**
  * cpufreq_add_dev - the cpufreq interface for a CPU device.
  * @dev: CPU device.
@@ -1336,22 +1297,28 @@ static int cpufreq_add_dev(struct device *dev, struct subsys_interface *sif)
 {
 	struct cpufreq_policy *policy;
 	unsigned cpu = dev->id;
+	int ret;
 
 	dev_dbg(dev, "%s: adding CPU%u\n", __func__, cpu);
 
-	if (cpu_online(cpu))
-		return cpufreq_online(cpu);
+	if (cpu_online(cpu)) {
+		ret = cpufreq_online(cpu);
+		if (ret)
+			return ret;
+	}
 
-	/*
-	 * A hotplug notifier will follow and we will handle it as CPU online
-	 * then.  For now, just create the sysfs link, unless there is no policy
-	 * or the link is already present.
-	 */
+	/* Create sysfs link on CPU registration */
 	policy = per_cpu(cpufreq_cpu_data, cpu);
 	if (!policy || cpumask_test_and_set_cpu(cpu, policy->real_cpus))
 		return 0;
 
-	return add_cpu_dev_symlink(policy, cpu);
+	ret = add_cpu_dev_symlink(policy, dev);
+	if (ret) {
+		cpumask_clear_cpu(cpu, policy->real_cpus);
+		cpufreq_offline(cpu);
+	}
+
+	return ret;
 }
 
 static void cpufreq_offline(unsigned int cpu)
@@ -1432,7 +1399,7 @@ static void cpufreq_remove_dev(struct device *dev, struct subsys_interface *sif)
 		cpufreq_offline(cpu);
 
 	cpumask_clear_cpu(cpu, policy->real_cpus);
-	remove_cpu_dev_symlink(policy, cpu);
+	remove_cpu_dev_symlink(policy, dev);
 
 	if (cpumask_empty(policy->real_cpus))
 		cpufreq_policy_free(policy, true);
-- 
2.7.1.410.g6faf27b

Re: Kernel warning in cpufreq_add_dev()

[Viresh Kumar]

On 31-08-16, 03:26, Rafael J. Wysocki wrote:
> I was thinking about something similar, but won't the WARN_ON()s in
> cpufreq_add/remove_dev_symlink() still trigger, say if there's more
> than one CPU in a policy and both happen to be online initially?

Right. I missed that. I have sent a patch just now in reply to the first email
from Russell. That should fix it all..

@Russell: Can you please test/verify the patch I have sent now?

-- 
viresh

Kernel warning in cpufreq_add_dev()

[Viresh Kumar]

On 31-08-16, 03:26, Rafael J. Wysocki wrote:
> I was thinking about something similar, but won't the WARN_ON()s in
> cpufreq_add/remove_dev_symlink() still trigger, say if there's more
> than one CPU in a policy and both happen to be online initially?

Right. I missed that. I have sent a patch just now in reply to the first email
from Russell. That should fix it all..

@Russell: Can you please test/verify the patch I have sent now?

-- 
viresh

Re: [PATCH] cpufreq: create link to policy only for registered CPUs

[Russell King - ARM Linux]

On Fri, Sep 09, 2016 at 03:24:14PM +0530, Viresh Kumar wrote:
> If a cpufreq driver is registered very early in the boot stage (e.g.
> registered from postcore_initcall()), then cpufreq core may generate
> kernel warnings for it.
> 
> In this case, the CPUs are registered as devices with the kernel only
> after the cpufreq driver is registered, while the CPUs were brought
> online way before that.

This seems confusing, maybe:

"In this case, the CPUs are brought online, then the cpufreq driver is
registered, and then the CPU topology devices are registered."

which gives more of a linear A happens, then B, then C.

> ... And by the time cpufreq_add_dev() gets called,
> the cpu device isn't stored in the per-cpu variable (cpu_sys_devices,)
> which is read by get_cpu_device().

s/And by/By/ or "However, by"

> And so cpufreq core fails to get device for the CPU, for which
> cpufreq_add_dev() was called in the first place and we will hit a
> WARN_ON(!cpu_dev).

s/And so/So the/

This isn't the WARN_ON() statement that's triggering for me.

> Even if we reuse the 'dev' parameter passed to cpufreq_add_dev() to
> avoid that warning, there might be other CPUs online that share the
> policy with the cpu for which cpufreq_add_dev() is called. And
> eventually get_cpu_device() will return NULL for them as well, and we
> will hit the same WARN_ON() again.

s/And eventually/Eventually/

> In order to fix these issues, change cpufreq core to create links to the
> policy for a cpu only when cpufreq_add_dev() is called for that CPU.
> 
> Reuse the 'real_cpus' mask to track that as well.
> 
> Note that cpufreq_remove_dev() already handles removal of the links for
> individual CPUs and cpufreq_add_dev() has aligned with that now.

I applied this patch, but I still get:

WARNING: CPU: 0 PID: 1 at drivers/cpufreq/cpufreq.c:1040 cpufreq_add_dev+0x144/0x634
Modules linked in:
CPU: 0 PID: 1 Comm: swapper Not tainted 4.8.0-rc5+ #1061
Hardware name: Intel-Assabet
Backtrace:
[<c0212190>] (dump_backtrace) from [<c021249c>] (show_stack+0x18/0x1c)
 r6:00000000 r5:c05ca11d r4:00000000
[<c0212484>] (show_stack) from [<c036e84c>] (dump_stack+0x20/0x28)
[<c036e82c>] (dump_stack) from [<c021f7ec>] (__warn+0xd0/0xfc)
[<c021f71c>] (__warn) from [<c021f840>] (warn_slowpath_null+0x28/0x30)
 r10:00000000 r8:c062927c r7:00000000 r6:00000000 r5:c063d288 r4:00000000
[<c021f818>] (warn_slowpath_null) from [<c043dc84>] (cpufreq_add_dev+0x144/0x634)
[<c043db40>] (cpufreq_add_dev) from [<c03dc43c>] (bus_probe_device+0x5c/0x84)
 r10:00000000 r9:00000000 r8:00000000 r7:00000000 r6:c062927c r5:c063d288
 r4:c0640148
[<c03dc3e0>] (bus_probe_device) from [<c03da7c4>] (device_add+0x390/0x520)
 r6:c0629284 r5:00000000 r4:c062927c
[<c03da434>] (device_add) from [<c03daad8>] (device_register+0x1c/0x20)
 r10:c061d848 r8:c0603524 r7:00000001 r6:00000000 r5:c062927c r4:c062927c
[<c03daabc>] (device_register) from [<c03df5e8>] (register_cpu+0x88/0xac)
 r4:c0629274
[<c03df560>] (register_cpu) from [<c0603544>] (topology_init+0x20/0x2c)
 r7:c0646760 r6:c0623568 r5:c061d834 r4:00000000
[<c0603524>] (topology_init) from [<c020974c>] (do_one_initcall+0xc0/0x178)
 r4:00000004
[<c020968c>] (do_one_initcall) from [<c0600e70>] (kernel_init_freeable+0xfc/0x1c4)
 r10:c061d848 r9:00000000 r8:0000008c r7:c0646760 r6:c0623568 r5:c061d834
 r4:00000004
[<c0600d74>] (kernel_init_freeable) from [<c052b6cc>] (kernel_init+0x10/0xf4)
 r10:00000000 r8:00000000 r7:00000000 r6:00000000 r5:c052b6bc r4:00000000
[<c052b6bc>] (kernel_init) from [<c020fcf0>] (ret_from_fork+0x14/0x24)
 r4:00000000
---[ end trace d7209ea270f4f585 ]---

I'm afraid I rather predicted that after reading the patch but before
running the test: the patch does nothing to solve the original warning,
as the code path which gets us to that warning remains untouched by
this patch.

The code path is:

static int cpufreq_add_dev(struct device *dev, struct subsys_interface *sif)
{
        if (cpu_online(cpu))
                return cpufreq_online(cpu);

static int cpufreq_online(unsigned int cpu)
{
        policy = per_cpu(cpufreq_cpu_data, cpu);
        if (policy) {
        } else {
                new_policy = true;
                policy = cpufreq_policy_alloc(cpu);

static struct cpufreq_policy *cpufreq_policy_alloc(unsigned int cpu)
{
        if (WARN_ON(!dev))
                return NULL;

The only change in your patch that affected this path was this:

-       if (cpu_online(cpu))
-               return cpufreq_online(cpu);
+       if (cpu_online(cpu)) {
+               ret = cpufreq_online(cpu);
+               if (ret)
+                       return ret;
+       }

which obviously has no bearing on that WARN_ON() firing.

Maybe I'm testing the wrong patch.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

[PATCH] cpufreq: create link to policy only for registered CPUs

[Russell King - ARM Linux]

On Fri, Sep 09, 2016 at 03:24:14PM +0530, Viresh Kumar wrote:
> If a cpufreq driver is registered very early in the boot stage (e.g.
> registered from postcore_initcall()), then cpufreq core may generate
> kernel warnings for it.
> 
> In this case, the CPUs are registered as devices with the kernel only
> after the cpufreq driver is registered, while the CPUs were brought
> online way before that.

This seems confusing, maybe:

"In this case, the CPUs are brought online, then the cpufreq driver is
registered, and then the CPU topology devices are registered."

which gives more of a linear A happens, then B, then C.

> ... And by the time cpufreq_add_dev() gets called,
> the cpu device isn't stored in the per-cpu variable (cpu_sys_devices,)
> which is read by get_cpu_device().

s/And by/By/ or "However, by"

> And so cpufreq core fails to get device for the CPU, for which
> cpufreq_add_dev() was called in the first place and we will hit a
> WARN_ON(!cpu_dev).

s/And so/So the/

This isn't the WARN_ON() statement that's triggering for me.

> Even if we reuse the 'dev' parameter passed to cpufreq_add_dev() to
> avoid that warning, there might be other CPUs online that share the
> policy with the cpu for which cpufreq_add_dev() is called. And
> eventually get_cpu_device() will return NULL for them as well, and we
> will hit the same WARN_ON() again.

s/And eventually/Eventually/

> In order to fix these issues, change cpufreq core to create links to the
> policy for a cpu only when cpufreq_add_dev() is called for that CPU.
> 
> Reuse the 'real_cpus' mask to track that as well.
> 
> Note that cpufreq_remove_dev() already handles removal of the links for
> individual CPUs and cpufreq_add_dev() has aligned with that now.

I applied this patch, but I still get:

WARNING: CPU: 0 PID: 1 at drivers/cpufreq/cpufreq.c:1040 cpufreq_add_dev+0x144/0x634
Modules linked in:
CPU: 0 PID: 1 Comm: swapper Not tainted 4.8.0-rc5+ #1061
Hardware name: Intel-Assabet
Backtrace:
[<c0212190>] (dump_backtrace) from [<c021249c>] (show_stack+0x18/0x1c)
 r6:00000000 r5:c05ca11d r4:00000000
[<c0212484>] (show_stack) from [<c036e84c>] (dump_stack+0x20/0x28)
[<c036e82c>] (dump_stack) from [<c021f7ec>] (__warn+0xd0/0xfc)
[<c021f71c>] (__warn) from [<c021f840>] (warn_slowpath_null+0x28/0x30)
 r10:00000000 r8:c062927c r7:00000000 r6:00000000 r5:c063d288 r4:00000000
[<c021f818>] (warn_slowpath_null) from [<c043dc84>] (cpufreq_add_dev+0x144/0x634)
[<c043db40>] (cpufreq_add_dev) from [<c03dc43c>] (bus_probe_device+0x5c/0x84)
 r10:00000000 r9:00000000 r8:00000000 r7:00000000 r6:c062927c r5:c063d288
 r4:c0640148
[<c03dc3e0>] (bus_probe_device) from [<c03da7c4>] (device_add+0x390/0x520)
 r6:c0629284 r5:00000000 r4:c062927c
[<c03da434>] (device_add) from [<c03daad8>] (device_register+0x1c/0x20)
 r10:c061d848 r8:c0603524 r7:00000001 r6:00000000 r5:c062927c r4:c062927c
[<c03daabc>] (device_register) from [<c03df5e8>] (register_cpu+0x88/0xac)
 r4:c0629274
[<c03df560>] (register_cpu) from [<c0603544>] (topology_init+0x20/0x2c)
 r7:c0646760 r6:c0623568 r5:c061d834 r4:00000000
[<c0603524>] (topology_init) from [<c020974c>] (do_one_initcall+0xc0/0x178)
 r4:00000004
[<c020968c>] (do_one_initcall) from [<c0600e70>] (kernel_init_freeable+0xfc/0x1c4)
 r10:c061d848 r9:00000000 r8:0000008c r7:c0646760 r6:c0623568 r5:c061d834
 r4:00000004
[<c0600d74>] (kernel_init_freeable) from [<c052b6cc>] (kernel_init+0x10/0xf4)
 r10:00000000 r8:00000000 r7:00000000 r6:00000000 r5:c052b6bc r4:00000000
[<c052b6bc>] (kernel_init) from [<c020fcf0>] (ret_from_fork+0x14/0x24)
 r4:00000000
---[ end trace d7209ea270f4f585 ]---

I'm afraid I rather predicted that after reading the patch but before
running the test: the patch does nothing to solve the original warning,
as the code path which gets us to that warning remains untouched by
this patch.

The code path is:

static int cpufreq_add_dev(struct device *dev, struct subsys_interface *sif)
{
        if (cpu_online(cpu))
                return cpufreq_online(cpu);

static int cpufreq_online(unsigned int cpu)
{
        policy = per_cpu(cpufreq_cpu_data, cpu);
        if (policy) {
        } else {
                new_policy = true;
                policy = cpufreq_policy_alloc(cpu);

static struct cpufreq_policy *cpufreq_policy_alloc(unsigned int cpu)
{
        if (WARN_ON(!dev))
                return NULL;

The only change in your patch that affected this path was this:

-       if (cpu_online(cpu))
-               return cpufreq_online(cpu);
+       if (cpu_online(cpu)) {
+               ret = cpufreq_online(cpu);
+               if (ret)
+                       return ret;
+       }

which obviously has no bearing on that WARN_ON() firing.

Maybe I'm testing the wrong patch.

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

Re: [PATCH] cpufreq: create link to policy only for registered CPUs

[Viresh Kumar]

On 09-09-16, 12:16, Russell King - ARM Linux wrote:
> On Fri, Sep 09, 2016 at 03:24:14PM +0530, Viresh Kumar wrote:
> > If a cpufreq driver is registered very early in the boot stage (e.g.
> > registered from postcore_initcall()), then cpufreq core may generate
> > kernel warnings for it.
> > 
> > In this case, the CPUs are registered as devices with the kernel only
> > after the cpufreq driver is registered, while the CPUs were brought
> > online way before that.
> 
> This seems confusing, maybe:
> 
> "In this case, the CPUs are brought online, then the cpufreq driver is
> registered, and then the CPU topology devices are registered."
> 
> which gives more of a linear A happens, then B, then C.

Sure, thanks for the tip..

> > ... And by the time cpufreq_add_dev() gets called,
> > the cpu device isn't stored in the per-cpu variable (cpu_sys_devices,)
> > which is read by get_cpu_device().
> 
> s/And by/By/ or "However, by"
> 
> > And so cpufreq core fails to get device for the CPU, for which
> > cpufreq_add_dev() was called in the first place and we will hit a
> > WARN_ON(!cpu_dev).
> 
> s/And so/So the/
> 
> This isn't the WARN_ON() statement that's triggering for me.

The WARN_ON() that was triggering for you was already removed by a
patch from Rafael (see below), but with that patch, you would have hit
this WARN_ON() :(.

> > Even if we reuse the 'dev' parameter passed to cpufreq_add_dev() to
> > avoid that warning, there might be other CPUs online that share the
> > policy with the cpu for which cpufreq_add_dev() is called. And
> > eventually get_cpu_device() will return NULL for them as well, and we
> > will hit the same WARN_ON() again.
> 
> s/And eventually/Eventually/

Thanks for all the suggestions..

> > In order to fix these issues, change cpufreq core to create links to the
> > policy for a cpu only when cpufreq_add_dev() is called for that CPU.
> > 
> > Reuse the 'real_cpus' mask to track that as well.
> > 
> > Note that cpufreq_remove_dev() already handles removal of the links for
> > individual CPUs and cpufreq_add_dev() has aligned with that now.
> 
> I applied this patch, but I still get:
> 
> WARNING: CPU: 0 PID: 1 at drivers/cpufreq/cpufreq.c:1040 cpufreq_add_dev+0x144/0x634
> Modules linked in:
> CPU: 0 PID: 1 Comm: swapper Not tainted 4.8.0-rc5+ #1061
> Hardware name: Intel-Assabet
> Backtrace:
> [<c0212190>] (dump_backtrace) from [<c021249c>] (show_stack+0x18/0x1c)
>  r6:00000000 r5:c05ca11d r4:00000000
> [<c0212484>] (show_stack) from [<c036e84c>] (dump_stack+0x20/0x28)
> [<c036e82c>] (dump_stack) from [<c021f7ec>] (__warn+0xd0/0xfc)
> [<c021f71c>] (__warn) from [<c021f840>] (warn_slowpath_null+0x28/0x30)
>  r10:00000000 r8:c062927c r7:00000000 r6:00000000 r5:c063d288 r4:00000000
> [<c021f818>] (warn_slowpath_null) from [<c043dc84>] (cpufreq_add_dev+0x144/0x634)
> [<c043db40>] (cpufreq_add_dev) from [<c03dc43c>] (bus_probe_device+0x5c/0x84)
>  r10:00000000 r9:00000000 r8:00000000 r7:00000000 r6:c062927c r5:c063d288
>  r4:c0640148
> [<c03dc3e0>] (bus_probe_device) from [<c03da7c4>] (device_add+0x390/0x520)
>  r6:c0629284 r5:00000000 r4:c062927c
> [<c03da434>] (device_add) from [<c03daad8>] (device_register+0x1c/0x20)
>  r10:c061d848 r8:c0603524 r7:00000001 r6:00000000 r5:c062927c r4:c062927c
> [<c03daabc>] (device_register) from [<c03df5e8>] (register_cpu+0x88/0xac)
>  r4:c0629274
> [<c03df560>] (register_cpu) from [<c0603544>] (topology_init+0x20/0x2c)
>  r7:c0646760 r6:c0623568 r5:c061d834 r4:00000000
> [<c0603524>] (topology_init) from [<c020974c>] (do_one_initcall+0xc0/0x178)
>  r4:00000004
> [<c020968c>] (do_one_initcall) from [<c0600e70>] (kernel_init_freeable+0xfc/0x1c4)
>  r10:c061d848 r9:00000000 r8:0000008c r7:c0646760 r6:c0623568 r5:c061d834
>  r4:00000004
> [<c0600d74>] (kernel_init_freeable) from [<c052b6cc>] (kernel_init+0x10/0xf4)
>  r10:00000000 r8:00000000 r7:00000000 r6:00000000 r5:c052b6bc r4:00000000
> [<c052b6bc>] (kernel_init) from [<c020fcf0>] (ret_from_fork+0x14/0x24)
>  r4:00000000
> ---[ end trace d7209ea270f4f585 ]---
> 
> I'm afraid I rather predicted that after reading the patch but before
> running the test: the patch does nothing to solve the original warning,
> as the code path which gets us to that warning remains untouched by
> this patch.
> 
> The code path is:
> 
> static int cpufreq_add_dev(struct device *dev, struct subsys_interface *sif)
> {
>         if (cpu_online(cpu))
>                 return cpufreq_online(cpu);
> 
> static int cpufreq_online(unsigned int cpu)
> {
>         policy = per_cpu(cpufreq_cpu_data, cpu);
>         if (policy) {
>         } else {
>                 new_policy = true;
>                 policy = cpufreq_policy_alloc(cpu);
> 
> static struct cpufreq_policy *cpufreq_policy_alloc(unsigned int cpu)
> {
>         if (WARN_ON(!dev))
>                 return NULL;
> 
> The only change in your patch that affected this path was this:
> 
> -       if (cpu_online(cpu))
> -               return cpufreq_online(cpu);
> +       if (cpu_online(cpu)) {
> +               ret = cpufreq_online(cpu);
> +               if (ret)
> +                       return ret;
> +       }
> 
> which obviously has no bearing on that WARN_ON() firing.
> 
> Maybe I'm testing the wrong patch.

Thanks for testing it.. You need another patch from Rafael, which
should be in linux-next by now..

commit 3689ad7ed6a8 ("cpufreq: Drop unnecessary check from
cpufreq_policy_alloc()")

Both patches combined will fix the problem you were getting.

-- 
viresh

[PATCH] cpufreq: create link to policy only for registered CPUs

[Viresh Kumar]

On 09-09-16, 12:16, Russell King - ARM Linux wrote:
> On Fri, Sep 09, 2016 at 03:24:14PM +0530, Viresh Kumar wrote:
> > If a cpufreq driver is registered very early in the boot stage (e.g.
> > registered from postcore_initcall()), then cpufreq core may generate
> > kernel warnings for it.
> > 
> > In this case, the CPUs are registered as devices with the kernel only
> > after the cpufreq driver is registered, while the CPUs were brought
> > online way before that.
> 
> This seems confusing, maybe:
> 
> "In this case, the CPUs are brought online, then the cpufreq driver is
> registered, and then the CPU topology devices are registered."
> 
> which gives more of a linear A happens, then B, then C.

Sure, thanks for the tip..

> > ... And by the time cpufreq_add_dev() gets called,
> > the cpu device isn't stored in the per-cpu variable (cpu_sys_devices,)
> > which is read by get_cpu_device().
> 
> s/And by/By/ or "However, by"
> 
> > And so cpufreq core fails to get device for the CPU, for which
> > cpufreq_add_dev() was called in the first place and we will hit a
> > WARN_ON(!cpu_dev).
> 
> s/And so/So the/
> 
> This isn't the WARN_ON() statement that's triggering for me.

The WARN_ON() that was triggering for you was already removed by a
patch from Rafael (see below), but with that patch, you would have hit
this WARN_ON() :(.

> > Even if we reuse the 'dev' parameter passed to cpufreq_add_dev() to
> > avoid that warning, there might be other CPUs online that share the
> > policy with the cpu for which cpufreq_add_dev() is called. And
> > eventually get_cpu_device() will return NULL for them as well, and we
> > will hit the same WARN_ON() again.
> 
> s/And eventually/Eventually/

Thanks for all the suggestions..

> > In order to fix these issues, change cpufreq core to create links to the
> > policy for a cpu only when cpufreq_add_dev() is called for that CPU.
> > 
> > Reuse the 'real_cpus' mask to track that as well.
> > 
> > Note that cpufreq_remove_dev() already handles removal of the links for
> > individual CPUs and cpufreq_add_dev() has aligned with that now.
> 
> I applied this patch, but I still get:
> 
> WARNING: CPU: 0 PID: 1 at drivers/cpufreq/cpufreq.c:1040 cpufreq_add_dev+0x144/0x634
> Modules linked in:
> CPU: 0 PID: 1 Comm: swapper Not tainted 4.8.0-rc5+ #1061
> Hardware name: Intel-Assabet
> Backtrace:
> [<c0212190>] (dump_backtrace) from [<c021249c>] (show_stack+0x18/0x1c)
>  r6:00000000 r5:c05ca11d r4:00000000
> [<c0212484>] (show_stack) from [<c036e84c>] (dump_stack+0x20/0x28)
> [<c036e82c>] (dump_stack) from [<c021f7ec>] (__warn+0xd0/0xfc)
> [<c021f71c>] (__warn) from [<c021f840>] (warn_slowpath_null+0x28/0x30)
>  r10:00000000 r8:c062927c r7:00000000 r6:00000000 r5:c063d288 r4:00000000
> [<c021f818>] (warn_slowpath_null) from [<c043dc84>] (cpufreq_add_dev+0x144/0x634)
> [<c043db40>] (cpufreq_add_dev) from [<c03dc43c>] (bus_probe_device+0x5c/0x84)
>  r10:00000000 r9:00000000 r8:00000000 r7:00000000 r6:c062927c r5:c063d288
>  r4:c0640148
> [<c03dc3e0>] (bus_probe_device) from [<c03da7c4>] (device_add+0x390/0x520)
>  r6:c0629284 r5:00000000 r4:c062927c
> [<c03da434>] (device_add) from [<c03daad8>] (device_register+0x1c/0x20)
>  r10:c061d848 r8:c0603524 r7:00000001 r6:00000000 r5:c062927c r4:c062927c
> [<c03daabc>] (device_register) from [<c03df5e8>] (register_cpu+0x88/0xac)
>  r4:c0629274
> [<c03df560>] (register_cpu) from [<c0603544>] (topology_init+0x20/0x2c)
>  r7:c0646760 r6:c0623568 r5:c061d834 r4:00000000
> [<c0603524>] (topology_init) from [<c020974c>] (do_one_initcall+0xc0/0x178)
>  r4:00000004
> [<c020968c>] (do_one_initcall) from [<c0600e70>] (kernel_init_freeable+0xfc/0x1c4)
>  r10:c061d848 r9:00000000 r8:0000008c r7:c0646760 r6:c0623568 r5:c061d834
>  r4:00000004
> [<c0600d74>] (kernel_init_freeable) from [<c052b6cc>] (kernel_init+0x10/0xf4)
>  r10:00000000 r8:00000000 r7:00000000 r6:00000000 r5:c052b6bc r4:00000000
> [<c052b6bc>] (kernel_init) from [<c020fcf0>] (ret_from_fork+0x14/0x24)
>  r4:00000000
> ---[ end trace d7209ea270f4f585 ]---
> 
> I'm afraid I rather predicted that after reading the patch but before
> running the test: the patch does nothing to solve the original warning,
> as the code path which gets us to that warning remains untouched by
> this patch.
> 
> The code path is:
> 
> static int cpufreq_add_dev(struct device *dev, struct subsys_interface *sif)
> {
>         if (cpu_online(cpu))
>                 return cpufreq_online(cpu);
> 
> static int cpufreq_online(unsigned int cpu)
> {
>         policy = per_cpu(cpufreq_cpu_data, cpu);
>         if (policy) {
>         } else {
>                 new_policy = true;
>                 policy = cpufreq_policy_alloc(cpu);
> 
> static struct cpufreq_policy *cpufreq_policy_alloc(unsigned int cpu)
> {
>         if (WARN_ON(!dev))
>                 return NULL;
> 
> The only change in your patch that affected this path was this:
> 
> -       if (cpu_online(cpu))
> -               return cpufreq_online(cpu);
> +       if (cpu_online(cpu)) {
> +               ret = cpufreq_online(cpu);
> +               if (ret)
> +                       return ret;
> +       }
> 
> which obviously has no bearing on that WARN_ON() firing.
> 
> Maybe I'm testing the wrong patch.

Thanks for testing it.. You need another patch from Rafael, which
should be in linux-next by now..

commit 3689ad7ed6a8 ("cpufreq: Drop unnecessary check from
cpufreq_policy_alloc()")

Both patches combined will fix the problem you were getting.

-- 
viresh

Re: [PATCH] cpufreq: create link to policy only for registered CPUs

[Russell King - ARM Linux]

On Fri, Sep 09, 2016 at 04:52:04PM +0530, Viresh Kumar wrote:
> Thanks for testing it.. You need another patch from Rafael, which
> should be in linux-next by now..
> 
> commit 3689ad7ed6a8 ("cpufreq: Drop unnecessary check from
> cpufreq_policy_alloc()")
> 
> Both patches combined will fix the problem you were getting.

Please send me this other patch.  Seems absurd to fix a reported problem
without copying appropriate patches to the reporter...

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

[PATCH] cpufreq: create link to policy only for registered CPUs

[Russell King - ARM Linux]

On Fri, Sep 09, 2016 at 04:52:04PM +0530, Viresh Kumar wrote:
> Thanks for testing it.. You need another patch from Rafael, which
> should be in linux-next by now..
> 
> commit 3689ad7ed6a8 ("cpufreq: Drop unnecessary check from
> cpufreq_policy_alloc()")
> 
> Both patches combined will fix the problem you were getting.

Please send me this other patch.  Seems absurd to fix a reported problem
without copying appropriate patches to the reporter...

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

Re: [PATCH] cpufreq: create link to policy only for registered CPUs

[Viresh Kumar]

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

On 09-09-16, 12:28, Russell King - ARM Linux wrote:
> On Fri, Sep 09, 2016 at 04:52:04PM +0530, Viresh Kumar wrote:
> > Thanks for testing it.. You need another patch from Rafael, which
> > should be in linux-next by now..
> > 
> > commit 3689ad7ed6a8 ("cpufreq: Drop unnecessary check from
> > cpufreq_policy_alloc()")
> > 
> > Both patches combined will fix the problem you were getting.
> 
> Please send me this other patch.  Seems absurd to fix a reported problem
> without copying appropriate patches to the reporter...

It got merged separately, and yes you should have been cc'd for that
as well.

Please find it attached now..

-- 
viresh

[-- Attachment #2: 0001-cpufreq-Drop-unnecessary-check-from-cpufreq_policy_a.patch --]
[-- Type: text/x-diff, Size: 1223 bytes --]

>From 3689ad7ed6a836c4eec5e7bdd17a11a79591bef9 Mon Sep 17 00:00:00 2001
Message-Id: <3689ad7ed6a836c4eec5e7bdd17a11a79591bef9.1473420826.git.viresh.kumar@linaro.org>
From: "Rafael J. Wysocki" <rafael.j.wysocki@intel.com>
Date: Wed, 31 Aug 2016 03:11:31 +0200
Subject: [PATCH] cpufreq: Drop unnecessary check from cpufreq_policy_alloc()

Since cpufreq_policy_alloc() doesn't use its dev variable for
anything useful, drop that variable from there along with the
NULL check against it.

Signed-off-by: Rafael J. Wysocki <rafael.j.wysocki@intel.com>
Acked-by: Viresh Kumar <viresh.kumar@linaro.org>
---
 drivers/cpufreq/cpufreq.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index 3dd4884c6f9e..13fb589b6d2c 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -1073,13 +1073,9 @@ static void handle_update(struct work_struct *work)
 
 static struct cpufreq_policy *cpufreq_policy_alloc(unsigned int cpu)
 {
-	struct device *dev = get_cpu_device(cpu);
 	struct cpufreq_policy *policy;
 	int ret;
 
-	if (WARN_ON(!dev))
-		return NULL;
-
 	policy = kzalloc(sizeof(*policy), GFP_KERNEL);
 	if (!policy)
 		return NULL;
-- 
2.7.1.410.g6faf27b

[PATCH] cpufreq: create link to policy only for registered CPUs

[Viresh Kumar]

On 09-09-16, 12:28, Russell King - ARM Linux wrote:
> On Fri, Sep 09, 2016 at 04:52:04PM +0530, Viresh Kumar wrote:
> > Thanks for testing it.. You need another patch from Rafael, which
> > should be in linux-next by now..
> > 
> > commit 3689ad7ed6a8 ("cpufreq: Drop unnecessary check from
> > cpufreq_policy_alloc()")
> > 
> > Both patches combined will fix the problem you were getting.
> 
> Please send me this other patch.  Seems absurd to fix a reported problem
> without copying appropriate patches to the reporter...

It got merged separately, and yes you should have been cc'd for that
as well.

Please find it attached now..

-- 
viresh
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-cpufreq-Drop-unnecessary-check-from-cpufreq_policy_a.patch
Type: text/x-diff
Size: 1223 bytes
Desc: not available
URL: <http://lists.infradead.org/pipermail/linux-arm-kernel/attachments/20160909/6d7f844a/attachment.bin>

Re: [PATCH] cpufreq: create link to policy only for registered CPUs

[Russell King - ARM Linux]

On Fri, Sep 09, 2016 at 05:04:59PM +0530, Viresh Kumar wrote:
> On 09-09-16, 12:28, Russell King - ARM Linux wrote:
> > On Fri, Sep 09, 2016 at 04:52:04PM +0530, Viresh Kumar wrote:
> > > Thanks for testing it.. You need another patch from Rafael, which
> > > should be in linux-next by now..
> > > 
> > > commit 3689ad7ed6a8 ("cpufreq: Drop unnecessary check from
> > > cpufreq_policy_alloc()")
> > > 
> > > Both patches combined will fix the problem you were getting.
> > 
> > Please send me this other patch.  Seems absurd to fix a reported problem
> > without copying appropriate patches to the reporter...
> 
> It got merged separately, and yes you should have been cc'd for that
> as well.
> 
> Please find it attached now..

Thanks, the warning is now gone.  From what I can tell, the sysfs
contents looks correct:

/sys/devices/system/cpu
|-- cpu0
|   |-- cpufreq -> ../cpufreq/policy0
|   |-- power
|   |   |-- autosuspend_delay_ms
|   |   |-- control
|   |   |-- runtime_active_time
|   |   |-- runtime_status
|   |   `-- runtime_suspended_time
|   |-- subsystem -> ../../../../bus/cpu
|   |-- topology
|   |   |-- core_id
|   |   |-- core_siblings
|   |   |-- core_siblings_list
|   |   |-- physical_package_id
|   |   |-- thread_siblings
|   |   `-- thread_siblings_list
|   `-- uevent
|-- cpufreq
|   `-- policy0
|       |-- affected_cpus
|       |-- cpuinfo_cur_freq
|       |-- cpuinfo_max_freq
|       |-- cpuinfo_min_freq
|       |-- cpuinfo_transition_latency
|       |-- related_cpus
|       |-- scaling_available_governors
|       |-- scaling_cur_freq
|       |-- scaling_driver
|       |-- scaling_governor
|       |-- scaling_max_freq
|       |-- scaling_min_freq
|       |-- scaling_setspeed
|       `-- stats
|           |-- time_in_state
|           `-- total_trans
|-- isolated
|-- kernel_max
|-- offline
|-- online
|-- possible
|-- power
|   |-- autosuspend_delay_ms
|   |-- control
|   |-- runtime_active_time
|   |-- runtime_status
|   `-- runtime_suspended_time
|-- present
`-- uevent

9 directories, 39 files

So, for both patches:

Tested-by: Russell King <rmk+kernel@arm.linux.org.uk>

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

[PATCH] cpufreq: create link to policy only for registered CPUs

[Russell King - ARM Linux]

On Fri, Sep 09, 2016 at 05:04:59PM +0530, Viresh Kumar wrote:
> On 09-09-16, 12:28, Russell King - ARM Linux wrote:
> > On Fri, Sep 09, 2016 at 04:52:04PM +0530, Viresh Kumar wrote:
> > > Thanks for testing it.. You need another patch from Rafael, which
> > > should be in linux-next by now..
> > > 
> > > commit 3689ad7ed6a8 ("cpufreq: Drop unnecessary check from
> > > cpufreq_policy_alloc()")
> > > 
> > > Both patches combined will fix the problem you were getting.
> > 
> > Please send me this other patch.  Seems absurd to fix a reported problem
> > without copying appropriate patches to the reporter...
> 
> It got merged separately, and yes you should have been cc'd for that
> as well.
> 
> Please find it attached now..

Thanks, the warning is now gone.  From what I can tell, the sysfs
contents looks correct:

/sys/devices/system/cpu
|-- cpu0
|   |-- cpufreq -> ../cpufreq/policy0
|   |-- power
|   |   |-- autosuspend_delay_ms
|   |   |-- control
|   |   |-- runtime_active_time
|   |   |-- runtime_status
|   |   `-- runtime_suspended_time
|   |-- subsystem -> ../../../../bus/cpu
|   |-- topology
|   |   |-- core_id
|   |   |-- core_siblings
|   |   |-- core_siblings_list
|   |   |-- physical_package_id
|   |   |-- thread_siblings
|   |   `-- thread_siblings_list
|   `-- uevent
|-- cpufreq
|   `-- policy0
|       |-- affected_cpus
|       |-- cpuinfo_cur_freq
|       |-- cpuinfo_max_freq
|       |-- cpuinfo_min_freq
|       |-- cpuinfo_transition_latency
|       |-- related_cpus
|       |-- scaling_available_governors
|       |-- scaling_cur_freq
|       |-- scaling_driver
|       |-- scaling_governor
|       |-- scaling_max_freq
|       |-- scaling_min_freq
|       |-- scaling_setspeed
|       `-- stats
|           |-- time_in_state
|           `-- total_trans
|-- isolated
|-- kernel_max
|-- offline
|-- online
|-- possible
|-- power
|   |-- autosuspend_delay_ms
|   |-- control
|   |-- runtime_active_time
|   |-- runtime_status
|   `-- runtime_suspended_time
|-- present
`-- uevent

9 directories, 39 files

So, for both patches:

Tested-by: Russell King <rmk+kernel@arm.linux.org.uk>

-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.

[PATCH V2] cpufreq: create link to policy only for registered CPUs

[Viresh Kumar]

If a cpufreq driver is registered very early in the boot stage (e.g.
registered from postcore_initcall()), then cpufreq core may generate
kernel warnings for it.

In this case, the CPUs are brought online, then the cpufreq driver is
registered, and then the CPU topology devices are registered. However,
by the time cpufreq_add_dev() gets called, the cpu device isn't stored
in the per-cpu variable (cpu_sys_devices,) which is read by
get_cpu_device().

So the cpufreq core fails to get device for the CPU, for which
cpufreq_add_dev() was called in the first place and we will hit a
WARN_ON(!cpu_dev).

Even if we reuse the 'dev' parameter passed to cpufreq_add_dev() to
avoid that warning, there might be other CPUs online that share the
policy with the cpu for which cpufreq_add_dev() is called. Eventually
get_cpu_device() will return NULL for them as well, and we will hit the
same WARN_ON() again.

In order to fix these issues, change cpufreq core to create links to the
policy for a cpu only when cpufreq_add_dev() is called for that CPU.

Reuse the 'real_cpus' mask to track that as well.

Note that cpufreq_remove_dev() already handles removal of the links for
individual CPUs and cpufreq_add_dev() has aligned with that now.

Reported-by: Russell King <rmk+kernel@arm.linux.org.uk>
Tested-by: Russell King <rmk+kernel@arm.linux.org.uk>
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
V1->V2:
- Updated changelog based on suggestions from Russell
- Tested by from Russell

 drivers/cpufreq/cpufreq.c | 89 +++++++++++++++--------------------------------
 1 file changed, 28 insertions(+), 61 deletions(-)

diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index 13fb589b6d2c..3a64136bf21b 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -916,58 +916,18 @@ static struct kobj_type ktype_cpufreq = {
 	.release	= cpufreq_sysfs_release,
 };
 
-static int add_cpu_dev_symlink(struct cpufreq_policy *policy, int cpu)
+static int add_cpu_dev_symlink(struct cpufreq_policy *policy,
+			       struct device *dev)
 {
-	struct device *cpu_dev;
-
-	pr_debug("%s: Adding symlink for CPU: %u\n", __func__, cpu);
-
-	if (!policy)
-		return 0;
-
-	cpu_dev = get_cpu_device(cpu);
-	if (WARN_ON(!cpu_dev))
-		return 0;
-
-	return sysfs_create_link(&cpu_dev->kobj, &policy->kobj, "cpufreq");
+	dev_dbg(dev, "%s: Adding symlink\n", __func__);
+	return sysfs_create_link(&dev->kobj, &policy->kobj, "cpufreq");
 }
 
-static void remove_cpu_dev_symlink(struct cpufreq_policy *policy, int cpu)
+static void remove_cpu_dev_symlink(struct cpufreq_policy *policy,
+				   struct device *dev)
 {
-	struct device *cpu_dev;
-
-	pr_debug("%s: Removing symlink for CPU: %u\n", __func__, cpu);
-
-	cpu_dev = get_cpu_device(cpu);
-	if (WARN_ON(!cpu_dev))
-		return;
-
-	sysfs_remove_link(&cpu_dev->kobj, "cpufreq");
-}
-
-/* Add/remove symlinks for all related CPUs */
-static int cpufreq_add_dev_symlink(struct cpufreq_policy *policy)
-{
-	unsigned int j;
-	int ret = 0;
-
-	/* Some related CPUs might not be present (physically hotplugged) */
-	for_each_cpu(j, policy->real_cpus) {
-		ret = add_cpu_dev_symlink(policy, j);
-		if (ret)
-			break;
-	}
-
-	return ret;
-}
-
-static void cpufreq_remove_dev_symlink(struct cpufreq_policy *policy)
-{
-	unsigned int j;
-
-	/* Some related CPUs might not be present (physically hotplugged) */
-	for_each_cpu(j, policy->real_cpus)
-		remove_cpu_dev_symlink(policy, j);
+	dev_dbg(dev, "%s: Removing symlink\n", __func__);
+	sysfs_remove_link(&dev->kobj, "cpufreq");
 }
 
 static int cpufreq_add_dev_interface(struct cpufreq_policy *policy)
@@ -999,7 +959,7 @@ static int cpufreq_add_dev_interface(struct cpufreq_policy *policy)
 			return ret;
 	}
 
-	return cpufreq_add_dev_symlink(policy);
+	return 0;
 }
 
 __weak struct cpufreq_governor *cpufreq_default_governor(void)
@@ -1129,7 +1089,6 @@ static void cpufreq_policy_put_kobj(struct cpufreq_policy *policy, bool notify)
 
 	down_write(&policy->rwsem);
 	cpufreq_stats_free_table(policy);
-	cpufreq_remove_dev_symlink(policy);
 	kobj = &policy->kobj;
 	cmp = &policy->kobj_unregister;
 	up_write(&policy->rwsem);
@@ -1211,8 +1170,8 @@ static int cpufreq_online(unsigned int cpu)
 	if (new_policy) {
 		/* related_cpus should at least include policy->cpus. */
 		cpumask_copy(policy->related_cpus, policy->cpus);
-		/* Remember CPUs present at the policy creation time. */
-		cpumask_and(policy->real_cpus, policy->cpus, cpu_present_mask);
+		/* Clear mask of registered CPUs */
+		cpumask_clear(policy->real_cpus);
 	}
 
 	/*
@@ -1327,6 +1286,8 @@ static int cpufreq_online(unsigned int cpu)
 	return ret;
 }
 
+static void cpufreq_offline(unsigned int cpu);
+
 /**
  * cpufreq_add_dev - the cpufreq interface for a CPU device.
  * @dev: CPU device.
@@ -1336,22 +1297,28 @@ static int cpufreq_add_dev(struct device *dev, struct subsys_interface *sif)
 {
 	struct cpufreq_policy *policy;
 	unsigned cpu = dev->id;
+	int ret;
 
 	dev_dbg(dev, "%s: adding CPU%u\n", __func__, cpu);
 
-	if (cpu_online(cpu))
-		return cpufreq_online(cpu);
+	if (cpu_online(cpu)) {
+		ret = cpufreq_online(cpu);
+		if (ret)
+			return ret;
+	}
 
-	/*
-	 * A hotplug notifier will follow and we will handle it as CPU online
-	 * then.  For now, just create the sysfs link, unless there is no policy
-	 * or the link is already present.
-	 */
+	/* Create sysfs link on CPU registration */
 	policy = per_cpu(cpufreq_cpu_data, cpu);
 	if (!policy || cpumask_test_and_set_cpu(cpu, policy->real_cpus))
 		return 0;
 
-	return add_cpu_dev_symlink(policy, cpu);
+	ret = add_cpu_dev_symlink(policy, dev);
+	if (ret) {
+		cpumask_clear_cpu(cpu, policy->real_cpus);
+		cpufreq_offline(cpu);
+	}
+
+	return ret;
 }
 
 static void cpufreq_offline(unsigned int cpu)
@@ -1432,7 +1399,7 @@ static void cpufreq_remove_dev(struct device *dev, struct subsys_interface *sif)
 		cpufreq_offline(cpu);
 
 	cpumask_clear_cpu(cpu, policy->real_cpus);
-	remove_cpu_dev_symlink(policy, cpu);
+	remove_cpu_dev_symlink(policy, dev);
 
 	if (cpumask_empty(policy->real_cpus))
 		cpufreq_policy_free(policy, true);
-- 
2.7.1.410.g6faf27b

[PATCH V2] cpufreq: create link to policy only for registered CPUs

[Viresh Kumar]

If a cpufreq driver is registered very early in the boot stage (e.g.
registered from postcore_initcall()), then cpufreq core may generate
kernel warnings for it.

In this case, the CPUs are brought online, then the cpufreq driver is
registered, and then the CPU topology devices are registered. However,
by the time cpufreq_add_dev() gets called, the cpu device isn't stored
in the per-cpu variable (cpu_sys_devices,) which is read by
get_cpu_device().

So the cpufreq core fails to get device for the CPU, for which
cpufreq_add_dev() was called in the first place and we will hit a
WARN_ON(!cpu_dev).

Even if we reuse the 'dev' parameter passed to cpufreq_add_dev() to
avoid that warning, there might be other CPUs online that share the
policy with the cpu for which cpufreq_add_dev() is called. Eventually
get_cpu_device() will return NULL for them as well, and we will hit the
same WARN_ON() again.

In order to fix these issues, change cpufreq core to create links to the
policy for a cpu only when cpufreq_add_dev() is called for that CPU.

Reuse the 'real_cpus' mask to track that as well.

Note that cpufreq_remove_dev() already handles removal of the links for
individual CPUs and cpufreq_add_dev() has aligned with that now.

Reported-by: Russell King <rmk+kernel@arm.linux.org.uk>
Tested-by: Russell King <rmk+kernel@arm.linux.org.uk>
Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>
---
V1->V2:
- Updated changelog based on suggestions from Russell
- Tested by from Russell

 drivers/cpufreq/cpufreq.c | 89 +++++++++++++++--------------------------------
 1 file changed, 28 insertions(+), 61 deletions(-)

diff --git a/drivers/cpufreq/cpufreq.c b/drivers/cpufreq/cpufreq.c
index 13fb589b6d2c..3a64136bf21b 100644
--- a/drivers/cpufreq/cpufreq.c
+++ b/drivers/cpufreq/cpufreq.c
@@ -916,58 +916,18 @@ static struct kobj_type ktype_cpufreq = {
 	.release	= cpufreq_sysfs_release,
 };
 
-static int add_cpu_dev_symlink(struct cpufreq_policy *policy, int cpu)
+static int add_cpu_dev_symlink(struct cpufreq_policy *policy,
+			       struct device *dev)
 {
-	struct device *cpu_dev;
-
-	pr_debug("%s: Adding symlink for CPU: %u\n", __func__, cpu);
-
-	if (!policy)
-		return 0;
-
-	cpu_dev = get_cpu_device(cpu);
-	if (WARN_ON(!cpu_dev))
-		return 0;
-
-	return sysfs_create_link(&cpu_dev->kobj, &policy->kobj, "cpufreq");
+	dev_dbg(dev, "%s: Adding symlink\n", __func__);
+	return sysfs_create_link(&dev->kobj, &policy->kobj, "cpufreq");
 }
 
-static void remove_cpu_dev_symlink(struct cpufreq_policy *policy, int cpu)
+static void remove_cpu_dev_symlink(struct cpufreq_policy *policy,
+				   struct device *dev)
 {
-	struct device *cpu_dev;
-
-	pr_debug("%s: Removing symlink for CPU: %u\n", __func__, cpu);
-
-	cpu_dev = get_cpu_device(cpu);
-	if (WARN_ON(!cpu_dev))
-		return;
-
-	sysfs_remove_link(&cpu_dev->kobj, "cpufreq");
-}
-
-/* Add/remove symlinks for all related CPUs */
-static int cpufreq_add_dev_symlink(struct cpufreq_policy *policy)
-{
-	unsigned int j;
-	int ret = 0;
-
-	/* Some related CPUs might not be present (physically hotplugged) */
-	for_each_cpu(j, policy->real_cpus) {
-		ret = add_cpu_dev_symlink(policy, j);
-		if (ret)
-			break;
-	}
-
-	return ret;
-}
-
-static void cpufreq_remove_dev_symlink(struct cpufreq_policy *policy)
-{
-	unsigned int j;
-
-	/* Some related CPUs might not be present (physically hotplugged) */
-	for_each_cpu(j, policy->real_cpus)
-		remove_cpu_dev_symlink(policy, j);
+	dev_dbg(dev, "%s: Removing symlink\n", __func__);
+	sysfs_remove_link(&dev->kobj, "cpufreq");
 }
 
 static int cpufreq_add_dev_interface(struct cpufreq_policy *policy)
@@ -999,7 +959,7 @@ static int cpufreq_add_dev_interface(struct cpufreq_policy *policy)
 			return ret;
 	}
 
-	return cpufreq_add_dev_symlink(policy);
+	return 0;
 }
 
 __weak struct cpufreq_governor *cpufreq_default_governor(void)
@@ -1129,7 +1089,6 @@ static void cpufreq_policy_put_kobj(struct cpufreq_policy *policy, bool notify)
 
 	down_write(&policy->rwsem);
 	cpufreq_stats_free_table(policy);
-	cpufreq_remove_dev_symlink(policy);
 	kobj = &policy->kobj;
 	cmp = &policy->kobj_unregister;
 	up_write(&policy->rwsem);
@@ -1211,8 +1170,8 @@ static int cpufreq_online(unsigned int cpu)
 	if (new_policy) {
 		/* related_cpus should at least include policy->cpus. */
 		cpumask_copy(policy->related_cpus, policy->cpus);
-		/* Remember CPUs present at the policy creation time. */
-		cpumask_and(policy->real_cpus, policy->cpus, cpu_present_mask);
+		/* Clear mask of registered CPUs */
+		cpumask_clear(policy->real_cpus);
 	}
 
 	/*
@@ -1327,6 +1286,8 @@ static int cpufreq_online(unsigned int cpu)
 	return ret;
 }
 
+static void cpufreq_offline(unsigned int cpu);
+
 /**
  * cpufreq_add_dev - the cpufreq interface for a CPU device.
  * @dev: CPU device.
@@ -1336,22 +1297,28 @@ static int cpufreq_add_dev(struct device *dev, struct subsys_interface *sif)
 {
 	struct cpufreq_policy *policy;
 	unsigned cpu = dev->id;
+	int ret;
 
 	dev_dbg(dev, "%s: adding CPU%u\n", __func__, cpu);
 
-	if (cpu_online(cpu))
-		return cpufreq_online(cpu);
+	if (cpu_online(cpu)) {
+		ret = cpufreq_online(cpu);
+		if (ret)
+			return ret;
+	}
 
-	/*
-	 * A hotplug notifier will follow and we will handle it as CPU online
-	 * then.  For now, just create the sysfs link, unless there is no policy
-	 * or the link is already present.
-	 */
+	/* Create sysfs link on CPU registration */
 	policy = per_cpu(cpufreq_cpu_data, cpu);
 	if (!policy || cpumask_test_and_set_cpu(cpu, policy->real_cpus))
 		return 0;
 
-	return add_cpu_dev_symlink(policy, cpu);
+	ret = add_cpu_dev_symlink(policy, dev);
+	if (ret) {
+		cpumask_clear_cpu(cpu, policy->real_cpus);
+		cpufreq_offline(cpu);
+	}
+
+	return ret;
 }
 
 static void cpufreq_offline(unsigned int cpu)
@@ -1432,7 +1399,7 @@ static void cpufreq_remove_dev(struct device *dev, struct subsys_interface *sif)
 		cpufreq_offline(cpu);
 
 	cpumask_clear_cpu(cpu, policy->real_cpus);
-	remove_cpu_dev_symlink(policy, cpu);
+	remove_cpu_dev_symlink(policy, dev);
 
 	if (cpumask_empty(policy->real_cpus))
 		cpufreq_policy_free(policy, true);
-- 
2.7.1.410.g6faf27b

Re: [PATCH V2] cpufreq: create link to policy only for registered CPUs

[Rafael J. Wysocki]

On Monday, September 12, 2016 12:07:05 PM Viresh Kumar wrote:
> If a cpufreq driver is registered very early in the boot stage (e.g.
> registered from postcore_initcall()), then cpufreq core may generate
> kernel warnings for it.
> 
> In this case, the CPUs are brought online, then the cpufreq driver is
> registered, and then the CPU topology devices are registered. However,
> by the time cpufreq_add_dev() gets called, the cpu device isn't stored
> in the per-cpu variable (cpu_sys_devices,) which is read by
> get_cpu_device().
> 
> So the cpufreq core fails to get device for the CPU, for which
> cpufreq_add_dev() was called in the first place and we will hit a
> WARN_ON(!cpu_dev).
> 
> Even if we reuse the 'dev' parameter passed to cpufreq_add_dev() to
> avoid that warning, there might be other CPUs online that share the
> policy with the cpu for which cpufreq_add_dev() is called. Eventually
> get_cpu_device() will return NULL for them as well, and we will hit the
> same WARN_ON() again.
> 
> In order to fix these issues, change cpufreq core to create links to the
> policy for a cpu only when cpufreq_add_dev() is called for that CPU.
> 
> Reuse the 'real_cpus' mask to track that as well.
> 
> Note that cpufreq_remove_dev() already handles removal of the links for
> individual CPUs and cpufreq_add_dev() has aligned with that now.
> 
> Reported-by: Russell King <rmk+kernel@arm.linux.org.uk>
> Tested-by: Russell King <rmk+kernel@arm.linux.org.uk>
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>

Applied.

Thanks,
Rafael

[PATCH V2] cpufreq: create link to policy only for registered CPUs

[Rafael J. Wysocki]

On Monday, September 12, 2016 12:07:05 PM Viresh Kumar wrote:
> If a cpufreq driver is registered very early in the boot stage (e.g.
> registered from postcore_initcall()), then cpufreq core may generate
> kernel warnings for it.
> 
> In this case, the CPUs are brought online, then the cpufreq driver is
> registered, and then the CPU topology devices are registered. However,
> by the time cpufreq_add_dev() gets called, the cpu device isn't stored
> in the per-cpu variable (cpu_sys_devices,) which is read by
> get_cpu_device().
> 
> So the cpufreq core fails to get device for the CPU, for which
> cpufreq_add_dev() was called in the first place and we will hit a
> WARN_ON(!cpu_dev).
> 
> Even if we reuse the 'dev' parameter passed to cpufreq_add_dev() to
> avoid that warning, there might be other CPUs online that share the
> policy with the cpu for which cpufreq_add_dev() is called. Eventually
> get_cpu_device() will return NULL for them as well, and we will hit the
> same WARN_ON() again.
> 
> In order to fix these issues, change cpufreq core to create links to the
> policy for a cpu only when cpufreq_add_dev() is called for that CPU.
> 
> Reuse the 'real_cpus' mask to track that as well.
> 
> Note that cpufreq_remove_dev() already handles removal of the links for
> individual CPUs and cpufreq_add_dev() has aligned with that now.
> 
> Reported-by: Russell King <rmk+kernel@arm.linux.org.uk>
> Tested-by: Russell King <rmk+kernel@arm.linux.org.uk>
> Signed-off-by: Viresh Kumar <viresh.kumar@linaro.org>

Applied.

Thanks,
Rafael