May 2024 - flashrom-gerrit - mail.coreboot.org

[M] Change in flashrom[main]: tests/erase: record the opcode for each erase
by Peter Marheine (Code Review) 13 May '24

13 May '24

Attention is currently required from: Anastasia Klimchuk, Nikolai Artemiev. Peter Marheine has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/82251?usp=email ) Change subject: tests/erase: record the opcode for each erase ...................................................................... Patch Set 4: This change is ready for review. -- To view, visit https://review.coreboot.org/c/flashrom/+/82251?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: main Gerrit-Change-Id: I3983fe42c2e7f06668a1bd20d2db7fafa93b8043 Gerrit-Change-Number: 82251 Gerrit-PatchSet: 4 Gerrit-Owner: Peter Marheine <pmarheine(a)chromium.org> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.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-Attention: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Comment-Date: Mon, 13 May 2024 05:11:23 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

[M] Change in flashrom[main]: erasure_layout: Fix get_flash_region bug
by Peter Marheine (Code Review) 13 May '24

13 May '24

Attention is currently required from: Aarya, Nikolai Artemiev. Hello Nikolai Artemiev, I'd like you to do a code review. Please visit https://review.coreboot.org/c/flashrom/+/82393?usp=email to review the following change. Change subject: erasure_layout: Fix get_flash_region bug ...................................................................... erasure_layout: Fix get_flash_region bug When flash regions are protected, erase could incorrectly erase regions which were meant to be protected by requesting the correct size but using an erase opcode with coarser granularity than desired (for instance using a 16-byte erase command while attempting to erase only 8 bytes). This fixes that by exchanging the nesting of the loops over erase blocks and flash regions. Old: - Select erasefns - Loop over blocks to erase for each selected erasefn - Loop over programmer flash regions within erase block - Erase regions (may fail since selected erasefn will be too big if flash region is smaller than erase block) New: - Loop over programmer flash regions within erase block - Select erasefns within programer flash region - Loop over blocks to erase for each selected erasefn - Erase regions Eraser selection and erasing has also been factored out into a helper function to manage nesting depth. TEST=builds, tests pass BUG=https://ticket.coreboot.org/issues/525 Change-Id: Ic5bf9d0f0e4a94c48d6f6e74e3cb9cccdc7adec9 Signed-off-by: Nikolai Artemiev <nartemiev(a)google.com> Signed-off-by: Peter Marheine <pmarheine(a)chromium.org> --- M erasure_layout.c 1 file changed, 102 insertions(+), 92 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/93/82393/1 diff --git a/erasure_layout.c b/erasure_layout.c index 1dd6b60..cf955e5 100644 --- a/erasure_layout.c +++ b/erasure_layout.c @@ -238,6 +238,80 @@ } } +static int erase_write_helper(struct flashctx *const flashctx, chipoff_t region_start, chipoff_t region_end, + uint8_t *curcontents, uint8_t *newcontents, + struct erase_layout *erase_layout, bool *all_skipped) +{ + const size_t erasefn_count = count_usable_erasers(flashctx); + + // select erase functions + for (size_t i = 0; i < erase_layout[erasefn_count - 1].block_count; i++) { + if (erase_layout[erasefn_count - 1].layout_list[i].start_addr <= region_end && + region_start <= erase_layout[erasefn_count - 1].layout_list[i].end_addr) + select_erase_functions(flashctx, erase_layout, + erasefn_count - 1, i, + curcontents, newcontents, + region_start, region_end); + } + + // erase + for (size_t i = 0; i < erasefn_count; i++) { + for (size_t j = 0; j < erase_layout[i].block_count; j++) { + if (!erase_layout[i].layout_list[j].selected) + continue; + + chipoff_t start_addr = erase_layout[i].layout_list[j].start_addr; + unsigned int block_len = erase_layout[i].layout_list[j].end_addr - start_addr + 1; + const uint8_t erased_value = ERASED_VALUE(flashctx); + // execute erase + erasefunc_t *erasefn = lookup_erase_func_ptr(erase_layout[i].eraser); + + + if (erasefn(flashctx, start_addr, block_len)) { + return -1; + } + if (check_erased_range(flashctx, start_addr, block_len)) { + return -1; + msg_cerr("ERASE FAILED!\n"); + } + + // adjust curcontents + memset(curcontents+start_addr, erased_value, block_len); + // after erase make it unselected again + erase_layout[i].layout_list[j].selected = false; + msg_cdbg("E(%"PRIx32":%"PRIx32")", start_addr, start_addr + block_len - 1); + + *all_skipped = false; + } + } + + // write + unsigned int start_here = 0, len_here = 0, erase_len = region_end - region_start + 1; + while ((len_here = get_next_write(curcontents + region_start + start_here, + newcontents + region_start + start_here, + erase_len - start_here, &start_here, + flashctx->chip->gran))) { + // execute write + int ret = write_flash(flashctx, + newcontents + region_start + start_here, + region_start + start_here, len_here); + if (ret) { + msg_cerr("Write failed at %#x, Abort.\n", region_start + start_here); + return -1; + } + + // adjust curcontents + memcpy(curcontents + region_start + start_here, + newcontents + region_start + start_here, len_here); + msg_cdbg("W(%"PRIx32":%"PRIx32")", region_start + start_here, region_start + start_here + len_here - 1); + + *all_skipped = false; + } + + return 0; +} + + /* * @brief wrapper to use the erase algorithm * @@ -253,12 +327,15 @@ uint8_t *curcontents, uint8_t *newcontents, struct erase_layout *erase_layout, bool *all_skipped) { - const size_t erasefn_count = count_usable_erasers(flashctx); int ret = 0; - size_t i; chipoff_t old_start = region_start, old_end = region_end; align_region(erase_layout, flashctx, &region_start, &region_end); + if (!flashctx->flags.skip_unwritable_regions) { + if (check_for_unwritable_regions(flashctx, region_start, region_end - region_start)) + return -1; + } + uint8_t *old_start_buf = NULL, *old_end_buf = NULL; old_start_buf = (uint8_t *)malloc(old_start - region_start); if (!old_start_buf) { @@ -285,101 +362,34 @@ memcpy(newcontents + end_offset, curcontents + end_offset, region_end - old_end); } - // select erase functions - for (i = 0; i < erase_layout[erasefn_count - 1].block_count; i++) { - if (erase_layout[erasefn_count - 1].layout_list[i].start_addr <= region_end && - region_start <= erase_layout[erasefn_count - 1].layout_list[i].end_addr) - select_erase_functions(flashctx, erase_layout, - erasefn_count - 1, i, - curcontents, newcontents, - region_start, region_end); - } + unsigned int len; + for (unsigned int addr = region_start; addr <= region_end; addr += len) { + struct flash_region region; + get_flash_region(flashctx, addr, &region); + len = min(region_end, region.end - 1) - addr + 1; - for (i = 0; i < erasefn_count; i++) { - for (size_t j = 0; j < erase_layout[i].block_count; j++) { - if (!erase_layout[i].layout_list[j].selected) continue; - - // erase - chipoff_t start_addr = erase_layout[i].layout_list[j].start_addr; - unsigned int block_len = erase_layout[i].layout_list[j].end_addr - start_addr + 1; - const uint8_t erased_value = ERASED_VALUE(flashctx); - // execute erase - erasefunc_t *erasefn = lookup_erase_func_ptr(erase_layout[i].eraser); - - if (!flashctx->flags.skip_unwritable_regions) { - if (check_for_unwritable_regions(flashctx, start_addr, block_len)) - goto _end; - } - - unsigned int len; - for (unsigned int addr = start_addr; addr < start_addr + block_len; addr += len) { - struct flash_region region; - get_flash_region(flashctx, addr, &region); - - len = min(start_addr + block_len, region.end) - addr; - - if (region.write_prot) { - msg_gdbg("%s: cannot erase inside %s " - "region (%#08"PRIx32"..%#08"PRIx32"), skipping range (%#08x..%#08x).\n", - __func__, region.name, - region.start, region.end - 1, - addr, addr + len - 1); - free(region.name); - continue; - } - - msg_gdbg("%s: %s region (%#08"PRIx32"..%#08"PRIx32") is " - "writable, erasing range (%#08x..%#08x).\n", - __func__, region.name, - region.start, region.end - 1, - addr, addr + len - 1); - free(region.name); - - if (erasefn(flashctx, addr, len)) { - ret = -1; - goto _end; - } - if (check_erased_range(flashctx, addr, len)) { - ret = - 1; - msg_cerr("ERASE FAILED!\n"); - goto _end; - } - } - - // adjust curcontents - memset(curcontents+start_addr, erased_value, block_len); - // after erase make it unselected again - erase_layout[i].layout_list[j].selected = false; - msg_cdbg("E(%"PRIx32":%"PRIx32")", start_addr, start_addr + block_len - 1); - - *all_skipped = false; + if (region.write_prot) { + msg_gdbg("%s: cannot erase inside %s " + "region (%#08"PRIx32"..%#08"PRIx32"), skipping range (%#08x..%#08x).\n", + __func__, region.name, + region.start, region.end - 1, + addr, addr + len - 1); + free(region.name); + continue; } - } - // write - unsigned int start_here = 0, len_here = 0, erase_len = region_end - region_start + 1; - while ((len_here = get_next_write(curcontents + region_start + start_here, - newcontents + region_start + start_here, - erase_len - start_here, &start_here, - flashctx->chip->gran))) { - // execute write - ret = write_flash(flashctx, - newcontents + region_start + start_here, - region_start + start_here, len_here); - if (ret) { - msg_cerr("Write failed at %#zx, Abort.\n", i); - ret = -1; + msg_gdbg("%s: %s region (%#08"PRIx32"..%#08"PRIx32") is " + "writable, erasing range (%#08x..%#08x).\n", + __func__, region.name, + region.start, region.end - 1, + addr, addr + len - 1); + free(region.name); + + + ret = erase_write_helper(flashctx, addr, addr + len - 1, curcontents, newcontents, erase_layout, all_skipped); + if (ret) goto _end; - } - - // adjust curcontents - memcpy(curcontents + region_start + start_here, - newcontents + region_start + start_here, len_here); - msg_cdbg("W(%"PRIx32":%"PRIx32")", region_start + start_here, region_start + start_here + len_here - 1); - - *all_skipped = false; } - _end: memcpy(newcontents + region_start, old_start_buf, old_start - region_start); memcpy(newcontents + old_end, old_end_buf, region_end - old_end); -- To view, visit https://review.coreboot.org/c/flashrom/+/82393?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: main Gerrit-Change-Id: Ic5bf9d0f0e4a94c48d6f6e74e3cb9cccdc7adec9 Gerrit-Change-Number: 82393 Gerrit-PatchSet: 1 Gerrit-Owner: Peter Marheine <pmarheine(a)chromium.org> Gerrit-Reviewer: Aarya <aarya.chaumal(a)gmail.com> Gerrit-Reviewer: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Attention: Aarya <aarya.chaumal(a)gmail.com> Gerrit-Attention: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-MessageType: newchange

