Re: [PATCH v7 1/9] OF: Introduce Device Tree resolve support.

[Pantelis Antoniou]

Hi Grant,

On Oct 1, 2014, at 3:46 PM, Grant Likely <grant.likely@xxxxxxxxxxxx> wrote:

> On Fri,  4 Jul 2014 19:59:20 +0300
> , Pantelis Antoniou <pantelis.antoniou@xxxxxxxxxxxx>
> wrote:
>> Introduce support for dynamic device tree resolution.
>> Using it, it is possible to prepare a device tree that's
>> been loaded on runtime to be modified and inserted at the kernel
>> live tree.
>> 
>> Export of of_resolve and bug fix of double free by
>> 	Guenter Roeck <groeck@xxxxxxxxxxx>
>> 
>> Signed-off-by: Pantelis Antoniou <pantelis.antoniou@xxxxxxxxxxxx>
> 
> Hi Pantelis,
> 
> I suspect you've got a slightly updated version of this patch not posted
> yet, but I do have a few comments. Overall they're pretty minor.
> 

Last time I checked (circa rc2), the last patch set applied. I’ll have to rebase and 
take a look again but I don’t expect major changes.

>> ---
>> .../devicetree/dynamic-resolution-notes.txt        |  25 ++
>> drivers/of/Kconfig                                 |   6 +
>> drivers/of/Makefile                                |   1 +
>> drivers/of/resolver.c                              | 403 +++++++++++++++++++++
>> include/linux/of.h                                 |  16 +
>> 5 files changed, 451 insertions(+)
>> create mode 100644 Documentation/devicetree/dynamic-resolution-notes.txt
>> create mode 100644 drivers/of/resolver.c
>> 
>> diff --git a/Documentation/devicetree/dynamic-resolution-notes.txt b/Documentation/devicetree/dynamic-resolution-notes.txt
>> new file mode 100644
>> index 0000000..0b396c4
>> --- /dev/null
>> +++ b/Documentation/devicetree/dynamic-resolution-notes.txt
>> @@ -0,0 +1,25 @@
>> +Device Tree Dynamic Resolver Notes
>> +----------------------------------
>> +
>> +This document describes the implementation of the in-kernel
>> +Device Tree resolver, residing in drivers/of/resolver.c and is a
>> +companion document to Documentation/devicetree/dt-object-internal.txt[1]
>> +
>> +How the resolver works
>> +----------------------
>> +
>> +The resolver is given as an input an arbitrary tree compiled with the
>> +proper dtc option and having a /plugin/ tag. This generates the
>> +appropriate __fixups__ & __local_fixups__ nodes as described in [1].
>> +
>> +In sequence the resolver works by the following steps:
>> +
>> +1. Get the maximum device tree phandle value from the live tree + 1.
>> +2. Adjust all the local phandles of the tree to resolve by that amount.
>> +3. Using the __local__fixups__ node information adjust all local references
>> +   by the same amount.
>> +4. For each property in the __fixups__ node locate the node it references
>> +   in the live tree. This is the label used to tag the node.
>> +5. Retrieve the phandle of the target of the fixup.
>> +5. For each fixup in the property locate the node:property:offset location
>> +   and replace it with the phandle value.
>> diff --git a/drivers/of/Kconfig b/drivers/of/Kconfig
>> index 2dcb054..0236a4e 100644
>> --- a/drivers/of/Kconfig
>> +++ b/drivers/of/Kconfig
>> @@ -78,4 +78,10 @@ config OF_RESERVED_MEM
>> 	help
>> 	  Helpers to allow for reservation of memory regions
>> 
>> +config OF_RESOLVE
>> +	bool
>> +	depends on OF && !SPARC
> 
> What part of this code breaks on SPARC?
> 

There are some extra properties on the SPARC device nodes, which I don’t know exactly how they work.
I don’t have access to a SPARC box, lest I break something this conditional.

>> +	select OF_DYNAMIC
>> +	select OF_DEVICE
> 
> This code doesn't appear to depend on either OF_DYNAMIC or OF_DEVICE. You
> should be able to drop the above 2 selects (at least it worked when I
> tried it here)
> 

Yes, good catch. It wouldn’t work of-course since all the calls to insert nodes would
fail.

