Re: [PATCH v3 08/15] flattree: Handle unknown tags

On Tue, Sep 15, 2026 at 12:16:35PM +0200, Herve Codina wrote:

Right, that's exactly what I'm suggesting.

Related to a helper, I have introduced one in the addon series where new tags
are present and these new tags are no more "unknown" tags and flat_read_subbuf()
has been introduced to parse them. You can see that in the patch 11/74 [0] or
directly in the final code [1]

[0] https://lore.kernel.org/devicetree-compiler/20260826094950.1088288-12-herve.codina@bootlin.com/ (local)
[1] https://github.com/bootlin/dtc/blob/c68038e0ff4cde5de37a21419df8a082032ef994/flattree.c#L1169

I can see to avoid some more code duplication between functions skipping "unknown" tags
and function parsing new "known" tags.

Uh.. I don't quite see the relevance of that here.  I'm just
suggesting the length calculation alone be a helper function.

quoted

quoted

+
+		/* Skip the following length bytes */
+		flat_read_chunk(inb, NULL, lng);
+
+		flat_realign(inb, sizeof(uint32_t));
+		break;
+	}
+
+	return true;
+}
+
+static uint32_t flat_read_tag(struct inbuf *inb)
+{
+	uint32_t tag;
+
+	do {
+		tag = flat_read_word(inb);
+		switch (tag) {
+		case FDT_BEGIN_NODE:
+		case FDT_END_NODE:
+		case FDT_PROP:
+		case FDT_NOP:
+		case FDT_END:
+			return tag;
+		default:
+			break;
+		}
+	} while (flat_skip_unknown_tag(inb, tag));  

Having this as a separate function seems odd to me...

Well, this clearly decouples "known" tags from "unknown" tags and keeps the
function small.

quoted

quoted

+	die("Cannot skip unknown tag 0x%08x\n", tag);
+}
+
 static const char *flat_read_string(struct inbuf *inb)
 {
 	int len = 0;

@@ -750,7 +806,7 @@ static struct node *unflatten_tree(struct inbuf *dtbuf,
 		struct property *prop;
 		struct node *child;
 
-		val = flat_read_word(dtbuf);
+		val = flat_read_tag(dtbuf);
 		switch (val) {  

.. rather than having handling unknown tags as part of the default:
case here.

Here and probably on some other part if we go in that direction.

Here you have already parsed a FDT_BEGIN_NODE to call unflatten_tree().

Unknown tags should be handle and skipped if possible at lower level to handle
them everywhere and without code duplication.

flat_read_tag() is this lower level.

quoted

quoted

 		case FDT_PROP:
 			if (node->children)

@@ -905,14 +961,13 @@ struct dt_info *dt_from_blob(const char *fname)
 
 	reservelist = flat_read_mem_reserve(&memresvbuf);
 
-	val = flat_read_word(&dtbuf);
-
+	val = flat_read_tag(&dtbuf);
 	if (val != FDT_BEGIN_NODE)
 		die("Device tree blob doesn't begin with FDT_BEGIN_NODE (begins with 0x%08x)\n", val);  

Hmm.. doesn't this already need to be fixed to handle NOP tags before
the root node?  Logically that change would go before this one.

Oh yes, good catch. I missed that one.

Will be update in next iteration (in offset 0 vs real root node offset part)
with 2 points:
  - handle the case here with something like
    --- 8< ---
       /* Skip possible FDT_NOP available before the root node */
       do {
	  val = flat_read_tag(&dtbuf);
       } while (tag == FDT_NOP);

       if (val != FDT_BEGIN_NODE)
	  die("Device tree blob doesn't begin with FDT_BEGIN_NODE (begins with 0x%08x)\n", val);
       ...
    --- 8< ---

  - Add a test calling dtc with a "nopulated" dtb
    This test is really missing. Only functions from libfdt are tested with
    a nopulated dtb. DTC has to be tested too.

Sounds good.

quoted

quoted

 
 	tree = unflatten_tree(&dtbuf, &strbuf, "", flags);
 
-	val = flat_read_word(&dtbuf);
+	val = flat_read_tag(&dtbuf);
 	if (val != FDT_END)
 		die("Device tree blob doesn't end with FDT_END\n");  

Likewise here for that matter, a NOP should be valid between the last
FDT_END_NODE and the FDT_END.

Yes, exactly and this will be taken into account in the next iteration.

Great.

quoted

quoted

diff --git a/tests/run_tests.sh b/tests/run_tests.sh
index f3647e63..8fc23cb7 100755
--- a/tests/run_tests.sh
+++ b/tests/run_tests.sh

@@ -882,6 +882,11 @@ dtc_tests () {
 
     # Tests for overlay/plugin generation
     dtc_overlay_tests
+
+    # Tests with "unknown tags"
+    run_dtc_test -I dtb -O dts -o unknown_tags_can_skip.dtb.dts unknown_tags_can_skip.dtb
+    base_run_test check_diff unknown_tags_can_skip.dtb.dts "$SRCDIR/unknown_tags_can_skip.dtb.dts.expect"  

It's best to avoid tests based on -O dts output unless we're
explicitly checking -O dts behaviour: because there are multiple ways
to format property values, the exact output isn't really guaranteed.

But at a give version dtc and a given dtb file, there is only one way
to generate a dts.

Yes, but if we tweak our -Odts formatting decisions, we don't want to
have to churn tests that aren't specifically related to -Odts.

If it change because of some modification in dtc, having some changes in
tests expected value should not be a big deal.

It's not a huge deal, but it's still preferable to avoid.

quoted

What I'd suggest instead is to adjust treegen to generate two dtbs
that are identical _except_ for the skippable tag.  Then you can use
dtc -I dtb -O dtb, and compare the dtc output (which should strip the
tag) against the dtb which was constructed without it in the first
place.

Or, rather than explicitly creating two new trees, you could make your
skippable tag example identical to test_tree1, except for the
additional tag, and re-use one of the other instances of test_tree1 as
the "tagless" version.

Why not just one dtb generated to treegen with unknown tags (already available
unknown_tags_can_skip.dtb)

dtc -I dtb -O dtb -o unknown_tags_can_skip.dtb.dtb unknown_tags_can_skip.dtb

And then
	base_run_test wrap_fdtdump unknown_tags_can_skip.dtb.dtb unknown_tags_can_skip.dtb.dtb.out
    	# Remove unneeded comments
	sed -i '/^\/\/ [^U]/d' unknown_tags_can_skip.dtb.out
    	base_run_test check_diff unknown_tags_can_skip.dtb.dtb.out "$SRCDIR/unknown_tags_can_skip.dtb.expect"

I don't like it - the output formatting of fdtdump is even less
guaranteed than -Odts.

This avoid the need for 2 dtbs generated by treegen and also avoid to compare
binary files which are difficult to analyze when the comparison detects a problem
due to something broken by some modifications.

We _want_ to understand and test things at the binary byte level.
Debugging differences is a little trickier, but it's really not that
bad - -Odts or fdtdump or dtdiff can be used if/when there's a test
failure.  I really think doing the comparison in binary is preferable
- that's the level at which the behaviour is specified and should be
tested.

quoted

quoted

+    run_wrap_error_test $DTC -I dtb -O dts -o unknown_tags_no_skip.dtb.dts unknown_tags_no_skip.dtb
 }
 
 cmp_tests () {

diff --git a/tests/unknown_tags_can_skip.dtb.dts.expect b/tests/unknown_tags_can_skip.dtb.dts.expect
new file mode 100644
index 00000000..2194025b
--- /dev/null
+++ b/tests/unknown_tags_can_skip.dtb.dts.expect

@@ -0,0 +1,19 @@
+/dts-v1/;
+
+/ {
+	prop-int = <0x3201>;
+	prop-str = "abcd";
+
+	subnode1 {
+		prop-int = <0x6401 0x6402>;
+	};
+
+	subnode2 {
+		prop-int1 = <0x64020 0x64021>;
+		prop-int2 = <0x32022>;
+
+		subsubnode {
+			prop-bool;
+		};
+	};
+};

-- 
2.55.0

  

Best regards,
Hervé

-- 
David Gibson (he or they)	| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you, not the other way
				| around.
http://www.ozlabs.org/~dgibson