[PATCH 0/2] metag: copy flattened devicetree to non-init memory

[PATCH 0/2] metag: copy flattened devicetree to non-init memory

[James Hogan]

Here are a couple more fixes for arch/metag which I'll try and get in
during the merge window. The first patch fixes a bug, where the fdt
isn't copied out of init memory so all the fdt strings will get poisoned
after init. The second patch is a trivial one.

James Hogan (2):
  metag: copy devicetree to non-init memory
  metag: prom.h: remove declaration of metag_dt_memblock_reserve()

 arch/metag/include/asm/prom.h |  2 +-
 arch/metag/kernel/devtree.c   | 17 +++++++++++++++++
 arch/metag/kernel/setup.c     |  2 ++
 3 files changed, 20 insertions(+), 1 deletion(-)

-- 
1.8.1.2

[PATCH 1/2] metag: copy devicetree to non-init memory

[James Hogan]

Make a copy of the device tree blob in non-init memory. It is required
when using built-in device tree files that the platform code copies the
blob to non-init memory prior to calling unflatten_device_tree(),
otherwise the strings that the device tree refer to will get poisoned
and potentially reused, breaking later reading of the device tree
post-init (such as compatible matching in modules, debugfs, and the
procfs interface).

Signed-off-by: James Hogan <james.hogan@imgtec.com>
---
 arch/metag/include/asm/prom.h |  1 +
 arch/metag/kernel/devtree.c   | 17 +++++++++++++++++
 arch/metag/kernel/setup.c     |  2 ++
 3 files changed, 20 insertions(+)

diff --git a/arch/metag/include/asm/prom.h b/arch/metag/include/asm/prom.h
index d881396..ccac97e 100644
--- a/arch/metag/include/asm/prom.h
+++ b/arch/metag/include/asm/prom.h
@@ -18,6 +18,7 @@
 #define HAVE_ARCH_DEVTREE_FIXUPS
 
 extern struct machine_desc *setup_machine_fdt(void *dt);
+extern void copy_fdt(void);
 extern void metag_dt_memblock_reserve(void);
 
 #endif /* __ASM_METAG_PROM_H */
diff --git a/arch/metag/kernel/devtree.c b/arch/metag/kernel/devtree.c
index 5b6b1d85..7cd0252 100644
--- a/arch/metag/kernel/devtree.c
+++ b/arch/metag/kernel/devtree.c
@@ -95,3 +95,20 @@ struct machine_desc * __init setup_machine_fdt(void *dt)
 
 	return mdesc_best;
 }
+
+/**
+ * copy_fdt - Copy device tree into non-init memory.
+ *
+ * We must copy the flattened device tree blob into non-init memory because the
+ * unflattened device tree will reference the strings in it directly.
+ */
+void __init copy_fdt(void)
+{
+	void *alloc = early_init_dt_alloc_memory_arch(
+			be32_to_cpu(initial_boot_params->totalsize), 0x40);
+	if (alloc) {
+		memcpy(alloc, initial_boot_params,
+		       be32_to_cpu(initial_boot_params->totalsize));
+		initial_boot_params = alloc;
+	}
+}
diff --git a/arch/metag/kernel/setup.c b/arch/metag/kernel/setup.c
index dd6c5ad..8792461 100644
--- a/arch/metag/kernel/setup.c
+++ b/arch/metag/kernel/setup.c
@@ -406,6 +406,8 @@ void __init setup_arch(char **cmdline_p)
 	cpu_2_hwthread_id[smp_processor_id()] = hard_processor_id();
 	hwthread_id_2_cpu[hard_processor_id()] = smp_processor_id();
 
+	/* Copy device tree blob into non-init memory before unflattening */
+	copy_fdt();
 	unflatten_device_tree();
 
 #ifdef CONFIG_SMP
-- 
1.8.1.2

[PATCH 2/2] metag: prom.h: remove declaration of metag_dt_memblock_reserve()

[James Hogan]

Metag doesn't have a metag_dt_memblock_reserve() function so remove the
declaration from asm/prom.h.

Signed-off-by: James Hogan <james.hogan@imgtec.com>
---
 arch/metag/include/asm/prom.h | 1 -
 1 file changed, 1 deletion(-)

diff --git a/arch/metag/include/asm/prom.h b/arch/metag/include/asm/prom.h
index ccac97e..d2aa35d 100644
--- a/arch/metag/include/asm/prom.h
+++ b/arch/metag/include/asm/prom.h
@@ -19,6 +19,5 @@
 
 extern struct machine_desc *setup_machine_fdt(void *dt);
 extern void copy_fdt(void);
