Re: [PATCH 5/5] OF: Introduce utility helper functions

[Grant Likely <grant.likely@secretlab.ca>]

On Tue, 12 Nov 2013 11:39:08 +0100, Pantelis Antoniou <panto@antoniou-consulting.com> 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@antoniou-consulting.com> 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@antoniou-consulting.com>
> > 
> > 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.