[PATCH 0/3] commands: of_diff: support applying fixups on arbitrary device trees

[PATCH 0/3] commands: of_diff: support applying fixups on arbitrary device trees

[Ahmad Fatoum]

of_diff - + is quite useful to find what fixups barebox would apply to
its own device tree. But for some cases, like barebox-state or OP-TEE
reservations, the nodes that would be fixed up into the kernel device
tree already exist within the barebox device tree and wouldn't be shown
by of_diff - +. To make them visible, let's redefine +:

  - Previously, it always meant the fixed up barebox DT
  - Now, it means the other DT with fixups applied

That way, of_diff - + continues to work, but kernel DTs can now be
diffed against their fixed up version by doing:

   of_diff /mnt/tftp/afa-oftree-myboard +

Ahmad Fatoum (3):
  of: change superfluous of_fix_tree int return type to void
  commands: of_diff: simplify error handling
  commands: of_diff: support applying fixups on arbitrary device trees

 commands/of_compatible.c |  7 ++---
 commands/of_diff.c       | 63 +++++++++++++++++++---------------------
 commands/of_dump.c       |  7 ++---
 common/oftree.c          | 10 ++-----
 drivers/of/base.c        |  3 ++
 include/of.h             |  2 +-
 6 files changed, 40 insertions(+), 52 deletions(-)

-- 
2.39.2

[PATCH 1/3] of: change superfluous of_fix_tree int return type to void

[Ahmad Fatoum]

of_fix_tree always returns 0, so propagating its return code and
checking it serves no purpose. Let's just change it to void *.

I considered having it return the first argument, but that could mislead
users to think that the argument is not modified.

Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
---
 commands/of_compatible.c |  7 ++-----
 commands/of_dump.c       |  7 ++-----
 common/oftree.c          | 10 ++--------
 include/of.h             |  2 +-
 4 files changed, 7 insertions(+), 19 deletions(-)

diff --git a/commands/of_compatible.c b/commands/of_compatible.c
index b460fecd3a86..d8704d50bb72 100644
--- a/commands/of_compatible.c
+++ b/commands/of_compatible.c
@@ -57,11 +57,8 @@ static int do_of_compatible(int argc, char *argv[])
 			root = of_free = of_dup(root);
 	}
 
-	if (fix) {
-		ret = of_fix_tree(root);
-		if (ret)
-			goto out;
-	}
+	if (fix)
+		of_fix_tree(root);
 
 	node = of_find_node_by_path_or_alias(root, nodename);
 	if (!node) {
diff --git a/commands/of_dump.c b/commands/of_dump.c
index 1d6c019bf08c..c77dc27c99b9 100644
--- a/commands/of_dump.c
+++ b/commands/of_dump.c
@@ -84,11 +84,8 @@ static int do_of_dump(int argc, char *argv[])
 			root = of_free = of_dup(root);
 	}
 
