[PATCH v2 5/6] cmd: bootmenu: add parameter -e for UEFI boot options

[PATCH v2 5/6] cmd: bootmenu: add parameter -e for UEFI boot options

[Heinrich Schuchardt]

The bootmenu command can display

* menu entries defined by environment variables
* menu entries defined by UEFI boot options

Not in all cases showing the UEFI boot options is desired.
Provide a new parameter '-e' to select the display of UEFI boot options.

Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
---
v2:
	new patch
---
 cmd/bootmenu.c             | 39 +++++++++++++++++++++++++++++---------
 doc/usage/cmd/bootmenu.rst | 13 ++++++++++---
 2 files changed, 40 insertions(+), 12 deletions(-)

diff --git a/cmd/bootmenu.c b/cmd/bootmenu.c
index ffa63a4628d..90f4f3d583c 100644
--- a/cmd/bootmenu.c
+++ b/cmd/bootmenu.c
@@ -330,7 +330,13 @@ static int prepare_uefi_bootorder_entry(struct bootmenu_data *menu,
 }
 #endif
 
-static struct bootmenu_data *bootmenu_create(int delay)
+/**
+ * bootmenu_create() - create boot menu entries
+ *
+ * @uefi:	consider UEFI boot options
+ * @delay:	autostart delay in seconds
+ */
+static struct bootmenu_data *bootmenu_create(int uefi, int delay)
 {
 	int ret;
 	unsigned short int i = 0;
@@ -357,7 +363,7 @@ static struct bootmenu_data *bootmenu_create(int delay)
 		goto cleanup;
 
 #if (IS_ENABLED(CONFIG_CMD_BOOTEFI_BOOTMGR)) && (IS_ENABLED(CONFIG_CMD_EFICONFIG))
-	if (i < MAX_COUNT - 1) {
+	if (uefi && i < MAX_COUNT - 1) {
 		efi_status_t efi_ret;
 
 		/*
@@ -481,7 +487,13 @@ static void handle_uefi_bootnext(void)
 		run_command("bootefi bootmgr", 0);
 }
 
-static enum bootmenu_ret bootmenu_show(int delay)
+/**
+ * bootmenu_show - display boot menu
+ *
+ * @uefi:	generated entries for UEFI boot options
+ * @delay:	autoboot delay in seconds
+ */
+static enum bootmenu_ret bootmenu_show(int uefi, int delay)
 {
 	int cmd_ret;
 	int init = 0;
@@ -495,7 +507,7 @@ static enum bootmenu_ret bootmenu_show(int delay)
 	efi_status_t efi_ret = EFI_SUCCESS;
 	char *option, *sep;
 
-	if (IS_ENABLED(CONFIG_CMD_BOOTEFI_BOOTMGR))
+	if (IS_ENABLED(CONFIG_CMD_BOOTEFI_BOOTMGR) && uefi)
 		handle_uefi_bootnext();
 
 	/* If delay is 0 do not create menu, just run first entry */
@@ -514,7 +526,7 @@ static enum bootmenu_ret bootmenu_show(int delay)
 		return (cmd_ret == CMD_RET_SUCCESS ? BOOTMENU_RET_SUCCESS : BOOTMENU_RET_FAIL);
 	}
 
-	bootmenu = bootmenu_create(delay);
+	bootmenu = bootmenu_create(uefi, delay);
 	if (!bootmenu)
 		return BOOTMENU_RET_FAIL;
 
@@ -609,7 +621,7 @@ int menu_show(int bootdelay)
 	int ret;
 
 	while (1) {
-		ret = bootmenu_show(bootdelay);
+		ret = bootmenu_show(1, bootdelay);
 		bootdelay = -1;
 		if (ret == BOOTMENU_RET_UPDATED)
 			continue;
@@ -635,11 +647,19 @@ int do_bootmenu(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[])
 {
 	char *delay_str = NULL;
 	int delay = 10;
+	int uefi = 0;
 
 #if defined(CONFIG_BOOTDELAY) && (CONFIG_BOOTDELAY >= 0)
 	delay = CONFIG_BOOTDELAY;
 #endif
 
+	if (argc >= 2) {
+		if (!strcmp("-e", argv[1])) {
+			uefi = 1;
+			--argc;
+			++argv;
+		}
+	}
 	if (argc >= 2)
 		delay_str = argv[1];
 
@@ -649,13 +669,14 @@ int do_bootmenu(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[])
 	if (delay_str)
 		delay = (int)simple_strtol(delay_str, NULL, 10);
 
-	bootmenu_show(delay);
+	bootmenu_show(uefi, delay);
 	return 0;
 }
 
 U_BOOT_CMD(
 	bootmenu, 2, 1, do_bootmenu,
 	"ANSI terminal bootmenu",
-	"[delay]\n"
-	"    - show ANSI terminal bootmenu with autoboot delay"
+	"[-e] [delay]\n"
+	"-e    - show UEFI entries\n"
+	"delay - show ANSI terminal bootmenu with autoboot delay"
 );
diff --git a/doc/usage/cmd/bootmenu.rst b/doc/usage/cmd/bootmenu.rst
index 294cc02b17a..cd5597bc646 100644
--- a/doc/usage/cmd/bootmenu.rst
+++ b/doc/usage/cmd/bootmenu.rst
@@ -11,7 +11,7 @@ Synopsis
 --------
 ::
 
-    bootmenu [delay]
+    bootmenu [-e] [delay]
 
 Description
 -----------
@@ -28,6 +28,14 @@ The "bootmenu" command interprets ANSI escape sequences, so
 an ANSI terminal is required for proper menu rendering and item
 selection.
 
+-e
+    show menu entries based on UEFI boot options
+
+delay
+    is the autoboot delay in seconds, after which the first
+    menu entry will be selected automatically
+
+
 The assembling of the menu is done via a set of environment variables
 "bootmenu_<num>" and "bootmenu_delay", i.e.::
 
@@ -35,8 +43,7 @@ The assembling of the menu is done via a set of environment variables
     bootmenu_<num>="<title>=<commands>"
 
 <delay>
-    is the autoboot delay in seconds, after which the first
-    menu entry will be selected automatically
+    autostart delay in seconds
 
 <num>
     is the boot menu entry number, starting from zero
-- 
2.45.2

Re: [PATCH v2 5/6] cmd: bootmenu: add parameter -e for UEFI boot options

[Ilias Apalodimas]

On Sat, 23 Nov 2024 at 23:46, Heinrich Schuchardt
<heinrich.schuchardt@canonical.com> wrote:
>
> The bootmenu command can display
>
> * menu entries defined by environment variables
> * menu entries defined by UEFI boot options
>
> Not in all cases showing the UEFI boot options is desired.
> Provide a new parameter '-e' to select the display of UEFI boot options.
>
> Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
> ---
> v2:
>         new patch
> ---
>  cmd/bootmenu.c             | 39 +++++++++++++++++++++++++++++---------
>  doc/usage/cmd/bootmenu.rst | 13 ++++++++++---
>  2 files changed, 40 insertions(+), 12 deletions(-)
>
> diff --git a/cmd/bootmenu.c b/cmd/bootmenu.c
> index ffa63a4628d..90f4f3d583c 100644
> --- a/cmd/bootmenu.c
> +++ b/cmd/bootmenu.c
> @@ -330,7 +330,13 @@ static int prepare_uefi_bootorder_entry(struct bootmenu_data *menu,
>  }
>  #endif
>
> -static struct bootmenu_data *bootmenu_create(int delay)
> +/**
> + * bootmenu_create() - create boot menu entries
> + *
> + * @uefi:      consider UEFI boot options
> + * @delay:     autostart delay in seconds
> + */
> +static struct bootmenu_data *bootmenu_create(int uefi, int delay)
>  {
>         int ret;
>         unsigned short int i = 0;
> @@ -357,7 +363,7 @@ static struct bootmenu_data *bootmenu_create(int delay)
>                 goto cleanup;
>
>  #if (IS_ENABLED(CONFIG_CMD_BOOTEFI_BOOTMGR)) && (IS_ENABLED(CONFIG_CMD_EFICONFIG))
> -       if (i < MAX_COUNT - 1) {
> +       if (uefi && i < MAX_COUNT - 1) {
>                 efi_status_t efi_ret;
>
>                 /*
> @@ -481,7 +487,13 @@ static void handle_uefi_bootnext(void)
>                 run_command("bootefi bootmgr", 0);
>  }
>
> -static enum bootmenu_ret bootmenu_show(int delay)
> +/**
> + * bootmenu_show - display boot menu
> + *
> + * @uefi:      generated entries for UEFI boot options
> + * @delay:     autoboot delay in seconds
> + */
> +static enum bootmenu_ret bootmenu_show(int uefi, int delay)
>  {
>         int cmd_ret;
>         int init = 0;
> @@ -495,7 +507,7 @@ static enum bootmenu_ret bootmenu_show(int delay)
>         efi_status_t efi_ret = EFI_SUCCESS;
>         char *option, *sep;
>
> -       if (IS_ENABLED(CONFIG_CMD_BOOTEFI_BOOTMGR))
> +       if (IS_ENABLED(CONFIG_CMD_BOOTEFI_BOOTMGR) && uefi)
>                 handle_uefi_bootnext();
>
>         /* If delay is 0 do not create menu, just run first entry */
> @@ -514,7 +526,7 @@ static enum bootmenu_ret bootmenu_show(int delay)
>                 return (cmd_ret == CMD_RET_SUCCESS ? BOOTMENU_RET_SUCCESS : BOOTMENU_RET_FAIL);
>         }
>
> -       bootmenu = bootmenu_create(delay);
> +       bootmenu = bootmenu_create(uefi, delay);
>         if (!bootmenu)
>                 return BOOTMENU_RET_FAIL;
>
> @@ -609,7 +621,7 @@ int menu_show(int bootdelay)
>         int ret;
>
>         while (1) {
> -               ret = bootmenu_show(bootdelay);
> +               ret = bootmenu_show(1, bootdelay);
>                 bootdelay = -1;
>                 if (ret == BOOTMENU_RET_UPDATED)
>                         continue;
> @@ -635,11 +647,19 @@ int do_bootmenu(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[])
>  {
>         char *delay_str = NULL;
>         int delay = 10;
> +       int uefi = 0;
>
>  #if defined(CONFIG_BOOTDELAY) && (CONFIG_BOOTDELAY >= 0)
>         delay = CONFIG_BOOTDELAY;
>  #endif
>
> +       if (argc >= 2) {
> +               if (!strcmp("-e", argv[1])) {
> +                       uefi = 1;
> +                       --argc;
> +                       ++argv;
> +               }
> +       }
>         if (argc >= 2)
>                 delay_str = argv[1];
>
> @@ -649,13 +669,14 @@ int do_bootmenu(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[])
>         if (delay_str)
>                 delay = (int)simple_strtol(delay_str, NULL, 10);
>
> -       bootmenu_show(delay);
> +       bootmenu_show(uefi, delay);
>         return 0;
>  }
>
>  U_BOOT_CMD(
>         bootmenu, 2, 1, do_bootmenu,
>         "ANSI terminal bootmenu",
> -       "[delay]\n"
> -       "    - show ANSI terminal bootmenu with autoboot delay"
> +       "[-e] [delay]\n"
> +       "-e    - show UEFI entries\n"
> +       "delay - show ANSI terminal bootmenu with autoboot delay"
>  );
> diff --git a/doc/usage/cmd/bootmenu.rst b/doc/usage/cmd/bootmenu.rst
> index 294cc02b17a..cd5597bc646 100644
> --- a/doc/usage/cmd/bootmenu.rst
> +++ b/doc/usage/cmd/bootmenu.rst
> @@ -11,7 +11,7 @@ Synopsis
>  --------
>  ::
>
> -    bootmenu [delay]
> +    bootmenu [-e] [delay]
>
>  Description
>  -----------
> @@ -28,6 +28,14 @@ The "bootmenu" command interprets ANSI escape sequences, so
>  an ANSI terminal is required for proper menu rendering and item
>  selection.
>
> +-e
> +    show menu entries based on UEFI boot options
> +
> +delay
> +    is the autoboot delay in seconds, after which the first
> +    menu entry will be selected automatically
> +
> +
>  The assembling of the menu is done via a set of environment variables
>  "bootmenu_<num>" and "bootmenu_delay", i.e.::
>
> @@ -35,8 +43,7 @@ The assembling of the menu is done via a set of environment variables
>      bootmenu_<num>="<title>=<commands>"
>
>  <delay>
> -    is the autoboot delay in seconds, after which the first
> -    menu entry will be selected automatically
> +    autostart delay in seconds
>
>  <num>
>      is the boot menu entry number, starting from zero
> --
> 2.45.2
>

Reviewed-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>

[PATCH v2 0/6] efi_loader: run bootdev_hunt() to find ESP

[Heinrich Schuchardt]

When initializing the EFI sub-system we need all block devices and the
NIC to be probed. Extensions should have been applied to the device-tree.

Our current implementation only guarantees that bound block devices are
probed. But this misses out devices where binding requires specific
routines to be executed, e.g. NVMe (nvme scan), SCSI (scsi start).

Invoking bootdev_hunt() allows to find all block devices, probe the
network nic, and enable extensions.

Reviewers complained that PXE boot was executed unexpectedly with a patch
that only added the bootdev_hunt() call to the EFI sub-system
initialization.

The EFI sub-system does not require the IP address to be set up. We can
reduce the boottime by not executing dchp_run() in eth_bootdev_hunt().

Furthermore on the legacy network stack with autostart=yes dhcp_run()
tries to download the boot file via TFTP and execute it which seems
to be a problem with non-observance of autoload=no.

To fix Gitlab CI issues the following changes are necessary:

In test_extension.py do not assume that extensions have not been loaded
by bootdev hunter in a previous test.

Remove CONFIG_AMIGA_PARTITION from sandbox_deconfig to avoid a timeout.

Remove CONFIG_USB_DWC3 on the CI from xilinx_versal_virt_defconfig to
avoid a boot failure.

Add a parameter '-e' to select if UEFI boot options shall be shown by
the bootmenu command.

v2:
	disable USB_DWC3 only in CI
	update commit message for eth_bootdev_hunt() change

Heinrich Schuchardt (6):
  test: fix test_extension.py
  configs: sandbox_deconfig: remove CONFIG_AMIGA_PARTITION
  CI: xilinx_versal_virt: disable USB_DWC3
  net: eth_bootdev_hunt() must not try to boot
  cmd: bootmenu: add parameter -e for UEFI boot options
  efi_loader: run bootdev_hunt() to find ESP

 .azure-pipelines.yml            |  1 +
 .gitlab-ci.yml                  |  1 +
 cmd/bootmenu.c                  | 39 +++++++++++++++++++++++++--------
 configs/sandbox_defconfig       |  1 -
 doc/usage/cmd/bootmenu.rst      | 13 ++++++++---
 lib/efi_loader/efi_setup.c      |  8 +++++++
 net/eth_bootdev.c               | 30 +++++++++++++++----------
 test/py/tests/test_extension.py |  4 +++-
 8 files changed, 71 insertions(+), 26 deletions(-)

-- 
2.45.2

[PATCH v2 1/6] test: fix test_extension.py

[Heinrich Schuchardt]

test_extension.py assumes that no extension is known at test start.
This assumption is wrong because we do not come out of reboot.
A prior test may have already hunted for the extension bootdev.

Remove the invalid assert.

Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
---
v2:
	no change
---
 test/py/tests/test_extension.py | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/test/py/tests/test_extension.py b/test/py/tests/test_extension.py
index 267cf2ff27c..2a3c5116171 100644
--- a/test/py/tests/test_extension.py
+++ b/test/py/tests/test_extension.py
@@ -26,7 +26,9 @@ def test_extension(u_boot_console):
     load_dtb(u_boot_console)
 
     output = u_boot_console.run_command('extension list')
-    assert('No extension' in output)
+    # extension_bootdev_hunt may have already run.
+    # Without reboot we cannot make any assumption here.
+    # assert('No extension' in output)
 
     output = u_boot_console.run_command('extension scan')
     assert output == 'Found 2 extension board(s).'
-- 
2.45.2

[PATCH v2 2/6] configs: sandbox_deconfig: remove CONFIG_AMIGA_PARTITION

[Heinrich Schuchardt]

We do not actually test the code.
Scanning for Amiga partitions of the sandbox is extremely slow,
especially on the partially implemented USB device.

For build testing the other sandbox defconfigs are good enough.

Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
---
v2:
	no change
---
 configs/sandbox_defconfig | 1 -
 1 file changed, 1 deletion(-)

diff --git a/configs/sandbox_defconfig b/configs/sandbox_defconfig
index 718e4a8283c..683888f238f 100644
--- a/configs/sandbox_defconfig
+++ b/configs/sandbox_defconfig
@@ -142,7 +142,6 @@ CONFIG_CMD_SQUASHFS=y
 CONFIG_CMD_MTDPARTS=y
 CONFIG_CMD_STACKPROTECTOR_TEST=y
 CONFIG_MAC_PARTITION=y
-CONFIG_AMIGA_PARTITION=y
 CONFIG_OF_CONTROL=y
 CONFIG_OF_LIVE=y
 CONFIG_ENV_IS_NOWHERE=y
-- 
2.45.2

[PATCH v2 3/6] CI: xilinx_versal_virt: disable USB_DWC3

[Heinrich Schuchardt]

The CI uses the following command to launch xilinx_versal_virt_defconfig:

    qemu-system-aarch64 -M xlnx-versal-virt \
    -display none -m 4G -serial mon:stdio \
    -device loader,file=u-boot,cpu-num=0

'usb start' or invoking eth_bootdev_hunt leads to a crash when function
dwc3_core_init() tries to access a register at offset 0xc704 (DWC3_DCTL)
relative to the register start address 0xfe20c100.

Disable CONFIG_USB_DWC3 in the CI until the driver problem is fixed.

Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
---
v2:
	Only disable CONFIG_USB_DWC3 in CI as actual hardware uses it.
---
 .azure-pipelines.yml | 1 +
 .gitlab-ci.yml       | 1 +
 2 files changed, 2 insertions(+)

diff --git a/.azure-pipelines.yml b/.azure-pipelines.yml
index 4ecf76eaa0b..bac577ec008 100644
--- a/.azure-pipelines.yml
+++ b/.azure-pipelines.yml
@@ -511,6 +511,7 @@ stages:
           TEST_PY_BD: "xilinx_versal_virt"
           TEST_PY_ID: "--id qemu"
           TEST_PY_TEST_SPEC: "not sleep"
+          OVERRIDE: "-a ~CONFIG_USB_DWC3"
         xtfpga:
           TEST_PY_BD: "xtfpga"
           TEST_PY_ID: "--id qemu"
diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml
index 2164ad79a72..714284a56df 100644
--- a/.gitlab-ci.yml
+++ b/.gitlab-ci.yml
@@ -508,6 +508,7 @@ xilinx_versal_virt test.py:
     TEST_PY_BD: "xilinx_versal_virt"
     TEST_PY_TEST_SPEC: "not sleep"
     TEST_PY_ID: "--id qemu"
+    OVERRIDE: "-a ~CONFIG_USB_DWC3"
   <<: *buildman_and_testpy_dfn
 
 xtfpga test.py:
-- 
2.45.2

[PATCH v2 4/6] net: eth_bootdev_hunt() should not run DHCP

[Heinrich Schuchardt]

Currently when booting dhcp_run() may be executed multiple times:
once in eth_bootdev_hunt() and once in the network booting bootmeth.

We need to call eth_bootdev_hunt() when setting up the EFI sub-system to
supply the simple network protocol. We don't need an IP address set up.

We can reduce the bootime by not executing dhcp_run() in
eth_bootdev_hunt().

Furthermore eth_bootdev_hunt() with autostart=yes leads on the legacy
network statck leads to downloading a file via TFTP and to booting the
downloaded file.

Instead of running dchp_run() just check that there is a network device
in eth_bootdev_hunt().

Reviewed-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
---
v2:
	reword the commit message
---
 net/eth_bootdev.c | 30 ++++++++++++++++++------------
 1 file changed, 18 insertions(+), 12 deletions(-)

diff --git a/net/eth_bootdev.c b/net/eth_bootdev.c
index 6ee54e3c790..b0fca6e8313 100644
--- a/net/eth_bootdev.c
+++ b/net/eth_bootdev.c
@@ -64,9 +64,23 @@ static int eth_bootdev_bind(struct udevice *dev)
 	return 0;
 }
 
+/**
+ * eth_bootdev_hunt() - probe all network devices
+ *
+ * Network devices can also come from USB, but that is a higher
+ * priority (BOOTDEVP_5_SCAN_SLOW) than network, so it should have been
+ * enumerated already. If something like 'bootflow scan dhcp' is used,
+ * then the user will need to run 'usb start' first.
+ *
+ * @info:	info structure describing this hunter
+ * @show:	true to show information from the hunter
+ *
+ * Return:	0 if device found, -EINVAL otherwise
+ */
 static int eth_bootdev_hunt(struct bootdev_hunter *info, bool show)
 {
 	int ret;
+	struct udevice *dev = NULL;
 
 	if (!test_eth_enabled())
 		return 0;
@@ -78,19 +92,11 @@ static int eth_bootdev_hunt(struct bootdev_hunter *info, bool show)
 			log_warning("Failed to init PCI (%dE)\n", ret);
 	}
 
-	/*
-	 * Ethernet devices can also come from USB, but that is a higher
-	 * priority (BOOTDEVP_5_SCAN_SLOW) than ethernet, so it should have been
-	 * enumerated already. If something like 'bootflow scan dhcp' is used
-	 * then the user will need to run 'usb start' first.
-	 */
-	if (IS_ENABLED(CONFIG_CMD_DHCP)) {
-		ret = dhcp_run(0, NULL, false);
-		if (ret)
-			return -EINVAL;
-	}
+	ret = -EINVAL;
+	uclass_foreach_dev_probe(UCLASS_ETH, dev)
+		ret = 0;
 
-	return 0;
+	return ret;
 }
 
 struct bootdev_ops eth_bootdev_ops = {
-- 
2.45.2

[PATCH v2 5/6] cmd: bootmenu: add parameter -e for UEFI boot options

[Heinrich Schuchardt]

The bootmenu command can display

* menu entries defined by environment variables
* menu entries defined by UEFI boot options

Not in all cases showing the UEFI boot options is desired.
Provide a new parameter '-e' to select the display of UEFI boot options.

Reviewed-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
---
v2:
	no change
---
 cmd/bootmenu.c             | 39 +++++++++++++++++++++++++++++---------
 doc/usage/cmd/bootmenu.rst | 13 ++++++++++---
 2 files changed, 40 insertions(+), 12 deletions(-)

diff --git a/cmd/bootmenu.c b/cmd/bootmenu.c
index ffa63a4628d..90f4f3d583c 100644
--- a/cmd/bootmenu.c
+++ b/cmd/bootmenu.c
@@ -330,7 +330,13 @@ static int prepare_uefi_bootorder_entry(struct bootmenu_data *menu,
 }
 #endif
 
-static struct bootmenu_data *bootmenu_create(int delay)
+/**
+ * bootmenu_create() - create boot menu entries
+ *
+ * @uefi:	consider UEFI boot options
+ * @delay:	autostart delay in seconds
+ */
+static struct bootmenu_data *bootmenu_create(int uefi, int delay)
 {
 	int ret;
 	unsigned short int i = 0;
@@ -357,7 +363,7 @@ static struct bootmenu_data *bootmenu_create(int delay)
 		goto cleanup;
 
 #if (IS_ENABLED(CONFIG_CMD_BOOTEFI_BOOTMGR)) && (IS_ENABLED(CONFIG_CMD_EFICONFIG))
-	if (i < MAX_COUNT - 1) {
+	if (uefi && i < MAX_COUNT - 1) {
 		efi_status_t efi_ret;
 
 		/*
@@ -481,7 +487,13 @@ static void handle_uefi_bootnext(void)
 		run_command("bootefi bootmgr", 0);
 }
 
-static enum bootmenu_ret bootmenu_show(int delay)
+/**
+ * bootmenu_show - display boot menu
+ *
+ * @uefi:	generated entries for UEFI boot options
+ * @delay:	autoboot delay in seconds
+ */
+static enum bootmenu_ret bootmenu_show(int uefi, int delay)
 {
 	int cmd_ret;
 	int init = 0;
@@ -495,7 +507,7 @@ static enum bootmenu_ret bootmenu_show(int delay)
 	efi_status_t efi_ret = EFI_SUCCESS;
 	char *option, *sep;
 
-	if (IS_ENABLED(CONFIG_CMD_BOOTEFI_BOOTMGR))
+	if (IS_ENABLED(CONFIG_CMD_BOOTEFI_BOOTMGR) && uefi)
 		handle_uefi_bootnext();
 
 	/* If delay is 0 do not create menu, just run first entry */
@@ -514,7 +526,7 @@ static enum bootmenu_ret bootmenu_show(int delay)
 		return (cmd_ret == CMD_RET_SUCCESS ? BOOTMENU_RET_SUCCESS : BOOTMENU_RET_FAIL);
 	}
 
-	bootmenu = bootmenu_create(delay);
+	bootmenu = bootmenu_create(uefi, delay);
 	if (!bootmenu)
 		return BOOTMENU_RET_FAIL;
 
@@ -609,7 +621,7 @@ int menu_show(int bootdelay)
 	int ret;
 
 	while (1) {
-		ret = bootmenu_show(bootdelay);
+		ret = bootmenu_show(1, bootdelay);
 		bootdelay = -1;
 		if (ret == BOOTMENU_RET_UPDATED)
 			continue;
@@ -635,11 +647,19 @@ int do_bootmenu(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[])
 {
 	char *delay_str = NULL;
 	int delay = 10;
+	int uefi = 0;
 
 #if defined(CONFIG_BOOTDELAY) && (CONFIG_BOOTDELAY >= 0)
 	delay = CONFIG_BOOTDELAY;
 #endif
 
+	if (argc >= 2) {
+		if (!strcmp("-e", argv[1])) {
+			uefi = 1;
+			--argc;
+			++argv;
+		}
+	}
 	if (argc >= 2)
 		delay_str = argv[1];
 
@@ -649,13 +669,14 @@ int do_bootmenu(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[])
 	if (delay_str)
 		delay = (int)simple_strtol(delay_str, NULL, 10);
 
-	bootmenu_show(delay);
+	bootmenu_show(uefi, delay);
 	return 0;
 }
 
 U_BOOT_CMD(
 	bootmenu, 2, 1, do_bootmenu,
 	"ANSI terminal bootmenu",
-	"[delay]\n"
-	"    - show ANSI terminal bootmenu with autoboot delay"
+	"[-e] [delay]\n"
+	"-e    - show UEFI entries\n"
+	"delay - show ANSI terminal bootmenu with autoboot delay"
 );
diff --git a/doc/usage/cmd/bootmenu.rst b/doc/usage/cmd/bootmenu.rst
index 294cc02b17a..cd5597bc646 100644
--- a/doc/usage/cmd/bootmenu.rst
+++ b/doc/usage/cmd/bootmenu.rst
@@ -11,7 +11,7 @@ Synopsis
 --------
 ::
 
-    bootmenu [delay]
+    bootmenu [-e] [delay]
 
 Description
 -----------
@@ -28,6 +28,14 @@ The "bootmenu" command interprets ANSI escape sequences, so
 an ANSI terminal is required for proper menu rendering and item
 selection.
 
+-e
+    show menu entries based on UEFI boot options
+
+delay
+    is the autoboot delay in seconds, after which the first
+    menu entry will be selected automatically
+
+
 The assembling of the menu is done via a set of environment variables
 "bootmenu_<num>" and "bootmenu_delay", i.e.::
 
@@ -35,8 +43,7 @@ The assembling of the menu is done via a set of environment variables
     bootmenu_<num>="<title>=<commands>"
 
 <delay>
-    is the autoboot delay in seconds, after which the first
-    menu entry will be selected automatically
+    autostart delay in seconds
 
 <num>
     is the boot menu entry number, starting from zero
-- 
2.45.2

[PATCH v2 6/6] efi_loader: run bootdev_hunt() to find ESP

[Heinrich Schuchardt]

Some hard devices need specific routines to scan for block devices,
e.g. NVMe (nvme scan), SCSI (scsi start).

Invoke bootdev_hunt() to find all block devices.

Reviewed-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
Reviewed-by: Simon Glass <sjg@chromium.org>
Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
---
v2:
	no change
---
 cmd/bootmenu.c             | 39 +++++++++++++++++++++++++++++---------
 doc/usage/cmd/bootmenu.rst | 13 ++++++++++---
 2 files changed, 40 insertions(+), 12 deletions(-)

diff --git a/cmd/bootmenu.c b/cmd/bootmenu.c
index ffa63a4628d..90f4f3d583c 100644
--- a/cmd/bootmenu.c
+++ b/cmd/bootmenu.c
@@ -330,7 +330,13 @@ static int prepare_uefi_bootorder_entry(struct bootmenu_data *menu,
 }
 #endif
 
-static struct bootmenu_data *bootmenu_create(int delay)
+/**
+ * bootmenu_create() - create boot menu entries
+ *
+ * @uefi:	consider UEFI boot options
+ * @delay:	autostart delay in seconds
+ */
+static struct bootmenu_data *bootmenu_create(int uefi, int delay)
 {
 	int ret;
 	unsigned short int i = 0;
@@ -357,7 +363,7 @@ static struct bootmenu_data *bootmenu_create(int delay)
 		goto cleanup;
 
 #if (IS_ENABLED(CONFIG_CMD_BOOTEFI_BOOTMGR)) && (IS_ENABLED(CONFIG_CMD_EFICONFIG))
-	if (i < MAX_COUNT - 1) {
+	if (uefi && i < MAX_COUNT - 1) {
 		efi_status_t efi_ret;
 lib/efi_loader/efi_setup.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/lib/efi_loader/efi_setup.c b/lib/efi_loader/efi_setup.c
index aa59bc7779d..8e0ff16f3eb 100644
--- a/lib/efi_loader/efi_setup.c
+++ b/lib/efi_loader/efi_setup.c
@@ -7,6 +7,7 @@
 
 #define LOG_CATEGORY LOGC_EFI
 
+#include <bootdev.h>
 #include <efi_loader.h>
 #include <efi_variable.h>
 #include <log.h>
@@ -228,6 +229,13 @@ efi_status_t efi_init_obj_list(void)
 	 * Probe block devices to find the ESP.
 	 * efi_disks_register() must be called before efi_init_variables().
 	 */
+	if (CONFIG_IS_ENABLED(BOOTSTD)) {
+		int r;
+
+		r = bootdev_hunt(NULL, 0);
+		if (r)
+			log_debug("No boot device available\n");
+	}
 	ret = efi_disks_register();
 	if (ret != EFI_SUCCESS)
 		goto out;
-- 
2.45.2

Re: [PATCH v2 2/6] configs: sandbox_deconfig: remove CONFIG_AMIGA_PARTITION

[Ilias Apalodimas]

On Wed, 27 Nov 2024 at 09:06, Heinrich Schuchardt
<heinrich.schuchardt@canonical.com> wrote:
>
> We do not actually test the code.
> Scanning for Amiga partitions of the sandbox is extremely slow,
> especially on the partially implemented USB device.
>
> For build testing the other sandbox defconfigs are good enough.
>
> Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
> ---
> v2:
>         no change
> ---
>  configs/sandbox_defconfig | 1 -
>  1 file changed, 1 deletion(-)
>
> diff --git a/configs/sandbox_defconfig b/configs/sandbox_defconfig
> index 718e4a8283c..683888f238f 100644
> --- a/configs/sandbox_defconfig
> +++ b/configs/sandbox_defconfig
> @@ -142,7 +142,6 @@ CONFIG_CMD_SQUASHFS=y
>  CONFIG_CMD_MTDPARTS=y
>  CONFIG_CMD_STACKPROTECTOR_TEST=y
>  CONFIG_MAC_PARTITION=y
> -CONFIG_AMIGA_PARTITION=y
>  CONFIG_OF_CONTROL=y
>  CONFIG_OF_LIVE=y
>  CONFIG_ENV_IS_NOWHERE=y
> --
> 2.45.2
>

Acked-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>

Re: [PATCH v2 3/6] CI: xilinx_versal_virt: disable USB_DWC3

[Michal Simek]

On 11/27/24 08:06, Heinrich Schuchardt wrote:
> The CI uses the following command to launch xilinx_versal_virt_defconfig:
> 
>      qemu-system-aarch64 -M xlnx-versal-virt \
>      -display none -m 4G -serial mon:stdio \
>      -device loader,file=u-boot,cpu-num=0
> 
> 'usb start' or invoking eth_bootdev_hunt leads to a crash when function
> dwc3_core_init() tries to access a register at offset 0xc704 (DWC3_DCTL)
> relative to the register start address 0xfe20c100.
> 
> Disable CONFIG_USB_DWC3 in the CI until the driver problem is fixed.
> 
> Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
> ---
> v2:
> 	Only disable CONFIG_USB_DWC3 in CI as actual hardware uses it.
> ---
>   .azure-pipelines.yml | 1 +
>   .gitlab-ci.yml       | 1 +
>   2 files changed, 2 insertions(+)
> 
> diff --git a/.azure-pipelines.yml b/.azure-pipelines.yml
> index 4ecf76eaa0b..bac577ec008 100644
> --- a/.azure-pipelines.yml
> +++ b/.azure-pipelines.yml
> @@ -511,6 +511,7 @@ stages:
>             TEST_PY_BD: "xilinx_versal_virt"
>             TEST_PY_ID: "--id qemu"
>             TEST_PY_TEST_SPEC: "not sleep"
> +          OVERRIDE: "-a ~CONFIG_USB_DWC3"
>           xtfpga:
>             TEST_PY_BD: "xtfpga"
>             TEST_PY_ID: "--id qemu"
> diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml
> index 2164ad79a72..714284a56df 100644
> --- a/.gitlab-ci.yml
> +++ b/.gitlab-ci.yml
> @@ -508,6 +508,7 @@ xilinx_versal_virt test.py:
>       TEST_PY_BD: "xilinx_versal_virt"
>       TEST_PY_TEST_SPEC: "not sleep"
>       TEST_PY_ID: "--id qemu"
> +    OVERRIDE: "-a ~CONFIG_USB_DWC3"
>     <<: *buildman_and_testpy_dfn
>   
>   xtfpga test.py:

If it is affecting only qemu CI then it is fine.
If this is related also to other HW lab then I think limitation should be done 
only on that particular qemu instance via u-boot-test-hook repo.

Thanks,
Michal

Re: [PATCH v2 4/6] net: eth_bootdev_hunt() should not run DHCP

[Simon Glass]

Hi Heinrich,

On Wed, 27 Nov 2024 at 00:07, Heinrich Schuchardt
<heinrich.schuchardt@canonical.com> wrote:
>
> Currently when booting dhcp_run() may be executed multiple times:
> once in eth_bootdev_hunt() and once in the network booting bootmeth.
>
> We need to call eth_bootdev_hunt() when setting up the EFI sub-system to
> supply the simple network protocol. We don't need an IP address set up.
>
> We can reduce the bootime by not executing dhcp_run() in
> eth_bootdev_hunt().
>
> Furthermore eth_bootdev_hunt() with autostart=yes leads on the legacy
> network statck leads to downloading a file via TFTP and to booting the

spelling

> downloaded file.

I have now found the feature you're referring to - the calls to
env_get_autostart(). Perhaps we can remove this feature? It seems
pretty old and I don't see boards using it.

That feature stops bootstd working properly.

But if we don't remove it, we need to disable autostart in dhcp_run(),
since in that case it does not behave correctly. I can do a patch for
that if needed.

>
> Instead of running dchp_run() just check that there is a network device
> in eth_bootdev_hunt().
>
> Reviewed-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
> ---
> v2:
>         reword the commit message
> ---
>  net/eth_bootdev.c | 30 ++++++++++++++++++------------
>  1 file changed, 18 insertions(+), 12 deletions(-)
>
> diff --git a/net/eth_bootdev.c b/net/eth_bootdev.c
> index 6ee54e3c790..b0fca6e8313 100644
> --- a/net/eth_bootdev.c
> +++ b/net/eth_bootdev.c
> @@ -64,9 +64,23 @@ static int eth_bootdev_bind(struct udevice *dev)
>         return 0;
>  }
>
> +/**
> + * eth_bootdev_hunt() - probe all network devices
> + *
> + * Network devices can also come from USB, but that is a higher
> + * priority (BOOTDEVP_5_SCAN_SLOW) than network, so it should have been
> + * enumerated already. If something like 'bootflow scan dhcp' is used,
> + * then the user will need to run 'usb start' first.
> + *
> + * @info:      info structure describing this hunter
> + * @show:      true to show information from the hunter
> + *
> + * Return:     0 if device found, -EINVAL otherwise
> + */
>  static int eth_bootdev_hunt(struct bootdev_hunter *info, bool show)
>  {
>         int ret;
> +       struct udevice *dev = NULL;
>
>         if (!test_eth_enabled())
>                 return 0;
> @@ -78,19 +92,11 @@ static int eth_bootdev_hunt(struct bootdev_hunter *info, bool show)
>                         log_warning("Failed to init PCI (%dE)\n", ret);
>         }
>
> -       /*
> -        * Ethernet devices can also come from USB, but that is a higher
> -        * priority (BOOTDEVP_5_SCAN_SLOW) than ethernet, so it should have been
> -        * enumerated already. If something like 'bootflow scan dhcp' is used
> -        * then the user will need to run 'usb start' first.
> -        */

Please keep this comment

> -       if (IS_ENABLED(CONFIG_CMD_DHCP)) {
> -               ret = dhcp_run(0, NULL, false);
> -               if (ret)
> -                       return -EINVAL;
> -       }
> +       ret = -EINVAL;
> +       uclass_foreach_dev_probe(UCLASS_ETH, dev)
> +               ret = 0;

There is a uclass_probe_all() function.

My suggestion from the original series was to just do the dhcp once.

How does probing the ethernet devices actually help EFI? It is not
needed for bootstd, since it probes any devices it uses.

>
> -       return 0;
> +       return ret;
>  }
>
>  struct bootdev_ops eth_bootdev_ops = {
> --
> 2.45.2
>

Regards,
Simon

Re: [PATCH v2 4/6] net: eth_bootdev_hunt() should not run DHCP

[Heinrich Schuchardt]

On 27.11.24 14:07, Simon Glass wrote:
> Hi Heinrich,
> 
> On Wed, 27 Nov 2024 at 00:07, Heinrich Schuchardt
> <heinrich.schuchardt@canonical.com> wrote:
>>
>> Currently when booting dhcp_run() may be executed multiple times:
>> once in eth_bootdev_hunt() and once in the network booting bootmeth.
>>
>> We need to call eth_bootdev_hunt() when setting up the EFI sub-system to
>> supply the simple network protocol. We don't need an IP address set up.
>>
>> We can reduce the bootime by not executing dhcp_run() in
>> eth_bootdev_hunt().
>>
>> Furthermore eth_bootdev_hunt() with autostart=yes leads on the legacy
>> network statck leads to downloading a file via TFTP and to booting the
> 
> spelling
> 
>> downloaded file.
> 
> I have now found the feature you're referring to - the calls to
> env_get_autostart(). Perhaps we can remove this feature? It seems
> pretty old and I don't see boards using it.
> 
> That feature stops bootstd working properly.
> 
> But if we don't remove it, we need to disable autostart in dhcp_run(),
> since in that case it does not behave correctly. I can do a patch for
> that if needed.
> 
>>
>> Instead of running dchp_run() just check that there is a network device
>> in eth_bootdev_hunt().
>>
>> Reviewed-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
>> Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
>> ---
>> v2:
>>          reword the commit message
>> ---
>>   net/eth_bootdev.c | 30 ++++++++++++++++++------------
>>   1 file changed, 18 insertions(+), 12 deletions(-)
>>
>> diff --git a/net/eth_bootdev.c b/net/eth_bootdev.c
>> index 6ee54e3c790..b0fca6e8313 100644
>> --- a/net/eth_bootdev.c
>> +++ b/net/eth_bootdev.c
>> @@ -64,9 +64,23 @@ static int eth_bootdev_bind(struct udevice *dev)
>>          return 0;
>>   }
>>
>> +/**
>> + * eth_bootdev_hunt() - probe all network devices
>> + *
>> + * Network devices can also come from USB, but that is a higher
>> + * priority (BOOTDEVP_5_SCAN_SLOW) than network, so it should have been
>> + * enumerated already. If something like 'bootflow scan dhcp' is used,
>> + * then the user will need to run 'usb start' first.
>> + *
>> + * @info:      info structure describing this hunter
>> + * @show:      true to show information from the hunter
>> + *
>> + * Return:     0 if device found, -EINVAL otherwise
>> + */
>>   static int eth_bootdev_hunt(struct bootdev_hunter *info, bool show)
>>   {
>>          int ret;
>> +       struct udevice *dev = NULL;
>>
>>          if (!test_eth_enabled())
>>                  return 0;
>> @@ -78,19 +92,11 @@ static int eth_bootdev_hunt(struct bootdev_hunter *info, bool show)
>>                          log_warning("Failed to init PCI (%dE)\n", ret);
>>          }
>>
>> -       /*
>> -        * Ethernet devices can also come from USB, but that is a higher
>> -        * priority (BOOTDEVP_5_SCAN_SLOW) than ethernet, so it should have been
>> -        * enumerated already. If something like 'bootflow scan dhcp' is used
>> -        * then the user will need to run 'usb start' first.
>> -        */
> 
> Please keep this comment

I have moved the comment to the function description. See above. I think 
that is the adequate place.

> 
>> -       if (IS_ENABLED(CONFIG_CMD_DHCP)) {
>> -               ret = dhcp_run(0, NULL, false);
>> -               if (ret)
>> -                       return -EINVAL;
>> -       }
>> +       ret = -EINVAL;
>> +       uclass_foreach_dev_probe(UCLASS_ETH, dev)
>> +               ret = 0;
> 
> There is a uclass_probe_all() function.
> 
> My suggestion from the original series was to just do the dhcp once.
> 
> How does probing the ethernet devices actually help EFI? It is not
> needed for bootstd, since it probes any devices it uses.

bootstd is not the only way to start an EFI application, you could use 
bootefi or bootm.

Best regards

Heinrich

> 
>>
>> -       return 0;
>> +       return ret;
>>   }
>>
>>   struct bootdev_ops eth_bootdev_ops = {
>> --
>> 2.45.2
>>
> 
> Regards,
> Simon

Re: [PATCH v2 4/6] net: eth_bootdev_hunt() should not run DHCP

[Simon Glass]

Hi Heinrich,

On Wed, 27 Nov 2024 at 07:00, Heinrich Schuchardt
<heinrich.schuchardt@canonical.com> wrote:
>
> On 27.11.24 14:07, Simon Glass wrote:
> > Hi Heinrich,
> >
> > On Wed, 27 Nov 2024 at 00:07, Heinrich Schuchardt
> > <heinrich.schuchardt@canonical.com> wrote:
> >>
> >> Currently when booting dhcp_run() may be executed multiple times:
> >> once in eth_bootdev_hunt() and once in the network booting bootmeth.
> >>
> >> We need to call eth_bootdev_hunt() when setting up the EFI sub-system to
> >> supply the simple network protocol. We don't need an IP address set up.
> >>
> >> We can reduce the bootime by not executing dhcp_run() in
> >> eth_bootdev_hunt().
> >>
> >> Furthermore eth_bootdev_hunt() with autostart=yes leads on the legacy
> >> network statck leads to downloading a file via TFTP and to booting the
> >
> > spelling
> >
> >> downloaded file.
> >
> > I have now found the feature you're referring to - the calls to
> > env_get_autostart(). Perhaps we can remove this feature? It seems
> > pretty old and I don't see boards using it.
> >
> > That feature stops bootstd working properly.
> >
> > But if we don't remove it, we need to disable autostart in dhcp_run(),
> > since in that case it does not behave correctly. I can do a patch for
> > that if needed.
> >
> >>
> >> Instead of running dchp_run() just check that there is a network device
> >> in eth_bootdev_hunt().
> >>
> >> Reviewed-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> >> Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
> >> ---
> >> v2:
> >>          reword the commit message
> >> ---
> >>   net/eth_bootdev.c | 30 ++++++++++++++++++------------
> >>   1 file changed, 18 insertions(+), 12 deletions(-)
> >>
> >> diff --git a/net/eth_bootdev.c b/net/eth_bootdev.c
> >> index 6ee54e3c790..b0fca6e8313 100644
> >> --- a/net/eth_bootdev.c
> >> +++ b/net/eth_bootdev.c
> >> @@ -64,9 +64,23 @@ static int eth_bootdev_bind(struct udevice *dev)
> >>          return 0;
> >>   }
> >>
> >> +/**
> >> + * eth_bootdev_hunt() - probe all network devices
> >> + *
> >> + * Network devices can also come from USB, but that is a higher
> >> + * priority (BOOTDEVP_5_SCAN_SLOW) than network, so it should have been
> >> + * enumerated already. If something like 'bootflow scan dhcp' is used,
> >> + * then the user will need to run 'usb start' first.
> >> + *
> >> + * @info:      info structure describing this hunter
> >> + * @show:      true to show information from the hunter
> >> + *
> >> + * Return:     0 if device found, -EINVAL otherwise
> >> + */
> >>   static int eth_bootdev_hunt(struct bootdev_hunter *info, bool show)
> >>   {
> >>          int ret;
> >> +       struct udevice *dev = NULL;
> >>
> >>          if (!test_eth_enabled())
> >>                  return 0;
> >> @@ -78,19 +92,11 @@ static int eth_bootdev_hunt(struct bootdev_hunter *info, bool show)
> >>                          log_warning("Failed to init PCI (%dE)\n", ret);
> >>          }
> >>
> >> -       /*
> >> -        * Ethernet devices can also come from USB, but that is a higher
> >> -        * priority (BOOTDEVP_5_SCAN_SLOW) than ethernet, so it should have been
> >> -        * enumerated already. If something like 'bootflow scan dhcp' is used
> >> -        * then the user will need to run 'usb start' first.
> >> -        */
> >
> > Please keep this comment
>
> I have moved the comment to the function description. See above. I think
> that is the adequate place.
>
> >
> >> -       if (IS_ENABLED(CONFIG_CMD_DHCP)) {
> >> -               ret = dhcp_run(0, NULL, false);
> >> -               if (ret)
> >> -                       return -EINVAL;
> >> -       }
> >> +       ret = -EINVAL;
> >> +       uclass_foreach_dev_probe(UCLASS_ETH, dev)
> >> +               ret = 0;
> >
> > There is a uclass_probe_all() function.
> >
> > My suggestion from the original series was to just do the dhcp once.
> >
> > How does probing the ethernet devices actually help EFI? It is not
> > needed for bootstd, since it probes any devices it uses.
>
> bootstd is not the only way to start an EFI application, you could use
> bootefi or bootm.

That's fine, but I'm just trying to understand what probing does, in
this particular case. Doesn't EFI probe a device before it uses it? I
think I am just missing some context.

Regards,
Simon

Re: [PATCH v2 4/6] net: eth_bootdev_hunt() should not run DHCP

[Heinrich Schuchardt]

On 27.11.24 15:14, Simon Glass wrote:
> Hi Heinrich,
> 
> On Wed, 27 Nov 2024 at 07:00, Heinrich Schuchardt
> <heinrich.schuchardt@canonical.com> wrote:
>>
>> On 27.11.24 14:07, Simon Glass wrote:
>>> Hi Heinrich,
>>>
>>> On Wed, 27 Nov 2024 at 00:07, Heinrich Schuchardt
>>> <heinrich.schuchardt@canonical.com> wrote:
>>>>
>>>> Currently when booting dhcp_run() may be executed multiple times:
>>>> once in eth_bootdev_hunt() and once in the network booting bootmeth.
>>>>
>>>> We need to call eth_bootdev_hunt() when setting up the EFI sub-system to
>>>> supply the simple network protocol. We don't need an IP address set up.
>>>>
>>>> We can reduce the bootime by not executing dhcp_run() in
>>>> eth_bootdev_hunt().
>>>>
>>>> Furthermore eth_bootdev_hunt() with autostart=yes leads on the legacy
>>>> network statck leads to downloading a file via TFTP and to booting the
>>>
>>> spelling

I will handle that in my pull request.

>>>
>>>> downloaded file.
>>>
>>> I have now found the feature you're referring to - the calls to
>>> env_get_autostart(). Perhaps we can remove this feature? It seems
>>> pretty old and I don't see boards using it.

Commands controlled by autostart include bootelf, dhcp, tftboot.

For tftpboot autostart=no makes sense if you may want to download 
multiple files (kernel, initrd, dtb) before booting or the downloaded 
file is a firmware upgrade.

But for PXE booting you will need autostart=yes. Many routers allow 
recovery via TFTP.

For bootelf autostart=no leads to only checking the file in memory but 
not actually booting.

I don't think that we can simply remove the setting looking at the 
diverse use cases.

>>>
>>> That feature stops bootstd working properly.
>>>
>>> But if we don't remove it, we need to disable autostart in dhcp_run(),
>>> since in that case it does not behave correctly. I can do a patch for
>>> that if needed.

That change can be implemented separately from this patch series.

>>>
>>>>
>>>> Instead of running dchp_run() just check that there is a network device
>>>> in eth_bootdev_hunt().
>>>>
>>>> Reviewed-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
>>>> Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
>>>> ---
>>>> v2:
>>>>           reword the commit message
>>>> ---
>>>>    net/eth_bootdev.c | 30 ++++++++++++++++++------------
>>>>    1 file changed, 18 insertions(+), 12 deletions(-)
>>>>
>>>> diff --git a/net/eth_bootdev.c b/net/eth_bootdev.c
>>>> index 6ee54e3c790..b0fca6e8313 100644
>>>> --- a/net/eth_bootdev.c
>>>> +++ b/net/eth_bootdev.c
>>>> @@ -64,9 +64,23 @@ static int eth_bootdev_bind(struct udevice *dev)
>>>>           return 0;
>>>>    }
>>>>
>>>> +/**
>>>> + * eth_bootdev_hunt() - probe all network devices
>>>> + *
>>>> + * Network devices can also come from USB, but that is a higher
>>>> + * priority (BOOTDEVP_5_SCAN_SLOW) than network, so it should have been
>>>> + * enumerated already. If something like 'bootflow scan dhcp' is used,
>>>> + * then the user will need to run 'usb start' first.
>>>> + *
>>>> + * @info:      info structure describing this hunter
>>>> + * @show:      true to show information from the hunter
>>>> + *
>>>> + * Return:     0 if device found, -EINVAL otherwise
>>>> + */
>>>>    static int eth_bootdev_hunt(struct bootdev_hunter *info, bool show)
>>>>    {
>>>>           int ret;
>>>> +       struct udevice *dev = NULL;
>>>>
>>>>           if (!test_eth_enabled())
>>>>                   return 0;
>>>> @@ -78,19 +92,11 @@ static int eth_bootdev_hunt(struct bootdev_hunter *info, bool show)
>>>>                           log_warning("Failed to init PCI (%dE)\n", ret);
>>>>           }
>>>>
>>>> -       /*
>>>> -        * Ethernet devices can also come from USB, but that is a higher
>>>> -        * priority (BOOTDEVP_5_SCAN_SLOW) than ethernet, so it should have been
>>>> -        * enumerated already. If something like 'bootflow scan dhcp' is used
>>>> -        * then the user will need to run 'usb start' first.
>>>> -        */
>>>
>>> Please keep this comment
>>
>> I have moved the comment to the function description. See above. I think
>> that is the adequate place.
>>
>>>
>>>> -       if (IS_ENABLED(CONFIG_CMD_DHCP)) {
>>>> -               ret = dhcp_run(0, NULL, false);
>>>> -               if (ret)
>>>> -                       return -EINVAL;
>>>> -       }
>>>> +       ret = -EINVAL;
>>>> +       uclass_foreach_dev_probe(UCLASS_ETH, dev)
>>>> +               ret = 0;
>>>
>>> There is a uclass_probe_all() function.
>>>
>>> My suggestion from the original series was to just do the dhcp once.
>>>
>>> How does probing the ethernet devices actually help EFI? It is not
>>> needed for bootstd, since it probes any devices it uses.
>>
>> bootstd is not the only way to start an EFI application, you could use
>> bootefi or bootm.
> 
> That's fine, but I'm just trying to understand what probing does, in
> this particular case. Doesn't EFI probe a device before it uses it? I
> think I am just missing some context.

EFI block devices will not even exist if not probed. See function 
efi_disk_probe().

For network devices I would like to change the code in a similar fashion 
in future to make all network devices available in the EFI sub-system.

Best regards

Heinrich

Re: [PATCH v2 4/6] net: eth_bootdev_hunt() should not run DHCP

[Simon Glass]

Hi Heinrich,

On Wed, 4 Dec 2024 at 02:10, Heinrich Schuchardt
<heinrich.schuchardt@canonical.com> wrote:
>
> On 27.11.24 15:14, Simon Glass wrote:
> > Hi Heinrich,
> >
> > On Wed, 27 Nov 2024 at 07:00, Heinrich Schuchardt
> > <heinrich.schuchardt@canonical.com> wrote:
> >>
> >> On 27.11.24 14:07, Simon Glass wrote:
> >>> Hi Heinrich,
> >>>
> >>> On Wed, 27 Nov 2024 at 00:07, Heinrich Schuchardt
> >>> <heinrich.schuchardt@canonical.com> wrote:
> >>>>
> >>>> Currently when booting dhcp_run() may be executed multiple times:
> >>>> once in eth_bootdev_hunt() and once in the network booting bootmeth.
> >>>>
> >>>> We need to call eth_bootdev_hunt() when setting up the EFI sub-system to
> >>>> supply the simple network protocol. We don't need an IP address set up.
> >>>>
> >>>> We can reduce the bootime by not executing dhcp_run() in
> >>>> eth_bootdev_hunt().
> >>>>
> >>>> Furthermore eth_bootdev_hunt() with autostart=yes leads on the legacy
> >>>> network statck leads to downloading a file via TFTP and to booting the
> >>>
> >>> spelling
>
> I will handle that in my pull request.
>
> >>>
> >>>> downloaded file.
> >>>
> >>> I have now found the feature you're referring to - the calls to
> >>> env_get_autostart(). Perhaps we can remove this feature? It seems
> >>> pretty old and I don't see boards using it.
>
> Commands controlled by autostart include bootelf, dhcp, tftboot.
>
> For tftpboot autostart=no makes sense if you may want to download
> multiple files (kernel, initrd, dtb) before booting or the downloaded
> file is a firmware upgrade.
>
> But for PXE booting you will need autostart=yes. Many routers allow
> recovery via TFTP.
>
> For bootelf autostart=no leads to only checking the file in memory but
> not actually booting.
>
> I don't think that we can simply remove the setting looking at the
> diverse use cases.
>
> >>>
> >>> That feature stops bootstd working properly.
> >>>
> >>> But if we don't remove it, we need to disable autostart in dhcp_run(),
> >>> since in that case it does not behave correctly. I can do a patch for
> >>> that if needed.
>
> That change can be implemented separately from this patch series.
>
> >>>
> >>>>
> >>>> Instead of running dchp_run() just check that there is a network device
> >>>> in eth_bootdev_hunt().
> >>>>
> >>>> Reviewed-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> >>>> Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
> >>>> ---
> >>>> v2:
> >>>>           reword the commit message
> >>>> ---
> >>>>    net/eth_bootdev.c | 30 ++++++++++++++++++------------
> >>>>    1 file changed, 18 insertions(+), 12 deletions(-)
> >>>>
> >>>> diff --git a/net/eth_bootdev.c b/net/eth_bootdev.c
> >>>> index 6ee54e3c790..b0fca6e8313 100644
> >>>> --- a/net/eth_bootdev.c
> >>>> +++ b/net/eth_bootdev.c
> >>>> @@ -64,9 +64,23 @@ static int eth_bootdev_bind(struct udevice *dev)
> >>>>           return 0;
> >>>>    }
> >>>>
> >>>> +/**
> >>>> + * eth_bootdev_hunt() - probe all network devices
> >>>> + *
> >>>> + * Network devices can also come from USB, but that is a higher
> >>>> + * priority (BOOTDEVP_5_SCAN_SLOW) than network, so it should have been
> >>>> + * enumerated already. If something like 'bootflow scan dhcp' is used,
> >>>> + * then the user will need to run 'usb start' first.
> >>>> + *
> >>>> + * @info:      info structure describing this hunter
> >>>> + * @show:      true to show information from the hunter
> >>>> + *
> >>>> + * Return:     0 if device found, -EINVAL otherwise
> >>>> + */
> >>>>    static int eth_bootdev_hunt(struct bootdev_hunter *info, bool show)
> >>>>    {
> >>>>           int ret;
> >>>> +       struct udevice *dev = NULL;
> >>>>
> >>>>           if (!test_eth_enabled())
> >>>>                   return 0;
> >>>> @@ -78,19 +92,11 @@ static int eth_bootdev_hunt(struct bootdev_hunter *info, bool show)
> >>>>                           log_warning("Failed to init PCI (%dE)\n", ret);
> >>>>           }
> >>>>
> >>>> -       /*
> >>>> -        * Ethernet devices can also come from USB, but that is a higher
> >>>> -        * priority (BOOTDEVP_5_SCAN_SLOW) than ethernet, so it should have been
> >>>> -        * enumerated already. If something like 'bootflow scan dhcp' is used
> >>>> -        * then the user will need to run 'usb start' first.
> >>>> -        */
> >>>
> >>> Please keep this comment
> >>
> >> I have moved the comment to the function description. See above. I think
> >> that is the adequate place.
> >>
> >>>
> >>>> -       if (IS_ENABLED(CONFIG_CMD_DHCP)) {
> >>>> -               ret = dhcp_run(0, NULL, false);
> >>>> -               if (ret)
> >>>> -                       return -EINVAL;
> >>>> -       }
> >>>> +       ret = -EINVAL;
> >>>> +       uclass_foreach_dev_probe(UCLASS_ETH, dev)
> >>>> +               ret = 0;
> >>>
> >>> There is a uclass_probe_all() function.
> >>>
> >>> My suggestion from the original series was to just do the dhcp once.
> >>>
> >>> How does probing the ethernet devices actually help EFI? It is not
> >>> needed for bootstd, since it probes any devices it uses.
> >>
> >> bootstd is not the only way to start an EFI application, you could use
> >> bootefi or bootm.
> >
> > That's fine, but I'm just trying to understand what probing does, in
> > this particular case. Doesn't EFI probe a device before it uses it? I
> > think I am just missing some context.
>
> EFI block devices will not even exist if not probed. See function
> efi_disk_probe().
>
> For network devices I would like to change the code in a similar fashion
> in future to make all network devices available in the EFI sub-system.

OK, thanks for the info. This patch seems OK since it fixes your problem,

Reviewed-by: Simon Glass <sjg@chromium.org>

Here's my understanding of what could be next:
- land my PXE series (which unfortunately needs another spin)
- update dhcp_run() so that it controls autostart, perhaps put
autostart in struct bootm_info?
- update bootstd so it only runs DHCP once on a device

Regards,
Simon

Re: [PATCH v2 5/6] cmd: bootmenu: add parameter -e for UEFI boot options

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 1104 bytes --]

On Wed, Nov 27, 2024 at 08:06:30AM +0100, Heinrich Schuchardt wrote:

> The bootmenu command can display
> 
> * menu entries defined by environment variables
> * menu entries defined by UEFI boot options
> 
> Not in all cases showing the UEFI boot options is desired.
> Provide a new parameter '-e' to select the display of UEFI boot options.
> 
> Reviewed-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>

Very strangely, this leads to the EFI selftest "Executing 'random number
generator'" test failing on Pi 4, where was was passing previously (and
Pi 4 has an RNG, unlike Pi 3), and this is on rpi_4_defconfig with:
CONFIG_CMD_BOOTEFI_HELLO=y
CONFIG_CMD_BOOTEFI_SELFTEST=y
CONFIG_IPV6=y
CONFIG_IPV6_ROUTER_DISCOVERY=y
CONFIG_CMD_TFTPPUT=y
CONFIG_FIT=y
CONFIG_FIT_SIGNATURE=y
CONFIG_CMD_BOOTMENU=y
CONFIG_CMD_LOG=y
CONFIG_UNIT_TEST=y
# CONFIG_CMD_EFIDEBUG is not set
CONFIG_BOOTSTAGE_STASH_ADDR=0x02400000
CONFIG_BOOTSTAGE=y
CONFIG_BOOTSTAGE_STASH=y
CONFIG_CMD_BOOTSTAGE=y

added.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH v2 5/6] cmd: bootmenu: add parameter -e for UEFI boot options

[Heinrich Schuchardt]

Tom Rini <trini@konsulko.com> schrieb am Fr., 10. Jan. 2025, 23:46:

> On Wed, Nov 27, 2024 at 08:06:30AM +0100, Heinrich Schuchardt wrote:
>
> > The bootmenu command can display
> >
> > * menu entries defined by environment variables
> > * menu entries defined by UEFI boot options
> >
> > Not in all cases showing the UEFI boot options is desired.
> > Provide a new parameter '-e' to select the display of UEFI boot options.
> >
> > Reviewed-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> > Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
>
> Very strangely, this leads to the EFI selftest "Executing 'random number
> generator'" test failing on Pi 4, where was was passing previously (and
> Pi 4 has an RNG, unlike Pi 3), and this is



Do you have a link to the log?

Best regards

Heinrich

on rpi_4_defconfig with:
> CONFIG_CMD_BOOTEFI_HELLO=y
> CONFIG_CMD_BOOTEFI_SELFTEST=y
> CONFIG_IPV6=y
> CONFIG_IPV6_ROUTER_DISCOVERY=y
> CONFIG_CMD_TFTPPUT=y
> CONFIG_FIT=y
> CONFIG_FIT_SIGNATURE=y
> CONFIG_CMD_BOOTMENU=y
> CONFIG_CMD_LOG=y
> CONFIG_UNIT_TEST=y
> # CONFIG_CMD_EFIDEBUG is not set
> CONFIG_BOOTSTAGE_STASH_ADDR=0x02400000
> CONFIG_BOOTSTAGE=y
> CONFIG_BOOTSTAGE_STASH=y
> CONFIG_CMD_BOOTSTAGE=y
>
> added.
>
> --
> Tom
>

Re: [PATCH v2 5/6] cmd: bootmenu: add parameter -e for UEFI boot options

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 1452 bytes --]

On Sun, Jan 12, 2025 at 08:08:13PM +0100, Heinrich Schuchardt wrote:
> Tom Rini <trini@konsulko.com> schrieb am Fr., 10. Jan. 2025, 23:46:
> 
> > On Wed, Nov 27, 2024 at 08:06:30AM +0100, Heinrich Schuchardt wrote:
> >
> > > The bootmenu command can display
> > >
> > > * menu entries defined by environment variables
> > > * menu entries defined by UEFI boot options
> > >
> > > Not in all cases showing the UEFI boot options is desired.
> > > Provide a new parameter '-e' to select the display of UEFI boot options.
> > >
> > > Reviewed-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> > > Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
> >
> > Very strangely, this leads to the EFI selftest "Executing 'random number
> > generator'" test failing on Pi 4, where was was passing previously (and
> > Pi 4 has an RNG, unlike Pi 3), and this is
> 
> Do you have a link to the log?

Here's the failing part:
Setting up 'random number generator'
ESC[0;37;40mESC[1;32;40mSetting up 'random number generator' succeeded
ESC[0;37;40mESC[1;34;40m
Executing 'random number generator'
ESC[0;37;40mESC[1;31;40mlib/efi_selftest/efi_selftest_rng.c(52):
ERROR: ESC[0;37;40mESC[1;31;40mRandom number generator protocol not
available
ESC[0;37;40mESC[1;31;40mlib/efi_selftest/efi_selftest.c(114):
ERROR: ESC[0;37;40mESC[1;31;40mExecuting 'random number generator'
failed
ESC[0;37;40mESC[1;34;40m

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH v2 5/6] cmd: bootmenu: add parameter -e for UEFI boot options

[Simon Glass]

Hi,

On Sun, 12 Jan 2025 at 18:51, Tom Rini <trini@konsulko.com> wrote:
>
> On Sun, Jan 12, 2025 at 08:08:13PM +0100, Heinrich Schuchardt wrote:
> > Tom Rini <trini@konsulko.com> schrieb am Fr., 10. Jan. 2025, 23:46:
> >
> > > On Wed, Nov 27, 2024 at 08:06:30AM +0100, Heinrich Schuchardt wrote:
> > >
> > > > The bootmenu command can display
> > > >
> > > > * menu entries defined by environment variables
> > > > * menu entries defined by UEFI boot options
> > > >
> > > > Not in all cases showing the UEFI boot options is desired.
> > > > Provide a new parameter '-e' to select the display of UEFI boot options.
> > > >
> > > > Reviewed-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> > > > Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
> > >
> > > Very strangely, this leads to the EFI selftest "Executing 'random number
> > > generator'" test failing on Pi 4, where was was passing previously (and
> > > Pi 4 has an RNG, unlike Pi 3), and this is
> >
> > Do you have a link to the log?
>
> Here's the failing part:
> Setting up 'random number generator'
> ESC[0;37;40mESC[1;32;40mSetting up 'random number generator' succeeded
> ESC[0;37;40mESC[1;34;40m
> Executing 'random number generator'
> ESC[0;37;40mESC[1;31;40mlib/efi_selftest/efi_selftest_rng.c(52):
> ERROR: ESC[0;37;40mESC[1;31;40mRandom number generator protocol not
> available
> ESC[0;37;40mESC[1;31;40mlib/efi_selftest/efi_selftest.c(114):
> ERROR: ESC[0;37;40mESC[1;31;40mExecuting 'random number generator'
> failed
> ESC[0;37;40mESC[1;34;40m

This might help make the log more intelligible.

https://patchwork.ozlabs.org/project/uboot/patch/20240811145209.4191404-37-sjg@chromium.org/

Regards,
Simon

Re: [PATCH v2 5/6] cmd: bootmenu: add parameter -e for UEFI boot options

[Heinrich Schuchardt]

Simon Glass <sjg@chromium.org> schrieb am Mo., 13. Jan. 2025, 20:02:

> Hi,
>
> On Sun, 12 Jan 2025 at 18:51, Tom Rini <trini@konsulko.com> wrote:
> >
> > On Sun, Jan 12, 2025 at 08:08:13PM +0100, Heinrich Schuchardt wrote:
> > > Tom Rini <trini@konsulko.com> schrieb am Fr., 10. Jan. 2025, 23:46:
> > >
> > > > On Wed, Nov 27, 2024 at 08:06:30AM +0100, Heinrich Schuchardt wrote:
> > > >
> > > > > The bootmenu command can display
> > > > >
> > > > > * menu entries defined by environment variables
> > > > > * menu entries defined by UEFI boot options
> > > > >
> > > > > Not in all cases showing the UEFI boot options is desired.
> > > > > Provide a new parameter '-e' to select the display of UEFI boot
> options.
> > > > >
> > > > > Reviewed-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> > > > > Signed-off-by: Heinrich Schuchardt <
> heinrich.schuchardt@canonical.com>
> > > >
> > > > Very strangely, this leads to the EFI selftest "Executing 'random
> number
> > > > generator'" test failing on Pi 4, where was was passing previously
> (and
> > > > Pi 4 has an RNG, unlike Pi 3), and this is
> > >
> > > Do you have a link to the log?
> >
> > Here's the failing part:
> > Setting up 'random number generator'
> > ESC[0;37;40mESC[1;32;40mSetting up 'random number generator' succeeded
> > ESC[0;37;40mESC[1;34;40m
> > Executing 'random number generator'
> > ESC[0;37;40mESC[1;31;40mlib/efi_selftest/efi_selftest_rng.c(52):
> > ERROR: ESC[0;37;40mESC[1;31;40mRandom number generator protocol not
> > available
> > ESC[0;37;40mESC[1;31;40mlib/efi_selftest/efi_selftest.c(114):
> > ERROR: ESC[0;37;40mESC[1;31;40mExecuting 'random number generator'
> > failed
> > ESC[0;37;40mESC[1;34;40m
>
> This might help make the log more intelligible.
>
>
> https://patchwork.ozlabs.org/project/uboot/patch/20240811145209.4191404-37-sjg@chromium.org/


No clue which problem you have to read the log.

Best regards

Heinrich


>
> Regards,
> Simon
>

Re: [PATCH v2 5/6] cmd: bootmenu: add parameter -e for UEFI boot options

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 2040 bytes --]

On Mon, Jan 13, 2025 at 12:01:45PM -0700, Simon Glass wrote:
> Hi,
> 
> On Sun, 12 Jan 2025 at 18:51, Tom Rini <trini@konsulko.com> wrote:
> >
> > On Sun, Jan 12, 2025 at 08:08:13PM +0100, Heinrich Schuchardt wrote:
> > > Tom Rini <trini@konsulko.com> schrieb am Fr., 10. Jan. 2025, 23:46:
> > >
> > > > On Wed, Nov 27, 2024 at 08:06:30AM +0100, Heinrich Schuchardt wrote:
> > > >
> > > > > The bootmenu command can display
> > > > >
> > > > > * menu entries defined by environment variables
> > > > > * menu entries defined by UEFI boot options
> > > > >
> > > > > Not in all cases showing the UEFI boot options is desired.
> > > > > Provide a new parameter '-e' to select the display of UEFI boot options.
> > > > >
> > > > > Reviewed-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> > > > > Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
> > > >
> > > > Very strangely, this leads to the EFI selftest "Executing 'random number
> > > > generator'" test failing on Pi 4, where was was passing previously (and
> > > > Pi 4 has an RNG, unlike Pi 3), and this is
> > >
> > > Do you have a link to the log?
> >
> > Here's the failing part:
> > Setting up 'random number generator'
> > ESC[0;37;40mESC[1;32;40mSetting up 'random number generator' succeeded
> > ESC[0;37;40mESC[1;34;40m
> > Executing 'random number generator'
> > ESC[0;37;40mESC[1;31;40mlib/efi_selftest/efi_selftest_rng.c(52):
> > ERROR: ESC[0;37;40mESC[1;31;40mRandom number generator protocol not
> > available
> > ESC[0;37;40mESC[1;31;40mlib/efi_selftest/efi_selftest.c(114):
> > ERROR: ESC[0;37;40mESC[1;31;40mExecuting 'random number generator'
> > failed
> > ESC[0;37;40mESC[1;34;40m
> 
> This might help make the log more intelligible.
> 
> https://patchwork.ozlabs.org/project/uboot/patch/20240811145209.4191404-37-sjg@chromium.org/

Shrug, it's the age old problem of sending raw stdout to a file. It
looks fine on console as it happens, I was just grabbing from an
existing log.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH v2 5/6] cmd: bootmenu: add parameter -e for UEFI boot options

[Simon Glass]

Hi Tom,

On Mon, 13 Jan 2025 at 12:24, Tom Rini <trini@konsulko.com> wrote:
>
> On Mon, Jan 13, 2025 at 12:01:45PM -0700, Simon Glass wrote:
> > Hi,
> >
> > On Sun, 12 Jan 2025 at 18:51, Tom Rini <trini@konsulko.com> wrote:
> > >
> > > On Sun, Jan 12, 2025 at 08:08:13PM +0100, Heinrich Schuchardt wrote:
> > > > Tom Rini <trini@konsulko.com> schrieb am Fr., 10. Jan. 2025, 23:46:
> > > >
> > > > > On Wed, Nov 27, 2024 at 08:06:30AM +0100, Heinrich Schuchardt wrote:
> > > > >
> > > > > > The bootmenu command can display
> > > > > >
> > > > > > * menu entries defined by environment variables
> > > > > > * menu entries defined by UEFI boot options
> > > > > >
> > > > > > Not in all cases showing the UEFI boot options is desired.
> > > > > > Provide a new parameter '-e' to select the display of UEFI boot options.
> > > > > >
> > > > > > Reviewed-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> > > > > > Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
> > > > >
> > > > > Very strangely, this leads to the EFI selftest "Executing 'random number
> > > > > generator'" test failing on Pi 4, where was was passing previously (and
> > > > > Pi 4 has an RNG, unlike Pi 3), and this is
> > > >
> > > > Do you have a link to the log?
> > >
> > > Here's the failing part:
> > > Setting up 'random number generator'
> > > ESC[0;37;40mESC[1;32;40mSetting up 'random number generator' succeeded
> > > ESC[0;37;40mESC[1;34;40m
> > > Executing 'random number generator'
> > > ESC[0;37;40mESC[1;31;40mlib/efi_selftest/efi_selftest_rng.c(52):
> > > ERROR: ESC[0;37;40mESC[1;31;40mRandom number generator protocol not
> > > available
> > > ESC[0;37;40mESC[1;31;40mlib/efi_selftest/efi_selftest.c(114):
> > > ERROR: ESC[0;37;40mESC[1;31;40mExecuting 'random number generator'
> > > failed
> > > ESC[0;37;40mESC[1;34;40m
> >
> > This might help make the log more intelligible.
> >
> > https://patchwork.ozlabs.org/project/uboot/patch/20240811145209.4191404-37-sjg@chromium.org/
>
> Shrug, it's the age old problem of sending raw stdout to a file. It
> looks fine on console as it happens, I was just grabbing from an
> existing log.

Oh well. I would probably suggest only writing this junk to log files
if we actually need it in there (i.e. something is programmatically
checking), but it seems like this is a different situation from the
one my patch fixes.

Regards,
Simon

Re: [PATCH v2 5/6] cmd: bootmenu: add parameter -e for UEFI boot options

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 2834 bytes --]

On Mon, Jan 13, 2025 at 01:09:16PM -0700, Simon Glass wrote:
> Hi Tom,
> 
> On Mon, 13 Jan 2025 at 12:24, Tom Rini <trini@konsulko.com> wrote:
> >
> > On Mon, Jan 13, 2025 at 12:01:45PM -0700, Simon Glass wrote:
> > > Hi,
> > >
> > > On Sun, 12 Jan 2025 at 18:51, Tom Rini <trini@konsulko.com> wrote:
> > > >
> > > > On Sun, Jan 12, 2025 at 08:08:13PM +0100, Heinrich Schuchardt wrote:
> > > > > Tom Rini <trini@konsulko.com> schrieb am Fr., 10. Jan. 2025, 23:46:
> > > > >
> > > > > > On Wed, Nov 27, 2024 at 08:06:30AM +0100, Heinrich Schuchardt wrote:
> > > > > >
> > > > > > > The bootmenu command can display
> > > > > > >
> > > > > > > * menu entries defined by environment variables
> > > > > > > * menu entries defined by UEFI boot options
> > > > > > >
> > > > > > > Not in all cases showing the UEFI boot options is desired.
> > > > > > > Provide a new parameter '-e' to select the display of UEFI boot options.
> > > > > > >
> > > > > > > Reviewed-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> > > > > > > Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
> > > > > >
> > > > > > Very strangely, this leads to the EFI selftest "Executing 'random number
> > > > > > generator'" test failing on Pi 4, where was was passing previously (and
> > > > > > Pi 4 has an RNG, unlike Pi 3), and this is
> > > > >
> > > > > Do you have a link to the log?
> > > >
> > > > Here's the failing part:
> > > > Setting up 'random number generator'
> > > > ESC[0;37;40mESC[1;32;40mSetting up 'random number generator' succeeded
> > > > ESC[0;37;40mESC[1;34;40m
> > > > Executing 'random number generator'
> > > > ESC[0;37;40mESC[1;31;40mlib/efi_selftest/efi_selftest_rng.c(52):
> > > > ERROR: ESC[0;37;40mESC[1;31;40mRandom number generator protocol not
> > > > available
> > > > ESC[0;37;40mESC[1;31;40mlib/efi_selftest/efi_selftest.c(114):
> > > > ERROR: ESC[0;37;40mESC[1;31;40mExecuting 'random number generator'
> > > > failed
> > > > ESC[0;37;40mESC[1;34;40m
> > >
> > > This might help make the log more intelligible.
> > >
> > > https://patchwork.ozlabs.org/project/uboot/patch/20240811145209.4191404-37-sjg@chromium.org/
> >
> > Shrug, it's the age old problem of sending raw stdout to a file. It
> > looks fine on console as it happens, I was just grabbing from an
> > existing log.
> 
> Oh well. I would probably suggest only writing this junk to log files
> if we actually need it in there (i.e. something is programmatically
> checking), but it seems like this is a different situation from the
> one my patch fixes.

I mean, yes? The problem here is that the random number generator test
now fails, when it didn't before. As far as I know (and I think the rest
of us understood?) your problem was that the ANSI sequences exist.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]

Re: [PATCH v2 5/6] cmd: bootmenu: add parameter -e for UEFI boot options

[Simon Glass]

Hi Tom,

On Mon, 13 Jan 2025 at 13:35, Tom Rini <trini@konsulko.com> wrote:
>
> On Mon, Jan 13, 2025 at 01:09:16PM -0700, Simon Glass wrote:
> > Hi Tom,
> >
> > On Mon, 13 Jan 2025 at 12:24, Tom Rini <trini@konsulko.com> wrote:
> > >
> > > On Mon, Jan 13, 2025 at 12:01:45PM -0700, Simon Glass wrote:
> > > > Hi,
> > > >
> > > > On Sun, 12 Jan 2025 at 18:51, Tom Rini <trini@konsulko.com> wrote:
> > > > >
> > > > > On Sun, Jan 12, 2025 at 08:08:13PM +0100, Heinrich Schuchardt wrote:
> > > > > > Tom Rini <trini@konsulko.com> schrieb am Fr., 10. Jan. 2025, 23:46:
> > > > > >
> > > > > > > On Wed, Nov 27, 2024 at 08:06:30AM +0100, Heinrich Schuchardt wrote:
> > > > > > >
> > > > > > > > The bootmenu command can display
> > > > > > > >
> > > > > > > > * menu entries defined by environment variables
> > > > > > > > * menu entries defined by UEFI boot options
> > > > > > > >
> > > > > > > > Not in all cases showing the UEFI boot options is desired.
> > > > > > > > Provide a new parameter '-e' to select the display of UEFI boot options.
> > > > > > > >
> > > > > > > > Reviewed-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> > > > > > > > Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
> > > > > > >
> > > > > > > Very strangely, this leads to the EFI selftest "Executing 'random number
> > > > > > > generator'" test failing on Pi 4, where was was passing previously (and
> > > > > > > Pi 4 has an RNG, unlike Pi 3), and this is
> > > > > >
> > > > > > Do you have a link to the log?
> > > > >
> > > > > Here's the failing part:
> > > > > Setting up 'random number generator'
> > > > > ESC[0;37;40mESC[1;32;40mSetting up 'random number generator' succeeded
> > > > > ESC[0;37;40mESC[1;34;40m
> > > > > Executing 'random number generator'
> > > > > ESC[0;37;40mESC[1;31;40mlib/efi_selftest/efi_selftest_rng.c(52):
> > > > > ERROR: ESC[0;37;40mESC[1;31;40mRandom number generator protocol not
> > > > > available
> > > > > ESC[0;37;40mESC[1;31;40mlib/efi_selftest/efi_selftest.c(114):
> > > > > ERROR: ESC[0;37;40mESC[1;31;40mExecuting 'random number generator'
> > > > > failed
> > > > > ESC[0;37;40mESC[1;34;40m
> > > >
> > > > This might help make the log more intelligible.
> > > >
> > > > https://patchwork.ozlabs.org/project/uboot/patch/20240811145209.4191404-37-sjg@chromium.org/
> > >
> > > Shrug, it's the age old problem of sending raw stdout to a file. It
> > > looks fine on console as it happens, I was just grabbing from an
> > > existing log.
> >
> > Oh well. I would probably suggest only writing this junk to log files
> > if we actually need it in there (i.e. something is programmatically
> > checking), but it seems like this is a different situation from the
> > one my patch fixes.
>
> I mean, yes? The problem here is that the random number generator test
> now fails, when it didn't before. As far as I know (and I think the rest
> of us understood?) your problem was that the ANSI sequences exist.

Yes, I want them to be controlled, so we can have them when needed
(e.g. for user benefit, getting the terminal size) but not when not
needed (CI output, log files, testing that ANSI works, etc.)

Regards,
Simon

Re: [PATCH v2 5/6] cmd: bootmenu: add parameter -e for UEFI boot options

[Tom Rini]

[-- Attachment #1: Type: text/plain, Size: 3782 bytes --]

On Mon, Jan 13, 2025 at 05:15:08PM -0700, Simon Glass wrote:
> Hi Tom,
> 
> On Mon, 13 Jan 2025 at 13:35, Tom Rini <trini@konsulko.com> wrote:
> >
> > On Mon, Jan 13, 2025 at 01:09:16PM -0700, Simon Glass wrote:
> > > Hi Tom,
> > >
> > > On Mon, 13 Jan 2025 at 12:24, Tom Rini <trini@konsulko.com> wrote:
> > > >
> > > > On Mon, Jan 13, 2025 at 12:01:45PM -0700, Simon Glass wrote:
> > > > > Hi,
> > > > >
> > > > > On Sun, 12 Jan 2025 at 18:51, Tom Rini <trini@konsulko.com> wrote:
> > > > > >
> > > > > > On Sun, Jan 12, 2025 at 08:08:13PM +0100, Heinrich Schuchardt wrote:
> > > > > > > Tom Rini <trini@konsulko.com> schrieb am Fr., 10. Jan. 2025, 23:46:
> > > > > > >
> > > > > > > > On Wed, Nov 27, 2024 at 08:06:30AM +0100, Heinrich Schuchardt wrote:
> > > > > > > >
> > > > > > > > > The bootmenu command can display
> > > > > > > > >
> > > > > > > > > * menu entries defined by environment variables
> > > > > > > > > * menu entries defined by UEFI boot options
> > > > > > > > >
> > > > > > > > > Not in all cases showing the UEFI boot options is desired.
> > > > > > > > > Provide a new parameter '-e' to select the display of UEFI boot options.
> > > > > > > > >
> > > > > > > > > Reviewed-by: Ilias Apalodimas <ilias.apalodimas@linaro.org>
> > > > > > > > > Signed-off-by: Heinrich Schuchardt <heinrich.schuchardt@canonical.com>
> > > > > > > >
> > > > > > > > Very strangely, this leads to the EFI selftest "Executing 'random number
> > > > > > > > generator'" test failing on Pi 4, where was was passing previously (and
> > > > > > > > Pi 4 has an RNG, unlike Pi 3), and this is
> > > > > > >
> > > > > > > Do you have a link to the log?
> > > > > >
> > > > > > Here's the failing part:
> > > > > > Setting up 'random number generator'
> > > > > > ESC[0;37;40mESC[1;32;40mSetting up 'random number generator' succeeded
> > > > > > ESC[0;37;40mESC[1;34;40m
> > > > > > Executing 'random number generator'
> > > > > > ESC[0;37;40mESC[1;31;40mlib/efi_selftest/efi_selftest_rng.c(52):
> > > > > > ERROR: ESC[0;37;40mESC[1;31;40mRandom number generator protocol not
> > > > > > available
> > > > > > ESC[0;37;40mESC[1;31;40mlib/efi_selftest/efi_selftest.c(114):
> > > > > > ERROR: ESC[0;37;40mESC[1;31;40mExecuting 'random number generator'
> > > > > > failed
> > > > > > ESC[0;37;40mESC[1;34;40m
> > > > >
> > > > > This might help make the log more intelligible.
> > > > >
> > > > > https://patchwork.ozlabs.org/project/uboot/patch/20240811145209.4191404-37-sjg@chromium.org/
> > > >
> > > > Shrug, it's the age old problem of sending raw stdout to a file. It
> > > > looks fine on console as it happens, I was just grabbing from an
> > > > existing log.
> > >
> > > Oh well. I would probably suggest only writing this junk to log files
> > > if we actually need it in there (i.e. something is programmatically
> > > checking), but it seems like this is a different situation from the
> > > one my patch fixes.
> >
> > I mean, yes? The problem here is that the random number generator test
> > now fails, when it didn't before. As far as I know (and I think the rest
> > of us understood?) your problem was that the ANSI sequences exist.
> 
> Yes, I want them to be controlled, so we can have them when needed
> (e.g. for user benefit, getting the terminal size) but not when not
> needed (CI output, log files, testing that ANSI works, etc.)

Yes, and Heinrich said for the test he wrote he finds having color
sequences to them (and not just the color sequence tests) helpful. That
is literally the only place we have them when not strictly needed AFAIK.
So using one of the countless "strip ansi color sequences from my log"
tool seems the best option here, when bothered by them.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]