diff mbox series

test/py: efi_capsule: Handle expected reset after capsule on disk

Message ID 164491595065.536855.9457820061065514578.stgit@localhost
State New
Headers show
Series test/py: efi_capsule: Handle expected reset after capsule on disk | expand

Commit Message

Masami Hiramatsu Feb. 15, 2022, 9:05 a.m. UTC
Since now the capsule_on_disk will restart the u-boot sandbox right
after the capsule update, if CONFIG_EFI_CAPSULE_ON_DISK_EARLY=y, the
boot with a new capsule file will repeat reboot sequence. On the
other hand, if CONFIG_EFI_CAPSULE_ON_DISK_EARLY=n, the 'env print -e'
command will execute the capsule update on disk and reboot.

Thus this update the uboot_console for those 2 cases;

 - restart_uboot(): Add expect_earlyreset optional parameter so that
   it can handle the reboot while booting.
 - run_command(): Add wait_for_reboot optional parameter so that it
   can handle the reboot after executing a command.

And enable those options in the test_capsule_firmware.py test cases.

Signed-off-by: Masami Hiramatsu <masami.hiramatsu@linaro.org>
---
 .../test_efi_capsule/test_capsule_firmware.py      |   39 ++++++--
 test/py/u_boot_console_base.py                     |   95 +++++++++++++++-----
 test/py/u_boot_console_sandbox.py                  |    6 +
 3 files changed, 102 insertions(+), 38 deletions(-)

Comments

Masami Hiramatsu Feb. 15, 2022, 9:09 a.m. UTC | #1
Hi Heinrich,

I and Takahiro found that the capsule update test case and test framework
didn't expect that a command can reboot the sandbox and the sandbox can
reboot while booting, which happens when my "Reset system after
CapsuleUpdate on disk" patch applied.
This patch fixes those issues.

Thank you,


2022年2月15日(火) 18:05 Masami Hiramatsu <masami.hiramatsu@linaro.org>:

> Since now the capsule_on_disk will restart the u-boot sandbox right
> after the capsule update, if CONFIG_EFI_CAPSULE_ON_DISK_EARLY=y, the
> boot with a new capsule file will repeat reboot sequence. On the
> other hand, if CONFIG_EFI_CAPSULE_ON_DISK_EARLY=n, the 'env print -e'
> command will execute the capsule update on disk and reboot.
>
> Thus this update the uboot_console for those 2 cases;
>
>  - restart_uboot(): Add expect_earlyreset optional parameter so that
>    it can handle the reboot while booting.
>  - run_command(): Add wait_for_reboot optional parameter so that it
>    can handle the reboot after executing a command.
>
> And enable those options in the test_capsule_firmware.py test cases.
>
> Signed-off-by: Masami Hiramatsu <masami.hiramatsu@linaro.org>
> ---
>  .../test_efi_capsule/test_capsule_firmware.py      |   39 ++++++--
>  test/py/u_boot_console_base.py                     |   95
> +++++++++++++++-----
>  test/py/u_boot_console_sandbox.py                  |    6 +
>  3 files changed, 102 insertions(+), 38 deletions(-)
>
> diff --git a/test/py/tests/test_efi_capsule/test_capsule_firmware.py
> b/test/py/tests/test_efi_capsule/test_capsule_firmware.py
> index 6e803f699f..a539134ec2 100644
> --- a/test/py/tests/test_efi_capsule/test_capsule_firmware.py
> +++ b/test/py/tests/test_efi_capsule/test_capsule_firmware.py
> @@ -143,13 +143,14 @@ class TestEfiCapsuleFirmwareFit(object):
>                  'fatls host 0:1 %s' % CAPSULE_INSTALL_DIR])
>              assert 'Test01' in ''.join(output)
>
> -        # reboot
> -        u_boot_console.restart_uboot()
> -
>          capsule_early = u_boot_config.buildconfig.get(
>              'config_efi_capsule_on_disk_early')
>          capsule_auth = u_boot_config.buildconfig.get(
>              'config_efi_capsule_authenticate')
> +
> +        # reboot
> +        u_boot_console.restart_uboot(expect_earlyreset = capsule_early)
> +
>          with u_boot_console.log.section('Test Case 2-b, after reboot'):
>              if not capsule_early:
>                  # make sure that dfu_alt_info exists even persistent
> variables
> @@ -162,7 +163,7 @@ class TestEfiCapsuleFirmwareFit(object):
>
>                  # need to run uefi command to initiate capsule handling
>                  output = u_boot_console.run_command(
> -                    'env print -e Capsule0000')
> +                    'env print -e Capsule0000', wait_for_reboot = True)
>
>              output = u_boot_console.run_command_list([
>                  'host bind 0 %s' % disk_img,
> @@ -218,13 +219,14 @@ class TestEfiCapsuleFirmwareFit(object):
>                  'fatls host 0:1 %s' % CAPSULE_INSTALL_DIR])
>              assert 'Test02' in ''.join(output)
>
> -        # reboot
> -        u_boot_console.restart_uboot()
> -
>          capsule_early = u_boot_config.buildconfig.get(
>              'config_efi_capsule_on_disk_early')
>          capsule_auth = u_boot_config.buildconfig.get(
>              'config_efi_capsule_authenticate')
> +
> +        # reboot
> +        u_boot_console.restart_uboot(expect_earlyreset = capsule_early)
> +
>          with u_boot_console.log.section('Test Case 3-b, after reboot'):
>              if not capsule_early:
>                  # make sure that dfu_alt_info exists even persistent
> variables
> @@ -237,7 +239,13 @@ class TestEfiCapsuleFirmwareFit(object):
>
>                  # need to run uefi command to initiate capsule handling
>                  output = u_boot_console.run_command(
> -                    'env print -e Capsule0000')
> +                    'env print -e Capsule0000', wait_for_reboot = True)
> +
> +            output = u_boot_console.run_command_list([
> +                'env set dfu_alt_info "sf 0:0=u-boot-bin raw 0x100000
> 0x50000;u-boot-env raw 0x150000 0x200000"',
> +                'host bind 0 %s' % disk_img,
> +                'fatls host 0:1 %s' % CAPSULE_INSTALL_DIR])
> +            assert 'Test02' not in ''.join(output)
>
>              output = u_boot_console.run_command_list(['efidebug capsule
> esrt'])
>
> @@ -293,13 +301,14 @@ class TestEfiCapsuleFirmwareFit(object):
>                  'fatls host 0:1 %s' % CAPSULE_INSTALL_DIR])
>              assert 'Test03' in ''.join(output)
>
> -        # reboot
> -        u_boot_console.restart_uboot()
> -
>          capsule_early = u_boot_config.buildconfig.get(
>              'config_efi_capsule_on_disk_early')
>          capsule_auth = u_boot_config.buildconfig.get(
>              'config_efi_capsule_authenticate')
> +
> +        # reboot
> +        u_boot_console.restart_uboot(expect_earlyreset = capsule_early)
> +
>          with u_boot_console.log.section('Test Case 4-b, after reboot'):
>              if not capsule_early:
>                  # make sure that dfu_alt_info exists even persistent
> variables
> @@ -312,7 +321,13 @@ class TestEfiCapsuleFirmwareFit(object):
>
>                  # need to run uefi command to initiate capsule handling
>                  output = u_boot_console.run_command(
> -                    'env print -e Capsule0000')
> +                    'env print -e Capsule0000', wait_for_reboot = True)
> +
> +            output = u_boot_console.run_command_list([
> +                'env set dfu_alt_info "sf 0:0=u-boot-bin raw 0x100000
> 0x50000;u-boot-env raw 0x150000 0x200000"',
> +                'host bind 0 %s' % disk_img,
> +                'fatls host 0:1 %s' % CAPSULE_INSTALL_DIR])
> +            assert 'Test03' not in ''.join(output)
>
>              output = u_boot_console.run_command_list(['efidebug capsule
> esrt'])
>
> diff --git a/test/py/u_boot_console_base.py
> b/test/py/u_boot_console_base.py
> index 384fd53c65..e84f4d74ef 100644
> --- a/test/py/u_boot_console_base.py
> +++ b/test/py/u_boot_console_base.py
> @@ -140,7 +140,7 @@ class ConsoleBase(object):
>          self.logstream.close()
>
>      def run_command(self, cmd, wait_for_echo=True, send_nl=True,
> -            wait_for_prompt=True):
> +            wait_for_prompt=True, wait_for_reboot=False):
>          """Execute a command via the U-Boot console.
>
>          The command is always sent to U-Boot.
> @@ -168,6 +168,8 @@ class ConsoleBase(object):
>              wait_for_prompt: Boolean indicating whether to wait for the
>                  command prompt to be sent by U-Boot. This typically occurs
>                  immediately after the command has been executed.
> +            wait_for_reboot: Boolean indication whether to wait for the
> +                reboot U-Boot. If this is True, wait_for_prompt is
> ignored.
>
>          Returns:
>              If wait_for_prompt == False:
> @@ -202,11 +204,48 @@ class ConsoleBase(object):
>                                      self.bad_pattern_ids[m - 1])
>              if not wait_for_prompt:
>                  return
> -            m = self.p.expect([self.prompt_compiled] + self.bad_patterns)
> -            if m != 0:
> -                self.at_prompt = False
> -                raise Exception('Bad pattern found on console: ' +
> -                                self.bad_pattern_ids[m - 1])
> +            if wait_for_reboot:
> +                bcfg = self.config.buildconfig
> +                config_spl = bcfg.get('config_spl', 'n') == 'y'
> +                config_spl_serial = bcfg.get('config_spl_serial',
> +                                                 'n') == 'y'
> +                env_spl_skipped = self.config.env.get('env__spl_skipped',
> +                                                      False)
> +                env_spl2_skipped =
> self.config.env.get('env__spl2_skipped',
> +                                                       True)
> +                if config_spl and config_spl_serial and not
> env_spl_skipped:
> +                    m = self.p.expect([pattern_u_boot_spl_signon] +
> +                                      self.bad_patterns)
> +                    if m != 0:
> +                        raise Exception('Bad pattern found on SPL
> console: ' +
> +                                        self.bad_pattern_ids[m - 1])
> +                if not env_spl2_skipped:
> +                    m = self.p.expect([pattern_u_boot_spl2_signon] +
> +                                      self.bad_patterns)
> +                    if m != 0:
> +                        raise Exception('Bad pattern found on SPL2
> console: ' +
> +                                        self.bad_pattern_ids[m - 1])
> +                m = self.p.expect([pattern_u_boot_main_signon] +
> self.bad_patterns)
> +                if m != 0:
> +                    raise Exception('Bad pattern found on console: ' +
> +                                    self.bad_pattern_ids[m - 1])
> +                self.u_boot_version_string = self.p.after
> +                while True:
> +                    m = self.p.expect([self.prompt_compiled,
> +                        pattern_stop_autoboot_prompt] + self.bad_patterns)
> +                    if m == 0:
> +                        break
> +                    if m == 1:
> +                        self.p.send(' ')
> +                        continue
> +                    raise Exception('Bad pattern found on console: ' +
> +                                    self.bad_pattern_ids[m - 2])
> +            else:
> +                m = self.p.expect([self.prompt_compiled] +
> self.bad_patterns)
> +                if m != 0:
> +                    self.at_prompt = False
> +                    raise Exception('Bad pattern found on console: ' +
> +                                    self.bad_pattern_ids[m - 1])
>              self.at_prompt = True
>              self.at_prompt_logevt = self.logstream.logfile.cur_evt
>              # Only strip \r\n; space/TAB might be significant if testing
> @@ -321,7 +360,7 @@ class ConsoleBase(object):
>          finally:
>              self.p.timeout = orig_timeout
>
> -    def ensure_spawned(self):
> +    def ensure_spawned(self, expect_earlyreset=False):
>          """Ensure a connection to a correctly running U-Boot instance.
>
>          This may require spawning a new Sandbox process or resetting
> target
> @@ -330,7 +369,8 @@ class ConsoleBase(object):
>          This is an internal function and should not be called directly.
>
>          Args:
> -            None.
> +            expect_earlyreset: This boot is expected to be reset in early
> +                stage (before prompt). False by default.
>
>          Returns:
>              Nothing.
> @@ -357,22 +397,29 @@ class ConsoleBase(object):
>                                                    False)
>              env_spl2_skipped = self.config.env.get('env__spl2_skipped',
>                                                    True)
> -            if config_spl and config_spl_serial and not env_spl_skipped:
> -                m = self.p.expect([pattern_u_boot_spl_signon] +
> -                                  self.bad_patterns)
> +            if expect_earlyreset:
> +                loop_num = 2
> +            else:
> +                loop_num = 1
> +
> +            while loop_num > 0:
> +                loop_num -= 1
> +                if config_spl and config_spl_serial and not
> env_spl_skipped:
> +                    m = self.p.expect([pattern_u_boot_spl_signon] +
> +                                      self.bad_patterns)
> +                    if m != 0:
> +                        raise Exception('Bad pattern found on SPL
> console: ' +
> +                                        self.bad_pattern_ids[m - 1])
> +                if not env_spl2_skipped:
> +                    m = self.p.expect([pattern_u_boot_spl2_signon] +
> +                                      self.bad_patterns)
> +                    if m != 0:
> +                        raise Exception('Bad pattern found on SPL2
> console: ' +
> +                                        self.bad_pattern_ids[m - 1])
> +                m = self.p.expect([pattern_u_boot_main_signon] +
> self.bad_patterns)
>                  if m != 0:
> -                    raise Exception('Bad pattern found on SPL console: ' +
> -                                    self.bad_pattern_ids[m - 1])
> -            if not env_spl2_skipped:
> -                m = self.p.expect([pattern_u_boot_spl2_signon] +
> -                                  self.bad_patterns)
> -                if m != 0:
> -                    raise Exception('Bad pattern found on SPL2 console: '
> +
> +                    raise Exception('Bad pattern found on console: ' +
>                                      self.bad_pattern_ids[m - 1])
> -            m = self.p.expect([pattern_u_boot_main_signon] +
> self.bad_patterns)
> -            if m != 0:
> -                raise Exception('Bad pattern found on console: ' +
> -                                self.bad_pattern_ids[m - 1])
>              self.u_boot_version_string = self.p.after
>              while True:
>                  m = self.p.expect([self.prompt_compiled,
> @@ -416,10 +463,10 @@ class ConsoleBase(object):
>              pass
>          self.p = None
>
> -    def restart_uboot(self):
> +    def restart_uboot(self, expect_earlyreset=False):
>          """Shut down and restart U-Boot."""
>          self.cleanup_spawn()
> -        self.ensure_spawned()
> +        self.ensure_spawned(expect_earlyreset)
>
>      def get_spawn_output(self):
>          """Return the start-up output from U-Boot
> diff --git a/test/py/u_boot_console_sandbox.py
> b/test/py/u_boot_console_sandbox.py
> index 7e1eb0e0b4..9cd9ccad30 100644
> --- a/test/py/u_boot_console_sandbox.py
> +++ b/test/py/u_boot_console_sandbox.py
> @@ -57,11 +57,13 @@ class ConsoleSandbox(ConsoleBase):
>          cmd += self.sandbox_flags
>          return Spawn(cmd, cwd=self.config.source_dir)
>
> -    def restart_uboot_with_flags(self, flags):
> +    def restart_uboot_with_flags(self, flags, expect_earlyreset=False):
>          """Run U-Boot with the given command-line flags
>
>          Args:
>              flags: List of flags to pass, each a string
> +            expect_earlyreset: This boot is expected to be reset in early
> +                stage (before prompt). False by default.
>
>          Returns:
>              A u_boot_spawn.Spawn object that is attached to U-Boot.
> @@ -69,7 +71,7 @@ class ConsoleSandbox(ConsoleBase):
>
>          try:
>              self.sandbox_flags = flags
> -            return self.restart_uboot()
> +            return self.restart_uboot(expect_earlyreset)
>          finally:
>              self.sandbox_flags = []
>
>
>
Heinrich Schuchardt Feb. 15, 2022, 1:51 p.m. UTC | #2
On 2/15/22 10:09, Masami Hiramatsu wrote:
> Hi Heinrich,
> 
> I and Takahiro found that the capsule update test case and test 
> framework didn't expect that a command can reboot the sandbox and the 
> sandbox can reboot while booting, which happens when my "Reset system 
> after CapsuleUpdate on disk" patch applied.
> This patch fixes those issues.

Gitlab CI testing does not pass. So this series has to be reworked.

Best regards

Heinrich

> 
> Thank you,
> 
> 
> 2022年2月15日(火) 18:05 Masami Hiramatsu <masami.hiramatsu@linaro.org 
> <mailto:masami.hiramatsu@linaro.org>>:
> 
>     Since now the capsule_on_disk will restart the u-boot sandbox right
>     after the capsule update, if CONFIG_EFI_CAPSULE_ON_DISK_EARLY=y, the
>     boot with a new capsule file will repeat reboot sequence. On the
>     other hand, if CONFIG_EFI_CAPSULE_ON_DISK_EARLY=n, the 'env print -e'
>     command will execute the capsule update on disk and reboot.
> 
>     Thus this update the uboot_console for those 2 cases;
> 
>       - restart_uboot(): Add expect_earlyreset optional parameter so that
>         it can handle the reboot while booting.
>       - run_command(): Add wait_for_reboot optional parameter so that it
>         can handle the reboot after executing a command.
> 
>     And enable those options in the test_capsule_firmware.py test cases.
> 
>     Signed-off-by: Masami Hiramatsu <masami.hiramatsu@linaro.org
>     <mailto:masami.hiramatsu@linaro.org>>
>     ---
>       .../test_efi_capsule/test_capsule_firmware.py      |   39 ++++++--
>       test/py/u_boot_console_base.py                     |   95
>     +++++++++++++++-----
>       test/py/u_boot_console_sandbox.py                  |    6 +
>       3 files changed, 102 insertions(+), 38 deletions(-)
> 
>     diff --git a/test/py/tests/test_efi_capsule/test_capsule_firmware.py
>     b/test/py/tests/test_efi_capsule/test_capsule_firmware.py
>     index 6e803f699f..a539134ec2 100644
>     --- a/test/py/tests/test_efi_capsule/test_capsule_firmware.py
>     +++ b/test/py/tests/test_efi_capsule/test_capsule_firmware.py
>     @@ -143,13 +143,14 @@ class TestEfiCapsuleFirmwareFit(object):
>                       'fatls host 0:1 %s' % CAPSULE_INSTALL_DIR])
>                   assert 'Test01' in ''.join(output)
> 
>     -        # reboot
>     -        u_boot_console.restart_uboot()
>     -
>               capsule_early = u_boot_config.buildconfig.get(
>                   'config_efi_capsule_on_disk_early')
>               capsule_auth = u_boot_config.buildconfig.get(
>                   'config_efi_capsule_authenticate')
>     +
>     +        # reboot
>     +        u_boot_console.restart_uboot(expect_earlyreset = capsule_early)
>     +
>               with u_boot_console.log.section('Test Case 2-b, after
>     reboot'):
>                   if not capsule_early:
>                       # make sure that dfu_alt_info exists even
>     persistent variables
>     @@ -162,7 +163,7 @@ class TestEfiCapsuleFirmwareFit(object):
> 
>                       # need to run uefi command to initiate capsule
>     handling
>                       output = u_boot_console.run_command(
>     -                    'env print -e Capsule0000')
>     +                    'env print -e Capsule0000', wait_for_reboot = True)
> 
>                   output = u_boot_console.run_command_list([
>                       'host bind 0 %s' % disk_img,
>     @@ -218,13 +219,14 @@ class TestEfiCapsuleFirmwareFit(object):
>                       'fatls host 0:1 %s' % CAPSULE_INSTALL_DIR])
>                   assert 'Test02' in ''.join(output)
> 
>     -        # reboot
>     -        u_boot_console.restart_uboot()
>     -
>               capsule_early = u_boot_config.buildconfig.get(
>                   'config_efi_capsule_on_disk_early')
>               capsule_auth = u_boot_config.buildconfig.get(
>                   'config_efi_capsule_authenticate')
>     +
>     +        # reboot
>     +        u_boot_console.restart_uboot(expect_earlyreset = capsule_early)
>     +
>               with u_boot_console.log.section('Test Case 3-b, after
>     reboot'):
>                   if not capsule_early:
>                       # make sure that dfu_alt_info exists even
>     persistent variables
>     @@ -237,7 +239,13 @@ class TestEfiCapsuleFirmwareFit(object):
> 
>                       # need to run uefi command to initiate capsule
>     handling
>                       output = u_boot_console.run_command(
>     -                    'env print -e Capsule0000')
>     +                    'env print -e Capsule0000', wait_for_reboot = True)
>     +
>     +            output = u_boot_console.run_command_list([
>     +                'env set dfu_alt_info "sf 0:0=u-boot-bin raw
>     0x100000 0x50000;u-boot-env raw 0x150000 0x200000"',
>     +                'host bind 0 %s' % disk_img,
>     +                'fatls host 0:1 %s' % CAPSULE_INSTALL_DIR])
>     +            assert 'Test02' not in ''.join(output)
> 
>                   output = u_boot_console.run_command_list(['efidebug
>     capsule esrt'])
> 
>     @@ -293,13 +301,14 @@ class TestEfiCapsuleFirmwareFit(object):
>                       'fatls host 0:1 %s' % CAPSULE_INSTALL_DIR])
>                   assert 'Test03' in ''.join(output)
> 
>     -        # reboot
>     -        u_boot_console.restart_uboot()
>     -
>               capsule_early = u_boot_config.buildconfig.get(
>                   'config_efi_capsule_on_disk_early')
>               capsule_auth = u_boot_config.buildconfig.get(
>                   'config_efi_capsule_authenticate')
>     +
>     +        # reboot
>     +        u_boot_console.restart_uboot(expect_earlyreset = capsule_early)
>     +
>               with u_boot_console.log.section('Test Case 4-b, after
>     reboot'):
>                   if not capsule_early:
>                       # make sure that dfu_alt_info exists even
>     persistent variables
>     @@ -312,7 +321,13 @@ class TestEfiCapsuleFirmwareFit(object):
> 
>                       # need to run uefi command to initiate capsule
>     handling
>                       output = u_boot_console.run_command(
>     -                    'env print -e Capsule0000')
>     +                    'env print -e Capsule0000', wait_for_reboot = True)
>     +
>     +            output = u_boot_console.run_command_list([
>     +                'env set dfu_alt_info "sf 0:0=u-boot-bin raw
>     0x100000 0x50000;u-boot-env raw 0x150000 0x200000"',
>     +                'host bind 0 %s' % disk_img,
>     +                'fatls host 0:1 %s' % CAPSULE_INSTALL_DIR])
>     +            assert 'Test03' not in ''.join(output)
> 
>                   output = u_boot_console.run_command_list(['efidebug
>     capsule esrt'])
> 
>     diff --git a/test/py/u_boot_console_base.py
>     b/test/py/u_boot_console_base.py
>     index 384fd53c65..e84f4d74ef 100644
>     --- a/test/py/u_boot_console_base.py
>     +++ b/test/py/u_boot_console_base.py
>     @@ -140,7 +140,7 @@ class ConsoleBase(object):
>               self.logstream.close()
> 
>           def run_command(self, cmd, wait_for_echo=True, send_nl=True,
>     -            wait_for_prompt=True):
>     +            wait_for_prompt=True, wait_for_reboot=False):
>               """Execute a command via the U-Boot console.
> 
>               The command is always sent to U-Boot.
>     @@ -168,6 +168,8 @@ class ConsoleBase(object):
>                   wait_for_prompt: Boolean indicating whether to wait
>     for the
>                       command prompt to be sent by U-Boot. This
>     typically occurs
>                       immediately after the command has been executed.
>     +            wait_for_reboot: Boolean indication whether to wait for the
>     +                reboot U-Boot. If this is True, wait_for_prompt is
>     ignored.
> 
>               Returns:
>                   If wait_for_prompt == False:
>     @@ -202,11 +204,48 @@ class ConsoleBase(object):
>                                           self.bad_pattern_ids[m - 1])
>                   if not wait_for_prompt:
>                       return
>     -            m = self.p.expect([self.prompt_compiled] +
>     self.bad_patterns)
>     -            if m != 0:
>     -                self.at_prompt = False
>     -                raise Exception('Bad pattern found on console: ' +
>     -                                self.bad_pattern_ids[m - 1])
>     +            if wait_for_reboot:
>     +                bcfg = self.config.buildconfig
>     +                config_spl = bcfg.get('config_spl', 'n') == 'y'
>     +                config_spl_serial = bcfg.get('config_spl_serial',
>     +                                                 'n') == 'y'
>     +                env_spl_skipped =
>     self.config.env.get('env__spl_skipped',
>     +                                                      False)
>     +                env_spl2_skipped =
>     self.config.env.get('env__spl2_skipped',
>     +                                                       True)
>     +                if config_spl and config_spl_serial and not
>     env_spl_skipped:
>     +                    m = self.p.expect([pattern_u_boot_spl_signon] +
>     +                                      self.bad_patterns)
>     +                    if m != 0:
>     +                        raise Exception('Bad pattern found on SPL
>     console: ' +
>     +                                        self.bad_pattern_ids[m - 1])
>     +                if not env_spl2_skipped:
>     +                    m = self.p.expect([pattern_u_boot_spl2_signon] +
>     +                                      self.bad_patterns)
>     +                    if m != 0:
>     +                        raise Exception('Bad pattern found on SPL2
>     console: ' +
>     +                                        self.bad_pattern_ids[m - 1])
>     +                m = self.p.expect([pattern_u_boot_main_signon] +
>     self.bad_patterns)
>     +                if m != 0:
>     +                    raise Exception('Bad pattern found on console: ' +
>     +                                    self.bad_pattern_ids[m - 1])
>     +                self.u_boot_version_string = self.p.after
>     +                while True:
>     +                    m = self.p.expect([self.prompt_compiled,
>     +                        pattern_stop_autoboot_prompt] +
>     self.bad_patterns)
>     +                    if m == 0:
>     +                        break
>     +                    if m == 1:
>     +                        self.p.send(' ')
>     +                        continue
>     +                    raise Exception('Bad pattern found on console: ' +
>     +                                    self.bad_pattern_ids[m - 2])
>     +            else:
>     +                m = self.p.expect([self.prompt_compiled] +
>     self.bad_patterns)
>     +                if m != 0:
>     +                    self.at_prompt = False
>     +                    raise Exception('Bad pattern found on console: ' +
>     +                                    self.bad_pattern_ids[m - 1])
>                   self.at_prompt = True
>                   self.at_prompt_logevt = self.logstream.logfile.cur_evt
>                   # Only strip \r\n; space/TAB might be significant if
>     testing
>     @@ -321,7 +360,7 @@ class ConsoleBase(object):
>               finally:
>                   self.p.timeout = orig_timeout
> 
>     -    def ensure_spawned(self):
>     +    def ensure_spawned(self, expect_earlyreset=False):
>               """Ensure a connection to a correctly running U-Boot instance.
> 
>               This may require spawning a new Sandbox process or
>     resetting target
>     @@ -330,7 +369,8 @@ class ConsoleBase(object):
>               This is an internal function and should not be called
>     directly.
> 
>               Args:
>     -            None.
>     +            expect_earlyreset: This boot is expected to be reset in
>     early
>     +                stage (before prompt). False by default.
> 
>               Returns:
>                   Nothing.
>     @@ -357,22 +397,29 @@ class ConsoleBase(object):
>                                                         False)
>                   env_spl2_skipped =
>     self.config.env.get('env__spl2_skipped',
>                                                         True)
>     -            if config_spl and config_spl_serial and not
>     env_spl_skipped:
>     -                m = self.p.expect([pattern_u_boot_spl_signon] +
>     -                                  self.bad_patterns)
>     +            if expect_earlyreset:
>     +                loop_num = 2
>     +            else:
>     +                loop_num = 1
>     +
>     +            while loop_num > 0:
>     +                loop_num -= 1
>     +                if config_spl and config_spl_serial and not
>     env_spl_skipped:
>     +                    m = self.p.expect([pattern_u_boot_spl_signon] +
>     +                                      self.bad_patterns)
>     +                    if m != 0:
>     +                        raise Exception('Bad pattern found on SPL
>     console: ' +
>     +                                        self.bad_pattern_ids[m - 1])
>     +                if not env_spl2_skipped:
>     +                    m = self.p.expect([pattern_u_boot_spl2_signon] +
>     +                                      self.bad_patterns)
>     +                    if m != 0:
>     +                        raise Exception('Bad pattern found on SPL2
>     console: ' +
>     +                                        self.bad_pattern_ids[m - 1])
>     +                m = self.p.expect([pattern_u_boot_main_signon] +
>     self.bad_patterns)
>                       if m != 0:
>     -                    raise Exception('Bad pattern found on SPL
>     console: ' +
>     -                                    self.bad_pattern_ids[m - 1])
>     -            if not env_spl2_skipped:
>     -                m = self.p.expect([pattern_u_boot_spl2_signon] +
>     -                                  self.bad_patterns)
>     -                if m != 0:
>     -                    raise Exception('Bad pattern found on SPL2
>     console: ' +
>     +                    raise Exception('Bad pattern found on console: ' +
>                                           self.bad_pattern_ids[m - 1])
>     -            m = self.p.expect([pattern_u_boot_main_signon] +
>     self.bad_patterns)
>     -            if m != 0:
>     -                raise Exception('Bad pattern found on console: ' +
>     -                                self.bad_pattern_ids[m - 1])
>                   self.u_boot_version_string = self.p.after
>                   while True:
>                       m = self.p.expect([self.prompt_compiled,
>     @@ -416,10 +463,10 @@ class ConsoleBase(object):
>                   pass
>               self.p = None
> 
>     -    def restart_uboot(self):
>     +    def restart_uboot(self, expect_earlyreset=False):
>               """Shut down and restart U-Boot."""
>               self.cleanup_spawn()
>     -        self.ensure_spawned()
>     +        self.ensure_spawned(expect_earlyreset)
> 
>           def get_spawn_output(self):
>               """Return the start-up output from U-Boot
>     diff --git a/test/py/u_boot_console_sandbox.py
>     b/test/py/u_boot_console_sandbox.py
>     index 7e1eb0e0b4..9cd9ccad30 100644
>     --- a/test/py/u_boot_console_sandbox.py
>     +++ b/test/py/u_boot_console_sandbox.py
>     @@ -57,11 +57,13 @@ class ConsoleSandbox(ConsoleBase):
>               cmd += self.sandbox_flags
>               return Spawn(cmd, cwd=self.config.source_dir)
> 
>     -    def restart_uboot_with_flags(self, flags):
>     +    def restart_uboot_with_flags(self, flags, expect_earlyreset=False):
>               """Run U-Boot with the given command-line flags
> 
>               Args:
>                   flags: List of flags to pass, each a string
>     +            expect_earlyreset: This boot is expected to be reset in
>     early
>     +                stage (before prompt). False by default.
> 
>               Returns:
>                   A u_boot_spawn.Spawn object that is attached to U-Boot.
>     @@ -69,7 +71,7 @@ class ConsoleSandbox(ConsoleBase):
> 
>               try:
>                   self.sandbox_flags = flags
>     -            return self.restart_uboot()
>     +            return self.restart_uboot(expect_earlyreset)
>               finally:
>                   self.sandbox_flags = []
> 
> 
> 
> 
> -- 
> Masami Hiramatsu
Heinrich Schuchardt Feb. 15, 2022, 1:52 p.m. UTC | #3
On 2/15/22 14:51, Heinrich Schuchardt wrote:
> On 2/15/22 10:09, Masami Hiramatsu wrote:
>> Hi Heinrich,
>>
>> I and Takahiro found that the capsule update test case and test 
>> framework didn't expect that a command can reboot the sandbox and the 
>> sandbox can reboot while booting, which happens when my "Reset system 
>> after CapsuleUpdate on disk" patch applied.
>> This patch fixes those issues.
> 
> Gitlab CI testing does not pass. So this series has to be reworked.

Sorry I missed that you put a new patch into the old series. I will retest.

Best regards

Heinrich

> 
> Best regards
> 
> Heinrich
> 
>>
>> Thank you,
>>
>>
>> 2022年2月15日(火) 18:05 Masami Hiramatsu <masami.hiramatsu@linaro.org 
>> <mailto:masami.hiramatsu@linaro.org>>:
>>
>>     Since now the capsule_on_disk will restart the u-boot sandbox right
>>     after the capsule update, if CONFIG_EFI_CAPSULE_ON_DISK_EARLY=y, the
>>     boot with a new capsule file will repeat reboot sequence. On the
>>     other hand, if CONFIG_EFI_CAPSULE_ON_DISK_EARLY=n, the 'env print -e'
>>     command will execute the capsule update on disk and reboot.
>>
>>     Thus this update the uboot_console for those 2 cases;
>>
>>       - restart_uboot(): Add expect_earlyreset optional parameter so that
>>         it can handle the reboot while booting.
>>       - run_command(): Add wait_for_reboot optional parameter so that it
>>         can handle the reboot after executing a command.
>>
>>     And enable those options in the test_capsule_firmware.py test cases.
>>
>>     Signed-off-by: Masami Hiramatsu <masami.hiramatsu@linaro.org
>>     <mailto:masami.hiramatsu@linaro.org>>
>>     ---
>>       .../test_efi_capsule/test_capsule_firmware.py      |   39 ++++++--
>>       test/py/u_boot_console_base.py                     |   95
>>     +++++++++++++++-----
>>       test/py/u_boot_console_sandbox.py                  |    6 +
>>       3 files changed, 102 insertions(+), 38 deletions(-)
>>
>>     diff --git a/test/py/tests/test_efi_capsule/test_capsule_firmware.py
>>     b/test/py/tests/test_efi_capsule/test_capsule_firmware.py
>>     index 6e803f699f..a539134ec2 100644
>>     --- a/test/py/tests/test_efi_capsule/test_capsule_firmware.py
>>     +++ b/test/py/tests/test_efi_capsule/test_capsule_firmware.py
>>     @@ -143,13 +143,14 @@ class TestEfiCapsuleFirmwareFit(object):
>>                       'fatls host 0:1 %s' % CAPSULE_INSTALL_DIR])
>>                   assert 'Test01' in ''.join(output)
>>
>>     -        # reboot
>>     -        u_boot_console.restart_uboot()
>>     -
>>               capsule_early = u_boot_config.buildconfig.get(
>>                   'config_efi_capsule_on_disk_early')
>>               capsule_auth = u_boot_config.buildconfig.get(
>>                   'config_efi_capsule_authenticate')
>>     +
>>     +        # reboot
>>     +        u_boot_console.restart_uboot(expect_earlyreset = 
>> capsule_early)
>>     +
>>               with u_boot_console.log.section('Test Case 2-b, after
>>     reboot'):
>>                   if not capsule_early:
>>                       # make sure that dfu_alt_info exists even
>>     persistent variables
>>     @@ -162,7 +163,7 @@ class TestEfiCapsuleFirmwareFit(object):
>>
>>                       # need to run uefi command to initiate capsule
>>     handling
>>                       output = u_boot_console.run_command(
>>     -                    'env print -e Capsule0000')
>>     +                    'env print -e Capsule0000', wait_for_reboot = 
>> True)
>>
>>                   output = u_boot_console.run_command_list([
>>                       'host bind 0 %s' % disk_img,
>>     @@ -218,13 +219,14 @@ class TestEfiCapsuleFirmwareFit(object):
>>                       'fatls host 0:1 %s' % CAPSULE_INSTALL_DIR])
>>                   assert 'Test02' in ''.join(output)
>>
>>     -        # reboot
>>     -        u_boot_console.restart_uboot()
>>     -
>>               capsule_early = u_boot_config.buildconfig.get(
>>                   'config_efi_capsule_on_disk_early')
>>               capsule_auth = u_boot_config.buildconfig.get(
>>                   'config_efi_capsule_authenticate')
>>     +
>>     +        # reboot
>>     +        u_boot_console.restart_uboot(expect_earlyreset = 
>> capsule_early)
>>     +
>>               with u_boot_console.log.section('Test Case 3-b, after
>>     reboot'):
>>                   if not capsule_early:
>>                       # make sure that dfu_alt_info exists even
>>     persistent variables
>>     @@ -237,7 +239,13 @@ class TestEfiCapsuleFirmwareFit(object):
>>
>>                       # need to run uefi command to initiate capsule
>>     handling
>>                       output = u_boot_console.run_command(
>>     -                    'env print -e Capsule0000')
>>     +                    'env print -e Capsule0000', wait_for_reboot = 
>> True)
>>     +
>>     +            output = u_boot_console.run_command_list([
>>     +                'env set dfu_alt_info "sf 0:0=u-boot-bin raw
>>     0x100000 0x50000;u-boot-env raw 0x150000 0x200000"',
>>     +                'host bind 0 %s' % disk_img,
>>     +                'fatls host 0:1 %s' % CAPSULE_INSTALL_DIR])
>>     +            assert 'Test02' not in ''.join(output)
>>
>>                   output = u_boot_console.run_command_list(['efidebug
>>     capsule esrt'])
>>
>>     @@ -293,13 +301,14 @@ class TestEfiCapsuleFirmwareFit(object):
>>                       'fatls host 0:1 %s' % CAPSULE_INSTALL_DIR])
>>                   assert 'Test03' in ''.join(output)
>>
>>     -        # reboot
>>     -        u_boot_console.restart_uboot()
>>     -
>>               capsule_early = u_boot_config.buildconfig.get(
>>                   'config_efi_capsule_on_disk_early')
>>               capsule_auth = u_boot_config.buildconfig.get(
>>                   'config_efi_capsule_authenticate')
>>     +
>>     +        # reboot
>>     +        u_boot_console.restart_uboot(expect_earlyreset = 
>> capsule_early)
>>     +
>>               with u_boot_console.log.section('Test Case 4-b, after
>>     reboot'):
>>                   if not capsule_early:
>>                       # make sure that dfu_alt_info exists even
>>     persistent variables
>>     @@ -312,7 +321,13 @@ class TestEfiCapsuleFirmwareFit(object):
>>
>>                       # need to run uefi command to initiate capsule
>>     handling
>>                       output = u_boot_console.run_command(
>>     -                    'env print -e Capsule0000')
>>     +                    'env print -e Capsule0000', wait_for_reboot = 
>> True)
>>     +
>>     +            output = u_boot_console.run_command_list([
>>     +                'env set dfu_alt_info "sf 0:0=u-boot-bin raw
>>     0x100000 0x50000;u-boot-env raw 0x150000 0x200000"',
>>     +                'host bind 0 %s' % disk_img,
>>     +                'fatls host 0:1 %s' % CAPSULE_INSTALL_DIR])
>>     +            assert 'Test03' not in ''.join(output)
>>
>>                   output = u_boot_console.run_command_list(['efidebug
>>     capsule esrt'])
>>
>>     diff --git a/test/py/u_boot_console_base.py
>>     b/test/py/u_boot_console_base.py
>>     index 384fd53c65..e84f4d74ef 100644
>>     --- a/test/py/u_boot_console_base.py
>>     +++ b/test/py/u_boot_console_base.py
>>     @@ -140,7 +140,7 @@ class ConsoleBase(object):
>>               self.logstream.close()
>>
>>           def run_command(self, cmd, wait_for_echo=True, send_nl=True,
>>     -            wait_for_prompt=True):
>>     +            wait_for_prompt=True, wait_for_reboot=False):
>>               """Execute a command via the U-Boot console.
>>
>>               The command is always sent to U-Boot.
>>     @@ -168,6 +168,8 @@ class ConsoleBase(object):
>>                   wait_for_prompt: Boolean indicating whether to wait
>>     for the
>>                       command prompt to be sent by U-Boot. This
>>     typically occurs
>>                       immediately after the command has been executed.
>>     +            wait_for_reboot: Boolean indication whether to wait 
>> for the
>>     +                reboot U-Boot. If this is True, wait_for_prompt is
>>     ignored.
>>
>>               Returns:
>>                   If wait_for_prompt == False:
>>     @@ -202,11 +204,48 @@ class ConsoleBase(object):
>>                                           self.bad_pattern_ids[m - 1])
>>                   if not wait_for_prompt:
>>                       return
>>     -            m = self.p.expect([self.prompt_compiled] +
>>     self.bad_patterns)
>>     -            if m != 0:
>>     -                self.at_prompt = False
>>     -                raise Exception('Bad pattern found on console: ' +
>>     -                                self.bad_pattern_ids[m - 1])
>>     +            if wait_for_reboot:
>>     +                bcfg = self.config.buildconfig
>>     +                config_spl = bcfg.get('config_spl', 'n') == 'y'
>>     +                config_spl_serial = bcfg.get('config_spl_serial',
>>     +                                                 'n') == 'y'
>>     +                env_spl_skipped =
>>     self.config.env.get('env__spl_skipped',
>>     +                                                      False)
>>     +                env_spl2_skipped =
>>     self.config.env.get('env__spl2_skipped',
>>     +                                                       True)
>>     +                if config_spl and config_spl_serial and not
>>     env_spl_skipped:
>>     +                    m = self.p.expect([pattern_u_boot_spl_signon] +
>>     +                                      self.bad_patterns)
>>     +                    if m != 0:
>>     +                        raise Exception('Bad pattern found on SPL
>>     console: ' +
>>     +                                        self.bad_pattern_ids[m - 1])
>>     +                if not env_spl2_skipped:
>>     +                    m = self.p.expect([pattern_u_boot_spl2_signon] +
>>     +                                      self.bad_patterns)
>>     +                    if m != 0:
>>     +                        raise Exception('Bad pattern found on SPL2
>>     console: ' +
>>     +                                        self.bad_pattern_ids[m - 1])
>>     +                m = self.p.expect([pattern_u_boot_main_signon] +
>>     self.bad_patterns)
>>     +                if m != 0:
>>     +                    raise Exception('Bad pattern found on 
>> console: ' +
>>     +                                    self.bad_pattern_ids[m - 1])
>>     +                self.u_boot_version_string = self.p.after
>>     +                while True:
>>     +                    m = self.p.expect([self.prompt_compiled,
>>     +                        pattern_stop_autoboot_prompt] +
>>     self.bad_patterns)
>>     +                    if m == 0:
>>     +                        break
>>     +                    if m == 1:
>>     +                        self.p.send(' ')
>>     +                        continue
>>     +                    raise Exception('Bad pattern found on 
>> console: ' +
>>     +                                    self.bad_pattern_ids[m - 2])
>>     +            else:
>>     +                m = self.p.expect([self.prompt_compiled] +
>>     self.bad_patterns)
>>     +                if m != 0:
>>     +                    self.at_prompt = False
>>     +                    raise Exception('Bad pattern found on 
>> console: ' +
>>     +                                    self.bad_pattern_ids[m - 1])
>>                   self.at_prompt = True
>>                   self.at_prompt_logevt = self.logstream.logfile.cur_evt
>>                   # Only strip \r\n; space/TAB might be significant if
>>     testing
>>     @@ -321,7 +360,7 @@ class ConsoleBase(object):
>>               finally:
>>                   self.p.timeout = orig_timeout
>>
>>     -    def ensure_spawned(self):
>>     +    def ensure_spawned(self, expect_earlyreset=False):
>>               """Ensure a connection to a correctly running U-Boot 
>> instance.
>>
>>               This may require spawning a new Sandbox process or
>>     resetting target
>>     @@ -330,7 +369,8 @@ class ConsoleBase(object):
>>               This is an internal function and should not be called
>>     directly.
>>
>>               Args:
>>     -            None.
>>     +            expect_earlyreset: This boot is expected to be reset in
>>     early
>>     +                stage (before prompt). False by default.
>>
>>               Returns:
>>                   Nothing.
>>     @@ -357,22 +397,29 @@ class ConsoleBase(object):
>>                                                         False)
>>                   env_spl2_skipped =
>>     self.config.env.get('env__spl2_skipped',
>>                                                         True)
>>     -            if config_spl and config_spl_serial and not
>>     env_spl_skipped:
>>     -                m = self.p.expect([pattern_u_boot_spl_signon] +
>>     -                                  self.bad_patterns)
>>     +            if expect_earlyreset:
>>     +                loop_num = 2
>>     +            else:
>>     +                loop_num = 1
>>     +
>>     +            while loop_num > 0:
>>     +                loop_num -= 1
>>     +                if config_spl and config_spl_serial and not
>>     env_spl_skipped:
>>     +                    m = self.p.expect([pattern_u_boot_spl_signon] +
>>     +                                      self.bad_patterns)
>>     +                    if m != 0:
>>     +                        raise Exception('Bad pattern found on SPL
>>     console: ' +
>>     +                                        self.bad_pattern_ids[m - 1])
>>     +                if not env_spl2_skipped:
>>     +                    m = self.p.expect([pattern_u_boot_spl2_signon] +
>>     +                                      self.bad_patterns)
>>     +                    if m != 0:
>>     +                        raise Exception('Bad pattern found on SPL2
>>     console: ' +
>>     +                                        self.bad_pattern_ids[m - 1])
>>     +                m = self.p.expect([pattern_u_boot_main_signon] +
>>     self.bad_patterns)
>>                       if m != 0:
>>     -                    raise Exception('Bad pattern found on SPL
>>     console: ' +
>>     -                                    self.bad_pattern_ids[m - 1])
>>     -            if not env_spl2_skipped:
>>     -                m = self.p.expect([pattern_u_boot_spl2_signon] +
>>     -                                  self.bad_patterns)
>>     -                if m != 0:
>>     -                    raise Exception('Bad pattern found on SPL2
>>     console: ' +
>>     +                    raise Exception('Bad pattern found on 
>> console: ' +
>>                                           self.bad_pattern_ids[m - 1])
>>     -            m = self.p.expect([pattern_u_boot_main_signon] +
>>     self.bad_patterns)
>>     -            if m != 0:
>>     -                raise Exception('Bad pattern found on console: ' +
>>     -                                self.bad_pattern_ids[m - 1])
>>                   self.u_boot_version_string = self.p.after
>>                   while True:
>>                       m = self.p.expect([self.prompt_compiled,
>>     @@ -416,10 +463,10 @@ class ConsoleBase(object):
>>                   pass
>>               self.p = None
>>
>>     -    def restart_uboot(self):
>>     +    def restart_uboot(self, expect_earlyreset=False):
>>               """Shut down and restart U-Boot."""
>>               self.cleanup_spawn()
>>     -        self.ensure_spawned()
>>     +        self.ensure_spawned(expect_earlyreset)
>>
>>           def get_spawn_output(self):
>>               """Return the start-up output from U-Boot
>>     diff --git a/test/py/u_boot_console_sandbox.py
>>     b/test/py/u_boot_console_sandbox.py
>>     index 7e1eb0e0b4..9cd9ccad30 100644
>>     --- a/test/py/u_boot_console_sandbox.py
>>     +++ b/test/py/u_boot_console_sandbox.py
>>     @@ -57,11 +57,13 @@ class ConsoleSandbox(ConsoleBase):
>>               cmd += self.sandbox_flags
>>               return Spawn(cmd, cwd=self.config.source_dir)
>>
>>     -    def restart_uboot_with_flags(self, flags):
>>     +    def restart_uboot_with_flags(self, flags, 
>> expect_earlyreset=False):
>>               """Run U-Boot with the given command-line flags
>>
>>               Args:
>>                   flags: List of flags to pass, each a string
>>     +            expect_earlyreset: This boot is expected to be reset in
>>     early
>>     +                stage (before prompt). False by default.
>>
>>               Returns:
>>                   A u_boot_spawn.Spawn object that is attached to U-Boot.
>>     @@ -69,7 +71,7 @@ class ConsoleSandbox(ConsoleBase):
>>
>>               try:
>>                   self.sandbox_flags = flags
>>     -            return self.restart_uboot()
>>     +            return self.restart_uboot(expect_earlyreset)
>>               finally:
>>                   self.sandbox_flags = []
>>
>>
>>
>>
>> -- 
>> Masami Hiramatsu
>
Heinrich Schuchardt Feb. 15, 2022, 2:15 p.m. UTC | #4
On 2/15/22 10:05, Masami Hiramatsu wrote:
> Since now the capsule_on_disk will restart the u-boot sandbox right
> after the capsule update, if CONFIG_EFI_CAPSULE_ON_DISK_EARLY=y, the
> boot with a new capsule file will repeat reboot sequence. On the
> other hand, if CONFIG_EFI_CAPSULE_ON_DISK_EARLY=n, the 'env print -e'
> command will execute the capsule update on disk and reboot.
>
> Thus this update the uboot_console for those 2 cases;
>
>   - restart_uboot(): Add expect_earlyreset optional parameter so that
>     it can handle the reboot while booting.
>   - run_command(): Add wait_for_reboot optional parameter so that it
>     can handle the reboot after executing a command.
>
> And enable those options in the test_capsule_firmware.py test cases.

When applying a patch series we must ensure that after each individual
patch we have a valid software state.

'make tests' fails in test_capsule_firmware.py after applying only this
patch to U-Boot origin/master.

This patch should be split in two:

* changes to the console base, to be applied before the capsule series
* changes to the capsule test, to be merged into the "EFI: Reset system
after capsule-on-disk" series.

Please, ensure that after each single patch 'make tests' succeeds.


>
> Signed-off-by: Masami Hiramatsu <masami.hiramatsu@linaro.org>
> ---
>   .../test_efi_capsule/test_capsule_firmware.py      |   39 ++++++--
>   test/py/u_boot_console_base.py                     |   95 +++++++++++++++-----
>   test/py/u_boot_console_sandbox.py                  |    6 +
>   3 files changed, 102 insertions(+), 38 deletions(-)
>
> diff --git a/test/py/tests/test_efi_capsule/test_capsule_firmware.py b/test/py/tests/test_efi_capsule/test_capsule_firmware.py
> index 6e803f699f..a539134ec2 100644
> --- a/test/py/tests/test_efi_capsule/test_capsule_firmware.py
> +++ b/test/py/tests/test_efi_capsule/test_capsule_firmware.py
> @@ -143,13 +143,14 @@ class TestEfiCapsuleFirmwareFit(object):
>                   'fatls host 0:1 %s' % CAPSULE_INSTALL_DIR])
>               assert 'Test01' in ''.join(output)
>
> -        # reboot
> -        u_boot_console.restart_uboot()
> -
>           capsule_early = u_boot_config.buildconfig.get(
>               'config_efi_capsule_on_disk_early')
>           capsule_auth = u_boot_config.buildconfig.get(
>               'config_efi_capsule_authenticate')
> +
> +        # reboot
> +        u_boot_console.restart_uboot(expect_earlyreset = capsule_early)
> +
>           with u_boot_console.log.section('Test Case 2-b, after reboot'):
>               if not capsule_early:
>                   # make sure that dfu_alt_info exists even persistent variables
> @@ -162,7 +163,7 @@ class TestEfiCapsuleFirmwareFit(object):
>
>                   # need to run uefi command to initiate capsule handling
>                   output = u_boot_console.run_command(
> -                    'env print -e Capsule0000')
> +                    'env print -e Capsule0000', wait_for_reboot = True)
>
>               output = u_boot_console.run_command_list([
>                   'host bind 0 %s' % disk_img,
> @@ -218,13 +219,14 @@ class TestEfiCapsuleFirmwareFit(object):
>                   'fatls host 0:1 %s' % CAPSULE_INSTALL_DIR])
>               assert 'Test02' in ''.join(output)
>
> -        # reboot
> -        u_boot_console.restart_uboot()
> -
>           capsule_early = u_boot_config.buildconfig.get(
>               'config_efi_capsule_on_disk_early')
>           capsule_auth = u_boot_config.buildconfig.get(
>               'config_efi_capsule_authenticate')
> +
> +        # reboot
> +        u_boot_console.restart_uboot(expect_earlyreset = capsule_early)
> +
>           with u_boot_console.log.section('Test Case 3-b, after reboot'):
>               if not capsule_early:
>                   # make sure that dfu_alt_info exists even persistent variables
> @@ -237,7 +239,13 @@ class TestEfiCapsuleFirmwareFit(object):
>
>                   # need to run uefi command to initiate capsule handling
>                   output = u_boot_console.run_command(
> -                    'env print -e Capsule0000')
> +                    'env print -e Capsule0000', wait_for_reboot = True)
> +
> +            output = u_boot_console.run_command_list([
> +                'env set dfu_alt_info "sf 0:0=u-boot-bin raw 0x100000 0x50000;u-boot-env raw 0x150000 0x200000"',
> +                'host bind 0 %s' % disk_img,
> +                'fatls host 0:1 %s' % CAPSULE_INSTALL_DIR])
> +            assert 'Test02' not in ''.join(output)
>
>               output = u_boot_console.run_command_list(['efidebug capsule esrt'])
>
> @@ -293,13 +301,14 @@ class TestEfiCapsuleFirmwareFit(object):
>                   'fatls host 0:1 %s' % CAPSULE_INSTALL_DIR])
>               assert 'Test03' in ''.join(output)
>
> -        # reboot
> -        u_boot_console.restart_uboot()
> -
>           capsule_early = u_boot_config.buildconfig.get(
>               'config_efi_capsule_on_disk_early')
>           capsule_auth = u_boot_config.buildconfig.get(
>               'config_efi_capsule_authenticate')
> +
> +        # reboot
> +        u_boot_console.restart_uboot(expect_earlyreset = capsule_early)
> +
>           with u_boot_console.log.section('Test Case 4-b, after reboot'):
>               if not capsule_early:
>                   # make sure that dfu_alt_info exists even persistent variables
> @@ -312,7 +321,13 @@ class TestEfiCapsuleFirmwareFit(object):
>
>                   # need to run uefi command to initiate capsule handling
>                   output = u_boot_console.run_command(
> -                    'env print -e Capsule0000')
> +                    'env print -e Capsule0000', wait_for_reboot = True)
> +
> +            output = u_boot_console.run_command_list([
> +                'env set dfu_alt_info "sf 0:0=u-boot-bin raw 0x100000 0x50000;u-boot-env raw 0x150000 0x200000"',
> +                'host bind 0 %s' % disk_img,
> +                'fatls host 0:1 %s' % CAPSULE_INSTALL_DIR])
> +            assert 'Test03' not in ''.join(output)
>
>               output = u_boot_console.run_command_list(['efidebug capsule esrt'])
>
> diff --git a/test/py/u_boot_console_base.py b/test/py/u_boot_console_base.py
> index 384fd53c65..e84f4d74ef 100644
> --- a/test/py/u_boot_console_base.py
> +++ b/test/py/u_boot_console_base.py
> @@ -140,7 +140,7 @@ class ConsoleBase(object):
>           self.logstream.close()
>
>       def run_command(self, cmd, wait_for_echo=True, send_nl=True,
> -            wait_for_prompt=True):
> +            wait_for_prompt=True, wait_for_reboot=False):
>           """Execute a command via the U-Boot console.
>
>           The command is always sent to U-Boot.
> @@ -168,6 +168,8 @@ class ConsoleBase(object):
>               wait_for_prompt: Boolean indicating whether to wait for the
>                   command prompt to be sent by U-Boot. This typically occurs
>                   immediately after the command has been executed.
> +            wait_for_reboot: Boolean indication whether to wait for the
> +                reboot U-Boot. If this is True, wait_for_prompt is ignored.
>
>           Returns:
>               If wait_for_prompt == False:
> @@ -202,11 +204,48 @@ class ConsoleBase(object):
>                                       self.bad_pattern_ids[m - 1])
>               if not wait_for_prompt:
>                   return
> -            m = self.p.expect([self.prompt_compiled] + self.bad_patterns)
> -            if m != 0:
> -                self.at_prompt = False
> -                raise Exception('Bad pattern found on console: ' +
> -                                self.bad_pattern_ids[m - 1])
> +            if wait_for_reboot:
> +                bcfg = self.config.buildconfig
> +                config_spl = bcfg.get('config_spl', 'n') == 'y'
> +                config_spl_serial = bcfg.get('config_spl_serial',
> +                                                 'n') == 'y'
> +                env_spl_skipped = self.config.env.get('env__spl_skipped',
> +                                                      False)
> +                env_spl2_skipped = self.config.env.get('env__spl2_skipped',
> +                                                       True)
> +                if config_spl and config_spl_serial and not env_spl_skipped:
> +                    m = self.p.expect([pattern_u_boot_spl_signon] +
> +                                      self.bad_patterns)
> +                    if m != 0:
> +                        raise Exception('Bad pattern found on SPL console: ' +
> +                                        self.bad_pattern_ids[m - 1])
> +                if not env_spl2_skipped:
> +                    m = self.p.expect([pattern_u_boot_spl2_signon] +
> +                                      self.bad_patterns)
> +                    if m != 0:
> +                        raise Exception('Bad pattern found on SPL2 console: ' +
> +                                        self.bad_pattern_ids[m - 1])
> +                m = self.p.expect([pattern_u_boot_main_signon] + self.bad_patterns)
> +                if m != 0:
> +                    raise Exception('Bad pattern found on console: ' +
> +                                    self.bad_pattern_ids[m - 1])
> +                self.u_boot_version_string = self.p.after
> +                while True:
> +                    m = self.p.expect([self.prompt_compiled,
> +                        pattern_stop_autoboot_prompt] + self.bad_patterns)
> +                    if m == 0:
> +                        break
> +                    if m == 1:
> +                        self.p.send(' ')
> +                        continue
> +                    raise Exception('Bad pattern found on console: ' +
> +                                    self.bad_pattern_ids[m - 2])
> +            else:
> +                m = self.p.expect([self.prompt_compiled] + self.bad_patterns)
> +                if m != 0:
> +                    self.at_prompt = False
> +                    raise Exception('Bad pattern found on console: ' +
> +                                    self.bad_pattern_ids[m - 1])
>               self.at_prompt = True
>               self.at_prompt_logevt = self.logstream.logfile.cur_evt
>               # Only strip \r\n; space/TAB might be significant if testing
> @@ -321,7 +360,7 @@ class ConsoleBase(object):
>           finally:
>               self.p.timeout = orig_timeout
>
> -    def ensure_spawned(self):
> +    def ensure_spawned(self, expect_earlyreset=False):
>           """Ensure a connection to a correctly running U-Boot instance.
>
>           This may require spawning a new Sandbox process or resetting target
> @@ -330,7 +369,8 @@ class ConsoleBase(object):
>           This is an internal function and should not be called directly.
>
>           Args:
> -            None.
> +            expect_earlyreset: This boot is expected to be reset in early
> +                stage (before prompt). False by default.

 From the description of the argument it is not clear that only a reset
