On Tue, Feb 01, 2022 at 11:26:38AM +0900, Masami Hiramatsu wrote:
> Hi Heinrich,
>
> 2022年1月31日(月) 21:17 Heinrich Schuchardt <xypron.g...@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.hirama...@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,
While I don't object to adding a message, I'd rather see that a sub-command,
like "efidebug capsule show-result," be added to efidebug command.
Please note the result for each capsule will be recorded in "CapsuleNNNN"
variable which may contain additional information.
(I haven't implemented efi_variable_result_fmp though.)
-Takahiro Akashi
>
> >
> > >> + 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
