On Wed, 7 Jun 2023 at 10:27, AKASHI Takahiro <takahiro.aka...@linaro.org> wrote:
>
> On Wed, Jun 07, 2023 at 10:00:37AM +0300, Ilias Apalodimas wrote:
> > On Wed, 7 Jun 2023 at 09:52, AKASHI Takahiro <takahiro.aka...@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.aka...@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.
>
Fair enough. Raymond can you resend the series and add
- a more detailed explanation of why we are not removing the options
in the _remove() callback currently
- a comment in the function that explains the same thing for future reference?
Thanks
/Ilias
> -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....@linaro.org>
> > > > > > Reviewed-by: Heinrich Schuchardt <xypron.g...@gmx.de>
> > > > > > Reviewed-by: Ilias Apalodimas <ilias.apalodi...@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
> > > > > >
