[PATCH 1/2] of: Split early_init_dt_scan into two parts

[PATCH 1/2] of: Split early_init_dt_scan into two parts

[Laura Abbott]

Currently, early_init_dt_scan validates the header, sets the
boot params, and scans for chosen/memory all in one function.
Split this up into two separate functions (validation/setting
boot params in one, scanning in another) to allow for
additional setup between boot params and scanning the memory.

Signed-off-by: Laura Abbott <lauraa@codeaurora.org>
---
 drivers/of/fdt.c       | 18 +++++++++++++++++-
 include/linux/of_fdt.h |  2 ++
 2 files changed, 19 insertions(+), 1 deletion(-)

diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
index c4cddf0..55bfca9 100644
--- a/drivers/of/fdt.c
+++ b/drivers/of/fdt.c
@@ -922,7 +922,7 @@ int __init __weak early_init_dt_reserve_memory_arch(phys_addr_t base,
 }
 #endif
 
-bool __init early_init_dt_scan(void *params)
+bool __init early_init_dt_verify(void *params)
 {
 	if (!params)
 		return false;
@@ -936,6 +936,12 @@ bool __init early_init_dt_scan(void *params)
 		return false;
 	}
 
+	return true;
+}
+
+
+void __init early_init_dt_scan_all(void)
+{
 	/* Retrieve various information from the /chosen node */
 	of_scan_flat_dt(early_init_dt_scan_chosen, boot_command_line);
 
@@ -944,7 +950,17 @@ bool __init early_init_dt_scan(void *params)
 
 	/* Setup memory, calling early_init_dt_add_memory_arch */
 	of_scan_flat_dt(early_init_dt_scan_memory, NULL);
+}
+
+bool __init early_init_dt_scan(void *params)
+{
+	bool status;
+
+	status = early_init_dt_verify(params);
+	if (!status)
+		return false;
 
+	early_init_dt_scan_all();
 	return true;
 }
 
diff --git a/include/linux/of_fdt.h b/include/linux/of_fdt.h
index 0511789..d600993 100644
--- a/include/linux/of_fdt.h
+++ b/include/linux/of_fdt.h
@@ -73,6 +73,8 @@ extern int early_init_dt_scan_root(unsigned long node, const char *uname,
 				   int depth, void *data);
 
 extern bool early_init_dt_scan(void *params);
