[PATCH v5 0/4] EFI: Reset system after capsule-on-disk

[PATCH v5 0/4] EFI: Reset system after capsule-on-disk

[Masami Hiramatsu]

Hi,

Here is the 5th version of the reset after capsule-on-disk. This version
includes the test updates for this new behavior. I finally split the test
update patches into 3 parts. ConsoleBase::run_command() update [2/4],
ConsoleBase::ensure_spawned() update [3/4] and the test code update.
The last part is merged into the reset after capsule-on-disk patch [4/4].

The reset after completing the capsule-on-disk is stated in the UEFI
specification 2.9, section 8.5.5 "Delivery of Capsules via file on Mass
Storage device" as below,

    In all cases that a capsule is identified for processing the system is
    restarted after capsule processing is completed.

Thank you,

---

Masami Hiramatsu (4):
      efi_loader: use efi_update_capsule_firmware() for capsule on disk
      test/py: Handle expected reset by command
      test/py: Handle expected reboot while booting sandbox
      efi_loader: test/py: Reset system after capsule update on disk


 lib/efi_loader/efi_capsule.c                       |   20 +++
 .../test_efi_capsule/test_capsule_firmware.py      |   37 ++++--
 test/py/u_boot_console_base.py                     |  115 ++++++++++++--------
 test/py/u_boot_console_sandbox.py                  |    7 +
 4 files changed, 115 insertions(+), 64 deletions(-)

--
Masami Hiramatsu <masami.hiramatsu@linaro.org>

[PATCH v5 1/4] efi_loader: use efi_update_capsule_firmware() for capsule on disk

[Masami Hiramatsu]

Since the efi_update_capsule() represents the UpdateCapsule() runtime
service, it has to handle the capsule flags and update ESRT. However
the capsule-on-disk doesn't need to care about such things.

Thus, the capsule-on-disk should use the efi_capsule_update_firmware()
directly instead of calling efi_update_capsule().

This means the roles of the efi_update_capsule() and capsule-on-disk
are different. We have to keep the efi_update_capsule() for providing
runtime service API at boot time.

Suggested-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
Signed-off-by: Masami Hiramatsu <masami.hiramatsu@linaro.org>
Reviewed-by: Heinrich Schuchardt <xypron.glpk@gmx.de>
---
 Changes in v4:
  - Update patch description.
---
 lib/efi_loader/efi_capsule.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/lib/efi_loader/efi_capsule.c b/lib/efi_loader/efi_capsule.c
index f4519c7317..d8141176df 100644
--- a/lib/efi_loader/efi_capsule.c
+++ b/lib/efi_loader/efi_capsule.c
@@ -1118,7 +1118,7 @@ efi_status_t efi_launch_capsules(void)
 			index = 0;
 		ret = efi_capsule_read_file(files[i], &capsule);
 		if (ret == EFI_SUCCESS) {
-			ret = EFI_CALL(efi_update_capsule(&capsule, 1, 0));
+			ret = efi_capsule_update_firmware(capsule);
 			if (ret != EFI_SUCCESS)
 				log_err("Applying capsule %ls failed\n",
 					files[i]);

[PATCH v5 2/4] test/py: Handle expected reset by command

[Masami Hiramatsu]

Add wait_for_reboot optional argument to ConsoleBase::run_command()
so that it can handle an expected reset by command execution.

This is useful if a command will reset the sandbox while testing
such commands, e.g. run_command("reset", wait_for_reboot = True)

Signed-off-by: Masami Hiramatsu <masami.hiramatsu@linaro.org>
---
 Changes in v5:
  - Split the wait-for-boot-prompt code as an internal function
    so that it is able to be shared with ensure_spawned().
  - Fix wait_for_reboot parameter description.
---
 test/py/u_boot_console_base.py |   99 +++++++++++++++++++++++-----------------
 1 file changed, 58 insertions(+), 41 deletions(-)

diff --git a/test/py/u_boot_console_base.py b/test/py/u_boot_console_base.py
index 384fd53c65..afae07d9cc 100644
--- a/test/py/u_boot_console_base.py
+++ b/test/py/u_boot_console_base.py
@@ -139,8 +139,53 @@ class ConsoleBase(object):
             self.p.close()
         self.logstream.close()
 
+    def wait_for_boot_prompt(self):
+        """Wait for the boot up until command prompt. This is for internal use only.
+        """
+        try:
+            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])
+
+        except Exception as ex:
+            self.log.error(str(ex))
+            self.cleanup_spawn()
+            raise
+        finally:
+            self.log.timestamp()
+
     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 +213,9 @@ 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 sets True, wait_for_prompt must also
+                be True.
 
         Returns:
             If wait_for_prompt == False:
@@ -202,11 +250,14 @@ 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:
+                self.wait_for_boot_prompt()
+            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
@@ -349,41 +400,7 @@ class ConsoleBase(object):
             if not self.config.gdbserver:
                 self.p.timeout = 30000
             self.p.logfile_read = self.logstream
-            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])
+            self.wait_for_boot_prompt()
             self.at_prompt = True
             self.at_prompt_logevt = self.logstream.logfile.cur_evt
         except Exception as ex:

[PATCH v5 3/4] test/py: Handle expected reboot while booting sandbox

[Masami Hiramatsu]

Add expected_reset optional argument to ConsoleBase::ensure_spawned(),
ConsoleBase::restart_uboot() and ConsoleSandbox::restart_uboot_with_flags()
so that it can handle a reset while the 1st boot process after main
boot logo before prompt correctly.

Signed-off-by: Masami Hiramatsu <masami.hiramatsu@linaro.org>
---
 Changes in v5:
  - Rename parameter to expect_reset and update the description to clarify
    the reset will happen between main boot and the command prompt.
---
 test/py/u_boot_console_base.py    |   48 ++++++++++++++++++++++---------------
 test/py/u_boot_console_sandbox.py |    7 ++++-
 2 files changed, 33 insertions(+), 22 deletions(-)

diff --git a/test/py/u_boot_console_base.py b/test/py/u_boot_console_base.py
index afae07d9cc..3938ec1302 100644
--- a/test/py/u_boot_console_base.py
+++ b/test/py/u_boot_console_base.py
@@ -139,7 +139,7 @@ class ConsoleBase(object):
             self.p.close()
         self.logstream.close()
 
-    def wait_for_boot_prompt(self):
+    def wait_for_boot_prompt(self, loop_num = 1):
         """Wait for the boot up until command prompt. This is for internal use only.
         """
         try:
@@ -149,22 +149,24 @@ class ConsoleBase(object):
             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)
+            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,
@@ -372,7 +374,7 @@ class ConsoleBase(object):
         finally:
             self.p.timeout = orig_timeout
 
-    def ensure_spawned(self):
+    def ensure_spawned(self, expect_reset=False):
         """Ensure a connection to a correctly running U-Boot instance.
 
         This may require spawning a new Sandbox process or resetting target
@@ -381,7 +383,9 @@ class ConsoleBase(object):
         This is an internal function and should not be called directly.
 
         Args:
-            None.
+            expect_reset: Boolean indication whether this boot is expected
+                to be reset while the 1st boot process after main boot before
+                prompt. False by default.
 
         Returns:
             Nothing.
@@ -400,7 +404,11 @@ class ConsoleBase(object):
             if not self.config.gdbserver:
                 self.p.timeout = 30000
             self.p.logfile_read = self.logstream
-            self.wait_for_boot_prompt()
+            if expect_reset:
+                loop_num = 2
+            else:
+                loop_num = 1
+            self.wait_for_boot_prompt(loop_num = loop_num)
             self.at_prompt = True
             self.at_prompt_logevt = self.logstream.logfile.cur_evt
         except Exception as ex:
@@ -433,10 +441,10 @@ class ConsoleBase(object):
             pass
         self.p = None
 
-    def restart_uboot(self):
+    def restart_uboot(self, expect_reset=False):
         """Shut down and restart U-Boot."""
         self.cleanup_spawn()
-        self.ensure_spawned()
+        self.ensure_spawned(expect_reset)
 
     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..ce4ca7e55e 100644
--- a/test/py/u_boot_console_sandbox.py
+++ b/test/py/u_boot_console_sandbox.py
@@ -57,11 +57,14 @@ 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_reset=False):
         """Run U-Boot with the given command-line flags
 
         Args:
             flags: List of flags to pass, each a string
+            expect_reset: Boolean indication whether this boot is expected
+                to be reset while the 1st boot process after main boot before
+                prompt. False by default.
 
         Returns:
             A u_boot_spawn.Spawn object that is attached to U-Boot.
@@ -69,7 +72,7 @@ class ConsoleSandbox(ConsoleBase):
 
         try:
             self.sandbox_flags = flags
-            return self.restart_uboot()
+            return self.restart_uboot(expect_reset)
         finally:
             self.sandbox_flags = []
 

[PATCH v5 4/4] efi_loader: test/py: Reset system after capsule update on disk

[Masami Hiramatsu]

Add a cold reset soon after processing capsule update on disk.
This is required in UEFI specification 2.9 Section 8.5.5
"Delivery of Capsules via file on Mass Storage device" as;

    In all cases that a capsule is identified for processing the system is
    restarted after capsule processing is completed.

This also reports the result of each capsule update so that the user can
notice that the capsule update has been succeeded or not from console log.

Signed-off-by: Masami Hiramatsu <masami.hiramatsu@linaro.org>
---
 Changes in v5:
  - Update testcase because this will change the expected behavior.
  - Merge dfu_alt_info setting with showing esrt command.
  - Remove redundant assertion from the test update.
 Changes in v4:
  - Do not use sysreset because that is a warm reset.
  - Fix patch description.
---
 lib/efi_loader/efi_capsule.c                       |   18 +++++++++-
 .../test_efi_capsule/test_capsule_firmware.py      |   37 ++++++++++++--------
 2 files changed, 39 insertions(+), 16 deletions(-)

diff --git a/lib/efi_loader/efi_capsule.c b/lib/efi_loader/efi_capsule.c
index d8141176df..613b531b82 100644
--- a/lib/efi_loader/efi_capsule.c
+++ b/lib/efi_loader/efi_capsule.c
@@ -14,6 +14,7 @@
 #include <env.h>
 #include <fdtdec.h>
 #include <fs.h>
