Re: [PATCH v2 7/9] libfdt: Add overlay application function

[Maxime Ripard <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>]

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

Hi Pantelis,

On Fri, Jun 10, 2016 at 05:28:11PM +0300, Pantelis Antoniou wrote:
> Hi Maxime,
> 
> > On May 27, 2016, at 12:13 , Maxime Ripard <maxime.ripard@free-electrons.com> wrote:
> > 
> > The device tree overlays are a good way to deal with user-modifyable
> > boards or boards with some kind of an expansion mechanism where we can
> > easily plug new board in (like the BBB, the Raspberry Pi or the CHIP).
> > 
> > Add a new function to merge overlays with a base device tree.
> > 
> > Signed-off-by: Maxime Ripard <maxime.ripard-wi1+55ScJUtKEb57/3fJTNBPR1lH4CV8@public.gmane.org>
> > ---
> > include/libfdt.h         |  30 ++++
> > lib/libfdt/Makefile      |   2 +-
> > lib/libfdt/fdt_overlay.c | 414 +++++++++++++++++++++++++++++++++++++++++++++++
> > 3 files changed, 445 insertions(+), 1 deletion(-)
> > create mode 100644 lib/libfdt/fdt_overlay.c
> > 
> > diff --git a/include/libfdt.h b/include/libfdt.h
> > index 1e01b2c7ebdf..783067e841a1 100644
> > --- a/include/libfdt.h
> > +++ b/include/libfdt.h
> > @@ -1698,6 +1698,36 @@ int fdt_add_subnode(void *fdt, int parentoffset, const char *name);
> >  */
> > int fdt_del_node(void *fdt, int nodeoffset);
> > 
> > +/**
> > + * fdt_overlay_apply - Applies a DT overlay on a base DT
> > + * @fdt: pointer to the base device tree blob
> > + * @fdto: pointer to the device tree overlay blob
> > + *
> > + * fdt_overlay_apply() will apply the given device tree overlay on the
> > + * given base device tree.
> > + *
> > + * Expect the base device tree to be modified, even if the function
> > + * returns an error.
> > + *
> 
> If the base tree has been modified on an error it is unsafe to use it
> for booting. A valid strategy would be to scribble over the DT magic
> number so that that blob is invalidated.
> 
> What are the other people’s opinion on this?

That would probably be safer yes, I'll change that.

> > +static int fdt_overlay_get_target(const void *fdt, const void *fdto,
> > +				  int fragment)
> > +{
> > +	uint32_t phandle;
> > +	const char *path;
> > +
> > +	/* Try first to do a phandle based lookup */
> > +	phandle = fdt_overlay_get_target_phandle(fdto, fragment);
> > +	if (phandle)
> > +		return fdt_node_offset_by_phandle(fdt, phandle);
> > +
> > +	/* And then a path based lookup */
> > +	path = fdt_getprop(fdto, fragment, "target-path", NULL);
> > +	if (!path)
> > +		return -FDT_ERR_NOTFOUND;
> > +
> > +	return fdt_path_offset(fdt, path);
> > +}
> > +
> 
> Those targets are fine; beware there are patches with more target options.

Ack.

> > +static int fdt_overlay_merge(void *dt, void *dto)
> > +{
> > +	int root, fragment;
> > +
> > +	root = fdt_path_offset(dto, "/");
> > +	if (root < 0)
> > +		return root;
> > +
> > +	fdt_for_each_subnode(dto, fragment, root) {
> > +		const char *name = fdt_get_name(dto, fragment, NULL);
> > +		uint32_t target;
> > +		int overlay;
> > +		int ret;
> > +
> > +		if (strncmp(name, "fragment", 8))
> > +			continue;
> > +
> 
> This is incorrect. The use of “fragment” is a convention only.
> The real test whether the node is an overlay fragment is that
> it contains a target property.
> 
> > +		target = fdt_overlay_get_target(dt, dto, fragment);
> > +		if (target < 0)
> > +			return target;
> > +
> 
> So you could do ‘if (target < 0) continue;’ or handle a more complex error code.

Ok, will change.

> I would caution against the liberal use of malloc in libfdt. We’re
> possibly running in a constrained environment; a custom extents
> based (non freeing) allocator should be better.

David had the same comments, and I will drop the mallocs entirely.

> We need some figures about memory consumption when this is enabled,
> and a CONFIG option to disable it.

The current code (before and after that patch) adds 18kB to a
sandbox_defconfig.

Maxime

-- 
Maxime Ripard, Free Electrons
Embedded Linux, Kernel and Android engineering
http://free-electrons.com

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