Attention is currently required from: Anastasia Klimchuk, Matt DeVillier.
Hello Matt DeVillier, Peter Marheine, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/flashrom/+/85655?usp=email
to look at the new patch set (#3).
The following approvals got outdated and were removed:
Verified+1 by build bot (Jenkins)
Change subject: doc: Add section about v1.5.1 into release notes
......................................................................
doc: Add section about v1.5.1 into release notes
Change-Id: I80f8423133bf779093d57ea6928f09d9d377d20e
Signed-off-by: Anastasia Klimchuk <aklm(a)flashrom.org>
Signed-off-by: Matt DeVillier <matt.devillier(a)gmail.com>
---
M doc/release_notes/v_1_5.rst
1 file changed, 32 insertions(+), 6 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/55/85655/3
--
To view, visit https://review.coreboot.org/c/flashrom/+/85655?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email
Gerrit-MessageType: newpatchset
Gerrit-Project: flashrom
Gerrit-Branch: 1.5.x
Gerrit-Change-Id: I80f8423133bf779093d57ea6928f09d9d377d20e
Gerrit-Change-Number: 85655
Gerrit-PatchSet: 3
Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Reviewer: Matt DeVillier <matt.devillier(a)gmail.com>
Gerrit-Reviewer: Peter Marheine <pmarheine(a)chromium.org>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Matt DeVillier <matt.devillier(a)gmail.com>
Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org>
Attention is currently required from: Anastasia Klimchuk, Matt DeVillier.
Peter Marheine has posted comments on this change by Anastasia Klimchuk. ( https://review.coreboot.org/c/flashrom/+/85655?usp=email )
Change subject: doc: Add section about v1.5.1 into release notes
......................................................................
Patch Set 2:
(1 comment)
Patchset:
PS2:
> The wording I tried to make such that I don't need to return back and change after putting a tag (which we usually do with main releases).
That's reasonable, but as long as we know what we want the URL to be I think it's fine to have a short window where there's a link that doesn't work. Better to make everything explicit, I think.
--
To view, visit https://review.coreboot.org/c/flashrom/+/85655?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email
Gerrit-MessageType: comment
Gerrit-Project: flashrom
Gerrit-Branch: 1.5.x
Gerrit-Change-Id: I80f8423133bf779093d57ea6928f09d9d377d20e
Gerrit-Change-Number: 85655
Gerrit-PatchSet: 2
Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Reviewer: Matt DeVillier <matt.devillier(a)gmail.com>
Gerrit-Reviewer: Peter Marheine <pmarheine(a)chromium.org>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Matt DeVillier <matt.devillier(a)gmail.com>
Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Comment-Date: Thu, 19 Dec 2024 23:38:01 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org>
Attention is currently required from: Anastasia Klimchuk, Matt DeVillier.
Peter Marheine has posted comments on this change by Anastasia Klimchuk. ( https://review.coreboot.org/c/flashrom/+/85655?usp=email )
Change subject: doc: Add section about v1.5.1 into release notes
......................................................................
Patch Set 2:
(1 comment)
File doc/release_notes/v_1_5.rst:
https://review.coreboot.org/c/flashrom/+/85655/comment/e801b007_69542160?us… :
PS2, Line 38: v1.5.1 can be downloaded in the same location, same fingerprint. Tarball name will be ``flashrom-v1.5.1.tar.xz``.
> I'd prefer to include the links so readers don't need to manually munge the URL, and put them up in […]
Forgot to include the git tag names, but I think those should also be included near the tarball links. So something like
```
Git tag: v1.5.1
Tarball: ...
...
Git tag: v1.5.0
Tarball: ...
```
--
To view, visit https://review.coreboot.org/c/flashrom/+/85655?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email
Gerrit-MessageType: comment
Gerrit-Project: flashrom
Gerrit-Branch: 1.5.x
Gerrit-Change-Id: I80f8423133bf779093d57ea6928f09d9d377d20e
Gerrit-Change-Number: 85655
Gerrit-PatchSet: 2
Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Reviewer: Matt DeVillier <matt.devillier(a)gmail.com>
Gerrit-Reviewer: Peter Marheine <pmarheine(a)chromium.org>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Matt DeVillier <matt.devillier(a)gmail.com>
Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Comment-Date: Thu, 19 Dec 2024 23:33:54 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Peter Marheine <pmarheine(a)chromium.org>
Attention is currently required from: Anastasia Klimchuk, Matt DeVillier.
Peter Marheine has posted comments on this change by Anastasia Klimchuk. ( https://review.coreboot.org/c/flashrom/+/85655?usp=email )
Change subject: doc: Add section about v1.5.1 into release notes
......................................................................
Patch Set 2:
(1 comment)
File doc/release_notes/v_1_5.rst:
https://review.coreboot.org/c/flashrom/+/85655/comment/373ebe44_13fe376c?us… :
PS2, Line 38: v1.5.1 can be downloaded in the same location, same fingerprint. Tarball name will be ``flashrom-v1.5.1.tar.xz``.
I'd prefer to include the links so readers don't need to manually munge the URL, and put them up in the Download section above so it's very obvious that 1.5.1 is the newest. Something like:
```
Download
========
flashrom v1.5 can be downloaded either via anonymous git at https://review.coreboot.org/flashrom.git, or as signed source tarballs.
The key fingerprint for source tarballs is
6E6E F9A0 BA47 8006 E277 6E4C C037 BB41 3134 D111.
Version 1.5.1
-------------
Version 1.5.1 fixed an issue flashing some Intel-based platforms with the internal programmer that was introduced in 1.5.0. All users are encouraged to update.
Tarball: https://download.flashrom.org/releases/flashrom-v1.5.1.tar.xz
Signature: https://download.flashrom.org/releases/flashrom-v1.5.1.tar.xz.asc
Version 1.5.0
-------------
Tarball: https://download.flashrom.org/releases/flashrom-v1.5.0.tar.xz
Signature: https://download.flashrom.org/releases/flashrom-v1.5.0.tar.xz.asc
```
--
To view, visit https://review.coreboot.org/c/flashrom/+/85655?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email
Gerrit-MessageType: comment
Gerrit-Project: flashrom
Gerrit-Branch: 1.5.x
Gerrit-Change-Id: I80f8423133bf779093d57ea6928f09d9d377d20e
Gerrit-Change-Number: 85655
Gerrit-PatchSet: 2
Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Reviewer: Matt DeVillier <matt.devillier(a)gmail.com>
Gerrit-Reviewer: Peter Marheine <pmarheine(a)chromium.org>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Matt DeVillier <matt.devillier(a)gmail.com>
Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Comment-Date: Thu, 19 Dec 2024 23:30:54 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Attention is currently required from: Matt DeVillier, Peter Marheine.
Anastasia Klimchuk has posted comments on this change by Anastasia Klimchuk. ( https://review.coreboot.org/c/flashrom/+/85655?usp=email )
Change subject: doc: Add section about v1.5.1 into release notes
......................................................................
Patch Set 2:
(1 comment)
Patchset:
PS2:
Peter, thank you so much for review!
The wording I tried to make such that I don't need to return back and change after putting a tag (which we usually do with main releases).
I will later cherry-pick to main, so that it can be published on the website. But that will be after the tag.
--
To view, visit https://review.coreboot.org/c/flashrom/+/85655?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email
Gerrit-MessageType: comment
Gerrit-Project: flashrom
Gerrit-Branch: 1.5.x
Gerrit-Change-Id: I80f8423133bf779093d57ea6928f09d9d377d20e
Gerrit-Change-Number: 85655
Gerrit-PatchSet: 2
Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Reviewer: Matt DeVillier <matt.devillier(a)gmail.com>
Gerrit-Reviewer: Peter Marheine <pmarheine(a)chromium.org>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Matt DeVillier <matt.devillier(a)gmail.com>
Gerrit-Attention: Peter Marheine <pmarheine(a)chromium.org>
Gerrit-Comment-Date: Thu, 19 Dec 2024 20:16:26 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Attention is currently required from: Anastasia Klimchuk.
Matt DeVillier has posted comments on this change by Anastasia Klimchuk. ( https://review.coreboot.org/c/flashrom/+/85655?usp=email )
Change subject: doc: Add section about v1.5.1 into release notes
......................................................................
Patch Set 2:
(1 comment)
File doc/release_notes/v_1_5.rst:
https://review.coreboot.org/c/flashrom/+/85655/comment/0245a752_607087a8?us… :
PS1, Line 34: <TODO ADD WHO IS AFFECTED>
> Matt, do you remember we discussed that you can help with writing this piece? :) I appreciate your h […]
Done
--
To view, visit https://review.coreboot.org/c/flashrom/+/85655?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email
Gerrit-MessageType: comment
Gerrit-Project: flashrom
Gerrit-Branch: 1.5.x
Gerrit-Change-Id: I80f8423133bf779093d57ea6928f09d9d377d20e
Gerrit-Change-Number: 85655
Gerrit-PatchSet: 2
Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Reviewer: Matt DeVillier <matt.devillier(a)gmail.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Comment-Date: Thu, 19 Dec 2024 18:10:28 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org>
Matt DeVillier has abandoned this change. ( https://review.coreboot.org/c/flashrom/+/85679?usp=email )
Change subject: doc: Add section about v1.5.1 into release notes
......................................................................
Abandoned
--
To view, visit https://review.coreboot.org/c/flashrom/+/85679?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email
Gerrit-MessageType: abandon
Gerrit-Project: flashrom
Gerrit-Branch: main
Gerrit-Change-Id: I80f8423133bf779093d57ea6928f09d9d377d20e
Gerrit-Change-Number: 85679
Gerrit-PatchSet: 1
Gerrit-Owner: Matt DeVillier <matt.devillier(a)gmail.com>
Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Attention is currently required from: Anastasia Klimchuk.
Hello Anastasia Klimchuk,
I'd like you to do a code review.
Please visit
https://review.coreboot.org/c/flashrom/+/85679?usp=email
to review the following change.
Change subject: doc: Add section about v1.5.1 into release notes
......................................................................
doc: Add section about v1.5.1 into release notes
Change-Id: I80f8423133bf779093d57ea6928f09d9d377d20e
Signed-off-by: Anastasia Klimchuk <aklm(a)flashrom.org>
Signed-off-by: Matt DeVillier <matt.devillier(a)gmail.com>
---
M doc/release_notes/v_1_5.rst
1 file changed, 18 insertions(+), 3 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/79/85679/1
diff --git a/doc/release_notes/v_1_5.rst b/doc/release_notes/v_1_5.rst
index 6e81353..9471df0 100644
--- a/doc/release_notes/v_1_5.rst
+++ b/doc/release_notes/v_1_5.rst
@@ -15,10 +15,25 @@
Anonymous checkout from the git repository at https://review.coreboot.org/flashrom.git
(tag v1.5.0)
-A tarball is available for download at <TODO add tarball>
-(signature <TODO add signature>)
+A tarball is available for download at https://download.flashrom.org/releases/flashrom-v1.5.0.tar.xz
+(signature https://download.flashrom.org/releases/flashrom-v1.5.0.tar.xz.asc)
-fingerprint: <TODO add fingerprint>
+fingerprint: 6E6E F9A0 BA47 8006 E277 6E4C C037 BB41 3134 D111
+
+Point release v1.5.1
+====================
+
+v1.5.1 includes one bug fix in addition to everything which is included in v1.5.0
+
+Ticket: https://ticket.coreboot.org/issues/573
+
+Patch: https://review.coreboot.org/c/flashrom/+/85612
+
+Who exactly may be affected by this bug:
+
+Users with older Intel-based platforms (Broadwell/Braswell and earlier) flashing using the internal
+programmer option may encounter an 'Invalid OPCODE' error when erasing/writing which leads to an
+incomplete flash and potentially a bricked device. External flashing is not affected at all.
Known issues
============
--
To view, visit https://review.coreboot.org/c/flashrom/+/85679?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email
Gerrit-MessageType: newchange
Gerrit-Project: flashrom
Gerrit-Branch: main
Gerrit-Change-Id: I80f8423133bf779093d57ea6928f09d9d377d20e
Gerrit-Change-Number: 85679
Gerrit-PatchSet: 1
Gerrit-Owner: Matt DeVillier <matt.devillier(a)gmail.com>
Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org>
Attention is currently required from: Anastasia Klimchuk, Anton Samsonov, Nikolai Artemiev, Stefan Reinauer.
Anton Samsonov has posted comments on this change by Anton Samsonov. ( https://review.coreboot.org/c/flashrom/+/85585?usp=email )
Change subject: flashchips: Add Spansion S25FS512S
......................................................................
Patch Set 2:
(3 comments)
File flashchips/spansion.c:
https://review.coreboot.org/c/flashrom/+/85585/comment/d5421685_7b7138ba?us… :
PS2, Line 1043: /* Note on FEATURE_4BA_ENTER: the command set only defines command 0xB7
: to enter 4BA mode, but there is no counterpart command "Exit 4BA mode",
: and the code 0xE9 is assigned to another command ("Password unlock"). */
: /* Note on FEATURE_4BA_ENTER_EAR7 (not set): the "Extended address mode" bit
: is stored in configuration register CR2V[7], which can only be read / written
: by generic commands ("Read any register" / "Write any register"), that itself
: expect a 3- or 4-byte address of register in question, which in turn depends on
: the same register, that is initially set from non-volatile register CR2NV[7]
: that defaults to 0, but can be programmed to 1 for starting in 32-bit mode. */
> Thank you for sharing bits of wisdom in the comment!
That is not a "wisdom", but merely observations of trial and error in finding proper combination. Without `FEATURE_4BA_ENTER`, this chip behaves very strange even when the sector 0 or any other sector <16M is addressed: it reports that the erase operation succeeded, when in fact most bits were left intact and other bits turned volatile — i. e. having random state (0 or 1) on each read.
Such an unreliable operation led me to Evaluate Erase Status command (code D0h, see section 9.6.4 on page 113 of the datasheet), which looked very promising as an almost-instant built-in implementation of `is_sector_blank()` functionality that abolish data transmission from chip to host for erase verification. I implemented that in s25f.c, also generalizing `s25fl_sector_erase()` into `s25f_sector_erase()` with adaptive timeouts and automatic retry of failed erases. However, it turned out that this command always returns "OK" even when no erase was attempted at all.
After finding out that simply adding `FEATURE_4BA_ENTER` "just works" even with generic `spi_block_erase_dc()`, I did not went further the "proprietary" way and did not bothered with adding Blank Check During Erase functionality (CR3V[5], see section 7.5.5.2 on page 62 of the datasheet). If someone is eager to have a look at their description and perhaps give an insight on what am I missing in understanding their intended behavior and/or proper use pattern, I can upload my current implementation as a separate "WiP" patch.
File include/flashchips.h:
https://review.coreboot.org/c/flashrom/+/85585/comment/c6526d23_9d59c80c?us… :
PS2, Line 717: 0x02200081
> 03h -> ?? where it is?
It is not a part of extended Device ID, but rather a header field indicating the total ID-CFI legacy structure size and the offset where the alternate structure starts (that is, 4Dh in this field means that the legacy part ends at byte 50h, and the alternate part starts at byte 51h). While it is essential for parsing the entire structure, it is obviously not a part of Device ID, thus `probe_spi_big_spansion()` simply concatenates bytes 01–02 and 04–05 to form a 32-bit identifier.
The exact chip name, for example "S25FS512S", is stored in parameter 0 of alternate structure (see Table 62 on page 140) starting at offset 02h there; and even the minor part number designation at offset 10h–11h — same as bytes at offset 06h–07h in the legacy Device ID structure. However, using such a string as identifier would require up to 14 bytes, not counting the minor designation, which only `uint128_t` can accommodate. So a hashing algorithm may be necessary to keep 32-bit compatible identifiers, such as "compute `sha256(string)` and take the first 4 bytes as a big-endian number".
https://review.coreboot.org/c/flashrom/+/85585/comment/9d60c1a6_2abbcd92?us… :
PS2, Line 717: SPANSION_S25FS512S
> `SPANSION_S25FS512S_UL`
To tell the truth, I was tearing apart between `S25FS512S` and just `S25FS512` (without the trailing "S"), as `S25FL512` already uses that naming scheme, and because the datasheet defines no major variations for 512-Mbit model.
If you vote for the `_UL` suffix, then should `S25FL512` also be changed to `S25FL512S_UL` simultaneously, since this patch alters that macro anyway?
--
To view, visit https://review.coreboot.org/c/flashrom/+/85585?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email
Gerrit-MessageType: comment
Gerrit-Project: flashrom
Gerrit-Branch: main
Gerrit-Change-Id: I40b6c081ec7d57eac4f6d2b69cea3878bc92bb47
Gerrit-Change-Number: 85585
Gerrit-PatchSet: 2
Gerrit-Owner: Anton Samsonov <avscomputing(a)gmail.com>
Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Reviewer: Anton Samsonov <devel(a)zxlab.ru>
Gerrit-Reviewer: Nikolai Artemiev <nartemiev(a)google.com>
Gerrit-Reviewer: Stefan Reinauer <stefan.reinauer(a)coreboot.org>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Stefan Reinauer <stefan.reinauer(a)coreboot.org>
Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Attention: Nikolai Artemiev <nartemiev(a)google.com>
Gerrit-Attention: Anton Samsonov <devel(a)zxlab.ru>
Gerrit-Comment-Date: Thu, 19 Dec 2024 13:13:20 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org>
Attention is currently required from: Anton Samsonov, David Hendricks, Matt DeVillier.
Anastasia Klimchuk has posted comments on this change by Matt DeVillier. ( https://review.coreboot.org/c/flashrom/+/85160?usp=email )
Change subject: cli_classic: Add option to use first detected chip if multiple found
......................................................................
Patch Set 2:
(1 comment)
Patchset:
PS2:
> > There are certainly cases where there is no harm in selecting the first chip of multiple. […]
Anton, thank you so much for joining. You have many useful ideas!
I am not doing anything right now because I am thinking what is the right thing to do here.
I fully understand the problem Matt is trying to solve, the IDs are repeating, more and more with time (because we never remove chip definitions, we only add new ones).
I want to make SFDP detection more smooth next year but in any case this does not cover old chips.
About the idea to use first chip: I still hold my initial opinion that this is a dangerous option in general, if we look at all the variety of use cases how people use flashrom and on which environments. However if we narrow down the use case, I understand that there is a use case when this option is fine. And actually, Matt's userbase is mostly belong to that category.
I don't currently have a good idea whether it is possible to add this option so that "safe target audience" would be using it, and everyone else would know it is dangerous and won't.
Maybe there is no safe way to add such option on upstream.
Let's keep this patch here for a while? As I said, repeated IDs are real, and this is something on my mind to improve. It was very interesting to me to read Anton's thoughts.
--
To view, visit https://review.coreboot.org/c/flashrom/+/85160?usp=email
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email
Gerrit-MessageType: comment
Gerrit-Project: flashrom
Gerrit-Branch: main
Gerrit-Change-Id: I0427b1ef80e4eca16623f0fc9119d79f7dd62551
Gerrit-Change-Number: 85160
Gerrit-PatchSet: 2
Gerrit-Owner: Matt DeVillier <matt.devillier(a)gmail.com>
Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Anton Samsonov <avscomputing(a)gmail.com>
Gerrit-CC: David Hendricks <david.hendricks(a)gmail.com>
Gerrit-Attention: Anton Samsonov <avscomputing(a)gmail.com>
Gerrit-Attention: Matt DeVillier <matt.devillier(a)gmail.com>
Gerrit-Attention: David Hendricks <david.hendricks(a)gmail.com>
Gerrit-Comment-Date: Thu, 19 Dec 2024 12:57:46 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Anton Samsonov <avscomputing(a)gmail.com>
Comment-In-Reply-To: Matt DeVillier <matt.devillier(a)gmail.com>
Comment-In-Reply-To: David Hendricks <david.hendricks(a)gmail.com>
Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org>