[PATCH v3 0/2] of: populate of_root_node if not set (alternate)

[PATCH v3 0/2] of: populate of_root_node if not set (alternate)

[Frank Rowand]

This series is a different implementation to achieve the goals of
https://lore.kernel.org/r/20220623105044.152832-1-clement.leger@bootlin.com

In order to apply overlays or create new nodes under the root node, the
kernel expects of_root to be set. On some system where a device-tree was
not provided by firmware (x86 for instance) if CONFIG_OF is enabled,
then we will end up with a null of_root. This series adds support to
create this root node using a builtin dtb and removes the manual
creation of the root node done in unittests.c.

Changes since version 2: (patch 1/2)
  - change of __dtb_empty_root_* from "void *" to "uint8_t []"

Changes since version 1: (patch 1/2)
  - refresh for 6.2-rc1
  - update Signed-off-by
  - fix typo in of_fdt.h: s/of_setup/setup_of
  - unflatten_device_tree(): validate size in header field dtb_empty_root
    that will be used to copy dtb_empty_root
  - add Kconfig option to manually select CONFIG_OF_EARLY_FLATTREE

Changes since version 1: (patch 2/2)
  - refresh for 6.2-rc1
  - update Signed-off-by
  - fix formatting error (leading space) in patch comment

Frank Rowand (2):
  of: create of_root if no dtb provided
  of: unittest: treat missing of_root as error instead of fixing up

 drivers/of/Kconfig        |  7 ++++++-
 drivers/of/Makefile       |  2 +-
 drivers/of/empty_root.dts |  6 ++++++
 drivers/of/fdt.c          | 27 ++++++++++++++++++++++++++-
 drivers/of/unittest.c     | 16 ++++++----------
 include/linux/of_fdt.h    |  2 ++
 init/main.c               |  2 ++
 7 files changed, 49 insertions(+), 13 deletions(-)
 create mode 100644 drivers/of/empty_root.dts

-- 
Frank Rowand <frowand.list@gmail.com>

[PATCH v3 1/2] of: create of_root if no dtb provided

[Frank Rowand]

When enabling CONFIG_OF on a platform where of_root is not populated by
firmware, we end up without a root node. In order to apply overlays and
create subnodes of the root node, we need one. Create this root node
by unflattening an empty builtin dtb.

If firmware provides a flattened device tree (FDT) then the FDT is
unflattened via setup_arch().  Otherwise, setup_of() which is called
immediately after setup_arch(), and will create the default root node
if it does not exist.

Signed-off-by: Frank Rowand <frowand.list@gmail.com>
---

Please wait for test results from Clement before accepting.

Changes since version 2:
  - change of __dtb_empty_root_* from "void *" to "uint8_t []"

Changes since version 1:
  - refresh for 6.2-rc1
  - update Signed-off-by
  - fix typo in of_fdt.h: s/of_setup/setup_of
  - unflatten_device_tree(): validate size in header field dtb_empty_root
    that will be used to copy dtb_empty_root
  - add Kconfig option to manually select CONFIG_OF_EARLY_FLATTREE

 drivers/of/Kconfig        |  7 ++++++-
 drivers/of/Makefile       |  2 +-
 drivers/of/empty_root.dts |  6 ++++++
 drivers/of/fdt.c          | 27 ++++++++++++++++++++++++++-
 include/linux/of_fdt.h    |  2 ++
 init/main.c               |  2 ++
 6 files changed, 43 insertions(+), 3 deletions(-)
 create mode 100644 drivers/of/empty_root.dts

diff --git a/drivers/of/Kconfig b/drivers/of/Kconfig
index 80b5fd44ab1c..591cfe386727 100644
--- a/drivers/of/Kconfig
+++ b/drivers/of/Kconfig
@@ -42,9 +42,14 @@ config OF_FLATTREE
 	select CRC32
 
 config OF_EARLY_FLATTREE
