On Tue, 12 Nov 2013 11:39:08 +0100, Pantelis Antoniou <panto@xxxxxxxxxxxxxxxxxxxxxxx> wrote:
> Hi Grant,
>
> On Nov 11, 2013, at 5:37 PM, Grant Likely wrote:
>
> > On Tue, 5 Nov 2013 19:50:16 +0200, Pantelis Antoniou <panto@xxxxxxxxxxxxxxxxxxxxxxx> wrote:
> >> Introduce helper functions for working with the live DT tree.
> >>
> >> __of_free_property() frees a dynamically created property
> >> __of_free_tree() recursively frees a device node tree
> >> __of_copy_property() copies a property dynamically
> >> __of_create_empty_node() creates an empty node
> >> __of_find_node_by_full_name() finds the node with the full name
> >> and
> >> of_multi_prop_cmp() performs a multi property compare but without
> >> having to take locks.
> >>
> >> Signed-off-by: Pantelis Antoniou <panto@xxxxxxxxxxxxxxxxxxxxxxx>
> >
> > So, this all looks like private stuff, or stuff that belongs in
> > drivers/of/base.c. Can you move stuff around. I've made more comments
> > below.
> >
>
> Placement is no big issue;
>
> > g.
> >
>
> [snip]
>
> >> + } else {
> >> + pr_warn("%s: node %p cannot be freed; memory is gone\n",
> >> + __func__, node);
> >> + }
> >> +}
> >
> > All of the above is potentially dangerous. There is no way to determine
> > if anything still holds a reference to a node. The proper way to handle
> > removal of properties is to have a release method when the last
> > of_node_put is called.
> >
>
> This is safe, and expected to be called only on a dynamically created tree,
> that's what all the checks against OF_DYNAMIC guard against.
>
> It is not ever meant to be called on an arbitrary tree, created by unflattening
> a blob.
I am talking about when being used on a dynamic tree. The problem is
when a driver or other code holds a reference to a dynamic nodes, but
doesn't release it correctly. The memory must not be freed until all of
the references are relased. OF_DYNAMIC doesn't actually help in that
case, and it is the reason for of_node_get()/of_node_put()
> Perhaps we could have a switch to control whether an unflattened tree is
> created dynamically and then freeing/releasing will work.
>
> kobject-ifcation will require it anyway, don't you agree?
Yes. Kobjectifcation will also take care of the release method.
>
> >> +
> >> +/**
> >> + * __of_copy_property - Copy a property dynamically.
> >> + * @prop: Property to copy
> >> + * @flags: Allocation flags (typically pass GFP_KERNEL)
> >> + *
> >> + * Copy a property by dynamically allocating the memory of both the
> >> + * property stucture and the property name & contents. The property's
> >> + * flags have the OF_DYNAMIC bit set so that we can differentiate between
> >> + * dynamically allocated properties and not.
> >> + * Returns the newly allocated property or NULL on out of memory error.
> >> + */
> >
> > What do you intend the use-case to be for this function? Will the
> > duplicated property be immediately modified? If so, what happens if the
> > property needs to be grown in size?
> >
>
> No, the property will no be modified. If it needs to grow it will be moved to
> deadprops (since we don't track refs to props) and a new one will be allocated.
>
> >> +struct property *__of_copy_property(const struct property *prop, gfp_t flags)
> >> +{
> >> + struct property *propn;
> >> +
> >> + propn = kzalloc(sizeof(*prop), flags);
> >> + if (propn == NULL)
> >> + return NULL;
> >> +
> >> + propn->name = kstrdup(prop->name, flags);
> >> + if (propn->name == NULL)
> >> + goto err_fail_name;
> >> +
> >> + if (prop->length > 0) {
> >> + propn->value = kmalloc(prop->length, flags);
> >> + if (propn->value == NULL)
> >> + goto err_fail_value;
> >> + memcpy(propn->value, prop->value, prop->length);
> >> + propn->length = prop->length;
> >> + }
> >> +
> >> + /* mark the property as dynamic */
> >> + of_property_set_flag(propn, OF_DYNAMIC);
> >> +
> >> + return propn;
> >> +
> >> +err_fail_value:
> >> + kfree(propn->name);
> >> +err_fail_name:
> >> + kfree(propn);
> >> + return NULL;
> >> +}
> >> +
> >> +/**
> >> + * __of_create_empty_node - Create an empty device node dynamically.
> >> + * @name: Name of the new device node
> >> + * @type: Type of the new device node
> >> + * @full_name: Full name of the new device node
> >> + * @phandle: Phandle of the new device node
> >> + * @flags: Allocation flags (typically pass GFP_KERNEL)
> >> + *
> >> + * Create an empty device tree node, suitable for further modification.
> >> + * The node data are dynamically allocated and all the node flags
> >> + * have the OF_DYNAMIC & OF_DETACHED bits set.
> >> + * Returns the newly allocated node or NULL on out of memory error.
> >> + */
> >> +struct device_node *__of_create_empty_node(
> >> + const char *name, const char *type, const char *full_name,
> >> + phandle phandle, gfp_t flags)
> >
> > I would like to see a user of this function in the core DT paths that
> > allocate nodes. It will make for less chance of breakage if the fdt and
> > pdt paths change something, but this function isn't updated.
> >
>
> Hmm, where do you think it can be used? Perhaps the unflatten parts?
Yes. In unflattening the tree, fdt.c and in extracting the trea from
real OFW (pdt.c).
>
> > g.
> >
> >> +{
> >> + struct device_node *node;
> >> +
> >> + node = kzalloc(sizeof(*node), flags);
> >> + if (node == NULL)
> >> + return NULL;
> >> +
> >> + node->name = kstrdup(name, flags);
> >> + if (node->name == NULL)
> >> + goto err_return;
> >> +
> >> + node->type = kstrdup(type, flags);
> >> + if (node->type == NULL)
> >> + goto err_return;
> >> +
> >> + node->full_name = kstrdup(full_name, flags);
> >> + if (node->type == NULL)
> >> + goto err_return;
> >
> > Again, who do you expect the user of this function to be? If it is part
> > of unflattening an overlay tree, is there a reason that the passed in
> > names cannot be used directly instead of kmallocing them?
> >
>
> I want to be able to get rid of the blob eventually; I don't need to keep
> dragging it around.
Why? It really doesn't hurt and it means data does not need to be
copied.
>
> >> +
> >> + node->phandle = phandle;
> >> + kref_init(&node->kref);
> >> + of_node_set_flag(node, OF_DYNAMIC);
> >> + of_node_set_flag(node, OF_DETACHED);
> >> +
> >> + return node;
> >> +
> >> +err_return:
> >> + __of_free_tree(node);
> >> + return NULL;
> >> +}
> >> +
> >> +/**
> >> + * __of_find_node_by_full_name - Find a node with the full name recursively
> >> + * @node: Root of the tree to perform the search
> >> + * @full_name: Full name of the node to find.
> >> + *
> >> + * Find a node with the give full name by recursively following any of
> >> + * the child node links.
> >> + * Returns the matching node, or NULL if not found.
> >> + * Note that the devtree lock is not taken, so this function is only
> >> + * safe to call on either detached trees, or when devtree lock is already
> >> + * taken.
> >> + */
> >> +struct device_node *__of_find_node_by_full_name(struct device_node *node,
> >> + const char *full_name)
> >
> > Sounds like something that should be in drivers/of/base.c
> >
>
> Yes.
>
> >> +{
> >> + struct device_node *child, *found;
> >> +
> >> + if (node == NULL)
> >> + return NULL;
> >> +
> >> + /* check */
> >> + if (of_node_cmp(node->full_name, full_name) == 0)
> >> + return node;
> >> +
> >> + __for_each_child_of_node(node, child) {
> >> + found = __of_find_node_by_full_name(child, full_name);
> >> + if (found != NULL)
> >> + return found;
> >> + }
> >
> > I'm not a huge fan of recursive calls. Why doesn't a slightly modified
> > of_fund_node_by_name() work here?
> >
> > I agree that of_find_node_by_name is not particularly elegant and it
> > would be good to have something more efficient, but it works and
> > following the same method would be consistent.
> >
>
> of_find_node_by_name is not iterating on the passed tree and it's subnodes,
> it is a global search starting from:
>
> np = from ? from->allnext : of_allnodes;
>
> This is just broken, since it depends on unflattening creating nodes in the
> allnodes list in sequence. I.e. that child nodes are after the parent node.
> This assumption breaks down when doing dynamic insertions of nodes.
> A detached tree in not linked on of_allnodes anyway.
It would be good to have a root-of-tree structure for both the core tree
and the overlay trees so that a common iterator can be implemented.
g.
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
