Hi Grant, On Nov 13, 2013, at 2:39 AM, Grant Likely wrote: > On Tue, 12 Nov 2013 09:28:42 +0100, Pantelis Antoniou <panto@xxxxxxxxxxxxxxxxxxxxxxx> wrote: >> >> On Nov 11, 2013, at 7:17 PM, Grant Likely wrote: >> >>> On Fri, 8 Nov 2013 17:06:08 +0200, Pantelis Antoniou <panto@xxxxxxxxxxxxxxxxxxxxxxx> 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 by Guenter Roeck <groeck@xxxxxxxxxxx> >>>> >>>> Signed-off-by: Pantelis Antoniou <panto@xxxxxxxxxxxxxxxxxxxxxxx> >>>> --- >>>> .../devicetree/dynamic-resolution-notes.txt | 25 ++ >>>> drivers/of/Kconfig | 9 + >>>> drivers/of/Makefile | 1 + >>>> drivers/of/resolver.c | 396 +++++++++++++++++++++ >>>> include/linux/of.h | 17 + >>>> 5 files changed, 448 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] >>> >>> dt-object-internal.txt is in the DTC patch, not the kernel tree. >>> >> >> Yes, good catch. I will fix the reference. >> >> BTW, what about moving/copying some of the DTC docs in the kernel doc >> directory? The dtc Documentation directory is missing from the kernel tree. >> >> >>>> + >>>> +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]. >>> >>> Missing footnote reference line for [1]? >>> >> >> Yes. >> >>>> + >>>> +In sequence the resolver works by the following steps: >>>> + >>>> +1. Get the maximum device tree phandle value from the live tree + 1. >>> >>> Is there a (realistic) worry about leaking phandle number space from >>> plugging/unplugging trees repeated addition/removal of overlays? >>> >> >> I think not. But doing it this way has the nice property of keeping all phandle >> values the same each time you do a load-unload-load sequence. > > It will break if there are two overlays "leapfrogging" each other on > loads/unloads. It may be a very outside corner case, but it is worth > thinking about. > Normally that can't happen when using an overlay manager when these calls are taken while holding it's lock. But I see your point. I will rework it. >> >>>> +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 78cc760..2a00ae5 100644 >>>> --- a/drivers/of/Kconfig >>>> +++ b/drivers/of/Kconfig >>>> @@ -74,4 +74,13 @@ config OF_MTD >>>> depends on MTD >>>> def_bool y >>>> >>>> +config OF_RESOLVE >>>> + bool "OF Dynamic resolution support" >>>> + depends on OF >>>> + select OF_DYNAMIC >>>> + select OF_DEVICE >>>> + help >>>> + Enable OF dynamic resolution support. This allows you to >>>> + load Device Tree object fragments are run time. >>>> + >>>> endmenu # OF >>>> diff --git a/drivers/of/Makefile b/drivers/of/Makefile >>>> index 9bc6d8c..93da457 100644 >>>> --- a/drivers/of/Makefile >>>> +++ b/drivers/of/Makefile >>>> @@ -9,3 +9,4 @@ obj-$(CONFIG_OF_MDIO) += of_mdio.o >>>> 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_RESOLVE) += resolver.o >>>> diff --git a/drivers/of/resolver.c b/drivers/of/resolver.c >>>> new file mode 100644 >>>> index 0000000..dfbb51a >>>> --- /dev/null >>>> +++ b/drivers/of/resolver.c >>>> @@ -0,0 +1,396 @@ >>>> +/* >>>> + * 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/of_fdt.h> >>>> +#include <linux/string.h> >>>> +#include <linux/ctype.h> >>>> +#include <linux/errno.h> >>>> +#include <linux/string.h> >>>> +#include <linux/slab.h> >>>> + >>>> +/** >>>> + * 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); >>>> + >>>> + return max_phandle; >>>> +} >>>> + >>>> +/** >>>> + * Find live tree's maximum phandle value. >>>> + */ >>>> +static phandle of_get_tree_max_phandle(void) >>>> +{ >>>> + struct device_node *node; >>>> + phandle phandle; >>>> + unsigned long flags; >>>> + >>>> + /* get root node */ >>>> + node = of_find_node_by_path("/"); >>>> + if (node == NULL) >>>> + return OF_PHANDLE_ILLEGAL; >>>> + >>>> + /* now search recursively */ >>>> + raw_spin_lock_irqsave(&devtree_lock, flags); >>>> + phandle = __of_get_tree_max_phandle(node, 0); >>>> + raw_spin_unlock_irqrestore(&devtree_lock, flags); >>> >>> I don't see another user. What is the reason for the __ version of >>> of_get_tree_max_phandle? >>> >>>> + >>>> + 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; >>>> + >>>> + /* 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_cpu(*(uint32_t *)prop->value); >>> >>> Unnecessary cast if you use: >>> phandle = be32_to_cpup(prop->value); >> >> OK. >> >>> >>>> + if (phandle == OF_PHANDLE_ILLEGAL) /* unresolved */ >>>> + continue; >>> >>> Isn't this an error condition? Should never have OF_PHANDLE_ILLEGAL in >>> the property here. >> >> Hmm, I think so. I'll see if there's anything special there. >> >>> >>>> + >>>> + /* adjust */ >>>> + *(uint32_t *)prop->value = cpu_to_be32(node->phandle); >>> >>> *(__be32*)prop->value = ... >> >> >> It is the same for the compiler, but you're right. >>> >>>> + } >>>> + >>>> + /* 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 == 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; >>>> + >>>> + /* 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, ':'); >>>> + if (s == NULL) { >>>> + 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 == NULL) { >>>> + pr_err("%s: Illegal symbol entry '%s' (2)\n", >>>> + __func__, (char *)rprop->value); >>>> + err = -EINVAL; >>>> + goto err_fail; >>>> + } >>>> + >>>> + *s++ = '\0'; >>>> + offset = simple_strtoul(s, NULL, 10); >>>> + >>>> + /* look into the resolve node for the full path */ >>>> + refnode = __of_find_node_by_full_name(node, nodestr); >>>> + if (refnode == NULL) { >>>> + 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 == NULL) { >>>> + pr_err("%s: Could not find property '%s'\n", >>>> + __func__, (char *)rprop->value); >>>> + err = -ENOENT; >>>> + goto err_fail; >>>> + } >>>> + >>>> + phandle = be32_to_cpu(*(uint32_t *) >>>> + (sprop->value + offset)); >>>> + *(uint32_t *)(sprop->value + offset) = >>>> + cpu_to_be32(phandle + phandle_delta); >>>> + } >>>> + >>>> + kfree(propval); >>>> + } >>>> + >>>> + 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 == NULL || >>>> + !of_node_check_flag(resolve, OF_DETACHED)) { >>>> + return -EINVAL; >>>> + } >>>> + >>>> + /* first we need to adjust the phandles */ >>>> + phandle_delta = of_get_tree_max_phandle() + 1; >>> >>> Probably need to grab the devtree lock before doing the above, and not >>> release it until after the trees are merged. Otherwise there is the >>> potential of trying to merge two trees at once and getting phandle >>> conflicts. >> >> No, because the device tree being passed it it guaranteed to be >> in detached state, it is not part of the live device tree; >> the check in the beginning of the function makes sure. >> >> When we apply the overlay the devtree lock is taken properly. > > That doesn't protect against getting duplicate phandle bases. You need > something to protect the range of phandles that the overlay trees will > want to use. The problem with the above code is that it calculates the > phandle base that it wants, but then goes and does a bunch of stuff > without a lock which allows another overlay to try and use the same > phandle range. > Yes, I see. I'll try to see how that can be reworked. > g. > Regards -- Pantelis -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html