Attention is currently required from: Jakub Czapiga, Martin L Roth, Maximilian Brune.
25 comments:
Patchset:
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:
Patch Set #20, Line 117: /* Find multiple nodes matching a given pattern relative to a parent node */
nit: "Returns number of nodes found."
Patch Set #20, 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:
nit: This should probably also start with skip_nops (instead or in addition to at the end).
Patch Set #20, 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.)
Patch Set #20, Line 166: addrs
Should this be `'regions'` (or `input array` or something)?
Patch Set #20, Line 214: * the search we are.
These are not updated to current implementation.
Patch Set #20, 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).
Patch Set #20, Line 244: = node_offset +
nit: elsewhere you use `+=`
Patch Set #20, Line 252: * fdt_read_reg_prop reads the reg property inside a node
Wrong function
Patch Set #20, Line 305: * Find a node in the tree from a string device tree path.
Wrong description
Patch Set #20, 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()`.)
Patch Set #20, Line 365: if (count_results >= results_len)
nit: could just move below warning here to avoid writing the check twice.
nit: `+=`?
Patch Set #20, 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()`.
Patch Set #20, 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 *`?
Patch Set #20, Line 406: * @params alias_name offset to the node of which the children should be searched
Wrong comment
Patch Set #20, 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:
Patch Set #7, 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:
Patch Set #20, 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.
Patch Set #20, 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.
Patch Set #20, 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).
nit: test a path that doesn't exist?
...and a property that doesn't exist
Patch Set #20, 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).
To view, visit change 81081. To unsubscribe, or for help writing mail filters, visit settings.