Attention is currently required from: Felix Held, Julius Werner, Lean Sheng Tan, Matt DeVillier, Maximilian Brune.
Benjamin Doron has posted comments on this change by Benjamin Doron. ( https://review.coreboot.org/c/coreboot/+/84796?usp=email )
Change subject: lib/{fit,fit_payload}.c: Enhance support for FIT images ......................................................................
Patch Set 6:
(2 comments)
File src/lib/fit_payload.c:
https://review.coreboot.org/c/coreboot/+/84796/comment/6b07322a_490b5c50?usp... : PS6, Line 18: __weak void platform_fdt_add_secondaries(struct device_tree *tree, const char *name, struct region *secondary)
I think weak functions should only be used when the base case is a very trivial "default" implementa […]
Ah, that's fair. Overusing weak functions is the sort of thing that makes platform/feature bringup more difficult too, if they hide the fact that the developer is obligated to provide the function definition. I disagree that they shouldn't be allowed to implement significant logic though, because sometimes there is a reasonable default case that we just want to be able to override in limited cases, but that's not the point here. This code makes another mistake.
The point was that implementations of this are only going to differ in what's in `snprintf`'s string, but I didn't want to implement that independently: it's not a constant string, and we can't return strings that are on our stack: a char array is a pointer, and this stack frame is about to get lost. Now that I'm thinking about it, I could turn the weak part into a 'return-path-prefix' that's called by a function in this file, but we can probably just remove this because of the below.
About `/firmware/coreboot/image@...`: yes, I made it up. I didn't realise `/firmware/coreboot/` is actually specced, and I especially didn't realise that the Linux kernel defines it. I figured there was an unspoken agreement, but I didn't think any longer about how they'd want it specced because I don't develop it.
UPL wants to use `/options/upl-images/image@%llx`. Ultimately, extra images aren't needed by others at the moment, so I'm thinking I'll just print a warning here, maybe a `BUG()` and implement it properly in the UPL-specific followup. If this gets wider use, we should probably define something in the FIT spec for everybody to use, rather than letting everybody pick their own path.
https://review.coreboot.org/c/coreboot/+/84796/comment/e80f8f94_1b8d6ee3?usp... : PS6, Line 306: dt_print_node(dt->root);
This prints the whole DT, right? I don't think we should do that by default? If you think it is usef […]
Yes. I extracted it out of upl_fdt_table (the follow-up) since the FDT is changed here, and then I figured it may as well stay since it can be useful to know. I'm happy dropping it now, aligning with CBTABLEs, and because it is quite verbose.
-
Benjamin Doron (Code Review)