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

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

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

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.

> +{
> +	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;
> +}
> +
> +bool fdt_tag_filter_skip_metadata(void *data, uint32_t tag)
> +{
> +	return false;
> +}
> +
> +bool fdt_tag_is_property_metadata(uint32_t tag)
> +{
> +	return false;
> +}
> +
>  int fdt_check_node_offset_(const void *fdt, int offset)
>  {
>  	if (!can_assume(VALID_INPUT)
> @@ -355,7 +382,8 @@ int fdt_root_offset(const void *fdt)
>  
>  	do {
>  		offset = nextoffset;
> -		tag = fdt_next_tag(fdt, offset, &nextoffset);
> +		tag = fdt_next_tag_filter(fdt, offset, &nextoffset,
> +					  fdt_tag_filter_skip_metadata, NULL);
>  		switch (tag) {
>  		case FDT_END_NODE:
>  		case FDT_PROP:
> @@ -393,8 +421,8 @@ int fdt_next_node(const void *fdt, int offset, int *depth)
>  
>  	do {
>  		offset = nextoffset;
> -		tag = fdt_next_tag(fdt, offset, &nextoffset);
> -
> +		tag = fdt_next_tag_filter(fdt, offset, &nextoffset,
> +					  fdt_tag_filter_skip_metadata, NULL);
>  		switch (tag) {
>  		case FDT_PROP:
>  		case FDT_NOP:
> diff --git a/libfdt/fdt_ro.c b/libfdt/fdt_ro.c
> index 50c0a8bc..ea6a540d 100644
> --- a/libfdt/fdt_ro.c
> +++ b/libfdt/fdt_ro.c
> @@ -206,7 +206,8 @@ static int nextprop_(const void *fdt, int offset)
>  	int nextoffset;
>  
>  	do {
> -		tag = fdt_next_tag(fdt, offset, &nextoffset);
> +		tag = fdt_next_tag_filter(fdt, offset, &nextoffset,
> +					  fdt_tag_filter_skip_metadata, NULL);
>  
>  		switch (tag) {
>  		case FDT_END:
> diff --git a/libfdt/fdt_rw.c b/libfdt/fdt_rw.c
> index 1c6ce0a3..30f3da40 100644
> --- a/libfdt/fdt_rw.c
> +++ b/libfdt/fdt_rw.c
> @@ -259,6 +259,42 @@ int fdt_prop_remove_unknown_tags(void *fdt, bool force_inplace,
>  	return 0;
>  }
>  
> +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.

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

> +
> +	} while ((tag != FDT_PROP) && (tag != FDT_BEGIN_NODE) &&
> +		 (tag != FDT_END_NODE));
> +
> +	return 0;
> +}
> +
>  static int fdt_resize_property_(void *fdt, int nodeoffset,
>  				const char *name, int namelen,
>  				int len, struct fdt_property **prop)
> @@ -273,10 +309,18 @@ static int fdt_resize_property_(void *fdt, int nodeoffset,
>  		return oldlen;
>  
>  	/*
> -	 * The property is resized. Remove possible unknown tags related to the
> -	 * property downgrading the dtb version.
> +	 * The property is resized and data are going to be modified. Remove
> +	 * property metadata tags
>  	 */
>  	prop_offset = fdt_ptr_offset_(fdt, *prop);
> +	err = fdt_prop_remove_metadata_tags(fdt, false, prop_offset);
> +	if (err)
> +		return err;
> +
> +	/*
> +	 * Remove also possible unknown tags related to the property downgrading
> +	 * the dtb version.
> +	 */
>  	err = fdt_prop_remove_unknown_tags(fdt, false, prop_offset, true);
>  	if (err)
>  		return err;
> @@ -289,7 +333,7 @@ static int fdt_resize_property_(void *fdt, int nodeoffset,
>  	return 0;
>  }
>  
> -static int fdt_node_skip_unknown_tags(void *fdt, int next)
> +static int fdt_node_skip_unknown_and_metadata_tags(void *fdt, int next)
>  {
>  	int nextoffset = next;
>  	int offset;
> @@ -299,6 +343,7 @@ static int fdt_node_skip_unknown_tags(void *fdt, int next)
>  	 * Skip all tags related to the current node. I.e. tags after the
>  	 * current node and before either the next property, a sub-node or the
>  	 * end of current node.
> +	 * FDT_NOP, unknown tags and potential node metadata tags are skipped.
>  	 */
>  	do {
>  		offset = nextoffset;
> @@ -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.

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

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

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

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


>  	/*
>  	 * Remove unknown tags related to the property downgrading the dtb
>  	 * version.
> @@ -81,10 +92,18 @@ int fdt_nop_property(void *fdt, int nodeoffset, const char *name)
>  		return len;
>  
>  	/*
> -	 * The property is going to be removed (nopified). Remove unknown tags
> -	 * related to this property. Keep the dtb version unchanged.
> +	 * The property is going to be removed (nopified). Remove its metadata
> +	 * tags
>  	 */
>  	prop_offset = fdt_ptr_offset_(fdt, prop);
> +	err = fdt_prop_remove_metadata_tags(fdt, true, prop_offset);
> +	if (err)
> +		return err;
> +
> +	/*
> +	 * Remove also possible unknown tags related to this property.
> +	 * Keep the dtb version unchanged.
> +	 */
>  	err = fdt_prop_remove_unknown_tags(fdt, true, prop_offset, false);
>  	if (err)
>  		return err;
> diff --git a/libfdt/libfdt_internal.h b/libfdt/libfdt_internal.h
> index d4154119..47dfc0d4 100644
> --- a/libfdt/libfdt_internal.h
> +++ b/libfdt/libfdt_internal.h
> @@ -23,6 +23,14 @@ int32_t fdt_ro_probe_(const void *fdt);
>  uint32_t fdt_next_tag_(const void *fdt, int startoffset, int *nextoffset,
>  		       bool *is_unknown);
>  
> +uint32_t fdt_next_tag_filter(const void *fdt, int offset, int *nextoffset,
> +			     bool (*tag_filter)(void *priv, uint32_t tag),
> +			     void *priv);
> +
> +bool fdt_tag_filter_skip_metadata(void *data, uint32_t tag);
> +
> +bool fdt_tag_is_property_metadata(uint32_t tag);
> +
>  int fdt_get_structured_tag_data(uint32_t tag, const void *fdt, int offset,
>  				uint32_t *data_len);
>  
> @@ -31,6 +39,8 @@ int fdt_check_prop_offset_(const void *fdt, int offset);
>  
>  int fdt_prop_remove_unknown_tags(void *fdt, bool force_inplace,
>  				 int prop_offset, bool downgrade_version);
> +int fdt_prop_remove_metadata_tags(void *fdt, bool force_inplace,
> +				  int prop_offset);
>  
>  int fdt_getprop_offset_namelen(const void *fdt, int nodeoffset,
>  			       const char *name, int namelen);
> -- 
> 2.55.0
> 
> 

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