On Wed, Jun 07, 2023 at 10:00:37AM +0300, Ilias Apalodimas wrote:
On Wed, 7 Jun 2023 at 09:52, AKASHI Takahiro takahiro.akashi@linaro.org wrote:
On Wed, Jun 07, 2023 at 09:38:30AM +0300, Ilias Apalodimas wrote:
On Wed, 7 Jun 2023 at 03:41, AKASHI Takahiro takahiro.akashi@linaro.org wrote:
On Tue, Jun 06, 2023 at 09:37:20AM -0700, Raymond Mao wrote:
Changes for complying to EFI spec ยง3.5.1.1 'Removable Media Boot Behavior'. Boot variables can be automatically generated during a removable media is probed. At the same time, unused boot variables will be detected and removed. Please note that currently the function 'efi_disk_remove' is being invoked during exiting U-Boot and booting an OS - that is why we don't add the boot variables management inside 'efi_disk_remove'.
It's not accurate. For instance, a user may run "scsi scan" or "usb reset" to re-enumerate devices added (or removed). They will invoke "remove" against pre-existing devices before invoking a "probe" routine.
So the text above doesn't justify not implementing "remove" hook.
I am not sure I am following you here. Yes resetting the device will call the remove callback and it's a current problem of the existing implementation,
I think it is a design decision.
It's not. Ideally, we should call the _remove() callbacks and reconfigure the boot options correctly if a device gets unplugged or the subsystem is restarted. However, at least in my opinion, the problem isn't that big at the moment to prevent the patches from getting merged. If you restart the subsystem the same boot options will be rescanned and the device will be able to boot. The corner case here is scanning a few boot options and then *removing* the device. But in that case, the device wasn't going to boot anyway from the removed media.
First of all, we should distinguish two cases when you mention the *existing* issues in Raymond's current implementation: 1. Some device is added or removed when we are still on U-Boot 2. When we are existing U-Boot
In the above text, you mention case(1) which is not covered by his original commit message. It is one reason why I think that the commit message is not accurate.
but why is the commit message inaccurate? Won't exiting u-boot to jump to the OS call all the _remove() functions?
Exiting U-Boot is irrelevant to maintaining boot options.
In theory yes, but the reality is that you can't remove the boot options and rewrite flashes of embedded boards, with a limited lifetime, every time you boot your OS. So adding it on the _remove
Here you have much more explanation than the original message has for case(2). It is another reason why the commit message is not accurate (or I should say 'not enough'). Without those texts, most people won't understand what the commit message tries to say.
-Takahiro Akashi
We could in the future add an extra flag to the callback *now* is a bad idea and that's what the commit message is trying to hint. remove callback which only gets set to false when we are exiting u-boot
Thanks /Ilias
-Takahiro Akashi
Thanks /Ilias
-Takahiro Akashi
Signed-off-by: Raymond Mao raymond.mao@linaro.org Reviewed-by: Heinrich Schuchardt xypron.glpk@gmx.de Reviewed-by: Ilias Apalodimas ilias.apalodimas@linaro.org
Changes in v3
- Split the patch into moving and renaming functions and individual patches for each changed functionality
Changes in v5
- Move function call of efi_bootmgr_update_media_device_boot_option() from efi_init_variables() to efi_init_obj_list()
Changes in v6
- Revert unrelated changes
Changes in v7
- adapt the return code of function efi_bootmgr_update_media_device_boot_option()
Changes in v8
- add a note in the commit message for future reference
lib/efi_loader/efi_disk.c | 7 +++++++ lib/efi_loader/efi_setup.c | 5 +++++ 2 files changed, 12 insertions(+)
diff --git a/lib/efi_loader/efi_disk.c b/lib/efi_loader/efi_disk.c index d2256713a8..d9b48ecf64 100644 --- a/lib/efi_loader/efi_disk.c +++ b/lib/efi_loader/efi_disk.c @@ -687,6 +687,13 @@ int efi_disk_probe(void *ctx, struct event *event) return -1; }
/* only do the boot option management when UEFI sub-system is initialized */if (efi_obj_list_initialized == EFI_SUCCESS) {ret = efi_bootmgr_update_media_device_boot_option();if (ret != EFI_SUCCESS)return -1;}return 0;}
diff --git a/lib/efi_loader/efi_setup.c b/lib/efi_loader/efi_setup.c index 58d4e13402..877f3878d6 100644 --- a/lib/efi_loader/efi_setup.c +++ b/lib/efi_loader/efi_setup.c @@ -245,6 +245,11 @@ efi_status_t efi_init_obj_list(void) if (ret != EFI_SUCCESS) goto out;
/* update boot option after variable service initialized */ret = efi_bootmgr_update_media_device_boot_option();if (ret != EFI_SUCCESS)goto out;/* Define supported languages */ ret = efi_init_platform_lang(); if (ret != EFI_SUCCESS)-- 2.25.1