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

[Heinrich Schuchardt]

On 2/2/22 05:15, AKASHI Takahiro wrote:
> On Wed, Feb 02, 2022 at 10:53:05AM +0900, Masami Hiramatsu wrote:
>> Hi Takahiro,
>>
>> 2022年2月1日(火) 20:38 AKASHI Takahiro <takahiro.akashi at linaro.org>:
>>
>>>
>>> On Tue, Feb 01, 2022 at 05:33:09PM +0900, 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.
>>>>
>>>> This also reports the result of each capsule update so that the user can
>>>> notice that the capsule update has been succeeded or not from console log.
>>>>
>>>> Signed-off-by: Masami Hiramatsu <masami.hiramatsu at linaro.org>
>>>> ---
>>>>   Changes in v2:
>>>>    - Remove kconfig option to disable this feature.
>>>>    - Use panic() instead of do_reset() so that if the reset fails,
>>>>      the machine halt.
>>>>    - Log the result of each capsule update always.
>>>> ---
>>>>   lib/efi_loader/efi_capsule.c |   12 +++++++++---
>>>>   1 file changed, 9 insertions(+), 3 deletions(-)
>>>>
>>>> diff --git a/lib/efi_loader/efi_capsule.c b/lib/efi_loader/efi_capsule.c
>>>> index 1ec7ea29ff..39bce714f7 100644
>>>> --- a/lib/efi_loader/efi_capsule.c
>>>> +++ b/lib/efi_loader/efi_capsule.c
>>>> @@ -1119,9 +1119,9 @@ efi_status_t efi_launch_capsules(void)
>>>>                ret = efi_capsule_read_file(files[i], &capsule);
>>>>                if (ret == EFI_SUCCESS) {
>>>>                        ret = efi_capsule_update_firmware(capsule);
>>>> -                     if (ret != EFI_SUCCESS)
>>>> -                             log_err("Applying capsule %ls failed\n",
>>>> -                                     files[i]);
>>>> +                     log_err("Applying capsule %ls %s\n",
>>>> +                             files[i],
>>>> +                             ret == EFI_SUCCESS ? "succeeded" : "failed");
>>>
>>> log_err()? log_info() is better, I think.
>>
>> Hmm, would you think to use log_info() even if it is failed? Or should
>> we have log_err(failure) and log_info(success)?
>
> It is what I meant :)
>
>>>
>>>>
>>>>                        /* create CapsuleXXXX */
>>>>                        set_capsule_result(index, capsule, ret);
>>>> @@ -1142,6 +1142,12 @@ 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.
>>>> +      */
>>>> +     panic("Reboot after firmware update");
>>>
>>> If CONFIG_PANIC_HANG is enabled, the system won't restart.
>>> It's not what we want here.
>>
>> Indeed.
>> Heinrich, what would you think if do_reset() doesn't work?
>> (I think it is OK to get it back here, but needs a warning)
>
> If (CONFIG_IS_ENABLED(SYSRESET)) {
>      puts ("resetting ...\n");
>      sysreset_reset_walk(SYSRESET_WARM);

do_reset() is implemented in many 25 places.
drivers/sysreset/sysreset-uclass.c is just one of them.

@Tom, @Simon:
Is there a migration timeline to replace all other do_reset()
implementations?

A dummy implementation like in arch/riscv/lib/reset.c should not exist.
The sysreset uclass handles the case of no sysreset driver already.

Best regards

Heinrich

> } else {
>      do_reset(...)
>      halt();
> }
> /* not reach here */
>
> -Takahiro Akashi
>
>
>> Thank you,
>>
>>>
>>> -Takahiro Akashi
>>>
>>>> +
>>>>        return ret;
>>>>   }
>>>>   #endif /* CONFIG_EFI_CAPSULE_ON_DISK */
>>>>
>>
>>
>>
>> --
>> Masami Hiramatsu