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

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

[frowand.list]

From: Frank Rowand <frank.rowand@sony.com>

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 add support to
create this root node using a builtin dtb and remove the manual
creation of the root node done in unittests.c.

This series modifies init/main.c.  It was not clear to me whether
to add Linus to the distribution, or if the change should flow
through Rob.  (I did not add Linus.)

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/Makefile    |  2 +-
 drivers/of/fdt.c       | 19 ++++++++++++++++++-
 drivers/of/unittest.c  | 16 ++++++----------
 include/linux/of_fdt.h |  2 ++
 init/main.c            |  2 ++
 5 files changed, 29 insertions(+), 12 deletions(-)

-- 
Frank Rowand <frank.rowand@sony.com>

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

[frowand.list]

From: Frank Rowand <frank.rowand@sony.com>

 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().  setup_of() is called immediately
 after setup_arch(), and will create the default root node if it
 does not exist.

Signed-off-by: Frank Rowand <frank.rowand@sony.com>
---

This patch modifies init/main.c.  It was not clear to me whether
to add Linus to the distribution, or if the change should flow
through Rob.  (I did not add Linus.)

 drivers/of/Makefile    |  2 +-
 drivers/of/fdt.c       | 19 ++++++++++++++++++-
 include/linux/of_fdt.h |  2 ++
 init/main.c            |  2 ++
 4 files changed, 23 insertions(+), 2 deletions(-)

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/fdt.c b/drivers/of/fdt.c
index a8f5b6532165..d439ded3b6c6 100644
--- a/drivers/of/fdt.c
+++ b/drivers/of/fdt.c
@@ -33,6 +33,12 @@
 
 #include "of_private.h"
 
+/*
+ * __dtb_empty_root_begin[] magically created by cmd_dt_S_dtb in
+ * scripts/Makefile.lib
+ */
+extern void *__dtb_empty_root_begin;
+
 /*
  * of_fdt_limit_memory - limit the number of regions in the /memory node
  * @limit: maximum entries
@@ -1332,8 +1338,13 @@ 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;
+		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);
@@ -1373,6 +1384,12 @@ void __init unflatten_and_copy_device_tree(void)
 	unflatten_device_tree();
 }
 
+void __init setup_of(void)
+{
+	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..4566876db351 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 of_setup(void) {}
 #endif /* CONFIG_OF_EARLY_FLATTREE */
 
 #endif /* __ASSEMBLY__ */
diff --git a/init/main.c b/init/main.c
index 0ee39cdcfcac..8b3a60d14bea 100644
--- a/init/main.c
+++ b/init/main.c
@@ -99,6 +99,7 @@
 #include <linux/kcsan.h>
 #include <linux/init_syscalls.h>
 #include <linux/stackdepot.h>
+#include <linux/of_fdt.h>
 #include <net/net_namespace.h>
 
 #include <asm/io.h>
@@ -949,6 +950,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 <frank.rowand@sony.com>

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

[Clément Léger]

Le Thu, 23 Jun 2022 22:43:26 -0500,
frowand.list@gmail.com a écrit :

