On 10/09/18 13:28, Alan Tull wrote:
> 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
I'll look at this tonight.
-Frank
> 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>
>>
>
