It doesn't explain why it needs to be runtime configurable and why CONFIG_LOCALVERSION doesn't work for you.

+1... VPD has the advantage that it's easier to modify after the image is already built, but since the coreboot build version should only change with, you know, a new build, it's not clear to me why CONFIG_LOCALVERSION wouldn't be enough.

5 comments:

Commit Message:
- Patch Set #5, Line 18: by spec, is terminated by 0x00 (VPD_TYPE_TERMINATOR) anyway. This
  No, this is *not* how that works! VPD_TYPE_TERMINATOR only comes after the last key/value pair in the VPD. You're just getting lucky right now that the version field comes last for you (you're probably testing with only a single field in the VPD? Try adding a few more afterwards and this should break).
  Please actually use vpd_gets().
File src/arch/x86/smbios.c:
- Patch Set #5, Line 40: #if CONFIG(VPD)
  There's no need to #if guard a header.
- Patch Set #5, Line 395: const char *__weak smbios_mainboard_bios_version(void)
  I don't think we should keep adding more functionality to a weak function. Weak functions should really only be used when the default implementation is basically a no-op or it becomes too confusing, and this is far from a no-op by now. I would recommend rearchitecting this as I outlined in https://review.coreboot.org/c/coreboot/+/42029/2/src/drivers/vpd/smbios.c#13
- Patch Set #5, Line 397: #if CONFIG(VPD)
  Use a C
```
 if (CONFIG(VPD))
```
  rather than a preprocessor
```
 #if CONFIG(VPD)
```
  unless really necessary (it isn't here).
  Also, you should probably make a new Kconfig for this that depends on VPD (and is default disabled). Otherwise, you're changing behavior for everyone else using VPD too, and they don't necessarily want this feature.
- Patch Set #5, Line 401: version
  Can we pick a more unique name here to avoid clashing with other potential uses? How about "smbios_version"?

To view, visit change 32905. To unsubscribe, or for help writing mail filters, visit settings.