Message-ID: <CANk1AXQScT_b1_UOiVuWNcB=LiA3gvvZrw53H1XVSCNqsGPqtA@mail.gmail.com>
Date:   Tue, 9 Oct 2018 15:28:35 -0500
From:   Alan Tull <atull@...nel.org>
To:     Frank Rowand <frowand.list@...il.com>
Cc:     Rob Herring <robh+dt@...nel.org>,
        Pantelis Antoniou <pantelis.antoniou@...sulko.com>,
        Michael Ellerman <mpe@...erman.id.au>,
        Benjamin Herrenschmidt <benh@...nel.crashing.org>,
        Paul Mackerras <paulus@...ba.org>,
        Moritz Fischer <mdf@...nel.org>,
        linux-kernel <linux-kernel@...r.kernel.org>,
        linuxppc-dev <linuxppc-dev@...ts.ozlabs.org>,
        "open list:OPEN FIRMWARE AND FLATTENED DEVICE TREE BINDINGS" 
        <devicetree@...r.kernel.org>, linux-fpga@...r.kernel.org
Subject: Re: [PATCH 05/16] of: overlay: use prop add changeset entry for
 property in new nodes

On Thu, Oct 4, 2018 at 11:14 PM <frowand.list@...il.com> wrote:
>
> From: Frank Rowand <frank.rowand@...y.com>
>

Hi Frank,

> The changeset entry 'update property' was used for new properties in
> an overlay instead of 'add property'.
>
> The decision of whether to use 'update property' was based on whether
> the property already exists in the subtree where the node is being
> spliced into.  At the top level of creating a changeset describing the
> overlay, the target node is in the live devicetree, so checking whether
> the property exists in the target node returns the correct result.
> As soon as the changeset creation algorithm recurses into a new node,
> the target is no longer in the live devicetree, but is instead in the
> detached overlay tree, thus all properties are incorrectly found to
> already exist in the target.

When I applied an overlay (that added a few gpio controllers, etc),
the node names for nodes added from the overlay end up NULL.   It
seems related to this patch and the next one.  I haven't completely
root caused this but if I back out to before this patch, the situation
is fixed.

root@...ia10:~/unit_tests# ls /sys/bus/platform/drivers/altera_gpio/
bind             ff200010.<NULL>  ff200020.<NULL>  ff200030.<NULL>
uevent           unbind

root@...ia10:~/unit_tests# ls /sys/bus/platform/drivers/altera_freeze_br/
bind             ff200450.<NULL>  uevent           unbind

Alan