>  
> +/*
> + * __dtb_empty_root_begin[] magically created by cmd_dt_S_dtb in
> + * scripts/Makefile.lib
> + */
> +extern void *__dtb_empty_root_begin;
> +
>  /*
>   * of_fdt_limit_memory - limit the number of regions in the /memory node
>   * @limit: maximum entries
> @@ -1332,8 +1338,13 @@ bool __init early_init_dt_scan(void *params)
>   */
>  void __init unflatten_device_tree(void)
>  {

Hi Frank,

This function is only defined when CONFIG_OF_EARLY_FLATTREE is enabled.
Which means that on platforms that do not select this, the default
empty device-tree creation will not be done.

This configuration option is selected by the platform and not by the
user. On x86, only one config enables this (X86_INTEL_CE) which means
this won't work on all the other platforms even if CONFIG_OF is
selected. I would need this to work by only selected CONFIG_OF.
That's why I decided to add the of_root creation in of_core_init()
using a function (of_fdt_unflatten()) that is provided if CONFIG_OF is
defined.

> -	__unflatten_device_tree(initial_boot_params, NULL, &of_root,
> +	if (!initial_boot_params) {
> +		initial_boot_params = (void *) __dtb_empty_root_begin;
> +		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);
> @@ -1373,6 +1384,12 @@ void __init unflatten_and_copy_device_tree(void)
>  	unflatten_device_tree();
>  }
>  
> +void __init setup_of(void)
> +{
> +	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..4566876db351 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 of_setup(void) {}

Shouldn't this be setup_of(void) ?

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

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

[Frank Rowand]

On 6/24/22 08:13, Clément Léger wrote:
> Le Thu, 23 Jun 2022 22:43:26 -0500,
> frowand.list@gmail.com a écrit :
> 
>>  
>> +/*
>> + * __dtb_empty_root_begin[] magically created by cmd_dt_S_dtb in
>> + * scripts/Makefile.lib
>> + */
>> +extern void *__dtb_empty_root_begin;
>> +
>>  /*
>>   * of_fdt_limit_memory - limit the number of regions in the /memory node
>>   * @limit: maximum entries
>> @@ -1332,8 +1338,13 @@ bool __init early_init_dt_scan(void *params)
>>   */
>>  void __init unflatten_device_tree(void)
>>  {
> 
> Hi Frank,
> 
> This function is only defined when CONFIG_OF_EARLY_FLATTREE is enabled.

More precisely, only if CONFIG_OF_FLATTREE is enabled.  But that would
most likely be seleved by CONFIG_OF_EARLY_FLATTREE, so in practice the
issue you raise is valid.

> Which means that on platforms that do not select this, the default
> empty device-tree creation will not be done.

Yes, so platforms that need this functionality need to select this
option.

> 
> This configuration option is selected by the platform and not by the
> user. On x86, only one config enables this (X86_INTEL_CE) which means
> this won't work on all the other platforms even if CONFIG_OF is
> selected. I would need this to work by only selected CONFIG_OF.

Maybe this means that CONFIG_OF should be changed to select
CONFIG_OF_FLATTREE.  Any opinions on this Rob?

> That's why I decided to add the of_root creation in of_core_init()
> using a function (of_fdt_unflatten()) that is provided if CONFIG_OF is
> defined.

I mentioned this in response to the previous patch series, but will
repeat here for those who might not have read that email thread.

I do not want the root live tree to be created buy different code in
different places; I want one central place where this occurs.  When
the tree can be created in multiple places by different code blocks,
it becomes more difficult to understand the code and more likely that
one of the tree creation code blocks is not updated when another is.

> 
>> -	__unflatten_device_tree(initial_boot_params, NULL, &of_root,
>> +	if (!initial_boot_params) {
>> +		initial_boot_params = (void *) __dtb_empty_root_begin;
>> +		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);
>> @@ -1373,6 +1384,12 @@ void __init unflatten_and_copy_device_tree(void)
>>  	unflatten_device_tree();
>>  }
>>  
>> +void __init setup_of(void)
>> +{
>> +	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..4566876db351 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 of_setup(void) {}
> 

> Shouldn't this be setup_of(void) ?

Yes, thanks!  Will fix.

One other thing I need to do is test this patch on a user mode linux
kernel.

-Frank

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

[Clément Léger]

Le Fri, 24 Jun 2022 11:44:07 -0500,
Frank Rowand <frowand.list@gmail.com> a écrit :

> On 6/24/22 08:13, Clément Léger wrote:
> > Le Thu, 23 Jun 2022 22:43:26 -0500,
> > frowand.list@gmail.com a écrit :
> >   
> >>  
> >> +/*
> >> + * __dtb_empty_root_begin[] magically created by cmd_dt_S_dtb in
> >> + * scripts/Makefile.lib
> >> + */
> >> +extern void *__dtb_empty_root_begin;
> >> +
> >>  /*
> >>   * of_fdt_limit_memory - limit the number of regions in the /memory node
> >>   * @limit: maximum entries
> >> @@ -1332,8 +1338,13 @@ bool __init early_init_dt_scan(void *params)
> >>   */
> >>  void __init unflatten_device_tree(void)
> >>  {  
> > 
> > Hi Frank,
> > 
> > This function is only defined when CONFIG_OF_EARLY_FLATTREE is enabled.  
> 
> More precisely, only if CONFIG_OF_FLATTREE is enabled.  But that would
> most likely be seleved by CONFIG_OF_EARLY_FLATTREE, so in practice the
> issue you raise is valid.
> 
> > Which means that on platforms that do not select this, the default
> > empty device-tree creation will not be done.  
> 
> Yes, so platforms that need this functionality need to select this
> option.

Yes, but this seems a bit odd because this is not really a early
flattree that is provided by the firmware. This simply allows to have a
working support for overlays (As a "side effect" I agree).

> 
> > 
> > This configuration option is selected by the platform and not by the
> > user. On x86, only one config enables this (X86_INTEL_CE) which means
> > this won't work on all the other platforms even if CONFIG_OF is
> > selected. I would need this to work by only selected CONFIG_OF.  
> 
> Maybe this means that CONFIG_OF should be changed to select
> CONFIG_OF_FLATTREE.  Any opinions on this Rob?
> 
> > That's why I decided to add the of_root creation in of_core_init()
> > using a function (of_fdt_unflatten()) that is provided if CONFIG_OF is
> > defined.  
> 
> I mentioned this in response to the previous patch series, but will
> repeat here for those who might not have read that email thread.
> 
> I do not want the root live tree to be created buy different code in
> different places; I want one central place where this occurs.  When
> the tree can be created in multiple places by different code blocks,
> it becomes more difficult to understand the code and more likely that
> one of the tree creation code blocks is not updated when another is.

Understood, my point was more about the fact that I did not wanted to
select CONFIG_OF_FLATTREE to be able to use that support which does not
seems entirely tied to having a "early flattree".

Thanks,

Clément

> 
> >   
> >> -	__unflatten_device_tree(initial_boot_params, NULL, &of_root,
> >> +	if (!initial_boot_params) {
> >> +		initial_boot_params = (void *) __dtb_empty_root_begin;
> >> +		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);
> >> @@ -1373,6 +1384,12 @@ void __init unflatten_and_copy_device_tree(void)
> >>  	unflatten_device_tree();
> >>  }
> >>  
> >> +void __init setup_of(void)
> >> +{
> >> +	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..4566876db351 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 of_setup(void) {}  
> >   
> 
> > Shouldn't this be setup_of(void) ?  
> 
> Yes, thanks!  Will fix.
> 
> One other thing I need to do is test this patch on a user mode linux
> kernel.
> 
> -Frank
> 



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

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

[Rob Herring]

On Fri, Jun 24, 2022 at 11:44:07AM -0500, Frank Rowand wrote:
> On 6/24/22 08:13, Clément Léger wrote:
> > Le Thu, 23 Jun 2022 22:43:26 -0500,
> > frowand.list@gmail.com a écrit :
> > 
> >>  
> >> +/*
> >> + * __dtb_empty_root_begin[] magically created by cmd_dt_S_dtb in
> >> + * scripts/Makefile.lib
> >> + */
> >> +extern void *__dtb_empty_root_begin;
> >> +
> >>  /*
> >>   * of_fdt_limit_memory - limit the number of regions in the /memory node
> >>   * @limit: maximum entries
> >> @@ -1332,8 +1338,13 @@ bool __init early_init_dt_scan(void *params)
> >>   */
> >>  void __init unflatten_device_tree(void)
> >>  {
> > 
> > Hi Frank,
> > 
> > This function is only defined when CONFIG_OF_EARLY_FLATTREE is enabled.
> 
> More precisely, only if CONFIG_OF_FLATTREE is enabled.  But that would
> most likely be seleved by CONFIG_OF_EARLY_FLATTREE, so in practice the
> issue you raise is valid.
> 
> > Which means that on platforms that do not select this, the default
> > empty device-tree creation will not be done.
> 
> Yes, so platforms that need this functionality need to select this
> option.
> 
> > 
> > This configuration option is selected by the platform and not by the
> > user. On x86, only one config enables this (X86_INTEL_CE) which means
> > this won't work on all the other platforms even if CONFIG_OF is
> > selected. I would need this to work by only selected CONFIG_OF.
> 
> Maybe this means that CONFIG_OF should be changed to select
> CONFIG_OF_FLATTREE.  Any opinions on this Rob?

I don't think that works in the PDT (Sparc) case.

I think either CONFIG_OF_FLATTREE or CONFIG_OF_EARLY_FLATTREE will need 
to become user selectable.

> 
> > That's why I decided to add the of_root creation in of_core_init()
> > using a function (of_fdt_unflatten()) that is provided if CONFIG_OF is
> > defined.
> 
> I mentioned this in response to the previous patch series, but will
> repeat here for those who might not have read that email thread.
> 
> I do not want the root live tree to be created buy different code in
> different places; I want one central place where this occurs.  When
> the tree can be created in multiple places by different code blocks,
> it becomes more difficult to understand the code and more likely that
> one of the tree creation code blocks is not updated when another is.

+1

Rob

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

[Clément Léger]

Le Fri, 24 Jun 2022 11:44:07 -0500,
Frank Rowand <frowand.list@gmail.com> a écrit :

> On 6/24/22 08:13, Clément Léger wrote:
> > Le Thu, 23 Jun 2022 22:43:26 -0500,
> > frowand.list@gmail.com a écrit :
> >   
> >>  
> >> +/*
> >> + * __dtb_empty_root_begin[] magically created by cmd_dt_S_dtb in
> >> + * scripts/Makefile.lib
> >> + */
> >> +extern void *__dtb_empty_root_begin;
> >> +
> >>  /*
> >>   * of_fdt_limit_memory - limit the number of regions in the /memory node
> >>   * @limit: maximum entries
> >> @@ -1332,8 +1338,13 @@ bool __init early_init_dt_scan(void *params)
> >>   */
> >>  void __init unflatten_device_tree(void)
> >>  {  
> > 


Any news on this series ?

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

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

[Clément Léger]

Le Tue, 11 Oct 2022 09:26:54 +0200,
Clément Léger <clement.leger@bootlin.com> a écrit :

> Le Fri, 24 Jun 2022 11:44:07 -0500,
> Frank Rowand <frowand.list@gmail.com> a écrit :
> 
> > On 6/24/22 08:13, Clément Léger wrote:  
> > > Le Thu, 23 Jun 2022 22:43:26 -0500,
> > > frowand.list@gmail.com a écrit :
> > >     
> > >>  
> > >> +/*
> > >> + * __dtb_empty_root_begin[] magically created by cmd_dt_S_dtb in
> > >> + * scripts/Makefile.lib
> > >> + */
> > >> +extern void *__dtb_empty_root_begin;
> > >> +
> > >>  /*
> > >>   * of_fdt_limit_memory - limit the number of regions in the /memory node
> > >>   * @limit: maximum entries
> > >> @@ -1332,8 +1338,13 @@ bool __init early_init_dt_scan(void *params)
> > >>   */
> > >>  void __init unflatten_device_tree(void)
> > >>  {    
> > >   
> 
> 
> Any news on this series ?
> 

Hi Frank,

Do you plan on resubmitting this series ? If not, could I resubmit it
after fixing problems that were raised in the review ?

Thanks,

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

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

[Frank Rowand]

On 1/9/23 02:40, Clément Léger wrote:
> Le Tue, 11 Oct 2022 09:26:54 +0200,
> Clément Léger <clement.leger@bootlin.com> a écrit :
> 
>> Le Fri, 24 Jun 2022 11:44:07 -0500,
>> Frank Rowand <frowand.list@gmail.com> a écrit :
>>
>>> On 6/24/22 08:13, Clément Léger wrote:  
>>>> Le Thu, 23 Jun 2022 22:43:26 -0500,
>>>> frowand.list@gmail.com a écrit :
>>>>     
>>>>>  
>>>>> +/*
>>>>> + * __dtb_empty_root_begin[] magically created by cmd_dt_S_dtb in
>>>>> + * scripts/Makefile.lib
>>>>> + */
>>>>> +extern void *__dtb_empty_root_begin;
>>>>> +
>>>>>  /*
>>>>>   * of_fdt_limit_memory - limit the number of regions in the /memory node
>>>>>   * @limit: maximum entries
>>>>> @@ -1332,8 +1338,13 @@ bool __init early_init_dt_scan(void *params)
>>>>>   */
>>>>>  void __init unflatten_device_tree(void)
>>>>>  {    
>>>>   
>>
>>
>> Any news on this series ?
>>
> 
> Hi Frank,
> 
> Do you plan on resubmitting this series ? If not, could I resubmit it
> after fixing problems that were raised in the review ?
>> Thanks,
> 

Thanks for the prod.  I'll re-spin it.

If I properly captured all the comments, I'll have to implement
Rob's suggestion:

  "either CONFIG_OF_FLATTREE or CONFIG_OF_EARLY_FLATTREE will need 
  to become user selectable."

-Frank

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

[Clément Léger]

Le Tue, 10 Jan 2023 00:27:16 -0600,
Frank Rowand <frowand.list@gmail.com> a écrit :

> On 1/9/23 02:40, Clément Léger wrote:
> > Le Tue, 11 Oct 2022 09:26:54 +0200,
> > Clément Léger <clement.leger@bootlin.com> a écrit :
> >   
> >> Le Fri, 24 Jun 2022 11:44:07 -0500,
> >> Frank Rowand <frowand.list@gmail.com> a écrit :
> >>  
> >>> On 6/24/22 08:13, Clément Léger wrote:    
> >>>> Le Thu, 23 Jun 2022 22:43:26 -0500,
> >>>> frowand.list@gmail.com a écrit :
> >>>>       
> >>>>>  
> >>>>> +/*
> >>>>> + * __dtb_empty_root_begin[] magically created by cmd_dt_S_dtb in
> >>>>> + * scripts/Makefile.lib
> >>>>> + */
> >>>>> +extern void *__dtb_empty_root_begin;
> >>>>> +
> >>>>>  /*
> >>>>>   * of_fdt_limit_memory - limit the number of regions in the /memory node
> >>>>>   * @limit: maximum entries
> >>>>> @@ -1332,8 +1338,13 @@ bool __init early_init_dt_scan(void *params)
> >>>>>   */
> >>>>>  void __init unflatten_device_tree(void)
> >>>>>  {      
> >>>>     
> >>
> >>
> >> Any news on this series ?
> >>  
> > 
> > Hi Frank,
> > 
> > Do you plan on resubmitting this series ? If not, could I resubmit it
> > after fixing problems that were raised in the review ?  
> >> Thanks,  
> >   
> 
> Thanks for the prod.  I'll re-spin it.

Ok great, thanks Frank.

> 
> If I properly captured all the comments, I'll have to implement
> Rob's suggestion:
> 
>   "either CONFIG_OF_FLATTREE or CONFIG_OF_EARLY_FLATTREE will need 
>   to become user selectable."

> 
> -Frank



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

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

[Frank Rowand]

On 1/10/23 02:12, Clément Léger wrote:
> Le Tue, 10 Jan 2023 00:27:16 -0600,
> Frank Rowand <frowand.list@gmail.com> a écrit :
> 
>> On 1/9/23 02:40, Clément Léger wrote:
>>> Le Tue, 11 Oct 2022 09:26:54 +0200,
>>> Clément Léger <clement.leger@bootlin.com> a écrit :
>>>   
>>>> Le Fri, 24 Jun 2022 11:44:07 -0500,
>>>> Frank Rowand <frowand.list@gmail.com> a écrit :

< snip >

>>>> Any news on this series ?
>>>>  
>>>
>>> Hi Frank,
>>>
>>> Do you plan on resubmitting this series ? If not, could I resubmit it
>>> after fixing problems that were raised in the review ?  
>>>> Thanks,  
>>>   
>>
>> Thanks for the prod.  I'll re-spin it.
> 
> Ok great, thanks Frank.

My apologies, I haven't done this yet and I'm going on vacation for a week or so.
I'll get back to this.

This is one of the three items at the top of my devicetree todo list (along with
Lizhi Hou's "Generate device tree node for pci devices" patch series).

-Frank

>>
>> If I properly captured all the comments, I'll have to implement
>> Rob's suggestion:
>>
>>   "either CONFIG_OF_FLATTREE or CONFIG_OF_EARLY_FLATTREE will need 
>>   to become user selectable."
> 
>>
>> -Frank
> 
> 
> 

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

[Clément Léger]

Le Tue, 24 Jan 2023 08:34:53 -0600,
Frank Rowand <frowand.list@gmail.com> a écrit :

> On 1/10/23 02:12, Clément Léger wrote:
> > Le Tue, 10 Jan 2023 00:27:16 -0600,
> > Frank Rowand <frowand.list@gmail.com> a écrit :
> >   
> >> On 1/9/23 02:40, Clément Léger wrote:  
> >>> Le Tue, 11 Oct 2022 09:26:54 +0200,
> >>> Clément Léger <clement.leger@bootlin.com> a écrit :
> >>>     
> >>>> Le Fri, 24 Jun 2022 11:44:07 -0500,
> >>>> Frank Rowand <frowand.list@gmail.com> a écrit :  
> 
> < snip >
> 
> >>>> Any news on this series ?
> >>>>    
> >>>
> >>> Hi Frank,
> >>>
> >>> Do you plan on resubmitting this series ? If not, could I resubmit it
> >>> after fixing problems that were raised in the review ?    
> >>>> Thanks,    
> >>>     
> >>
> >> Thanks for the prod.  I'll re-spin it.  
> > 
> > Ok great, thanks Frank.  
> 
> My apologies, I haven't done this yet and I'm going on vacation for a week or so.
> I'll get back to this.

Hi Frank, any news on this ? I'm asking again, but if you do not have
time for this, do you mind if I re-spin your series ? This item is
important for us.

Thanks,

Clément

> 
> This is one of the three items at the top of my devicetree todo list (along with
> Lizhi Hou's "Generate device tree node for pci devices" patch series).
> 
> -Frank
> 
> >>
> >> If I properly captured all the comments, I'll have to implement
> >> Rob's suggestion:
> >>
> >>   "either CONFIG_OF_FLATTREE or CONFIG_OF_EARLY_FLATTREE will need 
> >>   to become user selectable."  
> >   
> >>
> >> -Frank  
> > 
> > 
> >   
> 



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

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

[Frank Rowand]

On 2/20/23 05:15, Clément Léger wrote:
> Le Tue, 24 Jan 2023 08:34:53 -0600,
> Frank Rowand <frowand.list@gmail.com> a écrit :
> 
>> On 1/10/23 02:12, Clément Léger wrote:
>>> Le Tue, 10 Jan 2023 00:27:16 -0600,
>>> Frank Rowand <frowand.list@gmail.com> a écrit :
>>>   
>>>> On 1/9/23 02:40, Clément Léger wrote:  
>>>>> Le Tue, 11 Oct 2022 09:26:54 +0200,
>>>>> Clément Léger <clement.leger@bootlin.com> a écrit :
>>>>>     
>>>>>> Le Fri, 24 Jun 2022 11:44:07 -0500,
>>>>>> Frank Rowand <frowand.list@gmail.com> a écrit :  
>>
>> < snip >
>>
>>>>>> Any news on this series ?
>>>>>>    
>>>>>
>>>>> Hi Frank,
>>>>>
>>>>> Do you plan on resubmitting this series ? If not, could I resubmit it
>>>>> after fixing problems that were raised in the review ?    
>>>>>> Thanks,    
>>>>>     
>>>>
>>>> Thanks for the prod.  I'll re-spin it.  
>>>
>>> Ok great, thanks Frank.  
>>
>> My apologies, I haven't done this yet and I'm going on vacation for a week or so.
>> I'll get back to this.
> 
> Hi Frank, any news on this ? I'm asking again, but if you do not have
> time for this, do you mind if I re-spin your series ? This item is
> important for us.

No problem with asking again.

I am actively working on this.  Almost finished.

-Frank

> 
> Thanks,
> 
> Clément
> 
>>
>> This is one of the three items at the top of my devicetree todo list (along with
>> Lizhi Hou's "Generate device tree node for pci devices" patch series).
>>
>> -Frank
>>
>>>>
>>>> If I properly captured all the comments, I'll have to implement
>>>> Rob's suggestion:
>>>>
>>>>   "either CONFIG_OF_FLATTREE or CONFIG_OF_EARLY_FLATTREE will need 
>>>>   to become user selectable."  
>>>   
>>>>
>>>> -Frank  
>>>
>>>
>>>   
>>
> 
> 
> 

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

[frowand.list]

From: Frank Rowand <frank.rowand@sony.com>

 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 <frank.rowand@sony.com>
---
 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 7f6bba18c515..9d106998c1f2 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 <frank.rowand@sony.com>

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

[Rob Herring]

On Thu, Jun 23, 2022 at 10:43:27PM -0500, frowand.list@gmail.com wrote:
> From: Frank Rowand <frank.rowand@sony.com>
> 
>  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.

What happened with the formatting here?

> 
> Signed-off-by: Frank Rowand <frank.rowand@sony.com>
> ---
>  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 7f6bba18c515..9d106998c1f2 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 <frank.rowand@sony.com>
> 
> 

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

[Frank Rowand]

On 6/28/22 10:36, Rob Herring wrote:
> On Thu, Jun 23, 2022 at 10:43:27PM -0500, frowand.list@gmail.com wrote:
>> From: Frank Rowand <frank.rowand@sony.com>
>>
>>  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.
> 
> What happened with the formatting here?

I'm guessing you are referring to the leading space?

I pasted the text from another file into the commit message in
my repo and failed to remove the leading blank. So a "typo"
on my part.

-Frank

> 
>>
>> Signed-off-by: Frank Rowand <frank.rowand@sony.com>
>> ---
>>  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 7f6bba18c515..9d106998c1f2 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 <frank.rowand@sony.com>
>>
>>

[PATCH 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 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 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>
---

There are checkpatch warnings.  I have reviewed them and feel
they can be ignored.

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..0d2f6d016b7e 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 void *__dtb_empty_root_begin;
+extern void *__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>