- flashrom-gerrit - mail.coreboot.org

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

04 Apr '24

Anastasia Klimchuk has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/81664?usp=email ) Change subject: [WIP] tests: Add tests for erasing chip with protected region ...................................................................... [WIP] tests: Add tests for erasing chip with protected region Change-Id: I1e7cfd027d37437c917c054f30f3be59b76b0069 Signed-off-by: Anastasia Klimchuk <aklm(a)flashrom.org> --- M tests/erase_func_algo.c M tests/tests.c M tests/tests.h 3 files changed, 291 insertions(+), 2 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/64/81664/1 diff --git a/tests/erase_func_algo.c b/tests/erase_func_algo.c index ed9eb04..a600de2 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,127 @@ }, }; + +#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 are smaller than the smallest eraseblock. + * Chip with eraseblocks 8, 16. + */ + .chip = &chip_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 = {{0x0, 0x8}, {0x0, 0x8}, {0x0, 0x8}}, + .eraseblocks_expected_ind = 3, + .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 are smaller than the smallest eraseblock. + * Chip with eraseblocks 8, 16. + */ + .chip = &chip_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 = {{0x0, 0x8}, {0x0, 0x8}}, + .eraseblocks_expected_ind = 2, + .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 +867,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 +989,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/+/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-MessageType: newchange

1 0

[S] Change in flashrom[main]: erasure_layout: do not copy region buffers if they are null
by Alexander Goncharov (Code Review) 03 Apr '24

03 Apr '24

Attention is currently required from: Aarya, Anastasia Klimchuk, Nikolai Artemiev. Alexander Goncharov has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/81548?usp=email ) Change subject: erasure_layout: do not copy region buffers if they are null ...................................................................... Patch Set 1: (1 comment) File erasure_layout.c: https://review.coreboot.org/c/flashrom/+/81548/comment/e1b115ea_fb024bc5 : PS1, Line 263: old_start_buf = (uint8_t *)malloc(old_start - region_start); : if (!old_start_buf) { : msg_cerr("Not enough memory!\n"); : ret = -1; : goto _end; : } : old_end_buf = (uint8_t *)malloc(region_end - old_end); : if (!old_end_buf) { : msg_cerr("Not enough memory!\n"); : ret = -1; : goto _end; : } > Oh I think this piece is also somewhat relevant to the patch. […] Thanks for the details, Anastasia! Yeah, I will dive deeper to see possible solutions and understand code better. I have a few thoughts on how to rewrite it, but I need to test them. -- To view, visit https://review.coreboot.org/c/flashrom/+/81548?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: I99aad4119f9c11bea75e3419926cc833bc1f68c5 Gerrit-Change-Number: 81548 Gerrit-PatchSet: 1 Gerrit-Owner: Alexander Goncharov <chat(a)joursoir.net> Gerrit-Reviewer: Aarya <aarya.chaumal(a)gmail.com> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Nikolai Artemiev <nartemiev(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Nikolai Artemiev <nartemiev(a)chromium.org> Gerrit-Attention: Aarya <aarya.chaumal(a)gmail.com> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Comment-Date: Wed, 03 Apr 2024 16:07:35 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-MessageType: comment

1 0

[S] Change in flashrom[main]: flashchips: Add support for MXIC MX25L3273F
by Anastasia Klimchuk (Code Review) 03 Apr '24

03 Apr '24

Attention is currently required from: DZ, Nikolai Artemiev, Stefan Reinauer. Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/81562?usp=email ) Change subject: flashchips: Add support for MXIC MX25L3273F ...................................................................... Patch Set 7: Code-Review+1 (1 comment) File flashchips.c: https://review.coreboot.org/c/flashrom/+/81562/comment/75e0403f_84a2a4f9 : PS7, Line 9522: 1024B Same as other patch: datasheet says 4K-bit secured OTP, so maybe 512B ? -- To view, visit https://review.coreboot.org/c/flashrom/+/81562?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: I4adaaa796d1db34702e7b0ed8e6fb167a3a5f6d7 Gerrit-Change-Number: 81562 Gerrit-PatchSet: 7 Gerrit-Owner: DZ <danielzhang(a)mxic.com.cn> 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: DZ <danielzhang(a)mxic.com.cn> Gerrit-Attention: Stefan Reinauer <stefan.reinauer(a)coreboot.org> Gerrit-Attention: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Comment-Date: Wed, 03 Apr 2024 08:24:17 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

