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

[David Gibson]

On Tue, Oct 20, 2026 at 01:14:06PM +0200, Herve Codina wrote:
> Hi David,
> 
> On Tue, 20 Oct 2026 17:41:04 +1100
> David Gibson <david@xxxxxxxxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxxxxxxxx> 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@xxxxxxxxxxx>
> > > > > ---
> > > > >  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.

Ok, I'll carry on as time permits.

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

Ok, I'll bear that in mind.

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

Certainly.

-- 
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: signature.asc
Description: PGP signature