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@xxxxxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxxxxx>
> > ---
> > 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:
signature.asc
Description: PGP signature