>
> This fix will expose another devicetree bug that will be fixed
> in the following patch in the series.
>
> When this patch is applied the errors reported by the devictree
> unittest will change, and the unittest results will change from:
>
>    ### dt-test ### end of unittest - 210 passed, 0 failed
>
> to
>
>    ### dt-test ### end of unittest - 203 passed, 7 failed
>
> Signed-off-by: Frank Rowand <frank.rowand@...y.com>
> ---
>  drivers/of/overlay.c | 112 ++++++++++++++++++++++++++++++++++-----------------
>  1 file changed, 74 insertions(+), 38 deletions(-)
>
> diff --git a/drivers/of/overlay.c b/drivers/of/overlay.c
> index 32cfee68f2e3..0b0904f44bc7 100644
> --- a/drivers/of/overlay.c
> +++ b/drivers/of/overlay.c
> @@ -24,6 +24,26 @@
>  #include "of_private.h"
>
>  /**
> + * struct target - info about current target node as recursing through overlay
> + * @np:                        node where current level of overlay will be applied
> + * @in_livetree:       @np is a node in the live devicetree
> + *
> + * Used in the algorithm to create the portion of a changeset that describes
> + * an overlay fragment, which is a devicetree subtree.  Initially @np is a node
> + * in the live devicetree where the overlay subtree is targeted to be grafted
> + * into.  When recursing to the next level of the overlay subtree, the target
> + * also recurses to the next level of the live devicetree, as long as overlay
> + * subtree node also exists in the live devicetree.  When a node in the overlay
> + * subtree does not exist at the same level in the live devicetree, target->np
> + * points to a newly allocated node, and all subsequent targets in the subtree
> + * will be newly allocated nodes.
> + */
> +struct target {
> +       struct device_node *np;
> +       bool in_livetree;
> +};
> +
> +/**
>   * struct fragment - info about fragment nodes in overlay expanded device tree
>   * @target:    target of the overlay operation
>   * @overlay:   pointer to the __overlay__ node
> @@ -72,8 +92,7 @@ static int devicetree_corrupt(void)
>  }
>
>  static int build_changeset_next_level(struct overlay_changeset *ovcs,
> -               struct device_node *target_node,
> -               const struct device_node *overlay_node);
> +               struct target *target, const struct device_node *overlay_node);
>
>  /*
>   * of_resolve_phandles() finds the largest phandle in the live tree.
> @@ -257,14 +276,17 @@ static struct property *dup_and_fixup_symbol_prop(
>  /**
>   * add_changeset_property() - add @overlay_prop to overlay changeset
>   * @ovcs:              overlay changeset
> - * @target_node:       where to place @overlay_prop in live tree
> + * @target:            where @overlay_prop will be placed
>   * @overlay_prop:      property to add or update, from overlay tree
>   * @is_symbols_prop:   1 if @overlay_prop is from node "/__symbols__"
>   *
> - * If @overlay_prop does not already exist in @target_node, add changeset entry
> - * to add @overlay_prop in @target_node, else add changeset entry to update
> + * If @overlay_prop does not already exist in live devicetree, add changeset
> + * entry to add @overlay_prop in @target, else add changeset entry to update
>   * value of @overlay_prop.
>   *
> + * @target may be either in the live devicetree or in a new subtree that
> + * is contained in the changeset.
> + *
>   * Some special properties are not updated (no error returned).
>   *
>   * Update of property in symbols node is not allowed.
> @@ -273,20 +295,22 @@ static struct property *dup_and_fixup_symbol_prop(
>   * invalid @overlay.
>   */
>  static int add_changeset_property(struct overlay_changeset *ovcs,
> -               struct device_node *target_node,
> -               struct property *overlay_prop,
> +               struct target *target, struct property *overlay_prop,
>                 bool is_symbols_prop)
>  {
>         struct property *new_prop = NULL, *prop;
>         int ret = 0;
>
> -       prop = of_find_property(target_node, overlay_prop->name, NULL);
> -
>         if (!of_prop_cmp(overlay_prop->name, "name") ||
>             !of_prop_cmp(overlay_prop->name, "phandle") ||
>             !of_prop_cmp(overlay_prop->name, "linux,phandle"))
>                 return 0;
>
> +       if (target->in_livetree)
> +               prop = of_find_property(target->np, overlay_prop->name, NULL);
> +       else
> +               prop = NULL;
> +
>         if (is_symbols_prop) {
>                 if (prop)
>                         return -EINVAL;
> @@ -299,10 +323,10 @@ static int add_changeset_property(struct overlay_changeset *ovcs,
>                 return -ENOMEM;
>
>         if (!prop)
> -               ret = of_changeset_add_property(&ovcs->cset, target_node,
> +               ret = of_changeset_add_property(&ovcs->cset, target->np,
>                                                 new_prop);
>         else
> -               ret = of_changeset_update_property(&ovcs->cset, target_node,
> +               ret = of_changeset_update_property(&ovcs->cset, target->np,
>                                                    new_prop);
>
>         if (ret) {
> @@ -315,14 +339,14 @@ static int add_changeset_property(struct overlay_changeset *ovcs,
>
>  /**
>   * add_changeset_node() - add @node (and children) to overlay changeset
> - * @ovcs:              overlay changeset
> - * @target_node:       where to place @node in live tree
> - * @node:              node from within overlay device tree fragment
> + * @ovcs:      overlay changeset
> + * @target:    where @overlay_prop will be placed in live tree or changeset
> + * @node:      node from within overlay device tree fragment
>   *
> - * If @node does not already exist in @target_node, add changeset entry
> - * to add @node in @target_node.
> + * If @node does not already exist in @target, add changeset entry
> + * to add @node in @target.
>   *
> - * If @node already exists in @target_node, and the existing node has
> + * If @node already exists in @target, and the existing node has
>   * a phandle, the overlay node is not allowed to have a phandle.
>   *
>   * If @node has child nodes, add the children recursively via
> @@ -355,15 +379,16 @@ static int add_changeset_property(struct overlay_changeset *ovcs,
>   * invalid @overlay.
>   */
>  static int add_changeset_node(struct overlay_changeset *ovcs,
> -               struct device_node *target_node, struct device_node *node)
> +               struct target *target, struct device_node *node)
>  {
>         const char *node_kbasename;
>         struct device_node *tchild;
> +       struct target target_child;
>         int ret = 0;
>
>         node_kbasename = kbasename(node->full_name);
>
> -       for_each_child_of_node(target_node, tchild)
> +       for_each_child_of_node(target->np, tchild)
>                 if (!of_node_cmp(node_kbasename, kbasename(tchild->full_name)))
>                         break;
>
> @@ -372,22 +397,28 @@ static int add_changeset_node(struct overlay_changeset *ovcs,
>                 if (!tchild)
>                         return -ENOMEM;
>
> -               tchild->parent = target_node;
> +               tchild->parent = target->np;
>                 of_node_set_flag(tchild, OF_OVERLAY);
>
>                 ret = of_changeset_attach_node(&ovcs->cset, tchild);
>                 if (ret)
>                         return ret;
>
> -               ret = build_changeset_next_level(ovcs, tchild, node);
> +               target_child.np = tchild;
> +               target_child.in_livetree = false;
> +
> +               ret = build_changeset_next_level(ovcs, &target_child, node);
>                 of_node_put(tchild);
>                 return ret;
>         }
>
> -       if (node->phandle && tchild->phandle)
> +       if (node->phandle && tchild->phandle) {
>                 ret = -EINVAL;
> -       else
> -               ret = build_changeset_next_level(ovcs, tchild, node);
> +       } else {
> +               target_child.np = tchild;
> +               target_child.in_livetree = target->in_livetree;
> +               ret = build_changeset_next_level(ovcs, &target_child, node);
> +       }
>         of_node_put(tchild);
>
>         return ret;
> @@ -396,7 +427,7 @@ static int add_changeset_node(struct overlay_changeset *ovcs,
>  /**
>   * build_changeset_next_level() - add level of overlay changeset
>   * @ovcs:              overlay changeset
> - * @target_node:       where to place @overlay_node in live tree
> + * @target:            where to place @overlay_node in live tree
>   * @overlay_node:      node from within an overlay device tree fragment
>   *
>   * Add the properties (if any) and nodes (if any) from @overlay_node to the
> @@ -409,27 +440,26 @@ static int add_changeset_node(struct overlay_changeset *ovcs,
>   * invalid @overlay_node.
>   */
>  static int build_changeset_next_level(struct overlay_changeset *ovcs,
> -               struct device_node *target_node,
> -               const struct device_node *overlay_node)
> +               struct target *target, const struct device_node *overlay_node)
>  {
>         struct device_node *child;
>         struct property *prop;
>         int ret;
>
>         for_each_property_of_node(overlay_node, prop) {
> -               ret = add_changeset_property(ovcs, target_node, prop, 0);
> +               ret = add_changeset_property(ovcs, target, prop, 0);
>                 if (ret) {
>                         pr_debug("Failed to apply prop @%pOF/%s, err=%d\n",
> -                                target_node, prop->name, ret);
> +                                target->np, prop->name, ret);
>                         return ret;
>                 }
>         }
>
>         for_each_child_of_node(overlay_node, child) {
> -               ret = add_changeset_node(ovcs, target_node, child);
> +               ret = add_changeset_node(ovcs, target, child);
>                 if (ret) {
>                         pr_debug("Failed to apply node @%pOF/%s, err=%d\n",
> -                                target_node, child->name, ret);
> +                                target->np, child->name, ret);
>                         of_node_put(child);
>                         return ret;
>                 }
> @@ -442,17 +472,17 @@ static int build_changeset_next_level(struct overlay_changeset *ovcs,
>   * Add the properties from __overlay__ node to the @ovcs->cset changeset.
>   */
>  static int build_changeset_symbols_node(struct overlay_changeset *ovcs,
> -               struct device_node *target_node,
> +               struct target *target,
>                 const struct device_node *overlay_symbols_node)
>  {
>         struct property *prop;
>         int ret;
>
>         for_each_property_of_node(overlay_symbols_node, prop) {
> -               ret = add_changeset_property(ovcs, target_node, prop, 1);
> +               ret = add_changeset_property(ovcs, target, prop, 1);
>                 if (ret) {
>                         pr_debug("Failed to apply prop @%pOF/%s, err=%d\n",
> -                                target_node, prop->name, ret);
> +                                target->np, prop->name, ret);
>                         return ret;
>                 }
>         }
> @@ -475,6 +505,7 @@ static int build_changeset_symbols_node(struct overlay_changeset *ovcs,
>  static int build_changeset(struct overlay_changeset *ovcs)
>  {
>         struct fragment *fragment;
> +       struct target target;
>         int fragments_count, i, ret;
>
>         /*
> @@ -489,7 +520,9 @@ static int build_changeset(struct overlay_changeset *ovcs)
>         for (i = 0; i < fragments_count; i++) {
>                 fragment = &ovcs->fragments[i];
>
> -               ret = build_changeset_next_level(ovcs, fragment->target,
> +               target.np = fragment->target;
> +               target.in_livetree = true;
> +               ret = build_changeset_next_level(ovcs, &target,
>                                                  fragment->overlay);
>                 if (ret) {
>                         pr_debug("apply failed '%pOF'\n", fragment->target);
> @@ -499,7 +532,10 @@ static int build_changeset(struct overlay_changeset *ovcs)
>
>         if (ovcs->symbols_fragment) {
>                 fragment = &ovcs->fragments[ovcs->count - 1];
> -               ret = build_changeset_symbols_node(ovcs, fragment->target,
> +
> +               target.np = fragment->target;
> +               target.in_livetree = true;
> +               ret = build_changeset_symbols_node(ovcs, &target,
>                                                    fragment->overlay);
>                 if (ret) {
>                         pr_debug("apply failed '%pOF'\n", fragment->target);
> @@ -517,7 +553,7 @@ static int build_changeset(struct overlay_changeset *ovcs)
>   * 1) "target" property containing the phandle of the target
>   * 2) "target-path" property containing the path of the target
>   */
> -static struct device_node *find_target_node(struct device_node *info_node)
> +static struct device_node *find_target(struct device_node *info_node)
>  {
>         struct device_node *node;
>         const char *path;
> @@ -623,7 +659,7 @@ static int init_overlay_changeset(struct overlay_changeset *ovcs,
>
>                 fragment = &fragments[cnt];
>                 fragment->overlay = overlay_node;
> -               fragment->target = find_target_node(node);
> +               fragment->target = find_target(node);
>                 if (!fragment->target) {
>                         of_node_put(fragment->overlay);
>                         ret = -EINVAL;
> --
> Frank Rowand <frank.rowand@...y.com>
>

Powered by blists - more mailing lists