New issue
Advanced search Search tips

Issue 790875 link

Starred by 1 user

Issue metadata

Status: Archived
Owner:
Closed: Dec 2017
Cc:
Components:
EstimatedDays: ----
NextAction: ----
OS: Chrome
Pri: 1
Type: Bug



Sign in to add a comment

tpm-firmware-update: doesn't reboot after successful update

Project Member Reported by apronin@chromium.org, Dec 1 2017

Issue description

tpm firmware update process is currently broken on ToT. At least, on link, didn't try other boards.

The update workflow it is stuck on the "Installing update..." screen after the update is actually performed (running with dryrun:1) and doesn't reboot.

What I see is that tpm-firmware-updater finishes successfully (code 8 for link). run_updater() in tpm-firmware-update.sh runs fine after it to the very end of that function, /run/tpm-firmware-updater.status is created with the right code, if I insert statements after the last echo "${status:-1}" in it, they are executed.

However, the next line in the script after invoking run_updater is never called:

  local status="$(run_updater)"  
  case "${status}" in              <-- this line is never called

If I insert statements after local status="$(run_updater)", they are not executed.
 
The problem happened somewhere between 10106.0.0 (still works ok, reboots) and 10134.0.0 (stuck befre rebooting). I continue bisecting.
Description: Show this description
Cc: vapier@chromium.org jorgelo@chromium.org
10117.0.0 is the last good, 10118.0.0 is the first with the issue.
Could it be the dash uprev (https://crrev.com/c/749501) that caused it?
yes, it's due to the dash upgrade.  it's a bug in that script though -- `local` is only valid in functions and this tpm code isn't in a function.

another reason all scripts should be written using a proper main() func.
Oh true, I was staring at the code, but didn't notice the obvious local at the top level :(
Will wrap in main(). How did it work before - was the old dash more permissive?
yes, the older dash was buggy in that it allowed you to use `local` in the top/global scope.  bash would reject it.
Project Member

Comment 7 by bugdroid1@chromium.org, Dec 1 2017

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/overlays/chromiumos-overlay/+/640f9ff65244f26a9aa53583a1421af9c1e0a01e

commit 640f9ff65244f26a9aa53583a1421af9c1e0a01e
Author: Andrey Pronin <apronin@chromium.org>
Date: Fri Dec 01 08:39:43 2017

infineon-firmware-updater: wrap update.sh in main()

Before this CL, tpm-firmware-update.sh was using local variables
in the global scope, outside of any functions. After upgrading
dash that stopped working.

The CL wraps the top-level code in this script into the main()
function, and calls that function from the top level.

BUG= chromium:790875 
TEST=update is not stuck

Change-Id: I2379670291a8bf83a6eae31f77a41101f1737530
Reviewed-on: https://chromium-review.googlesource.com/802544
Commit-Ready: Andrey Pronin <apronin@chromium.org>
Tested-by: Andrey Pronin <apronin@chromium.org>
Reviewed-by: Mike Frysinger <vapier@chromium.org>

[modify] https://crrev.com/640f9ff65244f26a9aa53583a1421af9c1e0a01e/chromeos-base/infineon-firmware-updater/files/tpm-firmware-update.sh
[rename] https://crrev.com/640f9ff65244f26a9aa53583a1421af9c1e0a01e/chromeos-base/infineon-firmware-updater/infineon-firmware-updater-1.1.2459.0-r18.ebuild

Owner: apronin@chromium.org
Status: Started (was: Untriaged)
Thanks for fixing while I was distracted with other things.

Reminds me that I still need to write an autotest that covers the basic flow at least.

I assume this is fixed now?
Status: Fixed (was: Started)
Correct, fixed.
Status: Archived (was: Fixed)

Sign in to add a comment