inside main U-Boot is tolerated if expect_earlyreset==true while an
early reset in SPL leads to a test failure.

Best regards

Heinrich

>
>           Returns:
>               Nothing.
> @@ -357,22 +397,29 @@ class ConsoleBase(object):
>                                                     False)
>               env_spl2_skipped = self.config.env.get('env__spl2_skipped',
>                                                     True)
> -            if config_spl and config_spl_serial and not env_spl_skipped:
> -                m = self.p.expect([pattern_u_boot_spl_signon] +
> -                                  self.bad_patterns)
> +            if expect_earlyreset:
> +                loop_num = 2
> +            else:
> +                loop_num = 1
> +
> +            while loop_num > 0:
> +                loop_num -= 1
> +                if config_spl and config_spl_serial and not env_spl_skipped:
> +                    m = self.p.expect([pattern_u_boot_spl_signon] +
> +                                      self.bad_patterns)
> +                    if m != 0:
> +                        raise Exception('Bad pattern found on SPL console: ' +
> +                                        self.bad_pattern_ids[m - 1])
> +                if not env_spl2_skipped:
> +                    m = self.p.expect([pattern_u_boot_spl2_signon] +
> +                                      self.bad_patterns)
> +                    if m != 0:
> +                        raise Exception('Bad pattern found on SPL2 console: ' +
> +                                        self.bad_pattern_ids[m - 1])
> +                m = self.p.expect([pattern_u_boot_main_signon] + self.bad_patterns)
>                   if m != 0:
> -                    raise Exception('Bad pattern found on SPL console: ' +
> -                                    self.bad_pattern_ids[m - 1])
> -            if not env_spl2_skipped:
> -                m = self.p.expect([pattern_u_boot_spl2_signon] +
> -                                  self.bad_patterns)
> -                if m != 0:
> -                    raise Exception('Bad pattern found on SPL2 console: ' +
> +                    raise Exception('Bad pattern found on console: ' +
>                                       self.bad_pattern_ids[m - 1])
> -            m = self.p.expect([pattern_u_boot_main_signon] + self.bad_patterns)
> -            if m != 0:
> -                raise Exception('Bad pattern found on console: ' +
> -                                self.bad_pattern_ids[m - 1])
>               self.u_boot_version_string = self.p.after
>               while True:
>                   m = self.p.expect([self.prompt_compiled,
> @@ -416,10 +463,10 @@ class ConsoleBase(object):
>               pass
>           self.p = None
>
> -    def restart_uboot(self):
> +    def restart_uboot(self, expect_earlyreset=False):
>           """Shut down and restart U-Boot."""
>           self.cleanup_spawn()
> -        self.ensure_spawned()
> +        self.ensure_spawned(expect_earlyreset)
>
>       def get_spawn_output(self):
>           """Return the start-up output from U-Boot
> diff --git a/test/py/u_boot_console_sandbox.py b/test/py/u_boot_console_sandbox.py
> index 7e1eb0e0b4..9cd9ccad30 100644
> --- a/test/py/u_boot_console_sandbox.py
> +++ b/test/py/u_boot_console_sandbox.py
> @@ -57,11 +57,13 @@ class ConsoleSandbox(ConsoleBase):
>           cmd += self.sandbox_flags
>           return Spawn(cmd, cwd=self.config.source_dir)
>
> -    def restart_uboot_with_flags(self, flags):
> +    def restart_uboot_with_flags(self, flags, expect_earlyreset=False):
>           """Run U-Boot with the given command-line flags
>
>           Args:
>               flags: List of flags to pass, each a string
> +            expect_earlyreset: This boot is expected to be reset in early
> +                stage (before prompt). False by default.
>
>           Returns:
>               A u_boot_spawn.Spawn object that is attached to U-Boot.
> @@ -69,7 +71,7 @@ class ConsoleSandbox(ConsoleBase):
>
>           try:
>               self.sandbox_flags = flags
> -            return self.restart_uboot()
> +            return self.restart_uboot(expect_earlyreset)
>           finally:
>               self.sandbox_flags = []
>
>
Masami Hiramatsu Feb. 15, 2022, 11:50 p.m. UTC | #5
Hi

2022年2月15日(火) 23:15 Heinrich Schuchardt <xypron.glpk@gmx.de>:
>
> On 2/15/22 10:05, Masami Hiramatsu wrote:
> > Since now the capsule_on_disk will restart the u-boot sandbox right
> > after the capsule update, if CONFIG_EFI_CAPSULE_ON_DISK_EARLY=y, the
> > boot with a new capsule file will repeat reboot sequence. On the
> > other hand, if CONFIG_EFI_CAPSULE_ON_DISK_EARLY=n, the 'env print -e'
> > command will execute the capsule update on disk and reboot.
> >
> > Thus this update the uboot_console for those 2 cases;
> >
> >   - restart_uboot(): Add expect_earlyreset optional parameter so that
> >     it can handle the reboot while booting.
> >   - run_command(): Add wait_for_reboot optional parameter so that it
> >     can handle the reboot after executing a command.
> >
> > And enable those options in the test_capsule_firmware.py test cases.
>
> When applying a patch series we must ensure that after each individual
> patch we have a valid software state.
>
> 'make tests' fails in test_capsule_firmware.py after applying only this
> patch to U-Boot origin/master.

Ah, OK. So let me update and resend the reset-after-capsule series.

>
> This patch should be split in two:
>
> * changes to the console base, to be applied before the capsule series
> * changes to the capsule test, to be merged into the "EFI: Reset system
> after capsule-on-disk" series.
>
> Please, ensure that after each single patch 'make tests' succeeds.

OK, I'll make it bisectable. BTW, I think the latter part should be
merged to the "reset-after-capsule" patch because it changes
the behavior that the old test expects.

[snip]

> > @@ -330,7 +369,8 @@ class ConsoleBase(object):
> >           This is an internal function and should not be called directly.
> >
> >           Args:
> > -            None.
> > +            expect_earlyreset: This boot is expected to be reset in early
> > +                stage (before prompt). False by default.
>
>  From the description of the argument it is not clear that only a reset
> inside main U-Boot is tolerated if expect_earlyreset==true while an
> early reset in SPL leads to a test failure.

Indeed. I'll make it simply "expect_reset" and update the comment
that the reset is expected after SPL before prompt.

Thank you,

>
> Best regards
>
> Heinrich
>
> >
> >           Returns:
> >               Nothing.
> > @@ -357,22 +397,29 @@ class ConsoleBase(object):
> >                                                     False)
> >               env_spl2_skipped = self.config.env.get('env__spl2_skipped',
> >                                                     True)
> > -            if config_spl and config_spl_serial and not env_spl_skipped:
> > -                m = self.p.expect([pattern_u_boot_spl_signon] +
> > -                                  self.bad_patterns)
> > +            if expect_earlyreset:
> > +                loop_num = 2
> > +            else:
> > +                loop_num = 1
> > +
> > +            while loop_num > 0:
> > +                loop_num -= 1
> > +                if config_spl and config_spl_serial and not env_spl_skipped:
> > +                    m = self.p.expect([pattern_u_boot_spl_signon] +
> > +                                      self.bad_patterns)
> > +                    if m != 0:
> > +                        raise Exception('Bad pattern found on SPL console: ' +
> > +                                        self.bad_pattern_ids[m - 1])
> > +                if not env_spl2_skipped:
> > +                    m = self.p.expect([pattern_u_boot_spl2_signon] +
> > +                                      self.bad_patterns)
> > +                    if m != 0:
> > +                        raise Exception('Bad pattern found on SPL2 console: ' +
> > +                                        self.bad_pattern_ids[m - 1])
> > +                m = self.p.expect([pattern_u_boot_main_signon] + self.bad_patterns)
> >                   if m != 0:
> > -                    raise Exception('Bad pattern found on SPL console: ' +
> > -                                    self.bad_pattern_ids[m - 1])
> > -            if not env_spl2_skipped:
> > -                m = self.p.expect([pattern_u_boot_spl2_signon] +
> > -                                  self.bad_patterns)
> > -                if m != 0:
> > -                    raise Exception('Bad pattern found on SPL2 console: ' +
> > +                    raise Exception('Bad pattern found on console: ' +
> >                                       self.bad_pattern_ids[m - 1])
> > -            m = self.p.expect([pattern_u_boot_main_signon] + self.bad_patterns)
> > -            if m != 0:
> > -                raise Exception('Bad pattern found on console: ' +
> > -                                self.bad_pattern_ids[m - 1])
> >               self.u_boot_version_string = self.p.after
> >               while True:
> >                   m = self.p.expect([self.prompt_compiled,
> > @@ -416,10 +463,10 @@ class ConsoleBase(object):
> >               pass
> >           self.p = None
> >
> > -    def restart_uboot(self):
> > +    def restart_uboot(self, expect_earlyreset=False):
> >           """Shut down and restart U-Boot."""
> >           self.cleanup_spawn()
> > -        self.ensure_spawned()
> > +        self.ensure_spawned(expect_earlyreset)
> >
> >       def get_spawn_output(self):
> >           """Return the start-up output from U-Boot
> > diff --git a/test/py/u_boot_console_sandbox.py b/test/py/u_boot_console_sandbox.py
> > index 7e1eb0e0b4..9cd9ccad30 100644
> > --- a/test/py/u_boot_console_sandbox.py
> > +++ b/test/py/u_boot_console_sandbox.py
> > @@ -57,11 +57,13 @@ class ConsoleSandbox(ConsoleBase):
> >           cmd += self.sandbox_flags
> >           return Spawn(cmd, cwd=self.config.source_dir)
> >
> > -    def restart_uboot_with_flags(self, flags):
> > +    def restart_uboot_with_flags(self, flags, expect_earlyreset=False):
> >           """Run U-Boot with the given command-line flags
> >
> >           Args:
> >               flags: List of flags to pass, each a string
> > +            expect_earlyreset: This boot is expected to be reset in early
> > +                stage (before prompt). False by default.
> >
> >           Returns:
> >               A u_boot_spawn.Spawn object that is attached to U-Boot.
> > @@ -69,7 +71,7 @@ class ConsoleSandbox(ConsoleBase):
> >
> >           try:
> >               self.sandbox_flags = flags
> > -            return self.restart_uboot()
> > +            return self.restart_uboot(expect_earlyreset)
> >           finally:
> >               self.sandbox_flags = []
> >
> >
>


2022年2月15日(火) 23:15 Heinrich Schuchardt <xypron.glpk@gmx.de>:
>
> On 2/15/22 10:05, Masami Hiramatsu wrote:
> > Since now the capsule_on_disk will restart the u-boot sandbox right
> > after the capsule update, if CONFIG_EFI_CAPSULE_ON_DISK_EARLY=y, the
> > boot with a new capsule file will repeat reboot sequence. On the
> > other hand, if CONFIG_EFI_CAPSULE_ON_DISK_EARLY=n, the 'env print -e'
> > command will execute the capsule update on disk and reboot.
> >
> > Thus this update the uboot_console for those 2 cases;
> >
> >   - restart_uboot(): Add expect_earlyreset optional parameter so that
> >     it can handle the reboot while booting.
> >   - run_command(): Add wait_for_reboot optional parameter so that it
> >     can handle the reboot after executing a command.
> >
> > And enable those options in the test_capsule_firmware.py test cases.
>
> When applying a patch series we must ensure that after each individual
> patch we have a valid software state.
>
> 'make tests' fails in test_capsule_firmware.py after applying only this
> patch to U-Boot origin/master.
>
> This patch should be split in two:
>
> * changes to the console base, to be applied before the capsule series
> * changes to the capsule test, to be merged into the "EFI: Reset system
> after capsule-on-disk" series.
>
> Please, ensure that after each single patch 'make tests' succeeds.
>
>
> >
> > Signed-off-by: Masami Hiramatsu <masami.hiramatsu@linaro.org>
> > ---
> >   .../test_efi_capsule/test_capsule_firmware.py      |   39 ++++++--
> >   test/py/u_boot_console_base.py                     |   95 +++++++++++++++-----
> >   test/py/u_boot_console_sandbox.py                  |    6 +
> >   3 files changed, 102 insertions(+), 38 deletions(-)
> >
> > diff --git a/test/py/tests/test_efi_capsule/test_capsule_firmware.py b/test/py/tests/test_efi_capsule/test_capsule_firmware.py
> > index 6e803f699f..a539134ec2 100644
> > --- a/test/py/tests/test_efi_capsule/test_capsule_firmware.py
> > +++ b/test/py/tests/test_efi_capsule/test_capsule_firmware.py
> > @@ -143,13 +143,14 @@ class TestEfiCapsuleFirmwareFit(object):
> >                   'fatls host 0:1 %s' % CAPSULE_INSTALL_DIR])
> >               assert 'Test01' in ''.join(output)
> >
> > -        # reboot
> > -        u_boot_console.restart_uboot()
> > -
> >           capsule_early = u_boot_config.buildconfig.get(
> >               'config_efi_capsule_on_disk_early')
> >           capsule_auth = u_boot_config.buildconfig.get(
> >               'config_efi_capsule_authenticate')
> > +
> > +        # reboot
> > +        u_boot_console.restart_uboot(expect_earlyreset = capsule_early)
> > +
> >           with u_boot_console.log.section('Test Case 2-b, after reboot'):
> >               if not capsule_early:
> >                   # make sure that dfu_alt_info exists even persistent variables
> > @@ -162,7 +163,7 @@ class TestEfiCapsuleFirmwareFit(object):
> >
> >                   # need to run uefi command to initiate capsule handling
> >                   output = u_boot_console.run_command(
> > -                    'env print -e Capsule0000')
> > +                    'env print -e Capsule0000', wait_for_reboot = True)
> >
> >               output = u_boot_console.run_command_list([
> >                   'host bind 0 %s' % disk_img,
> > @@ -218,13 +219,14 @@ class TestEfiCapsuleFirmwareFit(object):
> >                   'fatls host 0:1 %s' % CAPSULE_INSTALL_DIR])
> >               assert 'Test02' in ''.join(output)
> >
> > -        # reboot
> > -        u_boot_console.restart_uboot()
> > -
> >           capsule_early = u_boot_config.buildconfig.get(
> >               'config_efi_capsule_on_disk_early')
> >           capsule_auth = u_boot_config.buildconfig.get(
> >               'config_efi_capsule_authenticate')
> > +
> > +        # reboot
> > +        u_boot_console.restart_uboot(expect_earlyreset = capsule_early)
> > +
> >           with u_boot_console.log.section('Test Case 3-b, after reboot'):
> >               if not capsule_early:
> >                   # make sure that dfu_alt_info exists even persistent variables
> > @@ -237,7 +239,13 @@ class TestEfiCapsuleFirmwareFit(object):
> >
> >                   # need to run uefi command to initiate capsule handling
> >                   output = u_boot_console.run_command(
> > -                    'env print -e Capsule0000')
> > +                    'env print -e Capsule0000', wait_for_reboot = True)
> > +
> > +            output = u_boot_console.run_command_list([
> > +                'env set dfu_alt_info "sf 0:0=u-boot-bin raw 0x100000 0x50000;u-boot-env raw 0x150000 0x200000"',
> > +                'host bind 0 %s' % disk_img,
> > +                'fatls host 0:1 %s' % CAPSULE_INSTALL_DIR])
> > +            assert 'Test02' not in ''.join(output)
> >
> >               output = u_boot_console.run_command_list(['efidebug capsule esrt'])
> >
> > @@ -293,13 +301,14 @@ class TestEfiCapsuleFirmwareFit(object):
> >                   'fatls host 0:1 %s' % CAPSULE_INSTALL_DIR])
> >               assert 'Test03' in ''.join(output)
> >
> > -        # reboot
> > -        u_boot_console.restart_uboot()
> > -
> >           capsule_early = u_boot_config.buildconfig.get(
> >               'config_efi_capsule_on_disk_early')
> >           capsule_auth = u_boot_config.buildconfig.get(
> >               'config_efi_capsule_authenticate')
> > +
> > +        # reboot
> > +        u_boot_console.restart_uboot(expect_earlyreset = capsule_early)
> > +
> >           with u_boot_console.log.section('Test Case 4-b, after reboot'):
> >               if not capsule_early:
> >                   # make sure that dfu_alt_info exists even persistent variables
> > @@ -312,7 +321,13 @@ class TestEfiCapsuleFirmwareFit(object):
> >
> >                   # need to run uefi command to initiate capsule handling
> >                   output = u_boot_console.run_command(
> > -                    'env print -e Capsule0000')
> > +                    'env print -e Capsule0000', wait_for_reboot = True)
> > +
> > +            output = u_boot_console.run_command_list([
> > +                'env set dfu_alt_info "sf 0:0=u-boot-bin raw 0x100000 0x50000;u-boot-env raw 0x150000 0x200000"',
> > +                'host bind 0 %s' % disk_img,
> > +                'fatls host 0:1 %s' % CAPSULE_INSTALL_DIR])
> > +            assert 'Test03' not in ''.join(output)
> >
> >               output = u_boot_console.run_command_list(['efidebug capsule esrt'])
> >
> > diff --git a/test/py/u_boot_console_base.py b/test/py/u_boot_console_base.py
> > index 384fd53c65..e84f4d74ef 100644
> > --- a/test/py/u_boot_console_base.py
> > +++ b/test/py/u_boot_console_base.py
> > @@ -140,7 +140,7 @@ class ConsoleBase(object):
> >           self.logstream.close()
> >
> >       def run_command(self, cmd, wait_for_echo=True, send_nl=True,
> > -            wait_for_prompt=True):
> > +            wait_for_prompt=True, wait_for_reboot=False):
> >           """Execute a command via the U-Boot console.
> >
> >           The command is always sent to U-Boot.
> > @@ -168,6 +168,8 @@ class ConsoleBase(object):
> >               wait_for_prompt: Boolean indicating whether to wait for the
> >                   command prompt to be sent by U-Boot. This typically occurs
> >                   immediately after the command has been executed.
> > +            wait_for_reboot: Boolean indication whether to wait for the
> > +                reboot U-Boot. If this is True, wait_for_prompt is ignored.
> >
> >           Returns:
> >               If wait_for_prompt == False:
> > @@ -202,11 +204,48 @@ class ConsoleBase(object):
> >                                       self.bad_pattern_ids[m - 1])
> >               if not wait_for_prompt:
> >                   return
> > -            m = self.p.expect([self.prompt_compiled] + self.bad_patterns)
> > -            if m != 0:
> > -                self.at_prompt = False
> > -                raise Exception('Bad pattern found on console: ' +
> > -                                self.bad_pattern_ids[m - 1])
> > +            if wait_for_reboot:
> > +                bcfg = self.config.buildconfig
> > +                config_spl = bcfg.get('config_spl', 'n') == 'y'
> > +                config_spl_serial = bcfg.get('config_spl_serial',
> > +                                                 'n') == 'y'
> > +                env_spl_skipped = self.config.env.get('env__spl_skipped',
> > +                                                      False)
> > +                env_spl2_skipped = self.config.env.get('env__spl2_skipped',
> > +                                                       True)
> > +                if config_spl and config_spl_serial and not env_spl_skipped:
> > +                    m = self.p.expect([pattern_u_boot_spl_signon] +
> > +                                      self.bad_patterns)
> > +                    if m != 0:
> > +                        raise Exception('Bad pattern found on SPL console: ' +
> > +                                        self.bad_pattern_ids[m - 1])
> > +                if not env_spl2_skipped:
> > +                    m = self.p.expect([pattern_u_boot_spl2_signon] +
> > +                                      self.bad_patterns)
> > +                    if m != 0:
> > +                        raise Exception('Bad pattern found on SPL2 console: ' +
> > +                                        self.bad_pattern_ids[m - 1])
> > +                m = self.p.expect([pattern_u_boot_main_signon] + self.bad_patterns)
> > +                if m != 0:
> > +                    raise Exception('Bad pattern found on console: ' +
> > +                                    self.bad_pattern_ids[m - 1])
> > +                self.u_boot_version_string = self.p.after
> > +                while True:
> > +                    m = self.p.expect([self.prompt_compiled,
> > +                        pattern_stop_autoboot_prompt] + self.bad_patterns)
> > +                    if m == 0:
> > +                        break
> > +                    if m == 1:
> > +                        self.p.send(' ')
> > +                        continue
> > +                    raise Exception('Bad pattern found on console: ' +
> > +                                    self.bad_pattern_ids[m - 2])
> > +            else:
> > +                m = self.p.expect([self.prompt_compiled] + self.bad_patterns)
> > +                if m != 0:
> > +                    self.at_prompt = False
> > +                    raise Exception('Bad pattern found on console: ' +
> > +                                    self.bad_pattern_ids[m - 1])
> >               self.at_prompt = True
> >               self.at_prompt_logevt = self.logstream.logfile.cur_evt
> >               # Only strip \r\n; space/TAB might be significant if testing
> > @@ -321,7 +360,7 @@ class ConsoleBase(object):
> >           finally:
> >               self.p.timeout = orig_timeout
> >
> > -    def ensure_spawned(self):
> > +    def ensure_spawned(self, expect_earlyreset=False):
> >           """Ensure a connection to a correctly running U-Boot instance.
> >
> >           This may require spawning a new Sandbox process or resetting target
> > @@ -330,7 +369,8 @@ class ConsoleBase(object):
> >           This is an internal function and should not be called directly.
> >
> >           Args:
> > -            None.
> > +            expect_earlyreset: This boot is expected to be reset in early
> > +                stage (before prompt). False by default.
>
>  From the description of the argument it is not clear that only a reset
> inside main U-Boot is tolerated if expect_earlyreset==true while an
> early reset in SPL leads to a test failure.
>
> Best regards
>
> Heinrich
>
> >
> >           Returns:
> >               Nothing.
> > @@ -357,22 +397,29 @@ class ConsoleBase(object):
> >                                                     False)
> >               env_spl2_skipped = self.config.env.get('env__spl2_skipped',
> >                                                     True)
> > -            if config_spl and config_spl_serial and not env_spl_skipped:
> > -                m = self.p.expect([pattern_u_boot_spl_signon] +
> > -                                  self.bad_patterns)
> > +            if expect_earlyreset:
> > +                loop_num = 2
> > +            else:
> > +                loop_num = 1
> > +
> > +            while loop_num > 0:
> > +                loop_num -= 1
> > +                if config_spl and config_spl_serial and not env_spl_skipped:
> > +                    m = self.p.expect([pattern_u_boot_spl_signon] +
> > +                                      self.bad_patterns)
> > +                    if m != 0:
> > +                        raise Exception('Bad pattern found on SPL console: ' +
> > +                                        self.bad_pattern_ids[m - 1])
> > +                if not env_spl2_skipped:
> > +                    m = self.p.expect([pattern_u_boot_spl2_signon] +
> > +                                      self.bad_patterns)
> > +                    if m != 0:
> > +                        raise Exception('Bad pattern found on SPL2 console: ' +
> > +                                        self.bad_pattern_ids[m - 1])
> > +                m = self.p.expect([pattern_u_boot_main_signon] + self.bad_patterns)
> >                   if m != 0:
> > -                    raise Exception('Bad pattern found on SPL console: ' +
> > -                                    self.bad_pattern_ids[m - 1])
> > -            if not env_spl2_skipped:
> > -                m = self.p.expect([pattern_u_boot_spl2_signon] +
> > -                                  self.bad_patterns)
> > -                if m != 0:
> > -                    raise Exception('Bad pattern found on SPL2 console: ' +
> > +                    raise Exception('Bad pattern found on console: ' +
> >                                       self.bad_pattern_ids[m - 1])
> > -            m = self.p.expect([pattern_u_boot_main_signon] + self.bad_patterns)
> > -            if m != 0:
> > -                raise Exception('Bad pattern found on console: ' +
> > -                                self.bad_pattern_ids[m - 1])
> >               self.u_boot_version_string = self.p.after
> >               while True:
> >                   m = self.p.expect([self.prompt_compiled,
> > @@ -416,10 +463,10 @@ class ConsoleBase(object):
> >               pass
> >           self.p = None
> >
> > -    def restart_uboot(self):
> > +    def restart_uboot(self, expect_earlyreset=False):
> >           """Shut down and restart U-Boot."""
> >           self.cleanup_spawn()
> > -        self.ensure_spawned()
> > +        self.ensure_spawned(expect_earlyreset)
> >
> >       def get_spawn_output(self):
> >           """Return the start-up output from U-Boot
> > diff --git a/test/py/u_boot_console_sandbox.py b/test/py/u_boot_console_sandbox.py
> > index 7e1eb0e0b4..9cd9ccad30 100644
> > --- a/test/py/u_boot_console_sandbox.py
> > +++ b/test/py/u_boot_console_sandbox.py
> > @@ -57,11 +57,13 @@ class ConsoleSandbox(ConsoleBase):
> >           cmd += self.sandbox_flags
> >           return Spawn(cmd, cwd=self.config.source_dir)
> >
> > -    def restart_uboot_with_flags(self, flags):
> > +    def restart_uboot_with_flags(self, flags, expect_earlyreset=False):
> >           """Run U-Boot with the given command-line flags
> >
> >           Args:
> >               flags: List of flags to pass, each a string
> > +            expect_earlyreset: This boot is expected to be reset in early
> > +                stage (before prompt). False by default.
> >
> >           Returns:
> >               A u_boot_spawn.Spawn object that is attached to U-Boot.
> > @@ -69,7 +71,7 @@ class ConsoleSandbox(ConsoleBase):
> >
> >           try:
> >               self.sandbox_flags = flags
> > -            return self.restart_uboot()
> > +            return self.restart_uboot(expect_earlyreset)
> >           finally:
> >               self.sandbox_flags = []
> >
> >
>
AKASHI Takahiro Feb. 16, 2022, 1:34 a.m. UTC | #6
On Tue, Feb 15, 2022 at 06:05:50PM +0900, Masami Hiramatsu wrote:
> Since now the capsule_on_disk will restart the u-boot sandbox right
> after the capsule update, if CONFIG_EFI_CAPSULE_ON_DISK_EARLY=y, the
> boot with a new capsule file will repeat reboot sequence. On the
> other hand, if CONFIG_EFI_CAPSULE_ON_DISK_EARLY=n, the 'env print -e'
> command will execute the capsule update on disk and reboot.
> 
> Thus this update the uboot_console for those 2 cases;
> 
>  - restart_uboot(): Add expect_earlyreset optional parameter so that
>    it can handle the reboot while booting.
>  - run_command(): Add wait_for_reboot optional parameter so that it
>    can handle the reboot after executing a command.
> 
> And enable those options in the test_capsule_firmware.py test cases.
> 
> Signed-off-by: Masami Hiramatsu <masami.hiramatsu@linaro.org>
> ---
>  .../test_efi_capsule/test_capsule_firmware.py      |   39 ++++++--
>  test/py/u_boot_console_base.py                     |   95 +++++++++++++++-----
>  test/py/u_boot_console_sandbox.py                  |    6 +
>  3 files changed, 102 insertions(+), 38 deletions(-)
> 
> diff --git a/test/py/tests/test_efi_capsule/test_capsule_firmware.py b/test/py/tests/test_efi_capsule/test_capsule_firmware.py
> index 6e803f699f..a539134ec2 100644
> --- a/test/py/tests/test_efi_capsule/test_capsule_firmware.py
> +++ b/test/py/tests/test_efi_capsule/test_capsule_firmware.py
> @@ -143,13 +143,14 @@ class TestEfiCapsuleFirmwareFit(object):
>                  'fatls host 0:1 %s' % CAPSULE_INSTALL_DIR])
>              assert 'Test01' in ''.join(output)
>  
> -        # reboot
> -        u_boot_console.restart_uboot()
> -
>          capsule_early = u_boot_config.buildconfig.get(
>              'config_efi_capsule_on_disk_early')
>          capsule_auth = u_boot_config.buildconfig.get(
>              'config_efi_capsule_authenticate')
> +
> +        # reboot
> +        u_boot_console.restart_uboot(expect_earlyreset = capsule_early)
> +
>          with u_boot_console.log.section('Test Case 2-b, after reboot'):
>              if not capsule_early:
>                  # make sure that dfu_alt_info exists even persistent variables
> @@ -162,7 +163,7 @@ class TestEfiCapsuleFirmwareFit(object):
>  
>                  # need to run uefi command to initiate capsule handling
>                  output = u_boot_console.run_command(
> -                    'env print -e Capsule0000')
> +                    'env print -e Capsule0000', wait_for_reboot = True)
>  
>              output = u_boot_console.run_command_list([
>                  'host bind 0 %s' % disk_img,
> @@ -218,13 +219,14 @@ class TestEfiCapsuleFirmwareFit(object):
>                  'fatls host 0:1 %s' % CAPSULE_INSTALL_DIR])
>              assert 'Test02' in ''.join(output)
>  
> -        # reboot
> -        u_boot_console.restart_uboot()
> -
>          capsule_early = u_boot_config.buildconfig.get(
>              'config_efi_capsule_on_disk_early')
>          capsule_auth = u_boot_config.buildconfig.get(
>              'config_efi_capsule_authenticate')
> +
> +        # reboot
> +        u_boot_console.restart_uboot(expect_earlyreset = capsule_early)
> +
>          with u_boot_console.log.section('Test Case 3-b, after reboot'):
>              if not capsule_early:
>                  # make sure that dfu_alt_info exists even persistent variables
> @@ -237,7 +239,13 @@ class TestEfiCapsuleFirmwareFit(object):
>  
>                  # need to run uefi command to initiate capsule handling
>                  output = u_boot_console.run_command(
> -                    'env print -e Capsule0000')
> +                    'env print -e Capsule0000', wait_for_reboot = True)
> +
> +            output = u_boot_console.run_command_list([
> +                'env set dfu_alt_info "sf 0:0=u-boot-bin raw 0x100000 0x50000;u-boot-env raw 0x150000 0x200000"',

Probably most people don't understand why we need dfu_alt_info here.
It would be better to merge it to "efidebug capsule esrt" and
leave a comment.

> +                'host bind 0 %s' % disk_img,
> +                'fatls host 0:1 %s' % CAPSULE_INSTALL_DIR])
> +            assert 'Test02' not in ''.join(output)