1 0

[L] Change in flashrom[main]: [WIP] tests: Add tests for erasing chip with protected region
by Peter Marheine (Code Review) 13 May '24

13 May '24

Attention is currently required from: Anastasia Klimchuk. Hello Anastasia Klimchuk, I'd like you to do a code review. Please visit https://review.coreboot.org/c/flashrom/+/82392?usp=email to review the following change. Change subject: [WIP] tests: Add tests for erasing chip with protected region ...................................................................... [WIP] tests: Add tests for erasing chip with protected region Change-Id: Ifd472b623dbbc4d11d6ffee7e5b24e38b1fb562a Signed-off-by: Anastasia Klimchuk <aklm(a)flashrom.org> Signed-off-by: Peter Marheine <pmarheine(a)chromium.org> --- M tests/erase_func_algo.c M tests/tests.c M tests/tests.h 3 files changed, 309 insertions(+), 2 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/92/82392/1 diff --git a/tests/erase_func_algo.c b/tests/erase_func_algo.c index ed9eb04..35f9a16 100644 --- a/tests/erase_func_algo.c +++ b/tests/erase_func_algo.c @@ -49,8 +49,8 @@ uint8_t written_buf[MOCK_CHIP_SIZE]; /* Expected content after write. */ struct erase_invoke eraseblocks_expected[MOCK_CHIP_SIZE]; /* Expected order of eraseblocks invocations. */ unsigned int eraseblocks_expected_ind; /* Expected number of eraseblocks invocations. */ - char erase_test_name[20]; /* Test case display name for testing erase operation. */ - char write_test_name[20]; /* Test case display name for testing write operation. */ + char erase_test_name[40]; /* Test case display name for testing erase operation. */ + char write_test_name[40]; /* Test case display name for testing write operation. */ }; struct all_state { @@ -703,6 +703,145 @@ }, }; + +#define START_PROTECTED_REGION 8 +#define END_PROTECTED_REGION 16 + +/* + * Setup all test cases with protected region. + * Protected region is the same for all test cases, between bytes 8 - 15. + */ +static struct test_case test_cases_protected_region[] = { + { + /* + * Test case #0 + * + * Initial vs written: all 16 bytes are different. + * One layout region for the whole chip. + * Chip with eraseblocks 1, 2, 4, 8, 16. + */ + .chip = &chip_1_2_4_8_16, + .regions = {{0, MIN_REAL_CHIP_SIZE - 1, "whole chip"}}, + .initial_buf = {0x0, 0x1, 0x2, 0x3, 0x4, 0x5, 0x6, 0x7, + 0x8, 0x9, 0xa, 0xb, 0xc, 0xd, 0xe, 0xf}, + .erased_buf = {ERASE_VALUE, ERASE_VALUE, ERASE_VALUE, ERASE_VALUE, + ERASE_VALUE, ERASE_VALUE, ERASE_VALUE, ERASE_VALUE, + 0x8, 0x9, 0xa, 0xb, 0xc, 0xd, 0xe, 0xf}, + .eraseblocks_expected = {{0x0, 0x8}}, + .eraseblocks_expected_ind = 1, + .erase_test_name = "Erase protected region test case #0", + }, { + /* + * Test case #1 + * + * Initial vs written: 9 bytes the same, 7 bytes different. + * Two layout regions each one 8 bytes, which is 1/2 size of chip. + * Chip with eraseblocks 1, 2, 4, 8, 16. + */ + .chip = &chip_1_2_4_8_16, + .regions = {{0, MOCK_CHIP_SIZE/2 - 1, "part1"}, {MOCK_CHIP_SIZE/2, MIN_REAL_CHIP_SIZE - 1, "part2"}}, + .initial_buf = {0xf0, 0xf1, 0xf2, 0xf3, 0xf4, 0xf5, 0xf6, 0xf7, + 0xf8, 0xf9, 0xfa, 0xfb, 0xfc, 0xfd, 0xfe, 0xff}, + .erased_buf = {ERASE_VALUE, ERASE_VALUE, ERASE_VALUE, ERASE_VALUE, + ERASE_VALUE, ERASE_VALUE, ERASE_VALUE, ERASE_VALUE, + 0xf8, 0xf9, 0xfa, 0xfb, 0xfc, 0xfd, 0xfe, 0xff}, + .eraseblocks_expected = {{0x0, 0x8}}, + .eraseblocks_expected_ind = 1, + .erase_test_name = "Erase protected region test case #1", + }, { + /* + * Test case #2 + * + * Initial vs written: 6 bytes the same, 4 bytes different, 4 bytes the same, 2 bytes different. + * Two layout regions 11 and 5 bytes each. + * Chip with eraseblocks 1, 2, 4, 8, 16. + */ + .chip = &chip_1_2_4_8_16, + .regions = {{0, 10, "odd1"}, {11, 15, "odd2"}, {MOCK_CHIP_SIZE, MIN_REAL_CHIP_SIZE - 1, "longtail"}}, + .initial_buf = {0xff, 0xff, 0x0, 0xff, 0x0, 0xff, 0x0, 0xff, + 0x0, 0xff, 0x0, 0xff, 0xff, 0xff, 0xff, 0xff}, + .erased_buf = {ERASE_VALUE, ERASE_VALUE, ERASE_VALUE, ERASE_VALUE, + ERASE_VALUE, ERASE_VALUE, ERASE_VALUE, ERASE_VALUE, + 0x0, 0xff, 0x0, 0xff, 0xff, 0xff, 0xff, 0xff}, + .eraseblocks_expected = {{0x0, 0x8}}, + .eraseblocks_expected_ind = 1, + .erase_test_name = "Erase protected region test case #2", + }, { + /* + * Test case #3 + * + * Initial vs written: all 16 bytes are different. + * Layout with unaligned regions 2+4+9+1b which require use of the 1-byte erase block. + * Chip with eraseblocks 1, 8, 16. + */ + .chip = &chip_1_8_16, + .regions = {{0, 1, "reg2"}, {2, 5, "reg4"}, {6, 14, "reg9"}, + {15, MIN_REAL_CHIP_SIZE - 1, "reg1"}}, + .initial_buf = {0x4, 0x4, 0x5, 0x5, 0x5, 0x5, 0x6, 0x6, + 0x6, 0x6, 0x6, 0x6, 0x6, 0x6, 0x6, 0x7}, + .erased_buf = {ERASE_VALUE, ERASE_VALUE, ERASE_VALUE, ERASE_VALUE, + ERASE_VALUE, ERASE_VALUE, ERASE_VALUE, ERASE_VALUE, + 0x6, 0x6, 0x6, 0x6, 0x6, 0x6, 0x6, 0x7}, + .eraseblocks_expected = { + {6, 1, TEST_ERASE_INJECTOR_1}, + {7, 1, TEST_ERASE_INJECTOR_1}, + {2, 1, TEST_ERASE_INJECTOR_1}, + {3, 1, TEST_ERASE_INJECTOR_1}, + {4, 1, TEST_ERASE_INJECTOR_1}, + {5, 1, TEST_ERASE_INJECTOR_1}, + {0, 1, TEST_ERASE_INJECTOR_1}, + {1, 1, TEST_ERASE_INJECTOR_1}, + }, + .eraseblocks_expected_ind = 8, + .erase_test_name = "Erase protected region test case #3", + }, { + /* + * Test case #4 + * + * Initial vs written: all 16 bytes are different. + * Layout with unaligned region 3+13b which require use of the 1-byte erase block. + * Chip with eraseblocks 1, 8, 16. + */ + .chip = &chip_1_8_16, + .regions = {{0, 2, "reg3"}, {3, MIN_REAL_CHIP_SIZE - 1, "tail"}}, + .initial_buf = {0x4, 0x4, 0x4, 0x6, 0x6, 0x6, 0x6, 0x6, + 0x6, 0x6, 0x6, 0x6, 0x6, 0x6, 0x6, 0x6}, + .erased_buf = {ERASE_VALUE, ERASE_VALUE, ERASE_VALUE, ERASE_VALUE, + ERASE_VALUE, ERASE_VALUE, ERASE_VALUE, ERASE_VALUE, + 0x6, 0x6, 0x6, 0x6, 0x6, 0x6, 0x6, 0x6}, + .eraseblocks_expected = { + {3, 1, TEST_ERASE_INJECTOR_1}, + {4, 1, TEST_ERASE_INJECTOR_1}, + {5, 1, TEST_ERASE_INJECTOR_1}, + {6, 1, TEST_ERASE_INJECTOR_1}, + {7, 1, TEST_ERASE_INJECTOR_1}, + {0, 1, TEST_ERASE_INJECTOR_1}, + {1, 1, TEST_ERASE_INJECTOR_1}, + {2, 1, TEST_ERASE_INJECTOR_1}, + }, + .eraseblocks_expected_ind = 8, + .erase_test_name = "Erase protected region test case #4", + }, { + /* + * Test case #5 + * + * Initial vs written: all 16 bytes are different. + * Layout with unaligned region 9+7b. + * Chip with eraseblocks 8, 16. + */ + .chip = &chip_8_16, + .regions = {{0, 8, "reg9"}, {9, MIN_REAL_CHIP_SIZE - 1, "tail"}}, + .initial_buf = {0x4, 0x4, 0x4, 0x4, 0x4, 0x4, 0x4, 0x4, + 0x4, 0x6, 0x6, 0x6, 0x6, 0x6, 0x6, 0x6}, + .erased_buf = {ERASE_VALUE, ERASE_VALUE, ERASE_VALUE, ERASE_VALUE, + ERASE_VALUE, ERASE_VALUE, ERASE_VALUE, ERASE_VALUE, + 0x4, 0x6, 0x6, 0x6, 0x6, 0x6, 0x6, 0x6}, + .eraseblocks_expected = {{0x0, 0x8}}, + .eraseblocks_expected_ind = 1, + .erase_test_name = "Erase test case #5", + }, +}; + static int setup(void **state) { struct test_case *current_test_case = *state; g_state.current_test_case = current_test_case; @@ -746,6 +885,29 @@ } /* + * Creates the array of tests for each test case in test_cases_protected_region[]. + * The caller needs to free the allocated memory. + */ +struct CMUnitTest *get_erase_protected_region_algo_tests(size_t *num_tests) { + const size_t test_cases_num = ARRAY_SIZE(test_cases_protected_region); + + struct CMUnitTest *all_cases = calloc(test_cases_num, sizeof(struct CMUnitTest)); + *num_tests = test_cases_num; + + for (size_t i = 0; i < test_cases_num; i++) { + all_cases[i] = (struct CMUnitTest) { + .name = test_cases_protected_region[i].erase_test_name, + .setup_func = setup, + .teardown_func = teardown, + .initial_state = &test_cases_protected_region[i], + .test_func = erase_unwritable_regions_skipflag_on_test_success, + }; + } + + return all_cases; +} + +/* * This function is invoked for every test case in test_cases[], * current test case is passed as an argument. */ @@ -845,3 +1007,135 @@ assert_int_equal(0, all_write_test_result); } + +static void get_protected_region(const struct flashctx *flash, unsigned int addr, struct flash_region *region) +{ + if (addr < 20) + printf("Inside test_get_flash_region for addr=0x%x\n", addr); + + if (addr < START_PROTECTED_REGION) { + region->name = strdup("not protected"); + region->start = 0; + region->end = START_PROTECTED_REGION; + region->read_prot = false; + region->write_prot = false; + } else if (addr < END_PROTECTED_REGION) { + region->name = strdup("protected"); + region->start = START_PROTECTED_REGION; + region->end = END_PROTECTED_REGION; + region->read_prot = true; + region->write_prot = true; + } else { + region->name = strdup("tail"); + region->start = 16; + region->end = flashrom_flash_getsize(flash); + region->read_prot = false; + region->write_prot = false; + } +} + +static int block_erase_chip_with_protected_region(struct flashctx *flash, unsigned int blockaddr, unsigned int blocklen) +{ + if (blockaddr + blocklen <= MOCK_CHIP_SIZE) { + LOG_ERASE_FUNC; + + /* Register eraseblock invocation. */ + g_state.eraseblocks_actual[g_state.eraseblocks_actual_ind].blocklen + = blocklen; + g_state.eraseblocks_actual[g_state.eraseblocks_actual_ind].blockaddr + = blockaddr; + g_state.eraseblocks_actual_ind++; + } + + assert_in_range(blockaddr + blocklen, 0, MIN_REAL_CHIP_SIZE); + + // Check we are not trying to erase protected region. This should not happen, + // because the logic should handle protected regions and never invoke erasefn + // for them. If this happens, means there is a bug in erasure logic, and test fails. + // + // Note: returning 1 instead of assert, so that the flow goes back to erasure code + // to clean up the memory after failed erase. Memory leaks are also tested by unit tests. + if ((blockaddr >= START_PROTECTED_REGION && blockaddr < END_PROTECTED_REGION) + || (blockaddr + blocklen - 1 >= START_PROTECTED_REGION + && blockaddr + blocklen - 1 < END_PROTECTED_REGION)) + return 1; + + memset(&g_state.buf[blockaddr], ERASE_VALUE, blocklen); + return 0; +} + +void erase_unwritable_regions_skipflag_on_test_success(void **state) +{ + struct test_case* current_test_case = *state; + + int all_erase_tests_result = 0; + struct flashrom_flashctx flashctx = { 0 }; + const char *param = ""; /* Default values for all params. */ + + struct flashrom_layout *layout; + + setup_chip(&flashctx, &layout, param, current_test_case); + + // This test needs special block erase to emulate protected regions. + g_test_erase_injector = block_erase_chip_with_protected_region; + + flashrom_flag_set(&flashctx, FLASHROM_FLAG_SKIP_UNWRITABLE_REGIONS, true); + + // We use dummyflasher programmer in tests, but for this test we need to + // replace dummyflasher's default get_region fn with test one. + // The rest of master struct is fine for this test. + // Note dummyflasher registers multiple masters by default, so replace + // get_region for each of them. + flashctx.mst->spi.get_region = &get_protected_region; + flashctx.mst->opaque.get_region = &get_protected_region; + + 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); + + 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); + + all_erase_tests_result |= ret; + all_erase_tests_result |= !chip_erased; + all_erase_tests_result |= !eraseblocks_in_order; + all_erase_tests_result |= !eraseblocks_invocations; + + teardown_chip(&layout); + + assert_int_equal(0, all_erase_tests_result); +} + +void erase_unwritable_regions_skipflag_off_test_success(void **state) +{ +// flashrom_flag_set(&flashctx, FLASHROM_FLAG_SKIP_UNWRITABLE_REGIONS, false); +} diff --git a/tests/tests.c b/tests/tests.c index c38a66c..bff24e5 100644 --- a/tests/tests.c +++ b/tests/tests.c @@ -509,6 +509,16 @@ ret |= _cmocka_run_group_tests("erase_func_algo.c tests", erase_func_algo_tests, n_erase_tests, NULL, NULL); free(erase_func_algo_tests); + size_t n_erase_protected_region_tests; + struct CMUnitTest *erase_protected_region_algo_tests + = get_erase_protected_region_algo_tests(&n_erase_protected_region_tests); + ret |= _cmocka_run_group_tests("erase_func_algo.c protected region tests", + erase_protected_region_algo_tests, + n_erase_protected_region_tests, + NULL, + NULL); + free(erase_protected_region_algo_tests); + // Write-protect group should run last. const struct CMUnitTest chip_wp_tests[] = { cmocka_unit_test(invalid_wp_range_dummyflasher_test_success), diff --git a/tests/tests.h b/tests/tests.h index af5f968..7f8fc3c 100644 --- a/tests/tests.h +++ b/tests/tests.h @@ -107,7 +107,10 @@ /* erase_func_algo.c */ struct CMUnitTest *get_erase_func_algo_tests(size_t *num_tests); +struct CMUnitTest *get_erase_protected_region_algo_tests(size_t *num_tests); void erase_function_algo_test_success(void **state); void write_function_algo_test_success(void **state); +void erase_unwritable_regions_skipflag_off_test_success(void **state); +void erase_unwritable_regions_skipflag_on_test_success(void **state); #endif /* TESTS_H */ -- To view, visit https://review.coreboot.org/c/flashrom/+/82392?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: main Gerrit-Change-Id: Ifd472b623dbbc4d11d6ffee7e5b24e38b1fb562a Gerrit-Change-Number: 82392 Gerrit-PatchSet: 1 Gerrit-Owner: Peter Marheine <pmarheine(a)chromium.org> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-MessageType: newchange

