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

[Herve Codina <herve.codina@bootlin.com>]

Hi David,

On Mon, 14 Sep 2026 18:23:49 +1000
David Gibson <david@gibson.dropbear.id.au> wrote:

> On Wed, Aug 26, 2026 at 10:31:39AM +0200, Herve Codina wrote:
> > The structured tag value definition introduced recently gives the
> > ability to ignore unknown tags without any error when they are read.
> > 
> > Handle those structured tag.
> > 
> > Signed-off-by: Herve Codina <herve.codina@bootlin.com>
> > Reviewed-by: Luca Ceresoli <luca.ceresoli@bootlin.com>
> > Reviewed-by: Frank Li <Frank.Li@nxp.com>
> > ---
> >  flattree.c                                 | 65 ++++++++++++++++++++--
> >  tests/run_tests.sh                         |  5 ++
> >  tests/unknown_tags_can_skip.dtb.dts.expect | 19 +++++++
> >  3 files changed, 84 insertions(+), 5 deletions(-)
> >  create mode 100644 tests/unknown_tags_can_skip.dtb.dts.expect
> > 
> > diff --git a/flattree.c b/flattree.c
> > index f3b698c1..88dbfa7e 100644
> > --- a/flattree.c
> > +++ b/flattree.c
> > @@ -579,7 +579,8 @@ static void flat_read_chunk(struct inbuf *inb, void *p, int len)
> >  	if ((inb->ptr + len) > inb->limit)
> >  		die("Premature end of data parsing flat device tree\n");
> >  
> > -	memcpy(p, inb->ptr, len);
> > +	if (p)
> > +		memcpy(p, inb->ptr, len);
> >  
> >  	inb->ptr += len;
> >  }
> > @@ -604,6 +605,61 @@ static void flat_realign(struct inbuf *inb, int align)
> >  		die("Premature end of data parsing flat device tree\n");
> >  }
> >  
> > +static bool flat_skip_unknown_tag(struct inbuf *inb, uint32_t tag)
> > +{
> > +	uint32_t lng;
> > +
> > +	if (!(tag & FDT_TAG_STRUCTURED) || !(tag & FDT_TAG_SKIP_SAFE))
> > +		return false;
> > +
> > +	switch (tag & FDT_TAG_DATA_MASK) {
> > +	case FDT_TAG_DATA_NONE:
> > +		break;
> > +
> > +	case FDT_TAG_DATA_1CELL:
> > +		flat_read_word(inb);
> > +		break;
> > +
> > +	case FDT_TAG_DATA_2CELLS:
> > +		flat_read_word(inb);
> > +		flat_read_word(inb);
> > +		break;
> > +
> > +	case FDT_TAG_DATA_VARLEN:
> > +		/* Get the length */
> > +		lng = flat_read_word(inb);  
> 
> I think it would be more natural to get the length as a single value,
> then have a common flat_read_chunk() and flat_realign() to consume it.
> That's for two reasons:
>  * Assuming we keep this length encoding, getting the final tag size
>    seems like it would make a useful helper function anyway.
>  * Using flat_read_word() is misleading - it implies it's integer data
>    where endianness matters.  In this case it's not - it's just some
>    bytes we're skipping over, we don't know the internal structure.

Well, without the length for all tags (I mean keeping some size encoding
in the tag value), we can avoid the flat_read_word().
--- 8< ---
	switch (tag & FDT_TAG_DATA_MASK) {
	case FDT_TAG_DATA_NONE:
		lng = 0;
		break;

	case FDT_TAG_DATA_1CELL:
		lng = sizeof(uint32_t);
		break;

	case FDT_TAG_DATA_2CELLS:
		lng = 2 * sizeof(uint32_t);
		break;

	case FDT_TAG_DATA_VARLEN:
		/* Get the length */
		lng = flat_read_word(inb)
		break;
        }

        if (lng) {
		flat_read_chunk(inb, NULL, lng);
		flat_realign(inb, sizeof(uint32_t));
        }
---- 8< ----

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/
[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.

> 
> > +
> > +		/* 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.

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

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

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

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

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

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


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.

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