Anyhow, this assertion exists below in this test case scenario, doesn't it?

>              output = u_boot_console.run_command_list(['efidebug capsule esrt'])
>  
> @@ -293,13 +301,14 @@ class TestEfiCapsuleFirmwareFit(object):
>                  'fatls host 0:1 %s' % CAPSULE_INSTALL_DIR])
>              assert 'Test03' in ''.join(output)
>  
> -        # reboot
> -        u_boot_console.restart_uboot()
> -
>          capsule_early = u_boot_config.buildconfig.get(
>              'config_efi_capsule_on_disk_early')
>          capsule_auth = u_boot_config.buildconfig.get(
>              'config_efi_capsule_authenticate')
> +
> +        # reboot
> +        u_boot_console.restart_uboot(expect_earlyreset = capsule_early)
> +
>          with u_boot_console.log.section('Test Case 4-b, after reboot'):
>              if not capsule_early:
>                  # make sure that dfu_alt_info exists even persistent variables
> @@ -312,7 +321,13 @@ class TestEfiCapsuleFirmwareFit(object):
>  
>                  # need to run uefi command to initiate capsule handling
>                  output = u_boot_console.run_command(
> -                    'env print -e Capsule0000')
> +                    'env print -e Capsule0000', wait_for_reboot = True)
> +
> +            output = u_boot_console.run_command_list([
> +                'env set dfu_alt_info "sf 0:0=u-boot-bin raw 0x100000 0x50000;u-boot-env raw 0x150000 0x200000"',
> +                'host bind 0 %s' % disk_img,
> +                'fatls host 0:1 %s' % CAPSULE_INSTALL_DIR])
> +            assert 'Test03' not in ''.join(output)
>  
>              output = u_boot_console.run_command_list(['efidebug capsule esrt'])
>  
> diff --git a/test/py/u_boot_console_base.py b/test/py/u_boot_console_base.py
> index 384fd53c65..e84f4d74ef 100644
> --- a/test/py/u_boot_console_base.py
> +++ b/test/py/u_boot_console_base.py
> @@ -140,7 +140,7 @@ class ConsoleBase(object):
>          self.logstream.close()
>  
>      def run_command(self, cmd, wait_for_echo=True, send_nl=True,
> -            wait_for_prompt=True):
> +            wait_for_prompt=True, wait_for_reboot=False):
>          """Execute a command via the U-Boot console.
>  
>          The command is always sent to U-Boot.
> @@ -168,6 +168,8 @@ class ConsoleBase(object):
>              wait_for_prompt: Boolean indicating whether to wait for the
>                  command prompt to be sent by U-Boot. This typically occurs
>                  immediately after the command has been executed.
> +            wait_for_reboot: Boolean indication whether to wait for the
> +                reboot U-Boot. If this is True, wait_for_prompt is ignored.

Umm, if so,

>  
>          Returns:
>              If wait_for_prompt == False:
> @@ -202,11 +204,48 @@ class ConsoleBase(object):
>                                      self.bad_pattern_ids[m - 1])
>              if not wait_for_prompt:
>                  return

This check must be pushed backward after "if wait_for_reboot".

> -            m = self.p.expect([self.prompt_compiled] + self.bad_patterns)
> -            if m != 0:
> -                self.at_prompt = False
> -                raise Exception('Bad pattern found on console: ' +
> -                                self.bad_pattern_ids[m - 1])
> +            if wait_for_reboot:
> +                bcfg = self.config.buildconfig
> +                config_spl = bcfg.get('config_spl', 'n') == 'y'
> +                config_spl_serial = bcfg.get('config_spl_serial',
> +                                                 'n') == 'y'
> +                env_spl_skipped = self.config.env.get('env__spl_skipped',
> +                                                      False)
> +                env_spl2_skipped = self.config.env.get('env__spl2_skipped',
> +                                                       True)
> +                if config_spl and config_spl_serial and not env_spl_skipped:
> +                    m = self.p.expect([pattern_u_boot_spl_signon] +
> +                                      self.bad_patterns)
> +                    if m != 0:
> +                        raise Exception('Bad pattern found on SPL console: ' +
> +                                        self.bad_pattern_ids[m - 1])
> +                if not env_spl2_skipped:
> +                    m = self.p.expect([pattern_u_boot_spl2_signon] +
> +                                      self.bad_patterns)
> +                    if m != 0:
> +                        raise Exception('Bad pattern found on SPL2 console: ' +
> +                                        self.bad_pattern_ids[m - 1])
> +                m = self.p.expect([pattern_u_boot_main_signon] + self.bad_patterns)
> +                if m != 0:
> +                    raise Exception('Bad pattern found on console: ' +
> +                                    self.bad_pattern_ids[m - 1])
> +                self.u_boot_version_string = self.p.after
> +                while True:
> +                    m = self.p.expect([self.prompt_compiled,
> +                        pattern_stop_autoboot_prompt] + self.bad_patterns)
> +                    if m == 0:
> +                        break
> +                    if m == 1:
> +                        self.p.send(' ')
> +                        continue
> +                    raise Exception('Bad pattern found on console: ' +
> +                                    self.bad_pattern_ids[m - 2])
> +            else:
> +                m = self.p.expect([self.prompt_compiled] + self.bad_patterns)
> +                if m != 0:
> +                    self.at_prompt = False
> +                    raise Exception('Bad pattern found on console: ' +
> +                                    self.bad_pattern_ids[m - 1])

This part of hunk is somehow duplicated in ensure_spawned() below
except loop_num. Please rework the code if possible.

-Takahiro Akashi

>              self.at_prompt = True
>              self.at_prompt_logevt = self.logstream.logfile.cur_evt
>              # Only strip \r\n; space/TAB might be significant if testing
> @@ -321,7 +360,7 @@ class ConsoleBase(object):
>          finally:
>              self.p.timeout = orig_timeout
>  
> -    def ensure_spawned(self):
> +    def ensure_spawned(self, expect_earlyreset=False):
>          """Ensure a connection to a correctly running U-Boot instance.
>  
>          This may require spawning a new Sandbox process or resetting target
> @@ -330,7 +369,8 @@ class ConsoleBase(object):
>          This is an internal function and should not be called directly.
>  
>          Args:
> -            None.
> +            expect_earlyreset: This boot is expected to be reset in early
> +                stage (before prompt). False by default.
>  
>          Returns:
>              Nothing.
> @@ -357,22 +397,29 @@ class ConsoleBase(object):
>                                                    False)
>              env_spl2_skipped = self.config.env.get('env__spl2_skipped',
>                                                    True)
> -            if config_spl and config_spl_serial and not env_spl_skipped:
> -                m = self.p.expect([pattern_u_boot_spl_signon] +
> -                                  self.bad_patterns)
> +            if expect_earlyreset:
> +                loop_num = 2
> +            else:
> +                loop_num = 1
> +
> +            while loop_num > 0:
> +                loop_num -= 1
> +                if config_spl and config_spl_serial and not env_spl_skipped:
> +                    m = self.p.expect([pattern_u_boot_spl_signon] +
> +                                      self.bad_patterns)
> +                    if m != 0:
> +                        raise Exception('Bad pattern found on SPL console: ' +
> +                                        self.bad_pattern_ids[m - 1])
> +                if not env_spl2_skipped:
> +                    m = self.p.expect([pattern_u_boot_spl2_signon] +
> +                                      self.bad_patterns)
> +                    if m != 0:
> +                        raise Exception('Bad pattern found on SPL2 console: ' +
> +                                        self.bad_pattern_ids[m - 1])
> +                m = self.p.expect([pattern_u_boot_main_signon] + self.bad_patterns)
>                  if m != 0:
> -                    raise Exception('Bad pattern found on SPL console: ' +
> -                                    self.bad_pattern_ids[m - 1])
> -            if not env_spl2_skipped:
> -                m = self.p.expect([pattern_u_boot_spl2_signon] +
> -                                  self.bad_patterns)
> -                if m != 0:
> -                    raise Exception('Bad pattern found on SPL2 console: ' +
> +                    raise Exception('Bad pattern found on console: ' +
>                                      self.bad_pattern_ids[m - 1])
> -            m = self.p.expect([pattern_u_boot_main_signon] + self.bad_patterns)
> -            if m != 0:
> -                raise Exception('Bad pattern found on console: ' +
> -                                self.bad_pattern_ids[m - 1])
>              self.u_boot_version_string = self.p.after
>              while True:
>                  m = self.p.expect([self.prompt_compiled,
> @@ -416,10 +463,10 @@ class ConsoleBase(object):
>              pass
>          self.p = None
>  
> -    def restart_uboot(self):
> +    def restart_uboot(self, expect_earlyreset=False):
>          """Shut down and restart U-Boot."""
>          self.cleanup_spawn()
> -        self.ensure_spawned()
> +        self.ensure_spawned(expect_earlyreset)
>  
>      def get_spawn_output(self):
>          """Return the start-up output from U-Boot
> diff --git a/test/py/u_boot_console_sandbox.py b/test/py/u_boot_console_sandbox.py
> index 7e1eb0e0b4..9cd9ccad30 100644
> --- a/test/py/u_boot_console_sandbox.py
> +++ b/test/py/u_boot_console_sandbox.py
> @@ -57,11 +57,13 @@ class ConsoleSandbox(ConsoleBase):
>          cmd += self.sandbox_flags
>          return Spawn(cmd, cwd=self.config.source_dir)
>  
> -    def restart_uboot_with_flags(self, flags):
> +    def restart_uboot_with_flags(self, flags, expect_earlyreset=False):
>          """Run U-Boot with the given command-line flags
>  
>          Args:
>              flags: List of flags to pass, each a string
> +            expect_earlyreset: This boot is expected to be reset in early
> +                stage (before prompt). False by default.
>  
>          Returns:
>              A u_boot_spawn.Spawn object that is attached to U-Boot.
> @@ -69,7 +71,7 @@ class ConsoleSandbox(ConsoleBase):
>  
>          try:
>              self.sandbox_flags = flags
> -            return self.restart_uboot()
> +            return self.restart_uboot(expect_earlyreset)
>          finally:
>              self.sandbox_flags = []
>  
>
Masami Hiramatsu Feb. 16, 2022, 1:46 a.m. UTC | #7
Hi Takahiro,

2022年2月16日(水) 10:35 AKASHI Takahiro <takahiro.akashi@linaro.org>:
>
> On Tue, Feb 15, 2022 at 06:05:50PM +0900, Masami Hiramatsu wrote:
> > Since now the capsule_on_disk will restart the u-boot sandbox right
> > after the capsule update, if CONFIG_EFI_CAPSULE_ON_DISK_EARLY=y, the
> > boot with a new capsule file will repeat reboot sequence. On the
> > other hand, if CONFIG_EFI_CAPSULE_ON_DISK_EARLY=n, the 'env print -e'
> > command will execute the capsule update on disk and reboot.
> >
> > Thus this update the uboot_console for those 2 cases;
> >
> >  - restart_uboot(): Add expect_earlyreset optional parameter so that
> >    it can handle the reboot while booting.
> >  - run_command(): Add wait_for_reboot optional parameter so that it
> >    can handle the reboot after executing a command.
> >
> > And enable those options in the test_capsule_firmware.py test cases.
> >
> > Signed-off-by: Masami Hiramatsu <masami.hiramatsu@linaro.org>
> > ---
> >  .../test_efi_capsule/test_capsule_firmware.py      |   39 ++++++--
> >  test/py/u_boot_console_base.py                     |   95 +++++++++++++++-----
> >  test/py/u_boot_console_sandbox.py                  |    6 +
> >  3 files changed, 102 insertions(+), 38 deletions(-)
> >
> > diff --git a/test/py/tests/test_efi_capsule/test_capsule_firmware.py b/test/py/tests/test_efi_capsule/test_capsule_firmware.py
> > index 6e803f699f..a539134ec2 100644
> > --- a/test/py/tests/test_efi_capsule/test_capsule_firmware.py
> > +++ b/test/py/tests/test_efi_capsule/test_capsule_firmware.py
> > @@ -143,13 +143,14 @@ class TestEfiCapsuleFirmwareFit(object):
> >                  'fatls host 0:1 %s' % CAPSULE_INSTALL_DIR])
> >              assert 'Test01' in ''.join(output)
> >
> > -        # reboot
> > -        u_boot_console.restart_uboot()
> > -
> >          capsule_early = u_boot_config.buildconfig.get(
> >              'config_efi_capsule_on_disk_early')
> >          capsule_auth = u_boot_config.buildconfig.get(
> >              'config_efi_capsule_authenticate')
> > +
> > +        # reboot
> > +        u_boot_console.restart_uboot(expect_earlyreset = capsule_early)
> > +
> >          with u_boot_console.log.section('Test Case 2-b, after reboot'):
> >              if not capsule_early:
> >                  # make sure that dfu_alt_info exists even persistent variables
> > @@ -162,7 +163,7 @@ class TestEfiCapsuleFirmwareFit(object):
> >
> >                  # need to run uefi command to initiate capsule handling
> >                  output = u_boot_console.run_command(
> > -                    'env print -e Capsule0000')
> > +                    'env print -e Capsule0000', wait_for_reboot = True)
> >
> >              output = u_boot_console.run_command_list([
> >                  'host bind 0 %s' % disk_img,
> > @@ -218,13 +219,14 @@ class TestEfiCapsuleFirmwareFit(object):
> >                  'fatls host 0:1 %s' % CAPSULE_INSTALL_DIR])
> >              assert 'Test02' in ''.join(output)
> >
> > -        # reboot
> > -        u_boot_console.restart_uboot()
> > -
> >          capsule_early = u_boot_config.buildconfig.get(
> >              'config_efi_capsule_on_disk_early')
> >          capsule_auth = u_boot_config.buildconfig.get(
> >              'config_efi_capsule_authenticate')
> > +
> > +        # reboot
> > +        u_boot_console.restart_uboot(expect_earlyreset = capsule_early)
> > +
> >          with u_boot_console.log.section('Test Case 3-b, after reboot'):
> >              if not capsule_early:
> >                  # make sure that dfu_alt_info exists even persistent variables
> > @@ -237,7 +239,13 @@ class TestEfiCapsuleFirmwareFit(object):
> >
> >                  # need to run uefi command to initiate capsule handling
> >                  output = u_boot_console.run_command(
> > -                    'env print -e Capsule0000')
> > +                    'env print -e Capsule0000', wait_for_reboot = True)
> > +
> > +            output = u_boot_console.run_command_list([
> > +                'env set dfu_alt_info "sf 0:0=u-boot-bin raw 0x100000 0x50000;u-boot-env raw 0x150000 0x200000"',
>
> Probably most people don't understand why we need dfu_alt_info here.
> It would be better to merge it to "efidebug capsule esrt" and
> leave a comment.

Sure.

>
> > +                'host bind 0 %s' % disk_img,
> > +                'fatls host 0:1 %s' % CAPSULE_INSTALL_DIR])
> > +            assert 'Test02' not in ''.join(output)
>
> Anyhow, this assertion exists below in this test case scenario, doesn't it?

Oops, I missed that. OK, let me remove it.