+#include <hang.h>
 #include <malloc.h>
 #include <mapmem.h>
 #include <sort.h>
@@ -1120,8 +1121,11 @@ efi_status_t efi_launch_capsules(void)
 		if (ret == EFI_SUCCESS) {
 			ret = efi_capsule_update_firmware(capsule);
 			if (ret != EFI_SUCCESS)
-				log_err("Applying capsule %ls failed\n",
+				log_err("Applying capsule %ls failed.\n",
 					files[i]);
+			else
+				log_info("Applying capsule %ls succeeded.\n",
+					 files[i]);
 
 			/* create CapsuleXXXX */
 			set_capsule_result(index, capsule, ret);
@@ -1142,6 +1146,16 @@ efi_status_t efi_launch_capsules(void)
 		free(files[i]);
 	free(files);
 
-	return ret;
+	/*
+	 * UEFI spec requires to reset system after complete processing capsule
+	 * update on the storage.
+	 */
+	log_info("Reboot after firmware update");
+	/* Cold reset is required for loading the new firmware. */
+	do_reset(NULL, 0, 0, NULL);
+	hang();
+	/* not reach here */
+
+	return 0;
 }
 #endif /* CONFIG_EFI_CAPSULE_ON_DISK */
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..1dcf1c70f4 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_reset = 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_reset = 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,9 +239,12 @@ 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(['efidebug capsule esrt'])
+            # make sure the dfu_alt_info exists because it is required for making ESRT.
+            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"',
+                'efidebug capsule esrt'])
 
             # ensure that EFI_FIRMWARE_IMAGE_TYPE_UBOOT_FIT_GUID is in the ESRT.
             assert 'AE13FF2D-9AD4-4E25-9AC8-6D80B3B22147' in ''.join(output)
@@ -293,13 +298,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_reset = 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,9 +318,12 @@ 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(['efidebug capsule esrt'])
+            # make sure the dfu_alt_info exists because it is required for making ESRT.
+            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"',
+                'efidebug capsule esrt'])
 
             # ensure that  EFI_FIRMWARE_IMAGE_TYPE_UBOOT_RAW_GUID is in the ESRT.
             assert 'E2BB9C06-70E9-4B14-97A3-5A7913176E3F' in ''.join(output)

Re: [PATCH v5 3/4] test/py: Handle expected reboot while booting sandbox

[Simon Glass]

Hi Masami,

On Tue, 15 Feb 2022 at 23:16, Masami Hiramatsu
<masami.hiramatsu@linaro.org> wrote:
>
> Add expected_reset optional argument to ConsoleBase::ensure_spawned(),
> ConsoleBase::restart_uboot() and ConsoleSandbox::restart_uboot_with_flags()
> so that it can handle a reset while the 1st boot process after main
> boot logo before prompt correctly.
>
> Signed-off-by: Masami Hiramatsu <masami.hiramatsu@linaro.org>
> ---
>  Changes in v5:
>   - Rename parameter to expect_reset and update the description to clarify
>     the reset will happen between main boot and the command prompt.
> ---
>  test/py/u_boot_console_base.py    |   48 ++++++++++++++++++++++---------------
>  test/py/u_boot_console_sandbox.py |    7 ++++-
>  2 files changed, 33 insertions(+), 22 deletions(-)
>

Didn't I already comment on this patch? Why did it come back?

Regards,
Simon

Re: [PATCH v5 3/4] test/py: Handle expected reboot while booting sandbox

[Heinrich Schuchardt]

On 2/26/22 19:37, Simon Glass wrote:
> Hi Masami,
>
> On Tue, 15 Feb 2022 at 23:16, Masami Hiramatsu
> <masami.hiramatsu@linaro.org> wrote:
>>
>> Add expected_reset optional argument to ConsoleBase::ensure_spawned(),
>> ConsoleBase::restart_uboot() and ConsoleSandbox::restart_uboot_with_flags()
>> so that it can handle a reset while the 1st boot process after main
>> boot logo before prompt correctly.
>>
>> Signed-off-by: Masami Hiramatsu <masami.hiramatsu@linaro.org>
>> ---
>>   Changes in v5:
>>    - Rename parameter to expect_reset and update the description to clarify
>>      the reset will happen between main boot and the command prompt.
>> ---
>>   test/py/u_boot_console_base.py    |   48 ++++++++++++++++++++++---------------
>>   test/py/u_boot_console_sandbox.py |    7 ++++-
>>   2 files changed, 33 insertions(+), 22 deletions(-)
>>
>
> Didn't I already comment on this patch? Why did it come back?

Dear Simon,

The discussion is in
https://patchwork.ozlabs.org/project/uboot/patch/164491595065.536855.9457820061065514578.stgit@localhost/

You suggested: "We have a means to avoid actually doing the reset, see
the reset driver."

We need a real reset on the sandbox and no fake reset as already said in
the referenced thread.

Best regards

Heinrich

>
> Regards,
> Simon

Re: [PATCH v5 3/4] test/py: Handle expected reboot while booting sandbox

[Tom Rini]

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

On Sat, Feb 26, 2022 at 11:37:20AM -0700, Simon Glass wrote:

> Hi Masami,
> 
> On Tue, 15 Feb 2022 at 23:16, Masami Hiramatsu
> <masami.hiramatsu@linaro.org> wrote:
> >
> > Add expected_reset optional argument to ConsoleBase::ensure_spawned(),
> > ConsoleBase::restart_uboot() and ConsoleSandbox::restart_uboot_with_flags()
> > so that it can handle a reset while the 1st boot process after main
> > boot logo before prompt correctly.
> >
> > Signed-off-by: Masami Hiramatsu <masami.hiramatsu@linaro.org>
> > ---
> >  Changes in v5:
> >   - Rename parameter to expect_reset and update the description to clarify
> >     the reset will happen between main boot and the command prompt.
> > ---
> >  test/py/u_boot_console_base.py    |   48 ++++++++++++++++++++++---------------
> >  test/py/u_boot_console_sandbox.py |    7 ++++-
> >  2 files changed, 33 insertions(+), 22 deletions(-)
> 
> Didn't I already comment on this patch? Why did it come back?

Maybe I'm confused.  This is so that we can "cold" reset sandbox so it
behaves more consistently to other platforms, yes?

-- 
Tom

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

Re: [PATCH v5 3/4] test/py: Handle expected reboot while booting sandbox

[Simon Glass]

Hi Heinrich,

On Sun, 27 Feb 2022 at 01:22, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>
> On 2/26/22 19:37, Simon Glass wrote:
> > Hi Masami,
> >
> > On Tue, 15 Feb 2022 at 23:16, Masami Hiramatsu
> > <masami.hiramatsu@linaro.org> wrote:
> >>
> >> Add expected_reset optional argument to ConsoleBase::ensure_spawned(),
> >> ConsoleBase::restart_uboot() and ConsoleSandbox::restart_uboot_with_flags()
> >> so that it can handle a reset while the 1st boot process after main
> >> boot logo before prompt correctly.
> >>
> >> Signed-off-by: Masami Hiramatsu <masami.hiramatsu@linaro.org>
> >> ---
> >>   Changes in v5:
> >>    - Rename parameter to expect_reset and update the description to clarify
> >>      the reset will happen between main boot and the command prompt.
> >> ---
> >>   test/py/u_boot_console_base.py    |   48 ++++++++++++++++++++++---------------
> >>   test/py/u_boot_console_sandbox.py |    7 ++++-
> >>   2 files changed, 33 insertions(+), 22 deletions(-)
> >>
> >
> > Didn't I already comment on this patch? Why did it come back?
>
> Dear Simon,
>
> The discussion is in
> https://patchwork.ozlabs.org/project/uboot/patch/164491595065.536855.9457820061065514578.stgit@localhost/
>
> You suggested: "We have a means to avoid actually doing the reset, see
> the reset driver."
>
> We need a real reset on the sandbox and no fake reset as already said in
> the referenced thread.

Why?

The fake reset is there for use by tests. We don't need this load of
Python code at all for sandbox. We should worry about it later.

Regards,
Simon

Re: [PATCH v5 3/4] test/py: Handle expected reboot while booting sandbox

[Tom Rini]

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

On Sun, Feb 27, 2022 at 08:39:30AM -0700, Simon Glass wrote:
> Hi Heinrich,
> 
> On Sun, 27 Feb 2022 at 01:22, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> >
> > On 2/26/22 19:37, Simon Glass wrote:
> > > Hi Masami,
> > >
> > > On Tue, 15 Feb 2022 at 23:16, Masami Hiramatsu
> > > <masami.hiramatsu@linaro.org> wrote:
> > >>
> > >> Add expected_reset optional argument to ConsoleBase::ensure_spawned(),
> > >> ConsoleBase::restart_uboot() and ConsoleSandbox::restart_uboot_with_flags()
> > >> so that it can handle a reset while the 1st boot process after main
> > >> boot logo before prompt correctly.
> > >>
> > >> Signed-off-by: Masami Hiramatsu <masami.hiramatsu@linaro.org>
> > >> ---
> > >>   Changes in v5:
> > >>    - Rename parameter to expect_reset and update the description to clarify
> > >>      the reset will happen between main boot and the command prompt.
> > >> ---
> > >>   test/py/u_boot_console_base.py    |   48 ++++++++++++++++++++++---------------
> > >>   test/py/u_boot_console_sandbox.py |    7 ++++-
> > >>   2 files changed, 33 insertions(+), 22 deletions(-)
> > >>
> > >
> > > Didn't I already comment on this patch? Why did it come back?
> >
> > Dear Simon,
> >
> > The discussion is in
> > https://patchwork.ozlabs.org/project/uboot/patch/164491595065.536855.9457820061065514578.stgit@localhost/
> >
> > You suggested: "We have a means to avoid actually doing the reset, see
> > the reset driver."
> >
> > We need a real reset on the sandbox and no fake reset as already said in
> > the referenced thread.
> 
> Why?
> 
> The fake reset is there for use by tests. We don't need this load of
> Python code at all for sandbox. We should worry about it later.

Well, isn't this going to make the tests more sandbox-centric then, and
then need changes later to be able to test on real hardware?

-- 
Tom

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

Re: [PATCH v5 3/4] test/py: Handle expected reboot while booting sandbox

[Simon Glass]

Hi Tom,

