Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/coreboot/+/45134 )
Change subject: console: Unify overriding console log level with get_option() ......................................................................
Patch Set 10:
(3 comments)
https://review.coreboot.org/c/coreboot/+/45134/10/src/include/option.h File src/include/option.h:
https://review.coreboot.org/c/coreboot/+/45134/10/src/include/option.h@26 PS10, Line 26: return cmos_get_option(dest, name); Similarly, we could have here:
if (CONFIG(VPD_OPTIONS)) /* opt-in for trying to find options in VPD */ return vpd_get_option(dest, name);
Then, a platform using VPD for options wouldn't have to use hacks like CONSOLE_OVERRIDE_LOGLEVEL at all.
https://review.coreboot.org/c/coreboot/+/45134/10/src/include/option.h@30 PS10, Line 30: return get_console_override_option(dest, name); I don't understand why this needs another function. We could also have the code directly here:
if (CONFIG(CONSOLE_OVERRIDE_LOGLEVEL) && ...) { *(int *)dest = get_console_loglevel(); return CB_SUCCESS; }
However, checks on the option `name` don't belong here. So I would keep the `if (CONFIG(CONSOLE_OVERRIDE_LOGLEVEL))` in `console/init.c`. Or, alternatively, move it to the implementation. For instance, get rid of `get_console_loglevel()` and create a generic mainboard get_option() instead, which would leave us here with
if (CONFIG(MAINBOARD_OPTIONS)) return mainboard_get_option(dest, name);
There's only a single implementation of `get_console_loglevel()`, so that wouldn't be much work. That one could then implement the `strcmp(name, "debug_level")` check.
https://review.coreboot.org/c/coreboot/+/45134/10/src/include/option.h@32 PS10, Line 32: } If we decide for a normalized
if (CONFIG(XXX)) return xxx_get_option();
the order should probably be different. For the one get_console_loglevel() implementation, it was intended that it take precedence.