>> +
>> endmenu # OF
>> diff --git a/drivers/of/Makefile b/drivers/of/Makefile
>> index 9a68cc9..35a148a 100644
>> --- a/drivers/of/Makefile
>> +++ b/drivers/of/Makefile
>> @@ -12,6 +12,7 @@ obj-$(CONFIG_OF_PCI)	+= of_pci.o
>> obj-$(CONFIG_OF_PCI_IRQ)  += of_pci_irq.o
>> obj-$(CONFIG_OF_MTD)	+= of_mtd.o
>> obj-$(CONFIG_OF_RESERVED_MEM) += of_reserved_mem.o
>> +obj-$(CONFIG_OF_RESOLVE)  += resolver.o
>> 
>> CFLAGS_fdt.o = -I$(src)/../../scripts/dtc/libfdt
>> CFLAGS_fdt_address.o = -I$(src)/../../scripts/dtc/libfdt
>> diff --git a/drivers/of/resolver.c b/drivers/of/resolver.c
>> new file mode 100644
>> index 0000000..18da5ca
>> --- /dev/null
>> +++ b/drivers/of/resolver.c
>> @@ -0,0 +1,403 @@
>> +/*
>> + * Functions for dealing with DT resolution
>> + *
>> + * Copyright (C) 2012 Pantelis Antoniou <panto@xxxxxxxxxxxxxxxxxxxxxxx>
>> + * 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>
>> +
>> +/**
>> + * Find a node with the give full name by recursively following any of
>> + * the child node links.
>> + */
>> +static 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;
>> +}
>> +
>> +/*
>> + * Find live tree's maximum phandle value.
>> + */
>> +static phandle of_get_tree_max_phandle(void)
>> +{
>> +	struct device_node *node;
>> +	phandle phandle;
>> +	unsigned long flags;
>> +
>> +	/* now search recursively */
>> +	raw_spin_lock_irqsave(&devtree_lock, flags);
>> +	phandle = 0;
>> +	for_each_of_allnodes(node) {
>> +		if (node->phandle != OF_PHANDLE_ILLEGAL &&
>> +				node->phandle > phandle)
>> +			phandle = node->phandle;
>> +	}
>> +	raw_spin_unlock_irqrestore(&devtree_lock, flags);
>> +
>> +	return phandle;
>> +}
>> +
>> +/*
>> + * Adjust a subtree's phandle values by a given delta.
>> + * Makes sure not to just adjust the device node's phandle value,
>> + * but modify the phandle properties values as well.
>> + */
>> +static void __of_adjust_tree_phandles(struct device_node *node,
>> +		int phandle_delta)
>> +{
>> +	struct device_node *child;
>> +	struct property *prop;
>> +	phandle phandle;
>> +
>> +	/* first adjust the node's phandle direct value */
>> +	if (node->phandle != 0 && node->phandle != OF_PHANDLE_ILLEGAL)
>> +		node->phandle += phandle_delta;
>> +
>> +	/* now adjust phandle & linux,phandle values */
>> +	for_each_property_of_node(node, prop) {
>> +
>> +		/* only look for these two */
>> +		if (of_prop_cmp(prop->name, "phandle") != 0 &&
>> +		    of_prop_cmp(prop->name, "linux,phandle") != 0)
>> +			continue;
>> +
>> +		/* must be big enough */
>> +		if (prop->length < 4)
>> +			continue;
>> +
>> +		/* read phandle value */
>> +		phandle = be32_to_cpup(prop->value);
>> +		if (phandle == OF_PHANDLE_ILLEGAL)	/* unresolved */
>> +			continue;
>> +
>> +		/* adjust */
>> +		*(uint32_t *)prop->value = cpu_to_be32(node->phandle);
>> +	}
>> +
>> +	/* now do the children recursively */
>> +	for_each_child_of_node(node, child)
>> +		__of_adjust_tree_phandles(child, phandle_delta);
>> +}
>> +
>> +/*
>> + * Adjust the local phandle references by the given phandle delta.
>> + * Assumes the existances of a __local_fixups__ node at the root
>> + * of the tree. Does not take any devtree locks so make sure you
>> + * call this on a tree which is at the detached state.
>> + */
>> +static int __of_adjust_tree_phandle_references(struct device_node *node,
>> +		int phandle_delta)
>> +{
>> +	phandle phandle;
>> +	struct device_node *refnode, *child;
>> +	struct property *rprop, *sprop;
>> +	char *propval, *propcur, *propend, *nodestr, *propstr, *s;
>> +	int offset, propcurlen;
>> +	int err;
>> +
>> +	/* locate the symbols & fixups nodes on resolve */
>> +	for_each_child_of_node(node, child)
>> +		if (of_node_cmp(child->name, "__local_fixups__") == 0)
>> +			break;
>> +
>> +	/* no local fixups */
>> +	if (!child)
>> +		return 0;
>> +
>> +	/* find the local fixups property */
>> +	for_each_property_of_node(child, rprop) {
>> +
>> +		/* skip properties added automatically */
>> +		if (of_prop_cmp(rprop->name, "name") == 0)
>> +			continue;
>> +
> 
> Everything below this line....
> 
>> +		/* make a copy */
>> +		propval = kmalloc(rprop->length, GFP_KERNEL);
>> +		if (!propval) {
>> +			pr_err("%s: Could not copy value of '%s'\n",
>> +					__func__, rprop->name);
>> +			return -ENOMEM;
>> +		}
>> +		memcpy(propval, rprop->value, rprop->length);
>> +
>> +		propend = propval + rprop->length;
>> +		for (propcur = propval; propcur < propend;
>> +				propcur += propcurlen + 1) {
>> +
>> +			propcurlen = strlen(propcur);
>> +
>> +			nodestr = propcur;
>> +			s = strchr(propcur, ':');
>> +			if (!s) {
>> +				pr_err("%s: Illegal symbol entry '%s' (1)\n",
>> +					__func__, propcur);
>> +				err = -EINVAL;
>> +				goto err_fail;
>> +			}
>> +			*s++ = '\0';
>> +
>> +			propstr = s;
>> +			s = strchr(s, ':');
>> +			if (!s) {
>> +				pr_err("%s: Illegal symbol entry '%s' (2)\n",
>> +					__func__, (char *)rprop->value);
>> +				err = -EINVAL;
>> +				goto err_fail;
>> +			}
>> +
>> +			*s++ = '\0';
>> +			err = kstrtoint(s, 10, &offset);
>> +			if (err != 0) {
>> +				pr_err("%s: Could get offset '%s'\n",
>> +					__func__, (char *)rprop->value);
>> +				goto err_fail;
>> +			}
>> +
>> +			/* look into the resolve node for the full path */
>> +			refnode = __of_find_node_by_full_name(node, nodestr);
>> +			if (!refnode) {
>> +				pr_warn("%s: Could not find refnode '%s'\n",
>> +					__func__, (char *)rprop->value);
>> +				continue;
>> +			}
>> +
>> +			/* now find the property */
>> +			for_each_property_of_node(refnode, sprop) {
>> +				if (of_prop_cmp(sprop->name, propstr) == 0)
>> +					break;
>> +			}
>> +
>> +			if (!sprop) {
>> +				pr_err("%s: Could not find property '%s'\n",
>> +					__func__, (char *)rprop->value);
>> +				err = -ENOENT;
>> +				goto err_fail;
>> +			}
>> +
>> +			phandle = be32_to_cpup(sprop->value + offset);
>> +			*(__be32 *)(sprop->value + offset) =
>> +				cpu_to_be32(phandle + phandle_delta);
>> +		}
>> +
>> +		kfree(propval);
>> +		propval = NULL;
> 
> ... to this line is almost identical to the block in of_resolve(). Can
> it be moved into a helper function?
> 

It appears so at first glance.


>> +	}
>> +
>> +	return 0;
>> +
>> +err_fail:
>> +	kfree(propval);
>> +	return err;
>> +}
>> +
>> +/**
>> + * of_resolve	- Resolve the given node against the live tree.
>> + *
>> + * @resolve:	Node to resolve
>> + *
>> + * Perform dynamic Device Tree resolution against the live tree
>> + * to the given node to resolve. This depends on the live tree
>> + * having a __symbols__ node, and the resolve node the __fixups__ &
>> + * __local_fixups__ nodes (if needed).
>> + * The result of the operation is a resolve node that it's contents
>> + * are fit to be inserted or operate upon the live tree.
>> + * Returns 0 on success or a negative error value on error.
>> + */
>> +int of_resolve(struct device_node *resolve)
>> +{
>> +	struct device_node *child, *refnode;
>> +	struct device_node *root_sym, *resolve_sym, *resolve_fix;
>> +	struct property *rprop, *sprop;
>> +	const char *refpath;
>> +	char *propval, *propcur, *propend, *nodestr, *propstr, *s;
>> +	int offset, propcurlen;
>> +	phandle phandle, phandle_delta;
>> +	int err;
>> +
>> +	/* the resolve node must exist, and be detached */
>> +	if (!resolve || !of_node_check_flag(resolve, OF_DETACHED))
>> +		return -EINVAL;
>> +
>> +	/* first we need to adjust the phandles */
>> +	phandle_delta = of_get_tree_max_phandle() + 1;
>> +	__of_adjust_tree_phandles(resolve, phandle_delta);
>> +	err = __of_adjust_tree_phandle_references(resolve, phandle_delta);
>> +	if (err != 0)
>> +		return err;
>> +
>> +	root_sym = NULL;
>> +	resolve_sym = NULL;
>> +	resolve_fix = NULL;
>> +
>> +	/* this may fail (if no fixups are required) */
>> +	root_sym = of_find_node_by_path("/__symbols__");
>> +
>> +	/* locate the symbols & fixups nodes on resolve */
>> +	for_each_child_of_node(resolve, child) {
>> +
>> +		if (!resolve_sym &&
>> +				of_node_cmp(child->name, "__symbols__") == 0)
>> +			resolve_sym = child;
> 
> Can /aliases be used instead of __symbols__? __symbols__ requires the
> .dtb to be compiled in a particular way and it means an overlay will
> never work with an existing dtb.
> 

Hmm. Aliases in theory can be used as symbols sure enough.
It is going to be very error prone (since you will need to explicitly define every symbol,
instead of just relying on the compiler to generate an entry for each label).

As a change it’s simple enough...

> We also need to review how overlays match up with nodes in the base
> tree. I can see a board with multiple identical expansion connectors,
> but each connector wires up to different pins. If we made the symbols
> resolution local to the expansion connector, then the same overlay could
> be used for different ports.
> 

I have something in mind how to fix this, and some even more wonky cases.
Think overlays applied on devices on busses that can be probed like PCI/USB etc.

I think we can work the details at ELCE. Perhaps ask the organizers to setup the DT
session we discussed.

>> +
>> +		if (!resolve_fix &&
>> +				of_node_cmp(child->name, "__fixups__") == 0)
>> +			resolve_fix = child;
>> +
>> +		/* both found, don't bother anymore */
>> +		if (resolve_sym && resolve_fix)
>> +			break;
>> +	}
>> +
>> +	/* we do allow for the case where no fixups are needed */
>> +	if (!resolve_fix) {
>> +		err = 0;	/* no error */
>> +		goto out;
>> +	}
>> +
>> +	/* we need to fixup, but no root symbols... */
>> +	if (!root_sym) {
>> +		err = -EINVAL;
>> +		goto out;
>> +	}
>> +
>> +	for_each_property_of_node(resolve_fix, rprop) {
>> +
>> +		/* skip properties added automatically */
>> +		if (of_prop_cmp(rprop->name, "name") == 0)
>> +			continue;
>> +
>> +		err = of_property_read_string(root_sym,
>> +				rprop->name, &refpath);
>> +		if (err != 0) {
>> +			pr_err("%s: Could not find symbol '%s'\n",
>> +					__func__, rprop->name);
>> +			goto out;
>> +		}
>> +
>> +		refnode = of_find_node_by_path(refpath);
>> +		if (!refnode) {
>> +			pr_err("%s: Could not find node by path '%s'\n",
>> +					__func__, refpath);
>> +			err = -ENOENT;
>> +			goto out;
>> +		}
>> +
>> +		phandle = refnode->phandle;
>> +		of_node_put(refnode);
>> +
>> +		pr_debug("%s: %s phandle is 0x%08x\n",
>> +				__func__, rprop->name, phandle);
>> +
>> +		/* make a copy */
>> +		propval = kmalloc(rprop->length, GFP_KERNEL);
>> +		if (!propval) {
>> +			pr_err("%s: Could not copy value of '%s'\n",
>> +					__func__, rprop->name);
>> +			err = -ENOMEM;
>> +			goto out;
>> +		}
>> +
>> +		memcpy(propval, rprop->value, rprop->length);
>> +
>> +		propend = propval + rprop->length;
>> +		for (propcur = propval; propcur < propend;
>> +				propcur += propcurlen + 1) {
>> +			propcurlen = strlen(propcur);
>> +
>> +			nodestr = propcur;
>> +			s = strchr(propcur, ':');
>> +			if (!s) {
>> +				pr_err("%s: Illegal symbol entry '%s' (1)\n",
>> +					__func__, (char *)rprop->value);
>> +				err = -EINVAL;
>> +				goto err_fail_free;
>> +			}
>> +			*s++ = '\0';
>> +
>> +			propstr = s;
>> +			s = strchr(s, ':');
>> +			if (!s) {
>> +				pr_err("%s: Illegal symbol entry '%s' (2)\n",
>> +					__func__, (char *)rprop->value);
>> +				err = -EINVAL;
>> +				goto err_fail_free;
>> +			}
>> +
>> +			*s++ = '\0';
>> +			err = kstrtoint(s, 10, &offset);
>> +			if (err != 0) {
>> +				pr_err("%s: Could get offset '%s'\n",
>> +					__func__, (char *)rprop->value);
>> +				goto err_fail_free;
>> +			}
>> +
>> +			/* look into the resolve node for the full path */
>> +			refnode = __of_find_node_by_full_name(resolve,
>> +					nodestr);
>> +			if (!refnode) {
>> +				pr_err("%s: Could not find refnode '%s'\n",
>> +					__func__, (char *)rprop->value);
>> +				err = -ENOENT;
>> +				goto err_fail_free;
>> +			}
>> +
>> +			/* now find the property */
>> +			for_each_property_of_node(refnode, sprop) {
>> +				if (of_prop_cmp(sprop->name, propstr) == 0)
>> +					break;
>> +			}
>> +
>> +			if (!sprop) {
>> +				pr_err("%s: Could not find property '%s'\n",
>> +					__func__, (char *)rprop->value);
>> +				err = -ENOENT;
>> +				goto err_fail_free;
>> +			}
>> +
>> +			*(__be32 *)(sprop->value + offset) =
>> +				cpu_to_be32(phandle);
>> +		}
>> +
>> +		kfree(propval);
>> +		propval = NULL;
>> +	}
>> +
>> +err_fail_free:
>> +	kfree(propval);
>> +
>> +out:
>> +	/* NULL is handled by of_node_put as NOP */
>> +	of_node_put(root_sym);
>> +
>> +	return err;
>> +}
>> +EXPORT_SYMBOL_GPL(of_resolve);
>> diff --git a/include/linux/of.h b/include/linux/of.h
>> index fa81b42..e9be31c 100644
>> --- a/include/linux/of.h
>> +++ b/include/linux/of.h
>> @@ -903,4 +903,20 @@ static inline int of_transaction_update_property(struct of_transaction *oft,
>> }
>> #endif
>> 
>> +/* illegal phandle value (set when unresolved) */
>> +#define OF_PHANDLE_ILLEGAL	0xdeadbeef
> 
> fdt.c should be made to use this macro also.
> 

OK.

> I've actually got a patches to makes all these changes. I've been trying
> to get the selftest code to use the resolver. The current code passes
> all the test cases, but while the testcase data is loaded there are
> duplicate phandles in the tree. :-(
> 

Told you them things are tricky! :)

Remember that patch we talked about a few weeks earlier? About the implicit properties?
"of: dynamic: Handle special properties changes”. That should do the trick.

Speaking of which I think it’s also time to look over the next set of changes.
Like having a tree DT structure, phandle lists and perhaps copy based devm_* driver APIs.


> g.

Regards

— Pantelis

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