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 13:
(17 comments)
File src/include/device_tree.h:
https://review.coreboot.org/c/coreboot/+/81081/comment/347b1794_1463f985 : PS12, Line 206: uintptr_t
Sorry, I just realized that you're using uintptr_t for node offsets everywhere, while all the existi […]
Done
https://review.coreboot.org/c/coreboot/+/81081/comment/48a5d568_e99923f8 : PS12, Line 207: uint8_t addr_cells, uint8_t size_cells,
Why u8? If anything, these should be u32.
Done
https://review.coreboot.org/c/coreboot/+/81081/comment/54402f67_b307a3b3 : PS12, Line 208: int n)
Not a big difference but the array size should probably be size_t.
Done
File src/lib/device_tree.c:
https://review.coreboot.org/c/coreboot/+/81081/comment/d26e414c_d30f01b9 : PS6, Line 118: //TODO replace with something more computational
Hmm... okay. You should take out the TODO then.
Done
File src/lib/device_tree.c:
https://review.coreboot.org/c/coreboot/+/81081/comment/d2597837_0dd76965 : PS12, Line 174: fdt_find_node_by_path_rel
I would suggest to just call this `fdt_find_node()`, because it's basically the FDT equivalent of `d […]
Done
https://review.coreboot.org/c/coreboot/+/81081/comment/370004a8_1ee598ba : PS12, Line 200: if (!strncmp(*path, node_name, path_sub_len)) {
I think you're forgetting to check that node_name isn't longer than the path segment here, you would […]
I fixed the `strncmp` to also check the character after the 'subpath'. A slash '/' is not a valid node name character so it shouldn't cause any problems. I could also write it like this: ``` if (!strncmp(*path, node_name, path_sub_len) && strlen(node_name) == path_sub_len) ```
I can't use strdup, because I can't use malloc/heap. Putting it in an array would make complicated use of the stack if we can't use the heap.
https://review.coreboot.org/c/coreboot/+/81081/comment/6a1c997a_ece9919f : PS12, Line 214: ince address-cells and size-cells are not inherited
That's not my understanding and not how the unflattened DT functions are implemented. […]
I think I will investigate a bit on how other projects handle that. I didn't get the impression though, that it is known good to treat these properties as inheritable.
https://review.coreboot.org/c/coreboot/+/81081/comment/73374e5f_276af485 : PS12, Line 234: (offset - size)
I think it would be cleaner to do the temporary offset adjustment in a different variable, rather th […]
How would you call the variable? `node_offset` is already taken (but I guess we could just use that one) and I feel like it could look more complicated if I add another variable.
https://review.coreboot.org/c/coreboot/+/81081/comment/652ce047_d9c58d9b : PS12, Line 235: } while (be32dec(blob + offset) != FDT_TOKEN_END_NODE);
Can't the whole thing just be a […]
Using your approach you cannot distinguish between a faulty FDT and just running to the end of your loop/search. To be fair I currently also not have an error message in place for that. I am not sure if that is important for us though. Maybe useful to know if the FDT is broken somewhere after the header.
https://review.coreboot.org/c/coreboot/+/81081/comment/40581a76_7bdc9552 : PS12, Line 280: *
Not all parameters documented.
Done
https://review.coreboot.org/c/coreboot/+/81081/comment/ca1606bc_190fa703 : PS12, Line 285: uint16_t
Why is this uint16_t? Should probably be size_t.
Done
https://review.coreboot.org/c/coreboot/+/81081/comment/078bdada_f19faafb : PS12, Line 293: printk(BIOS_ERR, "%s: prefix must not start with a /\n", __func__);
I'm not sure there's a point in checking this, I mean if people are going to call this with garbage […]
Done
https://review.coreboot.org/c/coreboot/+/81081/comment/3dd85b5e_04be0f5d : PS12, Line 297: printk(BIOS_ERR, "%s: prefix must not be empty/\n", __func__);
Why not? That would be a convenient way to get all subnodes.
Sure but I am not sure how obvious it is. I will just add it to the description of the function.
https://review.coreboot.org/c/coreboot/+/81081/comment/c9d01454_b3c0919e : PS12, Line 305: if(!size) {
This shouldn't be possible (right?), so it should be an assert or at least print an error.
I think an assert is a bit radical in this context. Could be a faulty FDT or whatever. But you can potentially still ignore it for a successful boot up. But the error message is important.
https://review.coreboot.org/c/coreboot/+/81081/comment/d38cf98e_d10b1e55 : PS12, Line 323: offset += size;
Since you need this twice it would probably be good to factor it out into a separate function (maybe […]
Done
https://review.coreboot.org/c/coreboot/+/81081/comment/78146373_a2b3f370 : PS12, Line 337: results[(*count_results)++] = offset;
Don't you need some part that tells you how big the original results array that got passed in was, s […]
Done I now check for it and print a warning. Not sure if we want to treat that as error? Maybe someone wants to just get the first 3 (or something) nodes with a given prefix?
https://review.coreboot.org/c/coreboot/+/81081/comment/7f98ed5a_9770669c : PS12, Line 348: return results[*count_results - 1];
This is odd, why return exactly that one? Maybe we should just return the count instead?
At some point I thought it was useful, but I think that was a few patchsets ago. Replaced it by returning the count now.
-
Maximilian Brune (Code Review)