Angel Pons has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45134 )
Change subject: console: Override coreboot log level via VPD variable ......................................................................
Patch Set 3: Code-Review+1
(4 comments)
https://review.coreboot.org/c/coreboot/+/45134/3/src/console/Kconfig File src/console/Kconfig:
https://review.coreboot.org/c/coreboot/+/45134/3/src/console/Kconfig@319 PS3, Line 319: bool Should we provide a user-visible prompt for this? Note that if we do, we should not select this option from other places, but have it `default y` instead.
https://review.coreboot.org/c/coreboot/+/45134/3/src/console/Kconfig@330 PS3, Line 330: default 7 Ideally, this should be constrained to the valid loglevel values. It would be nice if we could reuse the prompt right below. Currently, it is hidden when CONSOLE_OVERRIDE_LOGLEVEL is selected, but we can change that:
if !CONSOLE_OVERRIDE_LOGLEVEL || CONSOLE_VPD_OVERRIDE_LOGLEVEL
choice
[...]
endchoice
config DEFAULT_CONSOLE_LOGLEVEL int default 0 if DEFAULT_CONSOLE_LOGLEVEL_0 default 1 if DEFAULT_CONSOLE_LOGLEVEL_1 default 2 if DEFAULT_CONSOLE_LOGLEVEL_2 default 3 if DEFAULT_CONSOLE_LOGLEVEL_3 default 4 if DEFAULT_CONSOLE_LOGLEVEL_4 default 5 if DEFAULT_CONSOLE_LOGLEVEL_5 default 6 if DEFAULT_CONSOLE_LOGLEVEL_6 default 7 if DEFAULT_CONSOLE_LOGLEVEL_7 default 8 if DEFAULT_CONSOLE_LOGLEVEL_8 help Map the log level config names to an integer.
endif ## !CONSOLE_OVERRIDE_LOGLEVEL || CONSOLE_VPD_OVERRIDE_LOGLEVEL
Then, the CONSOLE_VPD_DEFAULT_LOGLEVEL symbol can be replaced with DEFAULT_CONSOLE_LOGLEVEL.
https://review.coreboot.org/c/coreboot/+/45134/3/src/console/loglevel_vpd.c File src/console/loglevel_vpd.c:
https://review.coreboot.org/c/coreboot/+/45134/3/src/console/loglevel_vpd.c@... PS3, Line 12: uint8_t To use this type, you should include it: #include <stdint.h>
However, if we're returning an int, we might as well use an int here for consistency.
https://review.coreboot.org/c/coreboot/+/45134/3/src/console/loglevel_vpd.c@... PS3, Line 12: uint8_t val; : char val_str[VPD_LEN]; : : if (vpd_gets(COREBOOT_LOG_LEVEL, val_str, VPD_LEN, VPD_RW_THEN_RO)) { : val = (uint8_t)atol(val_str); : if (val >= BIOS_NEVER) : return CONFIG_CONSOLE_VPD_DEFAULT_LOGLEVEL; : return val; : } else { : return CONFIG_CONSOLE_VPD_DEFAULT_LOGLEVEL; : } I'd say this would look better with a single `return` statement:
int log_level = CONFIG_CONSOLE_VPD_DEFAULT_LOGLEVEL; char val_str[VPD_LEN];
if (vpd_gets(COREBOOT_LOG_LEVEL, val_str, VPD_LEN, VPD_RW_THEN_RO)) { log_level = atol(val_str); if (log_level < 0 || log_level >= BIOS_NEVER) log_level = CONFIG_CONSOLE_VPD_DEFAULT_LOGLEVEL; } return log_level;
Also, we can rename `val` to `log_level` for clarity.