-	if (fix) {
-		ret = of_fix_tree(root);
-		if (ret)
-			goto out;
-	}
+	if (fix)
+		of_fix_tree(root);
 
 	node = of_find_node_by_path_or_alias(root, nodename);
 	if (!node) {
diff --git a/common/oftree.c b/common/oftree.c
index 1f751cc0ac89..620de6ed5649 100644
--- a/common/oftree.c
+++ b/common/oftree.c
@@ -392,7 +392,7 @@ int of_unregister_fixup(int (*fixup)(struct device_node *, void *),
  * Apply registered fixups for the given fdt. The fdt must have
  * enough free space to apply the fixups.
  */
-int of_fix_tree(struct device_node *node)
+void of_fix_tree(struct device_node *node)
 {
 	struct of_fixup *of_fixup;
 	int ret;
@@ -408,8 +408,6 @@ int of_fix_tree(struct device_node *node)
 			pr_warn("Failed to fixup node in %pS: %s\n",
 					of_fixup->fixup, strerror(-ret));
 	}
-
-	return 0;
 }
 
 /*
@@ -420,7 +418,6 @@ int of_fix_tree(struct device_node *node)
  */
 struct fdt_header *of_get_fixed_tree(struct device_node *node)
 {
-	int ret;
 	struct fdt_header *fdt = NULL;
 	struct device_node *freenp = NULL;
 
@@ -434,13 +431,10 @@ struct fdt_header *of_get_fixed_tree(struct device_node *node)
 			return NULL;
 	}
 
-	ret = of_fix_tree(node);
-	if (ret)
-		goto out;
+	of_fix_tree(node);
 
 	fdt = of_flatten_dtb(node);
 
-out:
 	of_delete_node(freenp);
 
 	return fdt;
diff --git a/include/of.h b/include/of.h
index 2b75ce63e185..0037bad6c4d7 100644
--- a/include/of.h
+++ b/include/of.h
@@ -65,7 +65,7 @@ struct device;
 struct driver;
 struct resource;
 
-int of_fix_tree(struct device_node *);
+void of_fix_tree(struct device_node *);
 
 int of_match(struct device *dev, struct driver *drv);
 
-- 
2.39.2

[PATCH 2/3] commands: of_diff: simplify error handling

[Ahmad Fatoum]

The error check and message is duplicated for each argument and
could be unified if we move it to get_tree instead.

While at it, we limit argc to exactly 3 in case we want to add options
in the future.

Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
---
 commands/of_diff.c | 55 ++++++++++++++++------------------------------
 drivers/of/base.c  |  3 +++
 2 files changed, 22 insertions(+), 36 deletions(-)

diff --git a/commands/of_diff.c b/commands/of_diff.c
index 19a4a26d2012..ff3d46c7f530 100644
--- a/commands/of_diff.c
+++ b/commands/of_diff.c
@@ -17,60 +17,43 @@ static struct device_node *get_tree(const char *filename, struct device_node *ro
 {
 	struct device_node *node;
 
+	if (!root)
+		root = ERR_PTR(-ENOENT);
+
 	if (!strcmp(filename, "-")) {
-		node = of_get_root_node();
-		if (!node)
-			return ERR_PTR(-ENOENT);
-
-		return of_dup(node);
-	}
-
-	if (!strcmp(filename, "+")) {
-		node = of_get_root_node();
-		if (!node)
-			return ERR_PTR(-ENOENT);
-
 		node = of_dup(root);
-
-		of_fix_tree(node);
-
-		return node;
+	} else if (!strcmp(filename, "+")) {
+		node = of_dup(root);
+		if (!IS_ERR(node))
+			of_fix_tree(node);
+	} else {
+		node = of_read_file(filename);
 	}
 
-	return of_read_file(filename);
+	if (IS_ERR(node))
+		printf("Cannot read %s: %pe\n", filename, node);
+
+	return node;
 }
 
 static int do_of_diff(int argc, char *argv[])
 {
-	int ret = 0;
+	int ret = COMMAND_ERROR;
 	struct device_node *a, *b, *root;
 
-	if (argc < 3)
+	if (argc != 3)
 		return COMMAND_ERROR_USAGE;
 
 	root = of_get_root_node();
 	a = get_tree(argv[1], root);
 	b = get_tree(argv[2], root);
 
-	if (IS_ERR(a)) {
-		printf("Cannot read %s: %pe\n", argv[1], a);
-		ret = COMMAND_ERROR;
-		a = NULL;
-		goto out;
-	}
+	if (!IS_ERR(a) && !IS_ERR(b))
+		ret = of_diff(a, b, 0) ? COMMAND_ERROR : COMMAND_SUCCESS;
 
-	if (IS_ERR(b)) {
-		printf("Cannot read %s: %pe\n", argv[2], b);
-		ret = COMMAND_ERROR;
-		b = NULL;
-		goto out;
-	}
-
-	ret = of_diff(a, b, 0) ? COMMAND_ERROR : COMMAND_SUCCESS;
-out:
-	if (a && a != root)
+	if (!IS_ERR(a) && a != root)
 		of_delete_node(a);
-	if (b && b != root)
+	if (!IS_ERR(b) && b != root)
 		of_delete_node(b);
 
 	return ret;
diff --git a/drivers/of/base.c b/drivers/of/base.c
index 5da188115547..e3416b5b75a7 100644
--- a/drivers/of/base.c
+++ b/drivers/of/base.c
@@ -2660,6 +2660,9 @@ struct device_node *of_copy_node(struct device_node *parent, const struct device
 
 struct device_node *of_dup(const struct device_node *root)
 {
+	if (IS_ERR_OR_NULL(root))
+		return ERR_CAST(root);
+
 	return of_copy_node(NULL, root);
 }
 
-- 
2.39.2

Re: [PATCH 2/3] commands: of_diff: simplify error handling

[Johannes Zink]

FTR - though I'm too late to the party:

This also fixes a null pointer exception when running of_diff with invalid 
arguments: 
https://lore.barebox.org/barebox/044fee32-5560-2e85-a0d3-cc7cea71ed48@pengutronix.de/

Tested-by: Johannes Zink <j.zink@pengutronix.de>

On 6/8/23 15:40, Ahmad Fatoum wrote:
> The error check and message is duplicated for each argument and
> could be unified if we move it to get_tree instead.
> 
> While at it, we limit argc to exactly 3 in case we want to add options
> in the future.
> 
> Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
> ---
>   commands/of_diff.c | 55 ++++++++++++++++------------------------------
>   drivers/of/base.c  |  3 +++
>   2 files changed, 22 insertions(+), 36 deletions(-)
> 
> diff --git a/commands/of_diff.c b/commands/of_diff.c
> index 19a4a26d2012..ff3d46c7f530 100644
> --- a/commands/of_diff.c
> +++ b/commands/of_diff.c
> @@ -17,60 +17,43 @@ static struct device_node *get_tree(const char *filename, struct device_node *ro
>   {
>   	struct device_node *node;
>   
> +	if (!root)
> +		root = ERR_PTR(-ENOENT);
> +
>   	if (!strcmp(filename, "-")) {
> -		node = of_get_root_node();
> -		if (!node)
> -			return ERR_PTR(-ENOENT);
> -
> -		return of_dup(node);
> -	}
> -
> -	if (!strcmp(filename, "+")) {
> -		node = of_get_root_node();
> -		if (!node)
> -			return ERR_PTR(-ENOENT);
> -
>   		node = of_dup(root);
> -
> -		of_fix_tree(node);
> -
> -		return node;
> +	} else if (!strcmp(filename, "+")) {
> +		node = of_dup(root);
> +		if (!IS_ERR(node))
> +			of_fix_tree(node);
> +	} else {
> +		node = of_read_file(filename);
>   	}
>   
> -	return of_read_file(filename);
> +	if (IS_ERR(node))
> +		printf("Cannot read %s: %pe\n", filename, node);
> +
> +	return node;
>   }
>   
>   static int do_of_diff(int argc, char *argv[])
>   {
> -	int ret = 0;
> +	int ret = COMMAND_ERROR;
>   	struct device_node *a, *b, *root;
>   
> -	if (argc < 3)
> +	if (argc != 3)
>   		return COMMAND_ERROR_USAGE;
>   
>   	root = of_get_root_node();
>   	a = get_tree(argv[1], root);
>   	b = get_tree(argv[2], root);
>   
> -	if (IS_ERR(a)) {
> -		printf("Cannot read %s: %pe\n", argv[1], a);
> -		ret = COMMAND_ERROR;
> -		a = NULL;
> -		goto out;
> -	}
> +	if (!IS_ERR(a) && !IS_ERR(b))
> +		ret = of_diff(a, b, 0) ? COMMAND_ERROR : COMMAND_SUCCESS;
>   
> -	if (IS_ERR(b)) {
> -		printf("Cannot read %s: %pe\n", argv[2], b);
> -		ret = COMMAND_ERROR;
> -		b = NULL;
> -		goto out;
> -	}
> -
> -	ret = of_diff(a, b, 0) ? COMMAND_ERROR : COMMAND_SUCCESS;
> -out:
> -	if (a && a != root)
> +	if (!IS_ERR(a) && a != root)
>   		of_delete_node(a);
> -	if (b && b != root)
> +	if (!IS_ERR(b) && b != root)
>   		of_delete_node(b);
>   
>   	return ret;
> diff --git a/drivers/of/base.c b/drivers/of/base.c
> index 5da188115547..e3416b5b75a7 100644
> --- a/drivers/of/base.c
> +++ b/drivers/of/base.c
> @@ -2660,6 +2660,9 @@ struct device_node *of_copy_node(struct device_node *parent, const struct device
>   
>   struct device_node *of_dup(const struct device_node *root)
>   {
> +	if (IS_ERR_OR_NULL(root))
> +		return ERR_CAST(root);
> +
>   	return of_copy_node(NULL, root);
>   }
>   

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

[PATCH 3/3] commands: of_diff: support applying fixups on arbitrary device trees

[Ahmad Fatoum]

of_diff - + is quite useful to find what fixups barebox would apply to
its own device tree. But for some cases, like barebox-state or OP-TEE
reservations, the nodes that would be fixed up into the kernel device
tree already exist within the barebox device tree and wouldn't be shown
by of_diff - +. To make them visible, let's redefine +:

  - Previously, it always meant the fixed up barebox DT
  - Now, it means the other DT with fixups applied

That way, of_diff - + continues to work, but kernel DTs can now be
diffed against their fixed up version by doing:

  of_diff /mnt/tftp/afa-oftree-myboard +

Signed-off-by: Ahmad Fatoum <a.fatoum@pengutronix.de>
---
 commands/of_diff.c | 30 ++++++++++++++++++++++--------
 1 file changed, 22 insertions(+), 8 deletions(-)

diff --git a/commands/of_diff.c b/commands/of_diff.c
index ff3d46c7f530..8b19c093ddab 100644
--- a/commands/of_diff.c
+++ b/commands/of_diff.c
@@ -17,15 +17,10 @@ static struct device_node *get_tree(const char *filename, struct device_node *ro
 {
 	struct device_node *node;
 
-	if (!root)
-		root = ERR_PTR(-ENOENT);
-
 	if (!strcmp(filename, "-")) {
-		node = of_dup(root);
+		node = of_dup(root) ?: ERR_PTR(-ENOENT);
 	} else if (!strcmp(filename, "+")) {
-		node = of_dup(root);
-		if (!IS_ERR(node))
-			of_fix_tree(node);
+		return NULL;
 	} else {
 		node = of_read_file(filename);
 	}
@@ -36,6 +31,17 @@ static struct device_node *get_tree(const char *filename, struct device_node *ro
 	return node;
 }
 
+static struct device_node *get_tree_fixed(const struct device_node *root)
+{
+	struct device_node *node;
+
+	node = of_dup(root);
+	if (!IS_ERR(node))
+		of_fix_tree(node);
+
+	return node;
+}
+
 static int do_of_diff(int argc, char *argv[])
 {
 	int ret = COMMAND_ERROR;
@@ -48,6 +54,14 @@ static int do_of_diff(int argc, char *argv[])
 	a = get_tree(argv[1], root);
 	b = get_tree(argv[2], root);
 
+	if (!a && !b)
+		return COMMAND_ERROR_USAGE;
+
+	if (!a)
+		a = get_tree_fixed(b);
+	if (!b)
+		b = get_tree_fixed(a);
+
 	if (!IS_ERR(a) && !IS_ERR(b))
 		ret = of_diff(a, b, 0) ? COMMAND_ERROR : COMMAND_SUCCESS;
 
@@ -63,7 +77,7 @@ BAREBOX_CMD_HELP_START(of_diff)
 BAREBOX_CMD_HELP_TEXT("This command prints a diff between two given device trees.")
 BAREBOX_CMD_HELP_TEXT("The device trees are given as dtb files or:")
 BAREBOX_CMD_HELP_TEXT("'-' to compare against the barebox live tree, or")
-BAREBOX_CMD_HELP_TEXT("'+' to compare against the fixed barebox live tree")
+BAREBOX_CMD_HELP_TEXT("'+' to compare against the other device tree after fixups")
 BAREBOX_CMD_HELP_END
 
 BAREBOX_CMD_START(of_diff)
-- 
2.39.2

Re: [PATCH 0/3] commands: of_diff: support applying fixups on arbitrary device trees

[Sascha Hauer]

On Thu, Jun 08, 2023 at 03:40:38PM +0200, Ahmad Fatoum wrote:
> of_diff - + is quite useful to find what fixups barebox would apply to
> its own device tree. But for some cases, like barebox-state or OP-TEE
> reservations, the nodes that would be fixed up into the kernel device
> tree already exist within the barebox device tree and wouldn't be shown
> by of_diff - +. To make them visible, let's redefine +:
> 
>   - Previously, it always meant the fixed up barebox DT
>   - Now, it means the other DT with fixups applied
> 
> That way, of_diff - + continues to work, but kernel DTs can now be
> diffed against their fixed up version by doing:
> 
>    of_diff /mnt/tftp/afa-oftree-myboard +
> 
> Ahmad Fatoum (3):
>   of: change superfluous of_fix_tree int return type to void
>   commands: of_diff: simplify error handling
>   commands: of_diff: support applying fixups on arbitrary device trees

Applied, thanks

Sascha

> 
>  commands/of_compatible.c |  7 ++---
>  commands/of_diff.c       | 63 +++++++++++++++++++---------------------
>  commands/of_dump.c       |  7 ++---
>  common/oftree.c          | 10 ++-----
>  drivers/of/base.c        |  3 ++
>  include/of.h             |  2 +-
>  6 files changed, 40 insertions(+), 52 deletions(-)
> 
> -- 
> 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 |