On Sun, 27 Feb 2022 at 11:14, Tom Rini <trini@konsulko.com> wrote:
>
> On Sun, Feb 27, 2022 at 08:39:30AM -0700, Simon Glass wrote:
> > Hi Heinrich,
> >
> > On Sun, 27 Feb 2022 at 01:22, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> > >
> > > On 2/26/22 19:37, Simon Glass wrote:
> > > > Hi Masami,
> > > >
> > > > On Tue, 15 Feb 2022 at 23:16, Masami Hiramatsu
> > > > <masami.hiramatsu@linaro.org> wrote:
> > > >>
> > > >> Add expected_reset optional argument to ConsoleBase::ensure_spawned(),
> > > >> ConsoleBase::restart_uboot() and ConsoleSandbox::restart_uboot_with_flags()
> > > >> so that it can handle a reset while the 1st boot process after main
> > > >> boot logo before prompt correctly.
> > > >>
> > > >> Signed-off-by: Masami Hiramatsu <masami.hiramatsu@linaro.org>
> > > >> ---
> > > >>   Changes in v5:
> > > >>    - Rename parameter to expect_reset and update the description to clarify
> > > >>      the reset will happen between main boot and the command prompt.
> > > >> ---
> > > >>   test/py/u_boot_console_base.py    |   48 ++++++++++++++++++++++---------------
> > > >>   test/py/u_boot_console_sandbox.py |    7 ++++-
> > > >>   2 files changed, 33 insertions(+), 22 deletions(-)
> > > >>
> > > >
> > > > Didn't I already comment on this patch? Why did it come back?
> > >
> > > Dear Simon,
> > >
> > > The discussion is in
> > > https://patchwork.ozlabs.org/project/uboot/patch/164491595065.536855.9457820061065514578.stgit@localhost/
> > >
> > > You suggested: "We have a means to avoid actually doing the reset, see
> > > the reset driver."
> > >
> > > We need a real reset on the sandbox and no fake reset as already said in
> > > the referenced thread.
> >
> > Why?
> >
> > The fake reset is there for use by tests. We don't need this load of
> > Python code at all for sandbox. We should worry about it later.
>
> Well, isn't this going to make the tests more sandbox-centric then, and
> then need changes later to be able to test on real hardware?

Yes, but it keeps the sandbox case simple. At present the sandbox
tests can run within U-Boot (see the 'ut' command) and I very much
want to keep it that way. That is, after all, why I wrote the reset
driver.

While tests on real hardware have value, I hope that all logic bugs
are found on sandbox.

Regards,
Simon

Re: [PATCH v5 3/4] test/py: Handle expected reboot while booting sandbox

[Heinrich Schuchardt]

Am 27. Februar 2022 20:11:01 MEZ schrieb Simon Glass <sjg@chromium.org>:
>Hi Tom,
>
>On Sun, 27 Feb 2022 at 11:14, Tom Rini <trini@konsulko.com> wrote:
>>
>> On Sun, Feb 27, 2022 at 08:39:30AM -0700, Simon Glass wrote:
>> > Hi Heinrich,
>> >
>> > On Sun, 27 Feb 2022 at 01:22, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>> > >
>> > > On 2/26/22 19:37, Simon Glass wrote:
>> > > > Hi Masami,
>> > > >
>> > > > On Tue, 15 Feb 2022 at 23:16, Masami Hiramatsu
>> > > > <masami.hiramatsu@linaro.org> wrote:
>> > > >>
>> > > >> Add expected_reset optional argument to ConsoleBase::ensure_spawned(),
>> > > >> ConsoleBase::restart_uboot() and ConsoleSandbox::restart_uboot_with_flags()
>> > > >> so that it can handle a reset while the 1st boot process after main
>> > > >> boot logo before prompt correctly.
>> > > >>
>> > > >> Signed-off-by: Masami Hiramatsu <masami.hiramatsu@linaro.org>
>> > > >> ---
>> > > >>   Changes in v5:
>> > > >>    - Rename parameter to expect_reset and update the description to clarify
>> > > >>      the reset will happen between main boot and the command prompt.
>> > > >> ---
>> > > >>   test/py/u_boot_console_base.py    |   48 ++++++++++++++++++++++---------------
>> > > >>   test/py/u_boot_console_sandbox.py |    7 ++++-
>> > > >>   2 files changed, 33 insertions(+), 22 deletions(-)
>> > > >>
>> > > >
>> > > > Didn't I already comment on this patch? Why did it come back?
>> > >
>> > > Dear Simon,
>> > >
>> > > The discussion is in
>> > > https://patchwork.ozlabs.org/project/uboot/patch/164491595065.536855.9457820061065514578.stgit@localhost/
>> > >
>> > > You suggested: "We have a means to avoid actually doing the reset, see
>> > > the reset driver."
>> > >
>> > > We need a real reset on the sandbox and no fake reset as already said in
>> > > the referenced thread.
>> >
>> > Why?
>> >
>> > The fake reset is there for use by tests. We don't need this load of
>> > Python code at all for sandbox. We should worry about it later.
>>
>> Well, isn't this going to make the tests more sandbox-centric then, and
>> then need changes later to be able to test on real hardware?
>
>Yes, but it keeps the sandbox case simple. At present the sandbox
>tests can run within U-Boot (see the 'ut' command) and I very much
>want to keep it that way. That is, after all, why I wrote the reset
>driver.
>
>While tests on real hardware have value, I hope that all logic bugs
>are found on sandbox.

How does this relate to your thoughts about this patch?

The patch enables testing capsule updates including resets. This does not stop running the tests on the sandbox.

Why do you suggest sandbox specific quirks in capsule updates?

Best regards

Heinrich 


>
>Regards,
>Simon

Re: [PATCH v5 3/4] test/py: Handle expected reboot while booting sandbox

[Simon Glass]

Hi Heinrich,

On Sun, 27 Feb 2022 at 12:21, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
>
>
>
> Am 27. Februar 2022 20:11:01 MEZ schrieb Simon Glass <sjg@chromium.org>:
> >Hi Tom,
> >
> >On Sun, 27 Feb 2022 at 11:14, Tom Rini <trini@konsulko.com> wrote:
> >>
> >> On Sun, Feb 27, 2022 at 08:39:30AM -0700, Simon Glass wrote:
> >> > Hi Heinrich,
> >> >
> >> > On Sun, 27 Feb 2022 at 01:22, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> >> > >
> >> > > On 2/26/22 19:37, Simon Glass wrote:
> >> > > > Hi Masami,
> >> > > >
> >> > > > On Tue, 15 Feb 2022 at 23:16, Masami Hiramatsu
> >> > > > <masami.hiramatsu@linaro.org> wrote:
> >> > > >>
> >> > > >> Add expected_reset optional argument to ConsoleBase::ensure_spawned(),
> >> > > >> ConsoleBase::restart_uboot() and ConsoleSandbox::restart_uboot_with_flags()
> >> > > >> so that it can handle a reset while the 1st boot process after main
> >> > > >> boot logo before prompt correctly.
> >> > > >>
> >> > > >> Signed-off-by: Masami Hiramatsu <masami.hiramatsu@linaro.org>
> >> > > >> ---
> >> > > >>   Changes in v5:
> >> > > >>    - Rename parameter to expect_reset and update the description to clarify
> >> > > >>      the reset will happen between main boot and the command prompt.
> >> > > >> ---
> >> > > >>   test/py/u_boot_console_base.py    |   48 ++++++++++++++++++++++---------------
> >> > > >>   test/py/u_boot_console_sandbox.py |    7 ++++-
> >> > > >>   2 files changed, 33 insertions(+), 22 deletions(-)
> >> > > >>
> >> > > >
> >> > > > Didn't I already comment on this patch? Why did it come back?
> >> > >
> >> > > Dear Simon,
> >> > >
> >> > > The discussion is in
> >> > > https://patchwork.ozlabs.org/project/uboot/patch/164491595065.536855.9457820061065514578.stgit@localhost/
> >> > >
> >> > > You suggested: "We have a means to avoid actually doing the reset, see
> >> > > the reset driver."
> >> > >
> >> > > We need a real reset on the sandbox and no fake reset as already said in
> >> > > the referenced thread.
> >> >
> >> > Why?
> >> >
> >> > The fake reset is there for use by tests. We don't need this load of
> >> > Python code at all for sandbox. We should worry about it later.
> >>
> >> Well, isn't this going to make the tests more sandbox-centric then, and
> >> then need changes later to be able to test on real hardware?
> >
> >Yes, but it keeps the sandbox case simple. At present the sandbox
> >tests can run within U-Boot (see the 'ut' command) and I very much
> >want to keep it that way. That is, after all, why I wrote the reset
> >driver.
> >
> >While tests on real hardware have value, I hope that all logic bugs
> >are found on sandbox.
>
> How does this relate to your thoughts about this patch?
>
> The patch enables testing capsule updates including resets. This does not stop running the tests on the sandbox.
>
> Why do you suggest sandbox specific quirks in capsule updates?

sandbox-specific but in any case I find the tone of that statement
offensive and misleading. I have asked you before to avoid this sort
of thing on the mailing list. Sandbox is what we use for unit tests.

How is the test run at present on sandbox? My understanding is that
you want sandbox to rely on the pytest framework to work...do I have
that wrong?

Regards,
Simon

Re: [PATCH v5 3/4] test/py: Handle expected reboot while booting sandbox

[Simon Glass]

Hi Heinrich,

