Re: [PATCH] test/py: efi_capsule: Handle expected reset after capsule on disk

[AKASHI Takahiro <takahiro.akashi@linaro.org>]

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
>