Attention is currently required from: Jakub Czapiga, Julius Werner, Martin L Roth.
Maximilian Brune has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/81081?usp=email )
Change subject: lib/device_tree: Add some FDT helper functions ......................................................................
Patch Set 18:
(1 comment)
File src/lib/device_tree.c:
https://review.coreboot.org/c/coreboot/+/81081/comment/056e60fa_2941e765 : PS18, Line 175: ""
Yeah, you're right, sorry. […]
``` Right now I believe(?) you want node_offset in fdt_find_node() to be pointing the the start of an array of child nodes, right? But that's not how you're calling it from fdt_find_node_by_path(). You're calling it with fdt_hdr->struct_offset, which points to the very start of the root node, not to an array of its children. The root node is a full node that begins with a FDT_BEGIN_NODE token, then a name (which is normally just four '\0' bytes), then potentially a bunch of FDT_PROP tokens, then the whole subtree of child nodes, and finally an FDT_END_NODE. So that means that when you first enter fdt_find_node() and immediately jump into that do-while loop, you're trying to iterate over an array of nodes but you actually only have a single root node. I guess it doesn't matter as long as the path you're searching can always be found under that root node, but theoretically, if the path check wasn't stopping it, your code would continue running over the end of the root node (and thereby the whole struct block) in search for more nodes. ``` I don't expect it to point at the start of an array of child nodes. I simply expect `offset` to point at a node (nothing more). Sure the root is usually only 1 node, but my code doesn't care if there is 1, 2, 3 or n nodes or if the node you are starting at is the first, second or ... child. It treats the root node like any other node therefore offset can point to it. The path check will always stop it even if there is only a single root node and nothing else. That is the reason why the first part of the path is always an empty string ("") if you do an absolute lookup (-> relative to root node). The empty string will always match the root node (or anything else for that matter) so I don't see how it is possible to skip the root node and read something after that. ``` I think it would make more sense to pass the offset of the start of the properties (i.e. what you get after you call fdt_next_node_name()). Then it can either return that immediately if we've reached the NULL in the path array, or it can parse addr/size cell props and then go iterate through the children if it hasn't. ``` That would mean that all functions that work on the properties of a node always get the offset to the start of the properties instead of the offset to the node. Seems weird to be honest since you usually traverse all properties from a given node to get the property you want. I am also not sure if the naming scheme is correct then. Since `fdt_find_node` and `fdt_find_node_by_path` do not really find the node but more like the properties of a node. ``` u32 fdt_find_node(const void *blob, u32 prop_start_offset, const char** path, u32 *addrcp, u32 *sizecp) { size_t size; const char *node_name;
if (!*path) return prop_start_offset;
u32 offset = fdt_read_cell_props(blob, prop_start_offset, addrcp, sizecp);
while ((size = fdt_next_node_name(blob, offset, &node_name))) { if (!strcmp(*path, node_name)) return fdt_find_node(blob, offset + size, path + 1, addrcp, sizecp); offset += fdt_skip_node(blob, offset); }
return 0; } ``` your implementation always finds a node relative to a parent node. `fdt_find_node` currently always finds a node relative to another node on the same level (for performance reasons). Much the same as the `dt_find_node` equivalent I mentioned (with the difference being that you now return the offset to the first property). The idea originally was that you could continue the search from a given node without having to traverse all children nodes again. Therefore one could first search for `/cpus/cpu@0` and afterwards search for `cpu@1` node relative to the `cpu@0` node and not relative to the `/cpus ` node. In a node with many children that can potentially save a significant amount of time. At this point though it doesn't make much sense anymore (it did when I still used the wildcard stuff) since it now implies that the order of the nodes in the same level is known (for example that cpu@1 comes after cpu@0 in the devicetree). I wanted to the most performant way to get the offset of a node relative to another node (without having to traverse all children every time). But no function currently makes use of that (since the wildcard is out now) so I don't feel strongly about keeping it that way. ``` (BTW, have you noticed that you're basically parsing each node name twice, because you need to first call fdt_next_node_name() manually to have the name to strcmp(), but then fdt_skip_node() wants to call fdt_next_node_name() again? It would probably be good to optimize that, maybe by adding a skip_name parameter to fdt_skip_node().) ``` I noticed, but fdt_next_node_name doesn't really do much except the `strlen()`. Also I am not sure what the skip_name parameter would do differently compared to `fdt_next_node_name`? Would `skip_name` imply that that the offset given to `fdt_skip_node()` points to the first property of the node instead of the name?