On Sun, 27 Feb 2022 at 12:51, Simon Glass <sjg@chromium.org> wrote:
>
> Hi Heinrich,
>
> On Sun, 27 Feb 2022 at 12:21, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> >
> >
> >
> > Am 27. Februar 2022 20:11:01 MEZ schrieb Simon Glass <sjg@chromium.org>:
> > >Hi Tom,
> > >
> > >On Sun, 27 Feb 2022 at 11:14, Tom Rini <trini@konsulko.com> wrote:
> > >>
> > >> On Sun, Feb 27, 2022 at 08:39:30AM -0700, Simon Glass wrote:
> > >> > Hi Heinrich,
> > >> >
> > >> > On Sun, 27 Feb 2022 at 01:22, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> > >> > >
> > >> > > On 2/26/22 19:37, Simon Glass wrote:
> > >> > > > Hi Masami,
> > >> > > >
> > >> > > > On Tue, 15 Feb 2022 at 23:16, Masami Hiramatsu
> > >> > > > <masami.hiramatsu@linaro.org> wrote:
> > >> > > >>
> > >> > > >> Add expected_reset optional argument to ConsoleBase::ensure_spawned(),
> > >> > > >> ConsoleBase::restart_uboot() and ConsoleSandbox::restart_uboot_with_flags()
> > >> > > >> so that it can handle a reset while the 1st boot process after main
> > >> > > >> boot logo before prompt correctly.
> > >> > > >>
> > >> > > >> Signed-off-by: Masami Hiramatsu <masami.hiramatsu@linaro.org>
> > >> > > >> ---
> > >> > > >>   Changes in v5:
> > >> > > >>    - Rename parameter to expect_reset and update the description to clarify
> > >> > > >>      the reset will happen between main boot and the command prompt.
> > >> > > >> ---
> > >> > > >>   test/py/u_boot_console_base.py    |   48 ++++++++++++++++++++++---------------
> > >> > > >>   test/py/u_boot_console_sandbox.py |    7 ++++-
> > >> > > >>   2 files changed, 33 insertions(+), 22 deletions(-)
> > >> > > >>
> > >> > > >
> > >> > > > Didn't I already comment on this patch? Why did it come back?
> > >> > >
> > >> > > Dear Simon,
> > >> > >
> > >> > > The discussion is in
> > >> > > https://patchwork.ozlabs.org/project/uboot/patch/164491595065.536855.9457820061065514578.stgit@localhost/
> > >> > >
> > >> > > You suggested: "We have a means to avoid actually doing the reset, see
> > >> > > the reset driver."
> > >> > >
> > >> > > We need a real reset on the sandbox and no fake reset as already said in
> > >> > > the referenced thread.
> > >> >
> > >> > Why?
> > >> >
> > >> > The fake reset is there for use by tests. We don't need this load of
> > >> > Python code at all for sandbox. We should worry about it later.
> > >>
> > >> Well, isn't this going to make the tests more sandbox-centric then, and
> > >> then need changes later to be able to test on real hardware?
> > >
> > >Yes, but it keeps the sandbox case simple. At present the sandbox
> > >tests can run within U-Boot (see the 'ut' command) and I very much
> > >want to keep it that way. That is, after all, why I wrote the reset
> > >driver.
> > >
> > >While tests on real hardware have value, I hope that all logic bugs
> > >are found on sandbox.
> >
> > How does this relate to your thoughts about this patch?
> >
> > The patch enables testing capsule updates including resets. This does not stop running the tests on the sandbox.
> >
> > Why do you suggest sandbox specific quirks in capsule updates?
>
> sandbox-specific but in any case I find the tone of that statement
> offensive and misleading. I have asked you before to avoid this sort
> of thing on the mailing list. Sandbox is what we use for unit tests.
>
> How is the test run at present on sandbox? My understanding is that
> you want sandbox to rely on the pytest framework to work...do I have
> that wrong?

Also I'd really appreciate it if you could review Takahiro's series
and help get it landed. It has been languishing for ages.

Regards,
Simon

Re: [PATCH v5 3/4] test/py: Handle expected reboot while booting sandbox

[Tom Rini]

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

On Sun, Feb 27, 2022 at 12:11:01PM -0700, Simon Glass wrote:
> Hi Tom,
> 
> On Sun, 27 Feb 2022 at 11:14, Tom Rini <trini@konsulko.com> wrote:
> >
> > On Sun, Feb 27, 2022 at 08:39:30AM -0700, Simon Glass wrote:
> > > Hi Heinrich,
> > >
> > > On Sun, 27 Feb 2022 at 01:22, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> > > >
> > > > On 2/26/22 19:37, Simon Glass wrote:
> > > > > Hi Masami,
> > > > >
> > > > > On Tue, 15 Feb 2022 at 23:16, Masami Hiramatsu
> > > > > <masami.hiramatsu@linaro.org> wrote:
> > > > >>
> > > > >> Add expected_reset optional argument to ConsoleBase::ensure_spawned(),
> > > > >> ConsoleBase::restart_uboot() and ConsoleSandbox::restart_uboot_with_flags()
> > > > >> so that it can handle a reset while the 1st boot process after main
> > > > >> boot logo before prompt correctly.
> > > > >>
> > > > >> Signed-off-by: Masami Hiramatsu <masami.hiramatsu@linaro.org>
> > > > >> ---
> > > > >>   Changes in v5:
> > > > >>    - Rename parameter to expect_reset and update the description to clarify
> > > > >>      the reset will happen between main boot and the command prompt.
> > > > >> ---
> > > > >>   test/py/u_boot_console_base.py    |   48 ++++++++++++++++++++++---------------
> > > > >>   test/py/u_boot_console_sandbox.py |    7 ++++-
> > > > >>   2 files changed, 33 insertions(+), 22 deletions(-)
> > > > >>
> > > > >
> > > > > Didn't I already comment on this patch? Why did it come back?
> > > >
> > > > Dear Simon,
> > > >
> > > > The discussion is in
> > > > https://patchwork.ozlabs.org/project/uboot/patch/164491595065.536855.9457820061065514578.stgit@localhost/
> > > >
> > > > You suggested: "We have a means to avoid actually doing the reset, see
> > > > the reset driver."
> > > >
> > > > We need a real reset on the sandbox and no fake reset as already said in
> > > > the referenced thread.
> > >
> > > Why?
> > >
> > > The fake reset is there for use by tests. We don't need this load of
> > > Python code at all for sandbox. We should worry about it later.
> >
> > Well, isn't this going to make the tests more sandbox-centric then, and
> > then need changes later to be able to test on real hardware?
> 
> Yes, but it keeps the sandbox case simple. At present the sandbox
> tests can run within U-Boot (see the 'ut' command) and I very much
> want to keep it that way. That is, after all, why I wrote the reset
> driver.
> 
> While tests on real hardware have value, I hope that all logic bugs
> are found on sandbox.

Yes, it's important to test the code in sandbox before testing it on
hardware, to avoid "obvious" oops-it-broke changes, it's still very
important to be able to easily run this on real hardware.  Ideally, I
hope to see updates to the pytest hook repository to flash the hardware
via capsule, as well as running a more formal pytest on hardware.  To
that end, is it not most important to make sandbox look like a real
hardware platform, rather than adapt the test to know about special
sandbox things?  Or am I missing something here and the test shouldn't
need changes / special handling to support both sandbox and real
hardware, with what you're suggesting?

-- 
Tom

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

Re: [PATCH v5 3/4] test/py: Handle expected reboot while booting sandbox

[Simon Glass]

Hi Tom,

On Sun, 27 Feb 2022 at 13:58, Tom Rini <trini@konsulko.com> wrote:
>
> On Sun, Feb 27, 2022 at 12:11:01PM -0700, Simon Glass wrote:
> > Hi Tom,
> >
> > On Sun, 27 Feb 2022 at 11:14, Tom Rini <trini@konsulko.com> wrote:
> > >
> > > On Sun, Feb 27, 2022 at 08:39:30AM -0700, Simon Glass wrote:
> > > > Hi Heinrich,
> > > >
> > > > On Sun, 27 Feb 2022 at 01:22, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> > > > >
> > > > > On 2/26/22 19:37, Simon Glass wrote:
> > > > > > Hi Masami,
> > > > > >
> > > > > > On Tue, 15 Feb 2022 at 23:16, Masami Hiramatsu
> > > > > > <masami.hiramatsu@linaro.org> wrote:
> > > > > >>
> > > > > >> Add expected_reset optional argument to ConsoleBase::ensure_spawned(),
> > > > > >> ConsoleBase::restart_uboot() and ConsoleSandbox::restart_uboot_with_flags()
> > > > > >> so that it can handle a reset while the 1st boot process after main
> > > > > >> boot logo before prompt correctly.
> > > > > >>
> > > > > >> Signed-off-by: Masami Hiramatsu <masami.hiramatsu@linaro.org>
> > > > > >> ---
> > > > > >>   Changes in v5:
> > > > > >>    - Rename parameter to expect_reset and update the description to clarify
> > > > > >>      the reset will happen between main boot and the command prompt.
> > > > > >> ---
> > > > > >>   test/py/u_boot_console_base.py    |   48 ++++++++++++++++++++++---------------
> > > > > >>   test/py/u_boot_console_sandbox.py |    7 ++++-
> > > > > >>   2 files changed, 33 insertions(+), 22 deletions(-)
> > > > > >>
> > > > > >
> > > > > > Didn't I already comment on this patch? Why did it come back?
> > > > >
> > > > > Dear Simon,
> > > > >
> > > > > The discussion is in
> > > > > https://patchwork.ozlabs.org/project/uboot/patch/164491595065.536855.9457820061065514578.stgit@localhost/
> > > > >
> > > > > You suggested: "We have a means to avoid actually doing the reset, see
> > > > > the reset driver."
> > > > >
> > > > > We need a real reset on the sandbox and no fake reset as already said in
> > > > > the referenced thread.
> > > >
> > > > Why?
> > > >
> > > > The fake reset is there for use by tests. We don't need this load of
> > > > Python code at all for sandbox. We should worry about it later.
> > >
> > > Well, isn't this going to make the tests more sandbox-centric then, and
> > > then need changes later to be able to test on real hardware?
> >
> > Yes, but it keeps the sandbox case simple. At present the sandbox
> > tests can run within U-Boot (see the 'ut' command) and I very much
> > want to keep it that way. That is, after all, why I wrote the reset
> > driver.
> >
> > While tests on real hardware have value, I hope that all logic bugs
> > are found on sandbox.
>
> Yes, it's important to test the code in sandbox before testing it on
> hardware, to avoid "obvious" oops-it-broke changes, it's still very
> important to be able to easily run this on real hardware.  Ideally, I
> hope to see updates to the pytest hook repository to flash the hardware
> via capsule, as well as running a more formal pytest on hardware.  To

Can you be specific about what bugs you are trying to catch in that
case? I am conscious of the nightmare that is Zephyr's thousands of
QEMU-based tests that take 20 minutes to run in parallel on a 64-core
machine, so I'd would like to make sure that real bugs are found in
unit tests where possible.

