[PATCH 1/8] of: overlay: add of.overlay.fitconfigpattern param

[PATCH 1/8] of: overlay: add of.overlay.fitconfigpattern param

[Marco Felsch]

Add the a parameter which will be used by image-fit to decide if a
config node belongs to a devicetree-overlay.

Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
---
 drivers/of/overlay.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c
index 2d2367fc101b..27dc6443b237 100644
--- a/drivers/of/overlay.c
+++ b/drivers/of/overlay.c
@@ -396,6 +396,7 @@ int of_register_overlay(struct device_node *overlay)
 	return of_register_fixup(of_overlay_fixup, overlay);
 }
 
+static char *of_overlay_fitconfigpattern;
 static char *of_overlay_filepattern;
 static char *of_overlay_dir;
 static char *of_overlay_basedir;
@@ -586,6 +587,7 @@ static struct of_overlay_filter of_overlay_compatible_filter = {
 
 static int of_overlay_init(void)
 {
+	of_overlay_fitconfigpattern = strdup("*.dtbo");
 	of_overlay_filepattern = strdup("*");
 	of_overlay_filter = strdup("filepattern compatible");
 	of_overlay_set_basedir("/");
@@ -594,6 +596,7 @@ static int of_overlay_init(void)
 	globalvar_add_simple_string("of.overlay.filepattern", &of_overlay_filepattern);
 	globalvar_add_simple_string("of.overlay.filter", &of_overlay_filter);
 	globalvar_add_simple_string("of.overlay.dir", &of_overlay_dir);
+	globalvar_add_simple_string("of.overlay.fitconfigpattern", &of_overlay_fitconfigpattern);
 
 	of_overlay_register_filter(&of_overlay_filepattern_filter);
 	of_overlay_register_filter(&of_overlay_compatible_filter);
@@ -608,3 +611,4 @@ BAREBOX_MAGICVAR(global.of.overlay.compatible, "space separated list of compatib
 BAREBOX_MAGICVAR(global.of.overlay.filepattern, "space separated list of filepatterns an overlay must match");
 BAREBOX_MAGICVAR(global.of.overlay.dir, "Directory to look for dt overlays");
 BAREBOX_MAGICVAR(global.of.overlay.filter, "space separated list of filters");
+BAREBOX_MAGICVAR(global.of.overlay.fitconfigpattern, "FIT config node name pattern to look for dt overlays");
-- 
2.39.2

[PATCH 2/8] FIT: skip possible overlay config nodes

[Marco Felsch]

The FIT spec is not very specific when it comes to device-tree overlay
handling. Overlays can be added directely to an config node:

	config-a {
		compatible = "machine-compatible";
		kernel = "kernel-img-name";
		fdt = "fdt-base-name", "fdt-overlay1-name", "...";
	}

or they are supplied via dedicated config nodes:

	overlay-2 {
		fdt = "fdt-overlay2-name";
	}

Of course these config nodes can have compatibles as well:

	overlay-3 {
		compatible = "machine-compatible";
		fdt = "fdt-overlay3-name";
	}

The current fit_find_compatible_unit() code would skip the overlay node
if the config-a compatible has the same score as the overlay-3
compatible and if the overlay-3 config-node is listed after the config-a
config-node. But if the compatible of config-a config-node has a lower
score or the overlay-3 config-note is listed first (the spec does not
specify any order) we end up in taking the overlay-3 config-node instead
of config-a config-node.

Make to code more robust by skip all config nodes matching the pattern
from global.of.overlay.fitconfigpattern.

Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
---
 common/image-fit.c | 26 +++++++++++++++++++++++++-
 1 file changed, 25 insertions(+), 1 deletion(-)

diff --git a/common/image-fit.c b/common/image-fit.c
index b16752de05bc..bf1562315b40 100644
--- a/common/image-fit.c
+++ b/common/image-fit.c
@@ -7,6 +7,7 @@
 
 #define pr_fmt(fmt) "FIT: " fmt
 #include <common.h>
+#include <environment.h>
 #include <init.h>
 #include <bootm.h>
 #include <libfile.h>
@@ -14,6 +15,7 @@
 #include <digest.h>
 #include <of.h>
 #include <fs.h>
+#include <fnmatch.h>
 #include <malloc.h>
 #include <linux/ctype.h>
 #include <asm/byteorder.h>
@@ -715,6 +717,23 @@ static int fit_config_verify_signature(struct fit_handle *handle, struct device_
 	return ret;
 }
 
+static bool fit_config_is_overlay(struct device_node *conf_node)
+{
+	const char *overlay_pattern;
+	int no_match;
+
+	if (!IS_ENABLED(CONFIG_OF_OVERLAY))
+		return false;
+
+	overlay_pattern = getenv_nonempty("global.of.overlay.fitconfigpattern");
+	if (!overlay_pattern)
+		return false;
+
+	no_match = fnmatch(overlay_pattern, conf_node->name, 0);
+
+	return no_match ? false : true;
+}
+
 static int fit_find_compatible_unit(struct device_node *conf_node,
 				    const char **unit)
 {
@@ -733,7 +752,12 @@ static int fit_find_compatible_unit(struct device_node *conf_node,
 		return -ENOENT;
 
 	for_each_child_of_node(conf_node, child) {
-		int score = of_device_is_compatible(child, machine);
+		int score;
+
+		if (fit_config_is_overlay(child))
+			continue;
+
+		score = of_device_is_compatible(child, machine);
 		if (score > best_score) {
 			best_score = score;
 			*unit = child->name;
-- 
2.39.2

[PATCH 3/8] of: overlay: make the pattern match function more generic

[Marco Felsch]

The current overlay mechanism can handle files only, so filepattern was
obvious. With the next commit this will change, so overlays can also
supplied via FIT images. To prepare the pattern filter to match FIT
config-nodes make the filter and global variable handling the pattern
more generic by dropping the "file" prefix.

Keep the backward compatibility by still providing the filepattern
filter and the global of.overlay.filepattern variable but notice the
user that the usage of those are deprecated.

No functional change.

Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
---
 drivers/of/overlay.c | 37 ++++++++++++++++++++++++++-----------
 1 file changed, 26 insertions(+), 11 deletions(-)

diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c
index 27dc6443b237..b688e708c382 100644
--- a/drivers/of/overlay.c
+++ b/drivers/of/overlay.c
@@ -397,7 +397,7 @@ int of_register_overlay(struct device_node *overlay)
 }
 
 static char *of_overlay_fitconfigpattern;
-static char *of_overlay_filepattern;
+static char *of_overlay_pattern;
 static char *of_overlay_dir;
 static char *of_overlay_basedir;
 
@@ -494,10 +494,10 @@ int of_overlay_register_filter(struct of_overlay_filter *filter)
 }
 
 /**
- * of_overlay_filter_filename - A filter that matches on the filename of
+ * of_overlay_filter_pattern - A filter that matches on the filename or
  *                                an overlay
  * @f: The filter
- * @filename: The filename of the overlay
+ * @pattern: The filename of the overlay
  *
  * This filter matches when the filename matches one of the patterns given
  * in global.of.overlay.filepattern. global.of.overlay.filepattern shall
@@ -505,20 +505,20 @@ int of_overlay_register_filter(struct of_overlay_filter *filter)
  *
  * @return: True when the overlay shall be applied, false otherwise.
  */
-static bool of_overlay_filter_filename(struct of_overlay_filter *f,
-				       const char *filename)
+static bool of_overlay_filter_pattern(struct of_overlay_filter *f,
+				      const char *pattern)
 {
 	char *p, *path, *n;
 	int ret;
 	bool apply;
 
-	p = path = strdup(of_overlay_filepattern);
+	p = path = strdup(of_overlay_pattern);
 
 	while ((n = strsep_unescaped(&p, " "))) {
 		if (!*n)
 			continue;
 
-		ret = fnmatch(n, filename, 0);
+		ret = fnmatch(n, pattern, 0);
 
 		if (!ret) {
 			apply = true;
@@ -533,6 +533,18 @@ static bool of_overlay_filter_filename(struct of_overlay_filter *f,
 	return apply;
 }
 
+static struct of_overlay_filter of_overlay_pattern_filter = {
+	.name = "pattern",
+	.filter_filename = of_overlay_filter_pattern,
+};
+
+static bool of_overlay_filter_filename(struct of_overlay_filter *f,
+				       const char *filename)
+{
+	pr_warn("'filepattern' filter is marked as deprecated, convert to 'pattern' filter\n");
+	return of_overlay_filter_pattern(f, filename);
+}
+
 static struct of_overlay_filter of_overlay_filepattern_filter = {
 	.name = "filepattern",
 	.filter_filename = of_overlay_filter_filename,
@@ -588,16 +600,19 @@ static struct of_overlay_filter of_overlay_compatible_filter = {
 static int of_overlay_init(void)
 {
 	of_overlay_fitconfigpattern = strdup("*.dtbo");
-	of_overlay_filepattern = strdup("*");
-	of_overlay_filter = strdup("filepattern compatible");
+	of_overlay_pattern = strdup("*");
+	of_overlay_filter = strdup("pattern compatible");
 	of_overlay_set_basedir("/");
 
 	globalvar_add_simple_string("of.overlay.compatible", &of_overlay_compatible);
-	globalvar_add_simple_string("of.overlay.filepattern", &of_overlay_filepattern);
+	globalvar_add_simple_string("of.overlay.pattern", &of_overlay_pattern);
 	globalvar_add_simple_string("of.overlay.filter", &of_overlay_filter);
 	globalvar_add_simple_string("of.overlay.dir", &of_overlay_dir);
 	globalvar_add_simple_string("of.overlay.fitconfigpattern", &of_overlay_fitconfigpattern);
 
+	globalvar_alias_deprecated("of.overlay.filepattern", "of.overlay.pattern");
+
+	of_overlay_register_filter(&of_overlay_pattern_filter);
 	of_overlay_register_filter(&of_overlay_filepattern_filter);
 	of_overlay_register_filter(&of_overlay_compatible_filter);
 
@@ -608,7 +623,7 @@ static int of_overlay_init(void)
 device_initcall(of_overlay_init);
 
 BAREBOX_MAGICVAR(global.of.overlay.compatible, "space separated list of compatibles an overlay must match");
-BAREBOX_MAGICVAR(global.of.overlay.filepattern, "space separated list of filepatterns an overlay must match");
+BAREBOX_MAGICVAR(global.of.overlay.pattern, "space separated list of filepatterns an overlay must match");
 BAREBOX_MAGICVAR(global.of.overlay.dir, "Directory to look for dt overlays");
 BAREBOX_MAGICVAR(global.of.overlay.filter, "space separated list of filters");
 BAREBOX_MAGICVAR(global.of.overlay.fitconfigpattern, "FIT config node name pattern to look for dt overlays");
-- 
2.39.2

[PATCH 4/8] of: overlay: make search dir/path more generic

[Marco Felsch]

In preparation of adding support for FIT image overlay handling the
global.of.overlay.dir can be reused if we rename it to a more generic
global.of.overlay.path since it does not imply an directory.

This commit adds the ground work by deprecating the old
global.of.overlay.dir and moving the directory handling into a separate
of_overlay_global_fixup_dir() helper.

No functional change.

Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
---
 drivers/of/overlay.c | 22 ++++++++++++++--------
 1 file changed, 14 insertions(+), 8 deletions(-)

diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c
index b688e708c382..e9fd5c0a1f7d 100644
--- a/drivers/of/overlay.c
+++ b/drivers/of/overlay.c
@@ -398,7 +398,7 @@ int of_register_overlay(struct device_node *overlay)
 
 static char *of_overlay_fitconfigpattern;
 static char *of_overlay_pattern;
-static char *of_overlay_dir;
+static char *of_overlay_path;
 static char *of_overlay_basedir;
 
 /**
@@ -453,18 +453,18 @@ static int of_overlay_apply_dir(struct device_node *root, const char *dirname,
 	return ret;
 }
 
-static int of_overlay_global_fixup(struct device_node *root, void *data)
+static int of_overlay_global_fixup_dir(struct device_node *root, const char *ovl_dir)
 {
 	char *dir;
 	int ret;
 
-	if (*of_overlay_dir == '/')
-		return of_overlay_apply_dir(root, of_overlay_dir, true);
+	if (*ovl_dir == '/')
+		return of_overlay_apply_dir(root, ovl_dir, true);
 
-	if (*of_overlay_dir == '\0')
+	if (*ovl_dir == '\0')
 		return 0;
 
-	dir = concat_path_file(of_overlay_basedir, of_overlay_dir);
+	dir = concat_path_file(of_overlay_basedir, ovl_dir);
 
 	ret = of_overlay_apply_dir(root, dir, true);
 
@@ -473,6 +473,11 @@ static int of_overlay_global_fixup(struct device_node *root, void *data)
 	return ret;
 }
 
+static int of_overlay_global_fixup(struct device_node *root, void *data)
+{
+	return of_overlay_global_fixup_dir(root, of_overlay_path);
+}
+
 /**
  * of_overlay_register_filter - register a new overlay filter
  * @filter: The new filter
@@ -607,10 +612,11 @@ static int of_overlay_init(void)
 	globalvar_add_simple_string("of.overlay.compatible", &of_overlay_compatible);
 	globalvar_add_simple_string("of.overlay.pattern", &of_overlay_pattern);
 	globalvar_add_simple_string("of.overlay.filter", &of_overlay_filter);
-	globalvar_add_simple_string("of.overlay.dir", &of_overlay_dir);
+	globalvar_add_simple_string("of.overlay.path", &of_overlay_path);
 	globalvar_add_simple_string("of.overlay.fitconfigpattern", &of_overlay_fitconfigpattern);
 
 	globalvar_alias_deprecated("of.overlay.filepattern", "of.overlay.pattern");
+	globalvar_alias_deprecated("of.overlay.dir", "of.overlay.path");
 
 	of_overlay_register_filter(&of_overlay_pattern_filter);
 	of_overlay_register_filter(&of_overlay_filepattern_filter);
@@ -624,6 +630,6 @@ device_initcall(of_overlay_init);
 
 BAREBOX_MAGICVAR(global.of.overlay.compatible, "space separated list of compatibles an overlay must match");
 BAREBOX_MAGICVAR(global.of.overlay.pattern, "space separated list of filepatterns an overlay must match");
-BAREBOX_MAGICVAR(global.of.overlay.dir, "Directory to look for dt overlays");
+BAREBOX_MAGICVAR(global.of.overlay.path, "Path to look for dt overlays");
 BAREBOX_MAGICVAR(global.of.overlay.filter, "space separated list of filters");
 BAREBOX_MAGICVAR(global.of.overlay.fitconfigpattern, "FIT config node name pattern to look for dt overlays");
-- 
2.39.2

[PATCH 5/8] FIT: expose useful helpers

[Marco Felsch]

The upcoming FIT overlay support requires these helpers for performing
check on a fit image.

Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
---
 common/image-fit.c  | 4 ++--
 include/image-fit.h | 2 ++
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/common/image-fit.c b/common/image-fit.c
index bf1562315b40..1e5d70caaaae 100644
--- a/common/image-fit.c
+++ b/common/image-fit.c
@@ -677,7 +677,7 @@ int fit_open_image(struct fit_handle *handle, void *configuration,
 	return 0;
 }
 
-static int fit_config_verify_signature(struct fit_handle *handle, struct device_node *conf_node)
+int fit_config_verify_signature(struct fit_handle *handle, struct device_node *conf_node)
 {
 	struct device_node *sig_node;
 	int ret = -EINVAL;
@@ -717,7 +717,7 @@ static int fit_config_verify_signature(struct fit_handle *handle, struct device_
 	return ret;
 }
 
-static bool fit_config_is_overlay(struct device_node *conf_node)
+bool fit_config_is_overlay(struct device_node *conf_node)
 {
 	const char *overlay_pattern;
 	int no_match;
diff --git a/include/image-fit.h b/include/image-fit.h
index 0b8e94bf4635..7e0cd38fea53 100644
--- a/include/image-fit.h
+++ b/include/image-fit.h
@@ -35,6 +35,8 @@ int fit_open_image(struct fit_handle *handle, void *configuration,
 int fit_get_image_address(struct fit_handle *handle, void *configuration,
 			  const char *name, const char *property,
 			  unsigned long *address);
+int fit_config_verify_signature(struct fit_handle *handle, struct device_node *conf_node);
+bool fit_config_is_overlay(struct device_node *conf_node);
 
 void fit_close(struct fit_handle *handle);
 
-- 
2.39.2

[PATCH 6/8] of: overlay: add FIT overlay support

[Marco Felsch]

This adds the support to load devicetree overlays from an FIT image.
There are quite a few options to handle FIT overlays since the FIT
overlay spec is not very strict.

This patch implement the most configurable case where each overlay does
have it's own config node (including the optional signature).

- The "name" filter check is performed on the config-node name (the node
  under the configurations) and not the FIT overlay image name (the node
  name under the images node).
- The "content" filter check does not differ from the file based overlay
  handling.

Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
---
 drivers/of/overlay.c | 110 ++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 103 insertions(+), 7 deletions(-)

diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c
index e9fd5c0a1f7d..c8e70ab00091 100644
--- a/drivers/of/overlay.c
+++ b/drivers/of/overlay.c
@@ -8,10 +8,12 @@
  */
 #define pr_fmt(fmt) "of_overlay: " fmt
 
+#include <bootm.h>
 #include <common.h>
 #include <of.h>
 #include <errno.h>
 #include <globalvar.h>
+#include <image-fit.h>
 #include <magicvar.h>
 #include <string.h>
 #include <libfile.h>
@@ -473,9 +475,103 @@ static int of_overlay_global_fixup_dir(struct device_node *root, const char *ovl
 	return ret;
 }
 
+static int of_overlay_apply_fit(struct device_node *root, struct fit_handle *fit,
+				struct device_node *config)
+{
+	const char *name = config->name;
+	struct device_node *overlay;
+	unsigned long ovl_sz;
+	const void *ovl;
+	int ret;
+
+	if (!fit_has_image(fit, config, "fdt"))
+		return 0;
+
+	if (!of_overlay_matches_filter(name, NULL))
+		return 0;
+
+	ret = fit_open_image(fit, config, "fdt", &ovl, &ovl_sz);
+	if (ret)
+		return ret;
+
+	overlay = of_unflatten_dtb(ovl, ovl_sz);
+
+	if (!of_overlay_matches_filter(NULL, overlay)) {
+		ret = 0;
+		goto out;
+	}
+
+	ret = of_overlay_apply_tree(root, overlay);
+	if (ret == -ENODEV)
+		pr_debug("Not applied %s (not compatible)\n", name);
+	else if (ret)
+		pr_err("Cannot apply %s: %s\n", name, strerror(-ret));
+	else
+		pr_info("Applied %s\n", name);
+
+out:
+	of_delete_node(overlay);
+
+	return ret;
+}
+
+static int of_overlay_global_fixup_fit(struct device_node *root, const char *ovl_dev)
+{
+	enum bootm_verify verify = bootm_get_verify_mode();
+	struct device_node *conf_node;
+	struct fit_handle *fit;
+	struct stat s;
+	int ret;
+
+	if (!IS_ENABLED(CONFIG_FITIMAGE))
+		return 0;
+
+	if (stat(of_overlay_path, &s))
+		return -errno;
+
+	fit = fit_open(ovl_dev, 0, verify, s.st_size);
+	if (IS_ERR(fit)) {
+		pr_err("Loading FIT image %s failed with: %pe\n", ovl_dev, fit);
+		return PTR_ERR(fit);
+	}
+
+	for_each_child_of_node(fit->configurations, conf_node) {
+		if (!fit_config_is_overlay(conf_node))
+			continue;
+
+		ret = fit_config_verify_signature(fit, conf_node);
+		if (ret)
+			goto out;
+
+		ret = of_overlay_apply_fit(root, fit, conf_node);
+		if (ret)
+			goto out;
+	}
+
+out:
+	fit_close(fit);
+	return ret;
+}
+
 static int of_overlay_global_fixup(struct device_node *root, void *data)
 {
-	return of_overlay_global_fixup_dir(root, of_overlay_path);
+	struct stat s;
+
+	if (isempty(of_overlay_path))
+		return 0;
+
+	if (stat(of_overlay_path, &s)) {
+		pr_err("Failed to detect file status\n");
+		return -errno;
+	}
+
+	if (S_ISDIR(s.st_mode))
+		return of_overlay_global_fixup_dir(root, of_overlay_path);
+	else if (S_ISCHR(s.st_mode) || S_ISBLK(s.st_mode))
+		return of_overlay_global_fixup_fit(root, of_overlay_path);
+
+	pr_err("of_overlay_path is invalid!\n");
+	return -EINVAL;
 }
 
 /**
@@ -500,13 +596,13 @@ int of_overlay_register_filter(struct of_overlay_filter *filter)
 
 /**
  * of_overlay_filter_pattern - A filter that matches on the filename or
- *                                an overlay
+ *			       FIT config-node name of an overlay
  * @f: The filter
- * @pattern: The filename of the overlay
+ * @pattern: The filename or FIT config-node name of the overlay
  *
- * This filter matches when the filename matches one of the patterns given
- * in global.of.overlay.filepattern. global.of.overlay.filepattern shall
- * contain a space separated list of wildcard patterns.
+ * This filter matches when the filename or FIT config-node name matches one of
+ * the patterns given in global.of.overlay.pattern. global.of.overlay.pattern
+ * shall contain a space separated list of wildcard patterns.
  *
  * @return: True when the overlay shall be applied, false otherwise.
  */
@@ -629,7 +725,7 @@ static int of_overlay_init(void)
 device_initcall(of_overlay_init);
 
 BAREBOX_MAGICVAR(global.of.overlay.compatible, "space separated list of compatibles an overlay must match");
-BAREBOX_MAGICVAR(global.of.overlay.pattern, "space separated list of filepatterns an overlay must match");
+BAREBOX_MAGICVAR(global.of.overlay.pattern, "space separated list of filename or fit config-node name patterns an overlay must match");
 BAREBOX_MAGICVAR(global.of.overlay.path, "Path to look for dt overlays");
 BAREBOX_MAGICVAR(global.of.overlay.filter, "space separated list of filters");
 BAREBOX_MAGICVAR(global.of.overlay.fitconfigpattern, "FIT config node name pattern to look for dt overlays");
-- 
2.39.2

[PATCH 7/8] of: overlay: drop unnecessary empty check in of_overlay_global_fixup_dir

[Marco Felsch]

This is now done during of_overlay_global_fixup() so we can drop it
here.

Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
---
 drivers/of/overlay.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c
index c8e70ab00091..150b07daccaa 100644
--- a/drivers/of/overlay.c
+++ b/drivers/of/overlay.c
@@ -463,9 +463,6 @@ static int of_overlay_global_fixup_dir(struct device_node *root, const char *ovl
 	if (*ovl_dir == '/')
 		return of_overlay_apply_dir(root, ovl_dir, true);
 
-	if (*ovl_dir == '\0')
-		return 0;
-
 	dir = concat_path_file(of_overlay_basedir, ovl_dir);
 
 	ret = of_overlay_apply_dir(root, dir, true);
-- 
2.39.2

[PATCH 8/8] of: overlay: replace filename with an more unique name

[Marco Felsch]

Since we do support FIT image overlays and file/dir based overlays the
filename variable is not sufficient anylonger. Therefore rename the
local variables as well as the filter function to hook.

To not break any systems keep the old filename based name too but mark
them as deprecated.

Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
---
 drivers/of/overlay.c | 39 +++++++++++++++++++++------------------
 include/of.h         |  3 ++-
 2 files changed, 23 insertions(+), 19 deletions(-)

diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c
index 150b07daccaa..964b56db2337 100644
--- a/drivers/of/overlay.c
+++ b/drivers/of/overlay.c
@@ -233,12 +233,12 @@ static struct of_overlay_filter *of_overlay_find_filter(const char *name)
 	return NULL;
 }
 
-static bool of_overlay_matches_filter(const char *filename, struct device_node *ovl)
+static bool of_overlay_matches_filter(const char *pattern, struct device_node *ovl)
 {
 	struct of_overlay_filter *filter;
 	char *p, *path, *n;
 	bool apply = false;
-	bool have_filename_filter = false;
+	bool have_pattern_filter = false;
 	bool have_content_filter = false;
 
 	p = path = strdup(of_overlay_filter);
@@ -255,14 +255,16 @@ static bool of_overlay_matches_filter(const char *filename, struct device_node *
 			continue;
 		}
 
-		if (filter->filter_filename)
-			have_filename_filter = true;
+		if (filter->filter_pattern || filter->filter_filename)
+			have_pattern_filter = true;
 		if (filter->filter_content)
 			have_content_filter = true;
 
-		if (filename) {
-			if (filter->filter_filename &&
-			    filter->filter_filename(filter, kbasename(filename)))
+		if (pattern) {
+			if ((filter->filter_pattern &&
+			     filter->filter_pattern(filter, kbasename(pattern))) ||
+			    (filter->filter_filename &&
+			     filter->filter_filename(filter, kbasename(pattern))))
 				score++;
 		} else {
 			score++;
@@ -285,11 +287,11 @@ static bool of_overlay_matches_filter(const char *filename, struct device_node *
 	free(path);
 
 	/* No filter found at all, no match */
-	if (!have_filename_filter && !have_content_filter)
+	if (!have_pattern_filter && !have_content_filter)
 		return false;
 
-	/* Want to match filename, but we do not have a filename_filter */
-	if (filename && !have_filename_filter)
+	/* Want to match pattern, but we do not have a filename_filter */
+	if (pattern && !have_pattern_filter)
 		return true;
 
 	/* Want to match content, but we do not have a content_filter */
@@ -297,12 +299,12 @@ static bool of_overlay_matches_filter(const char *filename, struct device_node *
 		return true;
 
 	if (apply)
-		pr_debug("filename %s, overlay %p: match against filter %s\n",
-			 filename ?: "<NONE>",
+		pr_debug("pattern %s, overlay %p: match against filter %s\n",
+			 pattern ?: "<NONE>",
 			 ovl, filter->name);
 	else
-		pr_debug("filename %s, overlay %p: no match\n",
-			 filename ?: "<NONE>", ovl);
+		pr_debug("pattern %s, overlay %p: no match\n",
+			 pattern ?: "<NONE>", ovl);
 
 	return apply;
 }
@@ -576,14 +578,15 @@ static int of_overlay_global_fixup(struct device_node *root, void *data)
  * @filter: The new filter
  *
  * Register a new overlay filter. A filter can either match on
- * the filename or on the content of an overlay, but not on both.
+ * a pattern or on the content of an overlay, but not on both.
  * If that's desired two filters have to be registered.
  *
  * @return: 0 for success, negative error code otherwise
  */
 int of_overlay_register_filter(struct of_overlay_filter *filter)
 {
-	if (filter->filter_filename && filter->filter_content)
+	if ((filter->filter_pattern || filter->filter_filename) &&
+	    filter->filter_content)
 		return -EINVAL;
 
 	list_add_tail(&filter->list, &of_overlay_filters);
@@ -633,7 +636,7 @@ static bool of_overlay_filter_pattern(struct of_overlay_filter *f,
 
 static struct of_overlay_filter of_overlay_pattern_filter = {
 	.name = "pattern",
-	.filter_filename = of_overlay_filter_pattern,
+	.filter_pattern = of_overlay_filter_pattern,
 };
 
 static bool of_overlay_filter_filename(struct of_overlay_filter *f,
@@ -645,7 +648,7 @@ static bool of_overlay_filter_filename(struct of_overlay_filter *f,
 
 static struct of_overlay_filter of_overlay_filepattern_filter = {
 	.name = "filepattern",
-	.filter_filename = of_overlay_filter_filename,
+	.filter_pattern = of_overlay_filter_filename,
 };
 
 /**
diff --git a/include/of.h b/include/of.h
index 19b8e7c038a4..9aace82b32ea 100644
--- a/include/of.h
+++ b/include/of.h
@@ -1248,7 +1248,8 @@ static inline struct device_node *of_find_root_node(struct device_node *node)
 }
 
 struct of_overlay_filter {
-	bool (*filter_filename)(struct of_overlay_filter *, const char *filename);
+	bool (*filter_filename)(struct of_overlay_filter *, const char *filename); /* deprecated */
+	bool (*filter_pattern)(struct of_overlay_filter *, const char *pattern);
 	bool (*filter_content)(struct of_overlay_filter *, struct device_node *);
 	char *name;
 	struct list_head list;
-- 
2.39.2

Re: [PATCH 2/8] FIT: skip possible overlay config nodes

[Sascha Hauer]

Hi Marco,

On Fri, Mar 22, 2024 at 05:49:47PM +0100, Marco Felsch wrote:
> The FIT spec is not very specific when it comes to device-tree overlay
> handling.

By FIT spec you mean
https://github.com/u-boot/u-boot/blob/master/doc/usage/fit/overlay-fdt-boot.rst,
right, or is there more?

> Overlays can be added directely to an config node:
> 
> 	config-a {
> 		compatible = "machine-compatible";
> 		kernel = "kernel-img-name";
> 		fdt = "fdt-base-name", "fdt-overlay1-name", "...";
> 	}
> 
> or they are supplied via dedicated config nodes:
> 
> 	overlay-2 {
> 		fdt = "fdt-overlay2-name";
> 	}
> 
> Of course these config nodes can have compatibles as well:
> 
> 	overlay-3 {
> 		compatible = "machine-compatible";
> 		fdt = "fdt-overlay3-name";
> 	}

The text I referenced above doesn't mention compatible properties in
overlay config nodes.

> 
> The current fit_find_compatible_unit() code would skip the overlay node
> if the config-a compatible has the same score as the overlay-3
> compatible and if the overlay-3 config-node is listed after the config-a
> config-node. But if the compatible of config-a config-node has a lower
> score or the overlay-3 config-note is listed first (the spec does not
> specify any order) we end up in taking the overlay-3 config-node instead
> of config-a config-node.

You could distinguish overlay config nodes from full config nodes by the
presence of a "kernel" property. Overlay config nodes do not have it.

Sascha

-- 
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 6/8] of: overlay: add FIT overlay support

[Sascha Hauer]

On Fri, Mar 22, 2024 at 05:49:51PM +0100, Marco Felsch wrote:
> This adds the support to load devicetree overlays from an FIT image.
> There are quite a few options to handle FIT overlays since the FIT
> overlay spec is not very strict.
> 
> This patch implement the most configurable case where each overlay does
> have it's own config node (including the optional signature).
> 
> - The "name" filter check is performed on the config-node name (the node
>   under the configurations) and not the FIT overlay image name (the node
>   name under the images node).
> - The "content" filter check does not differ from the file based overlay
>   handling.
> 
> Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
> ---
>  drivers/of/overlay.c | 110 ++++++++++++++++++++++++++++++++++++++++---
>  1 file changed, 103 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c
> index e9fd5c0a1f7d..c8e70ab00091 100644
> --- a/drivers/of/overlay.c
> +++ b/drivers/of/overlay.c
> @@ -8,10 +8,12 @@
>   */
>  #define pr_fmt(fmt) "of_overlay: " fmt
>  
> +#include <bootm.h>
>  #include <common.h>
>  #include <of.h>
>  #include <errno.h>
>  #include <globalvar.h>
> +#include <image-fit.h>
>  #include <magicvar.h>
>  #include <string.h>
>  #include <libfile.h>
> @@ -473,9 +475,103 @@ static int of_overlay_global_fixup_dir(struct device_node *root, const char *ovl
>  	return ret;
>  }
>  
> +static int of_overlay_apply_fit(struct device_node *root, struct fit_handle *fit,
> +				struct device_node *config)
> +{
> +	const char *name = config->name;
> +	struct device_node *overlay;
> +	unsigned long ovl_sz;
> +	const void *ovl;
> +	int ret;
> +
> +	if (!fit_has_image(fit, config, "fdt"))
> +		return 0;
> +
> +	if (!of_overlay_matches_filter(name, NULL))
> +		return 0;
> +
> +	ret = fit_open_image(fit, config, "fdt", &ovl, &ovl_sz);
> +	if (ret)
> +		return ret;
> +
> +	overlay = of_unflatten_dtb(ovl, ovl_sz);
> +
> +	if (!of_overlay_matches_filter(NULL, overlay)) {
> +		ret = 0;
> +		goto out;
> +	}
> +
> +	ret = of_overlay_apply_tree(root, overlay);
> +	if (ret == -ENODEV)
> +		pr_debug("Not applied %s (not compatible)\n", name);
> +	else if (ret)
> +		pr_err("Cannot apply %s: %s\n", name, strerror(-ret));
> +	else
> +		pr_info("Applied %s\n", name);
> +
> +out:
> +	of_delete_node(overlay);
> +
> +	return ret;
> +}
> +
> +static int of_overlay_global_fixup_fit(struct device_node *root, const char *ovl_dev)
> +{
> +	enum bootm_verify verify = bootm_get_verify_mode();
> +	struct device_node *conf_node;
> +	struct fit_handle *fit;
> +	struct stat s;
> +	int ret;
> +
> +	if (!IS_ENABLED(CONFIG_FITIMAGE))
> +		return 0;
> +
> +	if (stat(of_overlay_path, &s))
> +		return -errno;

Why this? The caller already checked for existence of of_overlay_path.
Besides, it is not even used in this function.

> +
> +	fit = fit_open(ovl_dev, 0, verify, s.st_size);
> +	if (IS_ERR(fit)) {
> +		pr_err("Loading FIT image %s failed with: %pe\n", ovl_dev, fit);
> +		return PTR_ERR(fit);
> +	}

Are you anticipating taking only the overlays from a FIT image and the
kernel coming from somewhere else? Otherwise I would expect the
integration to happen in the bootm and FIT code where we already have a
handle to the opened FIT image. It seems wasteful to open the same FIT
image here again.

> +
> +	for_each_child_of_node(fit->configurations, conf_node) {
> +		if (!fit_config_is_overlay(conf_node))
> +			continue;
> +
> +		ret = fit_config_verify_signature(fit, conf_node);
> +		if (ret)
> +			goto out;
> +
> +		ret = of_overlay_apply_fit(root, fit, conf_node);
> +		if (ret)
> +			goto out;
> +	}
> +
> +out:
> +	fit_close(fit);
> +	return ret;
> +}
> +
>  static int of_overlay_global_fixup(struct device_node *root, void *data)
>  {
> -	return of_overlay_global_fixup_dir(root, of_overlay_path);
> +	struct stat s;
> +
> +	if (isempty(of_overlay_path))
> +		return 0;
> +
> +	if (stat(of_overlay_path, &s)) {
> +		pr_err("Failed to detect file status\n");
> +		return -errno;
> +	}
> +
> +	if (S_ISDIR(s.st_mode))
> +		return of_overlay_global_fixup_dir(root, of_overlay_path);
> +	else if (S_ISCHR(s.st_mode) || S_ISBLK(s.st_mode))
> +		return of_overlay_global_fixup_fit(root, of_overlay_path);

Why must the FIT image providing overlays be on a plain block device?
Shouldn't we allow FIT images to live in a filesystem?

Anyway, as said I think this is the wrong place to implement this. When
opening a FIT image it's already clear that we should take the overlays
from that image, and not open some image again.

Sascha

-- 
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 2/8] FIT: skip possible overlay config nodes

[Marco Felsch]

Hi,

sorry for the delay on this patchset.

On 24-03-25, Sascha Hauer wrote:
> Hi Marco,
> 
> On Fri, Mar 22, 2024 at 05:49:47PM +0100, Marco Felsch wrote:
> > The FIT spec is not very specific when it comes to device-tree overlay
> > handling.
> 
> By FIT spec you mean
> https://github.com/u-boot/u-boot/blob/master/doc/usage/fit/overlay-fdt-boot.rst,
> right, or is there more?

this is just an example which is not complete e.g. it misses the
signature node in case of verified boot. I used [1] as reference but
after reading it again I see that this reference list the kernel or
firmware properties as mandatory.

[1] https://docs.u-boot.org/en/latest/usage/fit/source_file_format.html#configurations-node

> > Overlays can be added directely to an config node:
> > 
> > 	config-a {
> > 		compatible = "machine-compatible";
> > 		kernel = "kernel-img-name";
> > 		fdt = "fdt-base-name", "fdt-overlay1-name", "...";
> > 	}
> > 
> > or they are supplied via dedicated config nodes:
> > 
> > 	overlay-2 {
> > 		fdt = "fdt-overlay2-name";
> > 	}
> > 
> > Of course these config nodes can have compatibles as well:
> > 
> > 	overlay-3 {
> > 		compatible = "machine-compatible";
> > 		fdt = "fdt-overlay3-name";
> > 	}
> 
> The text I referenced above doesn't mention compatible properties in
> overlay config nodes.

You're right, but the format description chapter [1] does.

> > The current fit_find_compatible_unit() code would skip the overlay node
> > if the config-a compatible has the same score as the overlay-3
> > compatible and if the overlay-3 config-node is listed after the config-a
> > config-node. But if the compatible of config-a config-node has a lower
> > score or the overlay-3 config-note is listed first (the spec does not
> > specify any order) we end up in taking the overlay-3 config-node instead
> > of config-a config-node.
> 
> You could distinguish overlay config nodes from full config nodes by the
> presence of a "kernel" property. Overlay config nodes do not have it.

Of course this could be done but I wanted to make it more explicit since
the FIT spec is not very clear when it comes to overlays. Instead of
adding an explicit image type like:

  - type = "flat_dt_overlay";

they used the already existing definitions. Therefore I went this way so
it is up to user to specify the overlay config nodes explicit.

Regards,
  Marco

> 
> Sascha
> 
> -- 
> 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 6/8] of: overlay: add FIT overlay support

[Marco Felsch]

On 24-03-25, Sascha Hauer wrote:
> On Fri, Mar 22, 2024 at 05:49:51PM +0100, Marco Felsch wrote:
> > This adds the support to load devicetree overlays from an FIT image.
> > There are quite a few options to handle FIT overlays since the FIT
> > overlay spec is not very strict.
> > 
> > This patch implement the most configurable case where each overlay does
> > have it's own config node (including the optional signature).
> > 
> > - The "name" filter check is performed on the config-node name (the node
> >   under the configurations) and not the FIT overlay image name (the node
> >   name under the images node).
> > - The "content" filter check does not differ from the file based overlay
> >   handling.
> > 
> > Signed-off-by: Marco Felsch <m.felsch@pengutronix.de>
> > ---
> >  drivers/of/overlay.c | 110 ++++++++++++++++++++++++++++++++++++++++---
> >  1 file changed, 103 insertions(+), 7 deletions(-)
> > 
> > diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c
> > index e9fd5c0a1f7d..c8e70ab00091 100644
> > --- a/drivers/of/overlay.c
> > +++ b/drivers/of/overlay.c
> > @@ -8,10 +8,12 @@
> >   */
> >  #define pr_fmt(fmt) "of_overlay: " fmt
> >  
> > +#include <bootm.h>
> >  #include <common.h>
> >  #include <of.h>
> >  #include <errno.h>
> >  #include <globalvar.h>
> > +#include <image-fit.h>
> >  #include <magicvar.h>
> >  #include <string.h>
> >  #include <libfile.h>
> > @@ -473,9 +475,103 @@ static int of_overlay_global_fixup_dir(struct device_node *root, const char *ovl
> >  	return ret;
> >  }
> >  
> > +static int of_overlay_apply_fit(struct device_node *root, struct fit_handle *fit,
> > +				struct device_node *config)
> > +{
> > +	const char *name = config->name;
> > +	struct device_node *overlay;
> > +	unsigned long ovl_sz;
> > +	const void *ovl;
> > +	int ret;
> > +
> > +	if (!fit_has_image(fit, config, "fdt"))
> > +		return 0;
> > +
> > +	if (!of_overlay_matches_filter(name, NULL))
> > +		return 0;
> > +
> > +	ret = fit_open_image(fit, config, "fdt", &ovl, &ovl_sz);
> > +	if (ret)
> > +		return ret;
> > +
> > +	overlay = of_unflatten_dtb(ovl, ovl_sz);
> > +
> > +	if (!of_overlay_matches_filter(NULL, overlay)) {
> > +		ret = 0;
> > +		goto out;
> > +	}
> > +
> > +	ret = of_overlay_apply_tree(root, overlay);
> > +	if (ret == -ENODEV)
> > +		pr_debug("Not applied %s (not compatible)\n", name);
> > +	else if (ret)
> > +		pr_err("Cannot apply %s: %s\n", name, strerror(-ret));
> > +	else
> > +		pr_info("Applied %s\n", name);
> > +
> > +out:
> > +	of_delete_node(overlay);
> > +
> > +	return ret;
> > +}
> > +
> > +static int of_overlay_global_fixup_fit(struct device_node *root, const char *ovl_dev)
> > +{
> > +	enum bootm_verify verify = bootm_get_verify_mode();
> > +	struct device_node *conf_node;
> > +	struct fit_handle *fit;
> > +	struct stat s;
> > +	int ret;
> > +
> > +	if (!IS_ENABLED(CONFIG_FITIMAGE))
> > +		return 0;
> > +
> > +	if (stat(of_overlay_path, &s))
> > +		return -errno;
> 
> Why this? The caller already checked for existence of of_overlay_path.
> Besides, it is not even used in this function.

Yes, good point. I wanted to make the of_overlay_global_fixup_{dir,fit}
APIs symmetrical but I think we can drop this.

> > +
> > +	fit = fit_open(ovl_dev, 0, verify, s.st_size);
> > +	if (IS_ERR(fit)) {
> > +		pr_err("Loading FIT image %s failed with: %pe\n", ovl_dev, fit);
> > +		return PTR_ERR(fit);
> > +	}
> 
> Are you anticipating taking only the overlays from a FIT image and the
> kernel coming from somewhere else? Otherwise I would expect the
> integration to happen in the bootm and FIT code where we already have a
> handle to the opened FIT image. It seems wasteful to open the same FIT
> image here again.

I thought about this too but didn't went this way since it would not
allow us to patch the barebox-dt with overlays coming from the fit nor
is it possible to run the of_overlay command. It shouldn't matter to the
end-user if the overlay is coming from the (root)fs or the fit-image.

> > +	for_each_child_of_node(fit->configurations, conf_node) {
> > +		if (!fit_config_is_overlay(conf_node))
> > +			continue;
> > +
> > +		ret = fit_config_verify_signature(fit, conf_node);
> > +		if (ret)
> > +			goto out;
> > +
> > +		ret = of_overlay_apply_fit(root, fit, conf_node);
> > +		if (ret)
> > +			goto out;
> > +	}
> > +
> > +out:
> > +	fit_close(fit);
> > +	return ret;
> > +}
> > +
> >  static int of_overlay_global_fixup(struct device_node *root, void *data)
> >  {
> > -	return of_overlay_global_fixup_dir(root, of_overlay_path);
> > +	struct stat s;
> > +
> > +	if (isempty(of_overlay_path))
> > +		return 0;
> > +
> > +	if (stat(of_overlay_path, &s)) {
> > +		pr_err("Failed to detect file status\n");
> > +		return -errno;
> > +	}
> > +
> > +	if (S_ISDIR(s.st_mode))
> > +		return of_overlay_global_fixup_dir(root, of_overlay_path);
> > +	else if (S_ISCHR(s.st_mode) || S_ISBLK(s.st_mode))
> > +		return of_overlay_global_fixup_fit(root, of_overlay_path);
> 
> Why must the FIT image providing overlays be on a plain block device?
> Shouldn't we allow FIT images to live in a filesystem?

Good point, I didn't considered this since fit-images are mostly used on
secure/verified-boot devices. These devices often have a plain fit-image
partition.

> Anyway, as said I think this is the wrong place to implement this. When
> opening a FIT image it's already clear that we should take the overlays
> from that image, and not open some image again.

Please consider the use-cases listed above as well. If we strictly bind
it to the bootm command we can't do the live-patching of barebox dtbs
and IMHO we shouldn't care if the overlay is coming from the FIT or an
FS.

Regards,
  Marco

Re: [PATCH 2/8] FIT: skip possible overlay config nodes

[Sascha Hauer]

On Tue, Jun 11, 2024 at 10:36:47AM +0200, Marco Felsch wrote:
> Hi,
> 
> sorry for the delay on this patchset.
> 
> On 24-03-25, Sascha Hauer wrote:
> > Hi Marco,
> > 
> > On Fri, Mar 22, 2024 at 05:49:47PM +0100, Marco Felsch wrote:
> > > The FIT spec is not very specific when it comes to device-tree overlay
> > > handling.
> > 
> > By FIT spec you mean
> > https://github.com/u-boot/u-boot/blob/master/doc/usage/fit/overlay-fdt-boot.rst,
> > right, or is there more?
> 
> this is just an example which is not complete e.g. it misses the
> signature node in case of verified boot. I used [1] as reference but
> after reading it again I see that this reference list the kernel or
> firmware properties as mandatory.
> 
> [1] https://docs.u-boot.org/en/latest/usage/fit/source_file_format.html#configurations-node
> 
> > > Overlays can be added directely to an config node:
> > > 
> > > 	config-a {
> > > 		compatible = "machine-compatible";
> > > 		kernel = "kernel-img-name";
> > > 		fdt = "fdt-base-name", "fdt-overlay1-name", "...";
> > > 	}
> > > 
> > > or they are supplied via dedicated config nodes:
> > > 
> > > 	overlay-2 {
> > > 		fdt = "fdt-overlay2-name";
> > > 	}
> > > 
> > > Of course these config nodes can have compatibles as well:
> > > 
> > > 	overlay-3 {
> > > 		compatible = "machine-compatible";
> > > 		fdt = "fdt-overlay3-name";
> > > 	}
> > 
> > The text I referenced above doesn't mention compatible properties in
> > overlay config nodes.
> 
> You're right, but the format description chapter [1] does.
> 
> > > The current fit_find_compatible_unit() code would skip the overlay node
> > > if the config-a compatible has the same score as the overlay-3
> > > compatible and if the overlay-3 config-node is listed after the config-a
> > > config-node. But if the compatible of config-a config-node has a lower
> > > score or the overlay-3 config-note is listed first (the spec does not
> > > specify any order) we end up in taking the overlay-3 config-node instead
> > > of config-a config-node.
> > 
> > You could distinguish overlay config nodes from full config nodes by the
> > presence of a "kernel" property. Overlay config nodes do not have it.
> 
> Of course this could be done but I wanted to make it more explicit since
> the FIT spec is not very clear when it comes to overlays. Instead of
> adding an explicit image type like:
> 
>   - type = "flat_dt_overlay";
> 
> they used the already existing definitions. Therefore I went this way so
> it is up to user to specify the overlay config nodes explicit.

I don't buy this argumentation. A node that doesn't have a 'kernel'
property can not be booted, hence you can ignore it when looking for a
bootable config. A node that does have a 'kernel' property by
definition doesn't describe an overlay.

I agree that it's unfortunate that a overlay config node can only be
indirectly detected as such. Nevertheless I don't see a need for
adding another globalvar for that.

Sascha

-- 
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 2/8] FIT: skip possible overlay config nodes

[Marco Felsch]

On 24-06-17, Sascha Hauer wrote:
> On Tue, Jun 11, 2024 at 10:36:47AM +0200, Marco Felsch wrote:
> > Hi,
> > 
> > sorry for the delay on this patchset.
> > 
> > On 24-03-25, Sascha Hauer wrote:
> > > Hi Marco,
> > > 
> > > On Fri, Mar 22, 2024 at 05:49:47PM +0100, Marco Felsch wrote:
> > > > The FIT spec is not very specific when it comes to device-tree overlay
> > > > handling.
> > > 
> > > By FIT spec you mean
> > > https://github.com/u-boot/u-boot/blob/master/doc/usage/fit/overlay-fdt-boot.rst,
> > > right, or is there more?
> > 
> > this is just an example which is not complete e.g. it misses the
> > signature node in case of verified boot. I used [1] as reference but
> > after reading it again I see that this reference list the kernel or
> > firmware properties as mandatory.
> > 
> > [1] https://docs.u-boot.org/en/latest/usage/fit/source_file_format.html#configurations-node
> > 
> > > > Overlays can be added directely to an config node:
> > > > 
> > > > 	config-a {
> > > > 		compatible = "machine-compatible";
> > > > 		kernel = "kernel-img-name";
> > > > 		fdt = "fdt-base-name", "fdt-overlay1-name", "...";
> > > > 	}
> > > > 
> > > > or they are supplied via dedicated config nodes:
> > > > 
> > > > 	overlay-2 {
> > > > 		fdt = "fdt-overlay2-name";
> > > > 	}
> > > > 
> > > > Of course these config nodes can have compatibles as well:
> > > > 
> > > > 	overlay-3 {
> > > > 		compatible = "machine-compatible";
> > > > 		fdt = "fdt-overlay3-name";
> > > > 	}
> > > 
> > > The text I referenced above doesn't mention compatible properties in
> > > overlay config nodes.
> > 
> > You're right, but the format description chapter [1] does.
> > 
> > > > The current fit_find_compatible_unit() code would skip the overlay node
> > > > if the config-a compatible has the same score as the overlay-3
> > > > compatible and if the overlay-3 config-node is listed after the config-a
> > > > config-node. But if the compatible of config-a config-node has a lower
> > > > score or the overlay-3 config-note is listed first (the spec does not
> > > > specify any order) we end up in taking the overlay-3 config-node instead
> > > > of config-a config-node.
> > > 
> > > You could distinguish overlay config nodes from full config nodes by the
> > > presence of a "kernel" property. Overlay config nodes do not have it.
> > 
> > Of course this could be done but I wanted to make it more explicit since
> > the FIT spec is not very clear when it comes to overlays. Instead of
> > adding an explicit image type like:
> > 
> >   - type = "flat_dt_overlay";
> > 
> > they used the already existing definitions. Therefore I went this way so
> > it is up to user to specify the overlay config nodes explicit.
> 
> I don't buy this argumentation. A node that doesn't have a 'kernel'
> property can not be booted, hence you can ignore it when looking for a
> bootable config. A node that does have a 'kernel' property by
> definition doesn't describe an overlay.

As written in the commit message the overlays can be applied in two
ways:

| 	config-a {
| 		compatible = "machine-compatible";
| 		kernel = "kernel-img-name";
| 		fdt = "fdt-base-name", "fdt-overlay1-name", "...";
| 	}

or via:

| 	overlay-2 {
| 		fdt = "fdt-overlay2-name";
| 	}

albeit this patchset targets the latter. This helper is used later as
well to detect if the the config node is an overlay not just to detect
if it is bootable or not. Right now there are also config nodes which
don't have the kernel property included but also aren't overlays e.g.
(doc/usage/fit/sec_firmware_ppa.rst):

|        configurations {
|            default = "config-1";
|            config-1 {
|                description = "PPA Secure firmware";
|                firmware = "firmware@1";
|                loadables = "trustedOS@1", "fuse_scr";
|            };
|        };

and I assume that more such config nodes will be added over time.

> I agree that it's unfortunate that a overlay config node can only be
> indirectly detected as such. Nevertheless I don't see a need for
> adding another globalvar for that.

Of course I can convert the check to drop specifying the overlay-config
node explicit and instead add an implicit check but my concerns are that
this is not as robust as the explicit mechanism and over time we end up
in false-positives while detecting overlays.

Regards,
  Marco

Re: [PATCH 2/8] FIT: skip possible overlay config nodes

[Sascha Hauer]

On Wed, Jun 26, 2024 at 12:04:42PM +0200, Marco Felsch wrote:
> On 24-06-17, Sascha Hauer wrote:
> > On Tue, Jun 11, 2024 at 10:36:47AM +0200, Marco Felsch wrote:
> > > Hi,
> > > 
> > > sorry for the delay on this patchset.
> > > 
> > > On 24-03-25, Sascha Hauer wrote:
> > > > Hi Marco,
> > > > 
> > > > On Fri, Mar 22, 2024 at 05:49:47PM +0100, Marco Felsch wrote:
> > > > > The FIT spec is not very specific when it comes to device-tree overlay
> > > > > handling.
> > > > 
> > > > By FIT spec you mean
> > > > https://github.com/u-boot/u-boot/blob/master/doc/usage/fit/overlay-fdt-boot.rst,
> > > > right, or is there more?
> > > 
> > > this is just an example which is not complete e.g. it misses the
> > > signature node in case of verified boot. I used [1] as reference but
> > > after reading it again I see that this reference list the kernel or
> > > firmware properties as mandatory.
> > > 
> > > [1] https://docs.u-boot.org/en/latest/usage/fit/source_file_format.html#configurations-node
> > > 
> > > > > Overlays can be added directely to an config node:
> > > > > 
> > > > > 	config-a {
> > > > > 		compatible = "machine-compatible";
> > > > > 		kernel = "kernel-img-name";
> > > > > 		fdt = "fdt-base-name", "fdt-overlay1-name", "...";
> > > > > 	}
> > > > > 
> > > > > or they are supplied via dedicated config nodes:
> > > > > 
> > > > > 	overlay-2 {
> > > > > 		fdt = "fdt-overlay2-name";
> > > > > 	}
> > > > > 
> > > > > Of course these config nodes can have compatibles as well:
> > > > > 
> > > > > 	overlay-3 {
> > > > > 		compatible = "machine-compatible";
> > > > > 		fdt = "fdt-overlay3-name";
> > > > > 	}
> > > > 
> > > > The text I referenced above doesn't mention compatible properties in
> > > > overlay config nodes.
> > > 
> > > You're right, but the format description chapter [1] does.
> > > 
> > > > > The current fit_find_compatible_unit() code would skip the overlay node
> > > > > if the config-a compatible has the same score as the overlay-3
> > > > > compatible and if the overlay-3 config-node is listed after the config-a
> > > > > config-node. But if the compatible of config-a config-node has a lower
> > > > > score or the overlay-3 config-note is listed first (the spec does not
> > > > > specify any order) we end up in taking the overlay-3 config-node instead
> > > > > of config-a config-node.
> > > > 
> > > > You could distinguish overlay config nodes from full config nodes by the
> > > > presence of a "kernel" property. Overlay config nodes do not have it.
> > > 
> > > Of course this could be done but I wanted to make it more explicit since
> > > the FIT spec is not very clear when it comes to overlays. Instead of
> > > adding an explicit image type like:
> > > 
> > >   - type = "flat_dt_overlay";
> > > 
> > > they used the already existing definitions. Therefore I went this way so
> > > it is up to user to specify the overlay config nodes explicit.
> > 
> > I don't buy this argumentation. A node that doesn't have a 'kernel'
> > property can not be booted, hence you can ignore it when looking for a
> > bootable config. A node that does have a 'kernel' property by
> > definition doesn't describe an overlay.
> 
> As written in the commit message the overlays can be applied in two
> ways:
> 
> | 	config-a {
> | 		compatible = "machine-compatible";
> | 		kernel = "kernel-img-name";
> | 		fdt = "fdt-base-name", "fdt-overlay1-name", "...";
> | 	}
> 
> or via:
> 
> | 	overlay-2 {
> | 		fdt = "fdt-overlay2-name";
> | 	}
> 
> albeit this patchset targets the latter. This helper is used later as
> well to detect if the the config node is an overlay not just to detect
> if it is bootable or not. Right now there are also config nodes which
> don't have the kernel property included but also aren't overlays e.g.
> (doc/usage/fit/sec_firmware_ppa.rst):
> 
> |        configurations {
> |            default = "config-1";
> |            config-1 {
> |                description = "PPA Secure firmware";
> |                firmware = "firmware@1";
> |                loadables = "trustedOS@1", "fuse_scr";
> |            };
> |        };
> 
> and I assume that more such config nodes will be added over time.

Then let's put it differently:

Currently the bootm code calls fit_open_configuration() to get the
configuration node for a kernel. From the nodes above only one qualifies
being a kernel configuration, namely the one with the "kernel" property.

So instead of ignoring all nodes that are overlays you could instead
ignore all nodes that are not kernels.

Doing so would limit fit_open_configuration() to only kernel
configurations. This would break arch/arm/mach-layerscape/ppa.c
which calls fit_open_configuration() to find exactly the configuration
you are referring to above. So I think fit_open_configuration() needs
some way to let the user specify which configuration we are looking for,
maybe by adding some match() function pointer argument.

Sascha

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