[PATCH 0/3] ARM: kernel: cpu_logical_map misc fixes

[PATCH 0/3] ARM: kernel: cpu_logical_map misc fixes

[Lorenzo Pieralisi]

Hi Russell, Rob,

this series contains simple fixes/improvements related to the DT
cpu_logical_map initialization code. I would like to get these patches
in in order to have a stable base on top of which I can send the DT topology
bindings and cpu_logical_map bindings/parsing code updates for arm and arm64.

Thank you very much,
Lorenzo

Lorenzo Pieralisi (3):
  ARM: DT: kernel: move temporary cpu map stack array to static data
  ARM: kernel: fix arm_dt_init_cpu_maps() to skip non-cpu nodes
  ARM: kernel: fix __cpu_logical_map default initialization

 arch/arm/include/asm/cputype.h  |  2 ++
 arch/arm/include/asm/smp_plat.h |  2 +-
 arch/arm/kernel/devtree.c       | 14 ++++++++++----
 arch/arm/kernel/setup.c         |  2 +-
 4 files changed, 14 insertions(+), 6 deletions(-)

-- 
1.8.2.2

[PATCH 1/3] ARM: DT: kernel: move temporary cpu map stack array to static data

[Lorenzo Pieralisi]

As the number of CPUs increase, the temporary array allocated on the
stack in arm_dt_init_cpu_maps() can become too big and trigger stack
issues.
This patch moves the allocated memory to static __initdata so that stack
data is not used anymore to allocate the temporary array.
Memory is marked as __initdata since it need not be persistent after boot.

Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi-5wv7dgnIgG8@public.gmane.org>
---
 arch/arm/kernel/devtree.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/arch/arm/kernel/devtree.c b/arch/arm/kernel/devtree.c
index 5af04f6..3d652e4f 100644
--- a/arch/arm/kernel/devtree.c
+++ b/arch/arm/kernel/devtree.c
@@ -78,11 +78,12 @@ void __init arm_dt_init_cpu_maps(void)
 	 * contain a list of MPIDR[23:0] values where MPIDR[31:24] must
 	 * read as 0.
 	 */
+	static u32 tmp_map[NR_CPUS] __initdata = {
+				[0 ... NR_CPUS-1] = UINT_MAX };
 	struct device_node *cpu, *cpus;
 	u32 i, j, cpuidx = 1;
 	u32 mpidr = is_smp() ? read_cpuid_mpidr() & MPIDR_HWID_BITMASK : 0;
 
-	u32 tmp_map[NR_CPUS] = { [0 ... NR_CPUS-1] = UINT_MAX };
 	bool bootcpu_valid = false;
 	cpus = of_find_node_by_path("/cpus");
 
-- 
1.8.2.2

[PATCH 2/3] ARM: kernel: fix arm_dt_init_cpu_maps() to skip non-cpu nodes

[Lorenzo Pieralisi]

The introduction of the cpu-map topology node in the cpus node implies
that cpus node might have children that are not cpu nodes. The DT
parsing code needs updating otherwise it would check for cpu nodes
properties in nodes that are not required to contain them, resulting
in warnings that have no bearing on bindings defined in the dts source file.

Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi-5wv7dgnIgG8@public.gmane.org>
---
 arch/arm/kernel/devtree.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/arch/arm/kernel/devtree.c b/arch/arm/kernel/devtree.c