-	bool
+	bool "Functions for accessing Flat Devicetree (FDT) early in boot"
 	select DMA_DECLARE_COHERENT if HAS_DMA
 	select OF_FLATTREE
+	help
+	  Normally selected by platforms that process an FDT that has been
+	  passed to the kernel by the bootloader.  If the bootloader does not
+	  pass an FDT to the kernel and you need an empty devicetree that
+	  contains only a root node to exist, then say Y here.
 
 config OF_PROMTREE
 	bool
diff --git a/drivers/of/Makefile b/drivers/of/Makefile
index e0360a44306e..cbae92c5ed02 100644
--- a/drivers/of/Makefile
+++ b/drivers/of/Makefile
@@ -2,7 +2,7 @@
 obj-y = base.o device.o platform.o property.o
 obj-$(CONFIG_OF_KOBJ) += kobj.o
 obj-$(CONFIG_OF_DYNAMIC) += dynamic.o
-obj-$(CONFIG_OF_FLATTREE) += fdt.o
+obj-$(CONFIG_OF_FLATTREE) += fdt.o empty_root.dtb.o
 obj-$(CONFIG_OF_EARLY_FLATTREE) += fdt_address.o
 obj-$(CONFIG_OF_PROMTREE) += pdt.o
 obj-$(CONFIG_OF_ADDRESS)  += address.o
diff --git a/drivers/of/empty_root.dts b/drivers/of/empty_root.dts
new file mode 100644
index 000000000000..cf9e97a60f48
--- /dev/null
+++ b/drivers/of/empty_root.dts
@@ -0,0 +1,6 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/dts-v1/;
+
+/ {
+
+};
diff --git a/drivers/of/fdt.c b/drivers/of/fdt.c
index b2272bccf85c..eccc2d2107af 100644
--- a/drivers/of/fdt.c
+++ b/drivers/of/fdt.c
@@ -33,6 +33,13 @@
 
 #include "of_private.h"
 
