Attention is currently required from: Jakub Czapiga, Martin L Roth, Maximilian Brune.
Julius Werner 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/314ecaa8_bf4bbf8a : PS18, Line 175: ""
You always want the addr/size cells from the parent node, since these are the ones that influence th […]
Yeah, you're right, sorry. `dt_find_node()` is doing something different, that's what threw me for a loop there. I'm not sure if that's a bug or just intentional lazyness (because in practice nodes that have reg props don't also tend to have addr/size cell props for their children that are different than their own), but it wouldn't be hard to fix (just move the `dt_read_cell_props()` below the abort condition check), so we probably should.
For your case... hmm... actually, upon reading it closely again I think you're currently making a subtle mistake with what you pass through the offset parameter, and if you fix that, this should become easier:
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 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. So basically in `fdt_find_node_by_path()` you'd start by calling ``` u32 struct_offset = be32toh(fdt_hdr->structure_offset); size_t name_size = fdt_next_node_name(blob, struct_offset, NULL); fdt_find_node(blob, struct_offset + name_size, path_array /* of the form { "cpus", "cpu@1", NULL } */, addrcp, sizecp); ``` and then `fdt_find_node()` would look roughly like this: ``` 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; } ``` Would that make sense?
(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()`.)