Hi Heinrich,
The reason that we don't add calling to
efi_bootmgr_update_media_device_boot_option() under efi_disk_remove()
is because efi_disk_remove() is always being called during exiting U-Boot and
booting an OS. - it doesn't make sense that the boot options we added is
removed there.
I added a text in the commit message of 'v8 3/4' patch for reference as
suggested by Ilias.
Regards,
Raymond
On Tue, 6 Jun 2023 at 15:56, Heinrich Schuchardt <xypron.g...@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.apalodi...@linaro.org> wrote:
> >>
> >> Hi Raymond
> >>
> >> On Tue, 6 Jun 2023 at 19:37, Raymond Mao <raymond....@linaro.org> wrote:
> >>>
> >>> Correct the return code for out-of-memory and no boot option found
> >>>
> >>> Signed-off-by: Raymond Mao <raymond....@linaro.org>
> >>> Reviewed-by: Ilias Apalodimas <ilias.apalodi...@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-bbe2fa377...@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.
>
> Why is efi_bootmgr_update_media_device_boot_option() not invoked when
> block devices are removed?
>
> 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
> >>>
>
