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

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

Hi David,

On Tue, 20 Oct 2026 17:41:04 +1100
David Gibson <david@gibson.dropbear.id.au> wrote:

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

I wouldn't say you should stop reviewing.

Maybe you can have in mind that fdt_next_tag() will return all tags and so,
filtering out tags at fdt_next_tag() or using fdt_next_tag_filter() will not
be there anymore. This will not change fundamentally the way tags are used.
If they are skipped by filter function in this series, they will be skipped
by callers in the next iteration.

New tags added in the series will be there in the next iteration. The meaning
of those new tags will not be changed. As well as new dts keywords or other
concepts such as referencing an orphan node by path.

Tests introduced will also be there in the next iteration.

A lot of things available in the series will be there in the next iteration.

For all those reasons, I thing your review can still be useful.

Maybe you will find some other patches that could be applied or move to the
series introducing "structured" tags.

IMHO, even if I (we) know that some modification discussed in the series
adding support for structured tags will have impacts to this series, either
due the modification itself or because I will keep things consistent, having
your feedback and potential discussion on this series is still relevant.

Best regards,
Hervé