Re: [PATCH v3 1/4] libfdt: overlay_merge: Introduce fdt_overlay_merge()

On Wed, May 21, 2025 at 02:23:29PM +1000, David Gibson wrote:
> On Mon, May 19, 2025 at 02:40:40PM +0530, Wasim Nazir wrote:
> > From: Srivatsa Vaddagiri <[email protected]>
> > 
> > fdt_overlay_merge() merges two overlay blobs. It is largely expected to be
> > used offline on a build machine to combine two or more overlay blobs into one.
> > 
> > It is intended to help maintain device-tree overlay code in
> > multiple source repositories, but merge their binary forms (overlay blobs)
> > into one so that bootloader's task of searching for all relevant overlay blobs
> > is simplified.
> > 
> > Introduce fdt_overlay_merge() which is identical to fdt_overlay_apply().
> > Subsequent patches will introduce required changes to merge overlay blobs.
> > 
> > Signed-off-by: Srivatsa Vaddagiri <[email protected]>
> > Signed-off-by: Wasim Nazir <[email protected]>
> > ---
> >  libfdt/fdt_overlay.c | 59 ++++++++++++++++++++++++++++++++++++++++++++
> >  libfdt/libfdt.h      | 18 ++++++++++++++
> >  2 files changed, 77 insertions(+)
> > 
> > diff --git a/libfdt/fdt_overlay.c b/libfdt/fdt_overlay.c
> > index e6b9eb643958..8690ed55c8f6 100644
> > --- a/libfdt/fdt_overlay.c
> > +++ b/libfdt/fdt_overlay.c
> > @@ -1098,3 +1098,62 @@ err:
> > 
> >  	return ret;
> >  }
> > +
> > +int fdt_overlay_merge(void *fdt, void *fdto, int *fdto_nospace)
> 
> The parameters should be renamed to reflect the new semantics.

Sure will change to proper name.

> 
> The 'fdto_nospace' parameter seems weird.  Why not just return
> -FDT_ERR_NOSPACE from the function?

fdto_nospace variable is used to know cases when 2nd fdto (overlaying fdto)
needs more space while -FDT_ERR_NOSPACE is used for cases when
1st fdto (base fdto) needs more space.

I will change the arguments for better clarity.

> 
> > +{
> > +	uint32_t delta = fdt_get_max_phandle(fdt);
> > +	int ret;
> > +
> > +	FDT_RO_PROBE(fdt);
> > +	FDT_RO_PROBE(fdto);
> > +
> > +	*fdto_nospace = 0;
> > +
> > +	ret = overlay_adjust_local_phandles(fdto, delta);
> > +	if (ret) {
> > +		if (ret == -FDT_ERR_NOSPACE)
> > +			*fdto_nospace = 1;
> > +		goto err;
> > +	}
> > +
> > +	ret = overlay_update_local_references(fdto, delta);
> > +	if (ret) {
> > +		if (ret == -FDT_ERR_NOSPACE)
> > +			*fdto_nospace = 1;
> > +		goto err;
> > +	}
> > +
> > +	ret = overlay_fixup_phandles(fdt, fdto);
> > +	if (ret)
> > +		goto err;
> > +
> > +	ret = overlay_merge(fdt, fdto);
> > +	if (ret)
> > +		goto err;
> > +
> > +	ret = overlay_symbol_update(fdt, fdto);
> > +	if (ret)
> > +		goto err;
> > +
> > +	/*
> > +	 * The overlay has been damaged, erase its magic.
> > +	 */
> > +	fdt_set_magic(fdto, ~0);
> > +
> > +	return 0;
> > +
> > +err:
> > +	/*
> > +	 * The overlay might have been damaged, erase its magic.
> > +	 */
> > +	fdt_set_magic(fdto, ~0);
> > +
> > +	/*
> > +	 * The base device tree might have been damaged, erase its
> 
> This comment is no longer accurate in the new context.

New context here is two overlay files, while earlier it was dts and dtbo?
I'm concerned that I may not have understood correctly, can you provide
more details.

> 
> > +	 * magic.
> > +	 */
> > +	if (!*fdto_nospace)
> > +		fdt_set_magic(fdt, ~0);
> > +
> > +	return ret;
> > +}
> > diff --git a/libfdt/libfdt.h b/libfdt/libfdt.h
> > index b5e72001d115..664b141d5334 100644
> > --- a/libfdt/libfdt.h
> > +++ b/libfdt/libfdt.h
> > @@ -2313,6 +2313,24 @@ int fdt_overlay_apply(void *fdt, void *fdto);
> >  int fdt_overlay_target_offset(const void *fdt, const void *fdto,
> >  			      int fragment_offset, char const **pathp);
> > 
> > +/**
> > + * fdt_overlay_merge - Merge two overlays into one
> > + * @fdt: pointer to the first device tree overlay blob
> > + * @fdto: pointer to the second device tree overlay blob
> > + * @fdto_nospace: indicates if FDT_ERR_NOSPACE error code applies to @fdto
> > + *
> > + * fdt_overlay_merge() will merge second overlay blob into first overlay blob.
> > + *
> > + * Expect the first device tree to be modified, even if the function
> > + * returns an error.
> > + *
> > + * returns:
> > + *	0, on success
> > + *	-FDT_ERR_NOSPACE, there's not enough space in first device tree blob
> > + *	-FDT_ERR_BADVALUE
> > + */
> > +int fdt_overlay_merge(void *fdt, void *fdto, int *fdto_nospace);
> > +
> >  /**********************************************************************/
> >  /* Debugging / informational functions                                */
> >  /**********************************************************************/
> 
> -- 
> 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

Regards,
Wasim