Merge patch series "boot/fit: use fdt_for_each_subnode() in image-fit.c"

Aristo Chen <[email protected]> says:

This series ends with replacing the verbose fdt_next_node() + ndepth
idiom in boot/image-fit.c with fdt_for_each_subnode(), bringing the
file in line with boot/image-fit-sig.c. Six of the seven sites in
image-fit.c predate the macro by 2-6 years; the seventh was
copy-pasted from a neighbour in 2015 just after the macro landed.
The old idiom is legacy, not a deliberate technical choice.

Converting straight to the macro turned out to need a prerequisite,
which is patch 1. fit_print_contents() reads the default-config
property using the loop variable left over after iterating /images
children. With /images defined first in the source (the conventional
layout) libfdt's walker happens to leave that variable pointing at
/configurations and the read works. With /configurations defined
first the read returns NULL and the "Default Configuration" line is
silently omitted. fdt_for_each_subnode()'s post-loop value is
unconditionally a negative error code, so a naive conversion would
have made the missing line the unconditional behaviour. Patch 1
reads the property from confs_noffset directly and removes the
layout dependency.

Patch 2 adds a regression test for the configs-before-images
layout, which had no coverage.

Patch 3 is the mechanical conversion at all seven sites,
equivalence-preserving as described in the per-patch message.

Link: https://lore.kernel.org/r/[email protected]

2 files changed, 81 insertions, 81 deletions

diff --git a/boot/image-fit.c b/boot/image-fit.c
index b0fcaf6e17f..884a5105df5 100644
--- a/boot/image-fit.c
+++ b/boot/image-fit.c
@@ -156,18 +156,10 @@ static void fit_get_debug(const void *fit, int noffset,
 int fit_get_subimage_count(const void *fit, int images_noffset)
 {
 	int noffset;
-	int ndepth;
 	int count = 0;
 
-	/* Process its subnodes, print out component images details */
-	for (ndepth = 0, count = 0,
-		noffset = fdt_next_node(fit, images_noffset, &ndepth);
-	     (noffset >= 0) && (ndepth > 0);
-	     noffset = fdt_next_node(fit, noffset, &ndepth)) {
-		if (ndepth == 1) {
-			count++;
-		}
-	}
+	fdt_for_each_subnode(noffset, fit, images_noffset)
+		count++;
 
 	return count;
 }
@@ -291,7 +283,7 @@ static void fit_conf_print(const void *fit, int noffset, const char *p)
 	const char *uname;
 	int ret;
 	int fdt_index, loadables_index;
-	int ndepth;
+	int sub_noffset;
 
 	/* Mandatory properties */
 	ret = fit_get_desc(fit, noffset, &desc);
@@ -357,14 +349,8 @@ static void fit_conf_print(const void *fit, int noffset, const char *p)
 	}
 
 	/* Process all hash subnodes of the component configuration node */
-	for (ndepth = 0, noffset = fdt_next_node(fit, noffset, &ndepth);
-	     (noffset >= 0) && (ndepth > 0);
-	     noffset = fdt_next_node(fit, noffset, &ndepth)) {
-		if (ndepth == 1) {
-			/* Direct child node of the component configuration node */
-			fit_image_print_verification_data(fit, noffset, p);
-		}
-	}
+	fdt_for_each_subnode(sub_noffset, fit, noffset)
+		fit_image_print_verification_data(fit, sub_noffset, p);
 }
 
 /**
@@ -386,8 +372,7 @@ void fit_print_contents(const void *fit)
 	int images_noffset;
 	int confs_noffset;
 	int noffset;
-	int ndepth;
-	int count = 0;
+	int count;
 	int ret;
 	const char *p;
 	time_t timestamp;
@@ -424,20 +409,12 @@ void fit_print_contents(const void *fit)
 	}
 
 	/* Process its subnodes, print out component images details */
