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

On Tue, Jun 03, 2025 at 09:09:12PM +1000, David Gibson wrote:
> On Fri, May 30, 2025 at 05:58:58PM +0530, Wasim Nazir wrote:
> > 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.
> 
> Ah, I see.  I can see the reason now, but it's still a deeply ugly
> interface.
> 
> Hmm... why do you need to ever expand the applied size?  It's obvious
> you'd need to expand the based dtbo, but not why you'd ever need to
> expand the one you're applying on top.

Since, we are updating phandle references of fragments & __local_fixups__ of
2nd dtbo so we need to check that.

> 
> > 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.
> 
> No, I think you have it.  The comment references a "base device tree",
> i.e. not a dtbo, but that's no longer accurate.

Oh ok, got it.

> 
> > > > +	 * 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                                */
> > > >  /**********************************************************************/
> > > 
> > 
> > Regards,
> > Wasim
> > 
> 
> -- 
> 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