1 0

[XS] Change in flashrom[main]: flashrom_udev.rules: Add rule for CH347
by Anastasia Klimchuk (Code Review) 13 May '24

13 May '24

Anastasia Klimchuk has submitted this change. ( https://review.coreboot.org/c/flashrom/+/82162?usp=email ) Change subject: flashrom_udev.rules: Add rule for CH347 ...................................................................... flashrom_udev.rules: Add rule for CH347 This allows the CH347 programmer to be used without root permissions. Signed-off-by: Nicholas Chin <nic.c3.14(a)gmail.com> Change-Id: Ia83fa08f6d7c2f449b1a5c0c387c6d4368b99e3a Reviewed-on: https://review.coreboot.org/c/flashrom/+/82162 Reviewed-by: Anastasia Klimchuk <aklm(a)chromium.org> Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org> --- M util/flashrom_udev.rules 1 file changed, 3 insertions(+), 0 deletions(-) Approvals: build bot (Jenkins): Verified Anastasia Klimchuk: Looks good to me, approved diff --git a/util/flashrom_udev.rules b/util/flashrom_udev.rules index 9fd3330..331fee6 100644 --- a/util/flashrom_udev.rules +++ b/util/flashrom_udev.rules @@ -93,4 +93,7 @@ # Winchiphead (WCH) CH341a based programmer ATTRS{idVendor}=="1a86", ATTRS{idProduct}=="5512", MODE="664", GROUP="plugdev" +# Winchiphead (WCH) CH347 based programmer +ATTRS{idVendor}=="1a86", ATTRS{idProduct}=="55db", MODE="664", GROUP="plugdev" + LABEL="flashrom_rules_end" -- To view, visit https://review.coreboot.org/c/flashrom/+/82162?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: main Gerrit-Change-Id: Ia83fa08f6d7c2f449b1a5c0c387c6d4368b99e3a Gerrit-Change-Number: 82162 Gerrit-PatchSet: 2 Gerrit-Owner: Nicholas Chin <nic.c3.14(a)gmail.com> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-MessageType: merged

1 0

[M] Change in flashrom[main]: ch347_spi: Add driver support for CH347F packaging
by ZhiYuanNJ (Code Review) 13 May '24

13 May '24

Attention is currently required from: Nicholas Chin, Paul Menzel. ZhiYuanNJ has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/82193?usp=email ) Change subject: ch347_spi: Add driver support for CH347F packaging ...................................................................... Patch Set 9: (1 comment) File ch347_spi.c: PS2: > Yes, please move the code for the device speed settings to a new commit and push another patch to Ge […] Done -- To view, visit https://review.coreboot.org/c/flashrom/+/82193?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: main Gerrit-Change-Id: I693baf1a0d9dc20757f56fba626b5f5ad20f71dd Gerrit-Change-Number: 82193 Gerrit-PatchSet: 9 Gerrit-Owner: ZhiYuanNJ Gerrit-Reviewer: Nicholas Chin <nic.c3.14(a)gmail.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Nicholas Chin <nic.c3.14(a)gmail.com> Gerrit-Comment-Date: Mon, 13 May 2024 01:49:49 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Nicholas Chin <nic.c3.14(a)gmail.com> Comment-In-Reply-To: ZhiYuanNJ Gerrit-MessageType: comment

