Francois Toguo Fotso has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/32090 )
Change subject: smbios: Add and adapt fields for version 3.2 ......................................................................
Patch Set 6:
Patch Set 6:
tl;dr; Please focus on a commit message that provides a reasoning for the change, what it gains us and the scope of the changes. I still don't see the incentive for all this.
Francois, I think you are experiencing a lot of conflicting feedback from reviewers here, because you leave too much room for interpretation. For instance I guess, Paul asked you to properly refer to the other change because he might have thought it was a patch to coreboot. But it can't work out, as you don't know yet what commit hash the other change will have.
Also, I don't see a reason to mention the change to mosys. The SMBIOS spec is the interface between coreboot and mosys, both have to comply to SMBIOS, period. Otherwise, you might break compatibility to other programs whilst adding conflic- ting dependencies.
Reference to the mosys change in the commit message have been removed.
If I tell you, you shouldn't do this or that in this change, I merely interpret the scope of this change that you set with the commit message. And I'm still unsure what that should be. It is very important to define the scope particularly in this case because SMBIOS allows many things to be optional. I can't say that I read the full document, but so far I don't see anything that would have to change for 3.2 compatibility. The 64-bit entry point is optional too, and I guess, again, Patrick wouldn't have asked for it if you had provided a clear line around what this change should comprise and what not.
So please, before making any more changes to the code, tell us exactly what you want to achieve and why. e.g.
No more change(s) to the code is planned in this review.
- Add optional fields, as a programming excercise?
- SMBIOS 3.2 compliance in coreboot, because?
- SMBIOS 3.2 compliance plus optional fields, because?
- Keep things in sync with mosys?
A particular reason is project or test case related, we have some internal testing that will require to support SMBIOS 3.2 to make the test pass.
And regarding existing bugs (wrong core counts, clock vs. MTs etc.), if they make it harder to update the SMBIOS code, I suggest to fix them first. Please understand that you create a lot of reviewing work with this -- a change that doesn't gain us anything at the moment (anticipation of future changes?). So this has pro- bably lowest priority for most of us.