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

[Tom Rini <trini@konsulko.com>]

[-- Attachment #1: Type: text/plain, Size: 4046 bytes --]

On Thu, Feb 03, 2022 at 06:32:50PM +0100, Heinrich Schuchardt wrote:
> 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@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@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.

Not yet, please feel free to propose something, if it can't just be done
outright, right now.

-- 
Tom

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 659 bytes --]