[PATCH v8 3/4] Boot var automatic management for removable medias

[Ilias Apalodimas]

On Wed, 7 Jun 2023 at 09:52, AKASHI Takahiro <takahiro.akashi at 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 at 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.

>
> > 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
callback *now* is a bad idea and that's what the commit message is
trying to hint.  We could in the future add an extra flag to the
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 at linaro.org>
> > > > Reviewed-by: Heinrich Schuchardt <xypron.glpk at gmx.de>
> > > > Reviewed-by: Ilias Apalodimas <ilias.apalodimas at 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
> > > >