[PATCH] of: never fixup internal live tree

[PATCH] of: never fixup internal live tree

[Sascha Hauer]

Calling of_fix_tree() on the internal live tree has undesired side
effects, so refrain from doing so.

One of the side effects is that some parts of barebox store pointers
to properties in the live tree which might become invalid when during
of_fixup these properties are deleted or updated.

The other is that it's unexpected that the live tree is modified
after a dry run boot with the live tree.

This changes of_get_fixed_tree() to always call of_fix_tree() on
a copy of the device tree and not on the device tree itself. To
emphasize this make the device tree argument to of_get_fixed_tree()
const.

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 common/oftree.c | 19 ++++++++++---------
 include/of.h    |  2 +-
 2 files changed, 11 insertions(+), 10 deletions(-)

diff --git a/common/oftree.c b/common/oftree.c
index 3e85070d11..51eebd36bd 100644
--- a/common/oftree.c
+++ b/common/oftree.c
@@ -416,26 +416,27 @@ void of_fix_tree(struct device_node *node)
  * It increases the size of the tree and applies the registered
  * fixups.
  */
-struct fdt_header *of_get_fixed_tree(struct device_node *node)
+struct fdt_header *of_get_fixed_tree(const struct device_node *node)
 {
 	struct fdt_header *fdt = NULL;
-	struct device_node *freenp = NULL;
+	struct device_node *np;
 
 	if (!node) {
 		node = of_get_root_node();
 		if (!node)
 			return NULL;
-
-		freenp = node = of_dup(node);
-		if (!node)
-			return NULL;
 	}
 
-	of_fix_tree(node);
+	np = of_dup(node);
+
+	if (!np)
+		return NULL;
+
+	of_fix_tree(np);
 
-	fdt = of_flatten_dtb(node);
+	fdt = of_flatten_dtb(np);
 
-	of_delete_node(freenp);
+	of_delete_node(np);
 
 	return fdt;
 }
