Message-Id: <57092A71-2C68-4B92-A748-5D0DE69EC141@antoniou-consulting.com>
Date:	Wed, 6 Nov 2013 11:34:28 +0200
From:	Pantelis Antoniou <panto@...oniou-consulting.com>
To:	Ionut Nicu <ioan.nicu.ext@....com>
Cc:	Grant Likely <grant.likely@...retlab.ca>,
	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>,
	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 Ionut,

On Nov 6, 2013, at 11:21 AM, Ionut Nicu wrote:

> Hi,
> 
> First of all, good to see this patch set being submitted again!
> 
> We're using an older version of your patch set for some time and
> they're working good for us.
> 

Thanks, that's good to know.

> On 05.11.2013 18:50, ext Pantelis Antoniou 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>
>> ---
>> drivers/of/Makefile |   2 +-
>> drivers/of/util.c   | 253 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>> include/linux/of.h  |  59 ++++++++++++
>> 3 files changed, 313 insertions(+), 1 deletion(-)
>> create mode 100644 drivers/of/util.c
>> 
>> diff --git a/drivers/of/Makefile b/drivers/of/Makefile
>> index efd0510..9bc6d8c 100644
>> --- a/drivers/of/Makefile
>> +++ b/drivers/of/Makefile
>> @@ -1,4 +1,4 @@
>> -obj-y = base.o device.o platform.o
>> +obj-y = base.o device.o platform.o util.o
>> obj-$(CONFIG_OF_FLATTREE) += fdt.o
>> obj-$(CONFIG_OF_PROMTREE) += pdt.o
>> obj-$(CONFIG_OF_ADDRESS)  += address.o
>> diff --git a/drivers/of/util.c b/drivers/of/util.c
>> new file mode 100644
>> index 0000000..5117e2b
>> --- /dev/null
>> +++ b/drivers/of/util.c
>> @@ -0,0 +1,253 @@
>> +/*
>> + * Utility functions for working with device tree(s)
>> + *
>> + * Copyright (C) 2012 Pantelis Antoniou <panto@...oniou-consulting.com>
>> + * Copyright (C) 2012 Texas Instruments Inc.
>> + *
>> + * This program is free software; you can redistribute it and/or
>> + * modify it under the terms of the GNU General Public License
>> + * version 2 as published by the Free Software Foundation.
>> + */
>> +
>> +#include <linux/kernel.h>
>> +#include <linux/module.h>
>> +#include <linux/of.h>
>> +#include <linux/of_device.h>
>> +#include <linux/string.h>
>> +#include <linux/ctype.h>
>> +#include <linux/errno.h>
>> +#include <linux/string.h>
>> +#include <linux/slab.h>
>> +#include <linux/err.h>
>> +
>> +/**
>> + * __of_free_property - release the memory of an allocated property
>> + * @prop:	Property to release
>> + *
>> + * Release the memory of an allocated property only after checking
>> + * that the property has been marked as OF_DYNAMIC.
>> + * Only call on known allocated properties.
>> + */
>> +void __of_free_property(struct property *prop)
>> +{
>> +	if (prop == NULL)
>> +		return;
>> +
>> +	if (of_property_check_flag(prop, OF_DYNAMIC)) {
>> +		kfree(prop->value);
>> +		kfree(prop->name);
>> +		kfree(prop);
>> +	} else {
>> +		pr_warn("%s: property %p cannot be freed; memory is gone\n",
>> +				__func__, prop);
>> +	}
>> +}
>> +
>> +/**
>> + * __of_free_tree - release the memory of a device tree node and
>> + *		    of all it's children + properties.
>> + * @node:	Device Tree node to release
>> + *
>> + * Release the memory of a device tree node and of all it's children.
>> + * Also release the properties and the dead properties.
>> + * Only call on detached node trees, and you better be sure that
>> + * no pointer exist for any properties. Only safe to do if you 
>> + * absolutely control the life cycle of the node.
>> + * Also note that the node is not removed from the all_nodes list,
>> + * neither from the parent's child list; this should be handled before
>> + * calling this function.
>> + */
>> +void __of_free_tree(struct device_node *node)
>> +{
>> +	struct property *prop;
>> +	struct device_node *noden;
>> +
>> +	/* sanity check */
>> +	if (!node)
>> +		return;
>> +
>> +	/* free recursively any children */
>> +	while ((noden = node->child) != NULL) {
>> +		node->child = noden->sibling;
>> +		__of_free_tree(noden);
>> +	}
>> +
>> +	/* free every property already allocated */
>> +	while ((prop = node->properties) != NULL) {
>> +		node->properties = prop->next;
>> +		__of_free_property(prop);
>> +	}
>> +
>> +	/* free dead properties already allocated */
>> +	while ((prop = node->deadprops) != NULL) {
>> +		node->deadprops = prop->next;
>> +		__of_free_property(prop);
>> +	}
>> +
>> +	if (of_node_check_flag(node, OF_DYNAMIC)) {
>> +		kfree(node->type);
>> +		kfree(node->name);
>> +		kfree(node);
>> +	} else {
>> +		pr_warn("%s: node %p cannot be freed; memory is gone\n",
>> +				__func__, node);
>> +	}
>> +}
>> +
>> +/**
>> + * __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.
>> + */
>> +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;
>> +	}
> 
> 
> I think the prop->length check, should be removed. Properties with length 0, such
> as "interrupt-controller;" have length 0 and some parts of the linux kernel actually
> rely on their value being not NULL.
> 
> For example in drivers/of/irq.c, of_irq_map_raw() checks that a node is an interrupt
> controller by calling:
> 
> of_get_property(ipar, "interrupt-controller", NULL)
> 
> and checking that it returns a non-null value.
> 
> We can safely use kmalloc with size 0 which will return ZERO_SIZE_PTR. This will cause
> all the tests for non-null values in case of zero length properties to pass.
> 
> I've sent you a patch a while ago for this. I'm not sure you had time to review it.
> 

