Re: [RFC PATCH v2 05/74] libfdt: Prepare for metadata tag support

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

Hi David,

On Mon, 19 Oct 2026 11:58:17 +1100
David Gibson <david@gibson.dropbear.id.au> 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 <herve.codina@bootlin.com>
> > ---
> >  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.

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

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

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

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