On Mon, Oct 19, 2026 at 03:56:59PM +0200, Herve Codina wrote: > Hi David, > > On Mon, 19 Oct 2026 11:58:17 +1100 > David Gibson wrote: > > > On Wed, Aug 26, 2026 at 11:48:34AM +0200, Herve Codina wrote: > > > Metadata tags are tags carrying extra information, either global to > > > the DTB, specific to a node, or specific to a property. > > > > > > In several places (e.g. node/property iterators), these tags just need > > > to be skipped. Introduce fdt_next_tag_filter() which wraps > > > fdt_next_tag() and uses a user-supplied filter function to decide > > > whether a tag should be hidden from the caller. Filtered-out tags are > > > returned as FDT_NOP. > > > > > > Introduce fdt_tag_filter_skip_metadata() a filter function whose purpose > > > is to filter metadata tags; actual metadata tag filtering will be added > > > when metadata tags are defined. > > > > > > Metadata tags must also be handled when a DTB is modified. They have to > > > be removed or skipped depending on the modification. > > > > > > Here also, introduce fdt_tag_is_property_metadata() to provide a central > > > check for whether a tag is a property metadata tag; actual metadata > > > check will be added when metadata tags are defined. > > > > > > Signed-off-by: Herve Codina > > > --- > > > libfdt/fdt.c | 34 +++++++++++++-- > > > libfdt/fdt_ro.c | 3 +- > > > libfdt/fdt_rw.c | 90 +++++++++++++++++++++++++++++++++------- > > > libfdt/fdt_wip.c | 23 +++++++++- > > > libfdt/libfdt_internal.h | 10 +++++ > > > 5 files changed, 140 insertions(+), 20 deletions(-) > > > > > > diff --git a/libfdt/fdt.c b/libfdt/fdt.c > > > index 7941632f..8c026f26 100644 > > > --- a/libfdt/fdt.c > > > +++ b/libfdt/fdt.c > > > @@ -323,6 +323,33 @@ uint32_t fdt_next_tag(const void *fdt, int startoffset, int *nextoffset) > > > return fdt_next_tag_(fdt, startoffset, nextoffset, NULL); > > > } > > > > > > +uint32_t fdt_next_tag_filter(const void *fdt, int offset, int *nextoffset, > > > + bool (*tag_filter)(void *priv, uint32_t tag), > > > + void *priv) > > > > I don't really see why a higher order function is more useful than > > skipping tags you don't care about from the caller. > > Based on discussion we had related to fdt_next_tag() during the series introducing > structured tags, I has been decided that fdt_next_tag() will return all tags and > callers will handle them. > > With that done, fdt_next_tag_filter() doesn't make sense anymore. I have already > planned to fully remove it. Ok, great. > > > +{ > > > + int startoffset = offset; > > > + uint32_t tag; > > > + > > > + tag = fdt_next_tag_(fdt, startoffset, nextoffset, NULL); > > > + if (tag == FDT_END) > > > + return tag; > > > + > > > + if (tag_filter(priv, tag)) > > > + return FDT_NOP; > > > + > > > + return tag; > > > +} > > ... > > > > > > > +int fdt_prop_remove_metadata_tags(void *fdt, bool force_inplace, > > > + int prop_offset) > > > +{ > > > + int nextoffset, offset; > > > + uint32_t tag; > > > + > > > + /* > > > + * Only inplace using nopify is supported even if we could use an other > > > + * method involving splices if force_inplace is set to false. > > > + */ > > > > As with earlier things, I'd really prefer to see this splice, based on > > principle of least surprise. > > Yes, splice will be done (already planned for unknown tags in the series > adding support for structured tags). Ok. > > > + > > > + tag = fdt_next_tag(fdt, prop_offset, &nextoffset); > > > + if (tag == FDT_END) > > > + return nextoffset; > > > + > > > + /* > > > + * Look at all tags related to the current property. I.e. tags after the > > > + * current property and before either the next property, a sub-node or > > > + * the end of current node > > > + */ > > > + do { > > > + offset = nextoffset; > > > + tag = fdt_next_tag(fdt, offset, &nextoffset); > > > + if (tag == FDT_END) > > > + return nextoffset; > > > + > > > + /* Remove metadata tags related to the property */ > > > + if (fdt_tag_is_property_metadata(tag)) > > > + fdt_nopify_area(fdt, offset, nextoffset); > > > > The distinction between "metadata" and other tags doesn't make much > > sense to me. Specifically why would the tag being "metadata" make it > > more of a candidate for removal? What we need to know (for the > > callers so far) is whether the tag is invalidated by changing the > > contents of the property: I can't see how that would be related to > > whether it's "metadata" or not. > > All of this is going to be reworked based on discussion we've already had > during the previous series (introducing structured tag) review. > > The distinction was done because for "unknown" tags, downgrade should > be done whereas for "known" tags it shouldn't. > > Anyway, all of that is going to be reworked for the next iteration. Ok, fair enough. Should I stop reviewing the current version of this series, since it will probably need a lot of rework based on my comments to the first series. > > > + > > > + } while ((tag != FDT_PROP) && (tag != FDT_BEGIN_NODE) && > > > + (tag != FDT_END_NODE)); > > > + > > > + return 0; > > > +} > > ... > > > > @@ -332,10 +377,10 @@ static int fdt_add_property_(void *fdt, int nodeoffset, const char *name, > > > > > > /* > > > * nextoffset it at the first tag after the node. > > > - * Skip possible unknown tags related to the node in order to add the > > > - * property after those tags. > > > + * Skip possible unknown tags and metadata tags related to the node in > > > + * order to add the property after those tags. > > > */ > > > - nextoffset = fdt_node_skip_unknown_tags(fdt, nextoffset); > > > + nextoffset = fdt_node_skip_unknown_and_metadata_tags(fdt, nextoffset); > > > > This makes even less sense to me. The tags following the node belong > > to the node, until either FDT_BEGIN_NODE, FDT_END_NODE or FDT_PROP. > > We need to skip all of them regardless of whether they're "metadata" > > or not. > > Yes, I will rework that part too. > > > > > > if (nextoffset < 0) > > > return nextoffset; > > > > > > @@ -453,6 +498,12 @@ int fdt_appendprop(void *fdt, int nodeoffset, const char *name, > > > if (err) > > > return err; > > > > > > + /* > > > + * We append data to the property. If known metadata tags are > > > + * present, they are related to existing data and not impacted > > > + * by data added. No need to remove them. > > > > You don't know that. It might be true for the tags you have in mind > > so far, but there's no guarantee. It's a contrived example, but a tag > > which gives "number of 0 bytes in the property" would be invalidated > > by an append, for example. > > We talk about known tags. "Known" tags are tags that will be added in this series. > If other tags are added in the future, this could become incorrect indeed and should > be handle when those tags are introduce (in the future). > > Anyway, I will probably rework this part too in order to make things clear. > > The idea was to keep existing known tags related to the property if not > impacted by the append. Well.. more accurately, because we know the tags, we'll know whether they need to be removed or updated based on their specific semantics. > > > + */ > > > + > > > newlen = len + oldlen; > > > err = fdt_splice_struct_(fdt, prop->data, > > > FDT_TAGALIGN(oldlen), > > > @@ -484,11 +535,16 @@ int fdt_delprop(void *fdt, int nodeoffset, const char *name) > > > if (!prop) > > > return len; > > > > > > + /* The property is going to be removed. Remove its metadata tags */ > > > + prop_offset = fdt_ptr_offset_(fdt, prop); > > > + err = fdt_prop_remove_metadata_tags(fdt, false, prop_offset); > > > + if (err) > > > + return err; > > > > All tags from the property to the next END_NODE or FDT_PROP need to be > > removed, regardless of whether they're "metadata" or not. > > > > Also, rather than removing them one by one, I'd prefer to see the > > total length of them calculated, and remove the property and extra > > tags with a single splice. > > Will be updated in the next iteration. > > > > > > + > > > /* > > > - * The property is going to be removed. Remove also possible unknown > > > - * tags related to this property. Keep the dtb version unchanged. > > > + * Remove also possible unknown tags related to this property. > > > + * Keep the dtb version unchanged. > > > */ > > > - prop_offset = fdt_ptr_offset_(fdt, prop); > > > err = fdt_prop_remove_unknown_tags(fdt, false, prop_offset, false); > > > if (err) > > > return err; > > > @@ -522,10 +578,15 @@ int fdt_add_subnode_namelen(void *fdt, int parentoffset, > > > return offset; > > > > > > /* > > > - * Try to place the new node after the parent's properties and unknown > > > - * tags related to those properties. > > > - * Unknown tags are reported as FDT_NOP tags by fdt_next_tag. > > > - * Skipping FDT_NOP tags will correctly skip unknown tags. > > > + * Try to place the new node after: > > > + * - the parent's properties > > > + * - the metadata tags related to those properties > > > > Again, the "metadata" distinction doesn't really make sense. You want > > to place the new node immediately before the first FDT_BEGIN_NODE or > > FDT_END_NODE tag within the parent node. > > Here also, will be updated. > > > > > > + * - unknown tags related to those properties. > > > + * Unknown tags are reported as FDT_NOP tags by fdt_next_tag_filter. > > > + * metadata tags are also reported as FDT_NOP by fdt_next_tag_filter > > > + * thanks to the filter function used. > > > + * Skipping FDT_NOP tags will correctly skip unknown tags and metadata > > > + * tags. > > > */ > > > tag = fdt_next_tag(fdt, parentoffset, &nextoffset); > > > /* the fdt_subnode_offset_namelen() should ensure this never hits */ > > > @@ -533,7 +594,8 @@ int fdt_add_subnode_namelen(void *fdt, int parentoffset, > > > return -FDT_ERR_INTERNAL; > > > do { > > > offset = nextoffset; > > > - tag = fdt_next_tag(fdt, offset, &nextoffset); > > > + tag = fdt_next_tag_filter(fdt, offset, &nextoffset, > > > + fdt_tag_filter_skip_metadata, NULL); > > > } while ((tag == FDT_PROP) || (tag == FDT_NOP)); > > > > > > nh = fdt_offset_ptr_w_(fdt, offset); > > > diff --git a/libfdt/fdt_wip.c b/libfdt/fdt_wip.c > > > index 7ca3ffbc..b2f9f8d1 100644 > > > --- a/libfdt/fdt_wip.c > > > +++ b/libfdt/fdt_wip.c > > > @@ -31,6 +31,17 @@ int fdt_setprop_inplace_namelen_partial(void *fdt, int nodeoffset, > > > if ((unsigned)proplen < (len + idx)) > > > return -FDT_ERR_NOSPACE; > > > > > > + if (idx < (unsigned)proplen) { > > > + /* > > > + * The property existing data are going to be modified. > > > + * The property metadata tags are related to the property > > > + * data. Remove them. > > > + */ > > > + err = fdt_prop_remove_metadata_tags(fdt, true, prop_offset); > > > + if (err) > > > + return err; > > > + } > > > > Similar to removal, I'd prefer to see a calculation of the total > > length and a single splice_region, rather than removing them one by > > one. > > Yes. > > 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