+/*
+ * __dtb_empty_root_begin[] and __dtb_empty_root_end[] magically created by
+ * cmd_dt_S_dtb in scripts/Makefile.lib
+ */
+extern uint8_t __dtb_empty_root_begin[];
+extern uint8_t __dtb_empty_root_end[];
+
 /*
  * of_fdt_limit_memory - limit the number of regions in the /memory node
  * @limit: maximum entries
@@ -1326,8 +1333,19 @@ bool __init early_init_dt_scan(void *params)
  */
 void __init unflatten_device_tree(void)
 {
-	__unflatten_device_tree(initial_boot_params, NULL, &of_root,
+	if (!initial_boot_params) {
+		initial_boot_params = (void *) __dtb_empty_root_begin;
+		/* fdt_totalsize() will be used for copy size */
+		if (fdt_totalsize(initial_boot_params) >
+		    __dtb_empty_root_end - __dtb_empty_root_begin) {
+			pr_err("invalid size in dtb_empty_root\n");
+			return;
+		}
+		unflatten_and_copy_device_tree();
+	} else {
+		__unflatten_device_tree(initial_boot_params, NULL, &of_root,
 				early_init_dt_alloc_memory_arch, false);
+	}
 
 	/* Get pointer to "/chosen" and "/aliases" nodes for use everywhere */
 	of_alias_scan(early_init_dt_alloc_memory_arch);
@@ -1367,6 +1385,13 @@ void __init unflatten_and_copy_device_tree(void)
 	unflatten_device_tree();
 }
 
+void __init setup_of(void)
+{
+	/* if architecture did not unflatten devicetree, do it now */
+	if (!of_root)
+		unflatten_device_tree();
+}
+
 #ifdef CONFIG_SYSFS
 static ssize_t of_fdt_raw_read(struct file *filp, struct kobject *kobj,
 			       struct bin_attribute *bin_attr,
diff --git a/include/linux/of_fdt.h b/include/linux/of_fdt.h
index d69ad5bb1eb1..f0dc46d576da 100644
--- a/include/linux/of_fdt.h
+++ b/include/linux/of_fdt.h
@@ -81,6 +81,7 @@ extern const void *of_flat_dt_match_machine(const void *default_match,
 /* Other Prototypes */
 extern void unflatten_device_tree(void);
 extern void unflatten_and_copy_device_tree(void);
+extern void setup_of(void);
 extern void early_init_devtree(void *);
 extern void early_get_first_memblock_info(void *, phys_addr_t *);
 #else /* CONFIG_OF_EARLY_FLATTREE */
@@ -91,6 +92,7 @@ static inline void early_init_fdt_reserve_self(void) {}
 static inline const char *of_flat_dt_get_machine_name(void) { return NULL; }
 static inline void unflatten_device_tree(void) {}
 static inline void unflatten_and_copy_device_tree(void) {}
+static inline void setup_of(void) {}
 #endif /* CONFIG_OF_EARLY_FLATTREE */
 
 #endif /* __ASSEMBLY__ */
diff --git a/init/main.c b/init/main.c
index e1c3911d7c70..31e0931b5134 100644
--- a/init/main.c
+++ b/init/main.c
@@ -101,6 +101,7 @@
 #include <linux/init_syscalls.h>
 #include <linux/stackdepot.h>
 #include <linux/randomize_kstack.h>
+#include <linux/of_fdt.h>
 #include <net/net_namespace.h>
 
 #include <asm/io.h>
@@ -961,6 +962,7 @@ asmlinkage __visible void __init __no_sanitize_address start_kernel(void)
 	pr_notice("%s", linux_banner);
 	early_security_init();
 	setup_arch(&command_line);
+	setup_of();
 	setup_boot_config();
 	setup_command_line(command_line);
 	setup_nr_cpu_ids();
-- 
Frank Rowand <frowand.list@gmail.com>

Re: [PATCH v3 1/2] of: create of_root if no dtb provided

[Rob Herring]

On Thu, Feb 23, 2023 at 3:34 PM Frank Rowand <frowand.list@gmail.com> wrote:
>
> When enabling CONFIG_OF on a platform where of_root is not populated by
> firmware, we end up without a root node. In order to apply overlays and
> create subnodes of the root node, we need one. Create this root node
> by unflattening an empty builtin dtb.
>
> If firmware provides a flattened device tree (FDT) then the FDT is
> unflattened via setup_arch().  Otherwise, setup_of() which is called
> immediately after setup_arch(), and will create the default root node
> if it does not exist.

Why do we need a hook after setup_arch() rather than an initcall?

Rob

Re: [PATCH v3 1/2] of: create of_root if no dtb provided

[Frank Rowand]

On 2/27/23 11:17, Rob Herring wrote:
> On Thu, Feb 23, 2023 at 3:34 PM Frank Rowand <frowand.list@gmail.com> wrote:
>>
>> When enabling CONFIG_OF on a platform where of_root is not populated by
>> firmware, we end up without a root node. In order to apply overlays and
>> create subnodes of the root node, we need one. Create this root node
>> by unflattening an empty builtin dtb.
>>
>> If firmware provides a flattened device tree (FDT) then the FDT is
>> unflattened via setup_arch().  Otherwise, setup_of() which is called
>> immediately after setup_arch(), and will create the default root node
>> if it does not exist.
> 
> Why do we need a hook after setup_arch() rather than an initcall?
> 
> Rob

It might work as an initcall today.  Maybe not in the future as other
initcalls are added.

But my main stream of thinking is that before the patch "we know" that
the device tree data structure exists when setup_arch() returns.
Adding setup_of() immediately after setup_arch() retains that
guarantee, but one line later in start_kernel().

I could have instead put the call to setup_of() into each architectures'
setup_arch(), but that would just be duplicating the same code for each
architecture, which did not seem like a good choice.

-Frank

Re: [PATCH v3 1/2] of: create of_root if no dtb provided

[Rob Herring]

On Tue, Feb 28, 2023 at 08:05:58PM -0600, Frank Rowand wrote:
> On 2/27/23 11:17, Rob Herring wrote:
> > On Thu, Feb 23, 2023 at 3:34 PM Frank Rowand <frowand.list@gmail.com> wrote:
> >>
> >> When enabling CONFIG_OF on a platform where of_root is not populated by
> >> firmware, we end up without a root node. In order to apply overlays and
> >> create subnodes of the root node, we need one. Create this root node
> >> by unflattening an empty builtin dtb.
> >>
> >> If firmware provides a flattened device tree (FDT) then the FDT is
> >> unflattened via setup_arch().  Otherwise, setup_of() which is called
> >> immediately after setup_arch(), and will create the default root node
> >> if it does not exist.
> > 
> > Why do we need a hook after setup_arch() rather than an initcall?
> > 
> > Rob
> 
> It might work as an initcall today.  Maybe not in the future as other
> initcalls are added.

That's an argument for never using initcalls (not a bad one either). But 
we have them and we have little reason not to use them. Also, it's 
better to do things as late as possible I've found. The earlier you do 
things, the more architecture specific stuff you hit. That's a big 
reason for the remaining differences in FDT init across architectures. 
Maybe after setup_arch is late enough. IDK.

> But my main stream of thinking is that before the patch "we know" that
> the device tree data structure exists when setup_arch() returns.
> Adding setup_of() immediately after setup_arch() retains that
> guarantee, but one line later in start_kernel().

I get the logic. I'd just rather not add another hook between the DT 
code and the core/arch code. Especially for this niche usecase.

We already have the secondary init when sysfs is up. Can't we just do 
this there?

> I could have instead put the call to setup_of() into each architectures'
> setup_arch(), but that would just be duplicating the same code for each
> architecture, which did not seem like a good choice.

Uhh, no!

Rob

Re: [PATCH v3 1/2] of: create of_root if no dtb provided

[Frank Rowand]

On 2/28/23 21:00, Rob Herring wrote:
> On Tue, Feb 28, 2023 at 08:05:58PM -0600, Frank Rowand wrote:
>> On 2/27/23 11:17, Rob Herring wrote:
>>> On Thu, Feb 23, 2023 at 3:34 PM Frank Rowand <frowand.list@gmail.com> wrote:
>>>>
>>>> When enabling CONFIG_OF on a platform where of_root is not populated by
>>>> firmware, we end up without a root node. In order to apply overlays and
>>>> create subnodes of the root node, we need one. Create this root node
>>>> by unflattening an empty builtin dtb.
>>>>
>>>> If firmware provides a flattened device tree (FDT) then the FDT is
>>>> unflattened via setup_arch().  Otherwise, setup_of() which is called
>>>> immediately after setup_arch(), and will create the default root node
>>>> if it does not exist.
>>>
>>> Why do we need a hook after setup_arch() rather than an initcall?
>>>
>>> Rob
>>
>> It might work as an initcall today.  Maybe not in the future as other
>> initcalls are added.
> 
> That's an argument for never using initcalls (not a bad one either). But 
> we have them and we have little reason not to use them. Also, it's 
> better to do things as late as possible I've found. The earlier you do 
> things, the more architecture specific stuff you hit. That's a big 
> reason for the remaining differences in FDT init across architectures. 
> Maybe after setup_arch is late enough. IDK.
> 
>> But my main stream of thinking is that before the patch "we know" that
>> the device tree data structure exists when setup_arch() returns.
>> Adding setup_of() immediately after setup_arch() retains that
>> guarantee, but one line later in start_kernel().
> 
> I get the logic. I'd just rather not add another hook between the DT 
> code and the core/arch code. Especially for this niche usecase.
> 
> We already have the secondary init when sysfs is up. Can't we just do 
> this there?

In general, I agree with your sentiments about an initcall being a preferred
solution.

But when I was looking at the suggested alternatives, I noticed one sticking
point.  The new setup_of() calls unflatten_device_tree(), which calls
unittest_unflatten_overlay_base().  The call to unittest_unflatten_overlay_base()
is deliberately very early in the boot, so that the memory allocator used
for this very small portion of the devicetree nodes created for unittest
is the same early boot allocator that is used to unflatten an FDT passed
to the kernel from a bootloader.

Digging through this led me to another issue.  I have not tested this patch
series on a user mode linux kernel (on my todo list...).  For user mode linux,
unittest_data_add() is called directly from the late initcall of_unittest().
So for user mode linux, unittest_data_add() will be called a second time - I
need to remove that second call and make sure unittest still works on user
mode linux.

> 
>> I could have instead put the call to setup_of() into each architectures'
>> setup_arch(), but that would just be duplicating the same code for each
>> architecture, which did not seem like a good choice.
> 

> Uhh, no!

Agreed, I guess I was too subtle with "did not seem like a good choice". :-)

> 
> Rob

[PATCH v3 2/2] of: unittest: treat missing of_root as error instead of fixing up

[Frank Rowand]

setup_of() now ensures that of_root node is populated with the
root of a default devicetree. Remove the unittest code that
created of_root if it was missing.  Verify that of_root is
valid before attempting to attach the testcase-data subtree.

Signed-off-by: Frank Rowand <frowand.list@gmail.com>
---
Changes since version 2:
  - none

Changes since version 1:
  - refresh for 6.2-rc1
  - update Signed-off-by
  - fix formatting error (leading space) in patch comment

 drivers/of/unittest.c | 16 ++++++----------
 1 file changed, 6 insertions(+), 10 deletions(-)

diff --git a/drivers/of/unittest.c b/drivers/of/unittest.c
index bc0f1e50a4be..006713511c53 100644
--- a/drivers/of/unittest.c
+++ b/drivers/of/unittest.c
@@ -1469,20 +1469,16 @@ static int __init unittest_data_add(void)
 		return -EINVAL;
 	}
 
-	if (!of_root) {
-		of_root = unittest_data_node;
-		for_each_of_allnodes(np)
-			__of_attach_node_sysfs(np);
-		of_aliases = of_find_node_by_path("/aliases");
-		of_chosen = of_find_node_by_path("/chosen");
-		of_overlay_mutex_unlock();
-		return 0;
-	}
-
 	EXPECT_BEGIN(KERN_INFO,
 		     "Duplicate name in testcase-data, renamed to \"duplicate-name#1\"");
 
 	/* attach the sub-tree to live tree */
+	if (!of_root) {
+		pr_warn("%s: no live tree to attach sub-tree\n", __func__);
+		kfree(unittest_data);
+		return -ENODEV;
+	}
+
 	np = unittest_data_node->child;
 	while (np) {
 		struct device_node *next = np->sibling;
-- 
Frank Rowand <frowand.list@gmail.com>

Re: [PATCH v3 0/2] of: populate of_root_node if not set (alternate)

[Frank Rowand]

Hi Clément,

Can you please test this version of the patch series?

Thanks!

-Frank

On 2/23/23 15:34, Frank Rowand wrote:
> This series is a different implementation to achieve the goals of
> https://lore.kernel.org/r/20220623105044.152832-1-clement.leger@bootlin.com
> 
> In order to apply overlays or create new nodes under the root node, the
> kernel expects of_root to be set. On some system where a device-tree was
> not provided by firmware (x86 for instance) if CONFIG_OF is enabled,
> then we will end up with a null of_root. This series adds support to
> create this root node using a builtin dtb and removes the manual
> creation of the root node done in unittests.c.
> 
> Changes since version 2: (patch 1/2)
>   - change of __dtb_empty_root_* from "void *" to "uint8_t []"
> 
> Changes since version 1: (patch 1/2)
>   - refresh for 6.2-rc1
>   - update Signed-off-by
>   - fix typo in of_fdt.h: s/of_setup/setup_of
>   - unflatten_device_tree(): validate size in header field dtb_empty_root
>     that will be used to copy dtb_empty_root
>   - add Kconfig option to manually select CONFIG_OF_EARLY_FLATTREE
> 
> Changes since version 1: (patch 2/2)
>   - refresh for 6.2-rc1
>   - update Signed-off-by
>   - fix formatting error (leading space) in patch comment
> 
> Frank Rowand (2):
>   of: create of_root if no dtb provided
>   of: unittest: treat missing of_root as error instead of fixing up
> 
>  drivers/of/Kconfig        |  7 ++++++-
>  drivers/of/Makefile       |  2 +-
>  drivers/of/empty_root.dts |  6 ++++++
>  drivers/of/fdt.c          | 27 ++++++++++++++++++++++++++-
>  drivers/of/unittest.c     | 16 ++++++----------
>  include/linux/of_fdt.h    |  2 ++
>  init/main.c               |  2 ++
>  7 files changed, 49 insertions(+), 13 deletions(-)
>  create mode 100644 drivers/of/empty_root.dts
> 

Re: [PATCH v3 0/2] of: populate of_root_node if not set (alternate)

[Clément Léger]

Le Thu, 23 Feb 2023 15:39:31 -0600,
Frank Rowand <frowand.list@gmail.com> a écrit :

> Hi Clément,
> 
> Can you please test this version of the patch series?

Hi Frank,

I can confirm it now works as expected, thanks for your patch !

Tested-by: Clément Léger <clement.leger@bootlin.com>

Clément

> 
> Thanks!
> 
> -Frank
> 
> On 2/23/23 15:34, Frank Rowand wrote:
> > This series is a different implementation to achieve the goals of
> > https://lore.kernel.org/r/20220623105044.152832-1-clement.leger@bootlin.com
> > 
> > In order to apply overlays or create new nodes under the root node, the
> > kernel expects of_root to be set. On some system where a device-tree was
> > not provided by firmware (x86 for instance) if CONFIG_OF is enabled,
> > then we will end up with a null of_root. This series adds support to
> > create this root node using a builtin dtb and removes the manual
> > creation of the root node done in unittests.c.
> > 
> > Changes since version 2: (patch 1/2)
> >   - change of __dtb_empty_root_* from "void *" to "uint8_t []"
> > 
> > Changes since version 1: (patch 1/2)
> >   - refresh for 6.2-rc1
> >   - update Signed-off-by
> >   - fix typo in of_fdt.h: s/of_setup/setup_of
> >   - unflatten_device_tree(): validate size in header field dtb_empty_root
> >     that will be used to copy dtb_empty_root
> >   - add Kconfig option to manually select CONFIG_OF_EARLY_FLATTREE
> > 
> > Changes since version 1: (patch 2/2)
> >   - refresh for 6.2-rc1
> >   - update Signed-off-by
> >   - fix formatting error (leading space) in patch comment
> > 
> > Frank Rowand (2):
> >   of: create of_root if no dtb provided
> >   of: unittest: treat missing of_root as error instead of fixing up
> > 
> >  drivers/of/Kconfig        |  7 ++++++-
> >  drivers/of/Makefile       |  2 +-
> >  drivers/of/empty_root.dts |  6 ++++++
> >  drivers/of/fdt.c          | 27 ++++++++++++++++++++++++++-
> >  drivers/of/unittest.c     | 16 ++++++----------
> >  include/linux/of_fdt.h    |  2 ++
> >  init/main.c               |  2 ++
> >  7 files changed, 49 insertions(+), 13 deletions(-)
> >  create mode 100644 drivers/of/empty_root.dts
> >   
> 



-- 
Clément Léger,
Embedded Linux and Kernel engineer at Bootlin
https://bootlin.com