Re: [PATCH v2 07/15] dtc: Move label-reference handling out of the grammar

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

Hi David, Simon,

On Wed,  4 Nov 2026 14:17:46 -0700
Simon Glass <sjg@chromium.org> wrote:

> The grammar action for a top-level &label { ... } holds its merge logic,
> including making an overlay fragment in a plugin when the label is not
> found.
> 
> Move it into a new merge_ref(), passing it a struct fragment for the
> reference and node.
> 
> The output does not change.
> 
> Assisted-by: Claude:claude-opus-5-5
> Signed-off-by: Simon Glass <sjg@chromium.org>
> ---
> 
> Changes in v2:
> - Rename merge_label_ref() to merge_ref()
> - Build the fragment in one step, with build_fragment_ref()
> - Return true on success from merge_ref()
> - Check the fragment type with assert()
> 
>  dtc-parser.y | 23 +++++++++--------------
>  dtc.h        |  1 +
>  livetree.c   | 26 ++++++++++++++++++++++++++
>  3 files changed, 36 insertions(+), 14 deletions(-)
> 
> diff --git a/dtc-parser.y b/dtc-parser.y
> index 9b9b6db..c58a66d 100644
> --- a/dtc-parser.y
> +++ b/dtc-parser.y
> @@ -214,21 +214,16 @@ devicetree:
>  		}
>  	| devicetree DT_LABEL_REF nodedef
>  		{
> -			struct node *target = get_node_by_ref($1, $2);
> +			struct fragment *f;
>  
> -			if (target) {
> -				merge_nodes(target, $3);
> -			} else {
> -				/*
> -				 * We rely on the rule being always:
> -				 *   versioninfo plugindecl memreserves devicetree
> -				 * so $-1 is what we want (plugindecl)
> -				 */
> -				if ($<flags>-1 & DTSF_PLUGIN)
> -					add_orphan_node($1, $3, $2);
> -				else
> -					ERROR(&@2, "Label or path %s not found", $2);
> -			}
> +			f = build_fragment_ref(NULL, NULL, $2, &@2, $3);
> +			/*
> +			 * We rely on the rule being always:
> +			 *   versioninfo plugindecl memreserves devicetree
> +			 * so $-1 is what we want (plugindecl)
> +			 */
> +			if (!merge_ref($1, f, $<flags>-1 & DTSF_PLUGIN))
> +				treesource_error = true;
>  			$$ = $1;
>  		}
>  	| devicetree DT_DEL_NODE dt_ref ';'
> diff --git a/dtc.h b/dtc.h
> index 8c4d7bf..9b4389b 100644
> --- a/dtc.h
> +++ b/dtc.h
> @@ -373,6 +373,7 @@ struct fragment *build_fragment_omit_if_no_ref(char *ref,
>  struct fragment *append_fragment(struct fragment *list, struct fragment *new);
>  void free_fragment(struct fragment *f);
>  bool is_ref_relative(const char *ref);
> +bool merge_ref(struct node *dt, struct fragment *f, bool plugin);
>  bool merge_delete_node(struct node *dt, struct fragment *f);
>  bool merge_omit_if_no_ref(struct node *dt, struct fragment *f);
>  
> diff --git a/livetree.c b/livetree.c
> index 3ff4e14..179866d 100644
> --- a/livetree.c
> +++ b/livetree.c
> @@ -573,6 +573,32 @@ bool is_ref_relative(const char *ref)
>  	return ref[0] != '/' && strchr(&ref[1], '/');
>  }
>  
> +/*
> + * Merge a fragment which refers to a label into the node with that label. In
> + * a plugin, a label which is not found becomes an overlay fragment.
> + *
> + * Returns true on success, false on an error, which has been reported.
> + */
> +bool merge_ref(struct node *dt, struct fragment *f, bool plugin)
> +{
> +	struct node *target = get_node_by_ref(dt, f->ref);
> +
> +	assert(f->type == FRAGMENT_NODE);
> +	if (target) {
> +		merge_nodes(target, f->node);
> +		return true;
> +	}
> +	if (!plugin) {
> +		srcpos_error(f->ref_srcpos, "Error",
> +			     "Label or path %s not found", f->ref);
> +		return false;
> +	}
> +	/* The label may be in the base tree, so add a fragment@N node for it */
> +	add_orphan_node(dt, f->node, f->ref);
> +
> +	return true;
> +}
> +

Even if this function is slightly modified in later patches, I think it mixes
two different operations:
 - Merging fragments
 - Converting fragments in overlay output format.

IHMO, those two operations should be fully decoupled.

merge_ref() or any other operation whose goal is to merge a fragment should
only merge fragment either in the existing root node or in a previous fragment
available in the fragment list.

add_orphan_node() should not be called from a merge_*() function related to
fragments.

After merging all fragment we should have a root node and/or a list of fragment
reduced to unmerged ones.

Base on that, the overlay conversion can be performed using a dedicated
function, out of merge_*() functions.

Something like this:
---- 8< ----
void convert_to_overlay_output_format(struct dt_info *dti)
{
	struct fragment *f;

	if (!dti->dt) {
		/* The root node is not present. Create an empty one */
		dti->dt = ...;
	}

	/*
	 * The fragment list have been previously merged.
	 * Available fragment left in the list are fragments
	 * that cannot be merged and so those fragments are
	 * orphan nodes.
	 *
	 * Convert those orphan nodes to the /fragment@N format overlay
	 * format.
	 */
	for (f = dti->fragments; f; f = next) {
		add_orphan_node(dti->dt, f->node, f->ref);
	
	/* Add __fixups__ and __local_fixups__ nodes ? */
	todo();
}

in main():

	/* Parse input */
	dti = dt_from_*();

	checks_pre_fragment_merge(dti);

	merge_fragments(dti); /* <--- Fragments are merged.
			       *      Remaining items in the fragment list, if
			       *      any are Orphan nodes (i.e. fragments that
			       *      couldn't be merged).
			       */

	checks_post_fragment_merge(dti); <--- Orphan ok for overlays but ko for !overlay dts
	
	/* Convert outputs */
	if (dti->dtsflags & DTSF_PLUGIN) {
		convert_to_overlay_output_format(dti);
		checks_overlay_format(dti);  /* <--- Not sure it is needed */ 
	}

	/* Generate output */
	dt_to_*();
}
--- 8< ---

Best regards,
Hervé