1 0

[M] Change in flashrom[main]: ch347_spi: Add driver support for CH347F packaging
by ZhiYuanNJ (Code Review) 13 May '24

13 May '24

Attention is currently required from: Paul Menzel, ZhiYuanNJ. Hello Nicholas Chin, build bot (Jenkins), I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/flashrom/+/82193?usp=email to look at the new patch set (#9). Change subject: ch347_spi: Add driver support for CH347F packaging ...................................................................... ch347_spi: Add driver support for CH347F packaging CH347F can work simultaneously with SPI, I2C and other signals. CH347 introduce is available at the following URL: https://www.wch-ic.com/products/CH347.html Change-Id: I693baf1a0d9dc20757f56fba626b5f5ad20f71dd Signed-off-by: ZhiYuanNJ Liu <871238103(a)qq.com> --- M ch347_spi.c 1 file changed, 31 insertions(+), 22 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/93/82193/9 -- To view, visit https://review.coreboot.org/c/flashrom/+/82193?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: main Gerrit-Change-Id: I693baf1a0d9dc20757f56fba626b5f5ad20f71dd Gerrit-Change-Number: 82193 Gerrit-PatchSet: 9 Gerrit-Owner: ZhiYuanNJ Gerrit-Reviewer: Nicholas Chin <nic.c3.14(a)gmail.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: ZhiYuanNJ Gerrit-MessageType: newpatchset

1 0

[S] Change in flashrom[main]: doc: Fix index files in Supported HW section
by Peter Marheine (Code Review) 13 May '24

13 May '24

Attention is currently required from: Anastasia Klimchuk, Thomas Heijligen. Peter Marheine has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/82196?usp=email ) Change subject: doc: Fix index files in Supported HW section ...................................................................... Patch Set 2: Code-Review+2 -- To view, visit https://review.coreboot.org/c/flashrom/+/82196?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: main Gerrit-Change-Id: Ia15e9766cce6f19be1e69fbb1236a327ae3d57b3 Gerrit-Change-Number: 82196 Gerrit-PatchSet: 2 Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Peter Marheine <pmarheine(a)chromium.org> Gerrit-Reviewer: Sydney <git(a)funkeleinhorn.com> Gerrit-Reviewer: Thomas Heijligen <src(a)posteo.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Thomas Heijligen <src(a)posteo.de> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Comment-Date: Mon, 13 May 2024 01:01:25 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