+extern bool early_init_dt_verify(void *params);
+extern void early_init_dt_scan_all(void);
 
 extern const char *of_flat_dt_get_machine_name(void);
 extern const void *of_flat_dt_match_machine(const void *default_match,
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
hosted by The Linux Foundation

[PATCH 2/2] arm: Add back maximum bank limit

[Laura Abbott]

Commit 1c2f87c22566cd057bc8cde10c37ae9da1a1bb76
(ARM: 8025/1: Get rid of meminfo) dropped the upper bound on
the number of memory banks that can be added as there was no
technical need in the kernel. It turns out though, some bootloaders
(specifically the arndale-octa exynos boards) may pass invalid memory
information and rely on the kernel to not parse this data. This is a
bug in the bootloader but we still need to work around this.
Re-introduce a maximum bank limit per board to prevent invalid banks
from being passed to the kernel.

Reported-by: Tushar Behera <trblinux@gmail.com>
Signed-off-by: Laura Abbott <lauraa@codeaurora.org>
---
http://lists.infradead.org/pipermail/linux-arm-kernel/2014-June/265615.html

For those who did not see the discussion before
---
 arch/arm/include/asm/mach/arch.h | 10 ++++++++--
 arch/arm/kernel/atags_parse.c    |  1 +
 arch/arm/kernel/devtree.c        |  9 ++++++++-
 arch/arm/kernel/setup.c          | 20 ++++++++++++++++++++
 arch/arm/mach-exynos/exynos.c    |  1 +
 5 files changed, 38 insertions(+), 3 deletions(-)

diff --git a/arch/arm/include/asm/mach/arch.h b/arch/arm/include/asm/mach/arch.h
index 060a75e..ebcc543 100644
--- a/arch/arm/include/asm/mach/arch.h
+++ b/arch/arm/include/asm/mach/arch.h
@@ -40,6 +40,8 @@ struct machine_desc {
 	unsigned int		video_start;	/* start of video RAM	*/
 	unsigned int		video_end;	/* end of video RAM	*/
 
+	unsigned int		bank_limit;	/* maximum number of memory
+						 * banks to add */
 	unsigned char		reserve_lp0 :1;	/* never has lp0	*/
 	unsigned char		reserve_lp1 :1;	/* never has lp1	*/
 	unsigned char		reserve_lp2 :1;	/* never has lp2	*/
@@ -76,6 +78,8 @@ extern const struct machine_desc __arch_info_begin[], __arch_info_end[];
 #define for_each_machine_desc(p)			\
 	for (p = __arch_info_begin; p < __arch_info_end; p++)
 
+extern void set_max_bank_limit(const struct machine_desc *mdesc);
+
 /*
  * Set of macros to define architecture features.  This is built into
  * a table by the linker.
@@ -85,7 +89,8 @@ static const struct machine_desc __mach_desc_##_type	\
  __used							\
  __attribute__((__section__(".arch.info.init"))) = {	\
 	.nr		= MACH_TYPE_##_type,		\
-	.name		= _name,
+	.name		= _name,			\
+	.bank_limit	= 12,
 
 #define MACHINE_END				\
 };
@@ -95,6 +100,7 @@ static const struct machine_desc __mach_desc_##_name	\
  __used							\
  __attribute__((__section__(".arch.info.init"))) = {	\
 	.nr		= ~0,				\
-	.name		= _namestr,
+	.name		= _namestr,			\
+	.bank_limit	= 12,
 
 #endif
diff --git a/arch/arm/kernel/atags_parse.c b/arch/arm/kernel/atags_parse.c
index 7807ef5..6b01d20 100644
--- a/arch/arm/kernel/atags_parse.c
+++ b/arch/arm/kernel/atags_parse.c
@@ -204,6 +204,7 @@ setup_machine_tags(phys_addr_t __atags_pointer, unsigned int machine_nr)
 		dump_machine_table(); /* does not return */
 	}
 
+	set_max_bank_limit(mdesc);
 	if (__atags_pointer)
 		tags = phys_to_virt(__atags_pointer);
 	else if (mdesc->atag_offset)
diff --git a/arch/arm/kernel/devtree.c b/arch/arm/kernel/devtree.c
index e94a157..04213ce 100644
--- a/arch/arm/kernel/devtree.c
+++ b/arch/arm/kernel/devtree.c
@@ -27,6 +27,10 @@
 #include <asm/mach/arch.h>
 #include <asm/mach-types.h>
 
+void __init early_init_dt_add_memory_arch(u64 base, u64 size)
+{
+	arm_add_memory(base, size);
+}
 
 #ifdef CONFIG_SMP
 extern struct of_cpu_method __cpu_method_of_table[];
@@ -212,7 +216,7 @@ const struct machine_desc * __init setup_machine_fdt(unsigned int dt_phys)
 	mdesc_best = &__mach_desc_GENERIC_DT;
 #endif
 
-	if (!dt_phys || !early_init_dt_scan(phys_to_virt(dt_phys)))
+	if (!dt_phys || !early_init_dt_verify(phys_to_virt(dt_phys)))
 		return NULL;
 
 	mdesc = of_flat_dt_match_machine(mdesc_best, arch_get_next_mach);
@@ -237,6 +241,9 @@ const struct machine_desc * __init setup_machine_fdt(unsigned int dt_phys)
 		dump_machine_table(); /* does not return */
 	}
 
+	set_max_bank_limit(mdesc);
+	early_init_dt_scan_all();
+
 	/* Change machine number to match the mdesc we're using */
 	__machine_arch_type = mdesc->nr;
 