I am aware of that, and your patch looks sane.

As I mentioned earlier I'm trying to get this accepted in general term and then we'll
get around fixing any minor problems.

> Thanks,
> Ionut
> 

Regards

-- Pantelis

>> +
>> +	/* 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)
>> +{
>> +	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;
>> +
>> +	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)
>> +{
>> +	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;
>> +	}
>> +
>> +	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_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.
>> + */
>> +int of_multi_prop_cmp(const struct property *prop, const char *value)
>> +{
>> +	const char *cp;
>> +	int cplen, vlen, l;
>> +
>> +	if (prop == NULL || value == NULL)
>> +		return -1;
>> +
>> +	cp = prop->value;
>> +	cplen = prop->length;
>> +	vlen = strlen(value);
>> +
>> +	while (cplen > 0) {
>> +		if (of_compat_cmp(cp, value, vlen) == 0)
>> +			return 0;
>> +		l = strlen(cp) + 1;
>> +		cp += l;
>> +		cplen -= l;
>> +	}
>> +
>> +	return -1;
>> +}
>> +
>> diff --git a/include/linux/of.h b/include/linux/of.h
>> index a56c450..9d69bd2 100644
>> --- a/include/linux/of.h
>> +++ b/include/linux/of.h
>> @@ -662,4 +662,63 @@ extern void proc_device_tree_update_prop(struct proc_dir_entry *pde,
>> 					 struct property *oldprop);
>> #endif
>> 
>> +/**
>> + * General utilities for working with live trees.
>> + *
>> + * All functions with two leading underscores operate
>> + * without taking node references, so you either have to
>> + * own the devtree lock or work on detached trees only.
>> + */
>> +
>> +#ifdef CONFIG_OF
>> +
>> +/* iterator for internal use; not references, neither affects devtree lock */
>> +#define __for_each_child_of_node(dn, chld) \
>> +	for (chld = (dn)->child; chld != NULL; chld = chld->sibling)
>> +
>> +void __of_free_property(struct property *prop);
>> +void __of_free_tree(struct device_node *node);
>> +struct property *__of_copy_property(const struct property *prop, gfp_t flags);
>> +struct device_node *__of_create_empty_node( const char *name,
>> +		const char *type, const char *full_name,
>> +		phandle phandle, gfp_t flags);
>> +struct device_node *__of_find_node_by_full_name(struct device_node *node,
>> +		const char *full_name);
>> +int of_multi_prop_cmp(const struct property *prop, const char *value);
>> +
>> +#else /* !CONFIG_OF */
>> +
>> +#define __for_each_child_of_node(dn, chld) \
>> +	while (0)
>> +
>> +static inline void __of_free_property(struct property *prop) { }
>> +
>> +static inline void __of_free_tree(struct device_node *node) { }
>> +
>> +static inline struct property *__of_copy_property(const struct property *prop,
>> +		gfp_t flags)
>> +{
>> +	return NULL;
>> +}
>> +
>> +static inline struct device_node *__of_create_empty_node( const char *name,
>> +		const char *type, const char *full_name,
>> +		phandle phandle, gfp_t flags)
>> +{
>> +	return NULL;
>> +}
>> +
>> +static inline struct device_node *__of_find_node_by_full_name(struct device_node *node,
>> +		const char *full_name)
>> +{
>> +	return NULL;
>> +}
>> +
>> +static inline int of_multi_prop_cmp(const struct property *prop, const char *value)
>> +{
>> +	return -1;
>> +}
>> +
>> +#endif	/* !CONFIG_OF */
>> +
>> #endif /* _LINUX_OF_H */
>> 
> 

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