October 2024 - flashrom-gerrit

[XS] Change in flashrom[main]: ichspi: Change the opcode position for reprogramming on the fly 2->4
by Bill XIE (Code Review) Oct. 7, 2024

Oct. 7, 2024

Attention is currently required from: Anastasia Klimchuk. Bill XIE has posted comments on this change by Anastasia Klimchuk. ( https://review.coreboot.org/c/flashrom/+/84567?usp=email ) Change subject: ichspi: Change the opcode position for reprogramming on the fly 2->4 ...................................................................... Patch Set 2: (1 comment) Patchset: PS1: > Needs to be tested Done -- To view, visit https://review.coreboot.org/c/flashrom/+/84567?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: I6bc855daedf0af2e8de191f23a3512de3ebc3fef Gerrit-Change-Number: 84567 Gerrit-PatchSet: 2 Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Bill XIE <persmule(a)hardenedlinux.org> Gerrit-Reviewer: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Comment-Date: Mon, 07 Oct 2024 12:42:56 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org>

1 0

[XS] Change in flashrom[main]: ichspi: Change the opcode position for reprogramming on the fly 2->4
by Bill XIE (Code Review) Oct. 7, 2024

Oct. 7, 2024

Attention is currently required from: Anastasia Klimchuk. Bill XIE has posted comments on this change by Anastasia Klimchuk. ( https://review.coreboot.org/c/flashrom/+/84567?usp=email ) Change subject: ichspi: Change the opcode position for reprogramming on the fly 2->4 ...................................................................... Patch Set 2: (1 comment) Patchset: PS2: > Oh you put my patch first in the chain, this is so nice thank you! :) […] Yes. From my point of view, the whole chain works fine. -- To view, visit https://review.coreboot.org/c/flashrom/+/84567?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: I6bc855daedf0af2e8de191f23a3512de3ebc3fef Gerrit-Change-Number: 84567 Gerrit-PatchSet: 2 Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Bill XIE <persmule(a)hardenedlinux.org> Gerrit-Reviewer: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Comment-Date: Mon, 07 Oct 2024 12:42:40 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org>

1 0

[XS] Change in flashrom[main]: ichspi: Probe opcode in POSSIBLE_OPCODES[] as well
by Bill XIE (Code Review) Oct. 7, 2024

Oct. 7, 2024

Attention is currently required from: 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 5: (1 comment) File ichspi.c: https://review.coreboot.org/c/flashrom/+/84253/comment/d20e0c78_379f8b03?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 C […] Yes. From my point of view, the whole chain works fine. -- 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: 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: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Attention: Peter Marheine <pmarheine(a)chromium.org> Gerrit-Comment-Date: Mon, 07 Oct 2024 12:42:33 +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>

1 0

[XS] Change in flashrom[main]: erasure_layout: Erase larger block only when all sub-block need erase
by Anastasia Klimchuk (Code Review) Oct. 7, 2024

Oct. 7, 2024

Attention is currently required from: Aarya, Bill XIE. Anastasia Klimchuk has posted comments on this change by Bill XIE. ( https://review.coreboot.org/c/flashrom/+/84614?usp=email ) Change subject: erasure_layout: Erase larger block only when all sub-block need erase ...................................................................... Patch Set 1: (1 comment) Patchset: PS1: > Bill, thank you so much for looking into the details of the erasure logic! And the patch with the te […] I made a patch for p.2 CB:84686 -- To view, visit https://review.coreboot.org/c/flashrom/+/84614?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: I9e10749186e395da67ec80e296119f33c3f83122 Gerrit-Change-Number: 84614 Gerrit-PatchSet: 1 Gerrit-Owner: Bill XIE <persmule(a)hardenedlinux.org> Gerrit-Reviewer: Aarya <aarya.chaumal(a)gmail.com> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Bill XIE <persmule(a)hardenedlinux.org> Gerrit-Attention: Aarya <aarya.chaumal(a)gmail.com> Gerrit-Comment-Date: Mon, 07 Oct 2024 07:48:15 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org>

1 0

[S] Change in flashrom[main]: erase/write: Deselect all smaller blocks when large block is selected
by Anastasia Klimchuk (Code Review) Oct. 7, 2024

Oct. 7, 2024

Attention is currently required from: Aarya, Bill XIE. Anastasia Klimchuk has posted comments on this change by Anastasia Klimchuk. ( https://review.coreboot.org/c/flashrom/+/84686?usp=email ) Change subject: erase/write: Deselect all smaller blocks when large block is selected ...................................................................... Patch Set 1: (1 comment) File erasure_layout.c: https://review.coreboot.org/c/flashrom/+/84686/comment/7b1c80a5_34ed9df3?us… : PS1, Line 237: /* Deselect all smaller blocks covering the same region. */ I still need to add a test for this in `tests/erase_func_algo.c` -- To view, visit https://review.coreboot.org/c/flashrom/+/84686?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: Icfc18d5c090b1dcb92ab157e2c139be71af59300 Gerrit-Change-Number: 84686 Gerrit-PatchSet: 1 Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Aarya <aarya.chaumal(a)gmail.com> Gerrit-Reviewer: Bill XIE <persmule(a)hardenedlinux.org> Gerrit-Attention: Bill XIE <persmule(a)hardenedlinux.org> Gerrit-Attention: Aarya <aarya.chaumal(a)gmail.com> Gerrit-Comment-Date: Mon, 07 Oct 2024 07:45:27 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No

1 0

[S] Change in flashrom[main]: erase/write: Deselect all smaller blocks when large block is selected
by Anastasia Klimchuk (Code Review) Oct. 7, 2024

Oct. 7, 2024

Attention is currently required from: Aarya, Bill XIE. Anastasia Klimchuk has posted comments on this change by Anastasia Klimchuk. ( https://review.coreboot.org/c/flashrom/+/84686?usp=email ) Change subject: erase/write: Deselect all smaller blocks when large block is selected ...................................................................... Patch Set 1: (2 comments) Patchset: PS1: Run the test scenario from CB:84614 and that's the result: double-erase seems gone Reading old flash chip contents... read_flash: region (00000000..0xffffff) is readable, reading range (00000000..0xffffff). done. erase_write: region (00000000..0xffffff) is writable, erasing range (00000000..0xffffff). 000000..0xffffff verify_range: Verifying region (00000000..0xffffff) read_flash: region (00000000..0xffffff) is readable, reading range (00000000..0xffffff). E(0:ffffff)write_flash: region (00000000..0xffffff) is writable, writing range (00000000..0xffffff). W(0:ffffff)Erase/write done from 0 to ffffff Verifying flash... read_flash: region (00000000..0xffffff) is readable, reading range (00000000..0xffffff). VERIFIED. Writing chip.rom File erasure_layout.c: https://review.coreboot.org/c/flashrom/+/84686/comment/127814fe_df505a33?us… : PS1, Line 242: while (true) I want to re-write this as recursive fn `deselect_erase_functions` -- To view, visit https://review.coreboot.org/c/flashrom/+/84686?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: Icfc18d5c090b1dcb92ab157e2c139be71af59300 Gerrit-Change-Number: 84686 Gerrit-PatchSet: 1 Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Aarya <aarya.chaumal(a)gmail.com> Gerrit-Reviewer: Bill XIE <persmule(a)hardenedlinux.org> Gerrit-Attention: Bill XIE <persmule(a)hardenedlinux.org> Gerrit-Attention: Aarya <aarya.chaumal(a)gmail.com> Gerrit-Comment-Date: Mon, 07 Oct 2024 07:44:17 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No

1 0

[S] Change in flashrom[main]: erase/write: Deselect all smaller blocks when large block is selected
by Anastasia Klimchuk (Code Review) Oct. 7, 2024

Oct. 7, 2024

Anastasia Klimchuk has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/84686?usp=email ) Change subject: erase/write: Deselect all smaller blocks when large block is selected ...................................................................... erase/write: Deselect all smaller blocks when large block is selected Previously the logic which selected large block did deselect of smaller blocks, but only one level below. So some even smaller blocks could still remain selected, and this would result in duplicate erase. Change-Id: Icfc18d5c090b1dcb92ab157e2c139be71af59300 Spotted-by: persmule <persmule(a)hardenedlinux.org> Signed-off-by: Anastasia Klimchuk <aklm(a)flashrom.org> --- M erasure_layout.c 1 file changed, 22 insertions(+), 2 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/86/84686/1 diff --git a/erasure_layout.c b/erasure_layout.c index c3a415b..ad333eb 100644 --- a/erasure_layout.c +++ b/erasure_layout.c @@ -229,9 +229,29 @@ const int total_blocks = sub_block_end - sub_block_start + 1; if (count && count > total_blocks/2) { + /* More than a half of the region is covered by smaller blocks already. + We are selecting one large block instead, to send opcode once + instead of sending many smaller once. */ if (ll->start_addr >= rstart && ll->end_addr <= rend) { - for (int j = sub_block_start; j <= sub_block_end; j++) - layout[findex - 1].layout_list[j].selected = false; + + /* Deselect all smaller blocks covering the same region. */ + size_t index_to_deselect = findex - 1; // represents size of the block + int block_start_to_deselect = sub_block_start; + int block_end_to_deselect = sub_block_end; + + while (true) { + for (int j = block_start_to_deselect; j <= block_end_to_deselect; j++) + layout[index_to_deselect].layout_list[j].selected = false; + + block_start_to_deselect = layout[index_to_deselect].layout_list[block_start_to_deselect].first_sub_block_index; + block_end_to_deselect = layout[index_to_deselect].layout_list[block_end_to_deselect].last_sub_block_index; + if (index_to_deselect) + index_to_deselect--; + else + break; // index_to_deselect has already reached 0, the smallest size of block. we are done. + } + + /* Select large block. */ ll->selected = true; } } -- To view, visit https://review.coreboot.org/c/flashrom/+/84686?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: Icfc18d5c090b1dcb92ab157e2c139be71af59300 Gerrit-Change-Number: 84686 Gerrit-PatchSet: 1 Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org>

1 0

[L] Change in flashrom[main]: Complete and fix progress feature implementation for all operations
by Sergii Dmytruk (Code Review) Oct. 6, 2024

Oct. 6, 2024

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 7: (1 comment) Patchset: PS7: > Sergii, maybe you could have a look? Especially at the code which decides whether to replace previou […] Oh, and I don't think this needs to be tied to CB:84439. -- 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: 7 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, 06 Oct 2024 17:07:09 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org>

1 0

[L] Change in flashrom[main]: Complete and fix progress feature implementation for all operations
by Sergii Dmytruk (Code Review) Oct. 6, 2024

Oct. 6, 2024

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 7: (8 comments) Commit Message: https://review.coreboot.org/c/flashrom/+/84102/comment/8db69e36_d01915c2?us… : PS7, Line 42: The progress doesn't just dump lots of stuff on the screen which : probably won't fly because of CB:64668, but it's not hard to adjust : this. ```suggestion Similar to CB:64668, an effort is made to keep the progress on a single line. Non-progress output is kept track of to know when moving to a new line cannot be avoided. ``` File cli_output.c: https://review.coreboot.org/c/flashrom/+/84102/comment/a76f2895_9a4d0ebd?us… : PS4, Line 119: 16 * FLASHROM_PROGRESS_NR > Oh this is a backspace that is not deleting characters! Sorry I got so confused. […] Just saw that "..." is also printed below which needs to be erased as well. File cli_output.c: https://review.coreboot.org/c/flashrom/+/84102/comment/d5fabcff_c745576a?us… : PS7, Line 157: progress_printed = false; Those extra newlines are quite annoying. I think most of them can be deal with quite easily. The flag could be an enumeration: - progress - newline - midline This simple check should address absolute majority of prints: ```c last_line = (fmt[strlen(fmt) - 1] == '\n' ? NEWLINE : MIDLINE); ``` Then both `msg_ginfo("\n");` above can be skipped if we know we're at the new line to turn ``` flashrom 1.5.0-devel (git:v1.4.0-57-g83954aa8) on Linux 6.10.11 (x86_64) flashrom is free software, get the source code at https://flashrom.org Found Winbond flash chip "W25Q128.V" (16384 kB, SPI) on dummy. [READ: 100%][ERASE: 100%]...Erase/write done from 0 to ffffff ``` into ``` flashrom 1.5.0-devel (git:v1.4.0-57-g83954aa8) on Linux 6.10.11 (x86_64) flashrom is free software, get the source code at https://flashrom.org Found Winbond flash chip "W25Q128.V" (16384 kB, SPI) on dummy. [READ: 100%][ERASE: 100%]...Erase/write done from 0 to ffffff ``` File erasure_layout.c: https://review.coreboot.org/c/flashrom/+/84102/comment/ef44a75f_2c8b2286?us… : PS7, Line 275: msg_cerr("ERASE FAILED!\n"); Not added here, but this line is unreachable. File libflashrom.c: https://review.coreboot.org/c/flashrom/+/84102/comment/7ae97ec7_a6be4eed?us… : PS7, Line 82: update_progress(flashctx, stage, 0); Probably worth commenting that this is used to trigger callback invocation. File realtek_mst_i2c_spi.c: https://review.coreboot.org/c/flashrom/+/84102/comment/6b5b0fe2_0f696d61?us… : PS7, Line 400: RTK_PAGE_SIZE Looks like this should actually be `page_len`. File spi25.c: https://review.coreboot.org/c/flashrom/+/84102/comment/57f2adcc_0b5ced68?us… : PS7, Line 733: chunksize This should be `towrite`. File tests/chip.c: https://review.coreboot.org/c/flashrom/+/84102/comment/faa1aa14_d02457cd?us… : PS7, Line 84: } Doesn't look like this can catch progress going backward. If you want to check for that, this could verify that stage progress is either reset to 0 or increases while total amount changes only when current value is reset. Needs creating a state in tests to keep track of stages, basically what cli code does. -- 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: 7 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, 06 Oct 2024 17:06:04 +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>

1 0

[S] Change in flashrom[main]: doc/contact: Add note to IRC section and calm down the formatting
by Stefan Reinauer (Code Review) Oct. 6, 2024

Oct. 6, 2024

Attention is currently required from: Anastasia Klimchuk. Stefan Reinauer has posted comments on this change by Anastasia Klimchuk. ( https://review.coreboot.org/c/flashrom/+/84679?usp=email ) Change subject: doc/contact: Add note to IRC section and calm down the formatting ...................................................................... Patch Set 1: Code-Review+2 -- To view, visit https://review.coreboot.org/c/flashrom/+/84679?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: Ic808508b5da431d6c0b88a9b2847c34c7b02cfe0 Gerrit-Change-Number: 84679 Gerrit-PatchSet: 1 Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Stefan Reinauer <stefan.reinauer(a)coreboot.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Comment-Date: Sun, 06 Oct 2024 16:57:19 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes

1 0