From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: X-Spam-Checker-Version: SpamAssassin 3.4.0 (2014-02-07) on aws-us-west-2-korg-lkml-1.web.codeaurora.org Received: from phobos.denx.de (phobos.denx.de [85.214.62.61]) (using TLSv1.2 with cipher ECDHE-RSA-AES128-GCM-SHA256 (128/128 bits)) (No client certificate requested) by smtp.lore.kernel.org (Postfix) with ESMTPS id 39CDDC433EF for ; Mon, 14 Mar 2022 18:25:57 +0000 (UTC) Received: from h2850616.stratoserver.net (localhost [IPv6:::1]) by phobos.denx.de (Postfix) with ESMTP id BB7E983C8E; Mon, 14 Mar 2022 19:25:51 +0100 (CET) Authentication-Results: phobos.denx.de; dmarc=pass (p=none dis=none) header.from=chromium.org Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=u-boot-bounces@lists.denx.de Authentication-Results: phobos.denx.de; dkim=pass (1024-bit key; unprotected) header.d=chromium.org header.i=@chromium.org header.b="cMEy1S1z"; dkim-atps=neutral Received: by phobos.denx.de (Postfix, from userid 109) id 219AB83C39; Mon, 14 Mar 2022 19:25:09 +0100 (CET) Received: from mail-vk1-xa35.google.com (mail-vk1-xa35.google.com [IPv6:2607:f8b0:4864:20::a35]) (using TLSv1.3 with cipher TLS_AES_128_GCM_SHA256 (128/128 bits)) (No client certificate requested) by phobos.denx.de (Postfix) with ESMTPS id 97B8383BDF for ; Mon, 14 Mar 2022 19:24:51 +0100 (CET) Authentication-Results: phobos.denx.de; dmarc=pass (p=none dis=none) header.from=chromium.org Authentication-Results: phobos.denx.de; spf=pass smtp.mailfrom=sjg@google.com Received: by mail-vk1-xa35.google.com with SMTP id w128so8836468vkd.3 for ; Mon, 14 Mar 2022 11:24:51 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=chromium.org; s=google; h=mime-version:references:in-reply-to:from:date:message-id:subject:to :cc:content-transfer-encoding; bh=xW/pJDU5S6hovc3MgqLq3CDVRddqJz5DytsnWXNMAHQ=; b=cMEy1S1zx2YSCt+ioSzqsrUd8HynXoKCsMZ3l7oxhNinGf9OSezJ7HexgUxIb1mK3O hl5zo19I4UYQBNl5GY/4enk0JSecje/qe+FUTh/4tjRTA/9XBie5bQIyCMWJo2Eb1cDm fOuq3gnxqbOBO1YEIq8oWc7n68wIgoMZuBKXc= X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=x-gm-message-state:mime-version:references:in-reply-to:from:date :message-id:subject:to:cc:content-transfer-encoding; bh=xW/pJDU5S6hovc3MgqLq3CDVRddqJz5DytsnWXNMAHQ=; b=t9jghGIvgs34fNMTa/5N2jERbg4Kpkeb4UWFeURMZT5bDtnLjkgqT4IXZgiLOxRawl HjvdyWSK5H9Tp1mZmJELon0JHWM5jT0Y0Cc7YmQ+cX6NCgr6+iajNnwNBGx/1szoDbnw QCz+2iZU/Y3kRzkm3AaTh1FI9iUHbdc94fyBsKBqok98VUH//wxwuaLGWYNWVdoxc8zU wstMv7dCPv3xh/mdERPT1KNjsJ5iQgAmCCTaCWaK0zCFeMdr8p5/fmx164l9JdQ056k9 Bma5VgjtahxUhqaxvSJetYm5OaciEvyz7USl6vy14nHokrVYmiFJZfpyO9OVKcerm7pI nxWw== X-Gm-Message-State: AOAM533ItN0oLYLwN+ZvIIO7uN3nJCSkQcaJ5c0xrNYgkQp/Vyt59xd9 spS8XgWlO/Mt2O39XCBJfsV67hfoHlk0DokeR9Ne+g== X-Google-Smtp-Source: ABdhPJyqih1CXxeJhUTbs0lPUW6zzL9WD9/acFi6ADp2YgJn8s716meAebVcBinkgF6zTYPJOGaT5tbH5mibFdk2Zzs= X-Received: by 2002:a05:6122:20a1:b0:337:4784:31ab with SMTP id i33-20020a05612220a100b00337478431abmr10015006vkd.16.1647282290094; Mon, 14 Mar 2022 11:24:50 -0700 (PDT) MIME-Version: 1.0 References: <4ba143f8-6114-db0e-08e2-d97ac9dc6e13@gmx.de> <20220219011550.GB6672@laputa> <20220314010813.GA37492@laputa> <20220314024258.GC37492@laputa> In-Reply-To: From: Simon Glass Date: Mon, 14 Mar 2022 12:24:37 -0600 Message-ID: Subject: Re: [PATCH] test/py: efi_capsule: Handle expected reset after capsule on disk To: Masami Hiramatsu Cc: AKASHI Takahiro , Heinrich Schuchardt , Tom Rini , U-Boot Mailing List , Patrick Delaunay , Patrice Chotard , Alexander Graf , Bin Meng , Ilias Apalodimas , Jose Marinho , Grant Likely , Etienne Carriere , Sughosh Ganu , Paul Liu Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-BeenThere: u-boot@lists.denx.de X-Mailman-Version: 2.1.39 Precedence: list List-Id: U-Boot discussion List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: u-boot-bounces@lists.denx.de Sender: "U-Boot" X-Virus-Scanned: clamav-milter 0.103.5 at phobos.denx.de X-Virus-Status: Clean Hi Masami, On Mon, 14 Mar 2022 at 01:35, Masami Hiramatsu wrote: > > Hi Simon, > > 2022=E5=B9=B43=E6=9C=8814=E6=97=A5(=E6=9C=88) 15:45 Simon Glass : > > > > Hi Takahiro, > > > > On Sun, 13 Mar 2022 at 20:43, AKASHI Takahiro > > 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 > > > > 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 > > > > > > 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=E5=B9=B42=E6=9C=8818=E6=97=A5(=E9=87=91) 2:56 Simon = Glass : > > > > > > > > > > > > > > > > > > > > > > > > > > > > > Hi Masami, > > > > > > > > > > > > > > > > > > > > On Wed, 16 Feb 2022 at 18:11, Masami Hiramatsu > > > > > > > > > > wrote: > > > > > > > > > > > > > > > > > > > > > > Hi Simon, > > > > > > > > > > > > > > > > > > > > > > Let me confirm your point. > > > > > > > > > > > So are you concerning the 'real' reset for the capsul= e update test > > > > > > > > > > > case itself or this patch? > > > > > > > > > > > > > > > > > > > > > > I'm actually learning how the test is working, so ple= ase help me to > > > > > > > > > > > understand how I can solve it. > > > > > > > > > > > > > > > > > > > > > > There are 3 environments to run the test, sandbox, Qe= mu, 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 sh= ould 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 ca= psule updates > > > > > > > > lead to resets. > > > > > > > > > > > > > > > > > > > > > > > > > > In dm_test_sysreset_base(), I can see the below code, thi= s means > > > > > > > > > sysreset_request() > > > > > > > > > will not execute real reset, but just mimic the reset, ri= ght? > > > > > > > > > > > > > > > > > > state->sysreset_allowed[SYSRESET_WARM] =3D true; > > > > > > > > > ut_asserteq(-EINPROGRESS, sysreset_request(dev, SYSRESET_= WARM)); > > > > > > > > > state->sysreset_allowed[SYSRESET_WARM] =3D false; > > > > > > > > > > > > > > > > > > > > but Qemu and real board will cause a real reset and i= t will terminate > > > > > > > > > > > the qemu or stop the board (depends on how it is impl= emented). Thus, > > > > > > > > > > > if a command or boot process will cause a reset, it w= ill 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 a= s all other > > > > > > > > boards to get a meaningful test results. Therefore don't pu= t 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 firmwa= re > > > > > 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 b= y 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 th= e > > > > > > 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