Attention is currently required from: Paul Menzel, Arthur Heymans. Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/58934 )
Change subject: Documentation/coding_style.md: Avoid weakly linked symbols ......................................................................
Patch Set 2: Code-Review-1
(2 comments)
Patchset:
PS2: Has there been some recent discussion about this somewhere that I may have missed? There's not a lot of context on this CL (commit message?) and it seems to go against practice that has been established in many parts of coreboot for years.
I don't really agree with this at least in the form provided here -- I think weak functions are fine in the right context and cluttering Kconfig with a billion PLATFORM_USES_HOOK_XYZ isn't fundamentally a better alternative. We can discuss which situations are and aren't a good fit for weak symbols and then try to agree on some guidelines based on that, but I don't think a blanket "they're bad" is the right approach.
Personally, I think weak functions are a good solution whenever generic code has a hook that can optionally be implemented by more specific (e.g. SoC or mainboard) code. Good examples for this are things like bootblock_soc_init(), bootblock_mainboard_init(), bootblock_mainboard_early_init(), etc... most platforms implement most of these functions, but sometimes not all of them. Hiding each of them behind a Kconfig and splattering every soc/.../Kconfig file with `select HAS_BOOTBLOCK_SOC_INIT`, `select HAS_BOOTBLOCK_SOC_EARLY_INIT`, etc. would just create a lot more config boilerplate (making it harder to read the meaningful options in between) and I don't think it would really help anyone's understanding of the code. The other alternative would be to add empty stub functions to each individual mainboard/SoC file when they're not using those hooks, but that's just adding more boilerplate there and I don't see how it would really make anything better either... also, it is a really annoying maintenance burden because whenever you need to add a new hook somewhere, you would have to add empty stubs to a billion SoC/mainboard files. I think weak functions solve this problem well and without causing any real issues... I mean, it's a hook, either you do something there or you don't, when you forgot to implement it then I guess your platform didn't need it anyway and the default weak no-op function was the right choice for you.
On the other hand, I do agree that weak functions can make things very confusing when the weak function actually implements meaningful behavior, and then overriding the function means you're missing that behavior. That's a terrible pattern that I think/hope we don't really use in coreboot anyway, and I think codifying that in the style guide would be fine. Weak functions should only be allowed when the weak implementation is a no-op or otherwise represents the most simple and "default" way the function could be implemented (and when that default implementation is generally "safe" from a security standpoint -- e.g. the weak implementation of vboot's get_recovery_mode_switch() is behind an extra Kconfig for a reason), but for those cases I think they can be an appropriate tool.
File Documentation/contributing/coding_style.md:
https://review.coreboot.org/c/coreboot/+/58934/comment/c8088e6a_bfe9c6d2 PS2, Line 967: result in runtime : problems due to the lack of a proper implementation, where they could : otherwise be caught at compiletime Can you provide a concrete example of where this happened? I'm not really sure what you mean by this / don't think you can say this as an absolute.
-
Julius Werner (Code Review)