On 19.10.23 17:19, Tom Rini wrote:
On Thu, Oct 19, 2023 at 05:16:28PM +0200, Heinrich Schuchardt wrote:
On 19.10.23 17:00, Tom Rini wrote:
From: Simon Glass <s...@chromium.org>

The command should not be used to enable library functionality. Add a
new BOOTEFI_BOOTMGR Kconfig for that. Adjust the conditions so that the
same code is built.

Signed-off-by: Simon Glass <s...@chromium.org>
Suggested-by: AKASHI Takahiro <takahiro.aka...@linaro.org>
---
Cc: Heinrich Schuchardt <heinrich.schucha...@canonical.com>
Cc: Ilias Apalodimas <ilias.apalodi...@linaro.org>
Changes in v4:
- Integrate AKASHI Takahiro's feedback from v3
- Reword the help text on CMD_BOOTEFI_BOOTMGR slightly
---
   cmd/Kconfig             | 11 ++++++++++-
   lib/efi_loader/Kconfig  |  6 +++---
   lib/efi_loader/Makefile |  2 +-
   3 files changed, 14 insertions(+), 5 deletions(-)

diff --git a/cmd/Kconfig b/cmd/Kconfig
index 16e5cb8f0633..872cb49150cc 100644
--- a/cmd/Kconfig
+++ b/cmd/Kconfig
@@ -379,6 +379,15 @@ config CMD_BOOTEFI
        help
          Boot an EFI image from memory.
+config CMD_BOOTEFI_BOOTMGR
+       bool "UEFI Boot Manager command"
+       depends on BOOTEFI_BOOTMGR && CMD_BOOTEFI
+       default y
+       help
+         Select this option to enable the 'bootmgr' subcommand of 'bootefi'.
+         This subcommand will allow you to select the UEFI binary to be booted
+         via UEFI variables Boot####, BootOrder, and BootNext.
+
   config CMD_BOOTEFI_HELLO_COMPILE
        bool "Compile a standard EFI hello world binary for testing"
        depends on CMD_BOOTEFI && !CPU_V7M
@@ -2110,7 +2119,7 @@ config CMD_EFIDEBUG
   config CMD_EFICONFIG
        bool "eficonfig - provide menu-driven uefi variables maintenance 
interface"
        default y if !HAS_BOARD_SIZE_LIMIT
-       depends on CMD_BOOTEFI_BOOTMGR
+       depends on BOOTEFI_BOOTMGR
        select MENU
        help
          Enable the 'eficonfig' command which provides the menu-driven UEFI
diff --git a/lib/efi_loader/Kconfig b/lib/efi_loader/Kconfig
index d20aaab6dba4..13cad6342c36 100644
--- a/lib/efi_loader/Kconfig
+++ b/lib/efi_loader/Kconfig
@@ -32,14 +32,14 @@ config EFI_LOADER
   if EFI_LOADER
-config CMD_BOOTEFI_BOOTMGR
+config BOOTEFI_BOOTMGR
        bool "UEFI Boot Manager"
        default y
        select BOOTMETH_GLOBAL if BOOTSTD
        help
          Select this option if you want to select the UEFI binary to be booted
-         via UEFI variables Boot####, BootOrder, and BootNext. This enables the
-         'bootefi bootmgr' command.
+         via UEFI variables Boot####, BootOrder, and BootNext. You should also
+         normally enable CMD_BOOTEFI_BOOTMGR so that the command is available.
   choice
        prompt "Store for non-volatile UEFI variables"
diff --git a/lib/efi_loader/Makefile b/lib/efi_loader/Makefile
index 8d31fc61c601..0a2cb6e3c476 100644
--- a/lib/efi_loader/Makefile
+++ b/lib/efi_loader/Makefile
@@ -42,7 +42,7 @@ targets += initrddump.o
   endif
   obj-$(CONFIG_CMD_BOOTEFI_HELLO) += helloworld_efi.o
-obj-$(CONFIG_CMD_BOOTEFI_BOOTMGR) += efi_bootmgr.o
+obj-$(CONFIG_BOOTEFI_BOOTMGR) += efi_bootmgr.o
   obj-y += efi_boottime.o
   obj-y += efi_helper.o
   obj-$(CONFIG_EFI_HAVE_CAPSULE_SUPPORT) += efi_capsule.o

This patch looks wrong.

Symbol CONFIG_CMD_BOOTEFI_BOOTMGR is used in a lot of places where it is not
related to the 'bootefi bootmgr' subcommand.

I see no benefit in two separate symbols. If you want to rename the symbol,
please, replace *all* occurrences:

%s/CONFIG_CMD_BOOTEFI_BOOTMGR/CONFIG_BOOTEFI_BOOTMGR/

Yes, there's work on the EFI_LOADER side of things to support the use
case of "boot to menu" (or, "boot to efi bootmgr") of which this is the
starting point. The follow-up work that I'm hoping you or someone else
with more EFI_LOADER experience will pick up is splitting cmd/bootefi.c
such that we can call in to starting an EFI payload (or bootmgr) without
the command line.


Even after factoring out the boot functionality I would not know why we should have two separate symbols. I am fine with a rename which makes it clear that this symbol is about a library functionality.

Best regards

Heinrich

Reply via email to