| Message ID | 1357327870-13615-6-git-send-email-panto@antoniou-consulting.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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@antoniou-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. 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. 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. > + > + 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. > + /* 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. > + > + /* 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. > +} > + > +/** > + * 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. > + /* 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). > + if (s == NULL) { > + pr_err("%s: Illegal symbol entry '%s' (1)\n", > + __func__, propcur); > + err = -EINVAL; > + goto err_fail; > + } > + *s++ = '\0'; And using the separators you do leads to this rather ugly copy then mangle-in-place approach to parsing. > + 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; > + __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 == NULL && No need for the NULL check. If there are duplicate node names, you've already got big trouble, and picking the last matching will do no worse than picking the first matching. > + of_node_cmp(child->name, "__symbols__") == 0) > + resolve_sym = child; > + > + if (resolve_fix == NULL && > + of_node_cmp(child->name, "__fixups__") == 0) > + resolve_fix = child; > + > + /* both found, don't bother anymore */ > + if (resolve_sym != NULL && resolve_fix != NULL) > + break; > + } > + > + /* we do allow for the case where no fixups are needed */ > + if (resolve_fix == NULL) > + goto merge_sym; Hrm. I'm not convinced that's one of the kernel-allowed forms of goto. > + > + /* we need to fixup, but no root symbols... */ > + if (root_sym == NULL) > + return -EINVAL; > + > + 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 err_fail; > + } > + > + refnode = of_find_node_by_path(refpath); > + if (refnode == NULL) { > + pr_err("%s: Could not find node by path '%s'\n", > + __func__, refpath); > + err = -ENOENT; > + goto err_fail; > + } > + > + 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 == NULL) { > + pr_err("%s: Could not copy value of '%s'\n", > + __func__, rprop->name); > + err = -ENOMEM; > + goto err_fail; > + } > + > + 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__, (char *)rprop->value); > + kfree(propval); > + 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); > + kfree(propval); > + 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(resolve, > + nodestr); Re-using the 'refnode' variable here is pretty confusing, since it means very different things earlier and here (node pointed to, versus node containing the property which points). > + if (refnode == NULL) { > + pr_err("%s: Could not find refnode '%s'\n", > + __func__, (char *)rprop->value); > + kfree(propval); > + err = -ENOENT; > + goto err_fail; > + } > + > + /* 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); > + kfree(propval); > + err = -ENOENT; > + goto err_fail; > + } > + > + *(uint32_t *)(sprop->value + offset) = > + cpu_to_be32(phandle); > + } > + > + kfree(propval); > + } > + > +merge_sym: > + > + of_node_put(root_sym); > + > + return 0; > + > +err_fail: > + > + if (root_sym != NULL) > + of_node_put(root_sym); > + > + return err; > +} > diff --git a/include/linux/of.h b/include/linux/of.h > index c38e41a..ab52243 100644 > --- a/include/linux/of.h > +++ b/include/linux/of.h > @@ -650,4 +650,21 @@ static inline int of_multi_prop_cmp(const struct property *prop, const char *val > > #endif /* !CONFIG_OF */ > > + > +/* illegal phandle value (set when unresolved) */ > +#define OF_PHANDLE_ILLEGAL 0xdeadbeef Ugh. 0 and -1 are already reserved as illegal phandle values, don't invent a new one. > + > +#ifdef CONFIG_OF_RESOLVE > + > +int of_resolve(struct device_node *resolve); > + > +#else > + > +static inline int of_resolve(struct device_node *resolve) > +{ > + return -ENOTSUPP; > +} > + > +#endif > + > #endif /* _LINUX_OF_H */
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@antoniou-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. 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. > 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. > 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. So there's no allnodes pointer yet. 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. >> + >> + 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. >> +} >> + >> +/** >> + * 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"; > >> + /* 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. Most offsets are very small integer multiple of 4 since the usual case is foo =<&label>; And you lose the ability to dump a string and figure out what's going on. Not a big problem to change. > >> + if (s == NULL) { >> + pr_err("%s: Illegal symbol entry '%s' (1)\n", >> + __func__, propcur); >> + err = -EINVAL; >> + goto err_fail; >> + } >> + *s++ = '\0'; > > And using the separators you do leads to this rather ugly copy then > mangle-in-place approach to parsing. > >> + 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; >> + __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 == NULL && > > No need for the NULL check. If there are duplicate node names, you've > already got big trouble, and picking the last matching will do no > worse than picking the first matching. OK > >> + of_node_cmp(child->name, "__symbols__") == 0) >> + resolve_sym = child; >> + >> + if (resolve_fix == NULL && >> + of_node_cmp(child->name, "__fixups__") == 0) >> + resolve_fix = child; >> + >> + /* both found, don't bother anymore */ >> + if (resolve_sym != NULL && resolve_fix != NULL) >> + break; >> + } >> + >> + /* we do allow for the case where no fixups are needed */ >> + if (resolve_fix == NULL) >> + goto merge_sym; > > Hrm. I'm not convinced that's one of the kernel-allowed forms of > goto. > OK >> + >> + /* we need to fixup, but no root symbols... */ >> + if (root_sym == NULL) >> + return -EINVAL; >> + >> + 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 err_fail; >> + } >> + >> + refnode = of_find_node_by_path(refpath); >> + if (refnode == NULL) { >> + pr_err("%s: Could not find node by path '%s'\n", >> + __func__, refpath); >> + err = -ENOENT; >> + goto err_fail; >> + } >> + >> + 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 == NULL) { >> + pr_err("%s: Could not copy value of '%s'\n", >> + __func__, rprop->name); >> + err = -ENOMEM; >> + goto err_fail; >> + } >> + >> + 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__, (char *)rprop->value); >> + kfree(propval); >> + 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); >> + kfree(propval); >> + 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(resolve, >> + nodestr); > > Re-using the 'refnode' variable here is pretty confusing, since it > means very different things earlier and here (node pointed to, versus > node containing the property which points). > OK >> + if (refnode == NULL) { >> + pr_err("%s: Could not find refnode '%s'\n", >> + __func__, (char *)rprop->value); >> + kfree(propval); >> + err = -ENOENT; >> + goto err_fail; >> + } >> + >> + /* 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); >> + kfree(propval); >> + err = -ENOENT; >> + goto err_fail; >> + } >> + >> + *(uint32_t *)(sprop->value + offset) = >> + cpu_to_be32(phandle); >> + } >> + >> + kfree(propval); >> + } >> + >> +merge_sym: >> + >> + of_node_put(root_sym); >> + >> + return 0; >> + >> +err_fail: >> + >> + if (root_sym != NULL) >> + of_node_put(root_sym); >> + >> + return err; >> +} >> diff --git a/include/linux/of.h b/include/linux/of.h >> index c38e41a..ab52243 100644 >> --- a/include/linux/of.h >> +++ b/include/linux/of.h >> @@ -650,4 +650,21 @@ static inline int of_multi_prop_cmp(const struct property *prop, const char *val >> >> #endif /* !CONFIG_OF */ >> >> + >> +/* illegal phandle value (set when unresolved) */ >> +#define OF_PHANDLE_ILLEGAL 0xdeadbeef > > Ugh. 0 and -1 are already reserved as illegal phandle values, don't > invent a new one. OK > >> + >> +#ifdef CONFIG_OF_RESOLVE >> + >> +int of_resolve(struct device_node *resolve); >> + >> +#else >> + >> +static inline int of_resolve(struct device_node *resolve) >> +{ >> + return -ENOTSUPP; >> +} >> + >> +#endif >> + >> #endif /* _LINUX_OF_H */ > > -- > 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 -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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@antoniou-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.
Hi On Jan 22, 2013, at 6:05 AM, David Gibson wrote: > 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@antoniou-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). > It would be pointless to number all the use-cases that Grant put in that long document, but I can summarize the cases that we've seen on the bone. * Addition of child device nodes to the ocp node, creates new platform devices of various kind (audio/video/pwms/timers) - almost any kind of platform device that the SoC supports. Removing the overlay unregisters the devices (but precious few drivers support that cleanly ATM). Since the capes don't support hotplug that's not a big deal. * Addition of pinctrl configuration nodes. * Addition of i2c/spi etc device nodes and modification of the parent's node status property to "okay", creates the bus platform device & registers the devices on the bus. Slight complication with i2c client devices which are not platform devices need special handling. * Modification of configuration parameters of a disabled device and subsequent enablement. >> 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. > Define shouldn't. Let's say someone inserts a bogus overlay that makes the system fail, or misbehave in some other manner. What is the different between using the overlay and inserting kernel module that fails in a similar manner? As far as supporting arbitrary overlays, that is not the case, and never will be. Now since the beaglebone is for hobbyists too, we can't restrict what a user can do in any way; but that person is free to do whatever he wants. >>> 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? > There is no user-space involved what so ever. The only place where user-space might be involved is supplying the dtbo firmware image as a result of the loader driver issuing request_firmware(). > 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. > No. Just no. It is a support nightmare when dealing with the hobbyist market. Something similar has been tried before. While I don't have an exact figure, having users tinker with any part of the bootloader/init sequence in order to add some new cape lead to an unacceptable number of RMAed boards, which had absolutely nothing wrong with them, besides the fact that the person tinkering with it couldn't get it booting after screwing it. This wiped off any amount of profit for the board maker; possibly even made the whole operation a loss. >> >>> 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? > We have a use case (for devices that must be inserted very early in the boot process) that the overlay is stored in a part of the base tree, not in a different dtbo file. Some phandle juggling there uses this function. This patchset only uses it on the base tree. >> 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. How about having a recursion limit argument set to something sane like 10? > >>>> + >>>> + 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. > Previous comment applies here too. Recursion limit counter? >>>> +} >>>> + >>>> +/** >>>> + * 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. Err, I misread that. At first I tried to do it the way you suggested but run into the following problem: The DTC enforces the properties to always precede the child nodes. I wasn't able to find a way to insert the __local_fixups__ property easily. Perhaps you can suggest where would you put it? > >>>> + /* 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. Maybe. You also lose the ability to print the contents of the node and easily inspect. It's your call really, I don't mind that much. Regards -- Pantelis > > -- > 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 -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, Jan 22, 2013 at 01:06:09PM +0200, Pantelis Antoniou wrote: > Hi > > On Jan 22, 2013, at 6:05 AM, David Gibson wrote: > > > 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@antoniou-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). > > > > It would be pointless to number all the use-cases that Grant put in that > long document, but I can summarize the cases that we've seen on the bone. > > * Addition of child device nodes to the ocp node, creates new platform > devices of various kind (audio/video/pwms/timers) - almost any kind of > platform device that the SoC supports. Removing the overlay unregisters > the devices (but precious few drivers support that cleanly ATM). Since > the capes don't support hotplug that's not a big deal. Ok, that's just adding nodes, which is straightforward. > * Addition of pinctrl configuration nodes. Ok, do you know where I can look to see how the pinctrl stuff works? > * Addition of i2c/spi etc device nodes and modification of the parent's > node status property to "okay", Ok. I'm assuming this is basically just to enable the bus controller which was previously disabled because it had nothing attached to it? > creates the bus platform device & registers > the devices on the bus. Slight complication with i2c client devices which > are not platform devices need special handling. And this part is again just adding nodes. > * Modification of configuration parameters of a disabled device and subsequent > enablement. What sorts of modification are necessary to the parameters? Other than changing status = "disabled" of course. This is the only case I see here which might be changing data other than the status property, which if it were the only one could reasonably be special cased, I think. > >> 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. > > Define shouldn't. Let's say someone inserts a bogus overlay that makes the system > fail, or misbehave in some other manner. What is the different between > using the overlay and inserting kernel module that fails in a > similar manner? Hm, you do have a point there. Another reason for a more constrained format did occur to me though - it opens the possibility for multiple interchangeable sockets on the same board. e.g. we get the BeaglePlus or whatever which can take two capes simultaneously. With the current scheme an identical cape would need different overlays for insertion in socket A and socket B because it would need different labels for all the fixups. > As far as supporting arbitrary overlays, that is not the case, and never will > be. Now since the beaglebone is for hobbyists too, we can't restrict what > a user can do in any way; but that person is free to do whatever he wants. > > >>> 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? > > There is no user-space involved what so ever. The only place where > user-space might be involved is supplying the dtbo firmware image as a > result of the loader driver issuing request_firmware(). Right.. and couldn't the request_firmware() involve processing the resolutions, rather than just grabbing the file. > > 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. > > No. Just no. It is a support nightmare when dealing with the hobbyist market. > Something similar has been tried before. While I don't have an exact figure, > having users tinker with any part of the bootloader/init sequence in order to > add some new cape lead to an unacceptable number of RMAed boards, which > had absolutely nothing wrong with them, besides the fact that the person tinkering > with it couldn't get it booting after screwing it. > This wiped off any amount of profit for the board maker; possibly even made the > whole operation a loss. Ok. I certainly don't want to dismiss usability concerns. I was envisaging this pre-resolving process asn being part of the platform's equivalent of running the lilo / update-grub / whatever script. So I'm trying to understand what's different about the platform's boot sequence it so much more fraught in this case. The other thing I notice is that this sort of concern is exactly why I lean towards the idea of a more constrained "overlay" format where the base tree desribes what a socket can add / change, rather than allowing arbitrary alterations to the base tree. With such constraints, it's going to be much harder to screw up the basic platform booting due to a bad overlay. Obviously the cape won't work properly without correct device information, but you won't be able to clobber the basic device information for the board. [snip] > >>> 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? > > We have a use case (for devices that must be inserted very early in the boot > process) that the overlay is stored in a part of the base tree, not in a > different dtbo file. Some phandle juggling there uses this function. > This patchset only uses it on the base tree. Ok. Nonetheless it's not hard to avoid a recursive approach here. > >> 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. > > How about having a recursion limit argument set to something sane > like 10? Ugh. It's really best just to avoid recursion in the kernel. > >>>> + > >>>> + 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. > > Previous comment applies here too. Recursion limit counter? Is a nasty band-aid, when it's really not that hard to avoid recursion altogether. > >>>> +} > >>>> + > >>>> +/** > >>>> + * 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. > > Err, I misread that. > > At first I tried to do it the way you suggested but run into the following > problem: The DTC enforces the properties to always precede the child nodes. > I wasn't able to find a way to insert the __local_fixups__ property easily. > Perhaps you can suggest where would you put it? Ah, ok. I haven't looked at your dtc patch in any detail yet, since I wanted to review the concept first, through these patches. So the basic answer is that you should add the extra annotations for overlays onto the live tree that dtc maintains before flatenning it, rather than doing it as part of the flattening process. On the live tree you can add properties and subnodes in whatever order. It also means that you'll see the right overlay annotations for all dtc output formats. > >>>> + /* 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. > > Maybe. You also lose the ability to print the contents of the node and > easily inspect. It's your call really, I don't mind that much. Hm, ok I'll give it some thought. There are bigger things I'd like to be convinced on before worrying too much about details like this :).
Hi David, On Jan 23, 2013, at 6:40 AM, David Gibson wrote: > On Tue, Jan 22, 2013 at 01:06:09PM +0200, Pantelis Antoniou wrote: >> Hi >> >> On Jan 22, 2013, at 6:05 AM, David Gibson wrote: >> >>> 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@antoniou-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). >>> >> >> It would be pointless to number all the use-cases that Grant put in that >> long document, but I can summarize the cases that we've seen on the bone. >> >> * Addition of child device nodes to the ocp node, creates new platform >> devices of various kind (audio/video/pwms/timers) - almost any kind of >> platform device that the SoC supports. Removing the overlay unregisters >> the devices (but precious few drivers support that cleanly ATM). Since >> the capes don't support hotplug that's not a big deal. > > Ok, that's just adding nodes, which is straightforward. > >> * Addition of pinctrl configuration nodes. > > Ok, do you know where I can look to see how the pinctrl stuff works? > There's information about it in Documentation/pinctrl.txt, and there's the source in drivers/pinctrl. >> * Addition of i2c/spi etc device nodes and modification of the parent's >> node status property to "okay", > > Ok. I'm assuming this is basically just to enable the bus controller > which was previously disabled because it had nothing attached to it? > Yes, but the configuration of the device might need to be altered. For example the clock, or adding/removing some properties that affect the driver. >> creates the bus platform device & registers >> the devices on the bus. Slight complication with i2c client devices which >> are not platform devices need special handling. > > And this part is again just adding nodes. > >> * Modification of configuration parameters of a disabled device and subsequent >> enablement. > > What sorts of modification are necessary to the parameters? Other > than changing status = "disabled" of course. This is the only case I > see here which might be changing data other than the status property, > which if it were the only one could reasonably be special cased, I > think. Some devices might not work reliably with a high clock value. Others might need to add a flag that modifies the way the driver handles the hardware attached to the cape (for instance whether an SDcard is removable or not) etc. It is all up to the person that designed the hardware which is connected, so we can't really come up with a list of allowed changes. > >>>> 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. >> >> Define shouldn't. Let's say someone inserts a bogus overlay that makes the system >> fail, or misbehave in some other manner. What is the different between >> using the overlay and inserting kernel module that fails in a >> similar manner? > > Hm, you do have a point there. > > Another reason for a more constrained format did occur to me though - > it opens the possibility for multiple interchangeable sockets on the > same board. e.g. we get the BeaglePlus or whatever which can take two > capes simultaneously. With the current scheme an identical cape would > need different overlays for insertion in socket A and socket B because > it would need different labels for all the fixups. I see your point, and that's where the beauty of DT helps :) There's nothing that forces you to have a single label per node, so the number of labels a node can have is unlimited. As part of a convention laid down by the board maker, if one was to support a number of capes the labels of the nodes must be known in advance. For instance, let's take the case of beaglebone capes where they typically hook into &ocp & &pinmux etc. The manufacturer can lay down a policy that states the only allowable overlay targets, and make sure that every board that support a given set of capes to label the nodes consistently. What we are describing here, the device tree resolution & overlay though is not policy, it is a mechanism. It's not by accident that there is no direct user-space way to use the DT overlays, the access to it should come from a board/family specific driver that knows and will enforce the policy. > >> As far as supporting arbitrary overlays, that is not the case, and never will >> be. Now since the beaglebone is for hobbyists too, we can't restrict what >> a user can do in any way; but that person is free to do whatever he wants. >> >>>>> 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? >> >> There is no user-space involved what so ever. The only place where >> user-space might be involved is supplying the dtbo firmware image as a >> result of the loader driver issuing request_firmware(). > > Right.. and couldn't the request_firmware() involve processing the > resolutions, rather than just grabbing the file. > That's not possible AFAIKT. All that request_firmware() can do is hand you a blob that you've requested. >>> 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. >> >> No. Just no. It is a support nightmare when dealing with the hobbyist market. >> Something similar has been tried before. While I don't have an exact figure, >> having users tinker with any part of the bootloader/init sequence in order to >> add some new cape lead to an unacceptable number of RMAed boards, which >> had absolutely nothing wrong with them, besides the fact that the person tinkering >> with it couldn't get it booting after screwing it. >> This wiped off any amount of profit for the board maker; possibly even made the >> whole operation a loss. > > Ok. I certainly don't want to dismiss usability concerns. I was > envisaging this pre-resolving process asn being part of the platform's > equivalent of running the lilo / update-grub / whatever script. So > I'm trying to understand what's different about the platform's boot > sequence it so much more fraught in this case. > Don't get me started on the insanity of lilo / update-grub process :) Let's just say there is no need to force anyone to deal with again, there's absolutely no need to. > The other thing I notice is that this sort of concern is exactly why I > lean towards the idea of a more constrained "overlay" format where the > base tree desribes what a socket can add / change, rather than > allowing arbitrary alterations to the base tree. With such > constraints, it's going to be much harder to screw up the basic > platform booting due to a bad overlay. Obviously the cape won't work > properly without correct device information, but you won't be able to > clobber the basic device information for the board. > I understand your concern, really. That's why the patches do not expose any kind of policy management, they're only a low level mechanism that a policy aware loader driver can use. I feel that the label method is sufficiently generic, and easily understood by both users & developers. Now as part of the policy of the loader driver we can easily add policy constraints as to which device nodes are allowed as targets of the overlay. If need be, the loader can use an extra verification pass. > [snip] >>>>> 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? >> >> We have a use case (for devices that must be inserted very early in the boot >> process) that the overlay is stored in a part of the base tree, not in a >> different dtbo file. Some phandle juggling there uses this function. >> This patchset only uses it on the base tree. > > Ok. Nonetheless it's not hard to avoid a recursive approach here. How can I find the maximum phandle value of a subtree without using recursion. Note that the whole function is just 6 lines long. > >>>> 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. >> >> How about having a recursion limit argument set to something sane >> like 10? > > Ugh. It's really best just to avoid recursion in the kernel. > It is not always possible, these are non-balanced tree structures after all. Look at lib/btree.c btree_insert_level() function for example. It uses recursion and uses a recursion level check. >>>>>> + >>>>>> + 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. >> >> Previous comment applies here too. Recursion limit counter? > > Is a nasty band-aid, when it's really not that hard to avoid recursion > altogether. > >>>>>> +} >>>>>> + >>>>>> +/** >>>>>> + * 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. >> >> Err, I misread that. >> >> At first I tried to do it the way you suggested but run into the following >> problem: The DTC enforces the properties to always precede the child nodes. >> I wasn't able to find a way to insert the __local_fixups__ property easily. >> Perhaps you can suggest where would you put it? > > Ah, ok. I haven't looked at your dtc patch in any detail yet, since I > wanted to review the concept first, through these patches. > > So the basic answer is that you should add the extra annotations for > overlays onto the live tree that dtc maintains before flatenning it, > rather than doing it as part of the flattening process. On the live > tree you can add properties and subnodes in whatever order. It also > means that you'll see the right overlay annotations for all dtc output > formats. Already do (partly). The annotations are added before the flattening process. But I don't add the properties at that point. My life is complicated by the fact that I have to be extra careful to not affect non-uses of overlays. Sigh. > >>>>>> + /* 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. >> >> Maybe. You also lose the ability to print the contents of the node and >> easily inspect. It's your call really, I don't mind that much. > > Hm, ok I'll give it some thought. There are bigger things I'd like to > be convinced on before worrying too much about details like this :). You worry too much! :) > > -- > 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 -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wed, 23 Jan 2013 12:58:02 +0200, Pantelis Antoniou <panto@antoniou-consulting.com> wrote: > Hi David, > > On Jan 23, 2013, at 6:40 AM, David Gibson wrote: > > Ok. Nonetheless it's not hard to avoid a recursive approach here. > > How can I find the maximum phandle value of a subtree without using recursion. > Note that the whole function is just 6 lines long. It's a failure in the existing kernel DT data structures. We need a hash lookup for the phandles to eliminate the search entirely. Then you'd be able to allocated new phandles on the fly easily and resolve phandles without searching the whole tree (which has always been horrible). That said, I'd like to punt on the whole phandle resolution thing. The DT overlay support can be merged without the phandle resolution support if the core rejects any overlays with phandle collisions. g. -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Grant, On Mar 16, 2013, at 11:24 AM, Grant Likely wrote: > On Wed, 23 Jan 2013 12:58:02 +0200, Pantelis Antoniou <panto@antoniou-consulting.com> wrote: >> Hi David, >> >> On Jan 23, 2013, at 6:40 AM, David Gibson wrote: >>> Ok. Nonetheless it's not hard to avoid a recursive approach here. >> >> How can I find the maximum phandle value of a subtree without using recursion. >> Note that the whole function is just 6 lines long. > > It's a failure in the existing kernel DT data structures. We need a hash > lookup for the phandles to eliminate the search entirely. Then you'd be > able to allocated new phandles on the fly easily and resolve phandles > without searching the whole tree (which has always been horrible). > Yes, it is pretty obvious that the in-kernel data structures are sub-optimal. But I was not after modifying them, since that's a different kind of problem. Since we're having a 'sub-optimal' data structures, I'd like to point out that the usage of of_find_by_name(), mostly by drivers trying to find a child of their own node, works by a lucky accident of how the device nodes are instantiated by the flat tree loader. Most of the use cases should be replaced by a call to of_get_child_by_name() which does the right thing. > That said, I'd like to punt on the whole phandle resolution thing. The > DT overlay support can be merged without the phandle resolution support > if the core rejects any overlays with phandle collisions. > Fair enough, but be warned that phandle resolution the overlay feature is mostly useless. In actual practice the amount of driver nodes that can be overlaid without a single case of referencing phandles outside (or within) their own blob is close to zero. > g. Regards -- Pantelis -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tue, 19 Mar 2013 13:51:01 +0200, Pantelis Antoniou <panto@antoniou-consulting.com> wrote: > Hi Grant, > > On Mar 16, 2013, at 11:24 AM, Grant Likely wrote: > > > On Wed, 23 Jan 2013 12:58:02 +0200, Pantelis Antoniou <panto@antoniou-consulting.com> wrote: > >> Hi David, > >> > >> On Jan 23, 2013, at 6:40 AM, David Gibson wrote: > >>> Ok. Nonetheless it's not hard to avoid a recursive approach here. > >> > >> How can I find the maximum phandle value of a subtree without using recursion. > >> Note that the whole function is just 6 lines long. > > > > It's a failure in the existing kernel DT data structures. We need a hash > > lookup for the phandles to eliminate the search entirely. Then you'd be > > able to allocated new phandles on the fly easily and resolve phandles > > without searching the whole tree (which has always been horrible). > > > > Yes, it is pretty obvious that the in-kernel data structures are sub-optimal. > But I was not after modifying them, since that's a different kind of problem. Think about it this way; fixing up that aspect of the data structure makes the job you're trying to do a lot easier. I don't feel bad about asking you to add a radix tree for phandle lookups when it makes your patches a whole lot better. :-) > Since we're having a 'sub-optimal' data structures, I'd like to point out that > the usage of of_find_by_name(), mostly by drivers trying to find a child > of their own node, works by a lucky accident of how the device nodes are instantiated > by the flat tree loader. Most of the use cases should be replaced by a call > to of_get_child_by_name() which does the right thing. It is true. In fact, calling of_find_node_by_name() when using .dtb is most likely a bug since using node name to determine behaviour is strongly discouraged. > Fair enough, but be warned that phandle resolution the overlay feature is mostly useless. > > In actual practice the amount of driver nodes that can be overlaid without a single case > of referencing phandles outside (or within) their own blob is close to zero. That's not what I'm saying. I'm saying that (at least for now) we should require the overlay to already know the phandles from the parent and to refuse to load an overlay that defines phandles already in use in the base. Overlays do become usable at that point. A mechanism for phandle resolution so that conflicts can be found and resolved can be added as a feature enhancement. By splitting it out you'll be able to get the overlay feature merged even if we don't have agreement on the resolution mechanism yet. g. -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Grant, On Mar 19, 2013, at 7:18 PM, Grant Likely wrote: > On Tue, 19 Mar 2013 13:51:01 +0200, Pantelis Antoniou <panto@antoniou-consulting.com> wrote: >> Hi Grant, >> >> On Mar 16, 2013, at 11:24 AM, Grant Likely wrote: >> >>> On Wed, 23 Jan 2013 12:58:02 +0200, Pantelis Antoniou <panto@antoniou-consulting.com> wrote: >>>> Hi David, >>>> >>>> On Jan 23, 2013, at 6:40 AM, David Gibson wrote: >>>>> Ok. Nonetheless it's not hard to avoid a recursive approach here. >>>> >>>> How can I find the maximum phandle value of a subtree without using recursion. >>>> Note that the whole function is just 6 lines long. >>> >>> It's a failure in the existing kernel DT data structures. We need a hash >>> lookup for the phandles to eliminate the search entirely. Then you'd be >>> able to allocated new phandles on the fly easily and resolve phandles >>> without searching the whole tree (which has always been horrible). >>> >> >> Yes, it is pretty obvious that the in-kernel data structures are sub-optimal. >> But I was not after modifying them, since that's a different kind of problem. > > Think about it this way; fixing up that aspect of the data structure > makes the job you're trying to do a lot easier. I don't feel bad about > asking you to add a radix tree for phandle lookups when it makes your > patches a whole lot better. :-) > Oh that's going to be fun... maybe. >> Since we're having a 'sub-optimal' data structures, I'd like to point out that >> the usage of of_find_by_name(), mostly by drivers trying to find a child >> of their own node, works by a lucky accident of how the device nodes are instantiated >> by the flat tree loader. Most of the use cases should be replaced by a call >> to of_get_child_by_name() which does the right thing. > > It is true. In fact, calling of_find_node_by_name() when using .dtb is > most likely a bug since using node name to determine behaviour is > strongly discouraged. Maybe mark the function as deprecated then? Easier to get people to fix it. > >> Fair enough, but be warned that phandle resolution the overlay feature is mostly useless. >> >> In actual practice the amount of driver nodes that can be overlaid without a single case >> of referencing phandles outside (or within) their own blob is close to zero. > > That's not what I'm saying. I'm saying that (at least for now) we should > require the overlay to already know the phandles from the parent and to > refuse to load an overlay that defines phandles already in use in the > base. Overlays do become usable at that point. A mechanism for phandle > resolution so that conflicts can be found and resolved can be added > as a feature enhancement. By splitting it out you'll be able to get the > overlay feature merged even if we don't have agreement on the resolution > mechanism yet. > As long as we don't come up with something that's too difficult to use for non kernel developers. I would hate to punt and push all the complexity of phandle resolution to the driver/cape developer. FWIW, the resolution mechanism we use on the beaglebone seems to work just fine, and people seem to handle it just fine. The part that trips them is mostly how to install the updated dtc compiler that's required. > g. > Regards -- Pantelis -- To unsubscribe from this list: send the line "unsubscribe linux-omap" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 d37bfcf..f9a6193 100644 --- a/drivers/of/Kconfig +++ b/drivers/of/Kconfig @@ -83,4 +83,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 abcc08a..9a809c9 100644 --- a/drivers/of/Makefile +++ b/drivers/of/Makefile @@ -11,3 +11,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..016d7da --- /dev/null +++ b/drivers/of/resolver.c @@ -0,0 +1,394 @@ +/* + * Functions for dealing with DT resolution + * + * Copyright (C) 2012 Pantelis Antoniou <panto@antoniou-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/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; + + /* 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; + + /* 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); + 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 == 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; + __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 == NULL && + of_node_cmp(child->name, "__symbols__") == 0) + resolve_sym = child; + + if (resolve_fix == NULL && + of_node_cmp(child->name, "__fixups__") == 0) + resolve_fix = child; + + /* both found, don't bother anymore */ + if (resolve_sym != NULL && resolve_fix != NULL) + break; + } + + /* we do allow for the case where no fixups are needed */ + if (resolve_fix == NULL) + goto merge_sym; + + /* we need to fixup, but no root symbols... */ + if (root_sym == NULL) + return -EINVAL; + + 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 err_fail; + } + + refnode = of_find_node_by_path(refpath); + if (refnode == NULL) { + pr_err("%s: Could not find node by path '%s'\n", + __func__, refpath); + err = -ENOENT; + goto err_fail; + } + + 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 == NULL) { + pr_err("%s: Could not copy value of '%s'\n", + __func__, rprop->name); + err = -ENOMEM; + goto err_fail; + } + + 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__, (char *)rprop->value); + kfree(propval); + 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); + kfree(propval); + 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(resolve, + nodestr); + if (refnode == NULL) { + pr_err("%s: Could not find refnode '%s'\n", + __func__, (char *)rprop->value); + kfree(propval); + err = -ENOENT; + goto err_fail; + } + + /* 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); + kfree(propval); + err = -ENOENT; + goto err_fail; + } + + *(uint32_t *)(sprop->value + offset) = + cpu_to_be32(phandle); + } + + kfree(propval); + } + +merge_sym: + + of_node_put(root_sym); + + return 0; + +err_fail: + + if (root_sym != NULL) + of_node_put(root_sym); + + return err; +} diff --git a/include/linux/of.h b/include/linux/of.h index c38e41a..ab52243 100644 --- a/include/linux/of.h +++ b/include/linux/of.h @@ -650,4 +650,21 @@ static inline int of_multi_prop_cmp(const struct property *prop, const char *val #endif /* !CONFIG_OF */ + +/* illegal phandle value (set when unresolved) */ +#define OF_PHANDLE_ILLEGAL 0xdeadbeef + +#ifdef CONFIG_OF_RESOLVE + +int of_resolve(struct device_node *resolve); + +#else + +static inline int of_resolve(struct device_node *resolve) +{ + return -ENOTSUPP; +} + +#endif + #endif /* _LINUX_OF_H */
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@antoniou-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