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

Issue 642243 link

Starred by 1 user

Issue metadata

Status: Archived
Owner:
Last visit > 30 days ago
Closed: Sep 2016
Cc:
EstimatedDays: ----
NextAction: ----
OS: Chrome
Pri: 0
Type: Bug



Sign in to add a comment

flashrom in chroot can not longer flash correctly

Project Member Reported by diand...@chromium.org, Aug 30 2016

Issue description

I updated my chroot today and then spent a whole bunch of time tracking down what I thought was a firmware problem.  

It wasn't.  It was a flashrom problem.  Confirmed that running without "--noverify" showed that flashrom was reporting problems, like:

Verifying flash... VERIFY FAILED at 0x00001800! Expected=0x00, Read=0xff, failed byte count from 0x00000000-0x007fffff: 0x11e48

---

Problems appear to be with:

commit 9816fc84a2f33c580b603266268c14d8871c1623
Author:     Stefan Tauner <stefan.tauner@alumni.tuwien.ac.at>
AuthorDate: Fri Aug 12 15:47:49 2016 -0700
Commit:     chrome-bot <chrome-bot@chromium.org>
CommitDate: Mon Aug 29 03:02:38 2016 -0700

    Warnings for one-time programmable (OTP) memory
    
    Corresponds to r1493 from upstream.
    Commit log from upstream:
    Some flash chips contain OTP memory that we cannot read or write (yet). This
    prohibits us from cloning them, hence warn the user if we detect it. Not all
    variations of the tagged chips contain OTP memory. They are often only
    enabled on request or have there own ordering numbers. There is usually no
    way to distinguish them. Because this is a supposedly seldomly used feature
    the warning is shown in with dbg verbosity.
    
    The manpage is extended to describe the backgrounds a bit.
    
    This patch is based on the idea and code of Daniel Lenski.
    
    BUG=chromium:478356
    BRANCH=none
    TEST=needs testing
    
    Change-Id: Id6e7d1b8409e02b59aa451448490badd09e25200
    Signed-off-by: Souvik Ghosh <souvikghosh@google.com>
    Reviewed-on: https://chromium-review.googlesource.com/368880
    Commit-Ready: David Hendricks <dhendrix@chromium.org>
    Reviewed-by: David Hendricks <dhendrix@chromium.org>

---

Reverting that CL fixes me.
 
Revert them all and let <your diety here> sort them out.

https://chromium-review.googlesource.com/#/c/377704/
Cc: -souvikghosh@google.com
Seeing as how that patch basically just added debug prints I'd be pretty impressed if it were at fault.

I'll take a closer look once I get in tomorrow. There may be something overlooked in one of the other patches brought in by Souvik during his internship.

Comment 3 by groeck@chromium.org, Aug 30 2016

A closer look into the code reveals that FEATURE_ERASE_TO_ZERO and FEATURE_OTP have the same bit value.

Status: Started (was: Untriaged)
Ah, yep, that makes perfect sense. FEATURE_ERASE_TO_ZERO was introduced in the chromium branch and FEATURE_OTP was introduced in upstream at different times.

I whipped up a patch to fix the issue so we don't need to revert the original patch (which hasn't been merged anyway): https://chromium-review.googlesource.com/377693
Project Member

Comment 5 by bugdroid1@chromium.org, Aug 31 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/third_party/flashrom/+/ff55cf6e7ec5ab5946a134c2f99d6be64f1e4cd1

commit ff55cf6e7ec5ab5946a134c2f99d6be64f1e4cd1
Author: David Hendricks <dhendrix@chromium.org>
Date: Tue Aug 30 18:22:31 2016

Fix conflicting FEATURE_OTP and FEATUTRE_ERASE_TO_ZERO #defines

FEATURE_OTP and FEATURE_ERASE_TO_ZERO were introduced in different
branches at different times and ended up conflicting. This changes
FEATURE_ERASE_TO_ZERO to (1 << 9).

While we're at it, condense the list a bit so it's not as easy to
overlook next time we're merging a bunch of patches...

BUG= chromium:642243 
BRANCH=none
TEST=flashing didn't fail when FEATURE_OTP added (tested using
W25Q128FW on Reef)

Change-Id: Idd06f1c582c35f9d6b696abaabf4c6f38e7f5858
Signed-off-by: David Hendricks <dhendrix@chromium.org>
Reviewed-on: https://chromium-review.googlesource.com/377693
Reviewed-by: Douglas Anderson <dianders@chromium.org>
Reviewed-by: Guenter Roeck <groeck@chromium.org>

[modify] https://crrev.com/ff55cf6e7ec5ab5946a134c2f99d6be64f1e4cd1/flash.h