[S] Change in flashrom[main]: flashchips: Add support for MXIC MX25L3239E
by Anastasia Klimchuk (Code Review) 03 Apr '24

03 Apr '24

Attention is currently required from: DZ, Nikolai Artemiev, Stefan Reinauer. Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/81563?usp=email ) Change subject: flashchips: Add support for MXIC MX25L3239E ...................................................................... Patch Set 3: Code-Review+1 (1 comment) File flashchips.c: https://review.coreboot.org/c/flashrom/+/81563/comment/5854d838_d299ecee : PS3, Line 9523: 1024B I found it is "4K-bit secured OTP" in datasheet, so maybe twice less? -- To view, visit https://review.coreboot.org/c/flashrom/+/81563?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: Ic7a848028fe937deb1bf83ef2a9dddf1330334b6 Gerrit-Change-Number: 81563 Gerrit-PatchSet: 3 Gerrit-Owner: DZ <danielzhang(a)mxic.com.cn> 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: DZ <danielzhang(a)mxic.com.cn> Gerrit-Attention: Stefan Reinauer <stefan.reinauer(a)coreboot.org> Gerrit-Attention: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Comment-Date: Wed, 03 Apr 2024 07:59:11 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

[L] Change in flashrom[main]: udelay: only use OS time for delays, except on DOS
by Peter Marheine (Code Review) 03 Apr '24

03 Apr '24

Attention is currently required from: Anastasia Klimchuk, Brian Norris, Thomas Heijligen. Hello Anastasia Klimchuk, Brian Norris, Thomas Heijligen, build bot (Jenkins), I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/flashrom/+/81545?usp=email to look at the new patch set (#7). Change subject: udelay: only use OS time for delays, except on DOS ...................................................................... udelay: only use OS time for delays, except on DOS As proposed on the mailing list ("RFC: remove the calibrated delay loop"), this removes the calibrated delay loop and uses OS-based timing functions for all delays because the calibrated delay loop can delay for shorter times than intended. When sleeping this now uses nanosleep() unconditionally, since usleep was only used on DOS (where DJGPP lacks nanosleep). When busy-looping, it uses clock_gettime() with CLOCK_MONOTONIC or CLOCK_REALTIME depending on availability, and gettimeofday() otherwise. The calibrated delay loop is retained for DOS only, because timer resolution on DJGPP is only about 50 milliseconds. Since typical delays in flashrom are around 10 microseconds, using OS timing there would regress performance by around 500x. The old implementation is reused with some branches removed based on the knowledge that timer resolution will not be better than about 50 milliseconds. Tested by reading and writing flash on several Intel and AMD systems: * Lenovo P920 (Intel C620, read/verify only) * "nissa" chromebook (Intel Alder Lake-N) * "zork" chromebook (AMD Zen+) Signed-off-by: Peter Marheine <pmarheine(a)chromium.org> Change-Id: I7ac5450d194a475143698d65d64d8bcd2fd25e3f --- M Makefile M include/programmer.h M libflashrom.c M meson.build M tests/meson.build M tests/tests.c M tests/tests.h A tests/udelay.c M udelay.c A udelay_dos.c 10 files changed, 261 insertions(+), 172 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/45/81545/7 -- To view, visit https://review.coreboot.org/c/flashrom/+/81545?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: I7ac5450d194a475143698d65d64d8bcd2fd25e3f Gerrit-Change-Number: 81545 Gerrit-PatchSet: 7 Gerrit-Owner: Peter Marheine <pmarheine(a)chromium.org> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Brian Norris <briannorris(a)chromium.org> Gerrit-Reviewer: Thomas Heijligen <src(a)posteo.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Brian Norris <briannorris(a)chromium.org> Gerrit-Attention: Thomas Heijligen <src(a)posteo.de> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-MessageType: newpatchset

1 0

[L] Change in flashrom[main]: udelay: only use OS time for delays, except on DOS
by Peter Marheine (Code Review) 03 Apr '24

03 Apr '24

Attention is currently required from: Anastasia Klimchuk, Brian Norris, Thomas Heijligen. Peter Marheine has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/81545?usp=email ) Change subject: udelay: only use OS time for delays, except on DOS ...................................................................... Patch Set 6: (1 comment) Commit Message: https://review.coreboot.org/c/flashrom/+/81545/comment/dcab515f_21107f18 : PS4, Line 25: > For the patch like this, it would be useful to know what was tested, if you could add testing info h […] Done -- To view, visit https://review.coreboot.org/c/flashrom/+/81545?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: I7ac5450d194a475143698d65d64d8bcd2fd25e3f Gerrit-Change-Number: 81545 Gerrit-PatchSet: 6 Gerrit-Owner: Peter Marheine <pmarheine(a)chromium.org> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Brian Norris <briannorris(a)chromium.org> Gerrit-Reviewer: Thomas Heijligen <src(a)posteo.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Brian Norris <briannorris(a)chromium.org> Gerrit-Attention: Thomas Heijligen <src(a)posteo.de> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Comment-Date: Wed, 03 Apr 2024 00:52:01 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-MessageType: comment