-	for (ndepth = 0, count = 0,
-		noffset = fdt_next_node(fit, images_noffset, &ndepth);
-	     (noffset >= 0) && (ndepth > 0);
-	     noffset = fdt_next_node(fit, noffset, &ndepth)) {
-		if (ndepth == 1) {
-			/*
-			 * Direct child node of the images parent node,
-			 * i.e. component image node.
-			 */
-			printf("%s Image %u (%s)\n", p, count++,
-			       fit_get_name(fit, noffset, NULL));
+	count = 0;
+	fdt_for_each_subnode(noffset, fit, images_noffset) {
+		printf("%s Image %u (%s)\n", p, count++,
+		       fit_get_name(fit, noffset, NULL));
 
-			fit_image_print(fit, noffset, p);
-		}
+		fit_image_print(fit, noffset, p);
 	}
 
 	/* Find configurations parent node offset */
@@ -449,25 +426,17 @@ void fit_print_contents(const void *fit)
 	}
 
 	/* get default configuration unit name from default property */
-	uname = (char *)fdt_getprop(fit, noffset, FIT_DEFAULT_PROP, NULL);
+	uname = (char *)fdt_getprop(fit, confs_noffset, FIT_DEFAULT_PROP, NULL);
 	if (uname)
 		printf("%s Default Configuration: '%s'\n", p, uname);
 
 	/* Process its subnodes, print out configurations details */
-	for (ndepth = 0, count = 0,
-		noffset = fdt_next_node(fit, confs_noffset, &ndepth);
-	     (noffset >= 0) && (ndepth > 0);
-	     noffset = fdt_next_node(fit, noffset, &ndepth)) {
-		if (ndepth == 1) {
-			/*
-			 * Direct child node of the configurations parent node,
-			 * i.e. configuration node.
-			 */
-			printf("%s Configuration %u (%s)\n", p, count++,
-			       fit_get_name(fit, noffset, NULL));
+	count = 0;
+	fdt_for_each_subnode(noffset, fit, confs_noffset) {
+		printf("%s Configuration %u (%s)\n", p, count++,
+		       fit_get_name(fit, noffset, NULL));
 
-			fit_conf_print(fit, noffset, p);
-		}
+		fit_conf_print(fit, noffset, p);
 	}
 }
 
@@ -494,7 +463,6 @@ void fit_image_print(const void *fit, int image_noffset, const char *p)
 	ulong load, entry;
 	const void *data;
 	int noffset;
-	int ndepth;
 	int ret;
 
 	if (!CONFIG_IS_ENABLED(FIT_PRINT))
@@ -584,14 +552,8 @@ void fit_image_print(const void *fit, int image_noffset, const char *p)
 	}
 
 	/* Process all hash subnodes of the component image node */
-	for (ndepth = 0, noffset = fdt_next_node(fit, image_noffset, &ndepth);
-	     (noffset >= 0) && (ndepth > 0);
-	     noffset = fdt_next_node(fit, noffset, &ndepth)) {
-		if (ndepth == 1) {
-			/* Direct child node of the component image node */
-			fit_image_print_verification_data(fit, noffset, p);
-		}
-	}
+	fdt_for_each_subnode(noffset, fit, image_noffset)
+		fit_image_print_verification_data(fit, noffset, p);
 }
 
 /**
@@ -1477,7 +1439,6 @@ int fit_all_image_verify(const void *fit)
 {
 	int images_noffset;
 	int noffset;
-	int ndepth;
 	int count;
 
 	/* Find images parent node offset */
@@ -1491,23 +1452,15 @@ int fit_all_image_verify(const void *fit)
 	/* Process all image subnodes, check hashes for each */
 	printf("## Checking hash(es) for FIT Image at %08lx ...\n",
 	       (ulong)fit);