-extern void metag_dt_memblock_reserve(void);
 
 #endif /* __ASM_METAG_PROM_H */
-- 
1.8.1.2

Re: [PATCH 1/2] metag: copy devicetree to non-init memory

[Vineet Gupta]

Hi James,

On Wednesday 20 February 2013 08:22 PM, James Hogan wrote:
> Make a copy of the device tree blob in non-init memory. It is required
> when using built-in device tree files that the platform code copies the
> blob to non-init memory prior to calling unflatten_device_tree(),
> otherwise the strings that the device tree refer to will get poisoned
> and potentially reused, breaking later reading of the device tree
> post-init (such as compatible matching in modules, debugfs, and the
> procfs interface).

While the patch conceptually looks correct, I'm not sure why any user of DT -
post-init would refer to DT bindings using of_fdt_* API which use the flat tree,
instead of the binary tree (more efficient in space/usage). Is this to support
some in-transition drivers and other code.

And if this is a general problem, then it is probably better done in DT core.

I'm sure I'm missing something here.

> Signed-off-by: James Hogan <james.hogan@imgtec.com>
> ---
>  arch/metag/include/asm/prom.h |  1 +
>  arch/metag/kernel/devtree.c   | 17 +++++++++++++++++
>  arch/metag/kernel/setup.c     |  2 ++
>  3 files changed, 20 insertions(+)
>
> diff --git a/arch/metag/include/asm/prom.h b/arch/metag/include/asm/prom.h
> index d881396..ccac97e 100644
> --- a/arch/metag/include/asm/prom.h
> +++ b/arch/metag/include/asm/prom.h
> @@ -18,6 +18,7 @@
>  #define HAVE_ARCH_DEVTREE_FIXUPS
>  
>  extern struct machine_desc *setup_machine_fdt(void *dt);
> +extern void copy_fdt(void);
>  extern void metag_dt_memblock_reserve(void);
>  
>  #endif /* __ASM_METAG_PROM_H */
> diff --git a/arch/metag/kernel/devtree.c b/arch/metag/kernel/devtree.c
> index 5b6b1d85..7cd0252 100644
> --- a/arch/metag/kernel/devtree.c
> +++ b/arch/metag/kernel/devtree.c
> @@ -95,3 +95,20 @@ struct machine_desc * __init setup_machine_fdt(void *dt)
>  
>  	return mdesc_best;
>  }
> +
> +/**
> + * copy_fdt - Copy device tree into non-init memory.
> + *
> + * We must copy the flattened device tree blob into non-init memory because the
> + * unflattened device tree will reference the strings in it directly.
> + */
> +void __init copy_fdt(void)
> +{
> +	void *alloc = early_init_dt_alloc_memory_arch(
> +			be32_to_cpu(initial_boot_params->totalsize), 0x40);
> +	if (alloc) {
> +		memcpy(alloc, initial_boot_params,
> +		       be32_to_cpu(initial_boot_params->totalsize));
> +		initial_boot_params = alloc;
> +	}
> +}
> diff --git a/arch/metag/kernel/setup.c b/arch/metag/kernel/setup.c
> index dd6c5ad..8792461 100644
> --- a/arch/metag/kernel/setup.c
> +++ b/arch/metag/kernel/setup.c
> @@ -406,6 +406,8 @@ void __init setup_arch(char **cmdline_p)
>  	cpu_2_hwthread_id[smp_processor_id()] = hard_processor_id();
>  	hwthread_id_2_cpu[hard_processor_id()] = smp_processor_id();
>  
> +	/* Copy device tree blob into non-init memory before unflattening */
> +	copy_fdt();
>  	unflatten_device_tree();
>  
>  #ifdef CONFIG_SMP

Re: [PATCH 1/2] metag: copy devicetree to non-init memory

[James Hogan]

Hi Vineet,

On 21/02/13 09:08, Vineet Gupta wrote:
> On Wednesday 20 February 2013 08:22 PM, James Hogan wrote:
>> Make a copy of the device tree blob in non-init memory. It is required
>> when using built-in device tree files that the platform code copies the
>> blob to non-init memory prior to calling unflatten_device_tree(),
>> otherwise the strings that the device tree refer to will get poisoned
>> and potentially reused, breaking later reading of the device tree
>> post-init (such as compatible matching in modules, debugfs, and the
>> procfs interface).
> 
> While the patch conceptually looks correct, I'm not sure why any user of DT -
> post-init would refer to DT bindings using of_fdt_* API which use the flat tree,
> instead of the binary tree (more efficient in space/usage). Is this to support
> some in-transition drivers and other code.

