Attention is currently required from: Felix Singer, Angel Pons, Arthur Heymans, Keith Hui, Elyes Haouas.
Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/61540 )
Change subject: mb/asus/p8x7x-series: Refactor mainboard_get_spd() ......................................................................
Patch Set 6:
(1 comment)
Patchset:
PS4:
As far as precedents go, it has already been set.
Alas, that's a general problem in coreboot. Many developers can patch/merge what they like and there is no coordination. And then patterns spread no matter if they are good, bad, have been discussed or not. __weak spread before, even across project boundaries into one payload at least where it caused a 4-$-digit bug (not fun to learn that its inherently incompatible to symbols in static libraries like lib- payload). IMO, every instance of any __attribute__ should be treated with maximum delicacy.
TBH, this case here is actually one of the better if not best uses of __weak. However, there are many alternatives, and hence I don't think a __weak should be introduced as a footnote without discussion.
- ...
Seems a bit overkill.
- Conditionals in the function by board variant or # of RAM slots (which still needs to be defined somewhere). Compiler and the next programmer looking will have to sort through the inevitable rats nest.
Such definitions are done in Kconfig and that is actually the cannonical way to do things in coreboot, I believe. I don't understand what the rats nest is?
- Leave out the __weak and leave it to the guy porting a P8x7x with less than 4 RAM slots (eg. P8H77-I, ITX with 2 slots) to patch the override. This could just be kicking the ball down the road, and his patch could be more ugly.
It could be more ugly, could also be less ugly. I'm not able to predict the future and assume the same is true for others. A discussion with an actual case at hand usually leads better results than a preemptive one based on assumptions. Hence, I see nothing wrong with postponing the discussion.
- Leave out the __weak and force all current and future P8x7x variants to try reading 4 sets of SPD data. Current variants are not the problem (otherwise this patch would not exist) but future <4 slots boards would have useless work done in runtime.
I don't think we can enforce anything for the future (at least not with the current, unstructured coreboot development).
Let me throw another alternative into the ring:
5. Add an array with the SMBus addresses to the devicetree. The devicetree already has an override mechanism.
The thought actually reminded me that there might be patches for this already...
Also reminds me that I had another discussion about maintaining boilerplate-code vs. getting rid of it (by making more use of the devicetree for instance) less than two weeks ago. Would probably be better have such discussions on the ML where everybody can see them (I tried to encourage more ML discussions in the past but currently lack the strength for it (feels too much like nobody else wants to use the ML)).
-
Nico Huber (Code Review)