1 0

[L] Change in flashrom[main]: udelay: only use OS time for delays, except on DOS
by Peter Marheine (Code Review) 03 Apr '24

03 Apr '24

Attention is currently required from: Anastasia Klimchuk, Brian Norris, Thomas Heijligen. Hello Anastasia Klimchuk, Brian Norris, Thomas Heijligen, build bot (Jenkins), I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/flashrom/+/81545?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: udelay: only use OS time for delays, except on DOS ...................................................................... udelay: only use OS time for delays, except on DOS As proposed on the mailing list ("RFC: remove the calibrated delay loop"), this removes the calibrated delay loop and uses OS-based timing functions for all delays because the calibrated delay loop can delay for shorter times than intended. When sleeping this now uses nanosleep() if available, and otherwise usleep(). When busy-looping, it uses clock_gettime() with CLOCK_MONOTONIC or CLOCK_REALTIME depending on availability, and gettimeofday() otherwise. The calibrated delay loop is retained for DOS only, because timer resolution on DJGPP is only about 50 milliseconds. Since typical delays in flashrom are around 10 microseconds, using OS timing there would regress performance by around 500x. The old implementation is reused with some branches removed based on the knowledge that timer resolution will not be better than about 50 milliseconds. Tested by reading and writing flash on several Intel and AMD systems: * Lenovo P920 (Intel C620, read/verify only) * "nissa" chromebook (Intel Alder Lake-N) * "zork" chromebook (AMD Zen+) Signed-off-by: Peter Marheine <pmarheine(a)chromium.org> Change-Id: I7ac5450d194a475143698d65d64d8bcd2fd25e3f --- M Makefile M include/programmer.h M libflashrom.c M meson.build M tests/meson.build M tests/tests.c M tests/tests.h A tests/udelay.c M udelay.c A udelay_dos.c 10 files changed, 261 insertions(+), 172 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/45/81545/6 -- To view, visit https://review.coreboot.org/c/flashrom/+/81545?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: I7ac5450d194a475143698d65d64d8bcd2fd25e3f Gerrit-Change-Number: 81545 Gerrit-PatchSet: 6 Gerrit-Owner: Peter Marheine <pmarheine(a)chromium.org> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Brian Norris <briannorris(a)chromium.org> Gerrit-Reviewer: Thomas Heijligen <src(a)posteo.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Brian Norris <briannorris(a)chromium.org> Gerrit-Attention: Thomas Heijligen <src(a)posteo.de> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-MessageType: newpatchset

1 0

[L] Change in flashrom[main]: udelay: only use OS time for delays, except on DOS
by Peter Marheine (Code Review) 03 Apr '24

03 Apr '24

Attention is currently required from: Anastasia Klimchuk, Brian Norris, Thomas Heijligen. Peter Marheine has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/81545?usp=email ) Change subject: udelay: only use OS time for delays, except on DOS ...................................................................... Patch Set 5: (3 comments) File meson.build: https://review.coreboot.org/c/flashrom/+/81545/comment/18111898_4f216adf : PS4, Line 121: DHAVE_NANOSLEEP=1 > yeah, maybe something similar to the `Makefile.d/clock_gettime_test. […] On reflection, this isn't even required. We previously only avoided using `nanosleep` on DOS, and DOS now has its own version of this file so we can assume this function is always available. Removed this check and the sleep+usleep branch in `internal_sleep`. File udelay.c: https://review.coreboot.org/c/flashrom/+/81545/comment/988fc1ba_e62af74f : PS4, Line 6: * > I think this counts as a significant change, maybe you want to add your copyright here? Done https://review.coreboot.org/c/flashrom/+/81545/comment/0ab1fadd_9bdfed46 : PS4, Line 76: timeusec > I'm gonna say you probably didn't test this part 😜 😬 thanks. I've added a test for basic delay operation and hacked meson.build temporarily to verify that everything builds and the test passes in both cases now. -- To view, visit https://review.coreboot.org/c/flashrom/+/81545?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: I7ac5450d194a475143698d65d64d8bcd2fd25e3f Gerrit-Change-Number: 81545 Gerrit-PatchSet: 5 Gerrit-Owner: Peter Marheine <pmarheine(a)chromium.org> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Brian Norris <briannorris(a)chromium.org> Gerrit-Reviewer: Thomas Heijligen <src(a)posteo.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Brian Norris <briannorris(a)chromium.org> Gerrit-Attention: Thomas Heijligen <src(a)posteo.de> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Comment-Date: Tue, 02 Apr 2024 23:54:08 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Brian Norris <briannorris(a)chromium.org> Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-MessageType: comment