> that end, is it not most important to make sandbox look like a real
> hardware platform, rather than adapt the test to know about special
> sandbox things?  Or am I missing something here and the test shouldn't

The key thing is that sandbox runs essentially the same 'code under
test' as the real board and that we can quickly verified (using the
'ut xxx' command) that it works. In this case, we want to run the EFI
code under sandbox and make sure that it works.

> need changes / special handling to support both sandbox and real
> hardware, with what you're suggesting?

The title of this patch refers to specific hacks in pytest to handle
sandbox, doesn't it? So I think this is around the wrong way...that is
in fact my objection. It simply should not be done that way, with
special code in pytest, or whatever.

It should be possible to run 'ut xxx' and have the test run from start
to finish, without any outside influence. Sandbox has a sysreset
driver, just like any other board. We can make it do whatever we
want...see sandbox_sysreset_request().

We do use pytest to set things up beforehand, or to verify that things
worked after the run, but we should not need it to even just run a
unit test. In particular, it should not be necessary for sandbox to be
restarted by an outside influence.

Regards,
Simon

Re: [PATCH v5 3/4] test/py: Handle expected reboot while booting sandbox

[Tom Rini]

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

On Sun, Feb 27, 2022 at 02:45:36PM -0700, Simon Glass wrote:
> Hi Tom,
> 
> On Sun, 27 Feb 2022 at 13:58, Tom Rini <trini@konsulko.com> wrote:
> >
> > On Sun, Feb 27, 2022 at 12:11:01PM -0700, Simon Glass wrote:
> > > Hi Tom,
> > >
> > > On Sun, 27 Feb 2022 at 11:14, Tom Rini <trini@konsulko.com> wrote:
> > > >
> > > > On Sun, Feb 27, 2022 at 08:39:30AM -0700, Simon Glass wrote:
> > > > > Hi Heinrich,
> > > > >
> > > > > On Sun, 27 Feb 2022 at 01:22, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> > > > > >
> > > > > > On 2/26/22 19:37, Simon Glass wrote:
> > > > > > > Hi Masami,
> > > > > > >
> > > > > > > On Tue, 15 Feb 2022 at 23:16, Masami Hiramatsu
> > > > > > > <masami.hiramatsu@linaro.org> wrote:
> > > > > > >>
> > > > > > >> Add expected_reset optional argument to ConsoleBase::ensure_spawned(),
> > > > > > >> ConsoleBase::restart_uboot() and ConsoleSandbox::restart_uboot_with_flags()
> > > > > > >> so that it can handle a reset while the 1st boot process after main
> > > > > > >> boot logo before prompt correctly.
> > > > > > >>
> > > > > > >> Signed-off-by: Masami Hiramatsu <masami.hiramatsu@linaro.org>
> > > > > > >> ---
> > > > > > >>   Changes in v5:
> > > > > > >>    - Rename parameter to expect_reset and update the description to clarify
> > > > > > >>      the reset will happen between main boot and the command prompt.
> > > > > > >> ---
> > > > > > >>   test/py/u_boot_console_base.py    |   48 ++++++++++++++++++++++---------------
> > > > > > >>   test/py/u_boot_console_sandbox.py |    7 ++++-
> > > > > > >>   2 files changed, 33 insertions(+), 22 deletions(-)
> > > > > > >>
> > > > > > >
> > > > > > > Didn't I already comment on this patch? Why did it come back?
> > > > > >
> > > > > > Dear Simon,
> > > > > >
> > > > > > The discussion is in
> > > > > > https://patchwork.ozlabs.org/project/uboot/patch/164491595065.536855.9457820061065514578.stgit@localhost/
> > > > > >
> > > > > > You suggested: "We have a means to avoid actually doing the reset, see
> > > > > > the reset driver."
> > > > > >
> > > > > > We need a real reset on the sandbox and no fake reset as already said in
> > > > > > the referenced thread.
> > > > >
> > > > > Why?
> > > > >
> > > > > The fake reset is there for use by tests. We don't need this load of
> > > > > Python code at all for sandbox. We should worry about it later.
> > > >
> > > > Well, isn't this going to make the tests more sandbox-centric then, and
> > > > then need changes later to be able to test on real hardware?
> > >
> > > Yes, but it keeps the sandbox case simple. At present the sandbox
> > > tests can run within U-Boot (see the 'ut' command) and I very much
> > > want to keep it that way. That is, after all, why I wrote the reset
> > > driver.
> > >
> > > While tests on real hardware have value, I hope that all logic bugs
> > > are found on sandbox.
> >
> > Yes, it's important to test the code in sandbox before testing it on
> > hardware, to avoid "obvious" oops-it-broke changes, it's still very
> > important to be able to easily run this on real hardware.  Ideally, I
> > hope to see updates to the pytest hook repository to flash the hardware
> > via capsule, as well as running a more formal pytest on hardware.  To
> 
> Can you be specific about what bugs you are trying to catch in that
> case? I am conscious of the nightmare that is Zephyr's thousands of
> QEMU-based tests that take 20 minutes to run in parallel on a 64-core
> machine, so I'd would like to make sure that real bugs are found in
> unit tests where possible.
> 
> > that end, is it not most important to make sandbox look like a real
> > hardware platform, rather than adapt the test to know about special
> > sandbox things?  Or am I missing something here and the test shouldn't
> 
> The key thing is that sandbox runs essentially the same 'code under
> test' as the real board and that we can quickly verified (using the
> 'ut xxx' command) that it works. In this case, we want to run the EFI
> code under sandbox and make sure that it works.
> 
> > need changes / special handling to support both sandbox and real
> > hardware, with what you're suggesting?
> 
> The title of this patch refers to specific hacks in pytest to handle
> sandbox, doesn't it? So I think this is around the wrong way...that is
> in fact my objection. It simply should not be done that way, with
> special code in pytest, or whatever.
> 
> It should be possible to run 'ut xxx' and have the test run from start
> to finish, without any outside influence. Sandbox has a sysreset
> driver, just like any other board. We can make it do whatever we
> want...see sandbox_sysreset_request().
> 
> We do use pytest to set things up beforehand, or to verify that things
> worked after the run, but we should not need it to even just run a
> unit test. In particular, it should not be necessary for sandbox to be
> restarted by an outside influence.

Maybe we're talking at cross purposes here, or maybe I misread which
sets of tests this is ultimately for.  Functionality wise, I'm talking
about the capsule update functionality, and I want to ensure that we can
have something under test/py/tests/test_efi_capsule/ to update U-Boot
for a platform.  And that test should be abstracted such that it can run
on (a) sandbox (b) qemu as platformX (c) platformX in a HW lab.  If that
doesn't require changes under test/py/u_boot_console_sandbox.py then,
great!  I've just been confused.

-- 
Tom

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

Re: [PATCH v5 3/4] test/py: Handle expected reboot while booting sandbox

[Simon Glass]

Hi,

On Mon, 28 Feb 2022 at 06:48, Tom Rini <trini@konsulko.com> wrote:
>
> On Sun, Feb 27, 2022 at 02:45:36PM -0700, Simon Glass wrote:
> > Hi Tom,
> >
> > On Sun, 27 Feb 2022 at 13:58, Tom Rini <trini@konsulko.com> wrote:
> > >
> > > On Sun, Feb 27, 2022 at 12:11:01PM -0700, Simon Glass wrote:
> > > > Hi Tom,
> > > >
> > > > On Sun, 27 Feb 2022 at 11:14, Tom Rini <trini@konsulko.com> wrote:
> > > > >
> > > > > On Sun, Feb 27, 2022 at 08:39:30AM -0700, Simon Glass wrote:
> > > > > > Hi Heinrich,
> > > > > >
> > > > > > On Sun, 27 Feb 2022 at 01:22, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> > > > > > >
> > > > > > > On 2/26/22 19:37, Simon Glass wrote:
> > > > > > > > Hi Masami,
> > > > > > > >
> > > > > > > > On Tue, 15 Feb 2022 at 23:16, Masami Hiramatsu
> > > > > > > > <masami.hiramatsu@linaro.org> wrote:
> > > > > > > >>
> > > > > > > >> Add expected_reset optional argument to ConsoleBase::ensure_spawned(),
> > > > > > > >> ConsoleBase::restart_uboot() and ConsoleSandbox::restart_uboot_with_flags()
> > > > > > > >> so that it can handle a reset while the 1st boot process after main
> > > > > > > >> boot logo before prompt correctly.
> > > > > > > >>
> > > > > > > >> Signed-off-by: Masami Hiramatsu <masami.hiramatsu@linaro.org>
> > > > > > > >> ---
> > > > > > > >>   Changes in v5:
> > > > > > > >>    - Rename parameter to expect_reset and update the description to clarify
> > > > > > > >>      the reset will happen between main boot and the command prompt.
> > > > > > > >> ---
> > > > > > > >>   test/py/u_boot_console_base.py    |   48 ++++++++++++++++++++++---------------
> > > > > > > >>   test/py/u_boot_console_sandbox.py |    7 ++++-
> > > > > > > >>   2 files changed, 33 insertions(+), 22 deletions(-)
> > > > > > > >>
> > > > > > > >
> > > > > > > > Didn't I already comment on this patch? Why did it come back?
> > > > > > >
> > > > > > > Dear Simon,
> > > > > > >
> > > > > > > The discussion is in
> > > > > > > https://patchwork.ozlabs.org/project/uboot/patch/164491595065.536855.9457820061065514578.stgit@localhost/
> > > > > > >
> > > > > > > You suggested: "We have a means to avoid actually doing the reset, see
> > > > > > > the reset driver."
> > > > > > >
> > > > > > > We need a real reset on the sandbox and no fake reset as already said in
> > > > > > > the referenced thread.
> > > > > >
> > > > > > Why?
> > > > > >
> > > > > > The fake reset is there for use by tests. We don't need this load of
> > > > > > Python code at all for sandbox. We should worry about it later.
> > > > >
> > > > > Well, isn't this going to make the tests more sandbox-centric then, and
> > > > > then need changes later to be able to test on real hardware?
> > > >
> > > > Yes, but it keeps the sandbox case simple. At present the sandbox
> > > > tests can run within U-Boot (see the 'ut' command) and I very much
> > > > want to keep it that way. That is, after all, why I wrote the reset
> > > > driver.
> > > >
> > > > While tests on real hardware have value, I hope that all logic bugs
> > > > are found on sandbox.
> > >
> > > Yes, it's important to test the code in sandbox before testing it on
> > > hardware, to avoid "obvious" oops-it-broke changes, it's still very
> > > important to be able to easily run this on real hardware.  Ideally, I
> > > hope to see updates to the pytest hook repository to flash the hardware
> > > via capsule, as well as running a more formal pytest on hardware.  To
> >
> > Can you be specific about what bugs you are trying to catch in that
> > case? I am conscious of the nightmare that is Zephyr's thousands of
> > QEMU-based tests that take 20 minutes to run in parallel on a 64-core
> > machine, so I'd would like to make sure that real bugs are found in
> > unit tests where possible.
> >
> > > that end, is it not most important to make sandbox look like a real
> > > hardware platform, rather than adapt the test to know about special
> > > sandbox things?  Or am I missing something here and the test shouldn't
> >
> > The key thing is that sandbox runs essentially the same 'code under
> > test' as the real board and that we can quickly verified (using the
> > 'ut xxx' command) that it works. In this case, we want to run the EFI
> > code under sandbox and make sure that it works.
> >
> > > need changes / special handling to support both sandbox and real
> > > hardware, with what you're suggesting?
> >
> > The title of this patch refers to specific hacks in pytest to handle
> > sandbox, doesn't it? So I think this is around the wrong way...that is
> > in fact my objection. It simply should not be done that way, with
> > special code in pytest, or whatever.
> >
> > It should be possible to run 'ut xxx' and have the test run from start
> > to finish, without any outside influence. Sandbox has a sysreset
> > driver, just like any other board. We can make it do whatever we
> > want...see sandbox_sysreset_request().
> >
> > We do use pytest to set things up beforehand, or to verify that things
> > worked after the run, but we should not need it to even just run a
> > unit test. In particular, it should not be necessary for sandbox to be
> > restarted by an outside influence.
>
> Maybe we're talking at cross purposes here, or maybe I misread which
> sets of tests this is ultimately for.  Functionality wise, I'm talking
> about the capsule update functionality, and I want to ensure that we can
> have something under test/py/tests/test_efi_capsule/ to update U-Boot
> for a platform.  And that test should be abstracted such that it can run
> on (a) sandbox (b) qemu as platformX (c) platformX in a HW lab.  If that
> doesn't require changes under test/py/u_boot_console_sandbox.py then,
> great!  I've just been confused.

