From 008919aa6968b515598f71dc0f1360cb42893a6a Mon Sep 17 00:00:00 2001 From: Anastasia Klimchuk Date: Sat, 9 Aug 2025 19:52:41 +1000 Subject: [PATCH] Erase should respect --noverify option This commit adds check for the flag `verify_after_write` after erase operation and does not perfom verification if the flag disabled. From command line, this is set by --noverify option. libflashrom flag is FLASHROM_FLAG_VERIFY_AFTER_WRITE Default stays the same, by default verification is performed. This commit also adds test for the scenario. Previously, erase operation ignored --noverify option and always performed verification. For more details, see Ticket: https://ticket.coreboot.org/issues/520 Change-Id: I9f6cb7210f4dcdc32870f9096657a08b12e77c7f Signed-off-by: Anastasia Klimchuk Reviewed-on: https://review.coreboot.org/c/flashrom/+/88734 Reviewed-by: Stefan Reinauer Tested-by: build bot (Jenkins) Reviewed-by: Carly Zlabek Reviewed-by: Matt DeVillier Reviewed-by: Vincent Fazio --- doc/classic_cli_manpage.rst | 4 +- erasure_layout.c | 3 +- tests/erase_func_algo.c | 83 ++++++++++++++++++++++++++++++++++++- 3 files changed, 86 insertions(+), 4 deletions(-) diff --git a/doc/classic_cli_manpage.rst b/doc/classic_cli_manpage.rst index 043bc1583..1d398c14b 100644 --- a/doc/classic_cli_manpage.rst +++ b/doc/classic_cli_manpage.rst @@ -72,14 +72,14 @@ All operations involving any chip access (probe/read/write/...) require the ``-p **-n, --noverify** - Skip the automatic verification of flash ROM contents after writing. Using this option is **not** recommended, + Skip the automatic verification of flash ROM contents after writing or erasing. Using this option is **not** recommended, you should only use it if you know what you are doing and if you feel that the time for verification takes too long. Typical usage is:: flashrom -p prog -n -w - This option is only useful in combination with ``--write``. + This option is only useful in combination with ``--write`` or ``--erase``. **-N, --noverify-all** diff --git a/erasure_layout.c b/erasure_layout.c index 9b99c68b3..c0d25bdb6 100644 --- a/erasure_layout.c +++ b/erasure_layout.c @@ -301,7 +301,8 @@ static int erase_write_helper(struct flashctx *const flashctx, chipoff_t region_ if (erasefn(flashctx, start_addr, block_len)) { return -1; } - if (check_erased_range(flashctx, start_addr, block_len)) { + if (flashctx->flags.verify_after_write + && check_erased_range(flashctx, start_addr, block_len)) { msg_cerr("ERASE FAILED!\n"); return -1; } diff --git a/tests/erase_func_algo.c b/tests/erase_func_algo.c index 0d37b077b..595072f9e 100644 --- a/tests/erase_func_algo.c +++ b/tests/erase_func_algo.c @@ -1194,6 +1194,7 @@ static void test_erase_fails_for_unwritable_region(void **); static void test_write_with_sacrifice_ratio50(void **); static void erase_unwritable_regions_skipflag_on_test_success(void **); static void write_unwritable_regions_skipflag_on_test_success(void **); +static void test_erase_with_noverify(void **); /* * Creates the array of tests for each test case in test_cases_protected_region[]. @@ -1201,7 +1202,7 @@ static void write_unwritable_regions_skipflag_on_test_success(void **); */ struct CMUnitTest *get_erase_protected_region_algo_tests(size_t *num_tests) { const size_t num_parameterized = ARRAY_SIZE(test_cases_protected_region); - const size_t num_unparameterized = 2; + const size_t num_unparameterized = 3; // Twice the number of parameterized test cases, because each test case is run twice: // for erase and write. const size_t num_cases = num_parameterized * 2 + num_unparameterized; @@ -1237,6 +1238,10 @@ struct CMUnitTest *get_erase_protected_region_algo_tests(size_t *num_tests) { .name = "write with sacrifice ratio 50", .test_func = test_write_with_sacrifice_ratio50, }, + (const struct CMUnitTest) { + .name = "erase with noverify", + .test_func = test_erase_with_noverify, + }, }, sizeof(*all_cases) * num_unparameterized ); @@ -1783,3 +1788,79 @@ static void test_write_with_sacrifice_ratio50(void **state) { assert_int_equal(0, all_write_test_result); } + +static void test_erase_with_noverify(void **state) +{ + /* The exact test case doesn't matter that much, because we only want to test + * that verification was NOT invoked. */ + struct test_case* current_test_case = &test_cases[2]; + + int all_erase_tests_result = 0; + struct flashrom_flashctx flashctx = { 0 }; + const char *param = ""; /* Default values for all params. */ + + struct flashrom_layout *layout; + + const chipoff_t verify_end_boundary = setup_chip(&flashctx, &layout, param, current_test_case); + + /* We want to test that verify NOT performed. */ + flashrom_flag_set(&flashctx, FLASHROM_FLAG_VERIFY_AFTER_WRITE, false); + + printf("%s started.\n", current_test_case->erase_test_name); + int ret = flashrom_flash_erase(&flashctx); + printf("%s returned %d.\n", current_test_case->erase_test_name, ret); + + int chip_erased = !memcmp(g_state.buf, current_test_case->erased_buf, MOCK_CHIP_SIZE); + + int eraseblocks_in_order = !memcmp(g_state.eraseblocks_actual, + current_test_case->eraseblocks_expected, + current_test_case->eraseblocks_expected_ind * sizeof(struct erase_invoke)); + + int eraseblocks_invocations = (g_state.eraseblocks_actual_ind == + current_test_case->eraseblocks_expected_ind); + + int chip_verified = 0; + for (unsigned int i = 0; i <= verify_end_boundary; i++) + if (g_state.was_modified[i] && g_state.was_verified[i]) { + chip_verified = 1; /* byte was verified, but we disabled verification! */ + printf("Error: byte 0x%x was verified, but verification is disabled\n", i); + } + + if (chip_erased) + printf("Erased chip memory state for %s is CORRECT\n", + current_test_case->erase_test_name); + else + printf("Erased chip memory state for %s is WRONG\n", + current_test_case->erase_test_name); + + if (eraseblocks_in_order) + printf("Eraseblocks order of invocation for %s is CORRECT\n", + current_test_case->erase_test_name); + else + printf("Eraseblocks order of invocation for %s is WRONG\n", + current_test_case->erase_test_name); + + if (eraseblocks_invocations) + printf("Eraseblocks number of invocations for %s is CORRECT\n", + current_test_case->erase_test_name); + else + printf("Eraseblocks number of invocations for %s is WRONG, expected %d actual %d\n", + current_test_case->erase_test_name, + current_test_case->eraseblocks_expected_ind, + g_state.eraseblocks_actual_ind); + + if (!chip_verified) + printf("Verification was not performed, as it was disabled.\n"); + else + printf("ERROR: verify operation was disabled, but it was performed.\n"); + + all_erase_tests_result |= ret; + all_erase_tests_result |= !chip_erased; + all_erase_tests_result |= !eraseblocks_in_order; + all_erase_tests_result |= !eraseblocks_invocations; + all_erase_tests_result |= chip_verified; + + teardown_chip(&layout); + + assert_int_equal(0, all_erase_tests_result); +}