1 0

[L] Change in flashrom[main]: udelay: only use OS time for delays, except on DOS
by Peter Marheine (Code Review) 03 Apr '24

03 Apr '24

Attention is currently required from: Anastasia Klimchuk, Peter Marheine, Thomas Heijligen. Hello Anastasia Klimchuk, Brian Norris, Thomas Heijligen, build bot (Jenkins), I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/flashrom/+/81545?usp=email to look at the new patch set (#5). The following approvals got outdated and were removed: Verified+1 by build bot (Jenkins) Change subject: udelay: only use OS time for delays, except on DOS ...................................................................... udelay: only use OS time for delays, except on DOS As proposed on the mailing list ("RFC: remove the calibrated delay loop"), this removes the calibrated delay loop and uses OS-based timing functions for all delays because the calibrated delay loop can delay for shorter times than intended. When sleeping this now uses nanosleep() if available, and otherwise usleep(). When busy-looping, it uses clock_gettime() with CLOCK_MONOTONIC or CLOCK_REALTIME depending on availability, and gettimeofday() otherwise. The calibrated delay loop is retained for DOS only, because timer resolution on DJGPP is only about 50 milliseconds. Since typical delays in flashrom are around 10 microseconds, using OS timing there would regress performance by around 500x. The old implementation is reused with some branches removed based on the knowledge that timer resolution will not be better than about 50 milliseconds. Tested by reading and verifying flash on several Intel and AMD systems: * Lenovo P920 (Intel C620) * (more not done yet) Signed-off-by: Peter Marheine <pmarheine(a)chromium.org> Change-Id: I7ac5450d194a475143698d65d64d8bcd2fd25e3f --- M Makefile M include/programmer.h M libflashrom.c M meson.build M tests/meson.build M tests/tests.c M tests/tests.h A tests/udelay.c M udelay.c A udelay_dos.c 10 files changed, 260 insertions(+), 172 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/45/81545/5 -- To view, visit https://review.coreboot.org/c/flashrom/+/81545?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: I7ac5450d194a475143698d65d64d8bcd2fd25e3f Gerrit-Change-Number: 81545 Gerrit-PatchSet: 5 Gerrit-Owner: Peter Marheine <pmarheine(a)chromium.org> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Brian Norris <briannorris(a)chromium.org> 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-Attention: Peter Marheine <pmarheine(a)chromium.org> Gerrit-MessageType: newpatchset

1 0

[L] Change in flashrom[main]: udelay: only use OS time for delays, except on DOS
by Brian Norris (Code Review) 02 Apr '24

02 Apr '24

Attention is currently required from: Anastasia Klimchuk, Peter Marheine, Thomas Heijligen. Brian Norris has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/81545?usp=email ) Change subject: udelay: only use OS time for delays, except on DOS ...................................................................... Patch Set 4: (1 comment) Patchset: PS4: I meant to say: I'm happy to see this change in general. Aside from the relatively small review comments, this looks good to me -- To view, visit https://review.coreboot.org/c/flashrom/+/81545?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: I7ac5450d194a475143698d65d64d8bcd2fd25e3f Gerrit-Change-Number: 81545 Gerrit-PatchSet: 4 Gerrit-Owner: Peter Marheine <pmarheine(a)chromium.org> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Brian Norris <briannorris(a)chromium.org> 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-Attention: Peter Marheine <pmarheine(a)chromium.org> Gerrit-Comment-Date: Tue, 02 Apr 2024 21:22:35 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

2025

2024

2023

2022

2021

2020

2019

2018

2017

flashrom-gerrit