On Fri, Jul 14, 2017 at 02:56:37PM +0300, Pantelis Antoniou wrote:
> Hi David,
>
> On Fri, 2017-07-14 at 20:22 +1000, David Gibson wrote:
> > On Thu, Jul 13, 2017 at 12:24:18AM +0300, Pantelis Antoniou wrote:
> > > This patch enables an overlay to refer to a previous overlay's
> > > labels by performing a merge of symbol information at application
> > > time.
> > >
> > > In a nutshell it allows an overlay to refer to a symbol that a previous
> > > overlay has defined. It requires both the base and all the overlays
> > > to be compiled with the -@ command line switch so that symbol
> > > information is included.
> > >
> > > base.dts
> > > --------
> > >
> > > /dts-v1/;
> > > / {
> > > foo: foonode {
> > > foo-property;
> > > };
> > > };
> > >
> > > $ dtc -@ -I dts -O dtb -o base.dtb base.dts
> > >
> > > bar.dts
> > > -------
> > >
> > > /dts-v1/;
> > > /plugin/;
> > > / {
> > > fragment@1 {
> > > target = <&foo>;
> > > __overlay__ {
> > > overlay-1-property;
> > > bar: barnode {
> > > bar-property;
> > > };
> > > };
> > > };
> > > };
> > >
> > > $ dtc -@ -I dts -O dtb -o bar.dtb bar.dts
> > >
> > > baz.dts
> > > -------
> > >
> > > /dts-v1/;
> > > /plugin/;
> > > / {
> > > fragment@1 {
> > > target = <&bar>;
> > > __overlay__ {
> > > overlay-2-property;
> > > baz: baznode {
> > > baz-property;
> > > };
> > > };
> > > };
> > > };
> > >
> > > $ dtc -@ -I dts -O dtb -o baz.dtb baz.dts
> > >
> > > Applying the overlays:
> > >
> > > $ fdtoverlay -i base.dtb -o target.dtb bar.dtb baz.dtb
> > >
> > > Dumping:
> > >
> > > $ fdtdump target.dtb
> > > / {
> > > foonode {
> > > overlay-1-property;
> > > foo-property;
> > > linux,phandle = <0x00000001>;
> > > phandle = <0x00000001>;
> > > barnode {
> > > overlay-2-property;
> > > phandle = <0x00000002>;
> > > linux,phandle = <0x00000002>;
> > > bar-property;
> > > baznode {
> > > phandle = <0x00000003>;
> > > linux,phandle = <0x00000003>;
> > > baz-property;
> > > };
> > > };
> > > };
> > > __symbols__ {
> > > baz = "/foonode/barnode/baznode";
> > > bar = "/foonode/barnode";
> > > foo = "/foonode";
> > > };
> > > };
> > >
> > > Signed-off-by: Pantelis Antoniou <pantelis.antoniou@xxxxxxxxxxxx>
> > > ---
> > > .gitignore | 1 +
> > > libfdt/fdt_overlay.c | 131 ++++++++++++++++++++++++++++++++++++++++++++++++++-
> > > 2 files changed, 131 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/.gitignore b/.gitignore
> > > index 545b899..f333c28 100644
> > > --- a/.gitignore
> > > +++ b/.gitignore
> > > @@ -15,3 +15,4 @@ lex.yy.c
> > > /fdtput
> > > /patches
> > > /.pc
> > > +/fdtoverlay
> > > diff --git a/libfdt/fdt_overlay.c b/libfdt/fdt_overlay.c
> > > index ceb9687..fb17ef2 100644
> > > --- a/libfdt/fdt_overlay.c
> > > +++ b/libfdt/fdt_overlay.c
> > > @@ -590,7 +590,7 @@ static int overlay_apply_node(void *fdt, int target,
> > > *
> > > * overlay_merge() merges an overlay into its base device tree.
> > > *
> > > - * This is the final step in the device tree overlay application
> > > + * This is the next to last step in the device tree overlay application
> > > * process, when all the phandles have been adjusted and resolved and
> > > * you just have to merge overlay into the base device tree.
> > > *
> > > @@ -630,6 +630,131 @@ static int overlay_merge(void *fdt, void *fdto)
> > > return 0;
> > > }
> > >
> > > +/**
> > > + * overlay_symbol_update - Update the symbols of base tree after a merge
> > > + * @fdt: Base Device Tree blob
> > > + * @fdto: Device tree overlay blob
> > > + *
> > > + * overlay_symbol_update() updates the symbols of the base tree with the
> > > + * symbols of the applied overlay
> > > + *
> > > + * This is the last step in the device tree overlay application
> > > + * process, allowing the reference of overlay symbols by subsequent
> > > + * overlay operations.
> > > + *
> > > + * returns:
> > > + * 0 on success
> > > + * Negative error code on failure
> > > + */
> > > +static int overlay_symbol_update(void *fdt, void *fdto)
> > > +{
> > > + int root_sym, ov_sym, prop, path_len, fragment, target;
> > > + int len, frag_name_len, ret, rel_path_len;
> > > + const char *s;
> > > + const char *path;
> > > + const char *name;
> > > + const char *frag_name;
> > > + const char *rel_path;
> > > + char *buf;
> > > + void *p;
> > > +
> > > + root_sym = fdt_subnode_offset(fdt, 0, "__symbols__");
> > > + ov_sym = fdt_subnode_offset(fdto, 0, "__symbols__");
> > > +
> > > + /* if neither exist we can't update symbols, but that's OK */
> > > + if (root_sym < 0 || ov_sym < 0)
> > > + return 0;
> >
> > This isn't correct. If ov_sym isn't there, then indeed there is
> > nothing to do, but if root_sym is not there, you should create it.
> >
>
> OK, although in practice there is almost no chance that a valid tree
> blob will contain no symbols.
Sure, but it's pretty easy to get right, even for that rare case.
> > > +
> > > + /* iterate over each overlay symbol */
> > > + fdt_for_each_property_offset(prop, fdto, ov_sym) {
> > > +
> >
> > Stray blank line.
> >
> > > + path = fdt_getprop_by_offset(fdto, prop, &name, &path_len);
> > > + if (!path)
> > > + return path_len;
> > > +
> > > + /* skip autogenerated properties */
> >
> > Comment isn't correct. *Everything* in __symbols__ will typically be
> > autogenerated. 'name' probably is a special case, but I'm a bit
> > surprised it's there anyway, I though we only generated that for
> > really old dtb versions.
>
> The comment is valid; there are auto generated properties and name is
> one of them. Overlays should work even on really old dtb versions.
No, it's really not. Usually *all* the properites in this node will
be autogenerated, and you're certainly not skipping them all.
And no, overlays really don't need to work with dtbs *that* old. In
dtc, "name" properties are only generated for v3 and earlier dtbs. v3
was obsolete for years before overlays were invented. In fact v3 was
obsolete even before libfdt was created - libfdt won't work with them
at all (see FDT_FIRST_SUPPORTED_VERSION).
It's possible some other tool is adding "name" properties, but in that
case we should reference that tool as the reason, not make a comment
which is both vague and factually inaccurate.
>
> > > + if (!strcmp(name, "name"))
> > > + continue;
> > > +
> > > + /* format: /<fragment-name>/__overlay__/<relative-subnode-path> */
> > > +
> > > + if (*path != '/')
> > > + return -FDT_ERR_BADVALUE;
> > > +
> > > + /* get fragment name first */
> > > + s = strchr(path + 1, '/');
> > > + if (!s)
> > > + return -FDT_ERR_BADVALUE;
> > > +
> > > + frag_name = path + 1;
> > > + frag_name_len = s - path - 1;
> > > +
> > > + /* verify format */
> > > + len = strlen("/__overlay__/");
> > > + if (strncmp(s, "/__overlay__/", len))
> > > + return -FDT_ERR_NOTFOUND;
> >
> > This isn't correct, you're assuming the property is nul terminated
> > (which it should be, but you can't count on). Instead you should
> > compare path_len versus the length of /__overlay__/, then you can just
> > memcmp() to see if the right thing is there.
> >
>
> OK, but could only happen on really badly formatted (even maliciously
> so) properties.
Failing gracefully on corrupted or maliciously constructed blobs is a
design goal for libfdt.
--
David Gibson | 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:
signature.asc
Description: PGP signature
