[PATCH 1/2] topology: Check for missing CPU devices

[PATCH 1/2] topology: Check for missing CPU devices

[Ben Hutchings]

Commit ccbc60d3e19a1b6ae66ca0d89b3da02dde62088b ('topology: Provide
CPU topology in sysfs in !SMP configurations') causes a crash at boot
on a several architectures.  The topology sysfs code assumes that
there is a CPU device for each online CPU whereas some architectures
that do not support SMP or cpufreq do not register any CPU devices.
Check for this before trying to use a device.

Signed-off-by: Ben Hutchings <ben@decadent.org.uk>
---
 drivers/base/topology.c |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletions(-)

diff --git a/drivers/base/topology.c b/drivers/base/topology.c
index ae989c5..4467c85 100644
--- a/drivers/base/topology.c
+++ b/drivers/base/topology.c
@@ -147,6 +147,8 @@ static int __cpuinit topology_add_dev(unsigned int cpu)
 {
 	struct device *dev = get_cpu_device(cpu);
 
+	if (!dev)
+		return -ENODEV;
 	return sysfs_create_group(&dev->kobj, &topology_attr_group);
 }
 
@@ -154,7 +156,8 @@ static void __cpuinit topology_remove_dev(unsigned int cpu)
 {
 	struct device *dev = get_cpu_device(cpu);
 
-	sysfs_remove_group(&dev->kobj, &topology_attr_group);
+	if (dev)
+		sysfs_remove_group(&dev->kobj, &topology_attr_group);
 }
 
 static int __cpuinit topology_cpu_callback(struct notifier_block *nfb,
-- 
1.7.8.2

[PATCH 2/2] cpu: Register a generic CPU device on architectures that currently do not

[Ben Hutchings]

frv, h8300, microblaze, openrisc, score and xtensa currently do not
register a CPU device.  Add the config option GENERIC_CPU_DEVICES which
causes a generic CPU device to be registered for each present CPU, and
make all these architectures select it.

Signed-off-by: Ben Hutchings <ben@decadent.org.uk>
---
This is untested on any of the affected architectures.  However, I
bodged x86 to use this option rather than registering CPU devices, and
the resulting kernel booted and showed the expected things in sysfs.

There are several other architectures that could be changed to use this
option, but I don't want to combine that change with this.

Ben.

 arch/frv/Kconfig        |    1 +
 arch/h8300/Kconfig      |    1 +
 arch/m68k/Kconfig       |    1 +
 arch/microblaze/Kconfig |    1 +
 arch/openrisc/Kconfig   |    1 +
 arch/score/Kconfig      |    1 +
 arch/xtensa/Kconfig     |    1 +
 drivers/base/Kconfig    |    4 ++++
 drivers/base/cpu.c      |   26 +++++++++++++++++++++++++-
 9 files changed, 36 insertions(+), 1 deletions(-)

diff --git a/arch/frv/Kconfig b/arch/frv/Kconfig
index bad27a6..70554ba 100644
--- a/arch/frv/Kconfig
+++ b/arch/frv/Kconfig
@@ -8,6 +8,7 @@ config FRV
 	select HAVE_GENERIC_HARDIRQS
 	select GENERIC_IRQ_SHOW
 	select ARCH_HAVE_NMI_SAFE_CMPXCHG
+	select GENERIC_CPU_DEVICES
 
 config ZONE_DMA
 	bool
diff --git a/arch/h8300/Kconfig b/arch/h8300/Kconfig
index d1f377f..56e890d 100644
--- a/arch/h8300/Kconfig
+++ b/arch/h8300/Kconfig
@@ -4,6 +4,7 @@ config H8300
 	select HAVE_IDE
 	select HAVE_GENERIC_HARDIRQS
 	select GENERIC_IRQ_SHOW
+	select GENERIC_CPU_DEVICES
 
 config SYMBOL_PREFIX
 	string
diff --git a/arch/m68k/Kconfig b/arch/m68k/Kconfig
index 81fdaa7..f755ecf 100644
--- a/arch/m68k/Kconfig
+++ b/arch/m68k/Kconfig
@@ -6,6 +6,7 @@ config M68K
 	select HAVE_GENERIC_HARDIRQS
 	select GENERIC_IRQ_SHOW
 	select ARCH_HAVE_NMI_SAFE_CMPXCHG if RMW_INSNS
+	select GENERIC_CPU_DEVICES
 
 config RWSEM_GENERIC_SPINLOCK
 	bool
diff --git a/arch/microblaze/Kconfig b/arch/microblaze/Kconfig
index e446bab..16ca257 100644
--- a/arch/microblaze/Kconfig
+++ b/arch/microblaze/Kconfig
@@ -17,6 +17,7 @@ config MICROBLAZE
 	select HAVE_GENERIC_HARDIRQS
 	select GENERIC_IRQ_PROBE
 	select GENERIC_IRQ_SHOW
+	select GENERIC_CPU_DEVICES
 
 config SWAP
 	def_bool n
diff --git a/arch/openrisc/Kconfig b/arch/openrisc/Kconfig
index e518a5a..978e0bd 100644
--- a/arch/openrisc/Kconfig
+++ b/arch/openrisc/Kconfig
@@ -15,6 +15,7 @@ config OPENRISC
 	select GENERIC_IRQ_PROBE
 	select GENERIC_IRQ_SHOW
 	select GENERIC_IOMAP
+	select GENERIC_CPU_DEVICES
 
 config MMU
 	def_bool y
diff --git a/arch/score/Kconfig b/arch/score/Kconfig
index 8b0c946..51cbf1e 100644
--- a/arch/score/Kconfig
+++ b/arch/score/Kconfig
@@ -7,6 +7,7 @@ config SCORE
        select HAVE_MEMBLOCK
        select HAVE_MEMBLOCK_NODE_MAP
        select ARCH_DISCARD_MEMBLOCK
+       select GENERIC_CPU_DEVICES
 
 choice
 	prompt "System type"
diff --git a/arch/xtensa/Kconfig b/arch/xtensa/Kconfig
index c346ccd..8a3f835 100644
--- a/arch/xtensa/Kconfig
+++ b/arch/xtensa/Kconfig
@@ -9,6 +9,7 @@ config XTENSA
 	select HAVE_IDE
 	select HAVE_GENERIC_HARDIRQS
 	select GENERIC_IRQ_SHOW
+	select GENERIC_CPU_DEVICES
 	help
 	  Xtensa processors are 32-bit RISC machines designed by Tensilica
 	  primarily for embedded systems.  These processors are both
diff --git a/drivers/base/Kconfig b/drivers/base/Kconfig
index 21cf46f..4ebc5d3 100644
--- a/drivers/base/Kconfig
+++ b/drivers/base/Kconfig
@@ -172,6 +172,10 @@ config SYS_HYPERVISOR
 	bool
 	default n
 
+config GENERIC_CPU_DEVICES
+	bool
+	default n
+
 source "drivers/base/regmap/Kconfig"
 
 endmenu
diff --git a/drivers/base/cpu.c b/drivers/base/cpu.c
index 9a5578e..246f3b3 100644
--- a/drivers/base/cpu.c
+++ b/drivers/base/cpu.c
@@ -10,6 +10,7 @@
 #include <linux/device.h>
 #include <linux/node.h>
 #include <linux/gfp.h>
+#include <linux/slab.h>
 
 #include "base.h"
 
@@ -274,6 +275,26 @@ bool cpu_is_hotpluggable(unsigned cpu)
 }
 EXPORT_SYMBOL_GPL(cpu_is_hotpluggable);
 
+static int cpu_dev_register_generic(void)
+{
+#ifdef CONFIG_GENERIC_CPU_DEVICES
+	struct cpu *cpu_devices;
+	int err;
+	int i;
+
+	cpu_devices = kcalloc(nr_cpu_ids, sizeof(cpu_devices[0]), GFP_KERNEL);
+	if (!cpu_devices)
+		return -ENOMEM;
+
+	for_each_possible_cpu(i) {
+		err = register_cpu(&cpu_devices[i], i);
+		if (err)
+			return err;
+	}
+#endif
+	return 0;
+}
+
 int __init cpu_dev_init(void)
 {
 	int err;
@@ -284,6 +305,9 @@ int __init cpu_dev_init(void)
 
 #if defined(CONFIG_SCHED_MC) || defined(CONFIG_SCHED_SMT)
 	err = sched_create_sysfs_power_savings_entries(cpu_subsys.dev_root);
+	if (err)
+		return err;
 #endif
-	return err;
+
+	return cpu_dev_register_generic();
 }
-- 
1.7.8.2

Re: [PATCH 1/2] topology: Check for missing CPU devices

[Linus Torvalds]

Ok, both of the patches look sane to me, but it would really be nice
to hear from somebody with the actual affected architectures, and get
a tested-by.

Testing it on hacked-up x86 sounds fine, but doesn't quite have the
same kind of "yes, this fixes the actual problem" feel to it.

Also, can you clarify: does the second patch make the first patch just
an "irrelevant safety net", or are there possible callers of
topology_add_dev() that could cause problems? I'm just wondering
whether maybe the safety net ends up then possibly hiding some future
bug where we (once more) don't register a cpu and then never really
notice?

Or am I just being difficult?

                     Linus

On Sun, Jan 8, 2012 at 12:48 PM, Ben Hutchings <ben@decadent.org.uk> wrote:
> Commit ccbc60d3e19a1b6ae66ca0d89b3da02dde62088b ('topology: Provide
> CPU topology in sysfs in !SMP configurations') causes a crash at boot
> on a several architectures.  The topology sysfs code assumes that
> there is a CPU device for each online CPU whereas some architectures
> that do not support SMP or cpufreq do not register any CPU devices.
> Check for this before trying to use a device.
>
> Signed-off-by: Ben Hutchings <ben@decadent.org.uk>
> ---
>  drivers/base/topology.c |    5 ++++-
>  1 files changed, 4 insertions(+), 1 deletions(-)
>
> diff --git a/drivers/base/topology.c b/drivers/base/topology.c
> index ae989c5..4467c85 100644
> --- a/drivers/base/topology.c
> +++ b/drivers/base/topology.c
> @@ -147,6 +147,8 @@ static int __cpuinit topology_add_dev(unsigned int cpu)
>  {
>        struct device *dev = get_cpu_device(cpu);
>
> +       if (!dev)
> +               return -ENODEV;
>        return sysfs_create_group(&dev->kobj, &topology_attr_group);
>  }
>
> @@ -154,7 +156,8 @@ static void __cpuinit topology_remove_dev(unsigned int cpu)
>  {
>        struct device *dev = get_cpu_device(cpu);
>
> -       sysfs_remove_group(&dev->kobj, &topology_attr_group);
> +       if (dev)
> +               sysfs_remove_group(&dev->kobj, &topology_attr_group);
>  }
>
>  static int __cpuinit topology_cpu_callback(struct notifier_block *nfb,
> --
> 1.7.8.2
>
>
>

Re: [PATCH 1/2] topology: Check for missing CPU devices

[richard -rw- weinberger]

On Mon, Jan 9, 2012 at 1:18 AM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
> Ok, both of the patches look sane to me, but it would really be nice
> to hear from somebody with the actual affected architectures, and get
> a tested-by.

UML is affected:
https://lkml.org/lkml/2012/1/8/186

I wasted an hour finding out why it is crashing.
Instead of testing kernels I really should read more LKML. ;-)

-- 
Thanks,
//richard

Re: [PATCH 1/2] topology: Check for missing CPU devices

[Linus Torvalds]

On Sun, Jan 8, 2012 at 5:06 PM, richard -rw- weinberger
<richard.weinberger@gmail.com> wrote:
> On Mon, Jan 9, 2012 at 1:18 AM, Linus Torvalds
> <torvalds@linux-foundation.org> wrote:
>> Ok, both of the patches look sane to me, but it would really be nice
>> to hear from somebody with the actual affected architectures, and get
>> a tested-by.
>
> UML is affected:
> https://lkml.org/lkml/2012/1/8/186
>
> I wasted an hour finding out why it is crashing.
> Instead of testing kernels I really should read more LKML. ;-)

Hmm.

Ben - how about that

  static DEFINE_PER_CPU(struct cpu, cpu_devices);

approach that Richard uses in his patch, instead of the kcalloc? And
clearly UM should also do that CONFIG_GENERIC_CPU_DEVICES thing with
your patch.

Richard - does Ben's patch work for  you too if you just add "select
GENERIC_CPU_DEVICES" in the UM Kconfig too (Kconfig.common, probably)?

                      Linus

Re: [PATCH 1/2] topology: Check for missing CPU devices

[Ben Hutchings]

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

On Sun, 2012-01-08 at 16:18 -0800, Linus Torvalds wrote:
> Ok, both of the patches look sane to me, but it would really be nice
> to hear from somebody with the actual affected architectures, and get
> a tested-by.
> 
> Testing it on hacked-up x86 sounds fine, but doesn't quite have the
> same kind of "yes, this fixes the actual problem" feel to it.

Indeed.

> Also, can you clarify: does the second patch make the first patch just
> an "irrelevant safety net", or are there possible callers of
> topology_add_dev() that could cause problems? I'm just wondering
> whether maybe the safety net ends up then possibly hiding some future
> bug where we (once more) don't register a cpu and then never really
> notice?
[...]

driver_init() doesn't check that cpu_dev_init() - or any of the other
functions it calls - is successful.  So in theory at least we could boot
and still have no CPU devices after the first patch.

Ben.

-- 
Ben Hutchings
Life is what happens to you while you're busy making other plans.
                                                               - John Lennon

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

Re: [PATCH 1/2] topology: Check for missing CPU devices

[Ben Hutchings]

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

On Sun, 2012-01-08 at 17:29 -0800, Linus Torvalds wrote:
> On Sun, Jan 8, 2012 at 5:06 PM, richard -rw- weinberger
> <richard.weinberger@gmail.com> wrote:
> > On Mon, Jan 9, 2012 at 1:18 AM, Linus Torvalds
> > <torvalds@linux-foundation.org> wrote:
> >> Ok, both of the patches look sane to me, but it would really be nice
> >> to hear from somebody with the actual affected architectures, and get
> >> a tested-by.
> >
> > UML is affected:
> > https://lkml.org/lkml/2012/1/8/186
> >
> > I wasted an hour finding out why it is crashing.
> > Instead of testing kernels I really should read more LKML. ;-)
> 
> Hmm.
> 
> Ben - how about that
> 
>   static DEFINE_PER_CPU(struct cpu, cpu_devices);
> 
> approach that Richard uses in his patch, instead of the kcalloc?

