Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42029 )
Change subject: smbios: Add option VPD_SMBIOS_VERSION that reads BIOS version from a VPD variable ......................................................................
Patch Set 8:
(6 comments)
The VPD use in our plan for servers are not device-specific (for device-specific data is stored somewhere else called FRU) and normally the VPD should be erased and updated from version to version.
Well... honestly you're just using it in a way that it really wasn't meant to be used there. But this patch seems harmless enough so I'd be okay taking it if it makes your life easier.
https://review.coreboot.org/c/coreboot/+/42029/8/src/Kconfig File src/Kconfig:
https://review.coreboot.org/c/coreboot/+/42029/8/src/Kconfig@763 PS8, Line 763: default "firmware_version" Does this need to be configurable? Can't we just hardcode this name and have everyone use the same?
https://review.coreboot.org/c/coreboot/+/42029/8/src/arch/x86/smbios.c File src/arch/x86/smbios.c:
https://review.coreboot.org/c/coreboot/+/42029/8/src/arch/x86/smbios.c@22 PS8, Line 22: #if CONFIG(VPD_SMBIOS_VERSION) Header includes should not be guarded, they don't hurt anyone.
https://review.coreboot.org/c/coreboot/+/42029/8/src/arch/x86/smbios.c@374 PS8, Line 374: #if CONFIG(VPD_SMBIOS_VERSION) You shouldn't need to guard this (if you also don't guard the header include above). The linker will garbage collect the function automatically when it determines it to be unreachable.
https://review.coreboot.org/c/coreboot/+/42029/8/src/arch/x86/smbios.c@378 PS8, Line 378: static char buffer[VER_LEN] = {0}; Since this is ramstage-only (right?) maybe just use vpd_find() and malloc()? (We could also add a vpd_gets_alloc() wrapper to the VPD code for this if we want...)
https://review.coreboot.org/c/coreboot/+/42029/8/src/arch/x86/smbios.c@394 PS8, Line 394: return CONFIG_LOCALVERSION; Don't do this, just have this function return NULL and then have the caller proceed to the next option on error.
https://review.coreboot.org/c/coreboot/+/42029/8/src/arch/x86/smbios.c@404 PS8, Line 404: const char *__weak smbios_mainboard_bios_version(void) I still think we should rewrite this so the weak smbios_mainboard_bios_version() just returns "" and there's another function that handles checking VPD, LOCALVERSION, etc. in turn. It's bad style for weak function stubs that can be overridden to contain any real logic, they should just be no-ops. Also, I don't think the current behavior that a mainboard override has precedence over CONFIG_LOCALVERSION (or now the VPD) is intentional... precedence order should be VPD > LOCALVERSION > mainboard override > coreboot_version.