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

[David Gibson <david@gibson.dropbear.id.au>]

[-- Attachment #1: Type: text/plain, Size: 5604 bytes --]

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 <vatsa@codeaurora.org>
> > > 
> > > 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 <vatsa@codeaurora.org>
> > > Signed-off-by: Wasim Nazir <quic_wasimn@quicinc.com>
> > > ---
> > >  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.

> 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.

> > > +	 * 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

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]