That seems perfectly good.

> And
> clearly UM should also do that CONFIG_GENERIC_CPU_DEVICES thing with
> your patch.
>
> Richard - does Ben's patch work for  you too if you just add "select
> GENERIC_CPU_DEVICES" in the UM Kconfig too (Kconfig.common, probably)?

Sorry, I meant to cover UM as well but I couldn't see how its Kconfig
files were organised.

Ben.

-- 
Ben Hutchings
Life is what happens to you while you're busy making other plans.
                                                               - John Lennon

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

Re: [PATCH 1/2] topology: Check for missing CPU devices

[Ben Hutchings]

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

On Mon, 2012-01-09 at 02:47 +0000, Ben Hutchings wrote:
> On Sun, 2012-01-08 at 16:18 -0800, Linus Torvalds wrote:
> > Ok, both of the patches look sane to me, but it would really be nice
> > to hear from somebody with the actual affected architectures, and get
> > a tested-by.
> > 
> > Testing it on hacked-up x86 sounds fine, but doesn't quite have the
> > same kind of "yes, this fixes the actual problem" feel to it.
> 
> Indeed.
> 
> > Also, can you clarify: does the second patch make the first patch just
> > an "irrelevant safety net", or are there possible callers of
> > topology_add_dev() that could cause problems? I'm just wondering
> > whether maybe the safety net ends up then possibly hiding some future
> > bug where we (once more) don't register a cpu and then never really
> > notice?
> [...]
> 
> driver_init() doesn't check that cpu_dev_init() - or any of the other
> functions it calls - is successful.  So in theory at least we could boot
> and still have no CPU devices after the first patch.

