From cbdb8534d2df61599f6dc36b74f8c20a855037ec Mon Sep 17 00:00:00 2001 From: Anastasia Klimchuk Date: Fri, 20 Sep 2024 23:33:07 +1000 Subject: [PATCH] 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 Co-developed-by: Sergii Dmytruk Signed-off-by: Anastasia Klimchuk Reviewed-on: https://review.coreboot.org/c/flashrom/+/84439 Reviewed-by: Sergii Dmytruk Tested-by: build bot (Jenkins) --- doc/classic_cli_manpage.rst | 2 +- flashrom.c | 56 +++++++++++++++++++++++++++++++-- tests/chip.c | 62 ++++++++++++++++++++++++++++++++++--- tests/tests.c | 1 + tests/tests.h | 1 + 5 files changed, 113 insertions(+), 9 deletions(-) diff --git a/doc/classic_cli_manpage.rst b/doc/classic_cli_manpage.rst index f0fd6812c..f0879e472 100644 --- a/doc/classic_cli_manpage.rst +++ b/doc/classic_cli_manpage.rst @@ -317,7 +317,7 @@ All operations involving any chip access (probe/read/write/...) require the ``-p **--progress** - [Experimental feature] Show progress percentage of operations on the standard output. + Show progress percentage of operations on the standard output. **--sacrifice-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 88fa5d9ef..4ae939058 100644 --- a/flashrom.c +++ b/flashrom.c @@ -1243,6 +1243,56 @@ static void setup_progress_from_layout(struct flashctx *flashctx, 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 @@ static int erase_by_layout(struct flashctx *const flashctx) 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 @@ static int write_by_layout(struct flashctx *const flashctx, } 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 ac606639f..25bb8db76 100644 --- a/tests/chip.c +++ b/tests/chip.c @@ -47,7 +47,8 @@ static struct { }; 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 @@ static void progress_callback(struct flashctx *flash) { 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 @@ static const struct flashchip chip_8MiB = { .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 @@ void write_chip_feature_no_erase(void **state) 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 00b46656d..72b4868da 100644 --- a/tests/tests.c +++ b/tests/tests.c @@ -504,6 +504,7 @@ int main(int argc, char *argv[]) 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 dba33fbdf..85c4f52c7 100644 --- a/tests/tests.h +++ b/tests/tests.h @@ -91,6 +91,7 @@ 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_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);