[PATCH 2/2] efi_loader: Reset system after CapsuleUpdate on disk

[Masami Hiramatsu]

Hi Heinrich,

2022年1月31日(月) 21:17 Heinrich Schuchardt <xypron.glpk at gmx.de>:
>
> On 1/31/22 12:19, Grant Likely wrote:
> > On 31/01/2022 09:19, Masami Hiramatsu wrote:
> >> Add a config option to reset system soon after processing capsule update
> >> on disk. This is required in UEFI specification 2.9 Section 8.5.5
> >>   "Delivery of Capsules via file on Mass Storage device" as;
> >>
> >>      In all cases that a capsule is identified for processing the
> >> system is
> >>      restarted after capsule processing is completed.
> >>
> >> Signed-off-by: Masami Hiramatsu <masami.hiramatsu at linaro.org>
> >
> > Is there known use cases for making this an option? Feels a bit like
> > option creep that is too easy to choose the wrong setting.
> >
> > Otherwise, this looks good to me.
> >
> > g.
> >
> >> ---
> >>   lib/efi_loader/Kconfig       |   10 ++++++++++
> >>   lib/efi_loader/efi_capsule.c |    9 +++++++++
> >>   2 files changed, 19 insertions(+)
> >>
> >> diff --git a/lib/efi_loader/Kconfig b/lib/efi_loader/Kconfig
> >> index 24f9a2bb75..db05c3ad90 100644
> >> --- a/lib/efi_loader/Kconfig
> >> +++ b/lib/efi_loader/Kconfig
> >> @@ -146,6 +146,16 @@ config EFI_IGNORE_OSINDICATIONS
> >>         without setting the
> >> EFI_OS_INDICATIONS_FILE_CAPSULE_DELIVERY_SUPPORTED
> >>         flag in variable OsIndications.
> >>
> >> +config EFI_RESET_AFTER_CAPSULE_ON_DISK
> >> +     bool "Reset right after CapsuleUpdate on-disk"
> >> +     depends on EFI_CAPSULE_ON_DISK
> >> +     default y
> >> +     help
> >> +       UEFI specification requests the system to be restarted after
> >> capsule
> >> +       processing is complete. This implements that, but for some
> >> reason,
> >> +       if you want to keep the (old) system running after the capsule
> >> update
> >> +       on-disk, you can say 'n' here.
> >> +
> >>   config EFI_CAPSULE_ON_DISK_EARLY
> >>       bool "Initiate capsule-on-disk at U-Boot boottime"
> >>       depends on EFI_CAPSULE_ON_DISK
> >> diff --git a/lib/efi_loader/efi_capsule.c b/lib/efi_loader/efi_capsule.c
> >> index 98dab1c6f5..44d4fa2f82 100644
> >> --- a/lib/efi_loader/efi_capsule.c
> >> +++ b/lib/efi_loader/efi_capsule.c
> >> @@ -1142,6 +1142,15 @@ efi_status_t efi_launch_capsules(void)
> >>               free(files[i]);
> >>       free(files);
> >>
> >> +     /*
> >> +      * UEFI spec requires to reset system after complete processing
> >> capsule
> >> +      * update on the storage.
> >> +      */
> >> +     if (IS_ENABLED(CONFIG_EFI_RESET_AFTER_CAPSULE_ON_DISK)) {
>
> We should not allow configuring a non-compliant behavior.

OK, let me remove this option then.

>
> >> +             log_info("Restarting the system to boot the updated
> >> firmware.\n");
>
> I am missing a success message for each installed capsule.

Indeed, but this reboot will be done unconditionally.
let me add a message when the capsule update is successfully completed.
Thank you,


>
> >> +             do_reset(NULL, 0, 0, NULL);
>
> do_reset() may return. How about:
>
>     panic("Reboot after firmware update");
>
> This will hang if do_reset() returns.
>
> >> +     }
> >> +
> >>       if (IS_ENABLED(CONFIG_EFI_ESRT)) {
>
> We don't need this code anymore if we call panic() above.
>
> Best regards
>
> Heinrich
>
> >>               /* Rebuild the ESRT to reflect any updated FW images. */
> >>               ret = efi_esrt_populate();
> >>
> >



--
Masami Hiramatsu