New issue
Advanced search Search tips
Note: Color blocks (like or ) mean that a user may not be available. Tooltip shows the reason.

Issue 670492 link

Starred by 2 users

Issue metadata

Status: Fixed
Owner:
Closed: Oct 2
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: ----
Pri: 1
Type: Bug



Sign in to add a comment

Make vboot ignore power button shutdown requests if the button had continuously been pressed since boot

Project Member Reported by jwer...@chromium.org, Dec 1 2016

Issue description

According to Danny, a firmware testing policy change now requires us to support manual recovery mode entry where the user holds down Esc+Refresh+Power until the screen appears on future devices:

https://testtracker.googleplex.com/testplans/testcase/detail/413773?id=382&revision=271

On some boards the power button is piped through the EC to the AP, and the EC explicitly inhibits it in this case. But on others, the AP has direct access to the power button state. On these devices, depthcharge will read that state as soon as the vboot UI loop starts running in recovery mode and immediately react with a shutdown if it is pressed. This means that the recovery screen will blink for a split second before the device shuts off again.

We need a simple vboot change that calls VbExIsShutdownRequested() once at the start of VbSelectAndLoadKernel(), stores the state of the VB_SHUTDOWN_REQUEST_POWER_BUTTON flag, and uses that to inhibit future VbWantShutdown() tests until we have read it as 0 at least once.

Assigning to Stefan for triage. This should be fixed before the next device that is wired like this branches.
 

Comment 1 by dchan@chromium.org, Dec 2 2016

I think the requirement or test case was coming from you with heading "Re: recovery tricks"
Here's the email snippet:
  (d) held ESC+Refresh down until they saw the Recovery screen

There are not timing requirement when going into recovery mode. Is this specific to Oak family ? 

The current official instructions on www.google.com/chromeos/recovery say "press and hold Esc + Refresh, then press Power. Let go of Power, then let go of the other keys." -- they do not specify any timing details. Thus, unless we want to change that text, we should probably try to test a range of timings to cover as much possible user behavior as we can.
 
It is a good question, though, why this wasn't found during the more exhaustive manual/FSI testing?

The firmware test team performed 4 rounds of firmware testing on Elm, 2 on EVT and 2 on DVT.   As stated above we have various boot sequence tests.  AC connected is not part of the test case.  Should with and without AC be added as part of the test case ? Is this new to oak/elm and most likely on future boards ?  

Yes, since the Silego is tied to both recovery and battery cut-off on all current designs (although I'm not sure if that will change with Haven), it would probably be worth adding a more extensive "entering recovery" test case. Something like:

Perform each of these both with and without AC connected and verify that you end up on the recovery screen:

1. Hold down Esc+Refresh, then tap Power and let go of all keys as fast as you can (~100-300ms is probably humanly possible).
2. Hold down Esc+Refresh, then hold down power and release all keys after at least one full second.
3. Hold down Esc+Refresh, then hold down power and keep all keys held until the recovery screen appears.

Comment 2 by dchan@chromium.org, Dec 2 2016

Cc: venkatar...@chromium.org

Comment 3 by gkihumba@google.com, Mar 31 2017

Status: Assigned (was: Untriaged)
Project Member

Comment 4 by bugdroid1@chromium.org, Oct 2

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/platform/vboot_reference/+/00d4be66721b24903f977e770148179035254e19

commit 00d4be66721b24903f977e770148179035254e19
Author: Edward Hill <ecgh@chromium.org>
Date: Tue Oct 02 20:23:08 2018

Ignore power button if held on startup

Ignore a power button push until after we have seen it released,
to avoid shutting down immediately if the power button is held
down on startup.

BUG=b:116819414, chromium:670492 
BRANCH=grunt
TEST=manual:
1) Press and hold esc+refresh+power.
2) Depthcharge shows INSERT screen and does not power off.
3) Release esc+refresh+power.
4) Press and release power.
5) Depthcharge powers off.
TEST=test_that --fast -b grunt $grunt_ip firmware_ECLidShutdown
TEST=FEATURES=test emerge-grunt --nodeps vboot_reference

Change-Id: I7421a4b1a1b8a7894f0e7d1c7927ffc52d9faac0
Signed-off-by: Edward Hill <ecgh@chromium.org>
Reviewed-on: https://chromium-review.googlesource.com/1256023
Reviewed-by: Raul E Rangel <rrangel@chromium.org>
Reviewed-by: Julius Werner <jwerner@chromium.org>

[modify] https://crrev.com/00d4be66721b24903f977e770148179035254e19/tests/vboot_api_kernel2_tests.c
[modify] https://crrev.com/00d4be66721b24903f977e770148179035254e19/firmware/lib/vboot_ui.c

Owner: ecgh@chromium.org
Status: Fixed (was: Assigned)
Thanks for the fix!
Project Member

Comment 6 by bugdroid1@chromium.org, Oct 5

Labels: merge-merged-firmware-grunt-11031.B
The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/platform/vboot_reference/+/5536c751eda9e729c95310443f81767c13fd125e

commit 5536c751eda9e729c95310443f81767c13fd125e
Author: Edward Hill <ecgh@chromium.org>
Date: Fri Oct 05 02:11:10 2018

Ignore power button if held on startup

Ignore a power button push until after we have seen it released,
to avoid shutting down immediately if the power button is held
down on startup.

BUG=b:116819414, chromium:670492 
BRANCH=grunt
TEST=manual:
1) Press and hold esc+refresh+power.
2) Depthcharge shows INSERT screen and does not power off.
3) Release esc+refresh+power.
4) Press and release power.
5) Depthcharge powers off.
TEST=test_that --fast -b grunt $grunt_ip firmware_ECLidShutdown
TEST=FEATURES=test emerge-grunt --nodeps vboot_reference

Change-Id: I7421a4b1a1b8a7894f0e7d1c7927ffc52d9faac0
Signed-off-by: Edward Hill <ecgh@chromium.org>
Reviewed-on: https://chromium-review.googlesource.com/1256023
Reviewed-by: Raul E Rangel <rrangel@chromium.org>
Reviewed-by: Julius Werner <jwerner@chromium.org>
(cherry picked from commit 00d4be66721b24903f977e770148179035254e19)
Reviewed-on: https://chromium-review.googlesource.com/c/1263029
Reviewed-by: Martin Roth <martinroth@chromium.org>
Tested-by: Martin Roth <martinroth@chromium.org>

[modify] https://crrev.com/5536c751eda9e729c95310443f81767c13fd125e/tests/vboot_api_kernel2_tests.c
[modify] https://crrev.com/5536c751eda9e729c95310443f81767c13fd125e/firmware/lib/vboot_ui.c

Sign in to add a comment