index 3d652e4f..d2293c0 100644
--- a/arch/arm/kernel/devtree.c
+++ b/arch/arm/kernel/devtree.c
@@ -93,6 +93,9 @@ void __init arm_dt_init_cpu_maps(void)
 	for_each_child_of_node(cpus, cpu) {
 		u32 hwid;
 
+		if (of_node_cmp(cpu->type, "cpu"))
+			continue;
+
 		pr_debug(" * %s...\n", cpu->full_name);
 		/*
 		 * A device tree containing CPU nodes with missing "reg"
-- 
1.8.2.2

[PATCH 3/3] ARM: kernel: fix __cpu_logical_map default initialization

[Lorenzo Pieralisi]

For legacy reasons, the __cpu_logical_map array is initialized to 0.
On old pre-DT ARM platforms, smp_setup_processor_id() generates
__cpu_logical_map entries at boot before the number of possible CPUs is
set-up, with values that can be considered valid MPIDRs even if they are
not present in the system booting; this implies that the __cpu_logical_map[]
might end up containing MPIDR values that can be considered valid at logical
indexes corresponding to CPUs that are not marked as possible.

To prevent issues with the current implementation, this patch defines an
MPIDR_INVALID value, statically initializes the __cpu_logical_map[] array to it
and allows DT parsing code to overwrite values in the __cpu_logical_map array
that were erroneously set-up in smp_setup_processor_id().

Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi-5wv7dgnIgG8@public.gmane.org>
CC: Will Deacon <will.deacon-5wv7dgnIgG8@public.gmane.org>
---
 arch/arm/include/asm/cputype.h  |  2 ++
 arch/arm/include/asm/smp_plat.h |  2 +-
 arch/arm/kernel/devtree.c       | 10 ++++++----
 arch/arm/kernel/setup.c         |  2 +-
 4 files changed, 10 insertions(+), 6 deletions(-)

diff --git a/arch/arm/include/asm/cputype.h b/arch/arm/include/asm/cputype.h
index 7652712..dba62cb 100644
--- a/arch/arm/include/asm/cputype.h
+++ b/arch/arm/include/asm/cputype.h
@@ -32,6 +32,8 @@
 
 #define MPIDR_HWID_BITMASK 0xFFFFFF
 
+#define MPIDR_INVALID (~MPIDR_HWID_BITMASK)
+
 #define MPIDR_LEVEL_BITS 8
 #define MPIDR_LEVEL_MASK ((1 << MPIDR_LEVEL_BITS) - 1)
 
diff --git a/arch/arm/include/asm/smp_plat.h b/arch/arm/include/asm/smp_plat.h
index aaa61b6..e789832 100644
--- a/arch/arm/include/asm/smp_plat.h
+++ b/arch/arm/include/asm/smp_plat.h
@@ -49,7 +49,7 @@ static inline int cache_ops_need_broadcast(void)
 /*
  * Logical CPU mapping.
  */
-extern int __cpu_logical_map[];
+extern u32 __cpu_logical_map[];
 #define cpu_logical_map(cpu)	__cpu_logical_map[cpu]
 /*
  * Retrieve logical cpu index corresponding to a given MPIDR[23:0]
diff --git a/arch/arm/kernel/devtree.c b/arch/arm/kernel/devtree.c
index d2293c0..08f012e 100644
--- a/arch/arm/kernel/devtree.c
+++ b/arch/arm/kernel/devtree.c
@@ -79,12 +79,12 @@ void __init arm_dt_init_cpu_maps(void)
 	 * read as 0.
 	 */
 	static u32 tmp_map[NR_CPUS] __initdata = {
-				[0 ... NR_CPUS-1] = UINT_MAX };
+				[0 ... NR_CPUS-1] = MPIDR_INVALID };
 	struct device_node *cpu, *cpus;
 	u32 i, j, cpuidx = 1;
 	u32 mpidr = is_smp() ? read_cpuid_mpidr() & MPIDR_HWID_BITMASK : 0;
-
 	bool bootcpu_valid = false;
+
 	cpus = of_find_node_by_path("/cpus");
 
 	if (!cpus)
@@ -160,11 +160,13 @@ void __init arm_dt_init_cpu_maps(void)
 	/*
 	 * Since the boot CPU node contains proper data, and all nodes have
 	 * a reg property, the DT CPU list can be considered valid and the
-	 * logical map created in smp_setup_processor_id() can be overridden
+	 * logical map created in smp_setup_processor_id() can be overridden,
+	 * inclusive of entries that must contain invalid MPIDRs
 	 */
+	memcpy(__cpu_logical_map, tmp_map,
+			nr_cpu_ids * sizeof(tmp_map[0]));
 	for (i = 0; i < cpuidx; i++) {
 		set_cpu_possible(i, true);
-		cpu_logical_map(i) = tmp_map[i];
 		pr_debug("cpu logical map 0x%x\n", cpu_logical_map(i));
 	}
 }
diff --git a/arch/arm/kernel/setup.c b/arch/arm/kernel/setup.c
index 6ae71b7..eeac924 100644
--- a/arch/arm/kernel/setup.c
+++ b/arch/arm/kernel/setup.c
@@ -457,7 +457,7 @@ void notrace cpu_init(void)
 	    : "r14");
 }
 
