diff mbox series

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

Message ID 164388020701.446835.322529861013544459.stgit@localhost
State New
Headers show
Series EFI: Reset system after capsule-on-disk | expand

Commit Message

Masami Hiramatsu Feb. 3, 2022, 9:23 a.m. UTC 
Add a cold reset 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 v4:
  - Do not use sysreset because that is a warm reset.
  - Fix patch description.
---
 lib/efi_loader/efi_capsule.c |   18 ++++++++++++++++--
 1 file changed, 16 insertions(+), 2 deletions(-)

Comments

AKASHI Takahiro Feb. 9, 2022, 3:13 a.m. UTC | #1 
On Thu, Feb 03, 2022 at 06:23:27PM +0900, Masami Hiramatsu wrote:
> Add a cold reset 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.

Once this behavior is enforced on U-Boot, CONFIG_EFI_CAPSULE_ON_DISK_EARLY
will make little sense. This option will have to always be set.
Otherwise, a user will see a sudden system reboot when he or she types
any of efi commands, like "env print -e".

-Takahiro Akashi


> 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 v4:
>   - Do not use sysreset because that is a warm reset.
>   - Fix patch description.
> ---
>  lib/efi_loader/efi_capsule.c |   18 ++++++++++++++++--
>  1 file changed, 16 insertions(+), 2 deletions(-)
> 
> diff --git a/lib/efi_loader/efi_capsule.c b/lib/efi_loader/efi_capsule.c
> index 1ec7ea29ff..20d9490dbd 100644
> --- a/lib/efi_loader/efi_capsule.c
> +++ b/lib/efi_loader/efi_capsule.c
> @@ -14,6 +14,7 @@
>  #include <env.h>
>  #include <fdtdec.h>
>  #include <fs.h>
> +#include <hang.h>
>  #include <malloc.h>
>  #include <mapmem.h>
>  #include <sort.h>
> @@ -1120,8 +1121,11 @@ efi_status_t efi_launch_capsules(void)
>  		if (ret == EFI_SUCCESS) {
>  			ret = efi_capsule_update_firmware(capsule);
>  			if (ret != EFI_SUCCESS)
> -				log_err("Applying capsule %ls failed\n",
> +				log_err("Applying capsule %ls failed.\n",
>  					files[i]);
> +			else
> +				log_info("Applying capsule %ls succeeded.\n",
> +					 files[i]);
>  
>  			/* create CapsuleXXXX */
>  			set_capsule_result(index, capsule, ret);
> @@ -1142,6 +1146,16 @@ efi_status_t efi_launch_capsules(void)
>  		free(files[i]);
>  	free(files);
>  
> -	return ret;
> +	/*
> +	 * UEFI spec requires to reset system after complete processing capsule
> +	 * update on the storage.
> +	 */
> +	log_info("Reboot after firmware update");
> +	/* Cold reset is required for loading the new firmware. */
> +	do_reset(NULL, 0, 0, NULL);
> +	hang();
> +	/* not reach here */
> +
> +	return 0;
>  }
>  #endif /* CONFIG_EFI_CAPSULE_ON_DISK */
>
Masami Hiramatsu Feb. 9, 2022, 3:54 a.m. UTC | #2 
Hi Takahiro,

2022年2月9日(水) 12:13 AKASHI Takahiro <takahiro.akashi@linaro.org>:
>
> On Thu, Feb 03, 2022 at 06:23:27PM +0900, Masami Hiramatsu wrote:
> > Add a cold reset 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.
>
> Once this behavior is enforced on U-Boot, CONFIG_EFI_CAPSULE_ON_DISK_EARLY
> will make little sense. This option will have to always be set.

Agreed. This option is recommended. I hope U-Boot scans the devices
before running this early capsule-on-disk so that it can find the
appropriate storage for ESP, but anyway it works.

> Otherwise, a user will see a sudden system reboot when he or she types
> any of efi commands, like "env print -e".

Yes if there is a capsule to be updated, and they will see the new
u-boot coming back soon.
I guess the reason why the capsule-on-disk runs at first, is to avoid
inconsistent status for users, or we can postpone the capsule update
until running "efidebug capsule disk" command. If that is correct,
shouldn't we avoid showing the firmware "to be updated" too?

Thank you,

>
> -Takahiro Akashi
>
>
> > 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 v4:
> >   - Do not use sysreset because that is a warm reset.
> >   - Fix patch description.
> > ---
> >  lib/efi_loader/efi_capsule.c |   18 ++++++++++++++++--
> >  1 file changed, 16 insertions(+), 2 deletions(-)
> >
> > diff --git a/lib/efi_loader/efi_capsule.c b/lib/efi_loader/efi_capsule.c
> > index 1ec7ea29ff..20d9490dbd 100644
> > --- a/lib/efi_loader/efi_capsule.c
> > +++ b/lib/efi_loader/efi_capsule.c
> > @@ -14,6 +14,7 @@
> >  #include <env.h>
> >  #include <fdtdec.h>
> >  #include <fs.h>
> > +#include <hang.h>
> >  #include <malloc.h>
> >  #include <mapmem.h>
> >  #include <sort.h>
> > @@ -1120,8 +1121,11 @@ efi_status_t efi_launch_capsules(void)
> >               if (ret == EFI_SUCCESS) {
> >                       ret = efi_capsule_update_firmware(capsule);
> >                       if (ret != EFI_SUCCESS)
> > -                             log_err("Applying capsule %ls failed\n",
> > +                             log_err("Applying capsule %ls failed.\n",
> >                                       files[i]);
> > +                     else
> > +                             log_info("Applying capsule %ls succeeded.\n",
> > +                                      files[i]);
> >
> >                       /* create CapsuleXXXX */
> >                       set_capsule_result(index, capsule, ret);
> > @@ -1142,6 +1146,16 @@ efi_status_t efi_launch_capsules(void)
> >               free(files[i]);
> >       free(files);
> >
> > -     return ret;
> > +     /*
> > +      * UEFI spec requires to reset system after complete processing capsule
> > +      * update on the storage.
> > +      */
> > +     log_info("Reboot after firmware update");
> > +     /* Cold reset is required for loading the new firmware. */
> > +     do_reset(NULL, 0, 0, NULL);
> > +     hang();
> > +     /* not reach here */
> > +
> > +     return 0;
> >  }
> >  #endif /* CONFIG_EFI_CAPSULE_ON_DISK */
> >
Heinrich Schuchardt Feb. 13, 2022, 9:01 a.m. UTC | #3 
On 2/3/22 10:23, Masami Hiramatsu wrote:
> Add a cold reset 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>

Reviewed-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
Heinrich Schuchardt Feb. 13, 2022, 10:17 a.m. UTC | #4 
On 2/13/22 10:01, Heinrich Schuchardt wrote:
> On 2/3/22 10:23, Masami Hiramatsu wrote:
>> Add a cold reset 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>
>
> Reviewed-by: Heinrich Schuchardt <xypron.glpk@gmx.de>


Gitlab CI tests fail. Please, resubmit with the Python tests adjusted.
Make sure that 'make tests' does not fail.

https://source.denx.de/u-boot/custodians/u-boot-efi/-/jobs/392345

FAILED
test/py/tests/test_efi_capsule/test_capsule_firmware.py::TestEfiCapsuleFirmwareFit::test_efi_capsule_fw2
FAILED
test/py/tests/test_efi_capsule/test_capsule_firmware.py::TestEfiCapsuleFirmwareFit::test_efi_capsule_fw3
FAILED
test/py/tests/test_efi_capsule/test_capsule_firmware.py::TestEfiCapsuleFirmwareFit::test_efi_capsule_fw4

Best regards

Heinrich


Best regards

Heinrich
AKASHI Takahiro Feb. 14, 2022, 1:06 a.m. UTC | #5 
On Sun, Feb 13, 2022 at 11:17:37AM +0100, Heinrich Schuchardt wrote:
> On 2/13/22 10:01, Heinrich Schuchardt wrote:
> > On 2/3/22 10:23, Masami Hiramatsu wrote:
> > > Add a cold reset 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>
> > 
> > Reviewed-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> 
> 
> Gitlab CI tests fail. Please, resubmit with the Python tests adjusted.
> Make sure that 'make tests' does not fail.
> 
> https://source.denx.de/u-boot/custodians/u-boot-efi/-/jobs/392345
> 
> FAILED
> test/py/tests/test_efi_capsule/test_capsule_firmware.py::TestEfiCapsuleFirmwareFit::test_efi_capsule_fw2
> FAILED
> test/py/tests/test_efi_capsule/test_capsule_firmware.py::TestEfiCapsuleFirmwareFit::test_efi_capsule_fw3
> FAILED
> test/py/tests/test_efi_capsule/test_capsule_firmware.py::TestEfiCapsuleFirmwareFit::test_efi_capsule_fw4

I should have mentioned this in my previous comment.

My capsule tests assume that the capsule update does *not* initiate
a reboot automatically and does "reboot" by "env print -e Capsule0000".

Furthermore, since the current sandbox_defconfig does not enable any
U-Boot environment storage, "dfu_alt_info," for instance, cannot
retain across the reboot (and so I didn't use CAPSULE_ON_DISK_EARLY).

I will help Masami fix the issue.

-Takahiro Akashi





> Best regards
> 
> Heinrich
> 
> 
> Best regards
> 
> Heinrich
Masami Hiramatsu Feb. 14, 2022, 2:39 a.m. UTC | #6 
Hi Takahiro,

2022年2月14日(月) 10:06 AKASHI Takahiro <takahiro.akashi@linaro.org>:
>
> On Sun, Feb 13, 2022 at 11:17:37AM +0100, Heinrich Schuchardt wrote:
> > On 2/13/22 10:01, Heinrich Schuchardt wrote:
> > > On 2/3/22 10:23, Masami Hiramatsu wrote:
> > > > Add a cold reset 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>
> > >
> > > Reviewed-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
> >
> >
> > Gitlab CI tests fail. Please, resubmit with the Python tests adjusted.
> > Make sure that 'make tests' does not fail.
> >
> > https://source.denx.de/u-boot/custodians/u-boot-efi/-/jobs/392345
> >
> > FAILED
> > test/py/tests/test_efi_capsule/test_capsule_firmware.py::TestEfiCapsuleFirmwareFit::test_efi_capsule_fw2
> > FAILED
> > test/py/tests/test_efi_capsule/test_capsule_firmware.py::TestEfiCapsuleFirmwareFit::test_efi_capsule_fw3
> > FAILED
> > test/py/tests/test_efi_capsule/test_capsule_firmware.py::TestEfiCapsuleFirmwareFit::test_efi_capsule_fw4
>
> I should have mentioned this in my previous comment.
>
> My capsule tests assume that the capsule update does *not* initiate
> a reboot automatically and does "reboot" by "env print -e Capsule0000".

Hm, so this should be fixed by the test case.

>
> Furthermore, since the current sandbox_defconfig does not enable any
> U-Boot environment storage, "dfu_alt_info," for instance, cannot
> retain across the reboot (and so I didn't use CAPSULE_ON_DISK_EARLY).

OK, but would this be a matter? (I guess you can define dfu_alt_info and
update the firmware afterwards.)

>
> I will help Masami fix the issue.

Thank you!

>
> -Takahiro Akashi
>
>
>
>
>
> > Best regards
> >
> > Heinrich
> >
> >
> > Best regards
> >
> > Heinrich
Simon Glass Feb. 26, 2022, 6:37 p.m. UTC | #7 
Hi Masami,

On Thu, 3 Feb 2022 at 02:23, Masami Hiramatsu
<masami.hiramatsu@linaro.org> wrote:
>
> Add a cold reset 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 v4:
>   - Do not use sysreset because that is a warm reset.

I don't get that.

You should use sysreset and the SYSRESET_COLD type.

>   - Fix patch description.
> ---
>  lib/efi_loader/efi_capsule.c |   18 ++++++++++++++++--
>  1 file changed, 16 insertions(+), 2 deletions(-)
>
> diff --git a/lib/efi_loader/efi_capsule.c b/lib/efi_loader/efi_capsule.c
> index 1ec7ea29ff..20d9490dbd 100644
> --- a/lib/efi_loader/efi_capsule.c
> +++ b/lib/efi_loader/efi_capsule.c
> @@ -14,6 +14,7 @@
>  #include <env.h>
>  #include <fdtdec.h>
>  #include <fs.h>
> +#include <hang.h>
>  #include <malloc.h>
>  #include <mapmem.h>
>  #include <sort.h>
> @@ -1120,8 +1121,11 @@ efi_status_t efi_launch_capsules(void)
>                 if (ret == EFI_SUCCESS) {
>                         ret = efi_capsule_update_firmware(capsule);
>                         if (ret != EFI_SUCCESS)
> -                               log_err("Applying capsule %ls failed\n",
> +                               log_err("Applying capsule %ls failed.\n",
>                                         files[i]);
> +                       else
> +                               log_info("Applying capsule %ls succeeded.\n",
> +                                        files[i]);
>
>                         /* create CapsuleXXXX */
>                         set_capsule_result(index, capsule, ret);
> @@ -1142,6 +1146,16 @@ efi_status_t efi_launch_capsules(void)
>                 free(files[i]);
>         free(files);
>
> -       return ret;
> +       /*
> +        * UEFI spec requires to reset system after complete processing capsule
> +        * update on the storage.
> +        */
> +       log_info("Reboot after firmware update");
> +       /* Cold reset is required for loading the new firmware. */
> +       do_reset(NULL, 0, 0, NULL);

No! This should use sysreset.

> +       hang();
> +       /* not reach here */
> +
> +       return 0;
>  }
>  #endif /* CONFIG_EFI_CAPSULE_ON_DISK */
>

Regards,
Simon
Masami Hiramatsu Feb. 28, 2022, 4:19 a.m. UTC | #8 
Hi Simon,

2022年2月27日(日) 3:37 Simon Glass <sjg@chromium.org>:
>
> Hi Masami,
>
> On Thu, 3 Feb 2022 at 02:23, Masami Hiramatsu
> <masami.hiramatsu@linaro.org> wrote:
> >
> > Add a cold reset 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 v4:
> >   - Do not use sysreset because that is a warm reset.
>
> I don't get that.
>
> You should use sysreset and the SYSRESET_COLD type.

Thanks, yeah, I found that parameter.
Let's fix that.

Thank you,

>
> >   - Fix patch description.
> > ---
> >  lib/efi_loader/efi_capsule.c |   18 ++++++++++++++++--
> >  1 file changed, 16 insertions(+), 2 deletions(-)
> >
> > diff --git a/lib/efi_loader/efi_capsule.c b/lib/efi_loader/efi_capsule.c
> > index 1ec7ea29ff..20d9490dbd 100644
> > --- a/lib/efi_loader/efi_capsule.c
> > +++ b/lib/efi_loader/efi_capsule.c
> > @@ -14,6 +14,7 @@
> >  #include <env.h>
> >  #include <fdtdec.h>
> >  #include <fs.h>
> > +#include <hang.h>
> >  #include <malloc.h>
> >  #include <mapmem.h>
> >  #include <sort.h>
> > @@ -1120,8 +1121,11 @@ efi_status_t efi_launch_capsules(void)
> >                 if (ret == EFI_SUCCESS) {
> >                         ret = efi_capsule_update_firmware(capsule);
> >                         if (ret != EFI_SUCCESS)
> > -                               log_err("Applying capsule %ls failed\n",
> > +                               log_err("Applying capsule %ls failed.\n",
> >                                         files[i]);
> > +                       else
> > +                               log_info("Applying capsule %ls succeeded.\n",
> > +                                        files[i]);
> >
> >                         /* create CapsuleXXXX */
> >                         set_capsule_result(index, capsule, ret);
> > @@ -1142,6 +1146,16 @@ efi_status_t efi_launch_capsules(void)
> >                 free(files[i]);
> >         free(files);
> >
> > -       return ret;
> > +       /*
> > +        * UEFI spec requires to reset system after complete processing capsule
> > +        * update on the storage.
> > +        */
> > +       log_info("Reboot after firmware update");
> > +       /* Cold reset is required for loading the new firmware. */
> > +       do_reset(NULL, 0, 0, NULL);
>
> No! This should use sysreset.
>
> > +       hang();
> > +       /* not reach here */
> > +
> > +       return 0;
> >  }
> >  #endif /* CONFIG_EFI_CAPSULE_ON_DISK */
> >
>
> Regards,
> Simon
Masami Hiramatsu Feb. 28, 2022, 7:53 a.m. UTC | #9 
Hi Simon,

BTW, I saw the below code in the sysreset-uclass.c. It seems if I pass
0 to argc, it seems to do SYSRESET_COLD, isn't it?

int do_reset(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[])
{
        enum sysreset_t reset_type = SYSRESET_COLD;

        if (argc > 2)
                return CMD_RET_USAGE;

        if (argc == 2 && argv[1][0] == '-' && argv[1][1] == 'w') {
                reset_type = SYSRESET_WARM;
        }

        printf("resetting ...\n");
        mdelay(100);

        sysreset_walk_halt(reset_type);

        return 0;
}

2022年2月28日(月) 13:19 Masami Hiramatsu <masami.hiramatsu@linaro.org>:
>
> Hi Simon,
>
> 2022年2月27日(日) 3:37 Simon Glass <sjg@chromium.org>:
> >
> > Hi Masami,
> >
> > On Thu, 3 Feb 2022 at 02:23, Masami Hiramatsu
> > <masami.hiramatsu@linaro.org> wrote:
> > >
> > > Add a cold reset 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 v4:
> > >   - Do not use sysreset because that is a warm reset.
> >
> > I don't get that.
> >
> > You should use sysreset and the SYSRESET_COLD type.
>
> Thanks, yeah, I found that parameter.
> Let's fix that.
>
> Thank you,
>
> >
> > >   - Fix patch description.
> > > ---
> > >  lib/efi_loader/efi_capsule.c |   18 ++++++++++++++++--
> > >  1 file changed, 16 insertions(+), 2 deletions(-)
> > >
> > > diff --git a/lib/efi_loader/efi_capsule.c b/lib/efi_loader/efi_capsule.c
> > > index 1ec7ea29ff..20d9490dbd 100644
> > > --- a/lib/efi_loader/efi_capsule.c
> > > +++ b/lib/efi_loader/efi_capsule.c
> > > @@ -14,6 +14,7 @@
> > >  #include <env.h>
> > >  #include <fdtdec.h>
> > >  #include <fs.h>
> > > +#include <hang.h>
> > >  #include <malloc.h>
> > >  #include <mapmem.h>
> > >  #include <sort.h>
> > > @@ -1120,8 +1121,11 @@ efi_status_t efi_launch_capsules(void)
> > >                 if (ret == EFI_SUCCESS) {
> > >                         ret = efi_capsule_update_firmware(capsule);
> > >                         if (ret != EFI_SUCCESS)
> > > -                               log_err("Applying capsule %ls failed\n",
> > > +                               log_err("Applying capsule %ls failed.\n",
> > >                                         files[i]);
> > > +                       else
> > > +                               log_info("Applying capsule %ls succeeded.\n",
> > > +                                        files[i]);
> > >
> > >                         /* create CapsuleXXXX */
> > >                         set_capsule_result(index, capsule, ret);
> > > @@ -1142,6 +1146,16 @@ efi_status_t efi_launch_capsules(void)
> > >                 free(files[i]);
> > >         free(files);
> > >
> > > -       return ret;
> > > +       /*
> > > +        * UEFI spec requires to reset system after complete processing capsule
> > > +        * update on the storage.
> > > +        */
> > > +       log_info("Reboot after firmware update");
> > > +       /* Cold reset is required for loading the new firmware. */
> > > +       do_reset(NULL, 0, 0, NULL);
> >
> > No! This should use sysreset.
> >
> > > +       hang();
> > > +       /* not reach here */
> > > +
> > > +       return 0;
> > >  }
> > >  #endif /* CONFIG_EFI_CAPSULE_ON_DISK */
> > >
> >
> > Regards,
> > Simon
>
>
>
> --
> Masami Hiramatsu
Simon Glass March 1, 2022, 2:58 p.m. UTC | #10 
Hi Masami,

On Mon, 28 Feb 2022 at 00:53, Masami Hiramatsu
<masami.hiramatsu@linaro.org> wrote:
>
> Hi Simon,
>
> BTW, I saw the below code in the sysreset-uclass.c. It seems if I pass
> 0 to argc, it seems to do SYSRESET_COLD, isn't it?

Yes, but we should use the driver interface to do things, not the
command-line interface :-)


- Simon

>
> int do_reset(struct cmd_tbl *cmdtp, int flag, int argc, char *const argv[])
> {
>         enum sysreset_t reset_type = SYSRESET_COLD;
>
>         if (argc > 2)
>                 return CMD_RET_USAGE;
>
>         if (argc == 2 && argv[1][0] == '-' && argv[1][1] == 'w') {
>                 reset_type = SYSRESET_WARM;
>         }
>
>         printf("resetting ...\n");
>         mdelay(100);
>
>         sysreset_walk_halt(reset_type);
>
>         return 0;
> }
>
> 2022年2月28日(月) 13:19 Masami Hiramatsu <masami.hiramatsu@linaro.org>:
> >
> > Hi Simon,
> >
> > 2022年2月27日(日) 3:37 Simon Glass <sjg@chromium.org>:
> > >
> > > Hi Masami,
> > >
> > > On Thu, 3 Feb 2022 at 02:23, Masami Hiramatsu
> > > <masami.hiramatsu@linaro.org> wrote:
> > > >
> > > > Add a cold reset 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 v4:
> > > >   - Do not use sysreset because that is a warm reset.
> > >
> > > I don't get that.
> > >
> > > You should use sysreset and the SYSRESET_COLD type.
> >
> > Thanks, yeah, I found that parameter.
> > Let's fix that.
> >
> > Thank you,
> >
> > >
> > > >   - Fix patch description.
> > > > ---
> > > >  lib/efi_loader/efi_capsule.c |   18 ++++++++++++++++--
> > > >  1 file changed, 16 insertions(+), 2 deletions(-)
> > > >
> > > > diff --git a/lib/efi_loader/efi_capsule.c b/lib/efi_loader/efi_capsule.c
> > > > index 1ec7ea29ff..20d9490dbd 100644
> > > > --- a/lib/efi_loader/efi_capsule.c
> > > > +++ b/lib/efi_loader/efi_capsule.c
> > > > @@ -14,6 +14,7 @@
> > > >  #include <env.h>
> > > >  #include <fdtdec.h>
> > > >  #include <fs.h>
> > > > +#include <hang.h>
> > > >  #include <malloc.h>
> > > >  #include <mapmem.h>
> > > >  #include <sort.h>
> > > > @@ -1120,8 +1121,11 @@ efi_status_t efi_launch_capsules(void)
> > > >                 if (ret == EFI_SUCCESS) {
> > > >                         ret = efi_capsule_update_firmware(capsule);
> > > >                         if (ret != EFI_SUCCESS)
> > > > -                               log_err("Applying capsule %ls failed\n",
> > > > +                               log_err("Applying capsule %ls failed.\n",
> > > >                                         files[i]);
> > > > +                       else
> > > > +                               log_info("Applying capsule %ls succeeded.\n",
> > > > +                                        files[i]);
> > > >
> > > >                         /* create CapsuleXXXX */
> > > >                         set_capsule_result(index, capsule, ret);
> > > > @@ -1142,6 +1146,16 @@ efi_status_t efi_launch_capsules(void)
> > > >                 free(files[i]);
> > > >         free(files);
> > > >
> > > > -       return ret;
> > > > +       /*
> > > > +        * UEFI spec requires to reset system after complete processing capsule
> > > > +        * update on the storage.
> > > > +        */
> > > > +       log_info("Reboot after firmware update");
> > > > +       /* Cold reset is required for loading the new firmware. */
> > > > +       do_reset(NULL, 0, 0, NULL);
> > >
> > > No! This should use sysreset.
> > >
> > > > +       hang();
> > > > +       /* not reach here */
> > > > +
> > > > +       return 0;
> > > >  }
> > > >  #endif /* CONFIG_EFI_CAPSULE_ON_DISK */
> > > >
> > >
> > > Regards,
> > > Simon
> >
> >
> >
> > --
> > Masami Hiramatsu
>
>
>
> --
> Masami Hiramatsu
Masami Hiramatsu March 2, 2022, 1:58 a.m. UTC | #11 
Hi Simon,

2022年3月1日(火) 23:58 Simon Glass <sjg@chromium.org>:
>
> Hi Masami,
>
> On Mon, 28 Feb 2022 at 00:53, Masami Hiramatsu
> <masami.hiramatsu@linaro.org> wrote:
> >
> > Hi Simon,
> >
> > BTW, I saw the below code in the sysreset-uclass.c. It seems if I pass
> > 0 to argc, it seems to do SYSRESET_COLD, isn't it?
>
> Yes, but we should use the driver interface to do things, not the
> command-line interface :-)

OK, I got it. So something like this attached patch?
(This is for the next branch)

Thank you,
diff mbox series

Patch

diff --git a/lib/efi_loader/efi_capsule.c b/lib/efi_loader/efi_capsule.c
index 1ec7ea29ff..20d9490dbd 100644
--- a/lib/efi_loader/efi_capsule.c
+++ b/lib/efi_loader/efi_capsule.c
@@ -14,6 +14,7 @@ 
 #include <env.h>
 #include <fdtdec.h>
 #include <fs.h>
+#include <hang.h>
 #include <malloc.h>
 #include <mapmem.h>
 #include <sort.h>
@@ -1120,8 +1121,11 @@  efi_status_t efi_launch_capsules(void)
 		if (ret == EFI_SUCCESS) {
 			ret = efi_capsule_update_firmware(capsule);
 			if (ret != EFI_SUCCESS)
-				log_err("Applying capsule %ls failed\n",
+				log_err("Applying capsule %ls failed.\n",
 					files[i]);
+			else
+				log_info("Applying capsule %ls succeeded.\n",
+					 files[i]);
 
 			/* create CapsuleXXXX */
 			set_capsule_result(index, capsule, ret);
@@ -1142,6 +1146,16 @@  efi_status_t efi_launch_capsules(void)
 		free(files[i]);
 	free(files);
 
-	return ret;
+	/*
+	 * UEFI spec requires to reset system after complete processing capsule
+	 * update on the storage.
+	 */
+	log_info("Reboot after firmware update");
+	/* Cold reset is required for loading the new firmware. */
+	do_reset(NULL, 0, 0, NULL);
+	hang();
+	/* not reach here */
+
+	return 0;
 }
 #endif /* CONFIG_EFI_CAPSULE_ON_DISK */