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