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 20:
(25 comments)
Patchset:
PS20: Phew... okay, I think we're getting very close, thanks for bearing with me. Mostly small nits about comments left now.
File src/include/device_tree.h:
https://review.coreboot.org/c/coreboot/+/81081/comment/c2c15697_e55dfb49 : PS20, Line 117: /* Find multiple nodes matching a given pattern relative to a parent node */ nit: "Returns number of nodes found."
https://review.coreboot.org/c/coreboot/+/81081/comment/cc0a41ca_32840596 : PS20, Line 123: /* Read the node name into 'name' of the node behind 'node_offset' */ "and return total bytes used for name"
File src/lib/device_tree.c:
https://review.coreboot.org/c/coreboot/+/81081/comment/fac55114_2bee51bb : PS20, Line 92: nit: This should probably also start with skip_nops (instead or in addition to at the end).
https://review.coreboot.org/c/coreboot/+/81081/comment/5c3496a3_6a9eec8b : PS20, Line 165: printk(BIOS_ERR, "reg property at node_offset: %x has more entries (%zd)" \ Prefer to exceed the line limit rather than breaking strings. (Also, you're actually swallowing a space in the continuation here.)
https://review.coreboot.org/c/coreboot/+/81081/comment/bf774dfe_5788ac46 : PS20, Line 166: addrs Should this be `'regions'` (or `input array` or something)?
https://review.coreboot.org/c/coreboot/+/81081/comment/2489fe26_1b680eab : PS20, Line 214: * the search we are. These are not updated to current implementation.
https://review.coreboot.org/c/coreboot/+/81081/comment/8266df5b_090ff027 : PS20, Line 233: // intentionally follow the Linux implementation here and treat them as inheritable. nit: multi-line comments should not use C99 style (https://doc.coreboot.org/contributing/coding_style.html#commenting).
https://review.coreboot.org/c/coreboot/+/81081/comment/737be0be_35690b98 : PS20, Line 244: = node_offset + nit: elsewhere you use `+=`
https://review.coreboot.org/c/coreboot/+/81081/comment/a51f6be1_e9e7f34f : PS20, Line 252: * fdt_read_reg_prop reads the reg property inside a node Wrong function
https://review.coreboot.org/c/coreboot/+/81081/comment/77d3dc02_2c54044b : PS20, Line 305: * Find a node in the tree from a string device tree path. Wrong description
https://review.coreboot.org/c/coreboot/+/81081/comment/b7c8550e_ddcf743c : PS20, Line 349: // we have to set them to a default value if we can't find them This is not consistent with the decision in `fdt_find_node()` to inherit them. You should be leaving the values the way they were passed in and then just do one `fdt_read_cell_props()` to possibly update them. (The general idea should be that one calls `fdt_find_node_by_path()` to find the parent node, then call `fdt_find_subnodes_by_prefix()` on the result of that with the same `addrcp` and `sizecp` that fell out of `fdt_find_node_by_path()`.)
https://review.coreboot.org/c/coreboot/+/81081/comment/23cbc610_443295c3 : PS20, Line 365: if (count_results >= results_len) nit: could just move below warning here to avoid writing the check twice.
https://review.coreboot.org/c/coreboot/+/81081/comment/933ccb20_5cfa1b51 : PS20, Line 374: = nit: `+=`?
https://review.coreboot.org/c/coreboot/+/81081/comment/b1dafcf2_46375940 : PS20, Line 375: } while (be32dec(blob + offset) != FDT_TOKEN_END_NODE); I think you can also write this as a `while ((size = fdt_next_node_name(blob, offset, &node_name)))` to be a bit shorter and more consistent with `fdt_find_node()`.
https://review.coreboot.org/c/coreboot/+/81081/comment/029d9129_30b17549 : PS20, Line 399: return node_offset + prop_offset; This makes no sense, `node_offset + prop_offset` is meaningless (`prop_offset` on its own is already an absolute offset in `blob`.
Also, is there really any point in making this return that offset? Why not just make it return a `const char *`?
https://review.coreboot.org/c/coreboot/+/81081/comment/a4f464cd_92e80164 : PS20, Line 406: * @params alias_name offset to the node of which the children should be searched Wrong comment
https://review.coreboot.org/c/coreboot/+/81081/comment/64e90d39_08b9c9b4 : PS20, Line 589: static bool fdt_is_valid(const void *blob) nit: It seems a bit odd to factor this function out now if you're not using it anywhere. Did you mean to make it non-static so that API callers could call it before accessing an FDT blob?
File tests/lib/device_tree-test.c:
https://review.coreboot.org/c/coreboot/+/81081/comment/d967ba94_895e38c5 : PS7, Line 13: // regular FDT from RISC-V QEMU virt target
I will add separate commit to create the common code. The current patch is already big enough.
Is this still plan-of-record? When you said "separate commit" I had assumed you would put the commit before this one in the patch series so that this commit could then immediately use the new test file loading API.
File tests/lib/device_tree-test.c:
https://review.coreboot.org/c/coreboot/+/81081/comment/d597cc9b_dbf318aa : PS20, Line 13: // regular FDT from RISC-V QEMU virt target Would be good to add links to git.kernel.org here (with the exact commit SHA) to identify the source file exactly.
https://review.coreboot.org/c/coreboot/+/81081/comment/e93d71ca_04bca950 : PS20, Line 46: // code coverage confirmed fdt_find_node_by_path was called recursively 4 times (as expected) This comment seems out of date? `fdt_find_node_by_path()` shouldn't be called recursively, only `fdt_find_node()`, and I'd be surprised if it took that much stack space.
https://review.coreboot.org/c/coreboot/+/81081/comment/4e67a679_f7fb2d35 : PS20, Line 54: uint32_t results[3] = { 0 }; Would also be good to test both cases (array big enough and array not big enough) here, and ensure in the not-big-enough case that it didn't write too far (by putting a sentinel value in the latter fields and passing a smaller size).
https://review.coreboot.org/c/coreboot/+/81081/comment/52430a2f_a595ab9b : PS20, Line 74: nit: test a path that doesn't exist?
https://review.coreboot.org/c/coreboot/+/81081/comment/2357389d_a0ff6e0e : PS20, Line 79: } ...and a property that doesn't exist
https://review.coreboot.org/c/coreboot/+/81081/comment/635f2c79_32f5f761 : PS20, Line 86: struct device_tree_region regions[1]; nit: would be nice to test this with at least two regions (and also would be nice to test the case where the array isn't big enough, and ensure that no data gets written beyond its end).