I mean to say that we could have no CPU devices after the *second*
patch.  So the first patch is an extra defence against that.  (Though we
could just as well panic if register_cpu() fails at boot time.)

Ben.

-- 
Ben Hutchings
Life is what happens to you while you're busy making other plans.
                                                               - John Lennon

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 828 bytes --]

Re: [PATCH 1/2] topology: Check for missing CPU devices

[Richard Weinberger]

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

Am 09.01.2012 03:52, schrieb Ben Hutchings:
> On Sun, 2012-01-08 at 17:29 -0800, Linus Torvalds wrote:
>> On Sun, Jan 8, 2012 at 5:06 PM, richard -rw- weinberger
>> <richard.weinberger@gmail.com> wrote:
>>> On Mon, Jan 9, 2012 at 1:18 AM, Linus Torvalds
>>> <torvalds@linux-foundation.org> wrote:
>>>> Ok, both of the patches look sane to me, but it would really be nice
>>>> to hear from somebody with the actual affected architectures, and get
>>>> a tested-by.
>>>
>>> UML is affected:
>>> https://lkml.org/lkml/2012/1/8/186
>>>
>>> I wasted an hour finding out why it is crashing.
>>> Instead of testing kernels I really should read more LKML. ;-)
>>
>> Hmm.
>>
>> Ben - how about that
>>
>>   static DEFINE_PER_CPU(struct cpu, cpu_devices);
>>
>> approach that Richard uses in his patch, instead of the kcalloc?
> 
> That seems perfectly good.
> 
>> And
>> clearly UM should also do that CONFIG_GENERIC_CPU_DEVICES thing with
>> your patch.
>>
>> Richard - does Ben's patch work for  you too if you just add "select
>> GENERIC_CPU_DEVICES" in the UM Kconfig too (Kconfig.common, probably)?
> 