-int __cpu_logical_map[NR_CPUS];
+u32 __cpu_logical_map[NR_CPUS] = { [0 ... NR_CPUS-1] = MPIDR_INVALID };
 
 void __init smp_setup_processor_id(void)
 {
-- 
1.8.2.2

Re: [PATCH 3/3] ARM: kernel: fix __cpu_logical_map default initialization

[Will Deacon]

On Thu, May 16, 2013 at 04:34:07PM +0100, Lorenzo Pieralisi wrote:
> For legacy reasons, the __cpu_logical_map array is initialized to 0.
> On old pre-DT ARM platforms, smp_setup_processor_id() generates
> __cpu_logical_map entries at boot before the number of possible CPUs is
> set-up, with values that can be considered valid MPIDRs even if they are
> not present in the system booting; this implies that the __cpu_logical_map[]
> might end up containing MPIDR values that can be considered valid at logical
> indexes corresponding to CPUs that are not marked as possible.
> 
> To prevent issues with the current implementation, this patch defines an
> MPIDR_INVALID value, statically initializes the __cpu_logical_map[] array to it
> and allows DT parsing code to overwrite values in the __cpu_logical_map array
> that were erroneously set-up in smp_setup_processor_id().

What issues have you actually hit with this?

> Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi-5wv7dgnIgG8@public.gmane.org>
> CC: Will Deacon <will.deacon-5wv7dgnIgG8@public.gmane.org>
> ---
>  arch/arm/include/asm/cputype.h  |  2 ++
>  arch/arm/include/asm/smp_plat.h |  2 +-
>  arch/arm/kernel/devtree.c       | 10 ++++++----
>  arch/arm/kernel/setup.c         |  2 +-
>  4 files changed, 10 insertions(+), 6 deletions(-)
> 
> diff --git a/arch/arm/include/asm/cputype.h b/arch/arm/include/asm/cputype.h
> index 7652712..dba62cb 100644
> --- a/arch/arm/include/asm/cputype.h
> +++ b/arch/arm/include/asm/cputype.h
> @@ -32,6 +32,8 @@
>  
>  #define MPIDR_HWID_BITMASK 0xFFFFFF
>  
> +#define MPIDR_INVALID (~MPIDR_HWID_BITMASK)
> +
>  #define MPIDR_LEVEL_BITS 8
>  #define MPIDR_LEVEL_MASK ((1 << MPIDR_LEVEL_BITS) - 1)
>  
> diff --git a/arch/arm/include/asm/smp_plat.h b/arch/arm/include/asm/smp_plat.h
> index aaa61b6..e789832 100644
> --- a/arch/arm/include/asm/smp_plat.h
> +++ b/arch/arm/include/asm/smp_plat.h
> @@ -49,7 +49,7 @@ static inline int cache_ops_need_broadcast(void)
>  /*
>   * Logical CPU mapping.
>   */
> -extern int __cpu_logical_map[];
> +extern u32 __cpu_logical_map[];
>  #define cpu_logical_map(cpu)	__cpu_logical_map[cpu]
>  /*
>   * Retrieve logical cpu index corresponding to a given MPIDR[23:0]
> diff --git a/arch/arm/kernel/devtree.c b/arch/arm/kernel/devtree.c
> index d2293c0..08f012e 100644
> --- a/arch/arm/kernel/devtree.c
> +++ b/arch/arm/kernel/devtree.c
> @@ -79,12 +79,12 @@ void __init arm_dt_init_cpu_maps(void)
>  	 * read as 0.
>  	 */
>  	static u32 tmp_map[NR_CPUS] __initdata = {
> -				[0 ... NR_CPUS-1] = UINT_MAX };
> +				[0 ... NR_CPUS-1] = MPIDR_INVALID };
>  	struct device_node *cpu, *cpus;
>  	u32 i, j, cpuidx = 1;
>  	u32 mpidr = is_smp() ? read_cpuid_mpidr() & MPIDR_HWID_BITMASK : 0;
> -
>  	bool bootcpu_valid = false;
> +

Random whitespace change.

Will

Re: [PATCH 3/3] ARM: kernel: fix __cpu_logical_map default initialization

[Lorenzo Pieralisi]

On Thu, May 16, 2013 at 04:50:38PM +0100, Will Deacon wrote:
> On Thu, May 16, 2013 at 04:34:07PM +0100, Lorenzo Pieralisi wrote:
> > For legacy reasons, the __cpu_logical_map array is initialized to 0.
> > On old pre-DT ARM platforms, smp_setup_processor_id() generates
> > __cpu_logical_map entries at boot before the number of possible CPUs is
> > set-up, with values that can be considered valid MPIDRs even if they are
> > not present in the system booting; this implies that the __cpu_logical_map[]
> > might end up containing MPIDR values that can be considered valid at logical
> > indexes corresponding to CPUs that are not marked as possible.
> > 
> > To prevent issues with the current implementation, this patch defines an
> > MPIDR_INVALID value, statically initializes the __cpu_logical_map[] array to it
> > and allows DT parsing code to overwrite values in the __cpu_logical_map array
> > that were erroneously set-up in smp_setup_processor_id().
> 
> What issues have you actually hit with this?