>
> >              output = u_boot_console.run_command_list(['efidebug capsule esrt'])
> >
> > @@ -293,13 +301,14 @@ class TestEfiCapsuleFirmwareFit(object):
> >                  'fatls host 0:1 %s' % CAPSULE_INSTALL_DIR])
> >              assert 'Test03' in ''.join(output)
> >
> > -        # reboot
> > -        u_boot_console.restart_uboot()
> > -
> >          capsule_early = u_boot_config.buildconfig.get(
> >              'config_efi_capsule_on_disk_early')
> >          capsule_auth = u_boot_config.buildconfig.get(
> >              'config_efi_capsule_authenticate')
> > +
> > +        # reboot
> > +        u_boot_console.restart_uboot(expect_earlyreset = capsule_early)
> > +
> >          with u_boot_console.log.section('Test Case 4-b, after reboot'):
> >              if not capsule_early:
> >                  # make sure that dfu_alt_info exists even persistent variables
> > @@ -312,7 +321,13 @@ class TestEfiCapsuleFirmwareFit(object):
> >
> >                  # need to run uefi command to initiate capsule handling
> >                  output = u_boot_console.run_command(
> > -                    'env print -e Capsule0000')
> > +                    'env print -e Capsule0000', wait_for_reboot = True)
> > +
> > +            output = u_boot_console.run_command_list([
> > +                'env set dfu_alt_info "sf 0:0=u-boot-bin raw 0x100000 0x50000;u-boot-env raw 0x150000 0x200000"',
> > +                'host bind 0 %s' % disk_img,
> > +                'fatls host 0:1 %s' % CAPSULE_INSTALL_DIR])
> > +            assert 'Test03' not in ''.join(output)
> >
> >              output = u_boot_console.run_command_list(['efidebug capsule esrt'])
> >
> > diff --git a/test/py/u_boot_console_base.py b/test/py/u_boot_console_base.py
> > index 384fd53c65..e84f4d74ef 100644
> > --- a/test/py/u_boot_console_base.py
> > +++ b/test/py/u_boot_console_base.py
> > @@ -140,7 +140,7 @@ class ConsoleBase(object):
> >          self.logstream.close()
> >
> >      def run_command(self, cmd, wait_for_echo=True, send_nl=True,
> > -            wait_for_prompt=True):
> > +            wait_for_prompt=True, wait_for_reboot=False):
> >          """Execute a command via the U-Boot console.
> >
> >          The command is always sent to U-Boot.
> > @@ -168,6 +168,8 @@ class ConsoleBase(object):
> >              wait_for_prompt: Boolean indicating whether to wait for the
> >                  command prompt to be sent by U-Boot. This typically occurs
> >                  immediately after the command has been executed.
> > +            wait_for_reboot: Boolean indication whether to wait for the
> > +                reboot U-Boot. If this is True, wait_for_prompt is ignored.
>
> Umm, if so,
>
> >
> >          Returns:
> >              If wait_for_prompt == False:
> > @@ -202,11 +204,48 @@ class ConsoleBase(object):
> >                                      self.bad_pattern_ids[m - 1])
> >              if not wait_for_prompt:
> >                  return
>
> This check must be pushed backward after "if wait_for_reboot".

Oops, sorry, the description is wrong. Actually wait_for_prompt must be
True for wait_for_reboot because wait_for_reboot will wait for the
prompt after reboot.

>
> > -            m = self.p.expect([self.prompt_compiled] + self.bad_patterns)
> > -            if m != 0:
> > -                self.at_prompt = False
> > -                raise Exception('Bad pattern found on console: ' +
> > -                                self.bad_pattern_ids[m - 1])
> > +            if wait_for_reboot:
> > +                bcfg = self.config.buildconfig
> > +                config_spl = bcfg.get('config_spl', 'n') == 'y'
> > +                config_spl_serial = bcfg.get('config_spl_serial',
> > +                                                 'n') == 'y'
> > +                env_spl_skipped = self.config.env.get('env__spl_skipped',
> > +                                                      False)
> > +                env_spl2_skipped = self.config.env.get('env__spl2_skipped',
> > +                                                       True)
> > +                if config_spl and config_spl_serial and not env_spl_skipped:
> > +                    m = self.p.expect([pattern_u_boot_spl_signon] +
> > +                                      self.bad_patterns)
> > +                    if m != 0:
> > +                        raise Exception('Bad pattern found on SPL console: ' +
> > +                                        self.bad_pattern_ids[m - 1])
> > +                if not env_spl2_skipped:
> > +                    m = self.p.expect([pattern_u_boot_spl2_signon] +
> > +                                      self.bad_patterns)
> > +                    if m != 0:
> > +                        raise Exception('Bad pattern found on SPL2 console: ' +
> > +                                        self.bad_pattern_ids[m - 1])
> > +                m = self.p.expect([pattern_u_boot_main_signon] + self.bad_patterns)
> > +                if m != 0:
> > +                    raise Exception('Bad pattern found on console: ' +
> > +                                    self.bad_pattern_ids[m - 1])
> > +                self.u_boot_version_string = self.p.after
> > +                while True:
> > +                    m = self.p.expect([self.prompt_compiled,
> > +                        pattern_stop_autoboot_prompt] + self.bad_patterns)
> > +                    if m == 0:
> > +                        break
> > +                    if m == 1:
> > +                        self.p.send(' ')
> > +                        continue
> > +                    raise Exception('Bad pattern found on console: ' +
> > +                                    self.bad_pattern_ids[m - 2])
> > +            else:
> > +                m = self.p.expect([self.prompt_compiled] + self.bad_patterns)
> > +                if m != 0:
> > +                    self.at_prompt = False
> > +                    raise Exception('Bad pattern found on console: ' +
> > +                                    self.bad_pattern_ids[m - 1])
>
> This part of hunk is somehow duplicated in ensure_spawned() below
> except loop_num. Please rework the code if possible.

Yes, OK, I'll refactor this part out and reuse it from both methods.

Thank you!

>
> -Takahiro Akashi
>
> >              self.at_prompt = True
> >              self.at_prompt_logevt = self.logstream.logfile.cur_evt
> >              # Only strip \r\n; space/TAB might be significant if testing
> > @@ -321,7 +360,7 @@ class ConsoleBase(object):
> >          finally:
> >              self.p.timeout = orig_timeout
> >
> > -    def ensure_spawned(self):
> > +    def ensure_spawned(self, expect_earlyreset=False):
> >          """Ensure a connection to a correctly running U-Boot instance.
> >
> >          This may require spawning a new Sandbox process or resetting target
> > @@ -330,7 +369,8 @@ class ConsoleBase(object):
> >          This is an internal function and should not be called directly.
> >
> >          Args:
> > -            None.
> > +            expect_earlyreset: This boot is expected to be reset in early
> > +                stage (before prompt). False by default.
> >
> >          Returns:
> >              Nothing.
> > @@ -357,22 +397,29 @@ class ConsoleBase(object):
> >                                                    False)
> >              env_spl2_skipped = self.config.env.get('env__spl2_skipped',
> >                                                    True)
> > -            if config_spl and config_spl_serial and not env_spl_skipped:
> > -                m = self.p.expect([pattern_u_boot_spl_signon] +
> > -                                  self.bad_patterns)
> > +            if expect_earlyreset:
> > +                loop_num = 2
> > +            else:
> > +                loop_num = 1
> > +
> > +            while loop_num > 0:
> > +                loop_num -= 1
> > +                if config_spl and config_spl_serial and not env_spl_skipped:
> > +                    m = self.p.expect([pattern_u_boot_spl_signon] +
> > +                                      self.bad_patterns)
> > +                    if m != 0:
> > +                        raise Exception('Bad pattern found on SPL console: ' +
> > +                                        self.bad_pattern_ids[m - 1])
> > +                if not env_spl2_skipped:
> > +                    m = self.p.expect([pattern_u_boot_spl2_signon] +
> > +                                      self.bad_patterns)
> > +                    if m != 0:
> > +                        raise Exception('Bad pattern found on SPL2 console: ' +
> > +                                        self.bad_pattern_ids[m - 1])
> > +                m = self.p.expect([pattern_u_boot_main_signon] + self.bad_patterns)
> >                  if m != 0:
> > -                    raise Exception('Bad pattern found on SPL console: ' +
> > -                                    self.bad_pattern_ids[m - 1])
> > -            if not env_spl2_skipped:
> > -                m = self.p.expect([pattern_u_boot_spl2_signon] +
> > -                                  self.bad_patterns)
> > -                if m != 0:
> > -                    raise Exception('Bad pattern found on SPL2 console: ' +
> > +                    raise Exception('Bad pattern found on console: ' +
> >                                      self.bad_pattern_ids[m - 1])
> > -            m = self.p.expect([pattern_u_boot_main_signon] + self.bad_patterns)
> > -            if m != 0:
> > -                raise Exception('Bad pattern found on console: ' +
> > -                                self.bad_pattern_ids[m - 1])
> >              self.u_boot_version_string = self.p.after
> >              while True:
> >                  m = self.p.expect([self.prompt_compiled,
> > @@ -416,10 +463,10 @@ class ConsoleBase(object):
> >              pass
> >          self.p = None
> >
> > -    def restart_uboot(self):
> > +    def restart_uboot(self, expect_earlyreset=False):
> >          """Shut down and restart U-Boot."""
> >          self.cleanup_spawn()
> > -        self.ensure_spawned()
> > +        self.ensure_spawned(expect_earlyreset)
> >
> >      def get_spawn_output(self):
> >          """Return the start-up output from U-Boot
> > diff --git a/test/py/u_boot_console_sandbox.py b/test/py/u_boot_console_sandbox.py
> > index 7e1eb0e0b4..9cd9ccad30 100644
> > --- a/test/py/u_boot_console_sandbox.py
> > +++ b/test/py/u_boot_console_sandbox.py
> > @@ -57,11 +57,13 @@ class ConsoleSandbox(ConsoleBase):
> >          cmd += self.sandbox_flags
> >          return Spawn(cmd, cwd=self.config.source_dir)
> >
> > -    def restart_uboot_with_flags(self, flags):
> > +    def restart_uboot_with_flags(self, flags, expect_earlyreset=False):
> >          """Run U-Boot with the given command-line flags
> >
> >          Args:
> >              flags: List of flags to pass, each a string
> > +            expect_earlyreset: This boot is expected to be reset in early
> > +                stage (before prompt). False by default.
> >
> >          Returns:
> >              A u_boot_spawn.Spawn object that is attached to U-Boot.
> > @@ -69,7 +71,7 @@ class ConsoleSandbox(ConsoleBase):
> >
> >          try:
> >              self.sandbox_flags = flags
> > -            return self.restart_uboot()
> > +            return self.restart_uboot(expect_earlyreset)
> >          finally:
> >              self.sandbox_flags = []
> >
> >
Simon Glass Feb. 16, 2022, 3:26 p.m. UTC | #8
Hi Masami,

On Tue, 15 Feb 2022 at 02:05, Masami Hiramatsu
<masami.hiramatsu@linaro.org> wrote:
>
> Since now the capsule_on_disk will restart the u-boot sandbox right
> after the capsule update, if CONFIG_EFI_CAPSULE_ON_DISK_EARLY=y, the
> boot with a new capsule file will repeat reboot sequence. On the
> other hand, if CONFIG_EFI_CAPSULE_ON_DISK_EARLY=n, the 'env print -e'
> command will execute the capsule update on disk and reboot.
>
> Thus this update the uboot_console for those 2 cases;
>
>  - restart_uboot(): Add expect_earlyreset optional parameter so that
>    it can handle the reboot while booting.
>  - run_command(): Add wait_for_reboot optional parameter so that it
>    can handle the reboot after executing a command.
>
> And enable those options in the test_capsule_firmware.py test cases.
>
> Signed-off-by: Masami Hiramatsu <masami.hiramatsu@linaro.org>
> ---
>  .../test_efi_capsule/test_capsule_firmware.py      |   39 ++++++--
>  test/py/u_boot_console_base.py                     |   95 +++++++++++++++-----
>  test/py/u_boot_console_sandbox.py                  |    6 +
>  3 files changed, 102 insertions(+), 38 deletions(-)

We have a means to avoid actually doing the reset, see the reset driver.

PLEASE use that instead of adding all this code. Also make sure that
test works with 'make qcheck' too.

Regards,
Simon
Heinrich Schuchardt Feb. 16, 2022, 3:32 p.m. UTC | #9
On 2/16/22 16:26, Simon Glass wrote:
> Hi Masami,
>
> On Tue, 15 Feb 2022 at 02:05, Masami Hiramatsu
> <masami.hiramatsu@linaro.org> wrote:
>>
>> Since now the capsule_on_disk will restart the u-boot sandbox right
>> after the capsule update, if CONFIG_EFI_CAPSULE_ON_DISK_EARLY=y, the
>> boot with a new capsule file will repeat reboot sequence. On the
>> other hand, if CONFIG_EFI_CAPSULE_ON_DISK_EARLY=n, the 'env print -e'
>> command will execute the capsule update on disk and reboot.
>>
>> Thus this update the uboot_console for those 2 cases;
>>
>>   - restart_uboot(): Add expect_earlyreset optional parameter so that
>>     it can handle the reboot while booting.
>>   - run_command(): Add wait_for_reboot optional parameter so that it
>>     can handle the reboot after executing a command.
>>
>> And enable those options in the test_capsule_firmware.py test cases.
>>
>> Signed-off-by: Masami Hiramatsu <masami.hiramatsu@linaro.org>
>> ---
>>   .../test_efi_capsule/test_capsule_firmware.py      |   39 ++++++--
>>   test/py/u_boot_console_base.py                     |   95 +++++++++++++++-----
>>   test/py/u_boot_console_sandbox.py                  |    6 +
>>   3 files changed, 102 insertions(+), 38 deletions(-)
>
> We have a means to avoid actually doing the reset, see the reset driver.

The UEFI specification requires a cold reset after a capsule is updated
and before the console is reached. How could the reset driver help to
fix the Python tests?

Best regards

Heinrich

>
> PLEASE use that instead of adding all this code. Also make sure that
> test works with 'make qcheck' too.
>
> Regards,
> Simon
Tom Rini Feb. 16, 2022, 3:46 p.m. UTC | #10
On Wed, Feb 16, 2022 at 04:32:40PM +0100, Heinrich Schuchardt wrote:
> On 2/16/22 16:26, Simon Glass wrote:
> > Hi Masami,
> > 
> > On Tue, 15 Feb 2022 at 02:05, Masami Hiramatsu
> > <masami.hiramatsu@linaro.org> wrote:
> > > 
> > > Since now the capsule_on_disk will restart the u-boot sandbox right
> > > after the capsule update, if CONFIG_EFI_CAPSULE_ON_DISK_EARLY=y, the
> > > boot with a new capsule file will repeat reboot sequence. On the
> > > other hand, if CONFIG_EFI_CAPSULE_ON_DISK_EARLY=n, the 'env print -e'
> > > command will execute the capsule update on disk and reboot.
> > > 
> > > Thus this update the uboot_console for those 2 cases;
> > > 
> > >   - restart_uboot(): Add expect_earlyreset optional parameter so that
> > >     it can handle the reboot while booting.
> > >   - run_command(): Add wait_for_reboot optional parameter so that it
> > >     can handle the reboot after executing a command.
> > > 
> > > And enable those options in the test_capsule_firmware.py test cases.
> > > 
> > > Signed-off-by: Masami Hiramatsu <masami.hiramatsu@linaro.org>
> > > ---
> > >   .../test_efi_capsule/test_capsule_firmware.py      |   39 ++++++--
> > >   test/py/u_boot_console_base.py                     |   95 +++++++++++++++-----
> > >   test/py/u_boot_console_sandbox.py                  |    6 +
> > >   3 files changed, 102 insertions(+), 38 deletions(-)
> > 
> > We have a means to avoid actually doing the reset, see the reset driver.
> 
> The UEFI specification requires a cold reset after a capsule is updated
> and before the console is reached. How could the reset driver help to
> fix the Python tests?

Is this test going to be able to run on qemu, sandbox, real hardware, or
all 3?  The tests may well end up having to know a bit more, sadly,
about the type of system they're testing.
Heinrich Schuchardt Feb. 16, 2022, 5:45 p.m. UTC | #11
On 2/16/22 16:46, Tom Rini wrote:
> On Wed, Feb 16, 2022 at 04:32:40PM +0100, Heinrich Schuchardt wrote:
>> On 2/16/22 16:26, Simon Glass wrote:
>>> Hi Masami,
>>>
>>> On Tue, 15 Feb 2022 at 02:05, Masami Hiramatsu
>>> <masami.hiramatsu@linaro.org> wrote:
>>>>
>>>> Since now the capsule_on_disk will restart the u-boot sandbox right
>>>> after the capsule update, if CONFIG_EFI_CAPSULE_ON_DISK_EARLY=y, the
>>>> boot with a new capsule file will repeat reboot sequence. On the
>>>> other hand, if CONFIG_EFI_CAPSULE_ON_DISK_EARLY=n, the 'env print -e'
>>>> command will execute the capsule update on disk and reboot.
>>>>
>>>> Thus this update the uboot_console for those 2 cases;
>>>>
>>>>    - restart_uboot(): Add expect_earlyreset optional parameter so that
>>>>      it can handle the reboot while booting.
>>>>    - run_command(): Add wait_for_reboot optional parameter so that it
>>>>      can handle the reboot after executing a command.
>>>>
>>>> And enable those options in the test_capsule_firmware.py test cases.
>>>>
>>>> Signed-off-by: Masami Hiramatsu <masami.hiramatsu@linaro.org>
>>>> ---
>>>>    .../test_efi_capsule/test_capsule_firmware.py      |   39 ++++++--
>>>>    test/py/u_boot_console_base.py                     |   95 +++++++++++++++-----
>>>>    test/py/u_boot_console_sandbox.py                  |    6 +
>>>>    3 files changed, 102 insertions(+), 38 deletions(-)
>>>
>>> We have a means to avoid actually doing the reset, see the reset driver.
>>
>> The UEFI specification requires a cold reset after a capsule is updated
>> and before the console is reached. How could the reset driver help to
>> fix the Python tests?
>
> Is this test going to be able to run on qemu, sandbox, real hardware, or
> all 3?  The tests may well end up having to know a bit more, sadly,
> about the type of system they're testing.
>
Currently the test will only run on the sandbox in Gitlab (see usage of
@pytest.mark.boardspec('sandbox') in test/py/tests/test_efi_capsule/).

You will not want to update the firmware on real hardware especially if
there is a rollback protection. But testing on QEMU would make sense.

Best regards

Heinrich
Simon Glass Feb. 16, 2022, 5:46 p.m. UTC | #12
Hi Heinrich,

On Wed, 16 Feb 2022 at 08:37, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>
> On 2/16/22 16:26, Simon Glass wrote:
> > Hi Masami,
> >
> > On Tue, 15 Feb 2022 at 02:05, Masami Hiramatsu
> > <masami.hiramatsu@linaro.org> wrote:
> >>
> >> Since now the capsule_on_disk will restart the u-boot sandbox right
> >> after the capsule update, if CONFIG_EFI_CAPSULE_ON_DISK_EARLY=y, the
> >> boot with a new capsule file will repeat reboot sequence. On the
> >> other hand, if CONFIG_EFI_CAPSULE_ON_DISK_EARLY=n, the 'env print -e'
> >> command will execute the capsule update on disk and reboot.
> >>
> >> Thus this update the uboot_console for those 2 cases;
> >>
> >>   - restart_uboot(): Add expect_earlyreset optional parameter so that
> >>     it can handle the reboot while booting.
> >>   - run_command(): Add wait_for_reboot optional parameter so that it
> >>     can handle the reboot after executing a command.
> >>
> >> And enable those options in the test_capsule_firmware.py test cases.
> >>
> >> Signed-off-by: Masami Hiramatsu <masami.hiramatsu@linaro.org>
> >> ---
> >>   .../test_efi_capsule/test_capsule_firmware.py      |   39 ++++++--
> >>   test/py/u_boot_console_base.py                     |   95 +++++++++++++++-----
> >>   test/py/u_boot_console_sandbox.py                  |    6 +
> >>   3 files changed, 102 insertions(+), 38 deletions(-)
> >
> > We have a means to avoid actually doing the reset, see the reset driver.
>
> The UEFI specification requires a cold reset after a capsule is updated
> and before the console is reached. How could the reset driver help to
> fix the Python tests?

The sandbox reset driver can handle this, so at least for sandbox
(where CI runs) this should be used.

For real boards, you are getting into very strange territory, with
test harnesses and whatever.

Make it run on sandbox for now, and that is good enough for CI at present.

NAK on this patch.

> >
> > PLEASE use that instead of adding all this code. Also make sure that
> > test works with 'make qcheck' too.
> >
> > Regards,
> > Simon
>

Regards,
Simon
Tom Rini Feb. 16, 2022, 5:51 p.m. UTC | #13
On Wed, Feb 16, 2022 at 06:45:32PM +0100, Heinrich Schuchardt wrote:
> On 2/16/22 16:46, Tom Rini wrote:
> > On Wed, Feb 16, 2022 at 04:32:40PM +0100, Heinrich Schuchardt wrote:
> > > On 2/16/22 16:26, Simon Glass wrote:
> > > > Hi Masami,
> > > > 
> > > > On Tue, 15 Feb 2022 at 02:05, Masami Hiramatsu
> > > > <masami.hiramatsu@linaro.org> wrote:
> > > > > 
> > > > > Since now the capsule_on_disk will restart the u-boot sandbox right
> > > > > after the capsule update, if CONFIG_EFI_CAPSULE_ON_DISK_EARLY=y, the
> > > > > boot with a new capsule file will repeat reboot sequence. On the
> > > > > other hand, if CONFIG_EFI_CAPSULE_ON_DISK_EARLY=n, the 'env print -e'
> > > > > command will execute the capsule update on disk and reboot.
> > > > > 
> > > > > Thus this update the uboot_console for those 2 cases;
> > > > > 
> > > > >    - restart_uboot(): Add expect_earlyreset optional parameter so that
> > > > >      it can handle the reboot while booting.
> > > > >    - run_command(): Add wait_for_reboot optional parameter so that it
> > > > >      can handle the reboot after executing a command.
> > > > > 
> > > > > And enable those options in the test_capsule_firmware.py test cases.
> > > > > 
> > > > > Signed-off-by: Masami Hiramatsu <masami.hiramatsu@linaro.org>
> > > > > ---
> > > > >    .../test_efi_capsule/test_capsule_firmware.py      |   39 ++++++--
> > > > >    test/py/u_boot_console_base.py                     |   95 +++++++++++++++-----
> > > > >    test/py/u_boot_console_sandbox.py                  |    6 +
> > > > >    3 files changed, 102 insertions(+), 38 deletions(-)
> > > > 
> > > > We have a means to avoid actually doing the reset, see the reset driver.
> > > 
> > > The UEFI specification requires a cold reset after a capsule is updated
> > > and before the console is reached. How could the reset driver help to
> > > fix the Python tests?
> > 
> > Is this test going to be able to run on qemu, sandbox, real hardware, or
> > all 3?  The tests may well end up having to know a bit more, sadly,
> > about the type of system they're testing.
> 
> Currently the test will only run on the sandbox in Gitlab (see usage of
> @pytest.mark.boardspec('sandbox') in test/py/tests/test_efi_capsule/).

OK, I'll leave sandbox commentary to Simon.

> You will not want to update the firmware on real hardware especially if
> there is a rollback protection. But testing on QEMU would make sense.

On QEMU we should be able to since we can turn boards off and on and
would be writing to our emulated flash.  Then on real hardware, we
should be able to do the same tests, or at least the ones involving
"update was successful" ?  We have DFU tests today too, so I'm not
immediately sure why we couldn't (aside from possibly wearing down flash
life, and gating HW tests on emulator tests passing is always a good
idea).
Simon Glass Feb. 16, 2022, 5:52 p.m. UTC | #14
Hi Heinrich,

On Wed, 16 Feb 2022 at 10:50, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>
> On 2/16/22 16:46, Tom Rini wrote:
> > On Wed, Feb 16, 2022 at 04:32:40PM +0100, Heinrich Schuchardt wrote:
> >> On 2/16/22 16:26, Simon Glass wrote:
> >>> Hi Masami,
> >>>
> >>> On Tue, 15 Feb 2022 at 02:05, Masami Hiramatsu
> >>> <masami.hiramatsu@linaro.org> wrote:
> >>>>
> >>>> Since now the capsule_on_disk will restart the u-boot sandbox right
> >>>> after the capsule update, if CONFIG_EFI_CAPSULE_ON_DISK_EARLY=y, the
> >>>> boot with a new capsule file will repeat reboot sequence. On the
> >>>> other hand, if CONFIG_EFI_CAPSULE_ON_DISK_EARLY=n, the 'env print -e'
> >>>> command will execute the capsule update on disk and reboot.
> >>>>
> >>>> Thus this update the uboot_console for those 2 cases;
> >>>>
> >>>>    - restart_uboot(): Add expect_earlyreset optional parameter so that
> >>>>      it can handle the reboot while booting.
> >>>>    - run_command(): Add wait_for_reboot optional parameter so that it
> >>>>      can handle the reboot after executing a command.
> >>>>
> >>>> And enable those options in the test_capsule_firmware.py test cases.
> >>>>
> >>>> Signed-off-by: Masami Hiramatsu <masami.hiramatsu@linaro.org>
> >>>> ---
> >>>>    .../test_efi_capsule/test_capsule_firmware.py      |   39 ++++++--
> >>>>    test/py/u_boot_console_base.py                     |   95 +++++++++++++++-----
> >>>>    test/py/u_boot_console_sandbox.py                  |    6 +
> >>>>    3 files changed, 102 insertions(+), 38 deletions(-)
> >>>
> >>> We have a means to avoid actually doing the reset, see the reset driver.
> >>
> >> The UEFI specification requires a cold reset after a capsule is updated
> >> and before the console is reached. How could the reset driver help to
> >> fix the Python tests?
> >
> > Is this test going to be able to run on qemu, sandbox, real hardware, or
> > all 3?  The tests may well end up having to know a bit more, sadly,
> > about the type of system they're testing.
> >
> Currently the test will only run on the sandbox in Gitlab (see usage of
> @pytest.mark.boardspec('sandbox') in test/py/tests/test_efi_capsule/).

Let me know if you need help reworking this patch to operate on
sandbox without a 'real' reset.

Regards,
Simon
Masami Hiramatsu Feb. 17, 2022, 1:11 a.m. UTC | #15
Hi Simon,

Let me confirm your point.
So are you concerning the 'real' reset for the capsule update test
case itself or this patch?

I'm actually learning how the test is working, so please help me to
understand how I can solve it.

There are 3 environments to run the test, sandbox, Qemu, and a real board.
If we reset a sandbox, it will continue to run (just restart itself),
but Qemu and real board will cause a real reset and it will terminate
the qemu or stop the board (depends on how it is implemented). Thus,
if a command or boot process will cause a reset, it will need a
special care (maybe respawn?).

Since the capsule update testcase only runs on sandbox, it will not
cause real reset. But maybe it is possible to support running on Qemu.

Current my test patch (and capsule update testcase itself) doesn't
handle the real reset case correctly even on Qemu. The Qemu needs
spawn a new instance and re-connect the console when the reset
happens.

If so, I think there are 2 issues to be solved.
1. change the capsule update testcase runable on Qemu
2. change my patch to handle the real reset correctly (not only
waiting for the next boot, but also respawn it again)

Do I understand correctly?

Thank you,

2022年2月17日(木) 2:53 Simon Glass <sjg@chromium.org>:
>
> Hi Heinrich,
>
> On Wed, 16 Feb 2022 at 10:50, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> >
> > On 2/16/22 16:46, Tom Rini wrote:
> > > On Wed, Feb 16, 2022 at 04:32:40PM +0100, Heinrich Schuchardt wrote:
> > >> On 2/16/22 16:26, Simon Glass wrote:
> > >>> Hi Masami,
> > >>>
> > >>> On Tue, 15 Feb 2022 at 02:05, Masami Hiramatsu
> > >>> <masami.hiramatsu@linaro.org> wrote:
> > >>>>
> > >>>> Since now the capsule_on_disk will restart the u-boot sandbox right
> > >>>> after the capsule update, if CONFIG_EFI_CAPSULE_ON_DISK_EARLY=y, the
> > >>>> boot with a new capsule file will repeat reboot sequence. On the
> > >>>> other hand, if CONFIG_EFI_CAPSULE_ON_DISK_EARLY=n, the 'env print -e'
> > >>>> command will execute the capsule update on disk and reboot.
> > >>>>
> > >>>> Thus this update the uboot_console for those 2 cases;
> > >>>>
> > >>>>    - restart_uboot(): Add expect_earlyreset optional parameter so that
> > >>>>      it can handle the reboot while booting.
> > >>>>    - run_command(): Add wait_for_reboot optional parameter so that it
> > >>>>      can handle the reboot after executing a command.
> > >>>>
> > >>>> And enable those options in the test_capsule_firmware.py test cases.
> > >>>>
> > >>>> Signed-off-by: Masami Hiramatsu <masami.hiramatsu@linaro.org>
> > >>>> ---
> > >>>>    .../test_efi_capsule/test_capsule_firmware.py      |   39 ++++++--
> > >>>>    test/py/u_boot_console_base.py                     |   95 +++++++++++++++-----
> > >>>>    test/py/u_boot_console_sandbox.py                  |    6 +
> > >>>>    3 files changed, 102 insertions(+), 38 deletions(-)
> > >>>
> > >>> We have a means to avoid actually doing the reset, see the reset driver.
> > >>
> > >> The UEFI specification requires a cold reset after a capsule is updated
> > >> and before the console is reached. How could the reset driver help to
> > >> fix the Python tests?
> > >
> > > Is this test going to be able to run on qemu, sandbox, real hardware, or
> > > all 3?  The tests may well end up having to know a bit more, sadly,
> > > about the type of system they're testing.
> > >
> > Currently the test will only run on the sandbox in Gitlab (see usage of
> > @pytest.mark.boardspec('sandbox') in test/py/tests/test_efi_capsule/).
>
> Let me know if you need help reworking this patch to operate on
> sandbox without a 'real' reset.
>
> Regards,
> Simon
Simon Glass Feb. 17, 2022, 5:55 p.m. UTC | #16
Hi Masami,

On Wed, 16 Feb 2022 at 18:11, Masami Hiramatsu
<masami.hiramatsu@linaro.org> wrote:
>
> Hi Simon,
>
> Let me confirm your point.
> So are you concerning the 'real' reset for the capsule update test
> case itself or this patch?
>
> I'm actually learning how the test is working, so please help me to
> understand how I can solve it.
>
> There are 3 environments to run the test, sandbox, Qemu, and a real board.
> If we reset a sandbox, it will continue to run (just restart itself),

Here you should be able to avoid doing a reset. See
dm_test_sysreset_base() which tests sysreset drivers on sandbox.

> but Qemu and real board will cause a real reset and it will terminate
> the qemu or stop the board (depends on how it is implemented). Thus,
> if a command or boot process will cause a reset, it will need a
> special care (maybe respawn?).

Here you need to worry about the surrounding automation logic which
could be tbot of the U-Boot pytest hooks. I suggest you avoid this and
handle it some other way, without reset.

>
> Since the capsule update testcase only runs on sandbox, it will not
> cause real reset. But maybe it is possible to support running on Qemu.

Maybe, but I don't think you should worry about that, at least for
now. The sandbox test is enough.

>
> Current my test patch (and capsule update testcase itself) doesn't
> handle the real reset case correctly even on Qemu. The Qemu needs
> spawn a new instance and re-connect the console when the reset
> happens.

Indeed.

>
> If so, I think there are 2 issues to be solved.
> 1. change the capsule update testcase runable on Qemu
> 2. change my patch to handle the real reset correctly (not only
> waiting for the next boot, but also respawn it again)
>
> Do I understand correctly?

I think the best approach is to get your test running on sandbox, with
the faked reset. Don't worry about the other cases as we don't support
them.

Regards,
Simon


>
> Thank you,
>
> 2022年2月17日(木) 2:53 Simon Glass <sjg@chromium.org>:
> >
> > Hi Heinrich,
> >
> > On Wed, 16 Feb 2022 at 10:50, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> > >
> > > On 2/16/22 16:46, Tom Rini wrote:
> > > > On Wed, Feb 16, 2022 at 04:32:40PM +0100, Heinrich Schuchardt wrote:
> > > >> On 2/16/22 16:26, Simon Glass wrote:
> > > >>> Hi Masami,
> > > >>>
> > > >>> On Tue, 15 Feb 2022 at 02:05, Masami Hiramatsu
> > > >>> <masami.hiramatsu@linaro.org> wrote:
> > > >>>>
> > > >>>> Since now the capsule_on_disk will restart the u-boot sandbox right
> > > >>>> after the capsule update, if CONFIG_EFI_CAPSULE_ON_DISK_EARLY=y, the
> > > >>>> boot with a new capsule file will repeat reboot sequence. On the
> > > >>>> other hand, if CONFIG_EFI_CAPSULE_ON_DISK_EARLY=n, the 'env print -e'
> > > >>>> command will execute the capsule update on disk and reboot.
> > > >>>>
> > > >>>> Thus this update the uboot_console for those 2 cases;
> > > >>>>
> > > >>>>    - restart_uboot(): Add expect_earlyreset optional parameter so that
> > > >>>>      it can handle the reboot while booting.
> > > >>>>    - run_command(): Add wait_for_reboot optional parameter so that it
> > > >>>>      can handle the reboot after executing a command.
> > > >>>>
> > > >>>> And enable those options in the test_capsule_firmware.py test cases.
> > > >>>>
> > > >>>> Signed-off-by: Masami Hiramatsu <masami.hiramatsu@linaro.org>
> > > >>>> ---
> > > >>>>    .../test_efi_capsule/test_capsule_firmware.py      |   39 ++++++--
> > > >>>>    test/py/u_boot_console_base.py                     |   95 +++++++++++++++-----
> > > >>>>    test/py/u_boot_console_sandbox.py                  |    6 +
> > > >>>>    3 files changed, 102 insertions(+), 38 deletions(-)
> > > >>>
> > > >>> We have a means to avoid actually doing the reset, see the reset driver.
> > > >>
> > > >> The UEFI specification requires a cold reset after a capsule is updated
> > > >> and before the console is reached. How could the reset driver help to
> > > >> fix the Python tests?
> > > >
> > > > Is this test going to be able to run on qemu, sandbox, real hardware, or
> > > > all 3?  The tests may well end up having to know a bit more, sadly,
> > > > about the type of system they're testing.
> > > >
> > > Currently the test will only run on the sandbox in Gitlab (see usage of
> > > @pytest.mark.boardspec('sandbox') in test/py/tests/test_efi_capsule/).
> >
> > Let me know if you need help reworking this patch to operate on
> > sandbox without a 'real' reset.
> >
> > Regards,
> > Simon
>
>
>
> --
> Masami Hiramatsu
Masami Hiramatsu Feb. 18, 2022, 2:16 a.m. UTC | #17
Hi Simon,

Thank you for your reply.

2022年2月18日(金) 2:56 Simon Glass <sjg@chromium.org>:

>
> Hi Masami,
>
> On Wed, 16 Feb 2022 at 18:11, Masami Hiramatsu
> <masami.hiramatsu@linaro.org> wrote:
> >
> > Hi Simon,
> >
> > Let me confirm your point.
> > So are you concerning the 'real' reset for the capsule update test
> > case itself or this patch?
> >
> > I'm actually learning how the test is working, so please help me to
> > understand how I can solve it.
> >
> > There are 3 environments to run the test, sandbox, Qemu, and a real board.
> > If we reset a sandbox, it will continue to run (just restart itself),
>
> Here you should be able to avoid doing a reset. See
> dm_test_sysreset_base() which tests sysreset drivers on sandbox.

Would you mean that reset-after-capsule-on-disk itself should not
make a reset on sandbox?

In dm_test_sysreset_base(), I can see the below code, this means
sysreset_request()
will not execute real reset, but just mimic the reset, right?

state->sysreset_allowed[SYSRESET_WARM] = true;
ut_asserteq(-EINPROGRESS, sysreset_request(dev, SYSRESET_WARM));
state->sysreset_allowed[SYSRESET_WARM] = false;

> > but Qemu and real board will cause a real reset and it will terminate
> > the qemu or stop the board (depends on how it is implemented). Thus,
> > if a command or boot process will cause a reset, it will need a
> > special care (maybe respawn?).
>
> Here you need to worry about the surrounding automation logic which
> could be tbot of the U-Boot pytest hooks. I suggest you avoid this and
> handle it some other way, without reset.

Hmm, would you mean adding a runtime flag to sandbox so that
it will not does real reset but just showing some token on console like
"sandbox fake reset done." ?


> > Since the capsule update testcase only runs on sandbox, it will not
> > cause real reset. But maybe it is possible to support running on Qemu.
>
> Maybe, but I don't think you should worry about that, at least for
> now. The sandbox test is enough.
>
> >
> > Current my test patch (and capsule update testcase itself) doesn't
> > handle the real reset case correctly even on Qemu. The Qemu needs
> > spawn a new instance and re-connect the console when the reset
> > happens.
>
> Indeed.
>
> >
> > If so, I think there are 2 issues to be solved.
> > 1. change the capsule update testcase runable on Qemu
> > 2. change my patch to handle the real reset correctly (not only
> > waiting for the next boot, but also respawn it again)
> >
> > Do I understand correctly?
>
> I think the best approach is to get your test running on sandbox, with
> the faked reset. Don't worry about the other cases as we don't support
> them.

OK.

Thank you,

>
> Regards,
> Simon
>
>
> >
> > Thank you,
> >
> > 2022年2月17日(木) 2:53 Simon Glass <sjg@chromium.org>:
> > >
> > > Hi Heinrich,
> > >
> > > On Wed, 16 Feb 2022 at 10:50, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> > > >
> > > > On 2/16/22 16:46, Tom Rini wrote:
> > > > > On Wed, Feb 16, 2022 at 04:32:40PM +0100, Heinrich Schuchardt wrote:
> > > > >> On 2/16/22 16:26, Simon Glass wrote:
> > > > >>> Hi Masami,
> > > > >>>
> > > > >>> On Tue, 15 Feb 2022 at 02:05, Masami Hiramatsu
> > > > >>> <masami.hiramatsu@linaro.org> wrote:
> > > > >>>>
> > > > >>>> Since now the capsule_on_disk will restart the u-boot sandbox right
> > > > >>>> after the capsule update, if CONFIG_EFI_CAPSULE_ON_DISK_EARLY=y, the
> > > > >>>> boot with a new capsule file will repeat reboot sequence. On the
> > > > >>>> other hand, if CONFIG_EFI_CAPSULE_ON_DISK_EARLY=n, the 'env print -e'
> > > > >>>> command will execute the capsule update on disk and reboot.
> > > > >>>>
> > > > >>>> Thus this update the uboot_console for those 2 cases;
> > > > >>>>
> > > > >>>>    - restart_uboot(): Add expect_earlyreset optional parameter so that
> > > > >>>>      it can handle the reboot while booting.
> > > > >>>>    - run_command(): Add wait_for_reboot optional parameter so that it
> > > > >>>>      can handle the reboot after executing a command.
> > > > >>>>
> > > > >>>> And enable those options in the test_capsule_firmware.py test cases.
> > > > >>>>
> > > > >>>> Signed-off-by: Masami Hiramatsu <masami.hiramatsu@linaro.org>
> > > > >>>> ---
> > > > >>>>    .../test_efi_capsule/test_capsule_firmware.py      |   39 ++++++--
> > > > >>>>    test/py/u_boot_console_base.py                     |   95 +++++++++++++++-----
> > > > >>>>    test/py/u_boot_console_sandbox.py                  |    6 +
> > > > >>>>    3 files changed, 102 insertions(+), 38 deletions(-)
> > > > >>>
> > > > >>> We have a means to avoid actually doing the reset, see the reset driver.
> > > > >>
> > > > >> The UEFI specification requires a cold reset after a capsule is updated
> > > > >> and before the console is reached. How could the reset driver help to
> > > > >> fix the Python tests?
> > > > >
> > > > > Is this test going to be able to run on qemu, sandbox, real hardware, or
> > > > > all 3?  The tests may well end up having to know a bit more, sadly,
> > > > > about the type of system they're testing.
> > > > >
> > > > Currently the test will only run on the sandbox in Gitlab (see usage of
> > > > @pytest.mark.boardspec('sandbox') in test/py/tests/test_efi_capsule/).
> > >
> > > Let me know if you need help reworking this patch to operate on
> > > sandbox without a 'real' reset.
> > >
> > > Regards,
> > > Simon
> >
> >
> >
> > --
> > Masami Hiramatsu



--
Masami Hiramatsu
Heinrich Schuchardt Feb. 18, 2022, 1:48 p.m. UTC | #18
On 2/18/22 03:16, Masami Hiramatsu wrote:
> Hi Simon,
>
> Thank you for your reply.
>
> 2022年2月18日(金) 2:56 Simon Glass <sjg@chromium.org>:
>
>>
>> Hi Masami,
>>
>> On Wed, 16 Feb 2022 at 18:11, Masami Hiramatsu
>> <masami.hiramatsu@linaro.org> wrote:
>>>
>>> Hi Simon,
>>>
>>> Let me confirm your point.
>>> So are you concerning the 'real' reset for the capsule update test
>>> case itself or this patch?
>>>
>>> I'm actually learning how the test is working, so please help me to
>>> understand how I can solve it.
>>>
>>> There are 3 environments to run the test, sandbox, Qemu, and a real board.
>>> If we reset a sandbox, it will continue to run (just restart itself),
>>
>> Here you should be able to avoid doing a reset. See
>> dm_test_sysreset_base() which tests sysreset drivers on sandbox.
>
> Would you mean that reset-after-capsule-on-disk itself should not
> make a reset on sandbox?

We have several tests that do resets by calling do_reset(), e.g. the
UEFI unit tests. There is nothing wrong about it.

We want the sandbox to behave like any other board where capsule updates
lead to resets.

>
> In dm_test_sysreset_base(), I can see the below code, this means
> sysreset_request()
> will not execute real reset, but just mimic the reset, right?
>
> state->sysreset_allowed[SYSRESET_WARM] = true;
> ut_asserteq(-EINPROGRESS, sysreset_request(dev, SYSRESET_WARM));
> state->sysreset_allowed[SYSRESET_WARM] = false;
>
>>> but Qemu and real board will cause a real reset and it will terminate
>>> the qemu or stop the board (depends on how it is implemented). Thus,
>>> if a command or boot process will cause a reset, it will need a
>>> special care (maybe respawn?).
>>
>> Here you need to worry about the surrounding automation logic which
>> could be tbot of the U-Boot pytest hooks. I suggest you avoid this and
>> handle it some other way, without reset.

The sandbox should run through exactly the same code path as all other
boards to get a meaningful test results. Therefore don't put in any
quirks on C level. Your Python test changes are all that is needed.

Best regards

Heinrich

>
> Hmm, would you mean adding a runtime flag to sandbox so that
> it will not does real reset but just showing some token on console like
> "sandbox fake reset done." ?
>
>
>>> Since the capsule update testcase only runs on sandbox, it will not
>>> cause real reset. But maybe it is possible to support running on Qemu.
>>
>> Maybe, but I don't think you should worry about that, at least for
>> now. The sandbox test is enough.
>>
>>>
>>> Current my test patch (and capsule update testcase itself) doesn't
>>> handle the real reset case correctly even on Qemu. The Qemu needs
>>> spawn a new instance and re-connect the console when the reset
>>> happens.
>>
>> Indeed.
>>
>>>
>>> If so, I think there are 2 issues to be solved.
>>> 1. change the capsule update testcase runable on Qemu
>>> 2. change my patch to handle the real reset correctly (not only
>>> waiting for the next boot, but also respawn it again)
>>>
>>> Do I understand correctly?
>>
>> I think the best approach is to get your test running on sandbox, with
>> the faked reset. Don't worry about the other cases as we don't support
>> them.
>
> OK.
>
> Thank you,
>
>>
>> Regards,
>> Simon
>>
>>
>>>
>>> Thank you,
>>>
>>> 2022年2月17日(木) 2:53 Simon Glass <sjg@chromium.org>:
>>>>
>>>> Hi Heinrich,
>>>>
>>>> On Wed, 16 Feb 2022 at 10:50, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>>>>>
>>>>> On 2/16/22 16:46, Tom Rini wrote:
>>>>>> On Wed, Feb 16, 2022 at 04:32:40PM +0100, Heinrich Schuchardt wrote:
>>>>>>> On 2/16/22 16:26, Simon Glass wrote:
>>>>>>>> Hi Masami,
>>>>>>>>
>>>>>>>> On Tue, 15 Feb 2022 at 02:05, Masami Hiramatsu
>>>>>>>> <masami.hiramatsu@linaro.org> wrote:
>>>>>>>>>
>>>>>>>>> Since now the capsule_on_disk will restart the u-boot sandbox right
>>>>>>>>> after the capsule update, if CONFIG_EFI_CAPSULE_ON_DISK_EARLY=y, the
>>>>>>>>> boot with a new capsule file will repeat reboot sequence. On the
>>>>>>>>> other hand, if CONFIG_EFI_CAPSULE_ON_DISK_EARLY=n, the 'env print -e'
>>>>>>>>> command will execute the capsule update on disk and reboot.
>>>>>>>>>
>>>>>>>>> Thus this update the uboot_console for those 2 cases;
>>>>>>>>>
>>>>>>>>>     - restart_uboot(): Add expect_earlyreset optional parameter so that
>>>>>>>>>       it can handle the reboot while booting.
>>>>>>>>>     - run_command(): Add wait_for_reboot optional parameter so that it
>>>>>>>>>       can handle the reboot after executing a command.
>>>>>>>>>
>>>>>>>>> And enable those options in the test_capsule_firmware.py test cases.
>>>>>>>>>
>>>>>>>>> Signed-off-by: Masami Hiramatsu <masami.hiramatsu@linaro.org>
>>>>>>>>> ---
>>>>>>>>>     .../test_efi_capsule/test_capsule_firmware.py      |   39 ++++++--
>>>>>>>>>     test/py/u_boot_console_base.py                     |   95 +++++++++++++++-----
>>>>>>>>>     test/py/u_boot_console_sandbox.py                  |    6 +
>>>>>>>>>     3 files changed, 102 insertions(+), 38 deletions(-)
>>>>>>>>
>>>>>>>> We have a means to avoid actually doing the reset, see the reset driver.
>>>>>>>
>>>>>>> The UEFI specification requires a cold reset after a capsule is updated
>>>>>>> and before the console is reached. How could the reset driver help to
>>>>>>> fix the Python tests?
>>>>>>
>>>>>> Is this test going to be able to run on qemu, sandbox, real hardware, or
>>>>>> all 3?  The tests may well end up having to know a bit more, sadly,
>>>>>> about the type of system they're testing.
>>>>>>
>>>>> Currently the test will only run on the sandbox in Gitlab (see usage of
>>>>> @pytest.mark.boardspec('sandbox') in test/py/tests/test_efi_capsule/).
>>>>
>>>> Let me know if you need help reworking this patch to operate on
>>>> sandbox without a 'real' reset.
>>>>
>>>> Regards,
>>>> Simon
>>>
>>>
>>>
>>> --
>>> Masami Hiramatsu
>
>
>
> --
> Masami Hiramatsu
Heinrich Schuchardt Feb. 18, 2022, 1:59 p.m. UTC | #19
On 2/17/22 18:55, Simon Glass wrote:
> Hi Masami,
>
> On Wed, 16 Feb 2022 at 18:11, Masami Hiramatsu
> <masami.hiramatsu@linaro.org> wrote:
>>
>> Hi Simon,
>>
>> Let me confirm your point.
>> So are you concerning the 'real' reset for the capsule update test
>> case itself or this patch?
>>
>> I'm actually learning how the test is working, so please help me to
>> understand how I can solve it.
>>
>> There are 3 environments to run the test, sandbox, Qemu, and a real board.
>> If we reset a sandbox, it will continue to run (just restart itself),
>
> Here you should be able to avoid doing a reset. See
> dm_test_sysreset_base() which tests sysreset drivers on sandbox.
>
>> but Qemu and real board will cause a real reset and it will terminate
>> the qemu or stop the board (depends on how it is implemented). Thus,
>> if a command or boot process will cause a reset, it will need a
>> special care (maybe respawn?).
>
> Here you need to worry about the surrounding automation logic which
> could be tbot of the U-Boot pytest hooks. I suggest you avoid this and
> handle it some other way, without reset.
>
>>
>> Since the capsule update testcase only runs on sandbox, it will not
>> cause real reset. But maybe it is possible to support running on Qemu.
>
> Maybe, but I don't think you should worry about that, at least for
> now. The sandbox test is enough.
>
>>
>> Current my test patch (and capsule update testcase itself) doesn't
>> handle the real reset case correctly even on Qemu. The Qemu needs
>> spawn a new instance and re-connect the console when the reset
>> happens.

Respawning of QEMU instances after a reset is handled by the scripts in
https://source.denx.de/u-boot/u-boot-test-hooks.git on Gitlab CI. For my
local tests I have used:
https://github.com/xypron/u-boot-build/tree/qemu-arm64/u-boot-test

If you want to set up an environment for a real board, you have to write
your own python scripts and make them available over PYTHONPATH. For an
example see
https://github.com/xypron/u-boot-build/tree/orangepi-pc/u-boot-test

>
> Indeed.
>
>>
>> If so, I think there are 2 issues to be solved.
>> 1. change the capsule update testcase runable on Qemu
>> 2. change my patch to handle the real reset correctly (not only
>> waiting for the next boot, but also respawn it again)
>>
>> Do I understand correctly?
>
> I think the best approach is to get your test running on sandbox, with
> the faked reset. Don't worry about the other cases as we don't support
> them.

Why fake a reset? Just call the normal reset code. Avoid sandbox quirks.
We want the sandbox to just run through the same code as any other board
to get meaningful test results.

Best regards

Heinrich

>
> Regards,
> Simon
>
>
>>
>> Thank you,
>>
>> 2022年2月17日(木) 2:53 Simon Glass <sjg@chromium.org>:
>>>
>>> Hi Heinrich,
>>>
>>> On Wed, 16 Feb 2022 at 10:50, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>>>>
>>>> On 2/16/22 16:46, Tom Rini wrote:
>>>>> On Wed, Feb 16, 2022 at 04:32:40PM +0100, Heinrich Schuchardt wrote:
>>>>>> On 2/16/22 16:26, Simon Glass wrote:
>>>>>>> Hi Masami,
>>>>>>>
>>>>>>> On Tue, 15 Feb 2022 at 02:05, Masami Hiramatsu
>>>>>>> <masami.hiramatsu@linaro.org> wrote:
>>>>>>>>
>>>>>>>> Since now the capsule_on_disk will restart the u-boot sandbox right
>>>>>>>> after the capsule update, if CONFIG_EFI_CAPSULE_ON_DISK_EARLY=y, the
>>>>>>>> boot with a new capsule file will repeat reboot sequence. On the
>>>>>>>> other hand, if CONFIG_EFI_CAPSULE_ON_DISK_EARLY=n, the 'env print -e'
>>>>>>>> command will execute the capsule update on disk and reboot.
>>>>>>>>
>>>>>>>> Thus this update the uboot_console for those 2 cases;
>>>>>>>>
>>>>>>>>     - restart_uboot(): Add expect_earlyreset optional parameter so that
>>>>>>>>       it can handle the reboot while booting.
>>>>>>>>     - run_command(): Add wait_for_reboot optional parameter so that it
>>>>>>>>       can handle the reboot after executing a command.
>>>>>>>>
>>>>>>>> And enable those options in the test_capsule_firmware.py test cases.
>>>>>>>>
>>>>>>>> Signed-off-by: Masami Hiramatsu <masami.hiramatsu@linaro.org>
>>>>>>>> ---
>>>>>>>>     .../test_efi_capsule/test_capsule_firmware.py      |   39 ++++++--
>>>>>>>>     test/py/u_boot_console_base.py                     |   95 +++++++++++++++-----
>>>>>>>>     test/py/u_boot_console_sandbox.py                  |    6 +
>>>>>>>>     3 files changed, 102 insertions(+), 38 deletions(-)
>>>>>>>
>>>>>>> We have a means to avoid actually doing the reset, see the reset driver.
>>>>>>
>>>>>> The UEFI specification requires a cold reset after a capsule is updated
>>>>>> and before the console is reached. How could the reset driver help to
>>>>>> fix the Python tests?
>>>>>
>>>>> Is this test going to be able to run on qemu, sandbox, real hardware, or
>>>>> all 3?  The tests may well end up having to know a bit more, sadly,
>>>>> about the type of system they're testing.
>>>>>
>>>> Currently the test will only run on the sandbox in Gitlab (see usage of
>>>> @pytest.mark.boardspec('sandbox') in test/py/tests/test_efi_capsule/).
>>>
>>> Let me know if you need help reworking this patch to operate on
>>> sandbox without a 'real' reset.
>>>
>>> Regards,
>>> Simon
>>
>>
>>
>> --
>> Masami Hiramatsu
Tom Rini Feb. 18, 2022, 2:11 p.m. UTC | #20
On Fri, Feb 18, 2022 at 02:59:05PM +0100, Heinrich Schuchardt wrote:
> On 2/17/22 18:55, Simon Glass wrote:
> > Hi Masami,
> > 
> > On Wed, 16 Feb 2022 at 18:11, Masami Hiramatsu
> > <masami.hiramatsu@linaro.org> wrote:
> > > 
> > > Hi Simon,
> > > 
> > > Let me confirm your point.
> > > So are you concerning the 'real' reset for the capsule update test
> > > case itself or this patch?
> > > 
> > > I'm actually learning how the test is working, so please help me to
> > > understand how I can solve it.
> > > 
> > > There are 3 environments to run the test, sandbox, Qemu, and a real board.
> > > If we reset a sandbox, it will continue to run (just restart itself),
> > 
> > Here you should be able to avoid doing a reset. See
> > dm_test_sysreset_base() which tests sysreset drivers on sandbox.
> > 
> > > but Qemu and real board will cause a real reset and it will terminate
> > > the qemu or stop the board (depends on how it is implemented). Thus,
> > > if a command or boot process will cause a reset, it will need a
> > > special care (maybe respawn?).
> > 
> > Here you need to worry about the surrounding automation logic which
> > could be tbot of the U-Boot pytest hooks. I suggest you avoid this and
> > handle it some other way, without reset.
> > 
> > > 
> > > Since the capsule update testcase only runs on sandbox, it will not
> > > cause real reset. But maybe it is possible to support running on Qemu.
> > 
> > Maybe, but I don't think you should worry about that, at least for
> > now. The sandbox test is enough.
> > 
> > > 
> > > Current my test patch (and capsule update testcase itself) doesn't
> > > handle the real reset case correctly even on Qemu. The Qemu needs
> > > spawn a new instance and re-connect the console when the reset
> > > happens.
> 
> Respawning of QEMU instances after a reset is handled by the scripts in
> https://source.denx.de/u-boot/u-boot-test-hooks.git on Gitlab CI. For my
> local tests I have used:
> https://github.com/xypron/u-boot-build/tree/qemu-arm64/u-boot-test
> 
> If you want to set up an environment for a real board, you have to write
> your own python scripts and make them available over PYTHONPATH. For an
> example see
> https://github.com/xypron/u-boot-build/tree/orangepi-pc/u-boot-test

To be clear, Simon also has a number of physical boards in a CI lab.
Perhaps part of the confusion here is that we're thinking of these tests
as something more special than what we have already, but I don't think
that's the case.  It's just shifting the logic around a little bit in
that today we have "build u-boot, flash u-boot, ensure that version is
now running" and this is "build u-boot, update u-boot, ensure that
version is now running".
Tom Rini Feb. 18, 2022, 4:50 p.m. UTC | #21
On Thu, Feb 17, 2022 at 10:55:58AM -0700, Simon Glass wrote:
> Hi Masami,
> 
> On Wed, 16 Feb 2022 at 18:11, Masami Hiramatsu
> <masami.hiramatsu@linaro.org> wrote:
> >
> > Hi Simon,
> >
> > Let me confirm your point.
> > So are you concerning the 'real' reset for the capsule update test
> > case itself or this patch?
> >
> > I'm actually learning how the test is working, so please help me to
> > understand how I can solve it.
> >
> > There are 3 environments to run the test, sandbox, Qemu, and a real board.
> > If we reset a sandbox, it will continue to run (just restart itself),
> 
> Here you should be able to avoid doing a reset. See
> dm_test_sysreset_base() which tests sysreset drivers on sandbox.
> 
> > but Qemu and real board will cause a real reset and it will terminate
> > the qemu or stop the board (depends on how it is implemented). Thus,
> > if a command or boot process will cause a reset, it will need a
> > special care (maybe respawn?).
> 
> Here you need to worry about the surrounding automation logic which
> could be tbot of the U-Boot pytest hooks. I suggest you avoid this and
> handle it some other way, without reset.

Well, tbot and uboot-test-hooks implement the abstractions we have for
things like "reset the target".

> > Since the capsule update testcase only runs on sandbox, it will not
> > cause real reset. But maybe it is possible to support running on Qemu.
> 
> Maybe, but I don't think you should worry about that, at least for
> now. The sandbox test is enough.
> 
> >
> > Current my test patch (and capsule update testcase itself) doesn't
> > handle the real reset case correctly even on Qemu. The Qemu needs
> > spawn a new instance and re-connect the console when the reset
> > happens.
> 
> Indeed.
> 
> >
> > If so, I think there are 2 issues to be solved.
> > 1. change the capsule update testcase runable on Qemu
> > 2. change my patch to handle the real reset correctly (not only
> > waiting for the next boot, but also respawn it again)
> >
> > Do I understand correctly?
> 
> I think the best approach is to get your test running on sandbox, with
> the faked reset. Don't worry about the other cases as we don't support
> them.

I don't think we need to test qemu/real hardware to start with.  I think
we should be testing real hardware in the near future however.  Flash
this U-Boot and confirm it's running is the point of the "version" test.
AKASHI Takahiro Feb. 19, 2022, 1:15 a.m. UTC | #22
On Fri, Feb 18, 2022 at 02:48:54PM +0100, Heinrich Schuchardt wrote:
> On 2/18/22 03:16, Masami Hiramatsu wrote:
> > Hi Simon,
> > 
> > Thank you for your reply.
> > 
> > 2022年2月18日(金) 2:56 Simon Glass <sjg@chromium.org>:
> > 
> > > 
> > > Hi Masami,
> > > 
> > > On Wed, 16 Feb 2022 at 18:11, Masami Hiramatsu
> > > <masami.hiramatsu@linaro.org> wrote:
> > > > 
> > > > Hi Simon,
> > > > 
> > > > Let me confirm your point.
> > > > So are you concerning the 'real' reset for the capsule update test
> > > > case itself or this patch?
> > > > 
> > > > I'm actually learning how the test is working, so please help me to
> > > > understand how I can solve it.
> > > > 
> > > > There are 3 environments to run the test, sandbox, Qemu, and a real board.
> > > > If we reset a sandbox, it will continue to run (just restart itself),
> > > 
> > > Here you should be able to avoid doing a reset. See
> > > dm_test_sysreset_base() which tests sysreset drivers on sandbox.
> > 
> > Would you mean that reset-after-capsule-on-disk itself should not
> > make a reset on sandbox?
> 
> We have several tests that do resets by calling do_reset(), e.g. the
> UEFI unit tests. There is nothing wrong about it.
> 
> We want the sandbox to behave like any other board where capsule updates
> lead to resets.
> 
> > 
> > In dm_test_sysreset_base(), I can see the below code, this means
> > sysreset_request()
> > will not execute real reset, but just mimic the reset, right?
> > 
> > state->sysreset_allowed[SYSRESET_WARM] = true;
> > ut_asserteq(-EINPROGRESS, sysreset_request(dev, SYSRESET_WARM));
> > state->sysreset_allowed[SYSRESET_WARM] = false;
> > 
> > > > but Qemu and real board will cause a real reset and it will terminate
> > > > the qemu or stop the board (depends on how it is implemented). Thus,
> > > > if a command or boot process will cause a reset, it will need a
> > > > special care (maybe respawn?).
> > > 
> > > Here you need to worry about the surrounding automation logic which
> > > could be tbot of the U-Boot pytest hooks. I suggest you avoid this and
> > > handle it some other way, without reset.
> 
> The sandbox should run through exactly the same code path as all other
> boards to get a meaningful test results. Therefore don't put in any
> quirks on C level. Your Python test changes are all that is needed.

+1, I have the same opinion here.
To exercise capsule-on-disk code, we need a "real" reset
because pytest/CI loop is basically a black-box test.

-Takahiro Akashi

> 
> Best regards
> 
> Heinrich
> 
> > 
> > Hmm, would you mean adding a runtime flag to sandbox so that
> > it will not does real reset but just showing some token on console like
> > "sandbox fake reset done." ?
> > 
> > 
> > > > Since the capsule update testcase only runs on sandbox, it will not
> > > > cause real reset. But maybe it is possible to support running on Qemu.
> > > 
> > > Maybe, but I don't think you should worry about that, at least for
> > > now. The sandbox test is enough.
> > > 
> > > > 
> > > > Current my test patch (and capsule update testcase itself) doesn't
> > > > handle the real reset case correctly even on Qemu. The Qemu needs
> > > > spawn a new instance and re-connect the console when the reset
> > > > happens.
> > > 
> > > Indeed.
> > > 
> > > > 
> > > > If so, I think there are 2 issues to be solved.
> > > > 1. change the capsule update testcase runable on Qemu
> > > > 2. change my patch to handle the real reset correctly (not only
> > > > waiting for the next boot, but also respawn it again)
> > > > 
> > > > Do I understand correctly?
> > > 
> > > I think the best approach is to get your test running on sandbox, with
> > > the faked reset. Don't worry about the other cases as we don't support
> > > them.
> > 
> > OK.
> > 
> > Thank you,
> > 
> > > 
> > > Regards,
> > > Simon
> > > 
> > > 
> > > > 
> > > > Thank you,
> > > > 
> > > > 2022年2月17日(木) 2:53 Simon Glass <sjg@chromium.org>:
> > > > > 
> > > > > Hi Heinrich,
> > > > > 
> > > > > On Wed, 16 Feb 2022 at 10:50, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> > > > > > 
> > > > > > On 2/16/22 16:46, Tom Rini wrote:
> > > > > > > On Wed, Feb 16, 2022 at 04:32:40PM +0100, Heinrich Schuchardt wrote:
> > > > > > > > On 2/16/22 16:26, Simon Glass wrote:
> > > > > > > > > Hi Masami,
> > > > > > > > > 
> > > > > > > > > On Tue, 15 Feb 2022 at 02:05, Masami Hiramatsu
> > > > > > > > > <masami.hiramatsu@linaro.org> wrote:
> > > > > > > > > > 
> > > > > > > > > > Since now the capsule_on_disk will restart the u-boot sandbox right
> > > > > > > > > > after the capsule update, if CONFIG_EFI_CAPSULE_ON_DISK_EARLY=y, the
> > > > > > > > > > boot with a new capsule file will repeat reboot sequence. On the
> > > > > > > > > > other hand, if CONFIG_EFI_CAPSULE_ON_DISK_EARLY=n, the 'env print -e'
> > > > > > > > > > command will execute the capsule update on disk and reboot.
> > > > > > > > > > 
> > > > > > > > > > Thus this update the uboot_console for those 2 cases;
> > > > > > > > > > 
> > > > > > > > > >     - restart_uboot(): Add expect_earlyreset optional parameter so that
> > > > > > > > > >       it can handle the reboot while booting.
> > > > > > > > > >     - run_command(): Add wait_for_reboot optional parameter so that it
> > > > > > > > > >       can handle the reboot after executing a command.
> > > > > > > > > > 
> > > > > > > > > > And enable those options in the test_capsule_firmware.py test cases.
> > > > > > > > > > 
> > > > > > > > > > Signed-off-by: Masami Hiramatsu <masami.hiramatsu@linaro.org>
> > > > > > > > > > ---
> > > > > > > > > >     .../test_efi_capsule/test_capsule_firmware.py      |   39 ++++++--
> > > > > > > > > >     test/py/u_boot_console_base.py                     |   95 +++++++++++++++-----
> > > > > > > > > >     test/py/u_boot_console_sandbox.py                  |    6 +
> > > > > > > > > >     3 files changed, 102 insertions(+), 38 deletions(-)
> > > > > > > > > 
> > > > > > > > > We have a means to avoid actually doing the reset, see the reset driver.
> > > > > > > > 
> > > > > > > > The UEFI specification requires a cold reset after a capsule is updated
> > > > > > > > and before the console is reached. How could the reset driver help to
> > > > > > > > fix the Python tests?
> > > > > > > 
> > > > > > > Is this test going to be able to run on qemu, sandbox, real hardware, or
> > > > > > > all 3?  The tests may well end up having to know a bit more, sadly,
> > > > > > > about the type of system they're testing.
> > > > > > > 
> > > > > > Currently the test will only run on the sandbox in Gitlab (see usage of
> > > > > > @pytest.mark.boardspec('sandbox') in test/py/tests/test_efi_capsule/).
> > > > > 
> > > > > Let me know if you need help reworking this patch to operate on
> > > > > sandbox without a 'real' reset.
> > > > > 
> > > > > Regards,
> > > > > Simon
> > > > 
> > > > 
> > > > 
> > > > --
> > > > Masami Hiramatsu
> > 
> > 
> > 
> > --
> > Masami Hiramatsu
>
Masami Hiramatsu Feb. 19, 2022, 4:18 a.m. UTC | #23
Hi Heinrich,

2022年2月18日(金) 22:59 Heinrich Schuchardt <xypron.glpk@gmx.de>:
>
> On 2/17/22 18:55, Simon Glass wrote:
> > Hi Masami,
> >
> > On Wed, 16 Feb 2022 at 18:11, Masami Hiramatsu
> > <masami.hiramatsu@linaro.org> wrote:
> >>
> >> Hi Simon,
> >>
> >> Let me confirm your point.
> >> So are you concerning the 'real' reset for the capsule update test
> >> case itself or this patch?
> >>
> >> I'm actually learning how the test is working, so please help me to
> >> understand how I can solve it.
> >>
> >> There are 3 environments to run the test, sandbox, Qemu, and a real board.
> >> If we reset a sandbox, it will continue to run (just restart itself),
> >
> > Here you should be able to avoid doing a reset. See
> > dm_test_sysreset_base() which tests sysreset drivers on sandbox.
> >
> >> but Qemu and real board will cause a real reset and it will terminate
> >> the qemu or stop the board (depends on how it is implemented). Thus,
> >> if a command or boot process will cause a reset, it will need a
> >> special care (maybe respawn?).
> >
> > Here you need to worry about the surrounding automation logic which
> > could be tbot of the U-Boot pytest hooks. I suggest you avoid this and
> > handle it some other way, without reset.
> >
> >>
> >> Since the capsule update testcase only runs on sandbox, it will not
> >> cause real reset. But maybe it is possible to support running on Qemu.
> >
> > Maybe, but I don't think you should worry about that, at least for
> > now. The sandbox test is enough.
> >
> >>
> >> Current my test patch (and capsule update testcase itself) doesn't
> >> handle the real reset case correctly even on Qemu. The Qemu needs
> >> spawn a new instance and re-connect the console when the reset
> >> happens.
>
> Respawning of QEMU instances after a reset is handled by the scripts in
> https://source.denx.de/u-boot/u-boot-test-hooks.git on Gitlab CI. For my
> local tests I have used:
> https://github.com/xypron/u-boot-build/tree/qemu-arm64/u-boot-test
>
> If you want to set up an environment for a real board, you have to write
> your own python scripts and make them available over PYTHONPATH. For an
> example see
> https://github.com/xypron/u-boot-build/tree/orangepi-pc/u-boot-test

Great! thank you for the information.
I actually would like to confirm that my change on ConsoleBase class
will work on non-sandbox, or it should be ConsoleSandbox class
method.

>
> >
> > Indeed.
> >
> >>
> >> If so, I think there are 2 issues to be solved.
> >> 1. change the capsule update testcase runable on Qemu
> >> 2. change my patch to handle the real reset correctly (not only
> >> waiting for the next boot, but also respawn it again)
> >>
> >> Do I understand correctly?
> >
> > I think the best approach is to get your test running on sandbox, with
> > the faked reset. Don't worry about the other cases as we don't support
> > them.
>
> Why fake a reset? Just call the normal reset code. Avoid sandbox quirks.
> We want the sandbox to just run through the same code as any other board
> to get meaningful test results.

OK. I got it.

Thank you,

>
> Best regards
>
> Heinrich
>
> >
> > Regards,
> > Simon
> >
> >
> >>
> >> Thank you,
> >>
> >> 2022年2月17日(木) 2:53 Simon Glass <sjg@chromium.org>:
> >>>
> >>> Hi Heinrich,
> >>>
> >>> On Wed, 16 Feb 2022 at 10:50, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> >>>>
> >>>> On 2/16/22 16:46, Tom Rini wrote:
> >>>>> On Wed, Feb 16, 2022 at 04:32:40PM +0100, Heinrich Schuchardt wrote:
> >>>>>> On 2/16/22 16:26, Simon Glass wrote:
> >>>>>>> Hi Masami,
> >>>>>>>
> >>>>>>> On Tue, 15 Feb 2022 at 02:05, Masami Hiramatsu
> >>>>>>> <masami.hiramatsu@linaro.org> wrote:
> >>>>>>>>
> >>>>>>>> Since now the capsule_on_disk will restart the u-boot sandbox right
> >>>>>>>> after the capsule update, if CONFIG_EFI_CAPSULE_ON_DISK_EARLY=y, the
> >>>>>>>> boot with a new capsule file will repeat reboot sequence. On the
> >>>>>>>> other hand, if CONFIG_EFI_CAPSULE_ON_DISK_EARLY=n, the 'env print -e'
> >>>>>>>> command will execute the capsule update on disk and reboot.
> >>>>>>>>
> >>>>>>>> Thus this update the uboot_console for those 2 cases;
> >>>>>>>>
> >>>>>>>>     - restart_uboot(): Add expect_earlyreset optional parameter so that
> >>>>>>>>       it can handle the reboot while booting.
> >>>>>>>>     - run_command(): Add wait_for_reboot optional parameter so that it
> >>>>>>>>       can handle the reboot after executing a command.
> >>>>>>>>
> >>>>>>>> And enable those options in the test_capsule_firmware.py test cases.
> >>>>>>>>
> >>>>>>>> Signed-off-by: Masami Hiramatsu <masami.hiramatsu@linaro.org>
> >>>>>>>> ---
> >>>>>>>>     .../test_efi_capsule/test_capsule_firmware.py      |   39 ++++++--
> >>>>>>>>     test/py/u_boot_console_base.py                     |   95 +++++++++++++++-----
> >>>>>>>>     test/py/u_boot_console_sandbox.py                  |    6 +
> >>>>>>>>     3 files changed, 102 insertions(+), 38 deletions(-)
> >>>>>>>
> >>>>>>> We have a means to avoid actually doing the reset, see the reset driver.
> >>>>>>
> >>>>>> The UEFI specification requires a cold reset after a capsule is updated
> >>>>>> and before the console is reached. How could the reset driver help to
> >>>>>> fix the Python tests?
> >>>>>
> >>>>> Is this test going to be able to run on qemu, sandbox, real hardware, or
> >>>>> all 3?  The tests may well end up having to know a bit more, sadly,
> >>>>> about the type of system they're testing.
> >>>>>
> >>>> Currently the test will only run on the sandbox in Gitlab (see usage of
> >>>> @pytest.mark.boardspec('sandbox') in test/py/tests/test_efi_capsule/).
> >>>
> >>> Let me know if you need help reworking this patch to operate on
> >>> sandbox without a 'real' reset.
> >>>
> >>> Regards,
> >>> Simon
> >>
> >>
> >>
> >> --
> >> Masami Hiramatsu
>
Simon Glass March 12, 2022, 2:24 a.m. UTC | #24
Hi Takahiro,

On Fri, 18 Feb 2022 at 18:16, AKASHI Takahiro
<takahiro.akashi@linaro.org> wrote:
>
> On Fri, Feb 18, 2022 at 02:48:54PM +0100, Heinrich Schuchardt wrote:
> > On 2/18/22 03:16, Masami Hiramatsu wrote:
> > > Hi Simon,
> > >
> > > Thank you for your reply.
> > >
> > > 2022年2月18日(金) 2:56 Simon Glass <sjg@chromium.org>:
> > >
> > > >
> > > > Hi Masami,
> > > >
> > > > On Wed, 16 Feb 2022 at 18:11, Masami Hiramatsu
> > > > <masami.hiramatsu@linaro.org> wrote:
> > > > >
> > > > > Hi Simon,
> > > > >
> > > > > Let me confirm your point.
> > > > > So are you concerning the 'real' reset for the capsule update test
> > > > > case itself or this patch?
> > > > >
> > > > > I'm actually learning how the test is working, so please help me to
> > > > > understand how I can solve it.
> > > > >
> > > > > There are 3 environments to run the test, sandbox, Qemu, and a real board.
> > > > > If we reset a sandbox, it will continue to run (just restart itself),
> > > >
> > > > Here you should be able to avoid doing a reset. See
> > > > dm_test_sysreset_base() which tests sysreset drivers on sandbox.
> > >
> > > Would you mean that reset-after-capsule-on-disk itself should not
> > > make a reset on sandbox?
> >
> > We have several tests that do resets by calling do_reset(), e.g. the
> > UEFI unit tests. There is nothing wrong about it.
> >
> > We want the sandbox to behave like any other board where capsule updates
> > lead to resets.
> >
> > >
> > > In dm_test_sysreset_base(), I can see the below code, this means
> > > sysreset_request()
> > > will not execute real reset, but just mimic the reset, right?
> > >
> > > state->sysreset_allowed[SYSRESET_WARM] = true;
> > > ut_asserteq(-EINPROGRESS, sysreset_request(dev, SYSRESET_WARM));
> > > state->sysreset_allowed[SYSRESET_WARM] = false;
> > >
> > > > > but Qemu and real board will cause a real reset and it will terminate
> > > > > the qemu or stop the board (depends on how it is implemented). Thus,
> > > > > if a command or boot process will cause a reset, it will need a
> > > > > special care (maybe respawn?).
> > > >
> > > > Here you need to worry about the surrounding automation logic which
> > > > could be tbot of the U-Boot pytest hooks. I suggest you avoid this and
> > > > handle it some other way, without reset.
> >
> > The sandbox should run through exactly the same code path as all other
> > boards to get a meaningful test results. Therefore don't put in any
> > quirks on C level. Your Python test changes are all that is needed.
>
> +1, I have the same opinion here.
> To exercise capsule-on-disk code, we need a "real" reset
> because pytest/CI loop is basically a black-box test.

I don't see why you need the reset at all to test the code. You should
be able to run a command to make the update happen. How does the
updata actually get triggered when you reset?

[..]

Regards,
Simon
Masami Hiramatsu March 13, 2022, 2 p.m. UTC | #25
Hi Simon and Takahiro,

I also would like to know how the UEFI is tested in the U-Boot.
It seems that we have lib/efi_selftest/ but it seems not unified to
'ut' command.
Should I expand efi_selftest for testing capsule update with reset?

Thank you,

2022年3月12日(土) 11:24 Simon Glass <sjg@chromium.org>:
>
> Hi Takahiro,
>
> On Fri, 18 Feb 2022 at 18:16, AKASHI Takahiro
> <takahiro.akashi@linaro.org> wrote:
> >
> > On Fri, Feb 18, 2022 at 02:48:54PM +0100, Heinrich Schuchardt wrote:
> > > On 2/18/22 03:16, Masami Hiramatsu wrote:
> > > > Hi Simon,
> > > >
> > > > Thank you for your reply.
> > > >
> > > > 2022年2月18日(金) 2:56 Simon Glass <sjg@chromium.org>:
> > > >
> > > > >
> > > > > Hi Masami,
> > > > >
> > > > > On Wed, 16 Feb 2022 at 18:11, Masami Hiramatsu
> > > > > <masami.hiramatsu@linaro.org> wrote:
> > > > > >
> > > > > > Hi Simon,
> > > > > >
> > > > > > Let me confirm your point.
> > > > > > So are you concerning the 'real' reset for the capsule update test
> > > > > > case itself or this patch?
> > > > > >
> > > > > > I'm actually learning how the test is working, so please help me to
> > > > > > understand how I can solve it.
> > > > > >
> > > > > > There are 3 environments to run the test, sandbox, Qemu, and a real board.
> > > > > > If we reset a sandbox, it will continue to run (just restart itself),
> > > > >
> > > > > Here you should be able to avoid doing a reset. See
> > > > > dm_test_sysreset_base() which tests sysreset drivers on sandbox.
> > > >
> > > > Would you mean that reset-after-capsule-on-disk itself should not
> > > > make a reset on sandbox?
> > >
> > > We have several tests that do resets by calling do_reset(), e.g. the
> > > UEFI unit tests. There is nothing wrong about it.
> > >
> > > We want the sandbox to behave like any other board where capsule updates
> > > lead to resets.
> > >
> > > >
> > > > In dm_test_sysreset_base(), I can see the below code, this means
> > > > sysreset_request()
> > > > will not execute real reset, but just mimic the reset, right?
> > > >
> > > > state->sysreset_allowed[SYSRESET_WARM] = true;
> > > > ut_asserteq(-EINPROGRESS, sysreset_request(dev, SYSRESET_WARM));
> > > > state->sysreset_allowed[SYSRESET_WARM] = false;
> > > >
> > > > > > but Qemu and real board will cause a real reset and it will terminate
> > > > > > the qemu or stop the board (depends on how it is implemented). Thus,
> > > > > > if a command or boot process will cause a reset, it will need a
> > > > > > special care (maybe respawn?).
> > > > >
> > > > > Here you need to worry about the surrounding automation logic which
> > > > > could be tbot of the U-Boot pytest hooks. I suggest you avoid this and
> > > > > handle it some other way, without reset.
> > >
> > > The sandbox should run through exactly the same code path as all other
> > > boards to get a meaningful test results. Therefore don't put in any
> > > quirks on C level. Your Python test changes are all that is needed.
> >
> > +1, I have the same opinion here.
> > To exercise capsule-on-disk code, we need a "real" reset
> > because pytest/CI loop is basically a black-box test.
>
> I don't see why you need the reset at all to test the code. You should
> be able to run a command to make the update happen. How does the
> updata actually get triggered when you reset?
>
> [..]
>
> Regards,
> Simon
Simon Glass March 13, 2022, 10:23 p.m. UTC | #26
Hi Masami,

On Sun, 13 Mar 2022 at 08:00, Masami Hiramatsu
<masami.hiramatsu@linaro.org> wrote:
>
> Hi Simon and Takahiro,
>
> I also would like to know how the UEFI is tested in the U-Boot.
> It seems that we have lib/efi_selftest/ but it seems not unified to
> 'ut' command.

I think it would be good to add it there. There is some init that must
be done, but we do try to use 'ut' for all tests.

> Should I expand efi_selftest for testing capsule update with reset?

Well I think the update could be triggered by a command, so you can
test it without actually doing the reset, at least as a starting
point. We need to write the code with testing in mind.

Regards,
Simon


>
> Thank you,
>
> 2022年3月12日(土) 11:24 Simon Glass <sjg@chromium.org>:
> >
> > Hi Takahiro,
> >
> > On Fri, 18 Feb 2022 at 18:16, AKASHI Takahiro
> > <takahiro.akashi@linaro.org> wrote:
> > >
> > > On Fri, Feb 18, 2022 at 02:48:54PM +0100, Heinrich Schuchardt wrote:
> > > > On 2/18/22 03:16, Masami Hiramatsu wrote:
> > > > > Hi Simon,
> > > > >
> > > > > Thank you for your reply.
> > > > >
> > > > > 2022年2月18日(金) 2:56 Simon Glass <sjg@chromium.org>:
> > > > >
> > > > > >
> > > > > > Hi Masami,
> > > > > >
> > > > > > On Wed, 16 Feb 2022 at 18:11, Masami Hiramatsu
> > > > > > <masami.hiramatsu@linaro.org> wrote:
> > > > > > >
> > > > > > > Hi Simon,
> > > > > > >
> > > > > > > Let me confirm your point.
> > > > > > > So are you concerning the 'real' reset for the capsule update test
> > > > > > > case itself or this patch?
> > > > > > >
> > > > > > > I'm actually learning how the test is working, so please help me to
> > > > > > > understand how I can solve it.
> > > > > > >
> > > > > > > There are 3 environments to run the test, sandbox, Qemu, and a real board.
> > > > > > > If we reset a sandbox, it will continue to run (just restart itself),
> > > > > >
> > > > > > Here you should be able to avoid doing a reset. See
> > > > > > dm_test_sysreset_base() which tests sysreset drivers on sandbox.
> > > > >
> > > > > Would you mean that reset-after-capsule-on-disk itself should not
> > > > > make a reset on sandbox?
> > > >
> > > > We have several tests that do resets by calling do_reset(), e.g. the
> > > > UEFI unit tests. There is nothing wrong about it.
> > > >
> > > > We want the sandbox to behave like any other board where capsule updates
> > > > lead to resets.
> > > >
> > > > >
> > > > > In dm_test_sysreset_base(), I can see the below code, this means
> > > > > sysreset_request()
> > > > > will not execute real reset, but just mimic the reset, right?
> > > > >
> > > > > state->sysreset_allowed[SYSRESET_WARM] = true;
> > > > > ut_asserteq(-EINPROGRESS, sysreset_request(dev, SYSRESET_WARM));
> > > > > state->sysreset_allowed[SYSRESET_WARM] = false;
> > > > >
> > > > > > > but Qemu and real board will cause a real reset and it will terminate
> > > > > > > the qemu or stop the board (depends on how it is implemented). Thus,
> > > > > > > if a command or boot process will cause a reset, it will need a
> > > > > > > special care (maybe respawn?).
> > > > > >
> > > > > > Here you need to worry about the surrounding automation logic which
> > > > > > could be tbot of the U-Boot pytest hooks. I suggest you avoid this and
> > > > > > handle it some other way, without reset.
> > > >
> > > > The sandbox should run through exactly the same code path as all other
> > > > boards to get a meaningful test results. Therefore don't put in any
> > > > quirks on C level. Your Python test changes are all that is needed.
> > >
> > > +1, I have the same opinion here.
> > > To exercise capsule-on-disk code, we need a "real" reset
> > > because pytest/CI loop is basically a black-box test.
> >
> > I don't see why you need the reset at all to test the code. You should
> > be able to run a command to make the update happen. How does the
> > updata actually get triggered when you reset?
> >
> > [..]
> >
> > Regards,
> > Simon
>
>
>
> --
> Masami Hiramatsu
AKASHI Takahiro March 14, 2022, 1:08 a.m. UTC | #27
Hi Simon,

On Fri, Mar 11, 2022 at 07:24:39PM -0700, Simon Glass wrote:
> Hi Takahiro,
> 
> On Fri, 18 Feb 2022 at 18:16, AKASHI Takahiro
> <takahiro.akashi@linaro.org> wrote:
> >
> > On Fri, Feb 18, 2022 at 02:48:54PM +0100, Heinrich Schuchardt wrote:
> > > On 2/18/22 03:16, Masami Hiramatsu wrote:
> > > > Hi Simon,
> > > >
> > > > Thank you for your reply.
> > > >
> > > > 2022年2月18日(金) 2:56 Simon Glass <sjg@chromium.org>:
> > > >
> > > > >
> > > > > Hi Masami,
> > > > >
> > > > > On Wed, 16 Feb 2022 at 18:11, Masami Hiramatsu
> > > > > <masami.hiramatsu@linaro.org> wrote:
> > > > > >
> > > > > > Hi Simon,
> > > > > >
> > > > > > Let me confirm your point.
> > > > > > So are you concerning the 'real' reset for the capsule update test
> > > > > > case itself or this patch?
> > > > > >
> > > > > > I'm actually learning how the test is working, so please help me to
> > > > > > understand how I can solve it.
> > > > > >
> > > > > > There are 3 environments to run the test, sandbox, Qemu, and a real board.
> > > > > > If we reset a sandbox, it will continue to run (just restart itself),
> > > > >
> > > > > Here you should be able to avoid doing a reset. See
> > > > > dm_test_sysreset_base() which tests sysreset drivers on sandbox.
> > > >
> > > > Would you mean that reset-after-capsule-on-disk itself should not
> > > > make a reset on sandbox?
> > >
> > > We have several tests that do resets by calling do_reset(), e.g. the
> > > UEFI unit tests. There is nothing wrong about it.
> > >
> > > We want the sandbox to behave like any other board where capsule updates
> > > lead to resets.
> > >
> > > >
> > > > In dm_test_sysreset_base(), I can see the below code, this means
> > > > sysreset_request()
> > > > will not execute real reset, but just mimic the reset, right?
> > > >
> > > > state->sysreset_allowed[SYSRESET_WARM] = true;
> > > > ut_asserteq(-EINPROGRESS, sysreset_request(dev, SYSRESET_WARM));
> > > > state->sysreset_allowed[SYSRESET_WARM] = false;
> > > >
> > > > > > but Qemu and real board will cause a real reset and it will terminate
> > > > > > the qemu or stop the board (depends on how it is implemented). Thus,
> > > > > > if a command or boot process will cause a reset, it will need a
> > > > > > special care (maybe respawn?).
> > > > >
> > > > > Here you need to worry about the surrounding automation logic which
> > > > > could be tbot of the U-Boot pytest hooks. I suggest you avoid this and
> > > > > handle it some other way, without reset.
> > >
> > > The sandbox should run through exactly the same code path as all other
> > > boards to get a meaningful test results. Therefore don't put in any
> > > quirks on C level. Your Python test changes are all that is needed.
> >
> > +1, I have the same opinion here.
> > To exercise capsule-on-disk code, we need a "real" reset
> > because pytest/CI loop is basically a black-box test.
> 
> I don't see why you need the reset at all to test the code.

As I repeatedly said, I believe that this is a black-box test and
a system test. The purpose of the test is to make sure the firmware
update be performed in real operations as expected, that is,
a *reset* triggers the action *at the beginning of* system reboot.

> You should
> be able to run a command to make the update happen. How does the
> updata actually get triggered when you reset?

It's not the purpose of this test.

-Takahiro Akashi

> 
> [..]
> 
> Regards,
> Simon
Simon Glass March 14, 2022, 2:15 a.m. UTC | #28
Hi Takahiro,

On Sun, 13 Mar 2022 at 19:08, AKASHI Takahiro
<takahiro.akashi@linaro.org> wrote:
>
> Hi Simon,
>
> On Fri, Mar 11, 2022 at 07:24:39PM -0700, Simon Glass wrote:
> > Hi Takahiro,
> >
> > On Fri, 18 Feb 2022 at 18:16, AKASHI Takahiro
> > <takahiro.akashi@linaro.org> wrote:
> > >
> > > On Fri, Feb 18, 2022 at 02:48:54PM +0100, Heinrich Schuchardt wrote:
> > > > On 2/18/22 03:16, Masami Hiramatsu wrote:
> > > > > Hi Simon,
> > > > >
> > > > > Thank you for your reply.
> > > > >
> > > > > 2022年2月18日(金) 2:56 Simon Glass <sjg@chromium.org>:
> > > > >
> > > > > >
> > > > > > Hi Masami,
> > > > > >
> > > > > > On Wed, 16 Feb 2022 at 18:11, Masami Hiramatsu
> > > > > > <masami.hiramatsu@linaro.org> wrote:
> > > > > > >
> > > > > > > Hi Simon,
> > > > > > >
> > > > > > > Let me confirm your point.
> > > > > > > So are you concerning the 'real' reset for the capsule update test
> > > > > > > case itself or this patch?
> > > > > > >
> > > > > > > I'm actually learning how the test is working, so please help me to
> > > > > > > understand how I can solve it.
> > > > > > >
> > > > > > > There are 3 environments to run the test, sandbox, Qemu, and a real board.
> > > > > > > If we reset a sandbox, it will continue to run (just restart itself),
> > > > > >
> > > > > > Here you should be able to avoid doing a reset. See
> > > > > > dm_test_sysreset_base() which tests sysreset drivers on sandbox.
> > > > >
> > > > > Would you mean that reset-after-capsule-on-disk itself should not
> > > > > make a reset on sandbox?
> > > >
> > > > We have several tests that do resets by calling do_reset(), e.g. the
> > > > UEFI unit tests. There is nothing wrong about it.
> > > >
> > > > We want the sandbox to behave like any other board where capsule updates
> > > > lead to resets.
> > > >
> > > > >
> > > > > In dm_test_sysreset_base(), I can see the below code, this means
> > > > > sysreset_request()
> > > > > will not execute real reset, but just mimic the reset, right?
> > > > >
> > > > > state->sysreset_allowed[SYSRESET_WARM] = true;
> > > > > ut_asserteq(-EINPROGRESS, sysreset_request(dev, SYSRESET_WARM));
> > > > > state->sysreset_allowed[SYSRESET_WARM] = false;
> > > > >
> > > > > > > but Qemu and real board will cause a real reset and it will terminate
> > > > > > > the qemu or stop the board (depends on how it is implemented). Thus,
> > > > > > > if a command or boot process will cause a reset, it will need a
> > > > > > > special care (maybe respawn?).
> > > > > >
> > > > > > Here you need to worry about the surrounding automation logic which
> > > > > > could be tbot of the U-Boot pytest hooks. I suggest you avoid this and
> > > > > > handle it some other way, without reset.
> > > >
> > > > The sandbox should run through exactly the same code path as all other
> > > > boards to get a meaningful test results. Therefore don't put in any
> > > > quirks on C level. Your Python test changes are all that is needed.
> > >
> > > +1, I have the same opinion here.
> > > To exercise capsule-on-disk code, we need a "real" reset
> > > because pytest/CI loop is basically a black-box test.
> >
> > I don't see why you need the reset at all to test the code.
>
> As I repeatedly said, I believe that this is a black-box test and
> a system test. The purpose of the test is to make sure the firmware
> update be performed in real operations as expected, that is,
> a *reset* triggers the action *at the beginning of* system reboot.

I understand you are frustrated with this, but I just don't agree, or
perhaps don't understand.

What specific mechanism is used to initiate the firmware update? Is
this happening in U-Boot or somewhere else?

>
> > You should
> > be able to run a command to make the update happen. How does the
> > updata actually get triggered when you reset?
>
> It's not the purpose of this test.

Then drop the reset and design the feature with testing in mind.

Regards,
SImon
AKASHI Takahiro March 14, 2022, 2:42 a.m. UTC | #29
On Sun, Mar 13, 2022 at 08:15:02PM -0600, Simon Glass wrote:
> Hi Takahiro,
> 
> On Sun, 13 Mar 2022 at 19:08, AKASHI Takahiro
> <takahiro.akashi@linaro.org> wrote:
> >
> > Hi Simon,
> >
> > On Fri, Mar 11, 2022 at 07:24:39PM -0700, Simon Glass wrote:
> > > Hi Takahiro,
> > >
> > > On Fri, 18 Feb 2022 at 18:16, AKASHI Takahiro
> > > <takahiro.akashi@linaro.org> wrote:
> > > >
> > > > On Fri, Feb 18, 2022 at 02:48:54PM +0100, Heinrich Schuchardt wrote:
> > > > > On 2/18/22 03:16, Masami Hiramatsu wrote:
> > > > > > Hi Simon,
> > > > > >
> > > > > > Thank you for your reply.
> > > > > >
> > > > > > 2022年2月18日(金) 2:56 Simon Glass <sjg@chromium.org>:
> > > > > >
> > > > > > >
> > > > > > > Hi Masami,
> > > > > > >
> > > > > > > On Wed, 16 Feb 2022 at 18:11, Masami Hiramatsu
> > > > > > > <masami.hiramatsu@linaro.org> wrote:
> > > > > > > >
> > > > > > > > Hi Simon,
> > > > > > > >
> > > > > > > > Let me confirm your point.
> > > > > > > > So are you concerning the 'real' reset for the capsule update test
> > > > > > > > case itself or this patch?
> > > > > > > >
> > > > > > > > I'm actually learning how the test is working, so please help me to
> > > > > > > > understand how I can solve it.
> > > > > > > >
> > > > > > > > There are 3 environments to run the test, sandbox, Qemu, and a real board.
> > > > > > > > If we reset a sandbox, it will continue to run (just restart itself),
> > > > > > >
> > > > > > > Here you should be able to avoid doing a reset. See
> > > > > > > dm_test_sysreset_base() which tests sysreset drivers on sandbox.
> > > > > >
> > > > > > Would you mean that reset-after-capsule-on-disk itself should not
> > > > > > make a reset on sandbox?
> > > > >
> > > > > We have several tests that do resets by calling do_reset(), e.g. the
> > > > > UEFI unit tests. There is nothing wrong about it.
> > > > >
> > > > > We want the sandbox to behave like any other board where capsule updates
> > > > > lead to resets.
> > > > >
> > > > > >
> > > > > > In dm_test_sysreset_base(), I can see the below code, this means
> > > > > > sysreset_request()
> > > > > > will not execute real reset, but just mimic the reset, right?
> > > > > >
> > > > > > state->sysreset_allowed[SYSRESET_WARM] = true;
> > > > > > ut_asserteq(-EINPROGRESS, sysreset_request(dev, SYSRESET_WARM));
> > > > > > state->sysreset_allowed[SYSRESET_WARM] = false;
> > > > > >
> > > > > > > > but Qemu and real board will cause a real reset and it will terminate
> > > > > > > > the qemu or stop the board (depends on how it is implemented). Thus,
> > > > > > > > if a command or boot process will cause a reset, it will need a
> > > > > > > > special care (maybe respawn?).
> > > > > > >
> > > > > > > Here you need to worry about the surrounding automation logic which
> > > > > > > could be tbot of the U-Boot pytest hooks. I suggest you avoid this and
> > > > > > > handle it some other way, without reset.
> > > > >
> > > > > The sandbox should run through exactly the same code path as all other
> > > > > boards to get a meaningful test results. Therefore don't put in any
> > > > > quirks on C level. Your Python test changes are all that is needed.
> > > >
> > > > +1, I have the same opinion here.
> > > > To exercise capsule-on-disk code, we need a "real" reset
> > > > because pytest/CI loop is basically a black-box test.
> > >
> > > I don't see why you need the reset at all to test the code.
> >
> > As I repeatedly said, I believe that this is a black-box test and
> > a system test. The purpose of the test is to make sure the firmware
> > update be performed in real operations as expected, that is,
> > a *reset* triggers the action *at the beginning of* system reboot.
> 
> I understand you are frustrated with this, but I just don't agree, or
> perhaps don't understand.
> 
> What specific mechanism is used to initiate the firmware update? Is
> this happening in U-Boot or somewhere else?

There are two ways:
a. CapsuleUpdate runtime service
b. capsule delivery on disk

My original patch provides only (b), partly, because runtime
service is a bit hard to implement under the current framework.

UEFI specification requires that (b) can/should be initiated
by a *reset by a user* and another reset be automatically triggered by UEFI
when the update has been completed either successfully or in vain.
The latter behavior has been enforced on U-BOOT UEFI implementation
by Masami's patch (not this series).

Masami's patch (this series) fixes issues around those two resets
in pytest.

> >
> > > You should
> > > be able to run a command to make the update happen. How does the
> > > updata actually get triggered when you reset?
> >
> > It's not the purpose of this test.
> 
> Then drop the reset and design the feature with testing in mind.

So it's just beyond of my scope.

-Takahiro Akashi

> Regards,
> SImon
Simon Glass March 14, 2022, 6:45 a.m. UTC | #30
Hi Takahiro,

On Sun, 13 Mar 2022 at 20:43, AKASHI Takahiro
<takahiro.akashi@linaro.org> wrote:
>
> On Sun, Mar 13, 2022 at 08:15:02PM -0600, Simon Glass wrote:
> > Hi Takahiro,
> >
> > On Sun, 13 Mar 2022 at 19:08, AKASHI Takahiro
> > <takahiro.akashi@linaro.org> wrote:
> > >
> > > Hi Simon,
> > >
> > > On Fri, Mar 11, 2022 at 07:24:39PM -0700, Simon Glass wrote:
> > > > Hi Takahiro,
> > > >
> > > > On Fri, 18 Feb 2022 at 18:16, AKASHI Takahiro
> > > > <takahiro.akashi@linaro.org> wrote:
> > > > >
> > > > > On Fri, Feb 18, 2022 at 02:48:54PM +0100, Heinrich Schuchardt wrote:
> > > > > > On 2/18/22 03:16, Masami Hiramatsu wrote:
> > > > > > > Hi Simon,
> > > > > > >
> > > > > > > Thank you for your reply.
> > > > > > >
> > > > > > > 2022年2月18日(金) 2:56 Simon Glass <sjg@chromium.org>:
> > > > > > >
> > > > > > > >
> > > > > > > > Hi Masami,
> > > > > > > >
> > > > > > > > On Wed, 16 Feb 2022 at 18:11, Masami Hiramatsu
> > > > > > > > <masami.hiramatsu@linaro.org> wrote:
> > > > > > > > >
> > > > > > > > > Hi Simon,
> > > > > > > > >
> > > > > > > > > Let me confirm your point.
> > > > > > > > > So are you concerning the 'real' reset for the capsule update test
> > > > > > > > > case itself or this patch?
> > > > > > > > >
> > > > > > > > > I'm actually learning how the test is working, so please help me to
> > > > > > > > > understand how I can solve it.
> > > > > > > > >
> > > > > > > > > There are 3 environments to run the test, sandbox, Qemu, and a real board.
> > > > > > > > > If we reset a sandbox, it will continue to run (just restart itself),
> > > > > > > >
> > > > > > > > Here you should be able to avoid doing a reset. See
> > > > > > > > dm_test_sysreset_base() which tests sysreset drivers on sandbox.
> > > > > > >
> > > > > > > Would you mean that reset-after-capsule-on-disk itself should not
> > > > > > > make a reset on sandbox?
> > > > > >
> > > > > > We have several tests that do resets by calling do_reset(), e.g. the
> > > > > > UEFI unit tests. There is nothing wrong about it.
> > > > > >
> > > > > > We want the sandbox to behave like any other board where capsule updates
> > > > > > lead to resets.
> > > > > >
> > > > > > >
> > > > > > > In dm_test_sysreset_base(), I can see the below code, this means
> > > > > > > sysreset_request()
> > > > > > > will not execute real reset, but just mimic the reset, right?
> > > > > > >
> > > > > > > state->sysreset_allowed[SYSRESET_WARM] = true;
> > > > > > > ut_asserteq(-EINPROGRESS, sysreset_request(dev, SYSRESET_WARM));
> > > > > > > state->sysreset_allowed[SYSRESET_WARM] = false;
> > > > > > >
> > > > > > > > > but Qemu and real board will cause a real reset and it will terminate
> > > > > > > > > the qemu or stop the board (depends on how it is implemented). Thus,
> > > > > > > > > if a command or boot process will cause a reset, it will need a
> > > > > > > > > special care (maybe respawn?).
> > > > > > > >
> > > > > > > > Here you need to worry about the surrounding automation logic which
> > > > > > > > could be tbot of the U-Boot pytest hooks. I suggest you avoid this and
> > > > > > > > handle it some other way, without reset.
> > > > > >
> > > > > > The sandbox should run through exactly the same code path as all other
> > > > > > boards to get a meaningful test results. Therefore don't put in any
> > > > > > quirks on C level. Your Python test changes are all that is needed.
> > > > >
> > > > > +1, I have the same opinion here.
> > > > > To exercise capsule-on-disk code, we need a "real" reset
> > > > > because pytest/CI loop is basically a black-box test.
> > > >
> > > > I don't see why you need the reset at all to test the code.
> > >
> > > As I repeatedly said, I believe that this is a black-box test and
> > > a system test. The purpose of the test is to make sure the firmware
> > > update be performed in real operations as expected, that is,
> > > a *reset* triggers the action *at the beginning of* system reboot.
> >
> > I understand you are frustrated with this, but I just don't agree, or
> > perhaps don't understand.
> >
> > What specific mechanism is used to initiate the firmware update? Is
> > this happening in U-Boot or somewhere else?
>
> There are two ways:
> a. CapsuleUpdate runtime service
> b. capsule delivery on disk
>
> My original patch provides only (b), partly, because runtime
> service is a bit hard to implement under the current framework.
>
> UEFI specification requires that (b) can/should be initiated
> by a *reset by a user* and another reset be automatically triggered by UEFI
> when the update has been completed either successfully or in vain.
> The latter behavior has been enforced on U-BOOT UEFI implementation
> by Masami's patch (not this series).

OK, well 'reset by a user' presumably starts the board up and then
runs some code to do the update in U-Boot? Is that right? If so, we
just need to trigger that update from the test. We don't need to test
the actual reset, at least not with sandbox. As I said, we need to
write the code so that it is easy to test.

>
> Masami's patch (this series) fixes issues around those two resets
> in pytest.

Yes and that is the problem I have, at least on sandbox.

Regards,
Simon

>
> > >
> > > > You should
> > > > be able to run a command to make the update happen. How does the
> > > > updata actually get triggered when you reset?
> > >
> > > It's not the purpose of this test.
> >
> > Then drop the reset and design the feature with testing in mind.
>
> So it's just beyond of my scope.
>
> -Takahiro Akashi
>
> > Regards,
> > SImon
Masami Hiramatsu March 14, 2022, 7:35 a.m. UTC | #31
Hi Simon,

2022年3月14日(月) 15:45 Simon Glass <sjg@chromium.org>:
>
> Hi Takahiro,
>
> On Sun, 13 Mar 2022 at 20:43, AKASHI Takahiro
> <takahiro.akashi@linaro.org> wrote:
> >
> > On Sun, Mar 13, 2022 at 08:15:02PM -0600, Simon Glass wrote:
> > > Hi Takahiro,
> > >
> > > On Sun, 13 Mar 2022 at 19:08, AKASHI Takahiro
> > > <takahiro.akashi@linaro.org> wrote:
> > > >
> > > > Hi Simon,
> > > >
> > > > On Fri, Mar 11, 2022 at 07:24:39PM -0700, Simon Glass wrote:
> > > > > Hi Takahiro,
> > > > >
> > > > > On Fri, 18 Feb 2022 at 18:16, AKASHI Takahiro
> > > > > <takahiro.akashi@linaro.org> wrote:
> > > > > >
> > > > > > On Fri, Feb 18, 2022 at 02:48:54PM +0100, Heinrich Schuchardt wrote:
> > > > > > > On 2/18/22 03:16, Masami Hiramatsu wrote:
> > > > > > > > Hi Simon,
> > > > > > > >
> > > > > > > > Thank you for your reply.
> > > > > > > >
> > > > > > > > 2022年2月18日(金) 2:56 Simon Glass <sjg@chromium.org>:
> > > > > > > >
> > > > > > > > >
> > > > > > > > > Hi Masami,
> > > > > > > > >
> > > > > > > > > On Wed, 16 Feb 2022 at 18:11, Masami Hiramatsu
> > > > > > > > > <masami.hiramatsu@linaro.org> wrote:
> > > > > > > > > >
> > > > > > > > > > Hi Simon,
> > > > > > > > > >
> > > > > > > > > > Let me confirm your point.
> > > > > > > > > > So are you concerning the 'real' reset for the capsule update test
> > > > > > > > > > case itself or this patch?
> > > > > > > > > >
> > > > > > > > > > I'm actually learning how the test is working, so please help me to
> > > > > > > > > > understand how I can solve it.
> > > > > > > > > >
> > > > > > > > > > There are 3 environments to run the test, sandbox, Qemu, and a real board.
> > > > > > > > > > If we reset a sandbox, it will continue to run (just restart itself),
> > > > > > > > >
> > > > > > > > > Here you should be able to avoid doing a reset. See
> > > > > > > > > dm_test_sysreset_base() which tests sysreset drivers on sandbox.
> > > > > > > >
> > > > > > > > Would you mean that reset-after-capsule-on-disk itself should not
> > > > > > > > make a reset on sandbox?
> > > > > > >
> > > > > > > We have several tests that do resets by calling do_reset(), e.g. the
> > > > > > > UEFI unit tests. There is nothing wrong about it.
> > > > > > >
> > > > > > > We want the sandbox to behave like any other board where capsule updates
> > > > > > > lead to resets.
> > > > > > >
> > > > > > > >
> > > > > > > > In dm_test_sysreset_base(), I can see the below code, this means
> > > > > > > > sysreset_request()
> > > > > > > > will not execute real reset, but just mimic the reset, right?
> > > > > > > >
> > > > > > > > state->sysreset_allowed[SYSRESET_WARM] = true;
> > > > > > > > ut_asserteq(-EINPROGRESS, sysreset_request(dev, SYSRESET_WARM));
> > > > > > > > state->sysreset_allowed[SYSRESET_WARM] = false;
> > > > > > > >
> > > > > > > > > > but Qemu and real board will cause a real reset and it will terminate
> > > > > > > > > > the qemu or stop the board (depends on how it is implemented). Thus,
> > > > > > > > > > if a command or boot process will cause a reset, it will need a
> > > > > > > > > > special care (maybe respawn?).
> > > > > > > > >
> > > > > > > > > Here you need to worry about the surrounding automation logic which
> > > > > > > > > could be tbot of the U-Boot pytest hooks. I suggest you avoid this and
> > > > > > > > > handle it some other way, without reset.
> > > > > > >
> > > > > > > The sandbox should run through exactly the same code path as all other
> > > > > > > boards to get a meaningful test results. Therefore don't put in any
> > > > > > > quirks on C level. Your Python test changes are all that is needed.
> > > > > >
> > > > > > +1, I have the same opinion here.
> > > > > > To exercise capsule-on-disk code, we need a "real" reset
> > > > > > because pytest/CI loop is basically a black-box test.
> > > > >
> > > > > I don't see why you need the reset at all to test the code.
> > > >
> > > > As I repeatedly said, I believe that this is a black-box test and
> > > > a system test. The purpose of the test is to make sure the firmware
> > > > update be performed in real operations as expected, that is,
> > > > a *reset* triggers the action *at the beginning of* system reboot.
> > >
> > > I understand you are frustrated with this, but I just don't agree, or
> > > perhaps don't understand.
> > >
> > > What specific mechanism is used to initiate the firmware update? Is
> > > this happening in U-Boot or somewhere else?
> >
> > There are two ways:
> > a. CapsuleUpdate runtime service
> > b. capsule delivery on disk
> >
> > My original patch provides only (b), partly, because runtime
> > service is a bit hard to implement under the current framework.
> >
> > UEFI specification requires that (b) can/should be initiated
> > by a *reset by a user* and another reset be automatically triggered by UEFI
> > when the update has been completed either successfully or in vain.
> > The latter behavior has been enforced on U-BOOT UEFI implementation
> > by Masami's patch (not this series).
>
> OK, well 'reset by a user' presumably starts the board up and then
> runs some code to do the update in U-Boot? Is that right? If so, we
> just need to trigger that update from the test. We don't need to test
> the actual reset, at least not with sandbox. As I said, we need to
> write the code so that it is easy to test.

Actually, we already have that command, "efidebug capsule disk-update"
which kicks the capsule update code even without the 'reset by a
user'. So we can just kick this command for checking whether the
U-Boot UEFI code correctly find the capsule file from ESP which
specified by UEFI vars.

However, the 'capsule update on-disk' feature is also expected (and
defined in the spec?) to run when the UEFI subsystem is initialized.
This behavior will not be tested if we skip the 'reset by a user'. I
guess Takahiro's current test case tries to check it.

> > Masami's patch (this series) fixes issues around those two resets
> > in pytest.
>
> Yes and that is the problem I have, at least on sandbox.

So If I I call sysreset_walk_halt(SYSRESET_COLD) after capsule update,
it could help?

Thank you,

>
> Regards,
> Simon
>
> >
> > > >
> > > > > You should
> > > > > be able to run a command to make the update happen. How does the
> > > > > updata actually get triggered when you reset?
> > > >
> > > > It's not the purpose of this test.
> > >
> > > Then drop the reset and design the feature with testing in mind.
> >
> > So it's just beyond of my scope.
> >
> > -Takahiro Akashi
> >
> > > Regards,
> > > SImon
AKASHI Takahiro March 14, 2022, 8:03 a.m. UTC | #32
On Mon, Mar 14, 2022 at 04:35:45PM +0900, Masami Hiramatsu wrote:
> Hi Simon,
> 
> 2022年3月14日(月) 15:45 Simon Glass <sjg@chromium.org>:
> >
> > Hi Takahiro,
> >
> > On Sun, 13 Mar 2022 at 20:43, AKASHI Takahiro
> > <takahiro.akashi@linaro.org> wrote:
> > >
> > > On Sun, Mar 13, 2022 at 08:15:02PM -0600, Simon Glass wrote:
> > > > Hi Takahiro,
> > > >
> > > > On Sun, 13 Mar 2022 at 19:08, AKASHI Takahiro
> > > > <takahiro.akashi@linaro.org> wrote:
> > > > >
> > > > > Hi Simon,
> > > > >
> > > > > On Fri, Mar 11, 2022 at 07:24:39PM -0700, Simon Glass wrote:
> > > > > > Hi Takahiro,
> > > > > >
> > > > > > On Fri, 18 Feb 2022 at 18:16, AKASHI Takahiro
> > > > > > <takahiro.akashi@linaro.org> wrote:
> > > > > > >
> > > > > > > On Fri, Feb 18, 2022 at 02:48:54PM +0100, Heinrich Schuchardt wrote:
> > > > > > > > On 2/18/22 03:16, Masami Hiramatsu wrote:
> > > > > > > > > Hi Simon,
> > > > > > > > >
> > > > > > > > > Thank you for your reply.
> > > > > > > > >
> > > > > > > > > 2022年2月18日(金) 2:56 Simon Glass <sjg@chromium.org>:
> > > > > > > > >
> > > > > > > > > >
> > > > > > > > > > Hi Masami,
> > > > > > > > > >
> > > > > > > > > > On Wed, 16 Feb 2022 at 18:11, Masami Hiramatsu
> > > > > > > > > > <masami.hiramatsu@linaro.org> wrote:
> > > > > > > > > > >
> > > > > > > > > > > Hi Simon,
> > > > > > > > > > >
> > > > > > > > > > > Let me confirm your point.
> > > > > > > > > > > So are you concerning the 'real' reset for the capsule update test
> > > > > > > > > > > case itself or this patch?
> > > > > > > > > > >
> > > > > > > > > > > I'm actually learning how the test is working, so please help me to
> > > > > > > > > > > understand how I can solve it.
> > > > > > > > > > >
> > > > > > > > > > > There are 3 environments to run the test, sandbox, Qemu, and a real board.
> > > > > > > > > > > If we reset a sandbox, it will continue to run (just restart itself),
> > > > > > > > > >
> > > > > > > > > > Here you should be able to avoid doing a reset. See
> > > > > > > > > > dm_test_sysreset_base() which tests sysreset drivers on sandbox.
> > > > > > > > >
> > > > > > > > > Would you mean that reset-after-capsule-on-disk itself should not
> > > > > > > > > make a reset on sandbox?
> > > > > > > >
> > > > > > > > We have several tests that do resets by calling do_reset(), e.g. the
> > > > > > > > UEFI unit tests. There is nothing wrong about it.
> > > > > > > >
> > > > > > > > We want the sandbox to behave like any other board where capsule updates
> > > > > > > > lead to resets.
> > > > > > > >
> > > > > > > > >
> > > > > > > > > In dm_test_sysreset_base(), I can see the below code, this means
> > > > > > > > > sysreset_request()
> > > > > > > > > will not execute real reset, but just mimic the reset, right?
> > > > > > > > >
> > > > > > > > > state->sysreset_allowed[SYSRESET_WARM] = true;
> > > > > > > > > ut_asserteq(-EINPROGRESS, sysreset_request(dev, SYSRESET_WARM));
> > > > > > > > > state->sysreset_allowed[SYSRESET_WARM] = false;
> > > > > > > > >
> > > > > > > > > > > but Qemu and real board will cause a real reset and it will terminate
> > > > > > > > > > > the qemu or stop the board (depends on how it is implemented). Thus,
> > > > > > > > > > > if a command or boot process will cause a reset, it will need a
> > > > > > > > > > > special care (maybe respawn?).
> > > > > > > > > >
> > > > > > > > > > Here you need to worry about the surrounding automation logic which
> > > > > > > > > > could be tbot of the U-Boot pytest hooks. I suggest you avoid this and
> > > > > > > > > > handle it some other way, without reset.
> > > > > > > >
> > > > > > > > The sandbox should run through exactly the same code path as all other
> > > > > > > > boards to get a meaningful test results. Therefore don't put in any
> > > > > > > > quirks on C level. Your Python test changes are all that is needed.
> > > > > > >
> > > > > > > +1, I have the same opinion here.
> > > > > > > To exercise capsule-on-disk code, we need a "real" reset
> > > > > > > because pytest/CI loop is basically a black-box test.
> > > > > >
> > > > > > I don't see why you need the reset at all to test the code.
> > > > >
> > > > > As I repeatedly said, I believe that this is a black-box test and
> > > > > a system test. The purpose of the test is to make sure the firmware
> > > > > update be performed in real operations as expected, that is,
> > > > > a *reset* triggers the action *at the beginning of* system reboot.
> > > >
> > > > I understand you are frustrated with this, but I just don't agree, or
> > > > perhaps don't understand.
> > > >
> > > > What specific mechanism is used to initiate the firmware update? Is
> > > > this happening in U-Boot or somewhere else?
> > >
> > > There are two ways:
> > > a. CapsuleUpdate runtime service
> > > b. capsule delivery on disk
> > >
> > > My original patch provides only (b), partly, because runtime
> > > service is a bit hard to implement under the current framework.
> > >
> > > UEFI specification requires that (b) can/should be initiated
> > > by a *reset by a user* and another reset be automatically triggered by UEFI
> > > when the update has been completed either successfully or in vain.
> > > The latter behavior has been enforced on U-BOOT UEFI implementation
> > > by Masami's patch (not this series).
> >
> > OK, well 'reset by a user' presumably starts the board up and then
> > runs some code to do the update in U-Boot? Is that right? If so, we
> > just need to trigger that update from the test. We don't need to test
> > the actual reset, at least not with sandbox. As I said, we need to
> > write the code so that it is easy to test.
> 
> Actually, we already have that command, "efidebug capsule disk-update"
> which kicks the capsule update code even without the 'reset by a
> user'. So we can just kick this command for checking whether the
> U-Boot UEFI code correctly find the capsule file from ESP which
> specified by UEFI vars.

I oppose it simply

> However, the 'capsule update on-disk' feature is also expected (and
> defined in the spec?) to run when the UEFI subsystem is initialized.
> This behavior will not be tested if we skip the 'reset by a user'. I
> guess Takahiro's current test case tries to check it.

because of this.

System test should not use an internal function, it should only
exercise public interfaces from user's viewpoint.

-Takahiro Akashi

> > > Masami's patch (this series) fixes issues around those two resets
> > > in pytest.
> >
> > Yes and that is the problem I have, at least on sandbox.
> 
> So If I I call sysreset_walk_halt(SYSRESET_COLD) after capsule update,
> it could help?
> 
> Thank you,
> 
> >
> > Regards,
> > Simon
> >
> > >
> > > > >
> > > > > > You should
> > > > > > be able to run a command to make the update happen. How does the
> > > > > > updata actually get triggered when you reset?
> > > > >
> > > > > It's not the purpose of this test.
> > > >
> > > > Then drop the reset and design the feature with testing in mind.
> > >
> > > So it's just beyond of my scope.
> > >
> > > -Takahiro Akashi
> > >
> > > > Regards,
> > > > SImon
> 
> 
> 
> -- 
> Masami Hiramatsu
Simon Glass March 14, 2022, 6:24 p.m. UTC | #33
Hi Masami,

On Mon, 14 Mar 2022 at 01:35, Masami Hiramatsu
<masami.hiramatsu@linaro.org> wrote:
>
> Hi Simon,
>
> 2022年3月14日(月) 15:45 Simon Glass <sjg@chromium.org>:
> >
> > Hi Takahiro,
> >
> > On Sun, 13 Mar 2022 at 20:43, AKASHI Takahiro
> > <takahiro.akashi@linaro.org> wrote:
> > >
> > > On Sun, Mar 13, 2022 at 08:15:02PM -0600, Simon Glass wrote:
> > > > Hi Takahiro,
> > > >
> > > > On Sun, 13 Mar 2022 at 19:08, AKASHI Takahiro
> > > > <takahiro.akashi@linaro.org> wrote:
> > > > >
> > > > > Hi Simon,
> > > > >
> > > > > On Fri, Mar 11, 2022 at 07:24:39PM -0700, Simon Glass wrote:
> > > > > > Hi Takahiro,
> > > > > >
> > > > > > On Fri, 18 Feb 2022 at 18:16, AKASHI Takahiro
> > > > > > <takahiro.akashi@linaro.org> wrote:
> > > > > > >
> > > > > > > On Fri, Feb 18, 2022 at 02:48:54PM +0100, Heinrich Schuchardt wrote:
> > > > > > > > On 2/18/22 03:16, Masami Hiramatsu wrote:
> > > > > > > > > Hi Simon,
> > > > > > > > >
> > > > > > > > > Thank you for your reply.
> > > > > > > > >
> > > > > > > > > 2022年2月18日(金) 2:56 Simon Glass <sjg@chromium.org>:
> > > > > > > > >
> > > > > > > > > >
> > > > > > > > > > Hi Masami,
> > > > > > > > > >
> > > > > > > > > > On Wed, 16 Feb 2022 at 18:11, Masami Hiramatsu
> > > > > > > > > > <masami.hiramatsu@linaro.org> wrote:
> > > > > > > > > > >
> > > > > > > > > > > Hi Simon,
> > > > > > > > > > >
> > > > > > > > > > > Let me confirm your point.
> > > > > > > > > > > So are you concerning the 'real' reset for the capsule update test
> > > > > > > > > > > case itself or this patch?
> > > > > > > > > > >
> > > > > > > > > > > I'm actually learning how the test is working, so please help me to
> > > > > > > > > > > understand how I can solve it.
> > > > > > > > > > >
> > > > > > > > > > > There are 3 environments to run the test, sandbox, Qemu, and a real board.
> > > > > > > > > > > If we reset a sandbox, it will continue to run (just restart itself),
> > > > > > > > > >
> > > > > > > > > > Here you should be able to avoid doing a reset. See
> > > > > > > > > > dm_test_sysreset_base() which tests sysreset drivers on sandbox.
> > > > > > > > >
> > > > > > > > > Would you mean that reset-after-capsule-on-disk itself should not
> > > > > > > > > make a reset on sandbox?
> > > > > > > >
> > > > > > > > We have several tests that do resets by calling do_reset(), e.g. the
> > > > > > > > UEFI unit tests. There is nothing wrong about it.
> > > > > > > >
> > > > > > > > We want the sandbox to behave like any other board where capsule updates
> > > > > > > > lead to resets.
> > > > > > > >
> > > > > > > > >
> > > > > > > > > In dm_test_sysreset_base(), I can see the below code, this means
> > > > > > > > > sysreset_request()
> > > > > > > > > will not execute real reset, but just mimic the reset, right?
> > > > > > > > >
> > > > > > > > > state->sysreset_allowed[SYSRESET_WARM] = true;
> > > > > > > > > ut_asserteq(-EINPROGRESS, sysreset_request(dev, SYSRESET_WARM));
> > > > > > > > > state->sysreset_allowed[SYSRESET_WARM] = false;
> > > > > > > > >
> > > > > > > > > > > but Qemu and real board will cause a real reset and it will terminate
> > > > > > > > > > > the qemu or stop the board (depends on how it is implemented). Thus,
> > > > > > > > > > > if a command or boot process will cause a reset, it will need a
> > > > > > > > > > > special care (maybe respawn?).
> > > > > > > > > >
> > > > > > > > > > Here you need to worry about the surrounding automation logic which
> > > > > > > > > > could be tbot of the U-Boot pytest hooks. I suggest you avoid this and
> > > > > > > > > > handle it some other way, without reset.
> > > > > > > >
> > > > > > > > The sandbox should run through exactly the same code path as all other
> > > > > > > > boards to get a meaningful test results. Therefore don't put in any
> > > > > > > > quirks on C level. Your Python test changes are all that is needed.
> > > > > > >
> > > > > > > +1, I have the same opinion here.
> > > > > > > To exercise capsule-on-disk code, we need a "real" reset
> > > > > > > because pytest/CI loop is basically a black-box test.
> > > > > >
> > > > > > I don't see why you need the reset at all to test the code.
> > > > >
> > > > > As I repeatedly said, I believe that this is a black-box test and
> > > > > a system test. The purpose of the test is to make sure the firmware
> > > > > update be performed in real operations as expected, that is,
> > > > > a *reset* triggers the action *at the beginning of* system reboot.
> > > >
> > > > I understand you are frustrated with this, but I just don't agree, or
> > > > perhaps don't understand.
> > > >
> > > > What specific mechanism is used to initiate the firmware update? Is
> > > > this happening in U-Boot or somewhere else?
> > >
> > > There are two ways:
> > > a. CapsuleUpdate runtime service
> > > b. capsule delivery on disk
> > >
> > > My original patch provides only (b), partly, because runtime
> > > service is a bit hard to implement under the current framework.
> > >
> > > UEFI specification requires that (b) can/should be initiated
> > > by a *reset by a user* and another reset be automatically triggered by UEFI
> > > when the update has been completed either successfully or in vain.
> > > The latter behavior has been enforced on U-BOOT UEFI implementation
> > > by Masami's patch (not this series).
> >
> > OK, well 'reset by a user' presumably starts the board up and then
> > runs some code to do the update in U-Boot? Is that right? If so, we
> > just need to trigger that update from the test. We don't need to test
> > the actual reset, at least not with sandbox. As I said, we need to
> > write the code so that it is easy to test.
>
> Actually, we already have that command, "efidebug capsule disk-update"
> which kicks the capsule update code even without the 'reset by a
> user'. So we can just kick this command for checking whether the
> U-Boot UEFI code correctly find the capsule file from ESP which
> specified by UEFI vars.
>
> However, the 'capsule update on-disk' feature is also expected (and
> defined in the spec?) to run when the UEFI subsystem is initialized.
> This behavior will not be tested if we skip the 'reset by a user'. I
> guess Takahiro's current test case tries to check it.

The 'UEFI subsystem is intialised' is a problem, actually, since if it
were better integrated into driver model, it would not have separate
structures or they would be present and enabled when driver model is.
I hope that it can be fixed and Takahiro's series is a start in that
direction.

But as to a test that an update is called when UEFI starts, that seems
like a single line of code. Sure it is nice to test it, but it is much
more important to test the installation of the update and the
execution of the update. I suppose another way to test that is  to
shut down the UEFI subsystem and start it up?

Anyway we should design subsystems so they are easy to test.

>
> > > Masami's patch (this series) fixes issues around those two resets
> > > in pytest.
> >
> > Yes and that is the problem I have, at least on sandbox.
>
> So If I I call sysreset_walk_halt(SYSRESET_COLD) after capsule update,
> it could help?

Yes that can help, because sandbox can detect that and turn it into a nop.

Regards,
Simon
[..]

> > >
> > > > >
> > > > > > You should
> > > > > > be able to run a command to make the update happen. How does the
> > > > > > updata actually get triggered when you reset?
> > > > >
> > > > > It's not the purpose of this test.
> > > >
> > > > Then drop the reset and design the feature with testing in mind.
> > >
> > > So it's just beyond of my scope.
> > >
> > > -Takahiro Akashi
> > >
> > > > Regards,
> > > > SImon
Masami Hiramatsu March 15, 2022, 12:40 a.m. UTC | #34
Hi Simon,

2022年3月15日(火) 3:24 Simon Glass <sjg@chromium.org>:
>
> > > OK, well 'reset by a user' presumably starts the board up and then
> > > runs some code to do the update in U-Boot? Is that right? If so, we
> > > just need to trigger that update from the test. We don't need to test
> > > the actual reset, at least not with sandbox. As I said, we need to
> > > write the code so that it is easy to test.
> >
> > Actually, we already have that command, "efidebug capsule disk-update"
> > which kicks the capsule update code even without the 'reset by a
> > user'. So we can just kick this command for checking whether the
> > U-Boot UEFI code correctly find the capsule file from ESP which
> > specified by UEFI vars.
> >
> > However, the 'capsule update on-disk' feature is also expected (and
> > defined in the spec?) to run when the UEFI subsystem is initialized.
> > This behavior will not be tested if we skip the 'reset by a user'. I
> > guess Takahiro's current test case tries to check it.
>
> The 'UEFI subsystem is intialised' is a problem, actually, since if it
> were better integrated into driver model, it would not have separate
> structures or they would be present and enabled when driver model is.
> I hope that it can be fixed and Takahiro's series is a start in that
> direction.

OK.

> But as to a test that an update is called when UEFI starts, that seems
> like a single line of code. Sure it is nice to test it, but it is much
> more important to test the installation of the update and the
> execution of the update. I suppose another way to test that is  to
> shut down the UEFI subsystem and start it up?

Yes, currently we call do_reset() after install the capsule file.
(This reset can be avoided if we replace it with
sysreset_walk_halt(SYSRESET_COLD) as you said, right?)

Here is how I tested it on my machine;

> usb start
> fatload usb 0 $kernel_addr_r test.cap
> fatwrite mmc 0 $fileaddr EFI/UpdateCapsule/test.cap $filesize
> efidebug capsule disk-update
(run install process and reboot the machine)

So, if we can avoid the last reset, we can test the below without
reset on sandbox (depends on scenarios).
- confirm that the capsule update on disk can find the capsule file
from ESP specified by the BOOTXXXX EFI variable.
- confirm that the capsule update on disk writes the firmware
correctly to the storage which specified by DFU.
- confirm that the capsule update on disk success if the capsule image
type is supported.
- confirm that the capsule update on disk fails if the capsule image
type is not supported.
- confirm that the capsule update on disk will reboot after update
even if the update is failed.

The only spec we can not test is
- confirm that the capsule update on disk is kicked when the UEFI is
initialized.

>
> Anyway we should design subsystems so they are easy to test.

Here I guess you mean the unit test, not system test, am I correct?

>
> >
> > > > Masami's patch (this series) fixes issues around those two resets
> > > > in pytest.
> > >
> > > Yes and that is the problem I have, at least on sandbox.
> >
> > So If I I call sysreset_walk_halt(SYSRESET_COLD) after capsule update,
> > it could help?
>
> Yes that can help, because sandbox can detect that and turn it into a nop.

OK, let me submit a patch to update it.

Thank you,

>
> Regards,
> Simon
> [..]
>
> > > >
> > > > > >
> > > > > > > You should
> > > > > > > be able to run a command to make the update happen. How does the
> > > > > > > updata actually get triggered when you reset?
> > > > > >
> > > > > > It's not the purpose of this test.
> > > > >
> > > > > Then drop the reset and design the feature with testing in mind.
> > > >
> > > > So it's just beyond of my scope.
> > > >
> > > > -Takahiro Akashi
> > > >
> > > > > Regards,
> > > > > SImon
AKASHI Takahiro March 15, 2022, 5:02 a.m. UTC | #35
On Tue, Mar 15, 2022 at 09:40:34AM +0900, Masami Hiramatsu wrote:
> Hi Simon,
> 
> 2022年3月15日(火) 3:24 Simon Glass <sjg@chromium.org>:
> >
> > > > OK, well 'reset by a user' presumably starts the board up and then
> > > > runs some code to do the update in U-Boot? Is that right? If so, we
> > > > just need to trigger that update from the test. We don't need to test
> > > > the actual reset, at least not with sandbox. As I said, we need to
> > > > write the code so that it is easy to test.
> > >
> > > Actually, we already have that command, "efidebug capsule disk-update"
> > > which kicks the capsule update code even without the 'reset by a
> > > user'. So we can just kick this command for checking whether the
> > > U-Boot UEFI code correctly find the capsule file from ESP which
> > > specified by UEFI vars.
> > >
> > > However, the 'capsule update on-disk' feature is also expected (and
> > > defined in the spec?) to run when the UEFI subsystem is initialized.
> > > This behavior will not be tested if we skip the 'reset by a user'. I
> > > guess Takahiro's current test case tries to check it.
> >
> > The 'UEFI subsystem is intialised' is a problem, actually, since if it
> > were better integrated into driver model, it would not have separate
> > structures or they would be present and enabled when driver model is.
> > I hope that it can be fixed and Takahiro's series is a start in that
> > direction.
> 
> OK.
> 
> > But as to a test that an update is called when UEFI starts, that seems
> > like a single line of code. Sure it is nice to test it, but it is much
> > more important to test the installation of the update and the
> > execution of the update. I suppose another way to test that is  to
> > shut down the UEFI subsystem and start it up?
> 
> Yes, currently we call do_reset() after install the capsule file.
> (This reset can be avoided if we replace it with
> sysreset_walk_halt(SYSRESET_COLD) as you said, right?)
> 
> Here is how I tested it on my machine;
> 
> > usb start
> > fatload usb 0 $kernel_addr_r test.cap
> > fatwrite mmc 0 $fileaddr EFI/UpdateCapsule/test.cap $filesize
> > efidebug capsule disk-update
> (run install process and reboot the machine)
> 
> So, if we can avoid the last reset, we can test the below without
> reset on sandbox (depends on scenarios).
> - confirm that the capsule update on disk can find the capsule file
> from ESP specified by the BOOTXXXX EFI variable.
> - confirm that the capsule update on disk writes the firmware
> correctly to the storage which specified by DFU.
> - confirm that the capsule update on disk success if the capsule image
> type is supported.
> - confirm that the capsule update on disk fails if the capsule image
> type is not supported.
> - confirm that the capsule update on disk will reboot after update
> even if the update is failed.
> 
> The only spec we can not test is
> - confirm that the capsule update on disk is kicked when the UEFI is
> initialized.

It is important to verify this feature in system test like pytest.

> >
> > Anyway we should design subsystems so they are easy to test.

Anyway we should design systems test so that they emulate operations
in real world.

-Takahiro Akashi

> Here I guess you mean the unit test, not system test, am I correct?
> 
> >
> > >
> > > > > Masami's patch (this series) fixes issues around those two resets
> > > > > in pytest.
> > > >
> > > > Yes and that is the problem I have, at least on sandbox.
> > >
> > > So If I I call sysreset_walk_halt(SYSRESET_COLD) after capsule update,
> > > it could help?
> >
> > Yes that can help, because sandbox can detect that and turn it into a nop.
> 
> OK, let me submit a patch to update it.
> 
> Thank you,
> 
> >
> > Regards,
> > Simon
> > [..]
> >
> > > > >
> > > > > > >
> > > > > > > > You should
> > > > > > > > be able to run a command to make the update happen. How does the
> > > > > > > > updata actually get triggered when you reset?
> > > > > > >
> > > > > > > It's not the purpose of this test.
> > > > > >
> > > > > > Then drop the reset and design the feature with testing in mind.
> > > > >
> > > > > So it's just beyond of my scope.
> > > > >
> > > > > -Takahiro Akashi
> > > > >
> > > > > > Regards,
> > > > > > SImon
> 
> 
> 
> -- 
> Masami Hiramatsu
Simon Glass March 15, 2022, 5:04 a.m. UTC | #36
Hi Masami,

On Mon, 14 Mar 2022 at 18:40, Masami Hiramatsu
<masami.hiramatsu@linaro.org> wrote:
>
> Hi Simon,
>
> 2022年3月15日(火) 3:24 Simon Glass <sjg@chromium.org>:
> >
> > > > OK, well 'reset by a user' presumably starts the board up and then
> > > > runs some code to do the update in U-Boot? Is that right? If so, we
> > > > just need to trigger that update from the test. We don't need to test
> > > > the actual reset, at least not with sandbox. As I said, we need to
> > > > write the code so that it is easy to test.
> > >
> > > Actually, we already have that command, "efidebug capsule disk-update"
> > > which kicks the capsule update code even without the 'reset by a
> > > user'. So we can just kick this command for checking whether the
> > > U-Boot UEFI code correctly find the capsule file from ESP which
> > > specified by UEFI vars.
> > >
> > > However, the 'capsule update on-disk' feature is also expected (and
> > > defined in the spec?) to run when the UEFI subsystem is initialized.
> > > This behavior will not be tested if we skip the 'reset by a user'. I
> > > guess Takahiro's current test case tries to check it.
> >
> > The 'UEFI subsystem is intialised' is a problem, actually, since if it
> > were better integrated into driver model, it would not have separate
> > structures or they would be present and enabled when driver model is.
> > I hope that it can be fixed and Takahiro's series is a start in that
> > direction.
>
> OK.
>
> > But as to a test that an update is called when UEFI starts, that seems
> > like a single line of code. Sure it is nice to test it, but it is much
> > more important to test the installation of the update and the
> > execution of the update. I suppose another way to test that is  to
> > shut down the UEFI subsystem and start it up?
>
> Yes, currently we call do_reset() after install the capsule file.
> (This reset can be avoided if we replace it with
> sysreset_walk_halt(SYSRESET_COLD) as you said, right?)
>
> Here is how I tested it on my machine;
>
> > usb start
> > fatload usb 0 $kernel_addr_r test.cap
> > fatwrite mmc 0 $fileaddr EFI/UpdateCapsule/test.cap $filesize
> > efidebug capsule disk-update
> (run install process and reboot the machine)
>
> So, if we can avoid the last reset, we can test the below without
> reset on sandbox (depends on scenarios).
> - confirm that the capsule update on disk can find the capsule file
> from ESP specified by the BOOTXXXX EFI variable.
> - confirm that the capsule update on disk writes the firmware
> correctly to the storage which specified by DFU.
> - confirm that the capsule update on disk success if the capsule image
> type is supported.
> - confirm that the capsule update on disk fails if the capsule image
> type is not supported.
> - confirm that the capsule update on disk will reboot after update
> even if the update is failed.
>
> The only spec we can not test is
> - confirm that the capsule update on disk is kicked when the UEFI is
> initialized.

Even that could be tested, by installing an update and then initing UEFI?

>
> >
> > Anyway we should design subsystems so they are easy to test.
>
> Here I guess you mean the unit test, not system test, am I correct?

Yes. Easy testing is so important for developer productivity and
happiness. It is fine to have large system/functional tests as a fall
back or catch-all, but they tend to test the happy path only. When
they fail, they are hard to debug because they cover such as large
area of the code and they often have complex setup requirements so are
hard to run manually.

My hope is that all the functionality should be covered by unit tests
or integration tests, so that system/functional almost never fail.

>
> >
> > >
> > > > > Masami's patch (this series) fixes issues around those two resets
> > > > > in pytest.
> > > >
> > > > Yes and that is the problem I have, at least on sandbox.
> > >
> > > So If I I call sysreset_walk_halt(SYSRESET_COLD) after capsule update,
> > > it could help?
> >
> > Yes that can help, because sandbox can detect that and turn it into a nop.
>
> OK, let me submit a patch to update it.

OK thank you.

Regards,
Simon
Masami Hiramatsu March 15, 2022, 8:36 a.m. UTC | #37
Hi Simon,

2022年3月15日(火) 14:04 Simon Glass <sjg@chromium.org>:
>
> Hi Masami,
>
> On Mon, 14 Mar 2022 at 18:40, Masami Hiramatsu
> <masami.hiramatsu@linaro.org> wrote:
> >
> > Hi Simon,
> >
> > 2022年3月15日(火) 3:24 Simon Glass <sjg@chromium.org>:
> > >
> > > > > OK, well 'reset by a user' presumably starts the board up and then
> > > > > runs some code to do the update in U-Boot? Is that right? If so, we
> > > > > just need to trigger that update from the test. We don't need to test
> > > > > the actual reset, at least not with sandbox. As I said, we need to
> > > > > write the code so that it is easy to test.
> > > >
> > > > Actually, we already have that command, "efidebug capsule disk-update"
> > > > which kicks the capsule update code even without the 'reset by a
> > > > user'. So we can just kick this command for checking whether the
> > > > U-Boot UEFI code correctly find the capsule file from ESP which
> > > > specified by UEFI vars.
> > > >
> > > > However, the 'capsule update on-disk' feature is also expected (and
> > > > defined in the spec?) to run when the UEFI subsystem is initialized.
> > > > This behavior will not be tested if we skip the 'reset by a user'. I
> > > > guess Takahiro's current test case tries to check it.
> > >
> > > The 'UEFI subsystem is intialised' is a problem, actually, since if it
> > > were better integrated into driver model, it would not have separate
> > > structures or they would be present and enabled when driver model is.
> > > I hope that it can be fixed and Takahiro's series is a start in that
> > > direction.
> >
> > OK.
> >
> > > But as to a test that an update is called when UEFI starts, that seems
> > > like a single line of code. Sure it is nice to test it, but it is much
> > > more important to test the installation of the update and the
> > > execution of the update. I suppose another way to test that is  to
> > > shut down the UEFI subsystem and start it up?
> >
> > Yes, currently we call do_reset() after install the capsule file.
> > (This reset can be avoided if we replace it with
> > sysreset_walk_halt(SYSRESET_COLD) as you said, right?)
> >
> > Here is how I tested it on my machine;
> >
> > > usb start
> > > fatload usb 0 $kernel_addr_r test.cap
> > > fatwrite mmc 0 $fileaddr EFI/UpdateCapsule/test.cap $filesize
> > > efidebug capsule disk-update
> > (run install process and reboot the machine)
> >
> > So, if we can avoid the last reset, we can test the below without
> > reset on sandbox (depends on scenarios).
> > - confirm that the capsule update on disk can find the capsule file
> > from ESP specified by the BOOTXXXX EFI variable.
> > - confirm that the capsule update on disk writes the firmware
> > correctly to the storage which specified by DFU.
> > - confirm that the capsule update on disk success if the capsule image
> > type is supported.
> > - confirm that the capsule update on disk fails if the capsule image
> > type is not supported.
> > - confirm that the capsule update on disk will reboot after update
> > even if the update is failed.
> >
> > The only spec we can not test is
> > - confirm that the capsule update on disk is kicked when the UEFI is
> > initialized.
>
> Even that could be tested, by installing an update and then initing UEFI?

yeah, if the UEFI is not initialized yet, we can run some UEFI related
command (e.g. printenv -e) instead of efidebug capsule... to execute
the capsule update on disk.
But anyway, this is only available at the first time. We need a way to
reset UEFI subsystem without system reset.


> > >
> > > Anyway we should design subsystems so they are easy to test.
> >
> > Here I guess you mean the unit test, not system test, am I correct?
>
> Yes. Easy testing is so important for developer productivity and
> happiness. It is fine to have large system/functional tests as a fall
> back or catch-all, but they tend to test the happy path only. When
> they fail, they are hard to debug because they cover such as large
> area of the code and they often have complex setup requirements so are
> hard to run manually.
>
> My hope is that all the functionality should be covered by unit tests
> or integration tests, so that system/functional almost never fail.

My another question is how small is the granularity of the unit test.
As I showed, the UEFI capsule update needs to prepare a capsule file
installed in the storage.
That seems to be very system-level. But you think that is still be a unit test?
(I expected that the 'Unit test' is something like KUnit in Linux)

Thank you,

>
> >
> > >
> > > >
> > > > > > Masami's patch (this series) fixes issues around those two resets
> > > > > > in pytest.
> > > > >
> > > > > Yes and that is the problem I have, at least on sandbox.
> > > >
> > > > So If I I call sysreset_walk_halt(SYSRESET_COLD) after capsule update,
> > > > it could help?
> > >
> > > Yes that can help, because sandbox can detect that and turn it into a nop.
> >
> > OK, let me submit a patch to update it.
>
> OK thank you.
>
> Regards,
> Simon



--
Masami Hiramatsu
Simon Glass March 16, 2022, 3:13 a.m. UTC | #38
Hi Masami,

On Tue, 15 Mar 2022 at 02:36, Masami Hiramatsu
<masami.hiramatsu@linaro.org> wrote:
>
> Hi Simon,
>
> 2022年3月15日(火) 14:04 Simon Glass <sjg@chromium.org>:
> >
> > Hi Masami,
> >
> > On Mon, 14 Mar 2022 at 18:40, Masami Hiramatsu
> > <masami.hiramatsu@linaro.org> wrote:
> > >
> > > Hi Simon,
> > >
> > > 2022年3月15日(火) 3:24 Simon Glass <sjg@chromium.org>:
> > > >
> > > > > > OK, well 'reset by a user' presumably starts the board up and then
> > > > > > runs some code to do the update in U-Boot? Is that right? If so, we
> > > > > > just need to trigger that update from the test. We don't need to test
> > > > > > the actual reset, at least not with sandbox. As I said, we need to
> > > > > > write the code so that it is easy to test.
> > > > >
> > > > > Actually, we already have that command, "efidebug capsule disk-update"
> > > > > which kicks the capsule update code even without the 'reset by a
> > > > > user'. So we can just kick this command for checking whether the
> > > > > U-Boot UEFI code correctly find the capsule file from ESP which
> > > > > specified by UEFI vars.
> > > > >
> > > > > However, the 'capsule update on-disk' feature is also expected (and
> > > > > defined in the spec?) to run when the UEFI subsystem is initialized.
> > > > > This behavior will not be tested if we skip the 'reset by a user'. I
> > > > > guess Takahiro's current test case tries to check it.
> > > >
> > > > The 'UEFI subsystem is intialised' is a problem, actually, since if it
> > > > were better integrated into driver model, it would not have separate
> > > > structures or they would be present and enabled when driver model is.
> > > > I hope that it can be fixed and Takahiro's series is a start in that
> > > > direction.
> > >
> > > OK.
> > >
> > > > But as to a test that an update is called when UEFI starts, that seems
> > > > like a single line of code. Sure it is nice to test it, but it is much
> > > > more important to test the installation of the update and the
> > > > execution of the update. I suppose another way to test that is  to
> > > > shut down the UEFI subsystem and start it up?
> > >
> > > Yes, currently we call do_reset() after install the capsule file.
> > > (This reset can be avoided if we replace it with
> > > sysreset_walk_halt(SYSRESET_COLD) as you said, right?)
> > >
> > > Here is how I tested it on my machine;
> > >
> > > > usb start
> > > > fatload usb 0 $kernel_addr_r test.cap
> > > > fatwrite mmc 0 $fileaddr EFI/UpdateCapsule/test.cap $filesize
> > > > efidebug capsule disk-update
> > > (run install process and reboot the machine)
> > >
> > > So, if we can avoid the last reset, we can test the below without
> > > reset on sandbox (depends on scenarios).
> > > - confirm that the capsule update on disk can find the capsule file
> > > from ESP specified by the BOOTXXXX EFI variable.
> > > - confirm that the capsule update on disk writes the firmware
> > > correctly to the storage which specified by DFU.
> > > - confirm that the capsule update on disk success if the capsule image
> > > type is supported.
> > > - confirm that the capsule update on disk fails if the capsule image
> > > type is not supported.
> > > - confirm that the capsule update on disk will reboot after update
> > > even if the update is failed.
> > >
> > > The only spec we can not test is
> > > - confirm that the capsule update on disk is kicked when the UEFI is
> > > initialized.
> >
> > Even that could be tested, by installing an update and then initing UEFI?
>
> yeah, if the UEFI is not initialized yet, we can run some UEFI related
> command (e.g. printenv -e) instead of efidebug capsule... to execute
> the capsule update on disk.
> But anyway, this is only available at the first time. We need a way to
> reset UEFI subsystem without system reset.

Yes. It is certainly possible, but I'm not sure how easy it is.
Perhaps just drop all the EFI data structures and run the EFI init
again? We have something similar with driver model. See
dm_test_pre_run()

>
>
> > > >
> > > > Anyway we should design subsystems so they are easy to test.
> > >
> > > Here I guess you mean the unit test, not system test, am I correct?
> >
> > Yes. Easy testing is so important for developer productivity and
> > happiness. It is fine to have large system/functional tests as a fall
> > back or catch-all, but they tend to test the happy path only. When
> > they fail, they are hard to debug because they cover such as large
> > area of the code and they often have complex setup requirements so are
> > hard to run manually.
> >
> > My hope is that all the functionality should be covered by unit tests
> > or integration tests, so that system/functional almost never fail.
>
> My another question is how small is the granularity of the unit test.
> As I showed, the UEFI capsule update needs to prepare a capsule file
> installed in the storage.
> That seems to be very system-level. But you think that is still be a unit test?
> (I expected that the 'Unit test' is something like KUnit in Linux)

Well I am using your terminology here. Technically many of the U-Boot
tests (executed by 'ut' command) are not really unit tests. They bring
in a lot of code and run one test case using it.

For example, one of the tests brings up the USB subsystem, including a
fake USB stick, then checks it can read data from the stick, using the
USB stack.

Another one writes some things to the emulated display and then checks
that the correct pixels are there.

Perhaps a better name would be integration test. But the point is that
we can run these tests very, very quickly and (setup aside) without
outside influence, or without restarting the executable, etc.

> >
> > >
> > > >
> > > > >
> > > > > > > Masami's patch (this series) fixes issues around those two resets
> > > > > > > in pytest.
> > > > > >
> > > > > > Yes and that is the problem I have, at least on sandbox.
> > > > >
> > > > > So If I I call sysreset_walk_halt(SYSRESET_COLD) after capsule update,
> > > > > it could help?
> > > >
> > > > Yes that can help, because sandbox can detect that and turn it into a nop.
> > >
> > > OK, let me submit a patch to update it.
> >
> > OK thank you.

Regards,
SImon
AKASHI Takahiro March 16, 2022, 3:26 a.m. UTC | #39
On Tue, Mar 15, 2022 at 09:13:15PM -0600, Simon Glass wrote:
> Hi Masami,
> 
> On Tue, 15 Mar 2022 at 02:36, Masami Hiramatsu
> <masami.hiramatsu@linaro.org> wrote:
> >
> > Hi Simon,
> >
> > 2022年3月15日(火) 14:04 Simon Glass <sjg@chromium.org>:
> > >
> > > Hi Masami,
> > >
> > > On Mon, 14 Mar 2022 at 18:40, Masami Hiramatsu
> > > <masami.hiramatsu@linaro.org> wrote:
> > > >
> > > > Hi Simon,
> > > >
> > > > 2022年3月15日(火) 3:24 Simon Glass <sjg@chromium.org>:
> > > > >
> > > > > > > OK, well 'reset by a user' presumably starts the board up and then
> > > > > > > runs some code to do the update in U-Boot? Is that right? If so, we
> > > > > > > just need to trigger that update from the test. We don't need to test
> > > > > > > the actual reset, at least not with sandbox. As I said, we need to
> > > > > > > write the code so that it is easy to test.
> > > > > >
> > > > > > Actually, we already have that command, "efidebug capsule disk-update"
> > > > > > which kicks the capsule update code even without the 'reset by a
> > > > > > user'. So we can just kick this command for checking whether the
> > > > > > U-Boot UEFI code correctly find the capsule file from ESP which
> > > > > > specified by UEFI vars.
> > > > > >
> > > > > > However, the 'capsule update on-disk' feature is also expected (and
> > > > > > defined in the spec?) to run when the UEFI subsystem is initialized.
> > > > > > This behavior will not be tested if we skip the 'reset by a user'. I
> > > > > > guess Takahiro's current test case tries to check it.
> > > > >
> > > > > The 'UEFI subsystem is intialised' is a problem, actually, since if it
> > > > > were better integrated into driver model, it would not have separate
> > > > > structures or they would be present and enabled when driver model is.
> > > > > I hope that it can be fixed and Takahiro's series is a start in that
> > > > > direction.
> > > >
> > > > OK.
> > > >
> > > > > But as to a test that an update is called when UEFI starts, that seems
> > > > > like a single line of code. Sure it is nice to test it, but it is much
> > > > > more important to test the installation of the update and the
> > > > > execution of the update. I suppose another way to test that is  to
> > > > > shut down the UEFI subsystem and start it up?
> > > >
> > > > Yes, currently we call do_reset() after install the capsule file.
> > > > (This reset can be avoided if we replace it with
> > > > sysreset_walk_halt(SYSRESET_COLD) as you said, right?)
> > > >
> > > > Here is how I tested it on my machine;
> > > >
> > > > > usb start
> > > > > fatload usb 0 $kernel_addr_r test.cap
> > > > > fatwrite mmc 0 $fileaddr EFI/UpdateCapsule/test.cap $filesize
> > > > > efidebug capsule disk-update
> > > > (run install process and reboot the machine)
> > > >
> > > > So, if we can avoid the last reset, we can test the below without
> > > > reset on sandbox (depends on scenarios).
> > > > - confirm that the capsule update on disk can find the capsule file
> > > > from ESP specified by the BOOTXXXX EFI variable.
> > > > - confirm that the capsule update on disk writes the firmware
> > > > correctly to the storage which specified by DFU.
> > > > - confirm that the capsule update on disk success if the capsule image
> > > > type is supported.
> > > > - confirm that the capsule update on disk fails if the capsule image
> > > > type is not supported.
> > > > - confirm that the capsule update on disk will reboot after update
> > > > even if the update is failed.
> > > >
> > > > The only spec we can not test is
> > > > - confirm that the capsule update on disk is kicked when the UEFI is
> > > > initialized.
> > >
> > > Even that could be tested, by installing an update and then initing UEFI?
> >
> > yeah, if the UEFI is not initialized yet, we can run some UEFI related
> > command (e.g. printenv -e) instead of efidebug capsule... to execute
> > the capsule update on disk.
> > But anyway, this is only available at the first time. We need a way to
> > reset UEFI subsystem without system reset.
> 
> Yes. It is certainly possible, but I'm not sure how easy it is.
> Perhaps just drop all the EFI data structures and run the EFI init
> again? We have something similar with driver model. See
> dm_test_pre_run()
> 
> >
> >
> > > > >
> > > > > Anyway we should design subsystems so they are easy to test.
> > > >
> > > > Here I guess you mean the unit test, not system test, am I correct?
> > >
> > > Yes. Easy testing is so important for developer productivity and
> > > happiness. It is fine to have large system/functional tests as a fall
> > > back or catch-all, but they tend to test the happy path only. When
> > > they fail, they are hard to debug because they cover such as large
> > > area of the code and they often have complex setup requirements so are
> > > hard to run manually.
> > >
> > > My hope is that all the functionality should be covered by unit tests
> > > or integration tests, so that system/functional almost never fail.
> >
> > My another question is how small is the granularity of the unit test.
> > As I showed, the UEFI capsule update needs to prepare a capsule file
> > installed in the storage.
> > That seems to be very system-level. But you think that is still be a unit test?
> > (I expected that the 'Unit test' is something like KUnit in Linux)
> 
> Well I am using your terminology here. Technically many of the U-Boot
> tests (executed by 'ut' command) are not really unit tests. They bring
> in a lot of code and run one test case using it.
> 
> For example, one of the tests brings up the USB subsystem, including a
> fake USB stick, then checks it can read data from the stick, using the
> USB stack.
> 
> Another one writes some things to the emulated display and then checks
> that the correct pixels are there.
> 
> Perhaps a better name would be integration test. But the point is that
> we can run these tests very, very quickly and (setup aside) without
> outside influence, or without restarting the executable, etc.

I don't really understand why you want and need to avoid a real reset
in, what you call, integration test.

-Takahiro Akashi


> > >
> > > >
> > > > >
> > > > > >
> > > > > > > > Masami's patch (this series) fixes issues around those two resets
> > > > > > > > in pytest.
> > > > > > >
> > > > > > > Yes and that is the problem I have, at least on sandbox.
> > > > > >
> > > > > > So If I I call sysreset_walk_halt(SYSRESET_COLD) after capsule update,
> > > > > > it could help?
> > > > >
> > > > > Yes that can help, because sandbox can detect that and turn it into a nop.
> > > >
> > > > OK, let me submit a patch to update it.
> > >
> > > OK thank you.
> 
> Regards,
> SImon
Masami Hiramatsu March 16, 2022, 6:09 a.m. UTC | #40
Hi Simon,

2022年3月16日(水) 12:13 Simon Glass <sjg@chromium.org>:
>
> Hi Masami,
>
> On Tue, 15 Mar 2022 at 02:36, Masami Hiramatsu
> <masami.hiramatsu@linaro.org> wrote:
> >
> > Hi Simon,
> >
> > 2022年3月15日(火) 14:04 Simon Glass <sjg@chromium.org>:
> > >
> > > Hi Masami,
> > >
> > > On Mon, 14 Mar 2022 at 18:40, Masami Hiramatsu
> > > <masami.hiramatsu@linaro.org> wrote:
> > > >
> > > > Hi Simon,
> > > >
> > > > 2022年3月15日(火) 3:24 Simon Glass <sjg@chromium.org>:
> > > > >
> > > > > > > OK, well 'reset by a user' presumably starts the board up and then
> > > > > > > runs some code to do the update in U-Boot? Is that right? If so, we
> > > > > > > just need to trigger that update from the test. We don't need to test
> > > > > > > the actual reset, at least not with sandbox. As I said, we need to
> > > > > > > write the code so that it is easy to test.
> > > > > >
> > > > > > Actually, we already have that command, "efidebug capsule disk-update"
> > > > > > which kicks the capsule update code even without the 'reset by a
> > > > > > user'. So we can just kick this command for checking whether the
> > > > > > U-Boot UEFI code correctly find the capsule file from ESP which
> > > > > > specified by UEFI vars.
> > > > > >
> > > > > > However, the 'capsule update on-disk' feature is also expected (and
> > > > > > defined in the spec?) to run when the UEFI subsystem is initialized.
> > > > > > This behavior will not be tested if we skip the 'reset by a user'. I
> > > > > > guess Takahiro's current test case tries to check it.
> > > > >
> > > > > The 'UEFI subsystem is intialised' is a problem, actually, since if it
> > > > > were better integrated into driver model, it would not have separate
> > > > > structures or they would be present and enabled when driver model is.
> > > > > I hope that it can be fixed and Takahiro's series is a start in that
> > > > > direction.
> > > >
> > > > OK.
> > > >
> > > > > But as to a test that an update is called when UEFI starts, that seems
> > > > > like a single line of code. Sure it is nice to test it, but it is much
> > > > > more important to test the installation of the update and the
> > > > > execution of the update. I suppose another way to test that is  to
> > > > > shut down the UEFI subsystem and start it up?
> > > >
> > > > Yes, currently we call do_reset() after install the capsule file.
> > > > (This reset can be avoided if we replace it with
> > > > sysreset_walk_halt(SYSRESET_COLD) as you said, right?)
> > > >
> > > > Here is how I tested it on my machine;
> > > >
> > > > > usb start
> > > > > fatload usb 0 $kernel_addr_r test.cap
> > > > > fatwrite mmc 0 $fileaddr EFI/UpdateCapsule/test.cap $filesize
> > > > > efidebug capsule disk-update
> > > > (run install process and reboot the machine)
> > > >
> > > > So, if we can avoid the last reset, we can test the below without
> > > > reset on sandbox (depends on scenarios).
> > > > - confirm that the capsule update on disk can find the capsule file
> > > > from ESP specified by the BOOTXXXX EFI variable.
> > > > - confirm that the capsule update on disk writes the firmware
> > > > correctly to the storage which specified by DFU.
> > > > - confirm that the capsule update on disk success if the capsule image
> > > > type is supported.
> > > > - confirm that the capsule update on disk fails if the capsule image
> > > > type is not supported.
> > > > - confirm that the capsule update on disk will reboot after update
> > > > even if the update is failed.
> > > >
> > > > The only spec we can not test is
> > > > - confirm that the capsule update on disk is kicked when the UEFI is
> > > > initialized.
> > >
> > > Even that could be tested, by installing an update and then initing UEFI?
> >
> > yeah, if the UEFI is not initialized yet, we can run some UEFI related
> > command (e.g. printenv -e) instead of efidebug capsule... to execute
> > the capsule update on disk.
> > But anyway, this is only available at the first time. We need a way to
> > reset UEFI subsystem without system reset.
>
> Yes. It is certainly possible, but I'm not sure how easy it is.
> Perhaps just drop all the EFI data structures and run the EFI init
> again? We have something similar with driver model. See
> dm_test_pre_run()

EFI has the ExitBootServices call, but I'm not sure it is actually
clear all resources. Maybe we need to check what resources are
released by the ExitBootServices.

> > > > >
> > > > > Anyway we should design subsystems so they are easy to test.
> > > >
> > > > Here I guess you mean the unit test, not system test, am I correct?
> > >
> > > Yes. Easy testing is so important for developer productivity and
> > > happiness. It is fine to have large system/functional tests as a fall
> > > back or catch-all, but they tend to test the happy path only. When
> > > they fail, they are hard to debug because they cover such as large
> > > area of the code and they often have complex setup requirements so are
> > > hard to run manually.
> > >
> > > My hope is that all the functionality should be covered by unit tests
> > > or integration tests, so that system/functional almost never fail.
> >
> > My another question is how small is the granularity of the unit test.
> > As I showed, the UEFI capsule update needs to prepare a capsule file
> > installed in the storage.
> > That seems to be very system-level. But you think that is still be a unit test?
> > (I expected that the 'Unit test' is something like KUnit in Linux)
>
> Well I am using your terminology here. Technically many of the U-Boot
> tests (executed by 'ut' command) are not really unit tests. They bring
> in a lot of code and run one test case using it.

OK.

>
> For example, one of the tests brings up the USB subsystem, including a
> fake USB stick, then checks it can read data from the stick, using the
> USB stack.

So the fake USB stick data is generated in the build process?
If so, we also can build a fake ESP master image which already
includes a capsule file.

> Another one writes some things to the emulated display and then checks
> that the correct pixels are there.
>
> Perhaps a better name would be integration test. But the point is that
> we can run these tests very, very quickly and (setup aside) without
> outside influence, or without restarting the executable, etc.

OK.
BTW, as you said above, when we run such integration test for EFI
which includes to run sysreset, before that sandbox will switch the
sysreset driver for resetting EFI to avoid restarting it?

Thank you,
Simon Glass March 16, 2022, 7:23 p.m. UTC | #41
Hi Masami,

On Wed, 16 Mar 2022 at 00:09, Masami Hiramatsu
<masami.hiramatsu@linaro.org> wrote:
>
> Hi Simon,
>
> 2022年3月16日(水) 12:13 Simon Glass <sjg@chromium.org>:
> >
> > Hi Masami,
> >
> > On Tue, 15 Mar 2022 at 02:36, Masami Hiramatsu
> > <masami.hiramatsu@linaro.org> wrote:
> > >
> > > Hi Simon,
> > >
> > > 2022年3月15日(火) 14:04 Simon Glass <sjg@chromium.org>:
> > > >
> > > > Hi Masami,
> > > >
> > > > On Mon, 14 Mar 2022 at 18:40, Masami Hiramatsu
> > > > <masami.hiramatsu@linaro.org> wrote:
> > > > >
> > > > > Hi Simon,
> > > > >
> > > > > 2022年3月15日(火) 3:24 Simon Glass <sjg@chromium.org>:
> > > > > >
> > > > > > > > OK, well 'reset by a user' presumably starts the board up and then
> > > > > > > > runs some code to do the update in U-Boot? Is that right? If so, we
> > > > > > > > just need to trigger that update from the test. We don't need to test
> > > > > > > > the actual reset, at least not with sandbox. As I said, we need to
> > > > > > > > write the code so that it is easy to test.
> > > > > > >
> > > > > > > Actually, we already have that command, "efidebug capsule disk-update"
> > > > > > > which kicks the capsule update code even without the 'reset by a
> > > > > > > user'. So we can just kick this command for checking whether the
> > > > > > > U-Boot UEFI code correctly find the capsule file from ESP which
> > > > > > > specified by UEFI vars.
> > > > > > >
> > > > > > > However, the 'capsule update on-disk' feature is also expected (and
> > > > > > > defined in the spec?) to run when the UEFI subsystem is initialized.
> > > > > > > This behavior will not be tested if we skip the 'reset by a user'. I
> > > > > > > guess Takahiro's current test case tries to check it.
> > > > > >
> > > > > > The 'UEFI subsystem is intialised' is a problem, actually, since if it
> > > > > > were better integrated into driver model, it would not have separate
> > > > > > structures or they would be present and enabled when driver model is.
> > > > > > I hope that it can be fixed and Takahiro's series is a start in that
> > > > > > direction.
> > > > >
> > > > > OK.
> > > > >
> > > > > > But as to a test that an update is called when UEFI starts, that seems
> > > > > > like a single line of code. Sure it is nice to test it, but it is much
> > > > > > more important to test the installation of the update and the
> > > > > > execution of the update. I suppose another way to test that is  to
> > > > > > shut down the UEFI subsystem and start it up?
> > > > >
> > > > > Yes, currently we call do_reset() after install the capsule file.
> > > > > (This reset can be avoided if we replace it with
> > > > > sysreset_walk_halt(SYSRESET_COLD) as you said, right?)
> > > > >
> > > > > Here is how I tested it on my machine;
> > > > >
> > > > > > usb start
> > > > > > fatload usb 0 $kernel_addr_r test.cap
> > > > > > fatwrite mmc 0 $fileaddr EFI/UpdateCapsule/test.cap $filesize
> > > > > > efidebug capsule disk-update
> > > > > (run install process and reboot the machine)
> > > > >
> > > > > So, if we can avoid the last reset, we can test the below without
> > > > > reset on sandbox (depends on scenarios).
> > > > > - confirm that the capsule update on disk can find the capsule file
> > > > > from ESP specified by the BOOTXXXX EFI variable.
> > > > > - confirm that the capsule update on disk writes the firmware
> > > > > correctly to the storage which specified by DFU.
> > > > > - confirm that the capsule update on disk success if the capsule image
> > > > > type is supported.
> > > > > - confirm that the capsule update on disk fails if the capsule image
> > > > > type is not supported.
> > > > > - confirm that the capsule update on disk will reboot after update
> > > > > even if the update is failed.
> > > > >
> > > > > The only spec we can not test is
> > > > > - confirm that the capsule update on disk is kicked when the UEFI is
> > > > > initialized.
> > > >
> > > > Even that could be tested, by installing an update and then initing UEFI?
> > >
> > > yeah, if the UEFI is not initialized yet, we can run some UEFI related
> > > command (e.g. printenv -e) instead of efidebug capsule... to execute
> > > the capsule update on disk.
> > > But anyway, this is only available at the first time. We need a way to
> > > reset UEFI subsystem without system reset.
> >
> > Yes. It is certainly possible, but I'm not sure how easy it is.
> > Perhaps just drop all the EFI data structures and run the EFI init
> > again? We have something similar with driver model. See
> > dm_test_pre_run()
>
> EFI has the ExitBootServices call, but I'm not sure it is actually
> clear all resources. Maybe we need to check what resources are
> released by the ExitBootServices.
>
> > > > > >
> > > > > > Anyway we should design subsystems so they are easy to test.
> > > > >
> > > > > Here I guess you mean the unit test, not system test, am I correct?
> > > >
> > > > Yes. Easy testing is so important for developer productivity and
> > > > happiness. It is fine to have large system/functional tests as a fall
> > > > back or catch-all, but they tend to test the happy path only. When
> > > > they fail, they are hard to debug because they cover such as large
> > > > area of the code and they often have complex setup requirements so are
> > > > hard to run manually.
> > > >
> > > > My hope is that all the functionality should be covered by unit tests
> > > > or integration tests, so that system/functional almost never fail.
> > >
> > > My another question is how small is the granularity of the unit test.
> > > As I showed, the UEFI capsule update needs to prepare a capsule file
> > > installed in the storage.
> > > That seems to be very system-level. But you think that is still be a unit test?
> > > (I expected that the 'Unit test' is something like KUnit in Linux)
> >
> > Well I am using your terminology here. Technically many of the U-Boot
> > tests (executed by 'ut' command) are not really unit tests. They bring
> > in a lot of code and run one test case using it.
>
> OK.
>
> >
> > For example, one of the tests brings up the USB subsystem, including a
> > fake USB stick, then checks it can read data from the stick, using the
> > USB stack.
>
> So the fake USB stick data is generated in the build process?
> If so, we also can build a fake ESP master image which already
> includes a capsule file.
>
> > Another one writes some things to the emulated display and then checks
> > that the correct pixels are there.
> >
> > Perhaps a better name would be integration test. But the point is that
> > we can run these tests very, very quickly and (setup aside) without
> > outside influence, or without restarting the executable, etc.
>
> OK.
> BTW, as you said above, when we run such integration test for EFI
> which includes to run sysreset, before that sandbox will switch the
> sysreset driver for resetting EFI to avoid restarting it?

Yes. The UEFI update seems quite monolithic from what I am hearing.
Could it be split into a few separate U-Boot commands, liike:

- efi update prepare  (set up the update somewhere)
- efi update exec (do the update)

Regards,
SImon
Heinrich Schuchardt March 16, 2022, 8:41 p.m. UTC | #42
Am 16. März 2022 20:23:37 MEZ schrieb Simon Glass <sjg@chromium.org>:
>Hi Masami,
>
>On Wed, 16 Mar 2022 at 00:09, Masami Hiramatsu
><masami.hiramatsu@linaro.org> wrote:
>>
>> Hi Simon,
>>
>> 2022年3月16日(水) 12:13 Simon Glass <sjg@chromium.org>:
>> >
>> > Hi Masami,
>> >
>> > On Tue, 15 Mar 2022 at 02:36, Masami Hiramatsu
>> > <masami.hiramatsu@linaro.org> wrote:
>> > >
>> > > Hi Simon,
>> > >
>> > > 2022年3月15日(火) 14:04 Simon Glass <sjg@chromium.org>:
>> > > >
>> > > > Hi Masami,
>> > > >
>> > > > On Mon, 14 Mar 2022 at 18:40, Masami Hiramatsu
>> > > > <masami.hiramatsu@linaro.org> wrote:
>> > > > >
>> > > > > Hi Simon,
>> > > > >
>> > > > > 2022年3月15日(火) 3:24 Simon Glass <sjg@chromium.org>:
>> > > > > >
>> > > > > > > > OK, well 'reset by a user' presumably starts the board up and then
>> > > > > > > > runs some code to do the update in U-Boot? Is that right? If so, we
>> > > > > > > > just need to trigger that update from the test. We don't need to test
>> > > > > > > > the actual reset, at least not with sandbox. As I said, we need to
>> > > > > > > > write the code so that it is easy to test.
>> > > > > > >
>> > > > > > > Actually, we already have that command, "efidebug capsule disk-update"
>> > > > > > > which kicks the capsule update code even without the 'reset by a
>> > > > > > > user'. So we can just kick this command for checking whether the
>> > > > > > > U-Boot UEFI code correctly find the capsule file from ESP which
>> > > > > > > specified by UEFI vars.
>> > > > > > >
>> > > > > > > However, the 'capsule update on-disk' feature is also expected (and
>> > > > > > > defined in the spec?) to run when the UEFI subsystem is initialized.
>> > > > > > > This behavior will not be tested if we skip the 'reset by a user'. I
>> > > > > > > guess Takahiro's current test case tries to check it.
>> > > > > >
>> > > > > > The 'UEFI subsystem is intialised' is a problem, actually, since if it
>> > > > > > were better integrated into driver model, it would not have separate
>> > > > > > structures or they would be present and enabled when driver model is.
>> > > > > > I hope that it can be fixed and Takahiro's series is a start in that
>> > > > > > direction.
>> > > > >
>> > > > > OK.
>> > > > >
>> > > > > > But as to a test that an update is called when UEFI starts, that seems
>> > > > > > like a single line of code. Sure it is nice to test it, but it is much
>> > > > > > more important to test the installation of the update and the
>> > > > > > execution of the update. I suppose another way to test that is  to
>> > > > > > shut down the UEFI subsystem and start it up?
>> > > > >
>> > > > > Yes, currently we call do_reset() after install the capsule file.
>> > > > > (This reset can be avoided if we replace it with
>> > > > > sysreset_walk_halt(SYSRESET_COLD) as you said, right?)
>> > > > >
>> > > > > Here is how I tested it on my machine;
>> > > > >
>> > > > > > usb start
>> > > > > > fatload usb 0 $kernel_addr_r test.cap
>> > > > > > fatwrite mmc 0 $fileaddr EFI/UpdateCapsule/test.cap $filesize
>> > > > > > efidebug capsule disk-update
>> > > > > (run install process and reboot the machine)
>> > > > >
>> > > > > So, if we can avoid the last reset, we can test the below without
>> > > > > reset on sandbox (depends on scenarios).
>> > > > > - confirm that the capsule update on disk can find the capsule file
>> > > > > from ESP specified by the BOOTXXXX EFI variable.
>> > > > > - confirm that the capsule update on disk writes the firmware
>> > > > > correctly to the storage which specified by DFU.
>> > > > > - confirm that the capsule update on disk success if the capsule image
>> > > > > type is supported.
>> > > > > - confirm that the capsule update on disk fails if the capsule image
>> > > > > type is not supported.
>> > > > > - confirm that the capsule update on disk will reboot after update
>> > > > > even if the update is failed.
>> > > > >
>> > > > > The only spec we can not test is
>> > > > > - confirm that the capsule update on disk is kicked when the UEFI is
>> > > > > initialized.
>> > > >
>> > > > Even that could be tested, by installing an update and then initing UEFI?
>> > >
>> > > yeah, if the UEFI is not initialized yet, we can run some UEFI related
>> > > command (e.g. printenv -e) instead of efidebug capsule... to execute
>> > > the capsule update on disk.
>> > > But anyway, this is only available at the first time. We need a way to
>> > > reset UEFI subsystem without system reset.
>> >
>> > Yes. It is certainly possible, but I'm not sure how easy it is.
>> > Perhaps just drop all the EFI data structures and run the EFI init
>> > again? We have something similar with driver model. See
>> > dm_test_pre_run()
>>
>> EFI has the ExitBootServices call, but I'm not sure it is actually
>> clear all resources. Maybe we need to check what resources are
>> released by the ExitBootServices.
>>
>> > > > > >
>> > > > > > Anyway we should design subsystems so they are easy to test.
>> > > > >
>> > > > > Here I guess you mean the unit test, not system test, am I correct?
>> > > >
>> > > > Yes. Easy testing is so important for developer productivity and
>> > > > happiness. It is fine to have large system/functional tests as a fall
>> > > > back or catch-all, but they tend to test the happy path only. When
>> > > > they fail, they are hard to debug because they cover such as large
>> > > > area of the code and they often have complex setup requirements so are
>> > > > hard to run manually.
>> > > >
>> > > > My hope is that all the functionality should be covered by unit tests
>> > > > or integration tests, so that system/functional almost never fail.
>> > >
>> > > My another question is how small is the granularity of the unit test.
>> > > As I showed, the UEFI capsule update needs to prepare a capsule file
>> > > installed in the storage.
>> > > That seems to be very system-level. But you think that is still be a unit test?
>> > > (I expected that the 'Unit test' is something like KUnit in Linux)
>> >
>> > Well I am using your terminology here. Technically many of the U-Boot
>> > tests (executed by 'ut' command) are not really unit tests. They bring
>> > in a lot of code and run one test case using it.
>>
>> OK.
>>
>> >
>> > For example, one of the tests brings up the USB subsystem, including a
>> > fake USB stick, then checks it can read data from the stick, using the
>> > USB stack.
>>
>> So the fake USB stick data is generated in the build process?
>> If so, we also can build a fake ESP master image which already
>> includes a capsule file.
>>
>> > Another one writes some things to the emulated display and then checks
>> > that the correct pixels are there.
>> >
>> > Perhaps a better name would be integration test. But the point is that
>> > we can run these tests very, very quickly and (setup aside) without
>> > outside influence, or without restarting the executable, etc.
>>
>> OK.
>> BTW, as you said above, when we run such integration test for EFI
>> which includes to run sysreset, before that sandbox will switch the
>> sysreset driver for resetting EFI to avoid restarting it?
>
>Yes. The UEFI update seems quite monolithic from what I am hearing.
>Could it be split into a few separate U-Boot commands, liike:
>
>- efi update prepare  (set up the update somewhere)
>- efi update exec (do the update)

Capsule updates are not done via the command line.

Either you call the UpdateCapsule() service or you set a flag in the OsIndications variable and provide a file in a predefined path.

Best regards 

Heinrich


>
>Regards,
>SImon
Simon Glass March 17, 2022, 12:29 a.m. UTC | #43
Hi Heinrich,

On Wed, 16 Mar 2022 at 14:47, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>
>
>
> Am 16. März 2022 20:23:37 MEZ schrieb Simon Glass <sjg@chromium.org>:
> >Hi Masami,
> >
> >On Wed, 16 Mar 2022 at 00:09, Masami Hiramatsu
> ><masami.hiramatsu@linaro.org> wrote:
> >>
> >> Hi Simon,
> >>
> >> 2022年3月16日(水) 12:13 Simon Glass <sjg@chromium.org>:
> >> >
> >> > Hi Masami,
> >> >
> >> > On Tue, 15 Mar 2022 at 02:36, Masami Hiramatsu
> >> > <masami.hiramatsu@linaro.org> wrote:
> >> > >
> >> > > Hi Simon,
> >> > >
> >> > > 2022年3月15日(火) 14:04 Simon Glass <sjg@chromium.org>:
> >> > > >
> >> > > > Hi Masami,
> >> > > >
> >> > > > On Mon, 14 Mar 2022 at 18:40, Masami Hiramatsu
> >> > > > <masami.hiramatsu@linaro.org> wrote:
> >> > > > >
> >> > > > > Hi Simon,
> >> > > > >
> >> > > > > 2022年3月15日(火) 3:24 Simon Glass <sjg@chromium.org>:
> >> > > > > >
> >> > > > > > > > OK, well 'reset by a user' presumably starts the board up and then
> >> > > > > > > > runs some code to do the update in U-Boot? Is that right? If so, we
> >> > > > > > > > just need to trigger that update from the test. We don't need to test
> >> > > > > > > > the actual reset, at least not with sandbox. As I said, we need to
> >> > > > > > > > write the code so that it is easy to test.
> >> > > > > > >
> >> > > > > > > Actually, we already have that command, "efidebug capsule disk-update"
> >> > > > > > > which kicks the capsule update code even without the 'reset by a
> >> > > > > > > user'. So we can just kick this command for checking whether the
> >> > > > > > > U-Boot UEFI code correctly find the capsule file from ESP which
> >> > > > > > > specified by UEFI vars.
> >> > > > > > >
> >> > > > > > > However, the 'capsule update on-disk' feature is also expected (and
> >> > > > > > > defined in the spec?) to run when the UEFI subsystem is initialized.
> >> > > > > > > This behavior will not be tested if we skip the 'reset by a user'. I
> >> > > > > > > guess Takahiro's current test case tries to check it.
> >> > > > > >
> >> > > > > > The 'UEFI subsystem is intialised' is a problem, actually, since if it
> >> > > > > > were better integrated into driver model, it would not have separate
> >> > > > > > structures or they would be present and enabled when driver model is.
> >> > > > > > I hope that it can be fixed and Takahiro's series is a start in that
> >> > > > > > direction.
> >> > > > >
> >> > > > > OK.
> >> > > > >
> >> > > > > > But as to a test that an update is called when UEFI starts, that seems
> >> > > > > > like a single line of code. Sure it is nice to test it, but it is much
> >> > > > > > more important to test the installation of the update and the
> >> > > > > > execution of the update. I suppose another way to test that is  to
> >> > > > > > shut down the UEFI subsystem and start it up?
> >> > > > >
> >> > > > > Yes, currently we call do_reset() after install the capsule file.
> >> > > > > (This reset can be avoided if we replace it with
> >> > > > > sysreset_walk_halt(SYSRESET_COLD) as you said, right?)
> >> > > > >
> >> > > > > Here is how I tested it on my machine;
> >> > > > >
> >> > > > > > usb start
> >> > > > > > fatload usb 0 $kernel_addr_r test.cap
> >> > > > > > fatwrite mmc 0 $fileaddr EFI/UpdateCapsule/test.cap $filesize
> >> > > > > > efidebug capsule disk-update
> >> > > > > (run install process and reboot the machine)
> >> > > > >
> >> > > > > So, if we can avoid the last reset, we can test the below without
> >> > > > > reset on sandbox (depends on scenarios).
> >> > > > > - confirm that the capsule update on disk can find the capsule file
> >> > > > > from ESP specified by the BOOTXXXX EFI variable.
> >> > > > > - confirm that the capsule update on disk writes the firmware
> >> > > > > correctly to the storage which specified by DFU.
> >> > > > > - confirm that the capsule update on disk success if the capsule image
> >> > > > > type is supported.
> >> > > > > - confirm that the capsule update on disk fails if the capsule image
> >> > > > > type is not supported.
> >> > > > > - confirm that the capsule update on disk will reboot after update
> >> > > > > even if the update is failed.
> >> > > > >
> >> > > > > The only spec we can not test is
> >> > > > > - confirm that the capsule update on disk is kicked when the UEFI is
> >> > > > > initialized.
> >> > > >
> >> > > > Even that could be tested, by installing an update and then initing UEFI?
> >> > >
> >> > > yeah, if the UEFI is not initialized yet, we can run some UEFI related
> >> > > command (e.g. printenv -e) instead of efidebug capsule... to execute
> >> > > the capsule update on disk.
> >> > > But anyway, this is only available at the first time. We need a way to
> >> > > reset UEFI subsystem without system reset.
> >> >
> >> > Yes. It is certainly possible, but I'm not sure how easy it is.
> >> > Perhaps just drop all the EFI data structures and run the EFI init
> >> > again? We have something similar with driver model. See
> >> > dm_test_pre_run()
> >>
> >> EFI has the ExitBootServices call, but I'm not sure it is actually
> >> clear all resources. Maybe we need to check what resources are
> >> released by the ExitBootServices.
> >>
> >> > > > > >
> >> > > > > > Anyway we should design subsystems so they are easy to test.
> >> > > > >
> >> > > > > Here I guess you mean the unit test, not system test, am I correct?
> >> > > >
> >> > > > Yes. Easy testing is so important for developer productivity and
> >> > > > happiness. It is fine to have large system/functional tests as a fall
> >> > > > back or catch-all, but they tend to test the happy path only. When
> >> > > > they fail, they are hard to debug because they cover such as large
> >> > > > area of the code and they often have complex setup requirements so are
> >> > > > hard to run manually.
> >> > > >
> >> > > > My hope is that all the functionality should be covered by unit tests
> >> > > > or integration tests, so that system/functional almost never fail.
> >> > >
> >> > > My another question is how small is the granularity of the unit test.
> >> > > As I showed, the UEFI capsule update needs to prepare a capsule file
> >> > > installed in the storage.
> >> > > That seems to be very system-level. But you think that is still be a unit test?
> >> > > (I expected that the 'Unit test' is something like KUnit in Linux)
> >> >
> >> > Well I am using your terminology here. Technically many of the U-Boot
> >> > tests (executed by 'ut' command) are not really unit tests. They bring
> >> > in a lot of code and run one test case using it.
> >>
> >> OK.
> >>
> >> >
> >> > For example, one of the tests brings up the USB subsystem, including a
> >> > fake USB stick, then checks it can read data from the stick, using the
> >> > USB stack.
> >>
> >> So the fake USB stick data is generated in the build process?
> >> If so, we also can build a fake ESP master image which already
> >> includes a capsule file.
> >>
> >> > Another one writes some things to the emulated display and then checks
> >> > that the correct pixels are there.
> >> >
> >> > Perhaps a better name would be integration test. But the point is that
> >> > we can run these tests very, very quickly and (setup aside) without
> >> > outside influence, or without restarting the executable, etc.
> >>
> >> OK.
> >> BTW, as you said above, when we run such integration test for EFI
> >> which includes to run sysreset, before that sandbox will switch the
> >> sysreset driver for resetting EFI to avoid restarting it?
> >
> >Yes. The UEFI update seems quite monolithic from what I am hearing.
> >Could it be split into a few separate U-Boot commands, liike:
> >
> >- efi update prepare  (set up the update somewhere)
> >- efi update exec (do the update)
>
> Capsule updates are not done via the command line.
>
> Either you call the UpdateCapsule() service or you set a flag in the OsIndications variable and provide a file in a predefined path.

OK, but I think it would be helpful to provide this feature via the
cmdline too. That is how things normally work in U-Boot, right?

Regards,
Simon
diff mbox series

Patch

diff --git a/test/py/tests/test_efi_capsule/test_capsule_firmware.py b/test/py/tests/test_efi_capsule/test_capsule_firmware.py
index 6e803f699f..a539134ec2 100644
--- a/test/py/tests/test_efi_capsule/test_capsule_firmware.py
+++ b/test/py/tests/test_efi_capsule/test_capsule_firmware.py
@@ -143,13 +143,14 @@  class TestEfiCapsuleFirmwareFit(object):
                 'fatls host 0:1 %s' % CAPSULE_INSTALL_DIR])
             assert 'Test01' in ''.join(output)
 
