On Tue, Feb 07, 2023 at 08:31:59AM -0700, Simon Glass wrote:
Hi Tom,
On Tue, 7 Feb 2023 at 07:50, Tom Rini trini@konsulko.com wrote:
On Tue, Feb 07, 2023 at 08:39:38AM +0100, Heinrich Schuchardt wrote:
On 2/7/23 01:00, Tom Rini wrote:
On Tue, Feb 07, 2023 at 12:54:03AM +0100, Heinrich Schuchardt wrote:
On 2/6/23 01:53, Simon Glass wrote:
This converts 3 usages of this option to the non-SPL form, since there is no SPL_CMD_BOOTEFI_BOOTMGR defined in Kconfig
Signed-off-by: Simon Glass sjg@chromium.org
(no changes since v1)
boot/Makefile | 2 +- cmd/bootmenu.c | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/boot/Makefile b/boot/Makefile index 69c31adb77d..73b5b19816b 100644 --- a/boot/Makefile +++ b/boot/Makefile @@ -29,7 +29,7 @@ obj-$(CONFIG_$(SPL_TPL_)BOOTMETH_EFILOADER) += bootmeth_efi.o obj-$(CONFIG_$(SPL_TPL_)BOOTMETH_SANDBOX) += bootmeth_sandbox.o obj-$(CONFIG_$(SPL_TPL_)BOOTMETH_SCRIPT) += bootmeth_script.o ifdef CONFIG_$(SPL_TPL_)BOOTSTD_FULL -obj-$(CONFIG_$(SPL_TPL_)CMD_BOOTEFI_BOOTMGR) += bootmeth_efi_mgr.o +obj-$(CONFIG_CMD_BOOTEFI_BOOTMGR) += bootmeth_efi_mgr.o obj-$(CONFIG_$(SPL_TPL_)BOOTSTD) += bootflow_menu.o endif diff --git a/cmd/bootmenu.c b/cmd/bootmenu.c index 3236ca5d799..422ab411252 100644 --- a/cmd/bootmenu.c +++ b/cmd/bootmenu.c @@ -223,7 +223,7 @@ static int prepare_bootmenu_entry(struct bootmenu_data *menu, return 1; } -#if (CONFIG_IS_ENABLED(CMD_BOOTEFI_BOOTMGR)) && (CONFIG_IS_ENABLED(CMD_EFICONFIG)) +#if (IS_ENABLED(CONFIG_CMD_BOOTEFI_BOOTMGR)) && (CONFIG_IS_ENABLED(CMD_EFICONFIG))
There is no reason whatsoever for using different macros for the two options.
Here and elsewhere, one CONFIG is being fixed at a time. If at the end of the series this is not fixed, then that's an issue to address.
This cannot be reviewed easily. I never received the complete series.
This, and the related series, are among the most reviewed we've had in quite some time. Just FWIW.
CONFIG_IS_ENABLED() is more restrictive than IS_ENABLED(). No motivation is provided why the condition should be relaxed in the commit message.
The idea of "restrictive" is not how either of those macros should be evaluated.
Cover-letters are not in the commit history. But anyway the cover-letter does not provide any motivation for the change either.
NAK to this patch.
It's incorrect to use CONFIG_IS_ENABLED() instead of IS_ENABLED() outside of:
- CONFIG_FOO, CONFIG_SPL_FOO (etc) exist
- The code in question is compiled in the SPL (etc) context and we do need to know if the code block in question is required here and the implicit value of SPL_FOO being false is useful. This case is why Simon insists that adding def_bool n for SPL_EFI_LOADER, etc, is correct, but I'm not convinced.
The issue here is that we need a way to determine whether an option like CONFIG_FOO should apply to all build phases, or only U-Boot proper. The def_bool business creates an SPL symbol, so U-Boot then knows that the option applies only to U-Boot proper, with a separate one for CONFIG_SPL_FOO
Please bear in mind that today a lack of definition works, and is a feature, not a bug nor glitch. It's why, today, the tests in lib/vsprintf.c are correct.
There is a file call scripts/conf_nospl (in splc) which lists options that are exceptions. Otherwise we would need more of these def_bool things. The thing is, I don't really like the conf_nospl file, since it is configuring the operation of Kconfig but is not actually in the Kconfig description. So for things where I thought it was defensible, I added a def_bool.
Keep in mind that everything works as intended, today. What's being done in the EFI loader code works, but is a bad practice. It's what lead to the MMC_QUIRKS actual bug and time being lost debugging a problem that shouldn't have been.
What split config introduces / requires is for discussion over *there*.