Re: [RFC PATCH v2 07/74] Add support for FDT_PROPDATA_PHANDLE dtb tag

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

Hi David,

On Thu, 22 Oct 2026 14:02:05 +1100
David Gibson <david@gibson.dropbear.id.au> wrote:

...

> > What do you think is clunky ?
> > Is it the PROPDATA term itself ?  
> 
> Pretty much yes.
> 
> After the explanation above, I think I'm ok with FDT_PROPDATA_PHANDLE.

Ok, I will keep it (and FDT_PROPDATA_PHANDLE_REF).

...

> > > > -static struct data data_append_markers(struct data d, struct marker *m)
> > > > +struct data data_append_markers(struct data d, struct marker *m)
> > > >  {
> > > >  	struct marker **mp = &d.markers;
> > > >  
> > > > @@ -288,6 +288,7 @@ struct marker *alloc_marker(unsigned int offset, enum markertype type,
> > > >  	m->type = type;
> > > >  	m->ref = ref;
> > > >  	m->next = NULL;
> > > > +	m->is_local = 0;    
> > > 
> > > Storing is_local in the marker doesn't make sense to me.  I think the
> > > locality should be calculated only when we actually go to emit the dtb
> > > - I believe we should be able to deduce it from m->ref.  
> > 
> > When we parse a dtb, we do not have any reference for local phandle, we have
> > only the phandle value.  
> 
> Heh, right, I remarked on that further down.
> 
> > Later in build_dt_info(), complete_references() is called. This new function
> > is introduced in patch 9 ("dtc: Complete REF_PHANDLE markers with ref strings
> > and is_local flags"). It completes the reference either filling the ref field
> > based on the phandle value for all local reference (parsing from a dtb) or
> > setting is_local based on the reference (parsing from a dts).  
> 
> Ah, right having that logic in a different patch makes it harder to
> follow.
> 
> What concerns me here is that currently we have an invariant that a
> REF_PHANDLE marker always has a meaningful m->ref field - from as soon
> as it's added to the property data.
> 
> This patch breaks that assumption, which means when looking at some
> random part of the code it's hard to know whether we're in a state
> where m->ref is valid or it might not me.
> 
> One approach is to minimise the gap where m->ref is invalid.  That's
> both in terms of code path - at minimum the m->ref fields should be
> valid by return from dt_from_*().  It's also in terms of not leaving
> intermediate patches where this is not fixed up.
> 
> The other approach would be to create new marker types indicating
> these intermediate states.  The markers are for dtc internal use, they
> don't need to have 1 to 1 correspondance with things in either dtb or
> source format.

I don't want to introduce new markers for handling intermediate states.

I really prefer avoiding those kind of "fake" marker which will have
a short lifetime and should not be seen by code (other than the code
moving this intermediate state to the final state).

My strategy was to ensure that once build_dt_info() is called, all
markers are fully correct with all field (is_local and ref) correctly
set.

build_dt_info() is called by all parser at the end of the parsing
operation to return a struct dt_info which is the dtc internal
internal representation used by the rest of code.

Are you ok with this strategy?

If yes, patches order in this series will be changed.

The current order is
 - Patch 7: Add support for FDT_PROPDATA_PHANDLE dtb tag
 - Patch 8: livetree: Improve get_node_by_phandle()
 - Patch 9: dtc: Complete REF_PHANDLE markers with ref strings and is_local flags

With that order we have the incomplete marker state present after patch 7 and
before patch 9.

Moving 'Add support for FDT_PROPDATA_PHANDLE dtb tag' after introducing the marker
state completion will avoid the problem.
 - Patch 7: livetree: Improve get_node_by_phandle()
 - Patch 8: dtc: Complete REF_PHANDLE markers with ref strings and is_local flags
 - Patch 9: Add support for FDT_PROPDATA_PHANDLE dtb tag

Patch 8 and 9 could be squashed if you prefer to have the completion added
in the 'Add support for FDT_PROPDATA_PHANDLE dtb tag' patch.

> 
> It seems to me there's an important difference between the -Idts and
> -Idtb cases: in the dts case, we have a valid reference, but the
> phandle needs to be filled in later; in the dtb case, we have a valid
> phandle, but the reference needs to be filled in later.  Using
> different marker types for those cases doesn't seem unreasonable for
> me.  In fact, for clarity it might even make sense to have a third
> variant for "both phandle and m->ref are valid" - the fixup code
> between input and output would convert either of the first two
> variants into the third, which is what the final output code would
> require.
> 
> That's not necessarily the best way to do it, but I really prefer
> where what's valid in a structure can be determined from the type and
> contents alone - rather than having to know where you are in a complex
> code path to know what's valid.

Performing marker completion in build_dt_info() avoid additional markers
and avoid also incomplete markers in the code path.

Before build_dt_info() call, the internal devicetree representation doesn't
exits for dtc. We are in the process of parsing and so we are building this
internal representation. After this call, the internal representation exists
and all markers are fully set without the need to introduce (and handle)
additional markers.

In other word, dti returned by all dt_from_*() functions returns a fully
complete dt_info.
--- 8< ---
	if (streq(inform, "dts"))
		dti = dt_from_source(arg);
	else if (streq(inform, "fs"))
		dti = dt_from_fs(arg);
	else if(streq(inform, "dtb"))
		dti = dt_from_blob(arg);
	else
		die("Unknown input format \"%s\"\n", inform);
--- 8< ---

User of this dti will never see an incomplete marker.

...

...
> > > > +		case FDT_PROPDATA_PHANDLE:
> > > > +			if (!(flags & FTF_PROPDATA_PHANDLE))
> > > > +				die("PROPDATA_PHANDLE tag found in flat tree"
> > > > +				    " version <20\n");
> > > > +
> > > > +			if (!prop)
> > > > +				die("PROPDATA_PHANDLE tag found without a previous PROP tag");
> > > > +
> > > > +			offset = flat_read_word(dtbuf);
> > > > +
> > > > +			/*
> > > > +			 * A reference to a phandle is present in the property
> > > > +			 * Mark the whole property as TYPE_UINT32.
> > > > +			 */
> > > > +			property_add_marker(prop, TYPE_UINT32, 0, NULL);
> > > > +
> > > > +			/* Mark the offset as a local phandle reference */
> > > > +			m = alloc_marker(offset, REF_PHANDLE, NULL);
> > > > +			m->is_local = true;
> > > > +			prop->val = data_append_markers(prop->val, m);    
> > > 
> > > This doesn't properly populate m->ref which is an essential field in a
> > > REF_PHANDLE marker.  Of course, doing so is kind of tricky - it
> > > requires searching for the node with the referenced phandle, which
> > > might not have been parsed yet.  So, alas, it will require a second
> > > pass to fix up the REF_PHANDLE markers.  
> > 
> > We cannot do it here. The whole dtb needs to be parsed to set correctly
> > m->ref.  
> 
> Right, that's what I mean by a second pass.
> 
> > This is done by complete_references() introduced in patch 9.  
> 
> Yeah, as above I don't like leaving this structure in a potentially
> incomplete state for several patches.

Patch re-ordering proposed above will fix this problem.



...
> > > Need to check for overflows here.  
> > 
> > What do you think about
> > 	if ((tmp + sizeof(fdt32_t) < tmp) ||
> > 	    (uint32_t)proplen < tmp + sizeof(fdt32_t))
> > 		return -FDT_ERR_???
> > Probably using an uint32_t for tmp here.  
> 
> That'd work, but it might be more natural to use:
> 	if (proplen < sizeof(fdt32_t) ||
> 	    tmp < 0 ||
> 	    tmp > proplen - sizeof(fdt32_t))
> 		return <error>;
> 
> That lets tmp remain an 'int'.  Using 'int' for offsets is sometimes
> problematic, but it's a convention that's now pretty baked into the
> libfdt interface.  Note that offsets encoded into the dtb are required
> to be < 2^31, not just < 2^32.

Fine, will do what you've suggested.

> 
> >   
> > >   
> > > > +				return -FDT_ERR_BADSTRUCTURE;    
> > > 
> > > I'm not sure BADSTRUCTURE makes sense for this.  I think it's probably
> > > worth adding a new error code for bad metadata tags.  
> > 
> > #define FDT_ERR_BADMETADATA	20
> > 	/* FDT_ERR_BADMETADATA: The device tree has corrupt metadata. */
> > 
> > Is it ok on your side ?  
> 
> Works for me.

Perfect.

> 
> > > > +
> > > > +			/* Retrieve the phandle value from the property */
> > > > +			pfdt32 = prop;
> > > > +			pfdt32 += tmp / sizeof(fdt32_t);    
> > > 
> > > Again, this will need updating for non-aligned phandles.  
> > 
> > Will handle unaligned offset.
> >   
> > >   
> > > > +			val = fdt32_ld(pfdt32);    
> > > 
> > > ... which reminds me the entire fdt32_ld() interface is kind of
> > > broken.  They're specifically intended to handle misaligned accesses,
> > > strictly speaking it's already UB to generate a misaligned pointer, so
> > > there's a problem before we even call it.  Fixing this within API/ABI
> > > constraints is a nasty problem :(.  
> > 
> > I can introduce and use the following:
> > ---- 8< ----
> > static inline uint32_t fdt32_ld_unaligned(const uint8_t *pfdt32)
> > {
> > 	return ((uint32_t)b[0] << 24)
> > 		| ((uint32_t)b[1] << 16)
> > 		| ((uint32_t)b[2] << 8)
> > 		| b[3];
> > }
> > ---- 8< ----  
> 
> Uh.. I'd rather not handle the fdt32_ld() mess at the same time as
> dealing with the addon stuff.  For now I think use fdt32_ld(), and
> continue to rely on the fact that in practice creating (but not
> dereferencing) misaligned fdt32_t * pointers is generally fine.

Ok, will keep fdt32_ld().

...
> > > >   * be set to the offset immediately following the tag.
> > > >   *
> > > >   * returns:
> > > > - *	the tag type (FDT_BEGIN_NODE, FDT_END_NODE, FDT_PROP, FDT_NOP, FDT_END),
> > > > + *	the tag type (FDT_BEGIN_NODE, FDT_END_NODE, FDT_PROP, FDT_NOP, FDT_END
> > > > + *	or any other known tags),    
> > > 
> > > This hunk looks like it belongs in an earlier patch.  
> > 
> > Well we add FDT_PROPDATA_PHANDLE and this is the first known tag added other
> > the existing ones already listed. Do you prefer an exhaustive list of known tags?
> > 
> > Also worth noting that fdt_next_tag() can return all unknown strutured tags that
> > can be skipped this has to be mentioned in the series adding support for structured
> > tag.  
> 
> Ah, yes.  Probably easiest to revise this to remove the complete
> listing of tags at the point we introduce structured tags.

Will update in the next iteration of the structured tag series and so no
modification will be present here in this support for addon series.

What do you thing about:
--- 8< ---
	/**
	 * fdt_next_tag - get next tag in the device tree
	...
	* returns:
 	*	The tag type, either a known tag or an unknown structured tag
	*       if this unknown tag can be skipped (SAFE_SKIP bit set in this
	*       tag).
	*	In case of error, FDT_END is returned with @nextoffset set to
	*	a negative error code.
 	*/
--- 8< ---

I suppose here, that you're ok to re-introduce SAFE_SKIP bit in structured tags
format. Asked in [1] and you don't reply yet.

[1]: https://lore.kernel.org/devicetree-compiler/20261021155923.55e48904@bootlin.com/

Best regards,
Hervé