-        # reboot
-        u_boot_console.restart_uboot()
-
         capsule_early = u_boot_config.buildconfig.get(
             'config_efi_capsule_on_disk_early')
         capsule_auth = u_boot_config.buildconfig.get(
             'config_efi_capsule_authenticate')
+
+        # reboot
+        u_boot_console.restart_uboot(expect_earlyreset = capsule_early)
+
         with u_boot_console.log.section('Test Case 2-b, after reboot'):
             if not capsule_early:
                 # make sure that dfu_alt_info exists even persistent variables
@@ -162,7 +163,7 @@  class TestEfiCapsuleFirmwareFit(object):
 
                 # need to run uefi command to initiate capsule handling
                 output = u_boot_console.run_command(
-                    'env print -e Capsule0000')
+                    'env print -e Capsule0000', wait_for_reboot = True)
 
             output = u_boot_console.run_command_list([
                 'host bind 0 %s' % disk_img,
@@ -218,13 +219,14 @@  class TestEfiCapsuleFirmwareFit(object):
                 'fatls host 0:1 %s' % CAPSULE_INSTALL_DIR])
             assert 'Test02' in ''.join(output)
 
-        # reboot
-        u_boot_console.restart_uboot()
-
         capsule_early = u_boot_config.buildconfig.get(
             'config_efi_capsule_on_disk_early')
         capsule_auth = u_boot_config.buildconfig.get(
             'config_efi_capsule_authenticate')
