October 2024 - flashrom-gerrit

[M] Change in flashrom[main]: erasure_layout: Add an option to sacrifice unchanged blocks for speed
by Peter Marheine (Code Review) Oct. 27, 2024

Oct. 27, 2024

Attention is currently required from: Aarya, Anastasia Klimchuk, Bill XIE. Peter Marheine has posted comments on this change by Bill XIE. ( https://review.coreboot.org/c/flashrom/+/84721?usp=email ) Change subject: erasure_layout: Add an option to sacrifice unchanged blocks for speed ...................................................................... Patch Set 8: (3 comments) File cli_classic.c: https://review.coreboot.org/c/flashrom/+/84721/comment/59675f49_9e423cad?us… : PS8, Line 124: " --sacrifice-ratio <ratio> Maximum %% memory that is allowed for redundant\n" This is understandable, but reads somewhat awkwardly to me. Perhaps: > Fraction (as a percentage, 0-50) of an erase block that may be erased even if unmodified. Larger values may complete programming faster, but may also hurt chip longevity by erasing cells unnecessarily. Default 0. File erasure_layout.c: https://review.coreboot.org/c/flashrom/+/84721/comment/c2b42544_bc111060?us… : PS8, Line 253: needless needing? File tests/erase_func_algo.c: https://review.coreboot.org/c/flashrom/+/84721/comment/73021652_bf21c555?us… : PS8, Line 1235: } : }, : sizeof(*all_cases) : ); : memcpy( : &all_cases[num_parameterized * 2 + 1], : (const struct CMUnitTest[]){ : (const struct CMUnitTest) { : .name = "write with sacrifice ratio 50", : .test_func = test_write_with_sacrifice_ratio50, : } : }, : sizeof(*all_cases) : ); The previous `memcpy` is already set up to copy an array of structs, so you can inline the new struct there. ```suggestion }, (const struct CMUnitTest) { .name = "write with sacrifice ratio 50", .test_func = test_write_with_sacrifice_ratio50, }, }, sizeof(*all_cases) * num_unparameterized ); ``` -- To view, visit https://review.coreboot.org/c/flashrom/+/84721?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: I154e8a713f626c37dbbe118db700055b96d24803 Gerrit-Change-Number: 84721 Gerrit-PatchSet: 8 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: Peter Marheine <pmarheine(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-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Comment-Date: Sun, 27 Oct 2024 23:27:31 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No

1 0

[L] Change in flashrom[main]: cli_classic: Add rpmc command support
by Peter Marheine (Code Review) Oct. 27, 2024

Oct. 27, 2024

Attention is currently required from: Anastasia Klimchuk, Matti Finder. Peter Marheine has posted comments on this change by Matti Finder. ( https://review.coreboot.org/c/flashrom/+/84840?usp=email ) Change subject: cli_classic: Add rpmc command support ...................................................................... Patch Set 3: (2 comments) File cli_classic.c: https://review.coreboot.org/c/flashrom/+/84840/comment/b754461a_59e9f84e?us… : PS2, Line 159: printf(".\n\nYou can specify one of -h, -R, -L, " > Do you think the way I extended it is ok. Otherwise I think the message would get too long. […] Yeah, that seems good. It's clear from how we put those in their own section what options the RPMC commands are. File cli_classic.c: https://review.coreboot.org/c/flashrom/+/84840/comment/4843cd38_e29f3eb8?us… : PS3, Line 165: ", the RPMC commands" "a RPMC command" since specifying more than one in a single invocation is an error. -- To view, visit https://review.coreboot.org/c/flashrom/+/84840?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: I36c823bbee65f256eb6edabe6f058321c9a0cfa1 Gerrit-Change-Number: 84840 Gerrit-PatchSet: 3 Gerrit-Owner: Matti Finder <matti.finder(a)gmail.com> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> 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: Matti Finder <matti.finder(a)gmail.com> Gerrit-Comment-Date: Sun, 27 Oct 2024 22:46:42 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Matti Finder <matti.finder(a)gmail.com> Comment-In-Reply-To: Peter Marheine <pmarheine(a)chromium.org>

1 0

[S] Change in flashrom[main]: doc: Add chip models support into recent development
by Peter Marheine (Code Review) Oct. 27, 2024

Oct. 27, 2024

Attention is currently required from: Anastasia Klimchuk. Peter Marheine has posted comments on this change by Anastasia Klimchuk. ( https://review.coreboot.org/c/flashrom/+/84870?usp=email ) Change subject: doc: Add chip models support into recent development ...................................................................... Patch Set 1: Code-Review+2 -- To view, visit https://review.coreboot.org/c/flashrom/+/84870?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: I9b96902a02b83d35f0a0f221bd1678b7edf99dad Gerrit-Change-Number: 84870 Gerrit-PatchSet: 1 Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org> 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-Comment-Date: Sun, 27 Oct 2024 22:42:56 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes

1 0

[M] Change in flashrom[main]: erasure_layout: Add an option to sacrifice unchanged blocks for speed
by Anastasia Klimchuk (Code Review) Oct. 27, 2024

Oct. 27, 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/+/84721?usp=email ) Change subject: erasure_layout: Add an option to sacrifice unchanged blocks for speed ...................................................................... Patch Set 8: (1 comment) Patchset: PS8: I rebased and merged the conflict in flash.h (because another patch was submitted earlier today). Also ran the test scenario from CB:84614 Default case, without providing sacrifice ration option, erases in many small blocks. With `--sacrifice-ratio=50` added to the last command, erase is happening once, for the whole chip. So, it's working as expected. -- To view, visit https://review.coreboot.org/c/flashrom/+/84721?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: I154e8a713f626c37dbbe118db700055b96d24803 Gerrit-Change-Number: 84721 Gerrit-PatchSet: 8 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: Sun, 27 Oct 2024 11:44:12 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No

1 0

[M] Change in flashrom[main]: erasure_layout: Add an option to sacrifice unchanged blocks for speed
by Anastasia Klimchuk (Code Review) Oct. 27, 2024

Oct. 27, 2024

Attention is currently required from: Aarya, Anastasia Klimchuk. Anastasia Klimchuk has uploaded a new patch set (#8) to the change originally created by Bill XIE. ( https://review.coreboot.org/c/flashrom/+/84721?usp=email ) The following approvals got outdated and were removed: Verified+1 by build bot (Jenkins) Change subject: erasure_layout: Add an option to sacrifice unchanged blocks for speed ...................................................................... erasure_layout: Add an option to sacrifice unchanged blocks for speed The patch adds command line option to handle the following situation: There is a region which is requested to be erased (or written, because the write operation uses erase too). Some of the areas inside this region don't need to be erased, because the bytes already have expected value. Such areas can be skipped. The logic selects eraseblocks that can cover the areas which need to be erased. Suppose there is a region which is partially covered by eraseblocks of size S (partially because remaining areas don't need to be erased). Now suppose we can cover the whole region with eraseblock of larger size, S+1, and erase it all at once. This will run faster: erase opcode will only be sent once instead of many smaller opcodes. However, this will run erase over some areas of the chip memory that didn't need to be erased. Which means, the chip, as a hardware, will wear faster. New command line option sets the maximum % memory that is allowed for redundant erase. Default is 0, S+1 size block only selected if all the area needs to be erased in full. 50 means that if more than a half of the area needs to be erased, a S+1 size block can be selected to cover all area with one block. The tradeoff is the speed of programming operation VS the longevity of the chip. Default is longevity. Change-Id: I154e8a713f626c37dbbe118db700055b96d24803 Co-developed-by: persmule <persmule(a)hardenedlinux.org Co-developed-by: Anastasia Klimchuk <aklm(a)flashrom.org> Signed-off-by: persmule <persmule(a)hardenedlinux.org> Signed-off-by: Anastasia Klimchuk <aklm(a)flashrom.org> --- M cli_classic.c M erasure_layout.c M include/flash.h M tests/erase_func_algo.c 4 files changed, 119 insertions(+), 5 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/21/84721/8 -- To view, visit https://review.coreboot.org/c/flashrom/+/84721?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: I154e8a713f626c37dbbe118db700055b96d24803 Gerrit-Change-Number: 84721 Gerrit-PatchSet: 8 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: Aarya <aarya.chaumal(a)gmail.com> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org>

1 0

[XS] Change in flashrom[main]: erasure_layout: Fix unreachable error message
by Anastasia Klimchuk (Code Review) Oct. 27, 2024

Oct. 27, 2024

Anastasia Klimchuk has submitted this change. ( https://review.coreboot.org/c/flashrom/+/84725?usp=email ) ( 3 is the latest approved patch-set. No files were changed between the latest approved patch-set and the submitted one. )Change subject: erasure_layout: Fix unreachable error message ...................................................................... erasure_layout: Fix unreachable error message Change-Id: I11d2d1359f74475cb20a1c91bddb380b4952a704 Spotted-by: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com> Signed-off-by: Anastasia Klimchuk <aklm(a)flashrom.org> Reviewed-on: https://review.coreboot.org/c/flashrom/+/84725 Reviewed-by: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com> Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org> --- M erasure_layout.c 1 file changed, 1 insertion(+), 1 deletion(-) Approvals: build bot (Jenkins): Verified Sergii Dmytruk: Looks good to me, approved diff --git a/erasure_layout.c b/erasure_layout.c index 8b1a26f..55da163 100644 --- a/erasure_layout.c +++ b/erasure_layout.c @@ -300,8 +300,8 @@ return -1; } if (check_erased_range(flashctx, start_addr, block_len)) { - return -1; msg_cerr("ERASE FAILED!\n"); + return -1; } update_progress(flashctx, FLASHROM_PROGRESS_ERASE, block_len); -- To view, visit https://review.coreboot.org/c/flashrom/+/84725?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email Gerrit-MessageType: merged Gerrit-Project: flashrom Gerrit-Branch: main Gerrit-Change-Id: I11d2d1359f74475cb20a1c91bddb380b4952a704 Gerrit-Change-Number: 84725 Gerrit-PatchSet: 10 Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Aarya <aarya.chaumal(a)gmail.com> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>

1 0

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

Oct. 27, 2024

Anastasia Klimchuk has submitted this change. ( https://review.coreboot.org/c/flashrom/+/84102?usp=email ) Change subject: Complete and fix progress feature implementation for all operations ...................................................................... Complete and fix progress feature implementation for all operations Original progress reporting implemented in CB:49643 and it has some issues, for example: size_t start_address = start; size_t end_address = len - start; End address is anything but length minus start address. update_progress(flash, FLASHROM_PROGRESS_READ, /*current*/ start - start_address + to_read, /*total*/ end_address); Total should just be length if that's how current value is computed. --- libflashrom needs to know total size ahead of time. That's init_progress() and changed update_progress(). It also needs to store the last current value to be able to update it. That's stage_progress in flashrom_flashctx. Measuring accurately amount of data which will be read/erased/written isn't easy because things can be skipped as optimizations. The next patch in the chain aims to address this, there are TODO/FIXME comments there. --- CLI shares terminal with the rest of the code and has to maintain more state to handle that reasonably well. 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. --- A script to test the CLI: \#!/bin/bash t=${1:-rewW} shift if [[ $t =~ r ]]; then echo ">>> READ" ./flashrom -p dummy:emulate=W25Q128FV,freq=64mhz -r dump.rom --progress "$@" echo fi if [[ $t =~ e ]]; then echo ">>> ERASE" ./flashrom -p dummy:emulate=W25Q128FV,freq=64mhz -E --progress "$@" echo fi if [[ $t =~ w ]]; then echo ">>> WRITE (without erase)" dd if=/dev/zero of=zero.rom bs=1M count=16 2> /dev/null ./flashrom -p dummy:emulate=W25Q128FV,freq=64mhz -w zero.rom --progress "$@" echo fi if [[ $t =~ W ]]; then echo ">>> WRITE (with erase)" dd if=/dev/zero of=zero.rom bs=1M count=16 2> /dev/null dd if=/dev/random of=random.rom bs=1M count=16 2> /dev/null ./flashrom -p dummy:emulate=W25Q128FV,freq=64mhz,image=random.rom -w zero.rom --progress "$@" echo fi Co-developed-by: Anastasia Klimchuk <aklm(a)flashrom.org> Co-developed-by: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com> Change-Id: If1e40fc97f443c4f0c0501cef11cff1f3f84c051 Signed-off-by: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com> Signed-off-by: Anastasia Klimchuk <aklm(a)flashrom.org> Reviewed-on: https://review.coreboot.org/c/flashrom/+/84102 Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org> Reviewed-by: Anastasia Klimchuk <aklm(a)chromium.org> --- M 82802ab.c M at45db.c M cli_classic.c M cli_output.c M dediprog.c M doc/release_notes/devel.rst M en29lv640b.c M erasure_layout.c M flashrom.c M include/flash.h M it87spi.c M jedec.c M libflashrom.c M linux_mtd.c M parade_lspcon.c M realtek_mst_i2c_spi.c M spi.c M spi25.c M sst28sf040.c M tests/chip.c M tests/spi25.c M tests/tests.c M tests/tests.h 23 files changed, 333 insertions(+), 51 deletions(-) Approvals: Anastasia Klimchuk: Looks good to me, approved Sergii Dmytruk: Looks good to me, approved build bot (Jenkins): Verified diff --git a/82802ab.c b/82802ab.c index a440bd4..148042c 100644 --- a/82802ab.c +++ b/82802ab.c @@ -135,7 +135,7 @@ chip_writeb(flash, 0x40, dst); chip_writeb(flash, *src++, dst++); wait_82802ab(flash); - update_progress(flash, FLASHROM_PROGRESS_WRITE, i + 1, len); + update_progress(flash, FLASHROM_PROGRESS_WRITE, 1); } /* FIXME: Ignore errors for now. */ diff --git a/at45db.c b/at45db.c index 6d66a6e..c0c6eaf 100644 --- a/at45db.c +++ b/at45db.c @@ -551,7 +551,7 @@ msg_cerr("Writing page %u failed!\n", i); return 1; } - update_progress(flash, FLASHROM_PROGRESS_WRITE, i + page_size, len); + update_progress(flash, FLASHROM_PROGRESS_WRITE, page_size); } return 0; } diff --git a/cli_classic.c b/cli_classic.c index 3343438..3e9db50 100644 --- a/cli_classic.c +++ b/cli_classic.c @@ -1060,9 +1060,9 @@ fill_flash = &flashes[0]; - unsigned int progress_user_data[FLASHROM_PROGRESS_NR]; + struct cli_progress cli_progress = {0}; struct flashrom_progress progress_state = { - .user_data = progress_user_data + .user_data = &cli_progress }; if (options.show_progress) flashrom_set_progress_callback(fill_flash, &flashrom_progress_cb, &progress_state); diff --git a/cli_output.c b/cli_output.c index e5b829a..20295b8 100644 --- a/cli_output.c +++ b/cli_output.c @@ -24,6 +24,14 @@ enum flashrom_log_level verbose_screen = FLASHROM_MSG_INFO; enum flashrom_log_level verbose_logfile = FLASHROM_MSG_DEBUG2; +/* Enum to indicate what was the latest printed char prior to a progress indicator. */ +enum line_state { + NEWLINE, + MIDLINE, + PROGRESS +}; +static enum line_state line_state = NEWLINE; + static FILE *logfile = NULL; int close_logfile(void) @@ -75,18 +83,75 @@ return "UNKNOWN"; } +static void print_progress(const struct cli_progress *cli_progress, enum flashrom_progress_stage stage) +{ + if (!(cli_progress->visible_stages & (1 << stage))) + return; + + msg_ginfo("[%s: %2u%%]", flashrom_progress_stage_to_string(stage), cli_progress->stage_pc[stage]); +} + void flashrom_progress_cb(struct flashrom_flashctx *flashctx) { struct flashrom_progress *progress_state = flashctx->progress_state; unsigned int pc = 0; - unsigned int *percentages = progress_state->user_data; - if (progress_state->current > 0 && progress_state->total > 0) - pc = ((unsigned long long) progress_state->current * 10000llu) / - ((unsigned long long) progress_state->total * 100llu); - if (percentages[progress_state->stage] != pc) { - percentages[progress_state->stage] = pc; - msg_ginfo("[%s] %u%% complete... ", flashrom_progress_stage_to_string(progress_state->stage), pc); + struct cli_progress *cli_progress = progress_state->user_data; + + /* The expectation is that initial progress of zero is reported before doing anything. */ + if (progress_state->current == 0) { + if (!cli_progress->stage_setup) { + cli_progress->stage_setup = true; + + /* Initialization of some stage doesn't imply that it will make any progress, + * only show stages which have progressed. */ + cli_progress->visible_stages = 0; + + if (line_state != NEWLINE) { + /* We're going to clear and replace ongoing progress output, so make a new line. */ + msg_ginfo("\n"); + } + } + + cli_progress->stage_pc[progress_state->stage] = 0; + } else { + cli_progress->stage_setup = false; + cli_progress->visible_stages |= 1 << progress_state->stage; } + + if (progress_state->current > 0 && progress_state->total > 0) + pc = ((unsigned long long) progress_state->current * 100llu) / + ((unsigned long long) progress_state->total); + if (cli_progress->stage_pc[progress_state->stage] != pc) { + cli_progress->stage_pc[progress_state->stage] = pc; + + if (line_state == PROGRESS) { + /* Erase previous output, because it was previous progress step. */ + int i; + for (i = 0; i < 16 * FLASHROM_PROGRESS_NR; ++i) + msg_ginfo("\b \b"); + } else if (line_state == MIDLINE) { + /* Start with new line, to preserve some other previous message */ + msg_ginfo("\n"); + } // Remaining option is NEWLINE, which means nothing to do: newline has been printed already. + + /* The order is deliberate, the operations typically follow this sequence. */ + print_progress(cli_progress, FLASHROM_PROGRESS_READ); + print_progress(cli_progress, FLASHROM_PROGRESS_ERASE); + print_progress(cli_progress, FLASHROM_PROGRESS_WRITE); + + /* There can be output right after the progress, this acts as a separator. */ + msg_ginfo("..."); + + /* Reset the flag, because now the latest message is a progress one. */ + line_state = PROGRESS; + } +} + +static void update_line_state(const char *fmt) +{ + size_t len = strlen(fmt); + if (len > 0) + line_state = (fmt[len - 1] == '\n' ? NEWLINE : MIDLINE); } /* Please note that level is the verbosity, not the importance of the message. */ @@ -103,6 +168,7 @@ if (level <= verbose_screen) { ret = vfprintf(output_type, fmt, ap); + update_line_state(fmt); /* msg_*spew often happens inside chip accessors in possibly * time-critical operations. Don't slow them down by flushing. */ if (level != FLASHROM_MSG_SPEW) @@ -111,6 +177,7 @@ if ((level <= verbose_logfile) && logfile) { ret = vfprintf(logfile, fmt, logfile_args); + update_line_state(fmt); if (level != FLASHROM_MSG_SPEW) fflush(logfile); } diff --git a/dediprog.c b/dediprog.c index aa3a1cf..ba1f9d6 100644 --- a/dediprog.c +++ b/dediprog.c @@ -658,7 +658,7 @@ msg_perr("SPI bulk write failed, expected %i, got %s!\n", 512, libusb_error_name(ret)); return 1; } - update_progress(flash, FLASHROM_PROGRESS_WRITE, i + 1, count); + update_progress(flash, FLASHROM_PROGRESS_WRITE, chunksize); } return 0; diff --git a/doc/release_notes/devel.rst b/doc/release_notes/devel.rst index 919d364..c542bea 100644 --- a/doc/release_notes/devel.rst +++ b/doc/release_notes/devel.rst @@ -32,6 +32,11 @@ it. For those platforms don't support ECAM, libpci will terminate the process by exit. +Progress display +================ + +Progress display feature is now working for all operations: read, erase, write. + Chipset support =============== Added Raptor Point PCH support. diff --git a/en29lv640b.c b/en29lv640b.c index 4d93844..b647392 100644 --- a/en29lv640b.c +++ b/en29lv640b.c @@ -48,7 +48,7 @@ #endif dst += 2; src += 2; - update_progress(flash, FLASHROM_PROGRESS_WRITE, i + 2, len); + update_progress(flash, FLASHROM_PROGRESS_WRITE, 2); } /* FIXME: Ignore errors for now. */ diff --git a/erasure_layout.c b/erasure_layout.c index 7f3bb46..8b1a26f 100644 --- a/erasure_layout.c +++ b/erasure_layout.c @@ -304,6 +304,8 @@ msg_cerr("ERASE FAILED!\n"); } + update_progress(flashctx, FLASHROM_PROGRESS_ERASE, block_len); + // adjust curcontents memset(curcontents+start_addr, erased_value, block_len); // after erase make it unselected again diff --git a/flashrom.c b/flashrom.c index e2c60f4..88fa5d9 100644 --- a/flashrom.c +++ b/flashrom.c @@ -1225,6 +1225,24 @@ return chip - flashchips; } +static void setup_progress_from_layout(struct flashctx *flashctx, + enum flashrom_progress_stage stage) +{ + if (!flashctx->progress_callback) + return; + + const struct flashrom_layout *const flash_layout = get_layout(flashctx); + + size_t total = 0; + const struct romentry *entry = NULL; + while ((entry = layout_next_included(flash_layout, entry))) { + const struct flash_region *region = &entry->region; + total += region->end - region->start + 1; + } + + init_progress(flashctx, stage, total); +} + /** * @brief Reads the included layout regions into a buffer. * @@ -1241,6 +1259,8 @@ const struct flashrom_layout *const layout = get_layout(flashctx); const struct romentry *entry = NULL; + setup_progress_from_layout(flashctx, FLASHROM_PROGRESS_READ); + while ((entry = layout_next_included(layout, entry))) { const struct flash_region *region = &entry->region; const chipoff_t region_start = region->start; @@ -1353,6 +1373,9 @@ memset(curcontents, ~ERASED_VALUE(flashctx), flash_size); memset(newcontents, ERASED_VALUE(flashctx), flash_size); + setup_progress_from_layout(flashctx, FLASHROM_PROGRESS_READ); + setup_progress_from_layout(flashctx, FLASHROM_PROGRESS_ERASE); + const struct flashrom_layout *const flash_layout = get_layout(flashctx); const struct romentry *entry = NULL; while ((entry = layout_next_included(flash_layout, entry))) { @@ -1389,6 +1412,10 @@ goto _ret; } + setup_progress_from_layout(flashctx, FLASHROM_PROGRESS_READ); + setup_progress_from_layout(flashctx, FLASHROM_PROGRESS_WRITE); + setup_progress_from_layout(flashctx, FLASHROM_PROGRESS_ERASE); + const struct romentry *entry = NULL; while ((entry = layout_next_included(flash_layout, entry))) { ret = erase_write(flashctx, entry->region.start, @@ -1427,6 +1454,8 @@ { const struct romentry *entry = NULL; + setup_progress_from_layout(flashctx, FLASHROM_PROGRESS_READ); + while ((entry = layout_next_included(layout, entry))) { const struct flash_region *region = &entry->region; const chipoff_t region_start = region->start; @@ -1924,6 +1953,7 @@ */ msg_cinfo("Reading old flash chip contents... "); if (verify_all) { + init_progress(flashctx, FLASHROM_PROGRESS_READ, flash_size); if (read_flash(flashctx, oldcontents, 0, flash_size)) { msg_cinfo("FAILED.\n"); goto _finalize_ret; @@ -1940,12 +1970,14 @@ bool all_skipped = true; + msg_cinfo("Updating flash chip contents... "); if (write_by_layout(flashctx, curcontents, newcontents, &all_skipped)) { msg_cerr("Uh oh. Erase/write failed. "); ret = 2; if (verify_all) { msg_cerr("Checking if anything has changed.\n"); msg_cinfo("Reading current flash chip contents... "); + init_progress(flashctx, FLASHROM_PROGRESS_READ, flash_size); if (!read_flash(flashctx, curcontents, 0, flash_size)) { msg_cinfo("done.\n"); if (!memcmp(oldcontents, curcontents, flash_size)) { diff --git a/include/flash.h b/include/flash.h index d0e55af..60c099c 100644 --- a/include/flash.h +++ b/include/flash.h @@ -549,6 +549,11 @@ typedef int (blockprotect_func_t)(struct flashctx *flash); blockprotect_func_t *lookup_blockprotect_func_ptr(const struct flashchip *const chip); +struct stage_progress { + size_t current; + size_t total; +}; + struct flashrom_flashctx { struct flashchip *chip; /* FIXME: The memory mappings should be saved in a more structured way. */ @@ -587,6 +592,7 @@ /* Progress reporting */ flashrom_progress_callback *progress_callback; struct flashrom_progress *progress_state; + struct stage_progress stage_progress[FLASHROM_PROGRESS_NR]; }; /* Timing used in probe routines. ZERO is -2 to differentiate between an unset @@ -677,6 +683,12 @@ */ #define ERROR_FLASHROM_LIMIT -201 +struct cli_progress { + unsigned int stage_pc[FLASHROM_PROGRESS_NR]; + unsigned int visible_stages; /* Bitmask of stages with non-zero progress. */ + bool stage_setup; /* Flag to know when to reset progress data. */ +}; + /* cli_common.c */ void print_chip_support_status(const struct flashchip *chip); @@ -716,7 +728,8 @@ #define msg_gspew(...) print(FLASHROM_MSG_SPEW, __VA_ARGS__) /* general debug spew */ #define msg_pspew(...) print(FLASHROM_MSG_SPEW, __VA_ARGS__) /* programmer debug spew */ #define msg_cspew(...) print(FLASHROM_MSG_SPEW, __VA_ARGS__) /* chip debug spew */ -void update_progress(struct flashctx *flash, enum flashrom_progress_stage stage, size_t current, size_t total); +void init_progress(struct flashctx *flash, enum flashrom_progress_stage stage, size_t total); +void update_progress(struct flashctx *flash, enum flashrom_progress_stage stage, size_t increment); /* spi.c */ struct spi_command { @@ -730,4 +743,5 @@ int spi_send_multicommand(const struct flashctx *flash, struct spi_command *cmds); enum chipbustype get_buses_supported(void); + #endif /* !__FLASH_H__ */ diff --git a/it87spi.c b/it87spi.c index 9c64659..fddae3b 100644 --- a/it87spi.c +++ b/it87spi.c @@ -292,7 +292,7 @@ int ret = it8716f_spi_page_program(flash, buf, start); if (ret) return ret; - update_progress(flash, FLASHROM_PROGRESS_WRITE, chip->page_size - len, chip->page_size); + update_progress(flash, FLASHROM_PROGRESS_WRITE, chip->page_size); start += chip->page_size; len -= chip->page_size; buf += chip->page_size; diff --git a/jedec.c b/jedec.c index 2d18de7..df0d93a 100644 --- a/jedec.c +++ b/jedec.c @@ -382,7 +382,7 @@ if (write_byte_program_jedec_common(flash, src, dst)) failed = 1; dst++, src++; - update_progress(flash, FLASHROM_PROGRESS_WRITE, i + 1, len); + update_progress(flash, FLASHROM_PROGRESS_WRITE, 1); } if (failed) msg_cerr(" writing sector at 0x%" PRIxPTR " failed!\n", olddst); @@ -467,7 +467,7 @@ if (jedec_write_page(flash, buf + starthere - start, starthere, lenhere)) return 1; - update_progress(flash, FLASHROM_PROGRESS_WRITE, i + 1, nwrites + 1); + update_progress(flash, FLASHROM_PROGRESS_WRITE, lenhere); } return 0; diff --git a/libflashrom.c b/libflashrom.c index 7c885cf..dfc86e6 100644 --- a/libflashrom.c +++ b/libflashrom.c @@ -70,16 +70,36 @@ flashctx->progress_state = progress_state; } /** @private */ -void update_progress(struct flashrom_flashctx *flashctx, enum flashrom_progress_stage stage, size_t current, size_t total) +void init_progress(struct flashrom_flashctx *flashctx, enum flashrom_progress_stage stage, size_t total) { if (flashctx->progress_callback == NULL) return; - if (current > total) - current = total; + + struct stage_progress *stage_progress = &flashctx->stage_progress[stage]; + stage_progress->current = 0; + stage_progress->total = total; + + /* This is used to trigger callback invocation, with 0 current state and 0 increment: as an init call. */ + update_progress(flashctx, stage, 0); +} +/** @private */ +void update_progress(struct flashrom_flashctx *flashctx, enum flashrom_progress_stage stage, size_t increment) +{ + if (flashctx->progress_callback == NULL) + return; + + struct stage_progress *stage_progress = &flashctx->stage_progress[stage]; + + stage_progress->current += increment; + if (stage_progress->current > stage_progress->total) { + msg_gwarn("Fixing total value of stage %d progress on the fly.", stage); + stage_progress->total = stage_progress->current; + } flashctx->progress_state->stage = stage; - flashctx->progress_state->current = current; - flashctx->progress_state->total = total; + flashctx->progress_state->current = stage_progress->current; + flashctx->progress_state->total = stage_progress->total; + flashctx->progress_callback(flashctx); } diff --git a/linux_mtd.c b/linux_mtd.c index eea0cf2..d5fc509 100644 --- a/linux_mtd.c +++ b/linux_mtd.c @@ -215,7 +215,7 @@ } i += step; - update_progress(flash, FLASHROM_PROGRESS_READ, i, len); + update_progress(flash, FLASHROM_PROGRESS_READ, step); } return 0; @@ -258,7 +258,7 @@ } i += step; - update_progress(flash, FLASHROM_PROGRESS_WRITE, i, len); + update_progress(flash, FLASHROM_PROGRESS_WRITE, step); } return 0; @@ -295,7 +295,6 @@ __func__, ret, strerror(errno)); return 1; } - update_progress(flash, FLASHROM_PROGRESS_ERASE, u + data->erasesize, len); } return 0; diff --git a/parade_lspcon.c b/parade_lspcon.c index dfcf659..16283e7 100644 --- a/parade_lspcon.c +++ b/parade_lspcon.c @@ -354,7 +354,7 @@ for (i = 0; i < len; i += TUNNEL_PAGE_SIZE) { ret |= parade_lspcon_map_page(fd, start + i); ret |= parade_lspcon_read_data(fd, PAGE_ADDRESS, buf + i, min(len - i, TUNNEL_PAGE_SIZE)); - update_progress(flash, FLASHROM_PROGRESS_READ, i + TUNNEL_PAGE_SIZE, len); + update_progress(flash, FLASHROM_PROGRESS_READ, TUNNEL_PAGE_SIZE); } return ret; @@ -395,7 +395,7 @@ for (unsigned int i = 0; i < len; i += TUNNEL_PAGE_SIZE) { ret |= parade_lspcon_map_page(fd, start + i); ret |= parade_lspcon_write_page(fd, buf + i, min(len - i, TUNNEL_PAGE_SIZE)); - update_progress(flash, FLASHROM_PROGRESS_WRITE, i + TUNNEL_PAGE_SIZE, len); + update_progress(flash, FLASHROM_PROGRESS_WRITE, TUNNEL_PAGE_SIZE); } ret |= parade_lspcon_enable_write_protection(fd); diff --git a/realtek_mst_i2c_spi.c b/realtek_mst_i2c_spi.c index 57677ec..3a89eab 100644 --- a/realtek_mst_i2c_spi.c +++ b/realtek_mst_i2c_spi.c @@ -397,7 +397,7 @@ ret |= realtek_mst_i2c_execute_write(fd); if (ret) break; - update_progress(flash, FLASHROM_PROGRESS_WRITE, i + RTK_PAGE_SIZE, len); + update_progress(flash, FLASHROM_PROGRESS_WRITE, page_len); } return ret; diff --git a/spi.c b/spi.c index 3e81da2..b2ffc33 100644 --- a/spi.c +++ b/spi.c @@ -104,8 +104,6 @@ { int ret; size_t to_read; - size_t start_address = start; - size_t end_address = len - start; for (; len; len -= to_read, buf += to_read, start += to_read) { /* Do not cross 16MiB boundaries in a single transfer. This helps with @@ -115,7 +113,6 @@ ret = flash->mst->spi.read(flash, buf, start, to_read); if (ret) return ret; - update_progress(flash, FLASHROM_PROGRESS_READ, start - start_address + to_read, end_address); } return 0; } diff --git a/spi25.c b/spi25.c index 6a6ee75..ab479e5 100644 --- a/spi25.c +++ b/spi25.c @@ -681,14 +681,12 @@ { int ret; size_t to_read; - size_t start_address = start; - size_t end_address = len - start; for (; len; len -= to_read, buf += to_read, start += to_read) { to_read = min(chunksize, len); ret = spi_nbyte_read(flash, start, buf, to_read); if (ret) return ret; - update_progress(flash, FLASHROM_PROGRESS_READ, start - start_address + to_read, end_address); + update_progress(flash, FLASHROM_PROGRESS_READ, to_read); } return 0; } @@ -708,8 +706,6 @@ * we're OK for now. */ unsigned int page_size = flash->chip->page_size; - size_t start_address = start; - size_t end_address = len - start; /* Warning: This loop has a very unusual condition and body. * The loop needs to go through each page with at least one affected @@ -733,8 +729,9 @@ rc = spi_nbyte_program(flash, starthere + j, buf + starthere - start + j, towrite); if (rc) return rc; + + update_progress(flash, FLASHROM_PROGRESS_WRITE, towrite); } - update_progress(flash, FLASHROM_PROGRESS_WRITE, start - start_address + lenhere, end_address); } return 0; @@ -754,7 +751,7 @@ for (i = start; i < start + len; i++) { if (spi_nbyte_program(flash, i, buf + i - start, 1)) return 1; - update_progress(flash, FLASHROM_PROGRESS_WRITE, i - start, len - start); + update_progress(flash, FLASHROM_PROGRESS_WRITE, 1); } return 0; } diff --git a/sst28sf040.c b/sst28sf040.c index 9080684..e6a4b36 100644 --- a/sst28sf040.c +++ b/sst28sf040.c @@ -92,7 +92,7 @@ /* wait for Toggle bit ready */ toggle_ready_jedec(flash, bios); - update_progress(flash, FLASHROM_PROGRESS_WRITE, i + 1, len); + update_progress(flash, FLASHROM_PROGRESS_WRITE, 1); } return 0; diff --git a/tests/chip.c b/tests/chip.c index 604d79e..ac60663 100644 --- a/tests/chip.c +++ b/tests/chip.c @@ -46,6 +46,10 @@ .buf = { 0 }, }; +struct progress_user_data { + size_t last_seen; /* % of progress last reported, to be asserted in the progress callback. */ +}; + static int read_chip(struct flashctx *flash, uint8_t *buf, unsigned int start, unsigned int len) { printf("Read chip called with start=0x%x, len=0x%x\n", start, len); @@ -76,6 +80,21 @@ return 0; } +static void progress_callback(struct flashctx *flash) { + struct progress_user_data *progress_user_data = flash->progress_state->user_data; + if (flash->progress_state->current == 0) { + printf("Progress started for stage %d, initial callback call\n", flash->progress_state->stage); + } else { + /* Progress cannot go backwards. */ + assert_true(flash->progress_state->current >= progress_user_data->last_seen); + } + + if (flash->progress_state->current >= flash->progress_state->total) + printf("Progress complete for stage %d, final callback call\n", flash->progress_state->stage); + + progress_user_data->last_seen = flash->progress_state->current; +} + static void setup_chip(struct flashrom_flashctx *flashctx, struct flashrom_layout **layout, struct flashchip *chip, const char *programmer_param, const struct io_mock *io) { @@ -210,6 +229,41 @@ teardown(&layout); } +void erase_chip_with_progress(void **state) +{ + (void) state; /* unused */ + + static struct io_mock_fallback_open_state data = { + .noc = 0, + .paths = { NULL }, + }; + const struct io_mock chip_io = { + .fallback_open_state = &data, + }; + + g_test_write_injector = write_chip; + g_test_read_injector = read_chip; + g_test_erase_injector[0] = block_erase_chip; + struct flashrom_flashctx flashctx = { 0 }; + struct flashrom_layout *layout; + struct flashchip mock_chip = chip_8MiB; + const char *param = ""; /* Default values for all params. */ + + setup_chip(&flashctx, &layout, &mock_chip, param, &chip_io); + + struct progress_user_data progress_user_data = {0}; + struct flashrom_progress progress_state = { + .user_data = &progress_user_data + }; + flashrom_set_progress_callback(&flashctx, progress_callback, &progress_state); + + printf("Erase chip operation started.\n"); + assert_int_equal(0, flashrom_flash_erase(&flashctx)); + printf("Erase chip operation done.\n"); + + teardown(&layout); +} + void erase_chip_with_dummyflasher_test_success(void **state) { (void) state; /* unused */ @@ -277,6 +331,49 @@ free(buf); } +void read_chip_with_progress(void **state) +{ + (void) state; /* unused */ + + static struct io_mock_fallback_open_state data = { + .noc = 0, + .paths = { NULL }, + }; + const struct io_mock chip_io = { + .fallback_open_state = &data, + }; + + g_test_write_injector = write_chip; + g_test_read_injector = read_chip; + g_test_erase_injector[0] = block_erase_chip; + struct flashrom_flashctx flashctx = { 0 }; + struct flashrom_layout *layout; + struct flashchip mock_chip = chip_8MiB; + const char *param = ""; /* Default values for all params. */ + + setup_chip(&flashctx, &layout, &mock_chip, param, &chip_io); + + struct progress_user_data progress_user_data = {0}; + struct flashrom_progress progress_state = { + .user_data = &progress_user_data + }; + flashrom_set_progress_callback(&flashctx, progress_callback, &progress_state); + + const char *const filename = "read_chip.test"; + unsigned long size = mock_chip.total_size * 1024; + unsigned char *buf = calloc(size, sizeof(unsigned char)); + assert_non_null(buf); + + printf("Read chip operation started.\n"); + assert_int_equal(0, flashrom_image_read(&flashctx, buf, size)); + assert_int_equal(0, write_buf_to_file(buf, size, filename)); + printf("Read chip operation done.\n"); + + teardown(&layout); + + free(buf); +} + void read_chip_with_dummyflasher_test_success(void **state) { (void) state; /* unused */ @@ -365,6 +462,49 @@ free(newcontents); } +void write_chip_with_progress(void **state) +{ + (void) state; /* unused */ + + static struct io_mock_fallback_open_state data = { + .noc = 0, + .paths = { NULL }, + }; + const struct io_mock chip_io = { + .fallback_open_state = &data, + }; + + g_test_write_injector = write_chip; + g_test_read_injector = read_chip; + g_test_erase_injector[0] = block_erase_chip; + struct flashrom_flashctx flashctx = { 0 }; + struct flashrom_layout *layout; + struct flashchip mock_chip = chip_8MiB; + const char *param = ""; /* Default values for all params. */ + + setup_chip(&flashctx, &layout, &mock_chip, param, &chip_io); + + struct progress_user_data progress_user_data = {0}; + struct flashrom_progress progress_state = { + .user_data = &progress_user_data + }; + flashrom_set_progress_callback(&flashctx, progress_callback, &progress_state); + + const char *const filename = "-"; + unsigned long size = mock_chip.total_size * 1024; + uint8_t *const newcontents = malloc(size); + assert_non_null(newcontents); + + printf("Write chip operation started.\n"); + assert_int_equal(0, read_buf_from_file(newcontents, size, filename)); + assert_int_equal(0, flashrom_image_write(&flashctx, newcontents, size, NULL)); + printf("Write chip operation done.\n"); + + teardown(&layout); + + free(newcontents); +} + void write_chip_with_dummyflasher_test_success(void **state) { (void) state; /* unused */ diff --git a/tests/spi25.c b/tests/spi25.c index 8729593..a442c7b 100644 --- a/tests/spi25.c +++ b/tests/spi25.c @@ -55,7 +55,9 @@ */ return __real_spi_send_command(flash, writecnt, readcnt, writearr, readarr); - check_expected_ptr(flash); + if (!flash->progress_callback) + check_expected_ptr(flash); + assert_int_equal(writecnt, mock_type(int)); assert_int_equal(writearr[0], mock_type(int)); @@ -71,22 +73,22 @@ { struct flashrom_progress *progress_state = flashctx->progress_state; uint32_t *cnt = (uint32_t *) progress_state->user_data; - assert_int_equal(0x300, progress_state->total); + assert_int_equal(0x400, progress_state->total); switch (*cnt) { case 0: - assert_int_equal(0x100, progress_state->current); + assert_int_equal(0x0, progress_state->current); break; case 1: - assert_int_equal(0x200, progress_state->current); + assert_int_equal(0x100, progress_state->current); break; case 2: - assert_int_equal(0x300, progress_state->current); + assert_int_equal(0x200, progress_state->current); break; case 3: assert_int_equal(0x300, progress_state->current); break; case 4: - assert_int_equal(0x300, progress_state->current); + assert_int_equal(0x400, progress_state->current); break; default: fail(); @@ -94,7 +96,7 @@ (*cnt)++; } -void spi_read_chunked_test_success(void **state) +void default_spi_read_test_success(void **state) { (void) state; /* unused */ uint8_t buf[0x400] = { 0x0 }; @@ -115,15 +117,16 @@ .user_data = (void *) &cnt, }; flashrom_set_progress_callback(&flashctx, spi_read_progress_cb, &progress_state); + init_progress(&flashctx, FLASHROM_PROGRESS_READ, 0x400); for (int i = 0; i < 4; i++) { - expect_memory(__wrap_spi_send_command, flash, - &flashctx, sizeof(flashctx)); will_return(__wrap_spi_send_command, JEDEC_WRDI); will_return(__wrap_spi_send_command, JEDEC_READ); will_return(__wrap_spi_send_command, max_data_read); } - assert_int_equal(0, spi_chip_read(&flashctx, buf, offset, sizeof(buf))); + assert_int_equal(0, default_spi_read(&flashctx, buf, offset, sizeof(buf))); assert_int_equal(5, cnt); + + flashrom_set_progress_callback(&flashctx, NULL, NULL); } void spi_write_enable_test_success(void **state) diff --git a/tests/tests.c b/tests/tests.c index 3fee056..00b4665 100644 --- a/tests/tests.c +++ b/tests/tests.c @@ -441,7 +441,7 @@ const struct CMUnitTest spi25_tests[] = { cmocka_unit_test(spi_write_enable_test_success), cmocka_unit_test(spi_write_disable_test_success), - cmocka_unit_test(spi_read_chunked_test_success), + cmocka_unit_test(default_spi_read_test_success), cmocka_unit_test(probe_spi_rdid_test_success), cmocka_unit_test(probe_spi_rdid4_test_success), cmocka_unit_test(probe_spi_rems_test_success), @@ -495,10 +495,13 @@ const struct CMUnitTest chip_tests[] = { cmocka_unit_test(erase_chip_test_success), + cmocka_unit_test(erase_chip_with_progress), cmocka_unit_test(erase_chip_with_dummyflasher_test_success), cmocka_unit_test(read_chip_test_success), + cmocka_unit_test(read_chip_with_progress), cmocka_unit_test(read_chip_with_dummyflasher_test_success), cmocka_unit_test(write_chip_test_success), + cmocka_unit_test(write_chip_with_progress), cmocka_unit_test(write_chip_with_dummyflasher_test_success), cmocka_unit_test(write_chip_feature_no_erase), cmocka_unit_test(write_nonaligned_region_with_dummyflasher_test_success), diff --git a/tests/tests.h b/tests/tests.h index 100bda1..dba33fb 100644 --- a/tests/tests.h +++ b/tests/tests.h @@ -34,7 +34,7 @@ /* spi25.c */ void spi_write_enable_test_success(void **state); void spi_write_disable_test_success(void **state); -void spi_read_chunked_test_success(void **state); +void default_spi_read_test_success(void **state); void probe_spi_rdid_test_success(void **state); void probe_spi_rdid4_test_success(void **state); void probe_spi_rems_test_success(void **state); @@ -82,10 +82,13 @@ /* chip.c */ void erase_chip_test_success(void **state); +void erase_chip_with_progress(void **state); void erase_chip_with_dummyflasher_test_success(void **state); void read_chip_test_success(void **state); +void read_chip_with_progress(void **state); void read_chip_with_dummyflasher_test_success(void **state); void write_chip_test_success(void **state); +void write_chip_with_progress(void **state); void write_chip_with_dummyflasher_test_success(void **state); void write_chip_feature_no_erase(void **state); void write_nonaligned_region_with_dummyflasher_test_success(void **state); -- 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: merged Gerrit-Project: flashrom Gerrit-Branch: main Gerrit-Change-Id: If1e40fc97f443c4f0c0501cef11cff1f3f84c051 Gerrit-Change-Number: 84102 Gerrit-PatchSet: 14 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: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>

1 0

[M] Change in flashrom[main]: erasure_layout: Add an option to sacrifice unchanged blocks for speed
by Bill XIE (Code Review) Oct. 27, 2024

Oct. 27, 2024

Attention is currently required from: Aarya, Anastasia Klimchuk. Bill XIE has posted comments on this change by Bill XIE. ( https://review.coreboot.org/c/flashrom/+/84721?usp=email ) Change subject: erasure_layout: Add an option to sacrifice unchanged blocks for speed ...................................................................... Patch Set 6: (4 comments) Commit Message: https://review.coreboot.org/c/flashrom/+/84721/comment/2dad0d57_7381a63c?us… : PS5, Line 9: Someone may prefer to sacrifice sub-blocks needless to change within a : larger erase block for programming speed. These people who are okay to : fry their chip sooner can set this option. If the percentage of : sub-blocks needless to change within a larger erase block is lower : than the given value, the whole larger erase block will be erased. > I wrote another version of commit message, what do you think? I can update myself, but first I want […] Done File cli_classic.c: https://review.coreboot.org/c/flashrom/+/84721/comment/4bc97cd3_5669fe6f?us… : PS3, Line 124: " --sacrifice-ratio <ratio> The percentage of overall sub-blocks needless to\n" : " change within a larger erase block to be sacrificed\n" : " for programming speed, default 0, max 100\n" > I have an idea, what do you think: […] Done and added context for "S+1 size block". File cli_classic.c: https://review.coreboot.org/c/flashrom/+/84721/comment/fcde11ad_97a2a612?us… : PS5, Line 126: max 100 > What do you think about restricting max to 50? I don't think values more than 50 make sense. Done https://review.coreboot.org/c/flashrom/+/84721/comment/68789828_8c24791e?us… : PS5, Line 1141: Invalid input of sacrifice ratio, use default 0 > Invalid input of sacrifice ratio, valid range is 0-50. Fallback to default value 0. […] Done -- To view, visit https://review.coreboot.org/c/flashrom/+/84721?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: I154e8a713f626c37dbbe118db700055b96d24803 Gerrit-Change-Number: 84721 Gerrit-PatchSet: 6 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: Aarya <aarya.chaumal(a)gmail.com> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Comment-Date: Sun, 27 Oct 2024 03:25:39 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org>

1 0

[M] Change in flashrom[main]: erasure_layout: Add an option to sacrifice unchanged blocks for speed
by Bill XIE (Code Review) Oct. 27, 2024

Oct. 27, 2024

Attention is currently required from: Aarya, Bill XIE. Hello Aarya, Anastasia Klimchuk, build bot (Jenkins), I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/flashrom/+/84721?usp=email to look at the new patch set (#7). The following approvals got outdated and were removed: Verified-1 by build bot (Jenkins) Change subject: erasure_layout: Add an option to sacrifice unchanged blocks for speed ...................................................................... erasure_layout: Add an option to sacrifice unchanged blocks for speed The patch adds command line option to handle the following situation: There is a region which is requested to be erased (or written, because the write operation uses erase too). Some of the areas inside this region don't need to be erased, because the bytes already have expected value. Such areas can be skipped. The logic selects eraseblocks that can cover the areas which need to be erased. Suppose there is a region which is partially covered by eraseblocks of size S (partially because remaining areas don't need to be erased). Now suppose we can cover the whole region with eraseblock of larger size, S+1, and erase it all at once. This will run faster: erase opcode will only be sent once instead of many smaller opcodes. However, this will run erase over some areas of the chip memory that didn't need to be erased. Which means, the chip, as a hardware, will wear faster. New command line option sets the maximum % memory that is allowed for redundant erase. Default is 0, S+1 size block only selected if all the area needs to be erased in full. 50 means that if more than a half of the area needs to be erased, a S+1 size block can be selected to cover all area with one block. The tradeoff is the speed of programming operation VS the longevity of the chip. Default is longevity. Change-Id: I154e8a713f626c37dbbe118db700055b96d24803 Co-developed-by: persmule <persmule(a)hardenedlinux.org Co-developed-by: Anastasia Klimchuk <aklm(a)flashrom.org> Signed-off-by: persmule <persmule(a)hardenedlinux.org> Signed-off-by: Anastasia Klimchuk <aklm(a)flashrom.org> --- M cli_classic.c M erasure_layout.c M include/flash.h M tests/erase_func_algo.c 4 files changed, 118 insertions(+), 5 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/21/84721/7 -- To view, visit https://review.coreboot.org/c/flashrom/+/84721?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: I154e8a713f626c37dbbe118db700055b96d24803 Gerrit-Change-Number: 84721 Gerrit-PatchSet: 7 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>

1 0

[M] Change in flashrom[main]: erasure_layout: Add an option to sacrifice unchanged blocks for speed
by Bill XIE (Code Review) Oct. 27, 2024

Oct. 27, 2024

Attention is currently required from: Aarya, Bill XIE. Hello Aarya, Anastasia Klimchuk, build bot (Jenkins), I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/flashrom/+/84721?usp=email to look at the new patch set (#6). The following approvals got outdated and were removed: Verified+1 by build bot (Jenkins) Change subject: erasure_layout: Add an option to sacrifice unchanged blocks for speed ...................................................................... erasure_layout: Add an option to sacrifice unchanged blocks for speed The patch adds command line option to handle the following situation: There is a region which is requested to be erased (or written, because the write operation uses erase too). Some of the areas inside this region don't need to be erased, because the bytes already have expected value. Such areas can be skipped. The logic selects eraseblocks that can cover the areas which need to be erased. Suppose there is a region which is partially covered by eraseblocks of size S (partially because remaining areas don't need to be erased). Now suppose we can cover the whole region with eraseblock of larger size, S+1, and erase it all at once. This will run faster: erase opcode will only be sent once instead of many smaller opcodes. However, this will run erase over some areas of the chip memory that didn't need to be erased. Which means, the chip, as a hardware, will wear faster. New command line option sets the maximum % memory that is allowed for redundant erase. Default is 0, S+1 size block only selected if all the area needs to be erased in full. 50 means that if more than a half of the area needs to be erased, a S+1 size block can be selected to cover all area with one block. The tradeoff is the speed of programming operation VS the longevity of the chip. Default is longevity. Change-Id: I154e8a713f626c37dbbe118db700055b96d24803 Co-developed-by: persmule <persmule(a)hardenedlinux.org Co-developed-by: Anastasia Klimchuk <aklm(a)flashrom.org> Signed-off-by: persmule <persmule(a)hardenedlinux.org> Signed-off-by: Anastasia Klimchuk <aklm(a)flashrom.org> --- M cli_classic.c M erasure_layout.c M include/flash.h M tests/erase_func_algo.c 4 files changed, 118 insertions(+), 5 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/21/84721/6 -- To view, visit https://review.coreboot.org/c/flashrom/+/84721?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: I154e8a713f626c37dbbe118db700055b96d24803 Gerrit-Change-Number: 84721 Gerrit-PatchSet: 6 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>

1 0