On Wed, Sep 30, 2026 at 06:47:08PM +0200, Herve Codina wrote: > Hi David, > > On Sat, 19 Sep 2026 14:46:05 +1000 > David Gibson wrote: > > ... > > > > > > Upper layer using fdt_next_tag() should not handle this tag skipping the > > > general case and let fdt_next_tag() do this job. > > > > > > Got the feeling that we are going to add complexity at upper layer to handle > > > a feature that should be handled at low level. > > > > > > > > > > > > Ok, we can have a wrapper on top of > > > > > fdt_next_tag() to skip unknown tags. > > > > > > > > > > fdt_next_tag() is a low level function but the rule related to skipping > > > > > unknown tags is also a low level rule. > > > > > > > > > > We have to choose: > > > > > a) fdt_next_tag() as I proposed > > > > > Of course ok for me > > > > > > > > > > b) fdt_next_tag() returns all tags -> Full handling at caller side > > > > > Ok, we can go in that direction but it will bring quite a lot of > > > > > code duplication. > > > > > > > > I'm not sure it's that bad, since fdt_next_tag() loops will typically > > > > already have a 'default' case for irrelevant tags, which will > > > > generally be handled the same as unknown tags. > > > > > > Well as we did for "offset 0 vs real root node offset", I think the best way > > > to known if it's bad or not is to try. > > > > Agreed. > > > > > I will not have time this week to propose something and I have a training > > > next week. > > > > > > If you're ok for a try, I will do it but not as quickly as I did for the > > > "offset 0 vs real root node offset" topic. > > > > > I did my homework and I think I was wrong. fdt_next_tag() can return all tags > without adding too much complexity in current code. Right, I suspected that would be the case. > Here is the diff to consider that fully replaces this current patch. > > You can note that I've added the FDT_TAG_IS_SKIPPABLE() macro. This macro > should be added in a previous commit introducing structured tag (or skippable > if renamed). According to discussion we've already had related to "structure" > tag format, the FDT_TAG_IS_SKIPPABLE() macro content will not be that one but > you've got the idea of its usage in the diff. Ok. I'm having second thoughts about the "skippable" terminology based on the complications for read-write access, but I'll discuss that in full elsewhere. Whatever we call it, the idea of an is_new_style(tag) macro makes sense. > --- 8< --- > diff --git a/libfdt/fdt.c b/libfdt/fdt.c > index eb803e8a..e426414b 100644 > --- a/libfdt/fdt.c > +++ b/libfdt/fdt.c > @@ -218,7 +218,37 @@ uint32_t fdt_next_tag(const void *fdt, int startoffset, int *nextoffset) > break; > > default: > - return FDT_END; > + if (!(tag & FDT_TAG_STRUCTURED) || !(tag & FDT_TAG_SKIP_SAFE)) > + return FDT_END; > + > + switch (tag & FDT_TAG_DATA_MASK) { > + case FDT_TAG_DATA_NONE: > + break; > + case FDT_TAG_DATA_1CELL: > + offset += FDT_CELLSIZE; > + break; > + case FDT_TAG_DATA_2CELLS: > + offset += 2 * FDT_CELLSIZE; > + break; > + case FDT_TAG_DATA_VARLEN: > + /* Get the length */ > + lenp = fdt_offset_ptr(fdt, offset, sizeof(*lenp)); > + if (!can_assume(VALID_DTB) && !lenp) > + return FDT_END; /* premature end */ > + len = fdt32_to_cpu(*lenp); > + /* > + * Skip the cell encoding the length and the > + * following length bytes > + */ > + len += sizeof(*lenp); > + sum = len + offset; > + if (!can_assume(VALID_DTB) && > + (sum >= INT_MAX || sum < (uint32_t) offset)) > + return FDT_END; /* premature end */ > + > + offset += len; > + break; > + } > } > > if (!fdt_offset_ptr(fdt, startoffset, offset - startoffset)) > @@ -303,6 +333,7 @@ int fdt_next_node(const void *fdt, int offset, int *depth) > switch (tag) { > case FDT_PROP: > case FDT_NOP: > + default: /* All Skippable tags */ > break; > > case FDT_BEGIN_NODE: > diff --git a/libfdt/fdt.h b/libfdt/fdt.h > index f41a355f..55138261 100644 > --- a/libfdt/fdt.h > +++ b/libfdt/fdt.h > @@ -73,6 +73,8 @@ struct fdt_property { > #define FDT_TAG_CAN_SKIP(tag_data, tag_id) \ > (FDT_TAG_STRUCTURED | FDT_TAG_SKIP_SAFE | tag_data | tag_id) > > +#define FDT_TAG_IS_SKIPPABLE(_tag) (((_tag) & (FDT_TAG_STRUCTURED | FDT_TAG_SKIP_SAFE)) == (FDT_TAG_STRUCTURED | FDT_TAG_SKIP_SAFE)) > + > /* Tests reserved tags */ > #define FDT_TEST_NONE_CAN_SKIP FDT_TAG_CAN_SKIP(FDT_TAG_DATA_NONE, 0) > #define FDT_TEST_1CELL_CAN_SKIP FDT_TAG_CAN_SKIP(FDT_TAG_DATA_1CELL, 0) > diff --git a/libfdt/fdt_check.c b/libfdt/fdt_check.c > index 2fd5b61d..8bd9b252 100644 > --- a/libfdt/fdt_check.c > +++ b/libfdt/fdt_check.c > @@ -45,7 +45,8 @@ int fdt_check_full(const void *fdt, size_t bufsize) > return nextoffset; > > /* If we see two root nodes, something is wrong */ > - if (expect_end && tag != FDT_END && tag != FDT_NOP) > + if (expect_end && tag != FDT_END && tag != FDT_NOP && > + !FDT_TAG_IS_SKIPPABLE(tag)) > return -FDT_ERR_BADSTRUCTURE; > > switch (tag) { > @@ -92,7 +93,8 @@ int fdt_check_full(const void *fdt, size_t bufsize) > break; > > default: > - return -FDT_ERR_INTERNAL; > + /* Skippable tags -> Skip them */ > + break; > } > } > } > diff --git a/libfdt/fdt_ro.c b/libfdt/fdt_ro.c > index 856c62f1..3cc0435c 100644 > --- a/libfdt/fdt_ro.c > +++ b/libfdt/fdt_ro.c > @@ -219,7 +219,7 @@ static int nextprop_(const void *fdt, int offset) > return offset; > } > offset = nextoffset; > - } while (tag == FDT_NOP); > + } while (tag == FDT_NOP || FDT_TAG_IS_SKIPPABLE(tag)); > > return -FDT_ERR_NOTFOUND; > } > diff --git a/tests/run_tests.sh b/tests/run_tests.sh > index 8fc23cb7..225c22f8 100755 > --- a/tests/run_tests.sh > +++ b/tests/run_tests.sh > @@ -577,11 +577,12 @@ libfdt_tests () { > run_test dtbs_equal_ordered cell-overflow.test.dtb cell-overflow-results.test.dtb > > # check full tests > - for good in test_tree1.dtb; do > + for good in test_tree1.dtb unknown_tags_can_skip.dtb; do > run_test check_full $good > done > for bad in truncated_property.dtb truncated_string.dtb \ > - truncated_memrsv.dtb two_roots.dtb named_root.dtb; do > + truncated_memrsv.dtb two_roots.dtb named_root.dtb \ > + unknown_tags_no_skip.dtb; do > run_test check_full -n $bad > done > } > @@ -962,6 +963,10 @@ fdtget_tests () { > run_fdtget_test "" -tx \ > -d "" $dtb /randomnode doctor-who > run_fdtget_test "" -tx -d "" $dtb /memory doctor-who > + > + # test with unknown tags involved > + run_fdtget_test "25601 25602" unknown_tags_can_skip.dtb /subnode1 prop-int > + run_wrap_error_test $DTGET unknown_tags_no_skip.dtb /subnode1 prop-int > } > > fdtput_tests () { > --- 8< --- That looks fine, except insofar as other changes we've already discussed will require updates here too. > Also I've modified tests/dtbs_equal_ordered.c but I am not sure that the > modification I did is a modification you will accept. > > We could just ignore skippable tags but I choose to test data when such an > skippable tag is encoutered. > > This leads to the following modification: > --- 8< --- > --- a/tests/dtbs_equal_ordered.c > +++ b/tests/dtbs_equal_ordered.c > @@ -71,6 +71,7 @@ static void compare_structure(const void *fdt1, const void *fdt2) > int err; > const struct fdt_property *prop1, *prop2; > int len1, len2; > + const void *raw1, *raw2; > > while (1) { > do { > @@ -102,6 +103,9 @@ static void compare_structure(const void *fdt1, const void *fdt2) > name1, name2, offset1, offset2); > break; > > + case FDT_END_NODE: > + break; > + > case FDT_PROP: > prop1 = fdt_offset_ptr(fdt1, offset1, sizeof(*prop1)); > if (!prop1) > @@ -128,6 +132,36 @@ static void compare_structure(const void *fdt1, const void *fdt2) > > case FDT_END: > return; > + > + default: > + if (!FDT_TAG_IS_SKIPPABLE(tag1)) > + FAIL("Cannot check tag data at (%d, %d)", > + offset1, offset2); > + > + /* > + * Even if we don't known the meaning of data, > + * we can check that raw data are the same. > + */ > + len1 = nextoffset1 - offset1; > + len2 = nextoffset2 - offset2; > + > + if (len1 != len2) { > + MISMATCH("tag data length mismatch at (%d, %d)", > + offset1, offset2); > + } > + > + raw1 = fdt_offset_ptr(fdt1, offset1, len1); > + if (!raw1) > + FAIL("Could get fdt1 raw buffer at %d", offset1); > + > + raw2 = fdt_offset_ptr(fdt2, offset2, len2); > + if (!raw1) > + FAIL("Could get fdt1 raw buffer at %d", offset2); > + > + if (memcmp(raw1, raw2, len1) != 0) > + MISMATCH("Tag data value mismatch at (%d, %d)", > + offset1, offset2); > + break; > } > } > } > --- 8< --- So, as I mentioned elswhere a version of dtbs_equal_ordered that also checks tag data sounds like a very useful tool. But, I can also see the use of comparisons that ignore metadata tags. So, I think we want both variants. That could be either separate test binaries, or an option to dtbs_equal_ordered and/or dtbs_equal_unordered, whicever turns out easier. > With that done, my conclusion is: Yes we can return all tags in fdt_next_tag(). > I think you will agree this conclusion. Excellent. > For dtbs_equal_ordered.c, should we compare skippable tags and related data as > I did (diff above) or should we just skip them? what is your > opinion? As above, I think we want both options. > For the next patch ("libfdt: Handle unknown tags on dtb modifications") > returning all tags from fdt_next_tag() has also some impacts but nothing more > complex than modification provided here. I can provide the full diff if you > want. Let me know. Not for now, leave it until you post the next revision of the series. -- 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