+
+        # reboot
+        u_boot_console.restart_uboot(expect_earlyreset = capsule_early)
+
         with u_boot_console.log.section('Test Case 3-b, after reboot'):
             if not capsule_early:
                 # make sure that dfu_alt_info exists even persistent variables
@@ -237,7 +239,13 @@  class TestEfiCapsuleFirmwareFit(object):
 
                 # need to run uefi command to initiate capsule handling
                 output = u_boot_console.run_command(
-                    'env print -e Capsule0000')
+                    'env print -e Capsule0000', wait_for_reboot = True)
+
+            output = u_boot_console.run_command_list([
+                'env set dfu_alt_info "sf 0:0=u-boot-bin raw 0x100000 0x50000;u-boot-env raw 0x150000 0x200000"',
+                'host bind 0 %s' % disk_img,
+                'fatls host 0:1 %s' % CAPSULE_INSTALL_DIR])
+            assert 'Test02' not in ''.join(output)
 
             output = u_boot_console.run_command_list(['efidebug capsule esrt'])
 
@@ -293,13 +301,14 @@  class TestEfiCapsuleFirmwareFit(object):
                 'fatls host 0:1 %s' % CAPSULE_INSTALL_DIR])
             assert 'Test03' in ''.join(output)
 
-        # reboot
-        u_boot_console.restart_uboot()
-
         capsule_early = u_boot_config.buildconfig.get(
             'config_efi_capsule_on_disk_early')
         capsule_auth = u_boot_config.buildconfig.get(
             'config_efi_capsule_authenticate')