The strings aren't copied when the devicetree is unflattened, so the
unflattened version still points into initdata, so all the strings "in"
the unflattened version are wiped when it's freed too.

Documentation/kbuild/makefiles.txt has this to say:
>     dtc
> 	Create flattend device tree blob object suitable for linking
> 	into vmlinux. Device tree blobs linked into vmlinux are placed
> 	in an init section in the image. Platform code *must* copy the
> 	blob to non-init memory prior to calling unflatten_device_tree().

Other architectures using the builtin dtb also do the copy. I presume
it's in initdata in the first place to avoid keeping the built-in one
around if one is provided by the bootloader instead.

Cheers
James

Re: [PATCH 1/2] metag: copy devicetree to non-init memory

[Vineet Gupta]

On Thursday 21 February 2013 03:04 PM, James Hogan wrote:
> Hi Vineet,
>
> On 21/02/13 09:08, Vineet Gupta wrote:
>> On Wednesday 20 February 2013 08:22 PM, James Hogan wrote:
>>> Make a copy of the device tree blob in non-init memory. It is required
>>> when using built-in device tree files that the platform code copies the
>>> blob to non-init memory prior to calling unflatten_device_tree(),
>>> otherwise the strings that the device tree refer to will get poisoned
>>> and potentially reused, breaking later reading of the device tree
>>> post-init (such as compatible matching in modules, debugfs, and the
>>> procfs interface).
>> While the patch conceptually looks correct, I'm not sure why any user of DT -
>> post-init would refer to DT bindings using of_fdt_* API which use the flat tree,
>> instead of the binary tree (more efficient in space/usage). Is this to support
>> some in-transition drivers and other code.
> The strings aren't copied when the devicetree is unflattened, so the
> unflattened version still points into initdata, so all the strings "in"
> the unflattened version are wiped when it's freed too.

You are absolutely right, so ARC port is infested with same ticking time bomb !
Thanks for the heads up - I'll queue up a fix - but that will go in after the
first pull request as I want any change, however minimal/local to cook in -next
for some time (for paranoid sake at this stage)

> Documentation/kbuild/makefiles.txt has this to say:
>>     dtc
>> 	Create flattend device tree blob object suitable for linking
>> 	into vmlinux. Device tree blobs linked into vmlinux are placed
>> 	in an init section in the image. Platform code *must* copy the
>> 	blob to non-init memory prior to calling unflatten_device_tree().
> Other architectures using the builtin dtb also do the copy. I presume
> it's in initdata in the first place to avoid keeping the built-in one
> around if one is provided by the bootloader instead.

Makes sense !

-Vineet

Re: [PATCH 1/2] metag: copy devicetree to non-init memory

[Vineet Gupta]

On Wednesday 20 February 2013 08:22 PM, James Hogan wrote:
> Make a copy of the device tree blob in non-init memory. It is required
> when using built-in device tree files that the platform code copies the
> blob to non-init memory prior to calling unflatten_device_tree(),
> otherwise the strings that the device tree refer to will get poisoned
> and potentially reused, breaking later reading of the device tree
> post-init (such as compatible matching in modules, debugfs, and the
> procfs interface).
>
> Signed-off-by: James Hogan <james.hogan@imgtec.com>

In case you need it - Reviewed-by: Vineet Gupta <vgupta@synopsys.com>

Re: [PATCH 1/2] metag: copy devicetree to non-init memory

[James Hogan]

On 21/02/13 11:19, Vineet Gupta wrote:
> On Wednesday 20 February 2013 08:22 PM, James Hogan wrote:
>> Make a copy of the device tree blob in non-init memory. It is required
>> when using built-in device tree files that the platform code copies the
>> blob to non-init memory prior to calling unflatten_device_tree(),
>> otherwise the strings that the device tree refer to will get poisoned
>> and potentially reused, breaking later reading of the device tree
>> post-init (such as compatible matching in modules, debugfs, and the
>> procfs interface).
>>
>> Signed-off-by: James Hogan <james.hogan@imgtec.com>
> 
> In case you need it - Reviewed-by: Vineet Gupta <vgupta@synopsys.com>

Thanks Vineet,

Cheers
James