Re: [RFC PATCH v2 09/74] dtc: Complete REF_PHANDLE markers with ref strings and is_local flags

[Herve Codina]

Hi David,

On Fri, 30 Oct 2026 16:22:20 +1100
David Gibson <david@xxxxxxxxxxxxxxxxxxxxx> wrote:

...

> >  
> > +static void complete_ref_and_is_local(struct dt_info *dti, char **ref,
> > +				      bool *is_local, cell_t phandle)
> > +{
> > +	struct node *refnode;
> > +
> > +	/*
> > +	 * When created from a dts, the reference is available but the is_local
> > +	 * flag is not set.
> > +	 *
> > +	 * When created from a dtb, the is_local flag is set (and a valid
> > +	 * phandle value is available) but not the reference.
> > +	 *
> > +	 * Complete both ref and is_local based on existing values.  
> 
> The two cases share essentially zero code in this function, which
> suggests to me they'd be better as different functions (each of which
> could then have a more natural signature).
> 
> > +	 */
> > +
> > +	if (*ref) {
> > +		refnode = get_node_by_ref(dti->dt, *ref);
> > +		if (refnode)
> > +			*is_local = true;
> > +		else
> > +			*is_local = false;
> > +		return;
> > +	}
> > +
> > +	if (*is_local) {
> > +		refnode = get_node_by_phandle(dti->dt, phandle);
> > +		if (!refnode)
> > +			die("Node not found for phandle 0x%"PRIx32"\n", phandle);
> > +
> > +		*ref = refnode->fullpath;
> > +		return;
> > +	}
> > +
> > +	die("Cannot have a non local reference without a reference\n");
> > +}
> > +
> > +static void complete_references_internal(struct dt_info *dti, struct node *node)
> > +{
> > +	struct property *prop;
> > +	struct marker *m;
> > +	struct node *c;
> > +	cell_t phandle;
> > +
> > +	/*
> > +	 * In order to have a complete state, a REF_PHANDLE marker needs to
> > +	 * have a reference set (either a label or a path) and the is_local
> > +	 * flag must be also correctly set.
> > +	 *
> > +	 * When created from dtb, references are not present in REF_PHANDLE
> > +	 * markers created from the FDT_PROPDATA_PHANDLE tag. Indeed, only a
> > +	 * phandle value is available with this tag.
> > +	 *
> > +	 * When created from dts, references are available for REF_PHANDLE
> > +	 * markers but 'is_local' flag is not set.
> > +	 *
> > +	 * Completing a REF_PHANDLE marker consists in setting those fields.
> > +	 */
> > +	for_each_property(node, prop) {
> > +		m = prop->val.markers;
> > +		for_each_marker_of_type(m, REF_PHANDLE) {
> > +			phandle = propval_cell_n(prop,
> > +						 m->offset / sizeof(cell_t));
> > +			complete_ref_and_is_local(dti, &m->ref, &m->is_local,
> > +						  phandle);
> > +		}
> > +	}
> > +
> > +	for_each_child(node, c)
> > +		complete_references_internal(dti, c);  
> 
> Of course the change suggested above would mean that you need two
> different cases in the callers as well.  I think that's worth it:
> treating this as symmetric when we should know going in what direction
> _all_ the fixups will go seems odd to me.  Or more concretely, I'm
> concerned that if some bug means we get a marker in bogus state
> (e.g. ref valid when coming from dtb) this approach means we'll carry
> on sort of working, when it would be preferable to catch the bad state
> and die earlier.

ref can be valid when coming from dtb: FDT_PROPDATA_PHANDLE_REF.

Anyway, I understood the idea and will see what I can do for the next
iteration.

The is_local field will be removed (already discussed) and so, only the
ref field is present.
 - ref NULL: dtb only
 - ref non NULL: dtb and dts

Not sure that two functions will be needed. Will see when I remove is_local
but I keep your feedback in mind.

Best regards,
Hervé