+
+        # reboot
+        u_boot_console.restart_uboot(expect_earlyreset = capsule_early)
+
         with u_boot_console.log.section('Test Case 4-b, after reboot'):
             if not capsule_early:
                 # make sure that dfu_alt_info exists even persistent variables
@@ -312,7 +321,13 @@  class TestEfiCapsuleFirmwareFit(object):
 
                 # need to run uefi command to initiate capsule handling
                 output = u_boot_console.run_command(
-                    'env print -e Capsule0000')
+                    'env print -e Capsule0000', wait_for_reboot = True)
+
+            output = u_boot_console.run_command_list([
+                'env set dfu_alt_info "sf 0:0=u-boot-bin raw 0x100000 0x50000;u-boot-env raw 0x150000 0x200000"',
+                'host bind 0 %s' % disk_img,
+                'fatls host 0:1 %s' % CAPSULE_INSTALL_DIR])
+            assert 'Test03' not in ''.join(output)
 
             output = u_boot_console.run_command_list(['efidebug capsule esrt'])
 
diff --git a/test/py/u_boot_console_base.py b/test/py/u_boot_console_base.py
index 384fd53c65..e84f4d74ef 100644
--- a/test/py/u_boot_console_base.py
+++ b/test/py/u_boot_console_base.py
@@ -140,7 +140,7 @@  class ConsoleBase(object):
         self.logstream.close()
 
     def run_command(self, cmd, wait_for_echo=True, send_nl=True,
-            wait_for_prompt=True):
+            wait_for_prompt=True, wait_for_reboot=False):
         """Execute a command via the U-Boot console.
 
         The command is always sent to U-Boot.
@@ -168,6 +168,8 @@  class ConsoleBase(object):
             wait_for_prompt: Boolean indicating whether to wait for the
                 command prompt to be sent by U-Boot. This typically occurs
                 immediately after the command has been executed.
+            wait_for_reboot: Boolean indication whether to wait for the
+                reboot U-Boot. If this is True, wait_for_prompt is ignored.
 
         Returns:
             If wait_for_prompt == False:
@@ -202,11 +204,48 @@  class ConsoleBase(object):
                                     self.bad_pattern_ids[m - 1])
             if not wait_for_prompt:
                 return
-            m = self.p.expect([self.prompt_compiled] + self.bad_patterns)
-            if m != 0:
-                self.at_prompt = False
-                raise Exception('Bad pattern found on console: ' +
-                                self.bad_pattern_ids[m - 1])
+            if wait_for_reboot:
+                bcfg = self.config.buildconfig
+                config_spl = bcfg.get('config_spl', 'n') == 'y'
+                config_spl_serial = bcfg.get('config_spl_serial',
+                                                 'n') == 'y'
+                env_spl_skipped = self.config.env.get('env__spl_skipped',
+                                                      False)
+                env_spl2_skipped = self.config.env.get('env__spl2_skipped',
+                                                       True)
+                if config_spl and config_spl_serial and not env_spl_skipped:
+                    m = self.p.expect([pattern_u_boot_spl_signon] +
+                                      self.bad_patterns)
+                    if m != 0:
+                        raise Exception('Bad pattern found on SPL console: ' +
+                                        self.bad_pattern_ids[m - 1])
+                if not env_spl2_skipped:
+                    m = self.p.expect([pattern_u_boot_spl2_signon] +
+                                      self.bad_patterns)
+                    if m != 0:
+                        raise Exception('Bad pattern found on SPL2 console: ' +
+                                        self.bad_pattern_ids[m - 1])
+                m = self.p.expect([pattern_u_boot_main_signon] + self.bad_patterns)
+                if m != 0:
+                    raise Exception('Bad pattern found on console: ' +
+                                    self.bad_pattern_ids[m - 1])
+                self.u_boot_version_string = self.p.after
+                while True:
+                    m = self.p.expect([self.prompt_compiled,
+                        pattern_stop_autoboot_prompt] + self.bad_patterns)
+                    if m == 0:
+                        break
+                    if m == 1:
+                        self.p.send(' ')
+                        continue
+                    raise Exception('Bad pattern found on console: ' +
+                                    self.bad_pattern_ids[m - 2])
+            else:
+                m = self.p.expect([self.prompt_compiled] + self.bad_patterns)
+                if m != 0:
+                    self.at_prompt = False
+                    raise Exception('Bad pattern found on console: ' +
+                                    self.bad_pattern_ids[m - 1])
             self.at_prompt = True
             self.at_prompt_logevt = self.logstream.logfile.cur_evt
             # Only strip \r\n; space/TAB might be significant if testing
@@ -321,7 +360,7 @@  class ConsoleBase(object):
         finally:
             self.p.timeout = orig_timeout
 
-    def ensure_spawned(self):
+    def ensure_spawned(self, expect_earlyreset=False):
         """Ensure a connection to a correctly running U-Boot instance.
 
         This may require spawning a new Sandbox process or resetting target
