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

[Heinrich Schuchardt]

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.

>> +             log_info("Restarting the system to boot the updated
>> firmware.\n");

I am missing a success message for each installed capsule.

>> +             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();
>>
>