My point is that we should start with a test that runs on sandbox. It
should be a unit test, i.e. 'ut xxx' and not require changes in the
console handling and should use the sysreset driver to operate. This
should test *all* the code of the new feature, including failure
modes.

As to qemu and other platforms, they are really just going to be
happy-path tests. I suggest splitting the reset functionality out so
that it happens after the update is ready. Perhaps it is possible for
the test itself to be in control of the reset? We don't really need to
test that a board can reset, right? If we do, then we are going to
need some magic like this patch, but it is quite messy.

So please, unit test first.

Regards,
Simon

Re: [PATCH v5 3/4] test/py: Handle expected reboot while booting sandbox

[Masami Hiramatsu]

Hi Simon,

2022年2月28日(月) 6:45 Simon Glass <sjg@chromium.org>:

>
> Hi Tom,
>
> On Sun, 27 Feb 2022 at 13:58, Tom Rini <trini@konsulko.com> wrote:
> >
> > On Sun, Feb 27, 2022 at 12:11:01PM -0700, Simon Glass wrote:
> > > Hi Tom,
> > >
> > > On Sun, 27 Feb 2022 at 11:14, Tom Rini <trini@konsulko.com> wrote:
> > > >
> > > > On Sun, Feb 27, 2022 at 08:39:30AM -0700, Simon Glass wrote:
> > > > > Hi Heinrich,
> > > > >
> > > > > On Sun, 27 Feb 2022 at 01:22, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> > > > > >
> > > > > > On 2/26/22 19:37, Simon Glass wrote:
> > > > > > > Hi Masami,
> > > > > > >
> > > > > > > On Tue, 15 Feb 2022 at 23:16, Masami Hiramatsu
> > > > > > > <masami.hiramatsu@linaro.org> wrote:
> > > > > > >>
> > > > > > >> Add expected_reset optional argument to ConsoleBase::ensure_spawned(),
> > > > > > >> ConsoleBase::restart_uboot() and ConsoleSandbox::restart_uboot_with_flags()
> > > > > > >> so that it can handle a reset while the 1st boot process after main
> > > > > > >> boot logo before prompt correctly.
> > > > > > >>
> > > > > > >> Signed-off-by: Masami Hiramatsu <masami.hiramatsu@linaro.org>
> > > > > > >> ---
> > > > > > >>   Changes in v5:
> > > > > > >>    - Rename parameter to expect_reset and update the description to clarify
> > > > > > >>      the reset will happen between main boot and the command prompt.
> > > > > > >> ---
> > > > > > >>   test/py/u_boot_console_base.py    |   48 ++++++++++++++++++++++---------------
> > > > > > >>   test/py/u_boot_console_sandbox.py |    7 ++++-
> > > > > > >>   2 files changed, 33 insertions(+), 22 deletions(-)
> > > > > > >>
> > > > > > >
> > > > > > > Didn't I already comment on this patch? Why did it come back?
> > > > > >
> > > > > > Dear Simon,
> > > > > >
> > > > > > The discussion is in
> > > > > > https://patchwork.ozlabs.org/project/uboot/patch/164491595065.536855.9457820061065514578.stgit@localhost/
> > > > > >
> > > > > > You suggested: "We have a means to avoid actually doing the reset, see
> > > > > > the reset driver."
> > > > > >
> > > > > > We need a real reset on the sandbox and no fake reset as already said in
> > > > > > the referenced thread.
> > > > >
> > > > > Why?
> > > > >
> > > > > The fake reset is there for use by tests. We don't need this load of
> > > > > Python code at all for sandbox. We should worry about it later.
> > > >
> > > > Well, isn't this going to make the tests more sandbox-centric then, and
> > > > then need changes later to be able to test on real hardware?
> > >
> > > Yes, but it keeps the sandbox case simple. At present the sandbox
> > > tests can run within U-Boot (see the 'ut' command) and I very much
> > > want to keep it that way. That is, after all, why I wrote the reset
> > > driver.
> > >
> > > While tests on real hardware have value, I hope that all logic bugs
> > > are found on sandbox.
> >
> > Yes, it's important to test the code in sandbox before testing it on
> > hardware, to avoid "obvious" oops-it-broke changes, it's still very
> > important to be able to easily run this on real hardware.  Ideally, I
> > hope to see updates to the pytest hook repository to flash the hardware
> > via capsule, as well as running a more formal pytest on hardware.  To
>
> Can you be specific about what bugs you are trying to catch in that
> case? I am conscious of the nightmare that is Zephyr's thousands of
> QEMU-based tests that take 20 minutes to run in parallel on a 64-core
> machine, so I'd would like to make sure that real bugs are found in
> unit tests where possible.

I think this UEFI capsule update testcase is to check the capsule
file generated by the tool can be handled as we expected. E.g.
setting up the EFI variables and the capsule file is found in the
specific directory in the ESP etc.
(Note that this testcase doesn't check the capsule actually update the
firmware itself... which can not be done by sandbox because updated
firmware needs to be reloaded from the virtual storage.)

>
> > that end, is it not most important to make sandbox look like a real
> > hardware platform, rather than adapt the test to know about special
> > sandbox things?  Or am I missing something here and the test shouldn't
>
> The key thing is that sandbox runs essentially the same 'code under
> test' as the real board and that we can quickly verified (using the
> 'ut xxx' command) that it works. In this case, we want to run the EFI
> code under sandbox and make sure that it works.
>
> > need changes / special handling to support both sandbox and real
> > hardware, with what you're suggesting?
>
> The title of this patch refers to specific hacks in pytest to handle
> sandbox, doesn't it? So I think this is around the wrong way...that is
> in fact my objection. It simply should not be done that way, with
> special code in pytest, or whatever.

I agree with this point, yes, this changes the pytest ConsoleBase class
for sandbox test. If this cause a problem if this is used for normal platform,
it need to be changed.

> It should be possible to run 'ut xxx' and have the test run from start
> to finish, without any outside influence. Sandbox has a sysreset
> driver, just like any other board. We can make it do whatever we
> want...see sandbox_sysreset_request().

I'm not sure this part. What would you mean the 'outside influence'?

Thank you,

>
> We do use pytest to set things up beforehand, or to verify that things
> worked after the run, but we should not need it to even just run a
> unit test. In particular, it should not be necessary for sandbox to be
> restarted by an outside influence.
>
> Regards,
> Simon



--
Masami Hiramatsu

Re: [PATCH v5 3/4] test/py: Handle expected reboot while booting sandbox

[Masami Hiramatsu]

Hi Simon,

