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 13:
(8 comments)
Patchset:
PS13: BTW can you please fix all the checkpatch errors?
File src/include/device_tree.h:
https://review.coreboot.org/c/coreboot/+/81081/comment/1fbdb834_d1830d3f : PS13, Line 210: fdt_find_nodes_by_prefix nit: I would choose a different name because all the other `fdt_find_...` functions operate on he whole blob, this just under a specific node. Maybe `fdt_subnodes_with_prefix()`?
File src/lib/device_tree.c:
https://review.coreboot.org/c/coreboot/+/81081/comment/08c69354_f03399eb : PS12, Line 200: if (!strncmp(*path, node_name, path_sub_len)) {
I fixed the `strncmp` to also check the character after the 'subpath'. […]
Okay, then why not use the stack? These path strings aren't gonna be very long. Just use `alloca(strlen(path) + 1)` and then copy it in.
https://review.coreboot.org/c/coreboot/+/81081/comment/437924cd_590b1418 : PS12, Line 234: (offset - size)
How would you call the variable? `node_offset` is already taken (but I guess we could just use that […]
How about calling it `prop_offset` (because that's where the properties start) and the other one `node_offset`?
https://review.coreboot.org/c/coreboot/+/81081/comment/226865dc_6ea4d4b1 : PS12, Line 235: } while (be32dec(blob + offset) != FDT_TOKEN_END_NODE);
Using your approach you cannot distinguish between a faulty FDT and just running to the end of your […]
I don't really see how your code does anything different? You're still calling `fdt_next_node_name()` and erroring out if it returns 0, just in a way that's harder to read (I think).
I also don't think there's a point in going too overboard in error detection. It's not like the firmware can really do much about that anyway. We're already taking a blob pointer without a size and parsing it, so we're already putting all our faith in the FDT being well-formatted anyway.
https://review.coreboot.org/c/coreboot/+/81081/comment/5f09353a_f4618bd1 : PS12, Line 337: results[(*count_results)++] = offset;
Done […]
Maybe make it return the actual count of entries, but only fill as much as fits in the array? Then the caller can determine that there was an overrun if the function returned a larger number than the array size it put in. (But that might also make badly-written callers read beyond the end of their own array, so maybe it's safer to just ignore that case and always return the number of entries written.)
File src/lib/device_tree.c:
https://review.coreboot.org/c/coreboot/+/81081/comment/3b73bbc3_d36d8799 : PS13, Line 169: return offset; // return offset to reg property Should this maybe return `count` instead? Otherwise how does the caller know how many addresses it got?
https://review.coreboot.org/c/coreboot/+/81081/comment/ba4a9a60_9f4f99bc : PS13, Line 342: if (count_results == 0) nit: This case seems to be redundant with the main return path below.