Message-Id: <89491720-464B-47EC-A888-9CDE29B035F5@antoniou-consulting.com>
Date:	Tue, 12 Nov 2013 11:39:08 +0100
From:	Pantelis Antoniou <panto@...oniou-consulting.com>
To:	Grant Likely <grant.likely@...retlab.ca>
Cc:	Rob Herring <robherring2@...il.com>,
	Stephen Warren <swarren@...dotorg.org>,
	Matt Porter <matt.porter@...aro.org>,
	Koen Kooi <koen@...inion.thruhere.net>,
	Alison Chaiken <Alison_Chaiken@...tor.com>,
	Dinh Nguyen <dinh.linux@...il.com>,
	Jan Lubbe <jluebbe@...net.de>,
	Alexander Sverdlin <alexander.sverdlin@....com>,
	Michael Stickel <ms@...able.de>,
	Guenter Roeck <linux@...ck-us.net>,
	Dirk Behme <dirk.behme@...il.com>,
	Alan Tull <delicious.quinoa@...il.com>,
	Sascha Hauer <s.hauer@...gutronix.de>,
	Michael Bohan <mbohan@...eaurora.org>,
	Ionut Nicu <ioan.nicu.ext@....com>,
	Michal Simek <monstr@...str.eu>,
	Matt Ranostay <mranostay@...il.com>,
	devicetree@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH 5/5] OF: Introduce utility helper functions

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@...oniou-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@...oniou-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.

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? 

>> +
>> +/**
>> + * __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?

> 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.

>> +
>> +	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.

>> +
>> +	return NULL;
>> +}
>> +
>> +/**
>> + * of_multi_prop_cmp - Check if a property matches a value
>> + * @prop:	Property to check
>> + * @value:	Value to check against
>> + *
>> + * Check whether a property matches a value, using the standard
>> + * of_compat_cmp() test on each string. It is similar to the test
> 
> of_compat_cmp() is only for compatible strings, and it purely to paper
> over the way different OFW implementations work. Don't use the same
> function. Don't use it for any property other than compatible because
> that will just encourage bad behaviour.
> 

OK

>> + * of_device_is_compatible() makes, but it can be performed without
>> + * taking the devtree_lock, which is required in some cases.
>> + * Returns 0 on a match, -1 on no match.
> 
> That's what __of_device_is_compatible() is for. It is a version of the
> function that doesn't take the lock.

I see. The original version of the patches started before this was
available. Will remove.

> 
>> + */
>> +int of_multi_prop_cmp(const struct property *prop, const char *value)
>> +{
>> +	const char *cp;
>> +

[snip]

Regards

-- Pantelis

--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@...r.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/

Powered by blists - more mailing lists