[S] Change in flashrom[main]: flashchips: Add support for Boya B25Q64AS
by Anastasia Klimchuk (Code Review) 13 May '24

13 May '24

Attention is currently required from: Malcolm Boyes, Nikolai Artemiev, Stefan Reinauer. Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/82259?usp=email ) Change subject: flashchips: Add support for Boya B25Q64AS ...................................................................... Patch Set 2: Code-Review+2 (1 comment) Commit Message: https://review.coreboot.org/c/flashrom/+/82259/comment/ed71d257_3446a0e0 : PS2, Line 14: boyesm Sorry I missed this: I think it's more standard to put your name here (Malcolm Boyes). So you will have name and then <email> -- To view, visit https://review.coreboot.org/c/flashrom/+/82259?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: main Gerrit-Change-Id: I05ecf2b118902db974544d86e023a348912371dd Gerrit-Change-Number: 82259 Gerrit-PatchSet: 2 Gerrit-Owner: Malcolm Boyes <boyesmalcolm(a)gmail.com> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Reviewer: Stefan Reinauer <stefan.reinauer(a)coreboot.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Stefan Reinauer <stefan.reinauer(a)coreboot.org> Gerrit-Attention: Malcolm Boyes <boyesmalcolm(a)gmail.com> Gerrit-Attention: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Comment-Date: Mon, 13 May 2024 00:42:39 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