I have not hit any, I just thought it would be proper to have the
cpu_logical_map filled with only valid (ie present in HW) MPIDRs (and
by default initialized with MPIDR_INVALID, not 0s which are valid MPIDRs).

I wrote the patch while coding the MPIDR to logical CPU reverse look-up
for the CCI driver and noticed that, if for any reason we do not
constrain the look-up to possible cpus, we might get a logical index
for a CPU that does not exist since smp_setup_processor_id() initializes
the cpu_logical_map up to nr_cpu_ids, with values that might well be
real MPIDRs (but not present in HW).

If we pass an mpidr value to get_logical_index(), that function must not
return a logical index for an MPIDR that does not exist in HW. This can
happen with current code (but I do not think anyone will ever call
get_logical_index() with a mpidr value that has not been read from a CPU
coprocessor, so what I am saying is purely hypothetical).
True, we might check the return value is actually a CPU marked as possible,
provided that check is safe to carry out in low-level PM code where it is
likely to be used.

Overall it can be overkill, granted, it was just meant to clean-up the code,
no more than that.

> > Signed-off-by: Lorenzo Pieralisi <lorenzo.pieralisi-5wv7dgnIgG8@public.gmane.org>
> > CC: Will Deacon <will.deacon-5wv7dgnIgG8@public.gmane.org>
> > ---
> >  arch/arm/include/asm/cputype.h  |  2 ++
> >  arch/arm/include/asm/smp_plat.h |  2 +-
> >  arch/arm/kernel/devtree.c       | 10 ++++++----
> >  arch/arm/kernel/setup.c         |  2 +-
> >  4 files changed, 10 insertions(+), 6 deletions(-)
> > 
> > diff --git a/arch/arm/include/asm/cputype.h b/arch/arm/include/asm/cputype.h
> > index 7652712..dba62cb 100644
> > --- a/arch/arm/include/asm/cputype.h
> > +++ b/arch/arm/include/asm/cputype.h
> > @@ -32,6 +32,8 @@
> >  
> >  #define MPIDR_HWID_BITMASK 0xFFFFFF
> >  
> > +#define MPIDR_INVALID (~MPIDR_HWID_BITMASK)
> > +
> >  #define MPIDR_LEVEL_BITS 8
> >  #define MPIDR_LEVEL_MASK ((1 << MPIDR_LEVEL_BITS) - 1)
> >  
> > diff --git a/arch/arm/include/asm/smp_plat.h b/arch/arm/include/asm/smp_plat.h
> > index aaa61b6..e789832 100644
> > --- a/arch/arm/include/asm/smp_plat.h
> > +++ b/arch/arm/include/asm/smp_plat.h
> > @@ -49,7 +49,7 @@ static inline int cache_ops_need_broadcast(void)
> >  /*
> >   * Logical CPU mapping.
> >   */
> > -extern int __cpu_logical_map[];
> > +extern u32 __cpu_logical_map[];
> >  #define cpu_logical_map(cpu)	__cpu_logical_map[cpu]
> >  /*
> >   * Retrieve logical cpu index corresponding to a given MPIDR[23:0]
> > diff --git a/arch/arm/kernel/devtree.c b/arch/arm/kernel/devtree.c
> > index d2293c0..08f012e 100644
> > --- a/arch/arm/kernel/devtree.c
> > +++ b/arch/arm/kernel/devtree.c
> > @@ -79,12 +79,12 @@ void __init arm_dt_init_cpu_maps(void)
> >  	 * read as 0.
> >  	 */
> >  	static u32 tmp_map[NR_CPUS] __initdata = {
> > -				[0 ... NR_CPUS-1] = UINT_MAX };
> > +				[0 ... NR_CPUS-1] = MPIDR_INVALID };
> >  	struct device_node *cpu, *cpus;
> >  	u32 i, j, cpuidx = 1;
> >  	u32 mpidr = is_smp() ? read_cpuid_mpidr() & MPIDR_HWID_BITMASK : 0;
> > -
> >  	bool bootcpu_valid = false;
> > +
> 
> Random whitespace change.

Gah, sorry.

Thanks for having a look,
Lorenzo