-	for (ndepth = 0, count = 0,
-	     noffset = fdt_next_node(fit, images_noffset, &ndepth);
-			(noffset >= 0) && (ndepth > 0);
-			noffset = fdt_next_node(fit, noffset, &ndepth)) {
-		if (ndepth == 1) {
-			/*
-			 * Direct child node of the images parent node,
-			 * i.e. component image node.
-			 */
-			printf("   Hash(es) for Image %u (%s): ", count,
-			       fit_get_name(fit, noffset, NULL));
-			count++;
+	count = 0;
+	fdt_for_each_subnode(noffset, fit, images_noffset) {
+		printf("   Hash(es) for Image %u (%s): ", count,
+		       fit_get_name(fit, noffset, NULL));
+		count++;
 
-			if (!fit_image_verify(fit, noffset))
-				return 0;
-			printf("\n");
-		}
+		if (!fit_image_verify(fit, noffset))
+			return 0;
+		printf("\n");
 	}
 	return 1;
 }
@@ -1734,7 +1687,6 @@ int fit_check_format(const void *fit, ulong size)
 
 int fit_conf_find_compat(const void *fit, const void *fdt)
 {
-	int ndepth = 0;
 	int noffset, confs_noffset, images_noffset;
 	const void *fdt_compat;
 	int fdt_compat_len;
@@ -1757,9 +1709,7 @@ int fit_conf_find_compat(const void *fit, const void *fdt)
 	/*
 	 * Loop over the configurations in the FIT image.
 	 */
-	for (noffset = fdt_next_node(fit, confs_noffset, &ndepth);
-			(noffset >= 0) && (ndepth > 0);
-			noffset = fdt_next_node(fit, noffset, &ndepth)) {
+	fdt_for_each_subnode(noffset, fit, confs_noffset) {
 		const void *fdt;
 		const char *kfdt_name;
 		int kfdt_noffset, compat_noffset;
@@ -1768,9 +1718,6 @@ int fit_conf_find_compat(const void *fit, const void *fdt)
 		size_t sz;
 		int i;
 
-		if (ndepth > 1)
-			continue;
-
 		/* If there's a compat property in the config node, use that. */
 		if (fdt_getprop(fit, noffset, FIT_COMPAT_PROP, NULL)) {
 			fdt = fit;		  /* search in FIT image */
diff --git a/test/py/tests/test_fit.py b/test/py/tests/test_fit.py
index 4f56a1421e1..1c4bd667d6b 100755
--- a/test/py/tests/test_fit.py
+++ b/test/py/tests/test_fit.py
@@ -426,3 +426,56 @@ class TestFitImage:
 
         output = ubman.run_command_list(cmds)
         assert "can't get kernel image!" in '\n'.join(output)
+
+    def test_fit_iminfo_configs_first(self, ubman, fsetup):
+        """Regression: iminfo prints "Default Configuration" even when
+        /configurations is defined before /images in the source.
+
+        fit_print_contents() in boot/image-fit.c used to read the default
+        configuration name from whatever offset libfdt happened to return
+        after iterating /images children. With /images defined first that
+        offset accidentally landed on /configurations; with /configurations
+        defined first the read returned NULL and the line silently went
+        missing. Fixed in commit "boot/fit: read default-config property
+        from the configurations node".
+        """
+        configs_first_its = '''
+/dts-v1/;
+
+/ {
+        description = "FIT with /configurations before /images";
+        #address-cells = <1>;
+
+        configurations {
+                default = "conf-1";
+                conf-1 {
+                        description = "first config";
+                        kernel = "kernel-1";
+                };
+        };
+
+        images {
+                kernel-1 {
+                        description = "first image";
+                        data = /incbin/("%(kernel)s");
+                        type = "kernel";
+                        arch = "sandbox";
+                        os = "linux";
+                        compression = "none";
+                        load = <0x40000>;
+                        entry = <0x40000>;
+                };
+        };
+};
+'''
+        fit = fit_util.make_fit(ubman, fsetup['mkimage'], configs_first_its,
+                                fsetup, basename='configs-first.fit')
+        cmds = [
+            'host load hostfs 0 %#x %s' % (fsetup['fit_addr'], fit),
+            'iminfo %#x' % fsetup['fit_addr'],
+        ]
+        output = '\n'.join(ubman.run_command_list(cmds))
+        assert "Default Configuration: 'conf-1'" in output, (
+            'iminfo output is missing the "Default Configuration" line for a '
+            'FIT whose /configurations node precedes /images. Output was:\n'
+            + output)