[L] Change in flashrom[main]: [WIP] tests: Add tests for erasing chip with protected region
by Anastasia Klimchuk (Code Review) 12 May '24

12 May '24

Attention is currently required from: Nikolai Artemiev, Peter Marheine. Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/81664?usp=email ) Change subject: [WIP] tests: Add tests for erasing chip with protected region ...................................................................... Patch Set 1: (1 comment) File tests/erase_func_algo.c: https://review.coreboot.org/c/flashrom/+/81664/comment/a1e4ecad_66445329 : PS1, Line 130: TEST_ERASE_INJECTOR > Correction: "without CB:81385". I have added comment into CB:82251 -- To view, visit https://review.coreboot.org/c/flashrom/+/81664?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: main Gerrit-Change-Id: I1e7cfd027d37437c917c054f30f3be59b76b0069 Gerrit-Change-Number: 81664 Gerrit-PatchSet: 1 Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Peter Marheine <pmarheine(a)chromium.org> Gerrit-Attention: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Attention: Peter Marheine <pmarheine(a)chromium.org> Gerrit-Comment-Date: Sun, 12 May 2024 22:23:54 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org> Comment-In-Reply-To: Peter Marheine <pmarheine(a)chromium.org> Gerrit-MessageType: comment

1 0

[S] Change in flashrom[main]: flashchips: Add support for Boya B25Q64AS
by Malcolm Boyes (Code Review) 12 May '24