@@ -330,7 +369,8 @@  class ConsoleBase(object):
         This is an internal function and should not be called directly.
 
         Args:
-            None.
+            expect_earlyreset: This boot is expected to be reset in early
+                stage (before prompt). False by default.
 
         Returns:
             Nothing.
@@ -357,22 +397,29 @@  class ConsoleBase(object):
                                                   False)
             env_spl2_skipped = self.config.env.get('env__spl2_skipped',
                                                   True)
-            if config_spl and config_spl_serial and not env_spl_skipped:
-                m = self.p.expect([pattern_u_boot_spl_signon] +
-                                  self.bad_patterns)
+            if expect_earlyreset:
+                loop_num = 2
+            else:
+                loop_num = 1
+
+            while loop_num > 0:
+                loop_num -= 1
+                if config_spl and config_spl_serial and not env_spl_skipped:
+                    m = self.p.expect([pattern_u_boot_spl_signon] +
+                                      self.bad_patterns)
+                    if m != 0:
+                        raise Exception('Bad pattern found on SPL console: ' +
+                                        self.bad_pattern_ids[m - 1])
+                if not env_spl2_skipped:
+                    m = self.p.expect([pattern_u_boot_spl2_signon] +
+                                      self.bad_patterns)
+                    if m != 0:
+                        raise Exception('Bad pattern found on SPL2 console: ' +
+                                        self.bad_pattern_ids[m - 1])
+                m = self.p.expect([pattern_u_boot_main_signon] + self.bad_patterns)
                 if m != 0:
-                    raise Exception('Bad pattern found on SPL console: ' +
-                                    self.bad_pattern_ids[m - 1])
-            if not env_spl2_skipped:
-                m = self.p.expect([pattern_u_boot_spl2_signon] +
-                                  self.bad_patterns)
-                if m != 0:
-                    raise Exception('Bad pattern found on SPL2 console: ' +
+                    raise Exception('Bad pattern found on console: ' +
                                     self.bad_pattern_ids[m - 1])
-            m = self.p.expect([pattern_u_boot_main_signon] + self.bad_patterns)
-            if m != 0:
-                raise Exception('Bad pattern found on console: ' +
-                                self.bad_pattern_ids[m - 1])
             self.u_boot_version_string = self.p.after
             while True:
                 m = self.p.expect([self.prompt_compiled,
@@ -416,10 +463,10 @@  class ConsoleBase(object):
             pass
         self.p = None
 
-    def restart_uboot(self):
+    def restart_uboot(self, expect_earlyreset=False):
         """Shut down and restart U-Boot."""
         self.cleanup_spawn()
-        self.ensure_spawned()
+        self.ensure_spawned(expect_earlyreset)
 
     def get_spawn_output(self):
         """Return the start-up output from U-Boot
diff --git a/test/py/u_boot_console_sandbox.py b/test/py/u_boot_console_sandbox.py
index 7e1eb0e0b4..9cd9ccad30 100644
--- a/test/py/u_boot_console_sandbox.py
+++ b/test/py/u_boot_console_sandbox.py
@@ -57,11 +57,13 @@  class ConsoleSandbox(ConsoleBase):
         cmd += self.sandbox_flags
         return Spawn(cmd, cwd=self.config.source_dir)
 
-    def restart_uboot_with_flags(self, flags):
+    def restart_uboot_with_flags(self, flags, expect_earlyreset=False):
         """Run U-Boot with the given command-line flags
 
         Args:
             flags: List of flags to pass, each a string
+            expect_earlyreset: This boot is expected to be reset in early
+                stage (before prompt). False by default.
 
         Returns:
             A u_boot_spawn.Spawn object that is attached to U-Boot.
@@ -69,7 +71,7 @@  class ConsoleSandbox(ConsoleBase):
 
         try:
             self.sandbox_flags = flags
-            return self.restart_uboot()
+            return self.restart_uboot(expect_earlyreset)
         finally:
             self.sandbox_flags = []