diff --git a/arch/arm/kernel/setup.c b/arch/arm/kernel/setup.c
index 8a16ee5..bc796e3 100644
--- a/arch/arm/kernel/setup.c
+++ b/arch/arm/kernel/setup.c
@@ -629,11 +629,31 @@ void __init dump_machine_table(void)
 		/* can't use cpu_relax() here as it may require MMU setup */;
 }
 
+static unsigned int bank_cnt;
+static unsigned int max_cnt;
+
+void set_max_bank_limit(const struct machine_desc *mdesc)
+{
+	max_cnt = mdesc->bank_limit;
+}
+
 int __init arm_add_memory(u64 start, u64 size)
 {
 	u64 aligned_start;
 
 	/*
+	 * Some buggy bootloaders rely on the old meminfo behavior of not adding
+	 * more than n banks since anything past that may contain invalid data.
+	 */
+	if (bank_cnt >= max_cnt) {
+		pr_crit("Max banks too low, ignoring memory at 0x%08llx\n",
+			(long long)start);
+		return -EINVAL;
+	}
+
+	bank_cnt++;
+
+	/*
 	 * Ensure that start/size are aligned to a page boundary.
 	 * Size is appropriately rounded down, start is rounded up.
 	 */
diff --git a/arch/arm/mach-exynos/exynos.c b/arch/arm/mach-exynos/exynos.c
index f38cf7c..91283fd 100644
--- a/arch/arm/mach-exynos/exynos.c
+++ b/arch/arm/mach-exynos/exynos.c
@@ -350,4 +350,5 @@ DT_MACHINE_START(EXYNOS_DT, "SAMSUNG EXYNOS (Flattened Device Tree)")
 	.dt_compat	= exynos_dt_compat,
 	.restart	= exynos_restart,
 	.reserve	= exynos_reserve,
+	.bank_limit     = 8,
 MACHINE_END
-- 
The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
hosted by The Linux Foundation

Re: [PATCH 2/2] arm: Add back maximum bank limit

[Andreas Färber]

Hi Laura,

Am 29.06.2014 21:06, schrieb Laura Abbott:
> Commit 1c2f87c22566cd057bc8cde10c37ae9da1a1bb76
> (ARM: 8025/1: Get rid of meminfo) dropped the upper bound on
> the number of memory banks that can be added as there was no
> technical need in the kernel. It turns out though, some bootloaders
> (specifically the arndale-octa exynos boards) may pass invalid memory
> information and rely on the kernel to not parse this data. This is a
> bug in the bootloader but we still need to work around this.
> Re-introduce a maximum bank limit per board to prevent invalid banks
> from being passed to the kernel.
> 
> Reported-by: Tushar Behera <trblinux@gmail.com>
> Signed-off-by: Laura Abbott <lauraa@codeaurora.org>
> ---
> http://lists.infradead.org/pipermail/linux-arm-kernel/2014-June/265615.html

Tested-by: Andreas Färber <afaerber@suse.de>

[    0.000000] Max banks too low, ignoring memory at 0xfbffffaf
[    0.000000] Max banks too low, ignoring memory at 0xfffedbff
[    0.000000] Max banks too low, ignoring memory at 0xffbbffd9
[    0.000000] Max banks too low, ignoring memory at 0xffefefef

Thanks,
Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

Re: [PATCH 2/2] arm: Add back maximum bank limit

[Grant Likely]

On Sun, 29 Jun 2014 12:06:24 -0700, Laura Abbott <lauraa@codeaurora.org> wrote:
> Commit 1c2f87c22566cd057bc8cde10c37ae9da1a1bb76
> (ARM: 8025/1: Get rid of meminfo) dropped the upper bound on
> the number of memory banks that can be added as there was no
> technical need in the kernel. It turns out though, some bootloaders
> (specifically the arndale-octa exynos boards) may pass invalid memory
> information and rely on the kernel to not parse this data. This is a
> bug in the bootloader but we still need to work around this.
> Re-introduce a maximum bank limit per board to prevent invalid banks
> from being passed to the kernel.
> 
> Reported-by: Tushar Behera <trblinux@gmail.com>
> Signed-off-by: Laura Abbott <lauraa@codeaurora.org>

Hi Laura,

This is more invasive than I'd like. I think it can be fixed more
elegantly. Some suggestions below...

> ---
> http://lists.infradead.org/pipermail/linux-arm-kernel/2014-June/265615.html
> 
> For those who did not see the discussion before
> ---
>  arch/arm/include/asm/mach/arch.h | 10 ++++++++--
>  arch/arm/kernel/atags_parse.c    |  1 +
>  arch/arm/kernel/devtree.c        |  9 ++++++++-
>  arch/arm/kernel/setup.c          | 20 ++++++++++++++++++++
>  arch/arm/mach-exynos/exynos.c    |  1 +
>  5 files changed, 38 insertions(+), 3 deletions(-)
> 
> diff --git a/arch/arm/include/asm/mach/arch.h b/arch/arm/include/asm/mach/arch.h
> index 060a75e..ebcc543 100644
> --- a/arch/arm/include/asm/mach/arch.h
> +++ b/arch/arm/include/asm/mach/arch.h
> @@ -40,6 +40,8 @@ struct machine_desc {
>  	unsigned int		video_start;	/* start of video RAM	*/
>  	unsigned int		video_end;	/* end of video RAM	*/
>  
> +	unsigned int		bank_limit;	/* maximum number of memory
> +						 * banks to add */
>  	unsigned char		reserve_lp0 :1;	/* never has lp0	*/
>  	unsigned char		reserve_lp1 :1;	/* never has lp1	*/
>  	unsigned char		reserve_lp2 :1;	/* never has lp2	*/
> @@ -76,6 +78,8 @@ extern const struct machine_desc __arch_info_begin[], __arch_info_end[];
>  #define for_each_machine_desc(p)			\
>  	for (p = __arch_info_begin; p < __arch_info_end; p++)
>  
> +extern void set_max_bank_limit(const struct machine_desc *mdesc);
> +
>  /*
>   * Set of macros to define architecture features.  This is built into
>   * a table by the linker.
> @@ -85,7 +89,8 @@ static const struct machine_desc __mach_desc_##_type	\
>   __used							\
>   __attribute__((__section__(".arch.info.init"))) = {	\
>  	.nr		= MACH_TYPE_##_type,		\
> -	.name		= _name,
> +	.name		= _name,			\
> +	.bank_limit	= 12,

The hard limit of 12 seems entirely arbitrary. I don't like the idea of
reintroducing it.  There shouldn't be any need for a default at all.
Leave it as zero, and the limit will only apply if a value has been set.
12 seems like an artificial limit. (more below)

>  
>  #define MACHINE_END				\
>  };
> @@ -95,6 +100,7 @@ static const struct machine_desc __mach_desc_##_name	\
>   __used							\
>   __attribute__((__section__(".arch.info.init"))) = {	\
>  	.nr		= ~0,				\
> -	.name		= _namestr,
> +	.name		= _namestr,			\
> +	.bank_limit	= 12,
>  
>  #endif
> diff --git a/arch/arm/kernel/atags_parse.c b/arch/arm/kernel/atags_parse.c
> index 7807ef5..6b01d20 100644
> --- a/arch/arm/kernel/atags_parse.c
> +++ b/arch/arm/kernel/atags_parse.c
> @@ -204,6 +204,7 @@ setup_machine_tags(phys_addr_t __atags_pointer, unsigned int machine_nr)
>  		dump_machine_table(); /* does not return */
>  	}
>  
> +	set_max_bank_limit(mdesc);

Is there actually a bug on any atags booting? If this is firmly in the
realm of firmware workaround then I would drop the change from the atags
boot path.

>  	if (__atags_pointer)
>  		tags = phys_to_virt(__atags_pointer);
>  	else if (mdesc->atag_offset)
> diff --git a/arch/arm/kernel/devtree.c b/arch/arm/kernel/devtree.c
> index e94a157..04213ce 100644
> --- a/arch/arm/kernel/devtree.c
> +++ b/arch/arm/kernel/devtree.c
> @@ -27,6 +27,10 @@
>  #include <asm/mach/arch.h>
>  #include <asm/mach-types.h>
>  
> +void __init early_init_dt_add_memory_arch(u64 base, u64 size)
> +{
> +	arm_add_memory(base, size);
> +}
>  
>  #ifdef CONFIG_SMP
>  extern struct of_cpu_method __cpu_method_of_table[];
> @@ -212,7 +216,7 @@ const struct machine_desc * __init setup_machine_fdt(unsigned int dt_phys)
>  	mdesc_best = &__mach_desc_GENERIC_DT;
>  #endif
>  
> -	if (!dt_phys || !early_init_dt_scan(phys_to_virt(dt_phys)))
> +	if (!dt_phys || !early_init_dt_verify(phys_to_virt(dt_phys)))
>  		return NULL;

Instead of splitting early_init_dt_scan(), the call to
early_init_dt_scan() could probably be moved after the
of_flat_dt_match_machine() call. It's at least worth a try. Looking at
the code I don't see anything obvious that requires the
early_init_dt_scan() code to run first.

Once you've got the mdesc pointer, you've could set the limit before
doing the full scan. Or, better yet because this is a fix for broken
data, you could call a dt_fixup hook on the mdesc. Then exynos is the
only platform that needs special treatment. The best thing for exynos to
do is fix the buggy data by clearing the bogus entries. Then there's no
need to reintroduce early_init_dt_add_memory_arch() for ARM.

>  
>  	mdesc = of_flat_dt_match_machine(mdesc_best, arch_get_next_mach);
> @@ -237,6 +241,9 @@ const struct machine_desc * __init setup_machine_fdt(unsigned int dt_phys)
>  		dump_machine_table(); /* does not return */
>  	}
>  
> +	set_max_bank_limit(mdesc);
> +	early_init_dt_scan_all();
> +
>  	/* Change machine number to match the mdesc we're using */
>  	__machine_arch_type = mdesc->nr;
>  
> diff --git a/arch/arm/kernel/setup.c b/arch/arm/kernel/setup.c
> index 8a16ee5..bc796e3 100644
> --- a/arch/arm/kernel/setup.c
> +++ b/arch/arm/kernel/setup.c
> @@ -629,11 +629,31 @@ void __init dump_machine_table(void)
>  		/* can't use cpu_relax() here as it may require MMU setup */;
>  }
>  
> +static unsigned int bank_cnt;
> +static unsigned int max_cnt;
> +
> +void set_max_bank_limit(const struct machine_desc *mdesc)
> +{
> +	max_cnt = mdesc->bank_limit;
> +}
> +
>  int __init arm_add_memory(u64 start, u64 size)
>  {
>  	u64 aligned_start;
>  
>  	/*
> +	 * Some buggy bootloaders rely on the old meminfo behavior of not adding
> +	 * more than n banks since anything past that may contain invalid data.
> +	 */
> +	if (bank_cnt >= max_cnt) {

if (max_cnt && (bank_cnt >= max_cnt)) {

(But unnecessary if the suggestion to reorganize setup_machine_fdt()
works).

> +		pr_crit("Max banks too low, ignoring memory at 0x%08llx\n",
> +			(long long)start);
> +		return -EINVAL;
> +	}
> +
> +	bank_cnt++;
> +
> +	/*
>  	 * Ensure that start/size are aligned to a page boundary.
>  	 * Size is appropriately rounded down, start is rounded up.
>  	 */
> diff --git a/arch/arm/mach-exynos/exynos.c b/arch/arm/mach-exynos/exynos.c
> index f38cf7c..91283fd 100644
> --- a/arch/arm/mach-exynos/exynos.c
> +++ b/arch/arm/mach-exynos/exynos.c
> @@ -350,4 +350,5 @@ DT_MACHINE_START(EXYNOS_DT, "SAMSUNG EXYNOS (Flattened Device Tree)")
>  	.dt_compat	= exynos_dt_compat,
>  	.restart	= exynos_restart,
>  	.reserve	= exynos_reserve,
> +	.bank_limit     = 8,
>  MACHINE_END
> -- 
> The Qualcomm Innovation Center, Inc. is a member of the Code Aurora Forum,
> hosted by The Linux Foundation
> 

Re: [PATCH 2/2] arm: Add back maximum bank limit

[Laura Abbott]

On 6/30/2014 3:43 AM, Grant Likely wrote:
> 
> Instead of splitting early_init_dt_scan(), the call to
> early_init_dt_scan() could probably be moved after the
> of_flat_dt_match_machine() call. It's at least worth a try. Looking at
> the code I don't see anything obvious that requires the
> early_init_dt_scan() code to run first.
>

of_flat_dt_match_machine -> of_flat_dt_match which uses
initial_boot_params which isn't set until early_init_dt_scan. I had the
same thought about just rearranging but we still need something to be
set before doing the scan, hence the split.
 
> Once you've got the mdesc pointer, you've could set the limit before
> doing the full scan. Or, better yet because this is a fix for broken
> data, you could call a dt_fixup hook on the mdesc. Then exynos is the
> only platform that needs special treatment. The best thing for exynos to
> do is fix the buggy data by clearing the bogus entries. Then there's no
> need to reintroduce early_init_dt_add_memory_arch() for ARM.
> 

The fixup hook does sound better. The flow would become

setup_machine_fdt
	- early_init_dt_verify
	- of_flat_dt_match_machine
	- mdesc->dt_fixup
	- early_init_dt_scan_all

I notice that powerpc already has a DT fixup infrastructure but I don't see
any such thing for ARM. How much of that do you think could and should be
leveraged for ARM?

Thanks,
Laura

-- 
Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum,
hosted by The Linux Foundation

Re: [PATCH 2/2] arm: Add back maximum bank limit

[Grant Likely]

On Mon, Jun 30, 2014 at 7:03 PM, Laura Abbott <lauraa@codeaurora.org> wrote:
> On 6/30/2014 3:43 AM, Grant Likely wrote:
>>
>> Instead of splitting early_init_dt_scan(), the call to
>> early_init_dt_scan() could probably be moved after the
>> of_flat_dt_match_machine() call. It's at least worth a try. Looking at
>> the code I don't see anything obvious that requires the
>> early_init_dt_scan() code to run first.
>>
>
> of_flat_dt_match_machine -> of_flat_dt_match which uses
> initial_boot_params which isn't set until early_init_dt_scan. I had the
> same thought about just rearranging but we still need something to be
> set before doing the scan, hence the split.

Fair enough.

>> Once you've got the mdesc pointer, you've could set the limit before
>> doing the full scan. Or, better yet because this is a fix for broken
>> data, you could call a dt_fixup hook on the mdesc. Then exynos is the
>> only platform that needs special treatment. The best thing for exynos to
>> do is fix the buggy data by clearing the bogus entries. Then there's no
>> need to reintroduce early_init_dt_add_memory_arch() for ARM.
>>
>
> The fixup hook does sound better. The flow would become
>
> setup_machine_fdt
>         - early_init_dt_verify
>         - of_flat_dt_match_machine
>         - mdesc->dt_fixup
>         - early_init_dt_scan_all
>
> I notice that powerpc already has a DT fixup infrastructure but I don't see
> any such thing for ARM. How much of that do you think could and should be
> leveraged for ARM?

Not much. The PowerPC fixup infrastructure is based on having an
OpenFirmware implementation and the fixups are performed by executing
Forth commands in OFW before flattening the tree and killing OFW
context. For ARM, any DT fixup hooks should directly use libfdt calls.

g.