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

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 11763 bytes --]

On Tue, Sep 15, 2026 at 12:16:35PM +0200, Herve Codina wrote:
> 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< ----

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


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

Sounds good.

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

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

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

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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]