Attention is currently required from: Anastasia Klimchuk, Nikolai Artemiev, Peter Marheine.
Hello Anastasia Klimchuk, Nikolai Artemiev, Peter Marheine, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/flashrom/+/84253?usp=email
to look at the new patch set (#5).
The following approvals got outdated and were removed:
Verified+1 by build bot (Jenkins)
Change subject: ichspi: Probe opcode in POSSIBLE_OPCODES[] as well
......................................................................
ichspi: Probe opcode in POSSIBLE_OPCODES[] as well
ich_spi_send_command() and ich_spi_send_multicommand() will overwrite
the "Sector erase" opcode with the opcode for command via
reprogram_opcode_on_the_fly(), but not restore it, causing the "Sector
erase" opcode may get lost after sending commands, leaving only "Bulk
erase" opcode which erase the whole chip available.
In the mean time, ich_spi_probe_opcode() used not to report opcodes in
POSSIBLE_OPCODES[] but not in curopcodes->opcode[] as supported.
Now, if the opcode being probed is not in curopcodes->opcode[] but in
POSSIBLE_OPCODES[], it will be reported as supported, and programmed
later by ich_spi_send_(multi)command().
Fix:https://ticket.coreboot.org/issues/556
Change-Id: I3fc831fc072e2af9265835cb2f71bf8c222c6a64
Signed-off-by: persmule <persmule(a)hardenedlinux.org>
---
M ichspi.c
1 file changed, 5 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/53/84253/5
--
To view, visit https://review.coreboot.org/c/flashrom/+/84253?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: main
Gerrit-Change-Id: I3fc831fc072e2af9265835cb2f71bf8c222c6a64
Gerrit-Change-Number: 84253
Gerrit-PatchSet: 5
Gerrit-Owner: Bill XIE <persmule(a)hardenedlinux.org>
Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Reviewer: Nikolai Artemiev <nartemiev(a)google.com>
Gerrit-Reviewer: Peter Marheine <pmarheine(a)chromium.org>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Attention: Nikolai Artemiev <nartemiev(a)google.com>
Gerrit-Attention: Peter Marheine <pmarheine(a)chromium.org>
Attention is currently required from: Anastasia Klimchuk, Nikolai Artemiev, Peter Marheine.
Bill XIE has posted comments on this change by Bill XIE. ( https://review.coreboot.org/c/flashrom/+/84253?usp=email )
Change subject: ichspi: Probe opcode in POSSIBLE_OPCODES[] as well
......................................................................
Patch Set 4:
(1 comment)
File ichspi.c:
https://review.coreboot.org/c/flashrom/+/84253/comment/b3b4f9c5_83e52c40?us… :
PS1, Line 1843:
: static bool ich_spi_probe_opcode(const struct flashctx *flash, uint8_t opcode)
: {
: return find_opcode(curopcodes, opcode) >= 0;
> Please show me your elegant solution to program other erasing opcodes on the fly, but not before CB: […]
I managed to make up a solution to program an erase opcode being probed if it is in the POSSIBLE_OPCODES during ich_spi_probe_opcode().
--
To view, visit https://review.coreboot.org/c/flashrom/+/84253?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: I3fc831fc072e2af9265835cb2f71bf8c222c6a64
Gerrit-Change-Number: 84253
Gerrit-PatchSet: 4
Gerrit-Owner: Bill XIE <persmule(a)hardenedlinux.org>
Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Reviewer: Nikolai Artemiev <nartemiev(a)google.com>
Gerrit-Reviewer: Peter Marheine <pmarheine(a)chromium.org>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Attention: Nikolai Artemiev <nartemiev(a)google.com>
Gerrit-Attention: Peter Marheine <pmarheine(a)chromium.org>
Gerrit-Comment-Date: Mon, 30 Sep 2024 09:58:55 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Bill XIE <persmule(a)hardenedlinux.org>
Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org>
Comment-In-Reply-To: Nikolai Artemiev <nartemiev(a)google.com>
Bill XIE has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/84595?usp=email )
Change subject: ichspi: const-correct POSSIBLE_OPCODES[]
......................................................................
ichspi: const-correct POSSIBLE_OPCODES[]
POSSIBLE_OPCODES[] is never modified, so mark it as read-only.
Change-Id: I217f8a9e50b9e2e9f2731adec89a46780874c754
Signed-off-by: persmule <persmule(a)hardenedlinux.org>
---
M ichspi.c
1 file changed, 1 insertion(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/95/84595/1
diff --git a/ichspi.c b/ichspi.c
index 358d9f4..d53e671 100644
--- a/ichspi.c
+++ b/ichspi.c
@@ -399,7 +399,7 @@
* It is used to reprogram the chipset OPCODE table on-the-fly if an opcode
* is needed which is currently not in the chipset OPCODE table
*/
-static OPCODE POSSIBLE_OPCODES[] = {
+static const OPCODE POSSIBLE_OPCODES[] = {
{JEDEC_BYTE_PROGRAM, SPI_OPCODE_TYPE_WRITE_WITH_ADDRESS, 0}, // Write Byte
{JEDEC_READ, SPI_OPCODE_TYPE_READ_WITH_ADDRESS, 0}, // Read Data
{JEDEC_BE_D8, SPI_OPCODE_TYPE_WRITE_WITH_ADDRESS, 0}, // Erase Sector
--
To view, visit https://review.coreboot.org/c/flashrom/+/84595?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: I217f8a9e50b9e2e9f2731adec89a46780874c754
Gerrit-Change-Number: 84595
Gerrit-PatchSet: 1
Gerrit-Owner: Bill XIE <persmule(a)hardenedlinux.org>
Attention is currently required from: Anastasia Klimchuk, Nikolai Artemiev, Peter Marheine.
Hello Anastasia Klimchuk, Nikolai Artemiev, Peter Marheine, build bot (Jenkins),
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/flashrom/+/84253?usp=email
to look at the new patch set (#4).
The following approvals got outdated and were removed:
Verified+1 by build bot (Jenkins)
Change subject: ichspi: Probe opcode in POSSIBLE_OPCODES[] as well
......................................................................
ichspi: Probe opcode in POSSIBLE_OPCODES[] as well
ich_spi_send_command() and ich_spi_send_multicommand() will overwrite
the "Sector erase" opcode with the opcode for command via
reprogram_opcode_on_the_fly(), but not restore it, causing the "Sector
erase" opcode may get lost after sending commands, leaving only "Bulk
erase" opcode which erase the whole chip available.
In the mean time, ich_spi_probe_opcode() used not to report opcodes in
POSSIBLE_OPCODES[] but not in curopcodes->opcode[] as supported.
Now, if the opcode being probed is not in curopcodes->opcode[] but in
POSSIBLE_OPCODES[], it will be programmed on-the fly, and reported as
supported if successfully programmed.
Fix:https://ticket.coreboot.org/issues/556
Change-Id: I3fc831fc072e2af9265835cb2f71bf8c222c6a64
Signed-off-by: persmule <persmule(a)hardenedlinux.org>
---
M ichspi.c
1 file changed, 8 insertions(+), 1 deletion(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/53/84253/4
--
To view, visit https://review.coreboot.org/c/flashrom/+/84253?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: main
Gerrit-Change-Id: I3fc831fc072e2af9265835cb2f71bf8c222c6a64
Gerrit-Change-Number: 84253
Gerrit-PatchSet: 4
Gerrit-Owner: Bill XIE <persmule(a)hardenedlinux.org>
Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Reviewer: Nikolai Artemiev <nartemiev(a)google.com>
Gerrit-Reviewer: Peter Marheine <pmarheine(a)chromium.org>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Attention: Nikolai Artemiev <nartemiev(a)google.com>
Gerrit-Attention: Peter Marheine <pmarheine(a)chromium.org>
Attention is currently required from: Anastasia Klimchuk, Nikolai Artemiev, Peter Marheine.
Bill XIE has posted comments on this change by Bill XIE. ( https://review.coreboot.org/c/flashrom/+/84253?usp=email )
Change subject: ichspi: Restore Sector erase opcode after send command
......................................................................
Patch Set 3:
(1 comment)
File ichspi.c:
https://review.coreboot.org/c/flashrom/+/84253/comment/c2458138_26d3c0f0?us… :
PS1, Line 1843:
: static bool ich_spi_probe_opcode(const struct flashctx *flash, uint8_t opcode)
: {
: return find_opcode(curopcodes, opcode) >= 0;
> > That way, ich_spi_send_command will automatically reprogram the required opcode if it is missing a […]
Please show me your elegant solution to program other erasing opcodes on the fly, but not before CB:84567 and CB:84593. ;-)
--
To view, visit https://review.coreboot.org/c/flashrom/+/84253?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: I3fc831fc072e2af9265835cb2f71bf8c222c6a64
Gerrit-Change-Number: 84253
Gerrit-PatchSet: 3
Gerrit-Owner: Bill XIE <persmule(a)hardenedlinux.org>
Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Reviewer: Nikolai Artemiev <nartemiev(a)google.com>
Gerrit-Reviewer: Peter Marheine <pmarheine(a)chromium.org>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Attention: Nikolai Artemiev <nartemiev(a)google.com>
Gerrit-Attention: Peter Marheine <pmarheine(a)chromium.org>
Gerrit-Comment-Date: Mon, 30 Sep 2024 08:16:17 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Bill XIE <persmule(a)hardenedlinux.org>
Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org>
Comment-In-Reply-To: Nikolai Artemiev <nartemiev(a)google.com>
Attention is currently required from: Anastasia Klimchuk, Nikolai Artemiev, Peter Marheine.
Bill XIE has posted comments on this change by Bill XIE. ( https://review.coreboot.org/c/flashrom/+/84253?usp=email )
Change subject: ichspi: Restore Sector erase opcode after send command
......................................................................
Patch Set 1:
(1 comment)
File ichspi.c:
https://review.coreboot.org/c/flashrom/+/84253/comment/c9fb1e05_31547cef?us… :
PS1, Line 2078: reg
However, is it okay to just use spi_master_ich9 here? (maybe after renaming it to "spi_master_ich79" or something, identical to use ich_spi_probe_opcode() for .probe_opcode() for ich7 as well)
--
To view, visit https://review.coreboot.org/c/flashrom/+/84253?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: I3fc831fc072e2af9265835cb2f71bf8c222c6a64
Gerrit-Change-Number: 84253
Gerrit-PatchSet: 1
Gerrit-Owner: Bill XIE <persmule(a)hardenedlinux.org>
Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Reviewer: Nikolai Artemiev <nartemiev(a)google.com>
Gerrit-Reviewer: Peter Marheine <pmarheine(a)chromium.org>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Attention: Nikolai Artemiev <nartemiev(a)google.com>
Gerrit-Attention: Peter Marheine <pmarheine(a)chromium.org>
Gerrit-Comment-Date: Mon, 30 Sep 2024 06:58:48 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Attention is currently required from: Anastasia Klimchuk, Nikolai Artemiev, Peter Marheine.
Bill XIE has posted comments on this change by Bill XIE. ( https://review.coreboot.org/c/flashrom/+/84253?usp=email )
Change subject: ichspi: Restore Sector erase opcode after send command
......................................................................
Patch Set 1:
(1 comment)
File ichspi.c:
https://review.coreboot.org/c/flashrom/+/84253/comment/6ddd8e0f_f7111307?us… :
PS1, Line 1843:
: static bool ich_spi_probe_opcode(const struct flashctx *flash, uint8_t opcode)
: {
: return find_opcode(curopcodes, opcode) >= 0;
> That way, ich_spi_send_command will automatically reprogram the required opcode if it is missing and we don't have to restore JEDEC_SE after every command that reprograms it.
I have once considered this, but the issue is that spi_master_ich7 does use ich_spi_send_(multi)command() but does not implement .probe_opcode().
> Bill, would you agree to change your patch, and test the different solution?
As a plan B I can upload new version of the patch, but asking you first because you are the author (and you did all the debugging).
Also, in any case, original author of the patch stays (even if someone uploads new version)
If you have a better solution, you are free to replace mine.
--
To view, visit https://review.coreboot.org/c/flashrom/+/84253?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: I3fc831fc072e2af9265835cb2f71bf8c222c6a64
Gerrit-Change-Number: 84253
Gerrit-PatchSet: 1
Gerrit-Owner: Bill XIE <persmule(a)hardenedlinux.org>
Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Reviewer: Nikolai Artemiev <nartemiev(a)google.com>
Gerrit-Reviewer: Peter Marheine <pmarheine(a)chromium.org>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Attention: Nikolai Artemiev <nartemiev(a)google.com>
Gerrit-Attention: Peter Marheine <pmarheine(a)chromium.org>
Gerrit-Comment-Date: Mon, 30 Sep 2024 06:49:18 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org>
Comment-In-Reply-To: Nikolai Artemiev <nartemiev(a)google.com>
Attention is currently required from: Aarya, Anastasia Klimchuk, Nikolai Artemiev.
Sergii Dmytruk has posted comments on this change by Sergii Dmytruk. ( https://review.coreboot.org/c/flashrom/+/84102?usp=email )
Change subject: Complete and fix progress feature implementation for all operations
......................................................................
Patch Set 5:
(4 comments)
File cli_output.c:
https://review.coreboot.org/c/flashrom/+/84102/comment/43390002_53a3d3a8?us… :
PS4, Line 119: 16 * FLASHROM_PROGRESS_NR
For 100% it will be 13 chars which I rounded to 16. `\b \b` is:
1. Move cursor to the left
2. Print space to overwrite what is under the cursor (or to its right depending on cursor's shape)
3. Move cursor to the left again
> Does this relate to the part in commit message:
No, that's a separate thing.
> But the concern is still valid right?
Yes, but I just saw that it's not a real problem. A flag indicating that something was printed can be set in `flashrom_print_cb()` below and reset after printing a progress here. If the flag is set when this function is called, need to move to a new line (ideally would know whether last print included a new line or not), otherwise can just erase previous progress.
File flashrom.c:
https://review.coreboot.org/c/flashrom/+/84102/comment/b195d54f_ff781022?us… :
PS5, Line 1235: /* const size_t flash_size = flashctx->chip->total_size * 1024; */
: /* const size_t page_size = flashctx->chip->page_size; */
> I think I can remove these lines
Sure.
https://review.coreboot.org/c/flashrom/+/84102/comment/8574887a_14b1a599?us… :
PS5, Line 1958: init_progress(flashctx, FLASHROM_PROGRESS_READ, flash_size); // WTF?
> What was your concern on this line? Was something not working? (see line comment)
I think it was before addressing progress reporting on SPI reads, so adding this line caused progress to reach 200%.
File include/flash.h:
https://review.coreboot.org/c/flashrom/+/84102/comment/02956254_20130dd4?us… :
PS1, Line 587: struct stage_progress stage_progress[FLASHROM_PROGRESS_NR];
> I think (I think) I understand. […]
From the client side, yes. Also, this change switches from programmers reporting total and current position to them reporting amount of processed data, so the total and current values need to be stored somewhere and this is where it's stored.
--
To view, visit https://review.coreboot.org/c/flashrom/+/84102?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: If1e40fc97f443c4f0c0501cef11cff1f3f84c051
Gerrit-Change-Number: 84102
Gerrit-PatchSet: 5
Gerrit-Owner: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com>
Gerrit-Reviewer: Aarya <aarya.chaumal(a)gmail.com>
Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Reviewer: Nikolai Artemiev <nartemiev(a)google.com>
Gerrit-Reviewer: Peter Marheine <pmarheine(a)chromium.org>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Aarya <aarya.chaumal(a)gmail.com>
Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Attention: Nikolai Artemiev <nartemiev(a)google.com>
Gerrit-Comment-Date: Sun, 29 Sep 2024 12:07:49 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org>
Comment-In-Reply-To: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com>
Attention is currently required from: Aarya, Nikolai Artemiev, Sergii Dmytruk.
Anastasia Klimchuk has posted comments on this change by Sergii Dmytruk. ( https://review.coreboot.org/c/flashrom/+/84102?usp=email )
Change subject: Complete and fix progress feature implementation for all operations
......................................................................
Patch Set 5:
(2 comments)
File flashrom.c:
https://review.coreboot.org/c/flashrom/+/84102/comment/f2cbbbc6_b75dec64?us… :
PS5, Line 1235: /* const size_t flash_size = flashctx->chip->total_size * 1024; */
: /* const size_t page_size = flashctx->chip->page_size; */
I think I can remove these lines
https://review.coreboot.org/c/flashrom/+/84102/comment/d1d7406c_05e12624?us… :
PS5, Line 1958: init_progress(flashctx, FLASHROM_PROGRESS_READ, flash_size); // WTF?
What was your concern on this line? Was something not working? (see line comment)
--
To view, visit https://review.coreboot.org/c/flashrom/+/84102?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: If1e40fc97f443c4f0c0501cef11cff1f3f84c051
Gerrit-Change-Number: 84102
Gerrit-PatchSet: 5
Gerrit-Owner: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com>
Gerrit-Reviewer: Aarya <aarya.chaumal(a)gmail.com>
Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Reviewer: Nikolai Artemiev <nartemiev(a)google.com>
Gerrit-Reviewer: Peter Marheine <pmarheine(a)chromium.org>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Aarya <aarya.chaumal(a)gmail.com>
Gerrit-Attention: Nikolai Artemiev <nartemiev(a)google.com>
Gerrit-Attention: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com>
Gerrit-Comment-Date: Sun, 29 Sep 2024 07:29:51 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No