Message-ID: <20130122040524.GG23500@truffula.fritz.box>
Date:	Tue, 22 Jan 2013 15:05:24 +1100
From:	David Gibson <david@...son.dropbear.id.au>
To:	Pantelis Antoniou <panto@...oniou-consulting.com>
Cc:	Grant Likely <grant.likely@...retlab.ca>,
	Rob Herring <rob.herring@...xeda.com>,
	Rob Landley <rob@...dley.net>, Jon Loeliger <jdl@....com>,
	Tony Lindgren <tony@...mide.com>,
	Stephen Warren <swarren@...dotorg.org>,
	Benoit Cousson <b-cousson@...com>,
	Mitch Bradley <wmb@...mworks.com>,
	Alan Tull <atull@...era.com>, linux-omap@...r.kernel.org,
	devicetree-discuss@...ts.ozlabs.org, linux-doc@...r.kernel.org,
	linux-kernel@...r.kernel.org, Matt Porter <mporter@...com>,
	Russ Dill <Russ.Dill@...com>,
	Koen Kooi <koen@...inion.thruhere.net>,
	Joel A Fernandes <agnel.joel@...il.com>,
	Rob Clark <robdclark@...il.com>,
	Jason Kridner <jkridner@...gleboard.org>,
	Matt Ranostay <mranostay@...il.com>
Subject: Re: [PATCH 5/6] OF: Introduce Device Tree resolve support.

On Mon, Jan 21, 2013 at 12:59:15PM +0200, Pantelis Antoniou wrote:
> Hi David
> 
> On Jan 21, 2013, at 6:48 AM, David Gibson wrote:
> 
> > On Fri, Jan 04, 2013 at 09:31:09PM +0200, Pantelis Antoniou 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.
> >> 
> >> Signed-off-by: Pantelis Antoniou <panto@...oniou-consulting.com>
> >> ---
> >> .../devicetree/dynamic-resolution-notes.txt        |  25 ++
> >> drivers/of/Kconfig                                 |   9 +
> >> drivers/of/Makefile                                |   1 +
> >> drivers/of/resolver.c                              | 394 +++++++++++++++++++++
> >> include/linux/of.h                                 |  17 +
> >> 5 files changed, 446 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.
> > 
> > Hrm.  So, I'm really still not convinced by this approach.
> > 
> > 	First, I think it's unwise to allow overlays to change
> > essentially anything in the base tree, rather than having the base
> > tree define sockets of some sort where things can be attached.
> > 
> 
> One could say that the labels define the sockets. It's not just things
> to be attached, properties might have to change, or something more complex,
> as we've found out in practice.

Hrm.  I know a number of these have come up previously in that big
thread, but can you summarise some of these cases here.  If things
need modification in the base tree that still seems to me like the
base tree hasn't properly described the socket arrangement (I realise
that allowing such descriptions may require extensions to some of our
device tree conventions).

> As far as the unwise part, a good deal of care has been taken so that 
> people that don't use the overlay functionality have absolutely no
> overhead, or anything modified in the way they use DT.

Yeah, that's not what I'm concerned about.  I'm concerned about hard
to debug problems because some subtle change in the base tree or the
overlay or both causes the overlay to alter something in the base tree
it really shouldn't.

> > 	Second, even allowing overlays to change anything, I don't see
> > a lot of reason to do this kind of resolution within the kernel and
> > with data stored in the dtb itself, rather than doing the resolution
> > in userspace from an annotated overlay dts or dtb, then inserting the
> > fully resolved product into the kernel.  In either case, the overlay
> > needs to be constructed with pretty intimate knowledge of the base
> > tree.
> 
> Fair enough, but that's one more thing of user-space crud to drag along, which
> will get enabled pretty late in the boot sequence. Meaning a whole bunch of devices,
> like consoles, and root filesystems on the devices that need an overlay to operate
> won't work easily enough.

Hrm.  But doesn't your scheme already require userspace to identify
the hardware and load the overlay?  So why is having it resolve the
overlay significantly harder?

AFAICT devices wanted early can be handled in several possible ways
without having the resolved in kernel: an initramfs is the most
obvious, but for things you want really early, it should be possible
to do the resolution from the platform's bootloader update tool - so
the pre-resolved overlay gets bundled with the kernel/initrd/whatever
to get fired up from the bootloader.

