Hi Heinrich,
On Tue, 6 Jun 2023 at 22:56, Heinrich Schuchardt xypron.glpk@gmx.de wrote:
On 6/6/23 21:43, Raymond Mao wrote:
Hi Ilias,
--- a/lib/efi_loader/efi_bootmgr.c +++ b/lib/efi_loader/efi_bootmgr.c @@ -663,11 +663,13 @@ efi_status_t efi_bootmgr_update_media_device_boot_option(void) NULL, &count, (efi_handle_t **)&volume_handles); if (ret != EFI_SUCCESS)
return ret;
goto out;I missed that in my original review, but I think this change is wrong. If you follow the goto out tag now instead of returning directly there's a chance efi_locate_handle_buffer_int() will return EFI_NOT_FOUND. The goto out tag will convert that to EFI_SUCCESS although it's an actual error.
This is what we intend to do. When there are no removable medias plug-in, efi_locate_handle_buffer_int() will return EFI_NOT_FOUND. And if we don't convert it to EFI_SUCCESS here, efi_bootmgr_update_media_device_boot_option() will return EFI_NOT_FOUND and then efi_init_obj_list() fails. This leads to the efi init failure and it will just prompt 'Error: Cannot initialize UEFI sub-system' when you run any efidebug commands.
Regards, Raymond
On Tue, 6 Jun 2023 at 15:24, Ilias Apalodimas ilias.apalodimas@linaro.org wrote:
Hi Raymond
On Tue, 6 Jun 2023 at 19:37, Raymond Mao raymond.mao@linaro.org wrote:
Correct the return code for out-of-memory and no boot option found
Signed-off-by: Raymond Mao raymond.mao@linaro.org Reviewed-by: Ilias Apalodimas ilias.apalodimas@linaro.org
Changes in v7
new patch file created
cmd/bootmenu.c | 2 +- cmd/eficonfig.c | 2 +- lib/efi_loader/efi_bootmgr.c | 8 ++++++-- 3 files changed, 8 insertions(+), 4 deletions(-)
diff --git a/cmd/bootmenu.c b/cmd/bootmenu.c index 01daddca7b..987b16889f 100644 --- a/cmd/bootmenu.c +++ b/cmd/bootmenu.c @@ -352,7 +352,7 @@ static struct bootmenu_data *bootmenu_create(int delay) * a architecture-specific default image name such as BOOTAA64.EFI. */ efi_ret = efi_bootmgr_update_media_device_boot_option();
if (efi_ret != EFI_SUCCESS && efi_ret != EFI_NOT_FOUND)
if (efi_ret != EFI_SUCCESS) goto cleanup; ret = prepare_uefi_bootorder_entry(menu, &iter, &i);diff --git a/cmd/eficonfig.c b/cmd/eficonfig.c index 82a80306f4..e6e8a0a488 100644 --- a/cmd/eficonfig.c +++ b/cmd/eficonfig.c @@ -2314,7 +2314,7 @@ static int do_eficonfig(struct cmd_tbl *cmdtp, int flag, int argc, char *const a return CMD_RET_FAILURE;
ret = efi_bootmgr_update_media_device_boot_option();
if (ret != EFI_SUCCESS && ret != EFI_NOT_FOUND)
if (ret != EFI_SUCCESS) return ret; while (1) {diff --git a/lib/efi_loader/efi_bootmgr.c b/lib/efi_loader/efi_bootmgr.c index 97d5b8dc2b..28e800499c 100644 --- a/lib/efi_loader/efi_bootmgr.c +++ b/lib/efi_loader/efi_bootmgr.c @@ -663,11 +663,13 @@ efi_status_t efi_bootmgr_update_media_device_boot_option(void) NULL, &count, (efi_handle_t **)&volume_handles); if (ret != EFI_SUCCESS)
return ret;
goto out;I missed that in my original review, but I think this change is wrong. If you follow the goto out tag now instead of returning directly there's a chance efi_locate_handle_buffer_int() will return EFI_NOT_FOUND. The goto out tag will convert that to EFI_SUCCESS although it's an actual error.
It was due to my reply https://lore.kernel.org/u-boot/e951264b-f952-3e83-dacc-bbe2fa37715f@gmx.de/ that Raymond moved the EFI_NOT_FOUND handling into this function. As said non-availability of a file system on removable media should not stop the initialization of the EFI sub-system or the probing of block devices.
Yea, I had a similar comment on an earlier version. EFI_NOT_FOUND was ignored by the caller in the past, so the current change is fine.
Why is efi_bootmgr_update_media_device_boot_option() not invoked when block devices are removed?
_remove() is being called before we exit to the OS, so we would end up constantly adding/removing boot options and writing to flashes, which is something we want to avoid. The downside is that the device removal and the boot options that might have been added, will be removed on the next boot. It's still a problem, but I don't think it's that important at the moment. Thanks /Ilias
Best regards
Heinrich
Thanks /Ilias
opt = calloc(count, sizeof(struct eficonfig_media_boot_option));
if (!opt)
if (!opt) {ret = EFI_OUT_OF_RESOURCES; goto out;} /* enumerate all devices supporting EFI_SIMPLE_FILE_SYSTEM_PROTOCOL */ ret = efi_bootmgr_enumerate_boot_option(opt, volume_handles, count);@@ -720,5 +722,7 @@ out: free(opt); efi_free_pool(volume_handles);
if (ret == EFI_NOT_FOUND)return EFI_SUCCESS; return ret;-- 2.25.1