* [PATCH 1/3] of: change superfluous of_fix_tree int return type to void
2023-06-08 13:40 [PATCH 0/3] commands: of_diff: support applying fixups on arbitrary device trees Ahmad Fatoum
@ 2023-06-08 13:40 ` Ahmad Fatoum
2023-06-08 13:40 ` [PATCH 2/3] commands: of_diff: simplify error handling Ahmad Fatoum
` (2 subsequent siblings)
3 siblings, 0 replies; 6+ messages in thread
From: Ahmad Fatoum @ 2023-06-08 13:40 UTC (permalink / raw)
To: barebox; +Cc: 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
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 2/3] commands: of_diff: simplify error handling
2023-06-08 13:40 [PATCH 0/3] commands: of_diff: support applying fixups on arbitrary device trees Ahmad Fatoum
2023-06-08 13:40 ` [PATCH 1/3] of: change superfluous of_fix_tree int return type to void Ahmad Fatoum
@ 2023-06-08 13:40 ` Ahmad Fatoum
2023-06-13 11:48 ` Johannes Zink
2023-06-08 13:40 ` [PATCH 3/3] commands: of_diff: support applying fixups on arbitrary device trees Ahmad Fatoum
2023-06-09 6:41 ` [PATCH 0/3] " Sascha Hauer
3 siblings, 1 reply; 6+ messages in thread
From: Ahmad Fatoum @ 2023-06-08 13:40 UTC (permalink / raw)
To: barebox; +Cc: 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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2/3] commands: of_diff: simplify error handling
2023-06-08 13:40 ` [PATCH 2/3] commands: of_diff: simplify error handling Ahmad Fatoum
@ 2023-06-13 11:48 ` Johannes Zink
0 siblings, 0 replies; 6+ messages in thread
From: Johannes Zink @ 2023-06-13 11:48 UTC (permalink / raw)
To: barebox
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 |
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 3/3] commands: of_diff: support applying fixups on arbitrary device trees
2023-06-08 13:40 [PATCH 0/3] commands: of_diff: support applying fixups on arbitrary device trees Ahmad Fatoum
2023-06-08 13:40 ` [PATCH 1/3] of: change superfluous of_fix_tree int return type to void Ahmad Fatoum
2023-06-08 13:40 ` [PATCH 2/3] commands: of_diff: simplify error handling Ahmad Fatoum
@ 2023-06-08 13:40 ` Ahmad Fatoum
2023-06-09 6:41 ` [PATCH 0/3] " Sascha Hauer
3 siblings, 0 replies; 6+ messages in thread
From: Ahmad Fatoum @ 2023-06-08 13:40 UTC (permalink / raw)
To: barebox; +Cc: 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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 0/3] commands: of_diff: support applying fixups on arbitrary device trees
2023-06-08 13:40 [PATCH 0/3] commands: of_diff: support applying fixups on arbitrary device trees Ahmad Fatoum
` (2 preceding siblings ...)
2023-06-08 13:40 ` [PATCH 3/3] commands: of_diff: support applying fixups on arbitrary device trees Ahmad Fatoum
@ 2023-06-09 6:41 ` Sascha Hauer
3 siblings, 0 replies; 6+ messages in thread
From: Sascha Hauer @ 2023-06-09 6:41 UTC (permalink / raw)
To: Ahmad Fatoum; +Cc: barebox
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 |
^ permalink raw reply [flat|nested] 6+ messages in thread