2022年2月28日(月) 22:56 Simon Glass <sjg@chromium.org>:
>
> Hi,
>
> On Mon, 28 Feb 2022 at 06:48, Tom Rini <trini@konsulko.com> wrote:
> >
> > On Sun, Feb 27, 2022 at 02:45:36PM -0700, Simon Glass wrote:
> > > Hi Tom,
> > >
> > > On Sun, 27 Feb 2022 at 13:58, Tom Rini <trini@konsulko.com> wrote:
> > > >
> > > > On Sun, Feb 27, 2022 at 12:11:01PM -0700, Simon Glass wrote:
> > > > > Hi Tom,
> > > > >
> > > > > On Sun, 27 Feb 2022 at 11:14, Tom Rini <trini@konsulko.com> wrote:
> > > > > >
> > > > > > On Sun, Feb 27, 2022 at 08:39:30AM -0700, Simon Glass wrote:
> > > > > > > Hi Heinrich,
> > > > > > >
> > > > > > > On Sun, 27 Feb 2022 at 01:22, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> > > > > > > >
> > > > > > > > On 2/26/22 19:37, Simon Glass wrote:
> > > > > > > > > Hi Masami,
> > > > > > > > >
> > > > > > > > > On Tue, 15 Feb 2022 at 23:16, Masami Hiramatsu
> > > > > > > > > <masami.hiramatsu@linaro.org> wrote:
> > > > > > > > >>
> > > > > > > > >> Add expected_reset optional argument to ConsoleBase::ensure_spawned(),
> > > > > > > > >> ConsoleBase::restart_uboot() and ConsoleSandbox::restart_uboot_with_flags()
> > > > > > > > >> so that it can handle a reset while the 1st boot process after main
> > > > > > > > >> boot logo before prompt correctly.
> > > > > > > > >>
> > > > > > > > >> Signed-off-by: Masami Hiramatsu <masami.hiramatsu@linaro.org>
> > > > > > > > >> ---
> > > > > > > > >>   Changes in v5:
> > > > > > > > >>    - Rename parameter to expect_reset and update the description to clarify
> > > > > > > > >>      the reset will happen between main boot and the command prompt.
> > > > > > > > >> ---
> > > > > > > > >>   test/py/u_boot_console_base.py    |   48 ++++++++++++++++++++++---------------
> > > > > > > > >>   test/py/u_boot_console_sandbox.py |    7 ++++-
> > > > > > > > >>   2 files changed, 33 insertions(+), 22 deletions(-)
> > > > > > > > >>
> > > > > > > > >
> > > > > > > > > Didn't I already comment on this patch? Why did it come back?
> > > > > > > >
> > > > > > > > Dear Simon,
> > > > > > > >
> > > > > > > > The discussion is in
> > > > > > > > https://patchwork.ozlabs.org/project/uboot/patch/164491595065.536855.9457820061065514578.stgit@localhost/
> > > > > > > >
> > > > > > > > You suggested: "We have a means to avoid actually doing the reset, see
> > > > > > > > the reset driver."
> > > > > > > >
> > > > > > > > We need a real reset on the sandbox and no fake reset as already said in
> > > > > > > > the referenced thread.
> > > > > > >
> > > > > > > Why?
> > > > > > >
> > > > > > > The fake reset is there for use by tests. We don't need this load of
> > > > > > > Python code at all for sandbox. We should worry about it later.
> > > > > >
> > > > > > Well, isn't this going to make the tests more sandbox-centric then, and
> > > > > > then need changes later to be able to test on real hardware?
> > > > >
> > > > > Yes, but it keeps the sandbox case simple. At present the sandbox
> > > > > tests can run within U-Boot (see the 'ut' command) and I very much
> > > > > want to keep it that way. That is, after all, why I wrote the reset
> > > > > driver.
> > > > >
> > > > > While tests on real hardware have value, I hope that all logic bugs
> > > > > are found on sandbox.
> > > >
> > > > Yes, it's important to test the code in sandbox before testing it on
> > > > hardware, to avoid "obvious" oops-it-broke changes, it's still very
> > > > important to be able to easily run this on real hardware.  Ideally, I
> > > > hope to see updates to the pytest hook repository to flash the hardware
> > > > via capsule, as well as running a more formal pytest on hardware.  To
> > >
> > > Can you be specific about what bugs you are trying to catch in that
> > > case? I am conscious of the nightmare that is Zephyr's thousands of
> > > QEMU-based tests that take 20 minutes to run in parallel on a 64-core
> > > machine, so I'd would like to make sure that real bugs are found in
> > > unit tests where possible.
> > >
> > > > that end, is it not most important to make sandbox look like a real
> > > > hardware platform, rather than adapt the test to know about special
> > > > sandbox things?  Or am I missing something here and the test shouldn't
> > >
> > > The key thing is that sandbox runs essentially the same 'code under
> > > test' as the real board and that we can quickly verified (using the
> > > 'ut xxx' command) that it works. In this case, we want to run the EFI
> > > code under sandbox and make sure that it works.
> > >
> > > > need changes / special handling to support both sandbox and real
> > > > hardware, with what you're suggesting?
> > >
> > > The title of this patch refers to specific hacks in pytest to handle
> > > sandbox, doesn't it? So I think this is around the wrong way...that is
> > > in fact my objection. It simply should not be done that way, with
> > > special code in pytest, or whatever.
> > >
> > > It should be possible to run 'ut xxx' and have the test run from start
> > > to finish, without any outside influence. Sandbox has a sysreset
> > > driver, just like any other board. We can make it do whatever we
> > > want...see sandbox_sysreset_request().
> > >
> > > We do use pytest to set things up beforehand, or to verify that things
> > > worked after the run, but we should not need it to even just run a
> > > unit test. In particular, it should not be necessary for sandbox to be
> > > restarted by an outside influence.
> >
> > Maybe we're talking at cross purposes here, or maybe I misread which
> > sets of tests this is ultimately for.  Functionality wise, I'm talking
> > about the capsule update functionality, and I want to ensure that we can
> > have something under test/py/tests/test_efi_capsule/ to update U-Boot
> > for a platform.  And that test should be abstracted such that it can run
> > on (a) sandbox (b) qemu as platformX (c) platformX in a HW lab.  If that
> > doesn't require changes under test/py/u_boot_console_sandbox.py then,
> > great!  I've just been confused.
>
> My point is that we should start with a test that runs on sandbox. It
> should be a unit test, i.e. 'ut xxx' and not require changes in the
> console handling and should use the sysreset driver to operate. This
> should test *all* the code of the new feature, including failure
> modes.

Hmm, but the capsule update on disk requires to prepare
- the capsule file image which is properly composed
- the disk which has an EFI system partition
- the EFI variables required for the capsule update on disk
I guess this is the main reason why the test is written by the script...

Thank you,

>
> As to qemu and other platforms, they are really just going to be
> happy-path tests. I suggest splitting the reset functionality out so
> that it happens after the update is ready. Perhaps it is possible for
> the test itself to be in control of the reset? We don't really need to
> test that a board can reset, right? If we do, then we are going to
> need some magic like this patch, but it is quite messy.
>
> So please, unit test first.
>
> Regards,
> Simon



-- 
Masami Hiramatsu

Re: [PATCH v5 3/4] test/py: Handle expected reboot while booting sandbox

[Simon Glass]

Hi Masami,

On Mon, 28 Feb 2022 at 07:40, Masami Hiramatsu
<masami.hiramatsu@linaro.org> wrote:
>
> Hi Simon,
>
> 2022年2月28日(月) 6:45 Simon Glass <sjg@chromium.org>:
>
> >
> > Hi Tom,
> >
> > On Sun, 27 Feb 2022 at 13:58, Tom Rini <trini@konsulko.com> wrote:
> > >
> > > On Sun, Feb 27, 2022 at 12:11:01PM -0700, Simon Glass wrote:
> > > > Hi Tom,
> > > >
> > > > On Sun, 27 Feb 2022 at 11:14, Tom Rini <trini@konsulko.com> wrote:
> > > > >
> > > > > On Sun, Feb 27, 2022 at 08:39:30AM -0700, Simon Glass wrote:
> > > > > > Hi Heinrich,
> > > > > >
> > > > > > On Sun, 27 Feb 2022 at 01:22, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> > > > > > >
> > > > > > > On 2/26/22 19:37, Simon Glass wrote:
> > > > > > > > Hi Masami,
> > > > > > > >
> > > > > > > > On Tue, 15 Feb 2022 at 23:16, Masami Hiramatsu
> > > > > > > > <masami.hiramatsu@linaro.org> wrote:
> > > > > > > >>
> > > > > > > >> Add expected_reset optional argument to ConsoleBase::ensure_spawned(),
> > > > > > > >> ConsoleBase::restart_uboot() and ConsoleSandbox::restart_uboot_with_flags()
> > > > > > > >> so that it can handle a reset while the 1st boot process after main
> > > > > > > >> boot logo before prompt correctly.
> > > > > > > >>
> > > > > > > >> Signed-off-by: Masami Hiramatsu <masami.hiramatsu@linaro.org>
> > > > > > > >> ---
> > > > > > > >>   Changes in v5:
> > > > > > > >>    - Rename parameter to expect_reset and update the description to clarify
> > > > > > > >>      the reset will happen between main boot and the command prompt.
> > > > > > > >> ---
> > > > > > > >>   test/py/u_boot_console_base.py    |   48 ++++++++++++++++++++++---------------
> > > > > > > >>   test/py/u_boot_console_sandbox.py |    7 ++++-
> > > > > > > >>   2 files changed, 33 insertions(+), 22 deletions(-)
> > > > > > > >>
> > > > > > > >
> > > > > > > > Didn't I already comment on this patch? Why did it come back?
> > > > > > >
> > > > > > > Dear Simon,
> > > > > > >
> > > > > > > The discussion is in
> > > > > > > https://patchwork.ozlabs.org/project/uboot/patch/164491595065.536855.9457820061065514578.stgit@localhost/
> > > > > > >
> > > > > > > You suggested: "We have a means to avoid actually doing the reset, see
> > > > > > > the reset driver."
> > > > > > >
> > > > > > > We need a real reset on the sandbox and no fake reset as already said in
> > > > > > > the referenced thread.
> > > > > >
> > > > > > Why?
> > > > > >
> > > > > > The fake reset is there for use by tests. We don't need this load of
> > > > > > Python code at all for sandbox. We should worry about it later.
> > > > >
> > > > > Well, isn't this going to make the tests more sandbox-centric then, and
> > > > > then need changes later to be able to test on real hardware?
> > > >
> > > > Yes, but it keeps the sandbox case simple. At present the sandbox
> > > > tests can run within U-Boot (see the 'ut' command) and I very much
> > > > want to keep it that way. That is, after all, why I wrote the reset
> > > > driver.
> > > >
> > > > While tests on real hardware have value, I hope that all logic bugs
> > > > are found on sandbox.
> > >
> > > Yes, it's important to test the code in sandbox before testing it on
> > > hardware, to avoid "obvious" oops-it-broke changes, it's still very
> > > important to be able to easily run this on real hardware.  Ideally, I
> > > hope to see updates to the pytest hook repository to flash the hardware
> > > via capsule, as well as running a more formal pytest on hardware.  To
> >
> > Can you be specific about what bugs you are trying to catch in that
> > case? I am conscious of the nightmare that is Zephyr's thousands of
> > QEMU-based tests that take 20 minutes to run in parallel on a 64-core
> > machine, so I'd would like to make sure that real bugs are found in
> > unit tests where possible.
>
> I think this UEFI capsule update testcase is to check the capsule
> file generated by the tool can be handled as we expected. E.g.
> setting up the EFI variables and the capsule file is found in the
> specific directory in the ESP etc.