> 
> > That said, I have some implementation comments below.
> > 
> > [snip]
> >> +/**
> >> + * Find a subtree's maximum phandle value.
> >> + */
> >> +static phandle __of_get_tree_max_phandle(struct device_node *node,
> >> +		phandle max_phandle)
> >> +{
> >> +	struct device_node *child;
> >> +
> >> +	if (node->phandle != 0 && node->phandle != OF_PHANDLE_ILLEGAL &&
> >> +			node->phandle > max_phandle)
> >> +		max_phandle = node->phandle;
> >> +
> >> +	__for_each_child_of_node(node, child)
> >> +		max_phandle = __of_get_tree_max_phandle(child, max_phandle);
> > 
> > Recursion is best avoided given the kernel's limited stack space.
> > This is also trivial to implement non-recursively, using the allnext
> > pointer.
> 
> The caller passes a tree that's not yet been inserted in the live
> tree.

Um.. isn't this used to find the max phandle in the base tree, so how
is it not inserted in the live tree yet?

> So there's no allnodes pointer yet. 

I would strongly suggest populating that in the subtree as you build
it, then.  Except that I don't think it is a detached subtree in this
case, though it is in some of the cases below.

> Care has been taken for the function
> to not have excessive local variables. I would guess about 20-32 bytes for
> the stack frame + the local variables, so with a 4K stack we would overflow at a 
> nest level of 128, which has a pretty slim chance for a real system.

Hrm.  Recursion still makes me nervous.  I believe there are platforms
that have a non-trivial lower-bound on stack usage per frame, even
with few local variables.

> >> +
> >> +	return max_phandle;
> >> +}
> >> +
> >> +/**
> >> + * Find live tree's maximum phandle value.
> >> + */
> >> +static phandle of_get_tree_max_phandle(void)
> >> +{
> >> +	struct device_node *node;
> >> +	phandle phandle;
> >> +
> >> +	/* get root node */
> >> +	node = of_find_node_by_path("/");
> >> +	if (node == NULL)
> >> +		return OF_PHANDLE_ILLEGAL;
> >> +
> >> +	/* now search recursively */
> >> +	read_lock(&devtree_lock);
> >> +	phandle = __of_get_tree_max_phandle(node, 0);
> >> +	read_unlock(&devtree_lock);
> >> +
> >> +	of_node_put(node);
> >> +
> >> +	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;
> > 
> > You need to have some kind of check for overflow here, or the adjusted
> > phandle could be one of the illegal values (0 or -1) - or wrap around
> > and colllide with existing phandle values in the base tree.  dtc
> > (currently) allocates phandles from the bottom, but there's no
> > guarantee that a base tree will only have low phandle values - it only
> > takes one node with phandle set to 0xfffffffe in the base tree to have
> > this function make a mess of things.
> 
> Correct, I'll take care of handling the overflow.
> 
> > 
> > 
> >> +	/* 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;
> > 
> > If prop->length != 4 (including > 4) something is pretty wrong, and
> > you should probably bail with an error message.
> 
> OK, just playing it safe here.
> 
> > 
> >> +
> >> +		/* read phandle value */
> >> +		phandle = be32_to_cpu(*(uint32_t *)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);
> > 
> > Again, recursion is not a good idea.
> > 
> 
> No other way to handle it. This is not a node that's in the live
> tree yet.

There's always another way to handle it, although it may be less
elegant.  In this case it should be easy though - populate the allnext
pointers when you unflatten the tree.  Or, have the offset applied
when you actually apply the overlay rather than as a separate pass.
 
> >> +}
> >> +
> >> +/**
> >> + * 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 == NULL)
> >> +		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;
> > 
> > Ok, so you're interpreting any property except name in the
> > __local_fixups__ node in exactly the same way?  That's a bit strange.
> > Why not just have a single property rather than a node's worth in that
> > case.
> 
> It saves space. For example you might have to resolve a label reference
> more than once. So instead of doing:
> 
> 	label = "/foo:bar:0";
> 	label = "/bar:foo:4";
> 
> You can do this:
> 
> 	label = "/foo:bar:0", "/bar/foo:4";

Um.. you seem to have read me as saying the exact opposite of what I
said.  I'm _suggesting_ that you use:
	__local_fixups__ = "/foo:bar:0", "/bar/foo:4";
Since the 'label' name has no meaning in the case of local fixups.

> >> +		/* make a copy */
> >> +		propval = kmalloc(rprop->length, GFP_KERNEL);
> >> +		if (propval == NULL) {
> >> +			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, ':');
> > 
> > So, using strings with separators like this doesn't sit will with
> > existing device tree practice.  More similar to existing things would
> > have NUL separators and the integer values in binary, rather than
> > text (and yes, there is precedent for mixed string and integer content
> > in properties).
> > 
> 
> Hmm, I guess it can be done, but I wouldn't expect any space savings.

I'm not after space savings, just least-surprise by matching existing
common practices.

-- 
David Gibson			| I'll have my music baroque, and my code
david AT gibson.dropbear.id.au	| minimalist, thank you.  NOT _the_ _other_
				| _way_ _around_!
http://www.ozlabs.org/~dgibson

Download attachment "signature.asc" of type "application/pgp-signature" (199 bytes)

Powered by blists - more mailing lists