Yeah, Ben's patch solves the problem too. :-)
Ben, please add the attached patch snippet to your patch.

--

diff --git a/arch/um/Kconfig.common b/arch/um/Kconfig.common
index a923483..b37ae70 100644
--- a/arch/um/Kconfig.common
+++ b/arch/um/Kconfig.common
@@ -8,6 +8,7 @@ config UML
 	default y
 	select HAVE_GENERIC_HARDIRQS
 	select GENERIC_IRQ_SHOW
+	select GENERIC_CPU_DEVICES

 config MMU
 	bool


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 490 bytes --]

Re: [PATCH 1/2] topology: Check for missing CPU devices

[Geert Uytterhoeven]

On Sun, Jan 8, 2012 at 21:48, Ben Hutchings <ben@decadent.org.uk> wrote:
> Commit ccbc60d3e19a1b6ae66ca0d89b3da02dde62088b ('topology: Provide
> CPU topology in sysfs in !SMP configurations') causes a crash at boot
> on a several architectures.  The topology sysfs code assumes that
> there is a CPU device for each online CPU whereas some architectures
> that do not support SMP or cpufreq do not register any CPU devices.
> Check for this before trying to use a device.
>
> Signed-off-by: Ben Hutchings <ben@decadent.org.uk>

Thanks!
Works on its own or combined with "[PATCH 2/2] cpu: Register a generic
CPU device on architectures that currently do not".

Tested-by: Geert Uytterhoeven <geert@linux-m68k.org>

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 2/2] cpu: Register a generic CPU device on architectures that currently do not