OK

> (Note that this testcase doesn't check the capsule actually update the
> firmware itself... which can not be done by sandbox because updated
> firmware needs to be reloaded from the virtual storage.)

That can be done, if needed. See os_jump_to_file(). The Chromium OS
test does this.

>
> >
> > > that end, is it not most important to make sandbox look like a real
> > > hardware platform, rather than adapt the test to know about special
> > > sandbox things?  Or am I missing something here and the test shouldn't
> >
> > The key thing is that sandbox runs essentially the same 'code under
> > test' as the real board and that we can quickly verified (using the
> > 'ut xxx' command) that it works. In this case, we want to run the EFI
> > code under sandbox and make sure that it works.
> >
> > > need changes / special handling to support both sandbox and real
> > > hardware, with what you're suggesting?
> >
> > The title of this patch refers to specific hacks in pytest to handle
> > sandbox, doesn't it? So I think this is around the wrong way...that is
> > in fact my objection. It simply should not be done that way, with
> > special code in pytest, or whatever.
>
> I agree with this point, yes, this changes the pytest ConsoleBase class
> for sandbox test. If this cause a problem if this is used for normal platform,
> it need to be changed.

I don't think you need this patch for sandbox, which is my point. I
would rather simply not have it.

>
> > It should be possible to run 'ut xxx' and have the test run from start
> > to finish, without any outside influence. Sandbox has a sysreset
> > driver, just like any other board. We can make it do whatever we
> > want...see sandbox_sysreset_request().
>
> I'm not sure this part. What would you mean the 'outside influence'?

If you see this section it tells you how to run sandbox tests
directly, without pytest:

https://u-boot.readthedocs.io/en/latest/develop/tests_sandbox.html#running-sandbox-tests-directly

It should be possible to run your tests this way on sandbox, without
needing the pytest code. Of course there are exceptions, but most
tests work this way and it seems to me that this one can also.

'Outside influence' means Python code getting involved.
>
> Thank you,
>
> >
> > We do use pytest to set things up beforehand, or to verify that things
> > worked after the run, but we should not need it to even just run a
> > unit test. In particular, it should not be necessary for sandbox to be
> > restarted by an outside influence.

Regards,
Simon

Re: [PATCH v5 3/4] test/py: Handle expected reboot while booting sandbox

[Simon Glass]

Hi Masami,

On Mon, 28 Feb 2022 at 07:47, Masami Hiramatsu
<masami.hiramatsu@linaro.org> wrote:
>
> Hi Simon,
>
> 2022年2月28日(月) 22:56 Simon Glass <sjg@chromium.org>:
> >
> > Hi,
> >
> > On Mon, 28 Feb 2022 at 06:48, Tom Rini <trini@konsulko.com> wrote:
> > >
> > > On Sun, Feb 27, 2022 at 02:45:36PM -0700, Simon Glass wrote:
> > > > Hi Tom,
> > > >
> > > > On Sun, 27 Feb 2022 at 13:58, Tom Rini <trini@konsulko.com> wrote:
> > > > >
> > > > > On Sun, Feb 27, 2022 at 12:11:01PM -0700, Simon Glass wrote:
> > > > > > Hi Tom,
> > > > > >
> > > > > > On Sun, 27 Feb 2022 at 11:14, Tom Rini <trini@konsulko.com> wrote:
> > > > > > >
> > > > > > > On Sun, Feb 27, 2022 at 08:39:30AM -0700, Simon Glass wrote:
> > > > > > > > Hi Heinrich,
> > > > > > > >
> > > > > > > > On Sun, 27 Feb 2022 at 01:22, Heinrich Schuchardt <xypron.glpk@gmx.de> wrote:
> > > > > > > > >
> > > > > > > > > On 2/26/22 19:37, Simon Glass wrote:
> > > > > > > > > > Hi Masami,
> > > > > > > > > >
> > > > > > > > > > On Tue, 15 Feb 2022 at 23:16, Masami Hiramatsu
> > > > > > > > > > <masami.hiramatsu@linaro.org> wrote:
> > > > > > > > > >>
> > > > > > > > > >> Add expected_reset optional argument to ConsoleBase::ensure_spawned(),
> > > > > > > > > >> ConsoleBase::restart_uboot() and ConsoleSandbox::restart_uboot_with_flags()
> > > > > > > > > >> so that it can handle a reset while the 1st boot process after main
> > > > > > > > > >> boot logo before prompt correctly.
> > > > > > > > > >>
> > > > > > > > > >> Signed-off-by: Masami Hiramatsu <masami.hiramatsu@linaro.org>
> > > > > > > > > >> ---
> > > > > > > > > >>   Changes in v5:
> > > > > > > > > >>    - Rename parameter to expect_reset and update the description to clarify
> > > > > > > > > >>      the reset will happen between main boot and the command prompt.
> > > > > > > > > >> ---
> > > > > > > > > >>   test/py/u_boot_console_base.py    |   48 ++++++++++++++++++++++---------------
> > > > > > > > > >>   test/py/u_boot_console_sandbox.py |    7 ++++-
> > > > > > > > > >>   2 files changed, 33 insertions(+), 22 deletions(-)
> > > > > > > > > >>
> > > > > > > > > >
> > > > > > > > > > Didn't I already comment on this patch? Why did it come back?
> > > > > > > > >
> > > > > > > > > Dear Simon,
> > > > > > > > >
> > > > > > > > > The discussion is in
> > > > > > > > > https://patchwork.ozlabs.org/project/uboot/patch/164491595065.536855.9457820061065514578.stgit@localhost/
> > > > > > > > >
> > > > > > > > > You suggested: "We have a means to avoid actually doing the reset, see
> > > > > > > > > the reset driver."
> > > > > > > > >
> > > > > > > > > We need a real reset on the sandbox and no fake reset as already said in
> > > > > > > > > the referenced thread.
> > > > > > > >
> > > > > > > > Why?
> > > > > > > >
> > > > > > > > The fake reset is there for use by tests. We don't need this load of
> > > > > > > > Python code at all for sandbox. We should worry about it later.
> > > > > > >
> > > > > > > Well, isn't this going to make the tests more sandbox-centric then, and
> > > > > > > then need changes later to be able to test on real hardware?
> > > > > >
> > > > > > Yes, but it keeps the sandbox case simple. At present the sandbox
> > > > > > tests can run within U-Boot (see the 'ut' command) and I very much
> > > > > > want to keep it that way. That is, after all, why I wrote the reset
> > > > > > driver.
> > > > > >
> > > > > > While tests on real hardware have value, I hope that all logic bugs
> > > > > > are found on sandbox.
> > > > >
> > > > > Yes, it's important to test the code in sandbox before testing it on
> > > > > hardware, to avoid "obvious" oops-it-broke changes, it's still very
> > > > > important to be able to easily run this on real hardware.  Ideally, I
> > > > > hope to see updates to the pytest hook repository to flash the hardware
> > > > > via capsule, as well as running a more formal pytest on hardware.  To
> > > >
> > > > Can you be specific about what bugs you are trying to catch in that
> > > > case? I am conscious of the nightmare that is Zephyr's thousands of
> > > > QEMU-based tests that take 20 minutes to run in parallel on a 64-core
> > > > machine, so I'd would like to make sure that real bugs are found in
> > > > unit tests where possible.
> > > >
> > > > > that end, is it not most important to make sandbox look like a real
> > > > > hardware platform, rather than adapt the test to know about special
> > > > > sandbox things?  Or am I missing something here and the test shouldn't
> > > >
> > > > The key thing is that sandbox runs essentially the same 'code under
> > > > test' as the real board and that we can quickly verified (using the
> > > > 'ut xxx' command) that it works. In this case, we want to run the EFI
> > > > code under sandbox and make sure that it works.
> > > >
> > > > > need changes / special handling to support both sandbox and real
> > > > > hardware, with what you're suggesting?
> > > >
> > > > The title of this patch refers to specific hacks in pytest to handle
> > > > sandbox, doesn't it? So I think this is around the wrong way...that is
> > > > in fact my objection. It simply should not be done that way, with
> > > > special code in pytest, or whatever.
> > > >
> > > > It should be possible to run 'ut xxx' and have the test run from start
> > > > to finish, without any outside influence. Sandbox has a sysreset
> > > > driver, just like any other board. We can make it do whatever we
> > > > want...see sandbox_sysreset_request().
> > > >
> > > > We do use pytest to set things up beforehand, or to verify that things
> > > > worked after the run, but we should not need it to even just run a
> > > > unit test. In particular, it should not be necessary for sandbox to be
> > > > restarted by an outside influence.
> > >
> > > Maybe we're talking at cross purposes here, or maybe I misread which
> > > sets of tests this is ultimately for.  Functionality wise, I'm talking
> > > about the capsule update functionality, and I want to ensure that we can
> > > have something under test/py/tests/test_efi_capsule/ to update U-Boot
> > > for a platform.  And that test should be abstracted such that it can run
> > > on (a) sandbox (b) qemu as platformX (c) platformX in a HW lab.  If that
> > > doesn't require changes under test/py/u_boot_console_sandbox.py then,
> > > great!  I've just been confused.
> >
> > My point is that we should start with a test that runs on sandbox. It
> > should be a unit test, i.e. 'ut xxx' and not require changes in the
> > console handling and should use the sysreset driver to operate. This
> > should test *all* the code of the new feature, including failure
> > modes.
>
> Hmm, but the capsule update on disk requires to prepare
> - the capsule file image which is properly composed
> - the disk which has an EFI system partition
> - the EFI variables required for the capsule update on disk
> I guess this is the main reason why the test is written by the script...

Yes and that is fine, we do that sort of thing in many places. But
once the pre-conditions are ready, you should be able to run the unit
test from start to finish.

If you have specific problems, please let me know and I can help.

>
> Thank you,
>
> >
> > As to qemu and other platforms, they are really just going to be
> > happy-path tests. I suggest splitting the reset functionality out so
> > that it happens after the update is ready. Perhaps it is possible for
> > the test itself to be in control of the reset? We don't really need to
> > test that a board can reset, right? If we do, then we are going to
> > need some magic like this patch, but it is quite messy.
> >
> > So please, unit test first.

Regards,
SImon