| Message ID | 164491595065.536855.9457820061065514578.stgit@localhost |
|---|---|
| State | Superseded |
| Delegated to: | Heinrich Schuchardt |
| Headers | show |
| Series | test/py: efi_capsule: Handle expected reset after capsule on disk | expand |
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 = [] > > >
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
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 >
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 = [] > >
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 = [] > > > > >
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 = [] > >
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 = [] > > > >
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
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
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.
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
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
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).
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
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
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
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
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
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
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".
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.
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 >
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 >
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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,
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
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
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 --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 = []
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(-)