diff --git a/include/of.h b/include/of.h
index 92a15f5c4a..0deb327971 100644
--- a/include/of.h
+++ b/include/of.h
@@ -75,7 +75,7 @@ int of_add_initrd(struct device_node *root, resource_size_t start,
 
 struct fdt_header *fdt_get_tree(void);
 
-struct fdt_header *of_get_fixed_tree(struct device_node *node);
+struct fdt_header *of_get_fixed_tree(const struct device_node *node);
 
 /* Helper to read a big number; size is in cells (not bytes) */
 static inline u64 of_read_number(const __be32 *cell, int size)
-- 
2.39.2

Re: [PATCH] of: never fixup internal live tree

[Ahmad Fatoum]

On 03.08.23 10:51, Sascha Hauer wrote:
> Calling of_fix_tree() on the internal live tree has undesired side
> effects, so refrain from doing so.
> 
> One of the side effects is that some parts of barebox store pointers
> to properties in the live tree which might become invalid when during
> of_fixup these properties are deleted or updated.
> 
> The other is that it's unexpected that the live tree is modified
> after a dry run boot with the live tree.
> 
> This changes of_get_fixed_tree() to always call of_fix_tree() on
> a copy of the device tree and not on the device tree itself. To
> emphasize this make the device tree argument to of_get_fixed_tree()
> const.
> 
> Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>

Reviewed-by: Ahmad Fatoum <a.fatoum@pengutronix.de>

> ---
>  common/oftree.c | 19 ++++++++++---------
>  include/of.h    |  2 +-
>  2 files changed, 11 insertions(+), 10 deletions(-)
> 
> diff --git a/common/oftree.c b/common/oftree.c
> index 3e85070d11..51eebd36bd 100644
> --- a/common/oftree.c
> +++ b/common/oftree.c
> @@ -416,26 +416,27 @@ void of_fix_tree(struct device_node *node)
>   * It increases the size of the tree and applies the registered
>   * fixups.
>   */
> -struct fdt_header *of_get_fixed_tree(struct device_node *node)
> +struct fdt_header *of_get_fixed_tree(const struct device_node *node)
>  {
>  	struct fdt_header *fdt = NULL;
> -	struct device_node *freenp = NULL;
> +	struct device_node *np;
>  
>  	if (!node) {
>  		node = of_get_root_node();
>  		if (!node)
>  			return NULL;
> -
> -		freenp = node = of_dup(node);
> -		if (!node)
> -			return NULL;
>  	}
>  
> -	of_fix_tree(node);
> +	np = of_dup(node);
> +
> +	if (!np)
> +		return NULL;
> +
> +	of_fix_tree(np);
>  
> -	fdt = of_flatten_dtb(node);
> +	fdt = of_flatten_dtb(np);
>  
> -	of_delete_node(freenp);
> +	of_delete_node(np);
>  
>  	return fdt;
>  }
> diff --git a/include/of.h b/include/of.h
> index 92a15f5c4a..0deb327971 100644
> --- a/include/of.h
> +++ b/include/of.h
> @@ -75,7 +75,7 @@ int of_add_initrd(struct device_node *root, resource_size_t start,
>  
>  struct fdt_header *fdt_get_tree(void);
>  
> -struct fdt_header *of_get_fixed_tree(struct device_node *node);
> +struct fdt_header *of_get_fixed_tree(const struct device_node *node);
>  
>  /* Helper to read a big number; size is in cells (not bytes) */
>  static inline u64 of_read_number(const __be32 *cell, int size)

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH] of: never fixup internal live tree

[Alexander Shiyan]

Now it works as expected.

Tested-by: Alexander Shiyan <eagle.alexander923@gmail.com>

чт, 3 авг. 2023 г. в 11:51, Sascha Hauer <s.hauer@pengutronix.de>:
>
> Calling of_fix_tree() on the internal live tree has undesired side
> effects, so refrain from doing so.
>
> One of the side effects is that some parts of barebox store pointers
> to properties in the live tree which might become invalid when during
> of_fixup these properties are deleted or updated.
>
> The other is that it's unexpected that the live tree is modified
> after a dry run boot with the live tree.
>
> This changes of_get_fixed_tree() to always call of_fix_tree() on
> a copy of the device tree and not on the device tree itself. To
> emphasize this make the device tree argument to of_get_fixed_tree()
> const.
>
> Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
> ---
>  common/oftree.c | 19 ++++++++++---------
>  include/of.h    |  2 +-
>  2 files changed, 11 insertions(+), 10 deletions(-)
>
> diff --git a/common/oftree.c b/common/oftree.c
> index 3e85070d11..51eebd36bd 100644
> --- a/common/oftree.c
> +++ b/common/oftree.c
> @@ -416,26 +416,27 @@ void of_fix_tree(struct device_node *node)
>   * It increases the size of the tree and applies the registered
>   * fixups.
>   */
> -struct fdt_header *of_get_fixed_tree(struct device_node *node)
> +struct fdt_header *of_get_fixed_tree(const struct device_node *node)
>  {
>         struct fdt_header *fdt = NULL;
> -       struct device_node *freenp = NULL;
> +       struct device_node *np;
>
>         if (!node) {
>                 node = of_get_root_node();
>                 if (!node)
>                         return NULL;
> -
> -               freenp = node = of_dup(node);
> -               if (!node)
> -                       return NULL;
>         }
>
> -       of_fix_tree(node);
> +       np = of_dup(node);
> +
> +       if (!np)
> +               return NULL;
> +
> +       of_fix_tree(np);
>
> -       fdt = of_flatten_dtb(node);
> +       fdt = of_flatten_dtb(np);
>
> -       of_delete_node(freenp);
> +       of_delete_node(np);
>
>         return fdt;
>  }
> diff --git a/include/of.h b/include/of.h
> index 92a15f5c4a..0deb327971 100644
> --- a/include/of.h
> +++ b/include/of.h
> @@ -75,7 +75,7 @@ int of_add_initrd(struct device_node *root, resource_size_t start,
>
>  struct fdt_header *fdt_get_tree(void);
>
> -struct fdt_header *of_get_fixed_tree(struct device_node *node);
> +struct fdt_header *of_get_fixed_tree(const struct device_node *node);
>
>  /* Helper to read a big number; size is in cells (not bytes) */
>  static inline u64 of_read_number(const __be32 *cell, int size)
> --
> 2.39.2
>

Re: [PATCH] of: never fixup internal live tree

[Ahmad Fatoum]

Hi Alexander,

On 03.08.23 12:15, Alexander Shiyan wrote:
> Now it works as expected.
> 
> Tested-by: Alexander Shiyan <eagle.alexander923@gmail.com>

I'd highly recommend that you do not use the barebox device tree for
booting Linux, so you have the flexibility in future to update one
or the other component.

Cheers,
Ahmad

> 
> чт, 3 авг. 2023 г. в 11:51, Sascha Hauer <s.hauer@pengutronix.de>:
>>
>> Calling of_fix_tree() on the internal live tree has undesired side
>> effects, so refrain from doing so.
>>
>> One of the side effects is that some parts of barebox store pointers
>> to properties in the live tree which might become invalid when during
>> of_fixup these properties are deleted or updated.
>>
>> The other is that it's unexpected that the live tree is modified
>> after a dry run boot with the live tree.
>>
>> This changes of_get_fixed_tree() to always call of_fix_tree() on
>> a copy of the device tree and not on the device tree itself. To
>> emphasize this make the device tree argument to of_get_fixed_tree()
>> const.
>>
>> Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
>> ---
>>  common/oftree.c | 19 ++++++++++---------
>>  include/of.h    |  2 +-
>>  2 files changed, 11 insertions(+), 10 deletions(-)
>>
>> diff --git a/common/oftree.c b/common/oftree.c
>> index 3e85070d11..51eebd36bd 100644
>> --- a/common/oftree.c
>> +++ b/common/oftree.c
>> @@ -416,26 +416,27 @@ void of_fix_tree(struct device_node *node)
>>   * It increases the size of the tree and applies the registered
>>   * fixups.
>>   */
>> -struct fdt_header *of_get_fixed_tree(struct device_node *node)
>> +struct fdt_header *of_get_fixed_tree(const struct device_node *node)
>>  {
>>         struct fdt_header *fdt = NULL;
>> -       struct device_node *freenp = NULL;
>> +       struct device_node *np;
>>
>>         if (!node) {
>>                 node = of_get_root_node();
>>                 if (!node)
>>                         return NULL;
>> -
>> -               freenp = node = of_dup(node);
>> -               if (!node)
>> -                       return NULL;
>>         }
>>
>> -       of_fix_tree(node);
>> +       np = of_dup(node);
>> +
>> +       if (!np)
>> +               return NULL;
>> +
>> +       of_fix_tree(np);
>>
>> -       fdt = of_flatten_dtb(node);
>> +       fdt = of_flatten_dtb(np);
>>
>> -       of_delete_node(freenp);
>> +       of_delete_node(np);
>>
>>         return fdt;
>>  }
>> diff --git a/include/of.h b/include/of.h
>> index 92a15f5c4a..0deb327971 100644
>> --- a/include/of.h
>> +++ b/include/of.h
>> @@ -75,7 +75,7 @@ int of_add_initrd(struct device_node *root, resource_size_t start,
>>
>>  struct fdt_header *fdt_get_tree(void);
>>
>> -struct fdt_header *of_get_fixed_tree(struct device_node *node);
>> +struct fdt_header *of_get_fixed_tree(const struct device_node *node);
>>
>>  /* Helper to read a big number; size is in cells (not bytes) */
>>  static inline u64 of_read_number(const __be32 *cell, int size)
>> --
>> 2.39.2
>>
> 
> 

-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |