Julius Werner has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/42029 )
Change subject: drivers/vpd: Add a config option that would override coreboot version with VPD variable ......................................................................
Patch Set 2:
(5 comments)
https://review.coreboot.org/c/coreboot/+/42029/2/src/drivers/vpd/Kconfig File src/drivers/vpd/Kconfig:
https://review.coreboot.org/c/coreboot/+/42029/2/src/drivers/vpd/Kconfig@9 PS2, Line 9: VPD_COREBOOT_VERSION I would call this VPD_SMBIOS_VERSION since it's really more related to SMBIOS than to coreboot.
https://review.coreboot.org/c/coreboot/+/42029/2/src/drivers/vpd/Kconfig@12 PS2, Line 12: VPD && GENERATE_SMBIOS_TABLES
https://review.coreboot.org/c/coreboot/+/42029/2/src/drivers/vpd/Kconfig@19 PS2, Line 19: default "version" Does this really need to be user configurable? Can't you just pick a slightly more expressive name (e.g. smbios_version or something) that would prevent clashes with other uses and work for everyone? (Just trying to limit config explosion here.)
https://review.coreboot.org/c/coreboot/+/42029/2/src/drivers/vpd/Kconfig@27 PS2, Line 27: default "" We already have CONFIG_LOCALVERSION, isn't that the same thing?
https://review.coreboot.org/c/coreboot/+/42029/2/src/drivers/vpd/smbios.c File src/drivers/vpd/smbios.c:
https://review.coreboot.org/c/coreboot/+/42029/2/src/drivers/vpd/smbios.c@13 PS2, Line 13: smbios_mainboard_bios_version This is meant as an override by the mainboard, you can't just use it here (e.g. if you try to enable this option on a mainboard that also overrides this, you'll get multiple definition errors). You should handle this in smbios.c, e.g. make a shim function like
static const char *get_bios_version(void) { if (CONFIG(VPD_SMBIOS_VERSION)) { static char buffer[20]; if (vpd_gets("smbios_version", buffer, sizeof(buffer), VPD_RO)) return buffer; }
if (strlen(CONFIG_LOCALVERSION)) return CONFIG_LOCALVERSION;
if (smbios_mainboard_bios_version) return smbios_mainboard_bios_version();
return coreboot_version; }
and then use get_bios_version() in the code in that file where it was calling smbios_mainboard_bios_version() directly.