On Thu, Oct 4, 2018 at 11:14 PM <frowand.list@xxxxxxxxx> wrote:
>
> From: Frank Rowand <frank.rowand@xxxxxxxx>
>
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@arria10:~/unit_tests# ls /sys/bus/platform/drivers/altera_gpio/
bind ff200010.<NULL> ff200020.<NULL> ff200030.<NULL>
uevent unbind
root@arria10:~/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@xxxxxxxx>
> ---
> 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@xxxxxxxx>
>
