Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/41209 )
Change subject: fw_config: Add firmware configuration interface ......................................................................
Patch Set 7:
(5 comments)
https://review.coreboot.org/c/coreboot/+/41209/7/src/include/fw_config.h File src/include/fw_config.h:
https://review.coreboot.org/c/coreboot/+/41209/7/src/include/fw_config.h@43 PS7, Line 43: * @match_list: List of field and options to check. Should clarify that this needs to be terminated by an empty entry. In general maybe avoid the term 'list' (which probably makes most people think of linked lists) and call them zero-terminated arrays or something.
I'm a bit worried that people may call fw_config_probe(FW_CONFIG(...)) and not notice their error, and depending on stack layout it may even sometimes work and not be immediately obvious. Is there something we can do with the naming to make that less likely? Maybe change fw_config_probe_one() to something else like fw_config_check() so that it's clearer which one is for manual checking and which one is for devicetree probing? Maybe even rename the other one to fw_config_devicetree_probe() and make it take the whole struct device instead? Or maybe change FW_CONFIG() to FW_CONFIG_ONE() and add a variadic FW_CONFIG(field, option, option...) to build match lists when needed? Not sure what the best solution is.
https://review.coreboot.org/c/coreboot/+/41209/4/src/include/fw_config.h File src/include/fw_config.h:
https://review.coreboot.org/c/coreboot/+/41209/4/src/include/fw_config.h@9 PS4, Line 9: #include <static.h>
this file is pretty much useless without static. […]
FWIW I'd prefer to keep the include in here. It makes sense logically that when you include <fw_config.h> you have access to all the FW_CONFIG_... definitions, and the fact that they're actually in <static.h> is an implementation detail that will confuse a lot of people.
I think the only way to solve your build issue is to make the build of every single .c file depend on sconfig (which is a bit unfortunate because it prevents some build parallelization, but there's no way for make to add a dependency to only those files which have a certain #include). You can do that by adding $(DEVICETREE_STATIC_C) to <stage>-c-deps for all stages, like it's already done for $(OPTION_TABLE_H). (Technically you want $(DEVICETREE_STATIC_H), not $(DEVICETREE_STATIC_C), but the rule to create both is for $(DEVICETREE_STATIC_C). Also, it might be nice to change the part that makes use of $(class)-$(type)-deps in the toplevel Makefile to allow the use of generic-$(type)-deps, like we already have for generic-$(type)-ccopts, so we don't always have to add these things to every stage explicitly.)
https://review.coreboot.org/c/coreboot/+/41209/7/src/lib/fw_config.c File src/lib/fw_config.c:
https://review.coreboot.org/c/coreboot/+/41209/7/src/lib/fw_config.c@11 PS7, Line 11: MAYBE_STATIC_BSS bool fw_config_value_initialized; Actually, this doesn't work. The point of MAYBE_STATIC_BSS is to either add the 'static' keyword or not, so the variable is either put in the data segment (if possible) or on the stack (if not). But that only works for locals, not globals. Could probably rewrite this so fw_config_prepare() becomes uint32_t fw_config_get() or something and both of these are locals in there.
Also, all variables declared like that should be explicitly initialized to 0/NULL/false (otherwise, if the 'static' keyword wasn't there, the initial value would be undefined.)
(Also, I'm not sure about all these x86 details but I think these might be deprecated anyway? See CB:37055. Maybe they just forgot to clean this up...)
https://review.coreboot.org/c/coreboot/+/41209/7/src/lib/fw_config.c@27 PS7, Line 27: CONFIG_FW_CONFIG_SOURCE_CBFS_FILE Should this be prefixed with CONFIG_CBFS_PREFIX? (I'm not sure if the whole normal/fallback thing still works at all, but if it does it would probably be appropriate to use it for this as well.)
Also, is there really a point in making this name user-selectable? We don't allow people to change the name "romstage" in Kconfig either. Maybe just using fw_config as a well-known name is good enough?
https://review.coreboot.org/c/coreboot/+/41209/7/src/lib/fw_config.c@33 PS7, Line 33: } Your documentation says CBFS can override CBI (at least that's how I understood it), but the way this is written CBI actually overrides CBFS right now. Would either need to return here on success or swap the order or something.