Anastasia Klimchuk has submitted this change. ( https://review.coreboot.org/c/flashrom/+/84439?usp=email )
Change subject: Display progress for what is actually erased/written ......................................................................
Display progress for what is actually erased/written
The patch updates calculation of total length for the operation which is displayed with progress.
The reason is: even if, for example the whole chip erase or write was requested, the actual length of bytes modified can be less than whole chip size (areas which already have expected content, are skipped).
Change-Id: I88ac4d40f1b6ccc1636b1efb690d8d68bdebec08 Co-developed-by: Anastasia Klimchuk aklm@flashrom.org Co-developed-by: Sergii Dmytruk sergii.dmytruk@3mdeb.com Signed-off-by: Anastasia Klimchuk aklm@flashrom.org Reviewed-on: https://review.coreboot.org/c/flashrom/+/84439 Reviewed-by: Sergii Dmytruk sergii.dmytruk@3mdeb.com Tested-by: build bot (Jenkins) no-reply@coreboot.org --- M doc/classic_cli_manpage.rst M flashrom.c M tests/chip.c M tests/tests.c M tests/tests.h 5 files changed, 113 insertions(+), 9 deletions(-)
Approvals: build bot (Jenkins): Verified Sergii Dmytruk: Looks good to me, approved
diff --git a/doc/classic_cli_manpage.rst b/doc/classic_cli_manpage.rst index f0fd681..f0879e4 100644 --- a/doc/classic_cli_manpage.rst +++ b/doc/classic_cli_manpage.rst @@ -317,7 +317,7 @@
**--progress** - [Experimental feature] Show progress percentage of operations on the standard output. + Show progress percentage of operations on the standard output.
**--sacrifice-ratio <ratio>** Fraction (as a percentage, 0-50) of an erase block that may be erased even if unmodified. diff --git a/flashrom.c b/flashrom.c index 88fa5d9..4ae9390 100644 --- a/flashrom.c +++ b/flashrom.c @@ -1243,6 +1243,56 @@ init_progress(flashctx, stage, total); }
+ +static void setup_progress_from_layout_and_diff(struct flashctx *flashctx, + const void *have, + const void *want, + enum flashrom_progress_stage stage) +{ + if (!flashctx->progress_callback) + return; + + const struct flashrom_layout *flash_layout = get_layout(flashctx); + const size_t page_size = flashctx->chip->page_size; + + size_t total = 0; + + const struct romentry *entry = NULL; + while ((entry = layout_next_included(flash_layout, entry))) { + const struct flash_region *region = &entry->region; + + if (stage == FLASHROM_PROGRESS_ERASE) { + size_t offset; + for (offset = region->start; offset <= region->end; offset += page_size) { + const size_t len = min(page_size, region->end + 1 - offset); + + if (need_erase(have, want, len, flashctx->chip->gran, ERASED_VALUE(flashctx))) + total += len; + } + } + + if (stage == FLASHROM_PROGRESS_WRITE) { + unsigned int start = region->start; + unsigned int len; + while ((len = get_next_write(have + start, want + start, + region->end + 1 - start, &start, flashctx->chip->gran))) { + start += len; + total += len; + } + + if (flashctx->chip->feature_bits & FEATURE_NO_ERASE) + /* For chips with FEATURE_NO_ERASE erase op is running as write under the hood. + * So typical write, which usually consists of erasing and then writing, + * would be writing and then writing again. The planned total length for the + * progress indicator for write is double. */ + total *= 2; + } + } + + init_progress(flashctx, stage, total); +} + + /** * @brief Reads the included layout regions into a buffer. * @@ -1374,7 +1424,7 @@ memset(newcontents, ERASED_VALUE(flashctx), flash_size);
setup_progress_from_layout(flashctx, FLASHROM_PROGRESS_READ); - setup_progress_from_layout(flashctx, FLASHROM_PROGRESS_ERASE); + setup_progress_from_layout_and_diff(flashctx, curcontents, newcontents, FLASHROM_PROGRESS_ERASE);
const struct flashrom_layout *const flash_layout = get_layout(flashctx); const struct romentry *entry = NULL; @@ -1413,8 +1463,8 @@ }
setup_progress_from_layout(flashctx, FLASHROM_PROGRESS_READ); - setup_progress_from_layout(flashctx, FLASHROM_PROGRESS_WRITE); - setup_progress_from_layout(flashctx, FLASHROM_PROGRESS_ERASE); + setup_progress_from_layout_and_diff(flashctx, curcontents, newcontents, FLASHROM_PROGRESS_WRITE); + setup_progress_from_layout_and_diff(flashctx, curcontents, newcontents, FLASHROM_PROGRESS_ERASE);
const struct romentry *entry = NULL; while ((entry = layout_next_included(flash_layout, entry))) { diff --git a/tests/chip.c b/tests/chip.c index ac60663..25bb8db 100644 --- a/tests/chip.c +++ b/tests/chip.c @@ -47,7 +47,8 @@ };
struct progress_user_data { - size_t last_seen; /* % of progress last reported, to be asserted in the progress callback. */ + /* % of progress last reported for each operation, to be asserted in the progress callback. */ + size_t last_seen[FLASHROM_PROGRESS_NR]; };
static int read_chip(struct flashctx *flash, uint8_t *buf, unsigned int start, unsigned int len) @@ -86,13 +87,14 @@ 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); + assert_true(flash->progress_state->current >= progress_user_data->last_seen[flash->progress_state->stage]); }
- if (flash->progress_state->current >= flash->progress_state->total) - printf("Progress complete for stage %d, final callback call\n", flash->progress_state->stage); + if (flash->progress_state->current >= flash->progress_state->total - 1) + printf("Progress almost complete for stage %d, current %ld, total %ld\n", + flash->progress_state->stage, flash->progress_state->current, flash->progress_state->total);
- progress_user_data->last_seen = flash->progress_state->current; + progress_user_data->last_seen[flash->progress_state->stage] = flash->progress_state->current; }
static void setup_chip(struct flashrom_flashctx *flashctx, struct flashrom_layout **layout, @@ -144,6 +146,7 @@ .tested = TEST_OK_PREW, .read = TEST_READ_INJECTOR, .write = TEST_WRITE_INJECTOR, + .page_size = 256, .block_erasers = {{ /* All blocks within total size of the chip. */ @@ -586,6 +589,55 @@ free(newcontents); }
+void write_chip_feature_no_erase_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, + }; + + struct flashrom_flashctx flashctx = { 0 }; + struct flashrom_layout *layout; + + /* + * Tricking the dummyflasher by asking to emulate W25Q128FV but giving to it + * mock chip with FEATURE_NO_ERASE. + * As long as chip size is the same, this is fine. + */ + struct flashchip mock_chip = chip_no_erase; + const char *param_dup = "bus=spi,emulate=W25Q128FV"; + + setup_chip(&flashctx, &layout, &mock_chip, param_dup, &chip_io); + + /* See comment in write_chip_test_success */ + const char *const filename = "-"; + unsigned long size = mock_chip.total_size * 1024; + uint8_t *const newcontents = malloc(size); + assert_non_null(newcontents); + + 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("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)); + assert_int_equal(0, flashrom_image_verify(&flashctx, newcontents, size)); + printf("Write chip operation done.\n"); + + teardown(&layout); + + free(newcontents); +} + + void write_nonaligned_region_with_dummyflasher_test_success(void **state) { (void) state; /* unused */ diff --git a/tests/tests.c b/tests/tests.c index 00b4665..72b4868 100644 --- a/tests/tests.c +++ b/tests/tests.c @@ -504,6 +504,7 @@ 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_chip_feature_no_erase_with_progress), cmocka_unit_test(write_nonaligned_region_with_dummyflasher_test_success), cmocka_unit_test(verify_chip_test_success), cmocka_unit_test(verify_chip_with_dummyflasher_test_success), diff --git a/tests/tests.h b/tests/tests.h index dba33fb..85c4f52 100644 --- a/tests/tests.h +++ b/tests/tests.h @@ -91,6 +91,7 @@ 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_chip_feature_no_erase_with_progress(void **state); void write_nonaligned_region_with_dummyflasher_test_success(void **state); void verify_chip_test_success(void **state); void verify_chip_with_dummyflasher_test_success(void **state);
-
Anastasia Klimchuk (Code Review)