Status: Fixed (was: Started)
Should be fixed, but please do yell if you run into any further difficulties.
Status: Available (was: Fixed)
Looks like flashrom still failed on recent build, 8762.0.0.

Check the FAFT dashboard:
https://wmatrix.googleplex.com/platform/unfiltered?suites=faft_bios&days_back=30&releases=55&platforms=lulu&hide_missing=True&show_faft_view=True

The fix was landed on 8760 (https://crosland.corp.google.com/log/8759.0.0..8760.0.0)
Re #7: Hmmm, I suspect that CL:372463 may be the culprit in that case. It seems that the policy handling stuff we have to deal with the Intel flash protection doesn't handle these cases gracefully, so let's revert it (I tested the revert on auron_paine).

Side note: We recently wrote an shiny new test script for flashrom (chromium:621715) that can test all use cases on all the different platforms, it would be great to get that integrated into the test suite and avoid surprises like this down the road...
Project Member

Comment 10 by bugdroid1@chromium.org, Sep 4 2016

The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/third_party/flashrom/+/53540f9a5911f58a53c03fcee143ae571e082886

commit 53540f9a5911f58a53c03fcee143ae571e082886
Author: David Hendricks <dhendrix@chromium.org>
Date: Sat Sep 03 00:34:41 2016

Revert "ichspi.c - warn user and disable writes for a protected address range"

This reverts commit 18da72c22a599a7fc30307fd3119e7ea273d2cc8.

BUG= chromium:642243 
TEST=tested on paine

Change-Id: Iff4d4a6ec53ad1943063abe2f0b1fc82c256e1dc
Reviewed-on: https://chromium-review.googlesource.com/380935
Commit-Ready: David Hendricks <dhendrix@chromium.org>
Tested-by: David Hendricks <dhendrix@chromium.org>
Reviewed-by: David Hendricks <dhendrix@chromium.org>

[modify] https://crrev.com/53540f9a5911f58a53c03fcee143ae571e082886/flashrom.8
[modify] https://crrev.com/53540f9a5911f58a53c03fcee143ae571e082886/ichspi.c

Status: Fixed (was: Available)
Revert patch landed, closing out for now.
Labels: VerifyIn-55

Comment 13 by dchan@google.com, Nov 19 2016

Labels: VerifyIn-56
Project Member

Comment 14 by bugdroid1@chromium.org, Jan 17 2017

Labels: merge-merged-factory-glados-7828.B
The following revision refers to this bug:
  https://chromium.googlesource.com/chromiumos/third_party/flashrom/+/2a962ac9e9fe0e62059a7758824a1aa5aa4e9050

commit 2a962ac9e9fe0e62059a7758824a1aa5aa4e9050
Author: David Hendricks <dhendrix@chromium.org>
Date: Tue Aug 30 18:22:31 2016

Fix conflicting FEATURE_OTP and FEATUTRE_ERASE_TO_ZERO #defines

FEATURE_OTP and FEATURE_ERASE_TO_ZERO were introduced in different
branches at different times and ended up conflicting. This changes
FEATURE_ERASE_TO_ZERO to (1 << 9).

While we're at it, condense the list a bit so it's not as easy to
overlook next time we're merging a bunch of patches...

BUG= chromium:642243 
BRANCH=none
TEST=flashing didn't fail when FEATURE_OTP added (tested using
W25Q128FW on Reef)

Change-Id: Idd06f1c582c35f9d6b696abaabf4c6f38e7f5858
Signed-off-by: David Hendricks <dhendrix@chromium.org>
Reviewed-on: https://chromium-review.googlesource.com/377693
Reviewed-by: Douglas Anderson <dianders@chromium.org>
Reviewed-by: Guenter Roeck <groeck@chromium.org>
Reviewed-on: https://chromium-review.googlesource.com/428272
Tested-by: Jongpil Jung <jongpil19.jung@samsung.com>
Commit-Queue: Jongpil Jung <jongpil19.jung@samsung.com>

[modify] https://crrev.com/2a962ac9e9fe0e62059a7758824a1aa5aa4e9050/flash.h

Comment 15 by dchan@google.com, Jan 21 2017

Labels: VerifyIn-57

Comment 16 by dchan@google.com, Mar 4 2017

Labels: VerifyIn-58

Comment 17 by dchan@google.com, Apr 17 2017

Labels: VerifyIn-59

Comment 18 by dchan@google.com, May 30 2017

Labels: VerifyIn-60
Labels: VerifyIn-61

Comment 20 by dchan@chromium.org, Oct 14 2017

Status: Archived (was: Fixed)

Sign in to add a comment