12 May '24

Attention is currently required from: Anastasia Klimchuk, Nikolai Artemiev, Stefan Reinauer. Malcolm Boyes has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/82259?usp=email ) Change subject: flashchips: Add support for Boya B25Q64AS ...................................................................... Patch Set 2: (2 comments) Patchset: PS2: Ok Anastasia, I have changed the .unlock property in this profile to what you suggested. And yes, I didn't specifically test the write protection functionality in this profile. File flashchips.c: https://review.coreboot.org/c/flashrom/+/82259/comment/1f84947f_a68a2f19 : PS1, Line 3640: SPI_DISABLE_BLOCKPROTECT_AT25FS040 > I think SPI_DISABLE_BLOCKPROTECT_BP4_SRWD is more correct in this case […] Done -- To view, visit https://review.coreboot.org/c/flashrom/+/82259?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: main Gerrit-Change-Id: I05ecf2b118902db974544d86e023a348912371dd Gerrit-Change-Number: 82259 Gerrit-PatchSet: 2 Gerrit-Owner: Malcolm Boyes <boyesmalcolm(a)gmail.com> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Reviewer: Stefan Reinauer <stefan.reinauer(a)coreboot.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Stefan Reinauer <stefan.reinauer(a)coreboot.org> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Attention: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Comment-Date: Sun, 12 May 2024 18:51:50 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-MessageType: comment

1 0

2024

2023

2022

2021

2020

2019

2018

2017

flashrom-gerrit May 2024