[Geert Uytterhoeven]

On Sun, Jan 8, 2012 at 21:55, Ben Hutchings <ben@decadent.org.uk> wrote:
> frv, h8300, microblaze, openrisc, score and xtensa currently do not

m68k is also handled in your patch.
um is still missing.

> register a CPU device.  Add the config option GENERIC_CPU_DEVICES which
> causes a generic CPU device to be registered for each present CPU, and
> make all these architectures select it.
>
> Signed-off-by: Ben Hutchings <ben@decadent.org.uk>

Thanks!
On m68k, it works on its own or combined with "[PATCH 1/2] topology:
Check for missing CPU devices."

It adds

/sys/devices/system/cpu/cpu0
|-- subsystem -> ../../../../bus/cpu
|-- topology
|   |-- core_id
|   |-- core_siblings
|   |-- core_siblings_list
|   |-- physical_package_id
|   |-- thread_siblings
|   `-- thread_siblings_list
`-- uevent

and

/sys/bus/cpu/devices/cpu0 -> ../../../devices/system/cpu/cpu0

Tested-by: Geert Uytterhoeven <geert@linux-m68k.org>

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 1/2] topology: Check for missing CPU devices

[Geert Uytterhoeven]

On Mon, Jan 9, 2012 at 02:06, richard -rw- weinberger
<richard.weinberger@gmail.com> wrote:
> Instead of testing kernels I really should read more LKML. ;-)

As an architecture maintainer, you want to read at least linux-arch.

Gr{oetje,eeting}s,

                        Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
                                -- Linus Torvalds

Re: [PATCH 1/2] topology: Check for missing CPU devices

[Linus Torvalds]

On Sun, Jan 8, 2012 at 6:56 PM, Ben Hutchings <ben@decadent.org.uk> wrote:
>
> I mean to say that we could have no CPU devices after the *second*
> patch.  So the first patch is an extra defence against that.  (Though we
> could just as well panic if register_cpu() fails at boot time.)

I think I'd rather just panic - if you have allocation issues during
early boot, the machine is hosed anyway - and then get rid of the
first patch?

Willing to send out a new patch along those lines (and with UML added)?

Thanks,
                  Linus