November 2024 - flashrom-gerrit

[L] Change in flashrom[main]: rpmc: add rpmc commands feature
by Anastasia Klimchuk (Code Review) 04 Nov '24

04 Nov '24

Attention is currently required from: Matti Finder. Anastasia Klimchuk has posted comments on this change by Matti Finder. ( https://review.coreboot.org/c/flashrom/+/84934?usp=email ) Change subject: rpmc: add rpmc commands feature ...................................................................... Patch Set 4: (9 comments) Patchset: PS4: Matti, thank you so much for your patience! I made another round of comments, but probably the last one. There is one more thing, at the end when code is ready and all comments fixed, if you could re-test it again, on the latest version of the patch? I am thinking about two scenarios to test: 1) chip model that supports rpmc, and handled as "SFDP-capable chip" 2) chip model that does not support rpmc, but is recognised and handled as a "SFDP-capable chip": to make sure baseline case is not broken. File include/rpmc.h: https://review.coreboot.org/c/flashrom/+/84934/comment/29424b16_61412a25?us… : PS4, Line 90: struct flashrom_flashctx * flash Sorry I just noticed! we usually have asterisk sticking to the variable name (struct flashrom_flashctx *flash). As an example is sfdp.c. I see that you used space between * and <name> consistently in all your new function declarations and definitions... but I need to ask you to change to align with our existing code style. Thank you so much! (hope you are not too upset) File rpmc.c: https://review.coreboot.org/c/flashrom/+/84934/comment/76c3bb63_6891df63?us… : PS3, Line 224: return -1; > My original of how to handle the return values wasn't that good, and just lead to a lot of questions […] Enum of return codes is good idea! Now it's all nice and readable. File rpmc.c: https://review.coreboot.org/c/flashrom/+/84934/comment/153afc03_2820beee?us… : PS4, Line 117: usleep(flash->chip->rpmc_ctx.polling_long_delay_write_counter_us) Peter, I wanted to check, is this the right way to sleep? Are we supposed to be using `programmer_delay` everywhere? https://review.coreboot.org/c/flashrom/+/84934/comment/6f8648e0_a80bd1fc?us… : PS4, Line 131: if (res != RPMC_SUCCESS) { : return res; : } Found another places here (rpmc_poll_until_finished) where { } not needed because conditional body is one-line. And also above, when you call usleep https://review.coreboot.org/c/flashrom/+/84934/comment/2fd0acbb_c9d89e30?us… : PS4, Line 157: if (keyfile == NULL || read_buf_from_file(key, RPMC_HMAC_KEY_LENGTH, keyfile) != 0) { : return RPMC_ERROR_KEY_READ; : } And here, { } not needed, 1-line body https://review.coreboot.org/c/flashrom/+/84934/comment/02e8f632_a82d2ae4?us… : PS4, Line 497: no error string defined for this value Maybe > Unknown Internal Error: Error code not recognised (because, the fact that we don't have a string is not a problem by itself, the problem we don't know what happened) File sfdp.c: https://review.coreboot.org/c/flashrom/+/84934/comment/062d90aa_49653796?us… : PS3, Line 449: msg_cdbg("The chip contains an unknown " : "version of the JEDEC flash " : "parameters table, skipping it.\n"); > Done Thank you! File sfdp.c: https://review.coreboot.org/c/flashrom/+/84934/comment/acfe3672_a84de7bb?us… : PS4, Line 457: if (sfdp_fill_flash(flash->chip, tbuf, len) == 0) { : ret = 1; : } I have a question: if for some reasons `sfdp_fill_flash` fails (i.e. returns non-zero), would RPMC feature work? (or any other feature). It seems like having `sfdp_fill_flash` successful is a pre-condition for doing anything with the chip? If we are handling the chip as generic SFDP-chip. I am asking because right now failure of `sfdp_fill_flash` does not stop parsing the headers, and possibly below in line 468 `ret = 1` will mark probing successful, but it shouldn't be? Maybe this should return error straight away if `sfdp_fill_flash` fails? Previously it wasn't important because only page 0 of headers was parsed. -- To view, visit https://review.coreboot.org/c/flashrom/+/84934?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email Gerrit-MessageType: comment Gerrit-Project: flashrom Gerrit-Branch: main Gerrit-Change-Id: I6ab3d0446e9fd674b20550fdbfaf499b8d4a9b38 Gerrit-Change-Number: 84934 Gerrit-PatchSet: 4 Gerrit-Owner: Matti Finder <matti.finder(a)gmail.com> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Peter Marheine <pmarheine(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Matti Finder <matti.finder(a)gmail.com> Gerrit-Comment-Date: Mon, 04 Nov 2024 09:07:20 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org> Comment-In-Reply-To: Matti Finder <matti.finder(a)gmail.com>

1 0

[L] Change in flashrom[main]: cli_client: Add rpmc command support
by Peter Marheine (Code Review) 04 Nov '24

04 Nov '24

Attention is currently required from: Anastasia Klimchuk, Matti Finder. Peter Marheine has posted comments on this change by Matti Finder. ( https://review.coreboot.org/c/flashrom/+/84840?usp=email ) Change subject: cli_client: Add rpmc command support ...................................................................... Patch Set 8: Code-Review+2 -- To view, visit https://review.coreboot.org/c/flashrom/+/84840?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email Gerrit-MessageType: comment Gerrit-Project: flashrom Gerrit-Branch: main Gerrit-Change-Id: I36c823bbee65f256eb6edabe6f058321c9a0cfa1 Gerrit-Change-Number: 84840 Gerrit-PatchSet: 8 Gerrit-Owner: Matti Finder <matti.finder(a)gmail.com> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Peter Marheine <pmarheine(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Attention: Matti Finder <matti.finder(a)gmail.com> Gerrit-Comment-Date: Mon, 04 Nov 2024 00:16:52 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes

1 0

[L] Change in flashrom[main]: rpmc: add rpmc commands feature
by Peter Marheine (Code Review) 04 Nov '24

04 Nov '24

Attention is currently required from: Anastasia Klimchuk, Matti Finder. Peter Marheine has posted comments on this change by Matti Finder. ( https://review.coreboot.org/c/flashrom/+/84934?usp=email ) Change subject: rpmc: add rpmc commands feature ...................................................................... Patch Set 4: Code-Review+2 (1 comment) File include/rpmc.h: https://review.coreboot.org/c/flashrom/+/84934/comment/42f3d851_50f9be22?us… : PS4, Line 71: dependant ```suggestion * Some bits might exclude others or their meaning might be dependent ``` -- To view, visit https://review.coreboot.org/c/flashrom/+/84934?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email Gerrit-MessageType: comment Gerrit-Project: flashrom Gerrit-Branch: main Gerrit-Change-Id: I6ab3d0446e9fd674b20550fdbfaf499b8d4a9b38 Gerrit-Change-Number: 84934 Gerrit-PatchSet: 4 Gerrit-Owner: Matti Finder <matti.finder(a)gmail.com> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Peter Marheine <pmarheine(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Attention: Matti Finder <matti.finder(a)gmail.com> Gerrit-Comment-Date: Mon, 04 Nov 2024 00:13:49 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes

1 0

[M] Change in flashrom[main]: doc: Add few sections to recent development doc
by Peter Marheine (Code Review) 04 Nov '24

04 Nov '24

Attention is currently required from: Anastasia Klimchuk. Peter Marheine has posted comments on this change by Anastasia Klimchuk. ( https://review.coreboot.org/c/flashrom/+/84954?usp=email ) Change subject: doc: Add few sections to recent development doc ...................................................................... Patch Set 1: (9 comments) File doc/release_notes/devel.rst: https://review.coreboot.org/c/flashrom/+/84954/comment/af96bb2b_72e4ff57?us… : PS1, Line 70: Programmers ```suggestion Programmer updates ``` https://review.coreboot.org/c/flashrom/+/84954/comment/0d7aebfb_a09f5c3a?us… : PS1, Line 73: * ch347_spi: Add spi clock frequency selection ```suggestion * ch347_spi: Add spi clock frequency selection ("spispeed" option) ``` https://review.coreboot.org/c/flashrom/+/84954/comment/cd0a62c7_0cdea139?us… : PS1, Line 78: Bugs fixes ```suggestion Bugs fixed ``` I think it'll be more useful for these to describe what can go wrong, and link to the change. Users want to know how they might have been affected by the bugs that have been fixed (which isn't entirely clear with how these are written now), and distributors might want to know which patches they should backport. Suggestions inline. https://review.coreboot.org/c/flashrom/+/84954/comment/f08e31f3_5a52b9db?us… : PS1, Line 81: * Ensure verify operation completed in full if chip memory modified : : Post-cleanup after processing unaligned region for the case when end : region needs to be extended to align with erase block. Writing was : done correctly, but post-processing of newcontents could cause : one-off offset at the end of the region, which would make : verification appear false-negative ```suggestion * Modified bytes would sometimes not be verified after writing In some situations an off-by-one error would cause the last byte of memory that was modified by an operation to not be verified, which could prevent some erase or write errors from being detected. Fixed by https://review.coreboot.org/c/flashrom/+/84078. ``` https://review.coreboot.org/c/flashrom/+/84954/comment/bef7a071_86818501?us… : PS1, Line 89: * erasure_layout: Fix init_eraseblock segmentation fault : : Fix a segmentation fault that is caused by accessing an invalid : "subedata" pointer on the last iteration of the init_eraseblock loop. : Instead, short circuit the loop condition to check the sub block index : first, and do not access the invalid pointer if it is the last sub : block. : : Issue was encountered in OpenBSD. ```suggestion * Possible crashes while preparing erase operations An out-of-bounds memory read was found in the algorithm that selects methods to erase memory, which could cause flashrom to crash. This issue was first introduced in release 1.4, and crashes were observed when running on OpenBSD. See https://ticket.coreboot.org/issues/555, and fixed by https://review.coreboot.org/c/flashrom/+/84234. ``` https://review.coreboot.org/c/flashrom/+/84954/comment/ad7de33d_dc526450?us… : PS1, Line 99: * Fix FEATURE_NO_ERASE chips and add test for them : : https://ticket.coreboot.org/issues/553 ```suggestion * Crash when attempting to erase FEATURE_NO_ERASE chips When attempting to erase a chip that doesn't need to be erased before being written, flashrom could attempt to read through a null pointer and crash. The only supported chip that is affected is the M95M02 EEPROM. See https://ticket.coreboot.org/issues/553, and fixed by https://review.coreboot.org/c/flashrom/+/84203. ``` https://review.coreboot.org/c/flashrom/+/84954/comment/12fcf664_f4f55eb0?us… : PS1, Line 103: * build: never install cmocka : : https://ticket.coreboot.org/issues/561 ```suggestion * install failures related to libcmocka In some configurations, the install target provided by the buildsystem (like meson install) would fail to execute successfully due to a missing libcmocka file. This is fixed by not installing libcmocka, because it is a third-party library used by flashrom only for testing. See https://ticket.coreboot.org/issues/561, and fixed by https://review.coreboot.org/c/flashrom/+/84557. ``` https://review.coreboot.org/c/flashrom/+/84954/comment/2d2a7ed4_3cd7c9a8?us… : PS1, Line 107: * ichspi: Probe opcode in POSSIBLE_OPCODES[] as well : : https://ticket.coreboot.org/issues/556 ```suggestion * Excess erase of automatically-probed chips on Intel chipsets When erasing some chips using the ichspi programmer (for Intel ICH chipsets), the entire chip would be erased and rewritten even when the hardware supported erasing smaller blocks, causing operations to take longer to complete and negatively impacting chip longevity. This issue was first introduced in version 1.4. See https://ticket.coreboot.org/issues/556, and fixed by https://review.coreboot.org/c/flashrom/+/84253. ``` https://review.coreboot.org/c/flashrom/+/84954/comment/bfbc5ac4_6d937ca3?us… : PS1, Line 111: * erasure_layout: Erase larger block only when all sub-block need erase : : * erase/write: Deselect all smaller blocks when large block is selected These are two changes relating to the same issue, so I think they make sense to combine. ```suggestion * Unnecessary erases When erasing parts of a memory, some blocks could be erased and rewritten unnecessarily or erased multiple times which could hurt the longevity of the memory chip. This behavior was introduced in version 1.4. Fixed by https://review.coreboot.org/c/flashrom/+/84614 and https://review.coreboot.org/c/flashrom/+/84686. ``` -- To view, visit https://review.coreboot.org/c/flashrom/+/84954?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email Gerrit-MessageType: comment Gerrit-Project: flashrom Gerrit-Branch: main Gerrit-Change-Id: Iedaca4a704c57c1db399c7888f743ad2961cbf9d Gerrit-Change-Number: 84954 Gerrit-PatchSet: 1 Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Peter Marheine <pmarheine(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Comment-Date: Mon, 04 Nov 2024 00:09:55 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No

1 0

[XS] Change in flashrom[main]: doc: Change link from gitiles to github
by Peter Marheine (Code Review) 03 Nov '24

03 Nov '24

Attention is currently required from: Anastasia Klimchuk. Peter Marheine has posted comments on this change by Anastasia Klimchuk. ( https://review.coreboot.org/c/flashrom/+/84960?usp=email ) Change subject: doc: Change link from gitiles to github ...................................................................... Patch Set 2: Code-Review+2 -- To view, visit https://review.coreboot.org/c/flashrom/+/84960?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email Gerrit-MessageType: comment Gerrit-Project: flashrom Gerrit-Branch: main Gerrit-Change-Id: I9103e5199bdf1b65fa3136aa01259a85e788a251 Gerrit-Change-Number: 84960 Gerrit-PatchSet: 2 Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Peter Marheine <pmarheine(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Comment-Date: Sun, 03 Nov 2024 22:32:39 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes

1 0

[M] Change in flashrom[main]: Display progress for what is actually erased/written
by Anastasia Klimchuk (Code Review) 03 Nov '24

03 Nov '24

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(a)flashrom.org> Co-developed-by: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com> Signed-off-by: Anastasia Klimchuk <aklm(a)flashrom.org> Reviewed-on: https://review.coreboot.org/c/flashrom/+/84439 Reviewed-by: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com> Tested-by: build bot (Jenkins) <no-reply(a)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); -- To view, visit https://review.coreboot.org/c/flashrom/+/84439?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email Gerrit-MessageType: merged Gerrit-Project: flashrom Gerrit-Branch: main Gerrit-Change-Id: I88ac4d40f1b6ccc1636b1efb690d8d68bdebec08 Gerrit-Change-Number: 84439 Gerrit-PatchSet: 18 Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>

1 0

[S] Change in flashrom[main]: Split cli_output declarations to a separate header.
by Antonio Vázquez Blanco (Code Review) 03 Nov '24

03 Nov '24

Antonio Vázquez Blanco has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/84985?usp=email ) Change subject: Split cli_output declarations to a separate header. ...................................................................... Split cli_output declarations to a separate header. This is a simple refactor that aims to simplify maintenance and to clarify file dependency inside the project. Currently, many declarations reside in flash.h making it difficult to really understand file dependency. Change-Id: I45d62bd219bdbed919788ae17a64aeb119a8aac4 Signed-off-by: Antonio Vázquez Blanco <antoniovazquezblanco(a)gmail.com> --- M cli_classic.c M cli_output.c A include/cli_output.h M include/flash.h 4 files changed, 25 insertions(+), 9 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/85/84985/1 diff --git a/cli_classic.c b/cli_classic.c index 3343438..1669076 100644 --- a/cli_classic.c +++ b/cli_classic.c @@ -25,6 +25,7 @@ #include <stdbool.h> #include <stdlib.h> #include <cli_classic.h> +#include "cli_output.h" #include "flash.h" #include "flashchips.h" #include "fmap.h" diff --git a/cli_output.c b/cli_output.c index e5b829a..cf96bc0 100644 --- a/cli_output.c +++ b/cli_output.c @@ -15,11 +15,12 @@ * GNU General Public License for more details. */ +#include "cli_output.h" + #include <stdio.h> #include <stdarg.h> #include <string.h> #include <errno.h> -#include "flash.h" enum flashrom_log_level verbose_screen = FLASHROM_MSG_INFO; enum flashrom_log_level verbose_logfile = FLASHROM_MSG_DEBUG2; diff --git a/include/cli_output.h b/include/cli_output.h new file mode 100644 index 0000000..3f6107a --- /dev/null +++ b/include/cli_output.h @@ -0,0 +1,21 @@ +/* + * This file is part of the flashrom project. + * + * SPDX-FileCopyrightText: 2024 Antonio Vázquez Blanco <antoniovazquezblanco(a)gmail.com> + * SPDX-License-Identifier: GPL-2.0-or-later + */ +#ifndef __CLI_OUTPUT_H__ +#define __CLI_OUTPUT_H__ + +#include <stdarg.h> +#include "flash.h" + +extern enum flashrom_log_level verbose_screen; +extern enum flashrom_log_level verbose_logfile; +int open_logfile(const char * const filename); +int close_logfile(void); +void start_logging(void); +int flashrom_print_cb(enum flashrom_log_level level, const char *fmt, va_list ap); +void flashrom_progress_cb(struct flashrom_flashctx *flashctx); + +#endif /* __CLI_OUTPUT_H__ */ diff --git a/include/flash.h b/include/flash.h index d0e55af..f0b7f3b 100644 --- a/include/flash.h +++ b/include/flash.h @@ -680,14 +680,7 @@ /* cli_common.c */ void print_chip_support_status(const struct flashchip *chip); -/* cli_output.c */ -extern enum flashrom_log_level verbose_screen; -extern enum flashrom_log_level verbose_logfile; -int open_logfile(const char * const filename); -int close_logfile(void); -void start_logging(void); -int flashrom_print_cb(enum flashrom_log_level level, const char *fmt, va_list ap); -void flashrom_progress_cb(struct flashrom_flashctx *flashctx); +/* libflashrom.c */ /* Let gcc and clang check for correct printf-style format strings. */ int print(enum flashrom_log_level level, const char *fmt, ...) #ifdef __MINGW32__ -- To view, visit https://review.coreboot.org/c/flashrom/+/84985?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email Gerrit-MessageType: newchange Gerrit-Project: flashrom Gerrit-Branch: main Gerrit-Change-Id: I45d62bd219bdbed919788ae17a64aeb119a8aac4 Gerrit-Change-Number: 84985 Gerrit-PatchSet: 1 Gerrit-Owner: Antonio Vázquez Blanco <antoniovazquezblanco(a)gmail.com>

1 0

[M] Change in flashrom[main]: Rename cli_classic.h to a more adequate cli_getop.h
by Antonio Vázquez Blanco (Code Review) 03 Nov '24

03 Nov '24

Antonio Vázquez Blanco has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/84983?usp=email ) Change subject: Rename cli_classic.h to a more adequate cli_getop.h ...................................................................... Rename cli_classic.h to a more adequate cli_getop.h The header only defines getop related stuff so it seems more intuitive this way. Also, use SPDX to reduce header comment sizes. Change-Id: I5986828f3bdec583af6f7b02cbe1a9c45ed2000f Signed-off-by: Antonio Vázquez Blanco <antoniovazquezblanco(a)gmail.com> --- M cli_classic.c M cli_getopt.c D include/cli_classic.h A include/cli_getopt.h 4 files changed, 50 insertions(+), 68 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/83/84983/1 diff --git a/cli_classic.c b/cli_classic.c index 3343438..3b6b6c8 100644 --- a/cli_classic.c +++ b/cli_classic.c @@ -24,7 +24,7 @@ #include <string.h> #include <stdbool.h> #include <stdlib.h> -#include <cli_classic.h> +#include "cli_getopt.h" #include "flash.h" #include "flashchips.h" #include "fmap.h" diff --git a/cli_getopt.c b/cli_getopt.c index fc51fcf..8e5a330 100644 --- a/cli_getopt.c +++ b/cli_getopt.c @@ -1,37 +1,21 @@ /* * This file is part of the flashrom project. - * It comes originally from the musl libc project and is licensed under the - * terms of the MIT license. + * It comes originally from the musl libc project. * - * Copyringht (C) 2023 Rich Felker and the musl authors - * Adjusted for flashrom by Thomas Heijligen<thomas.heijligen(a)secunet.com> - * - * Permission is hereby granted, free of charge, to any person obtaining a copy - * of this software and associated documentation files (the "Software"), to deal - * in the Software without restriction, including without limitation the rights - * to use, copy, modify, merge, publish, distribute, sublicense, and/or sell - * copies of the Software, and to permit persons to whom the Software is - * furnished to do so, subject to the following conditions: - * - * The above copyright notice and this permission notice shall be included in - * all copies or substantial portions of the Software. - * - * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR - * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY, - * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT. IN NO EVENT SHALL THE - * AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER - * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING FROM, - * OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS IN THE - * SOFTWARE. + * SPDX-FileCopyrightText: 2023 Rich Felker and the musl authors + * SPDX-FileCopyrightText: Thomas Heijligen <thomas.heijligen(a)secunet.com> + * SPDX-FileCopyrightText: 2024 Antonio Vázquez Blanco <antoniovazquezblanco(a)gmail.com> + * SPDX-License-Identifier: MIT */ +#include "cli_getopt.h" + #include <unistd.h> #include <wchar.h> #include <string.h> #include <limits.h> #include <stdlib.h> -#include "cli_classic.h" -#include "flash.h" +#include "flash.h" // msg_gerr char *optarg; int optind=1, opterr=1, optopt, optpos; diff --git a/include/cli_classic.h b/include/cli_classic.h deleted file mode 100644 index eb1f0fe..0000000 --- a/include/cli_classic.h +++ /dev/null @@ -1,43 +0,0 @@ -/* - * This file is part of the flashrom project. - * - * This program is free software; you can redistribute it and/or modify - * it under the terms of the GNU General Public License as published by - * the Free Software Foundation; either version 2 of the License, or - * (at your option) any later version. - * - * This program is distributed in the hope that it will be useful, - * but WITHOUT ANY WARRANTY; without even the implied warranty of - * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the - * GNU General Public License for more details. - */ - -#ifndef CLI_CLASSIC_H -#define CLI_CLASSIC_H - -#ifdef HAVE_GETOPT_H -#include <getopt.h> -#else - -#define no_argument 0 -#define required_argument 1 -#define optional_argument 2 - -extern char *optarg; -extern int optind, opterr, optopt; - -struct option { - const char *name; - int has_arg; - int *flag; - int val; -}; - -int getopt (int argc, char *const *argv, const char *shortopts); -int getopt_long (int argc, char *const *argv, const char *shortopts, - const struct option *longopts, int *longind); -int getopt_long_only (int argc, char *const *argv, const char *shortopts, - const struct option *longopts, int *longind); - -#endif /* HAVE_GETOPT_H */ -#endif /* CLI_CLASSIC_H */ diff --git a/include/cli_getopt.h b/include/cli_getopt.h new file mode 100644 index 0000000..5c84828 --- /dev/null +++ b/include/cli_getopt.h @@ -0,0 +1,41 @@ +/* + * This file is part of the flashrom project. + * + * SPDX-FileCopyrightText: 2024 Antonio Vázquez Blanco <antoniovazquezblanco(a)gmail.com> + * SPDX-License-Identifier: GPL-2.0-or-later + */ + +#ifndef __CLI_GETOPT_H__ +#define __CLI_GETOPT_H__ + +/** + * This header is responsible for either including a standard getop + * implementation header or to provide a compatible one. + */ + +#ifdef HAVE_GETOPT_H +#include <getopt.h> +#else + +#define no_argument 0 +#define required_argument 1 +#define optional_argument 2 + +extern char *optarg; +extern int optind, opterr, optopt; + +struct option { + const char *name; + int has_arg; + int *flag; + int val; +}; + +int getopt (int argc, char *const *argv, const char *shortopts); +int getopt_long (int argc, char *const *argv, const char *shortopts, + const struct option *longopts, int *longind); +int getopt_long_only (int argc, char *const *argv, const char *shortopts, + const struct option *longopts, int *longind); + +#endif /* HAVE_GETOPT_H */ +#endif /* __CLI_GETOPT_H__ */ -- To view, visit https://review.coreboot.org/c/flashrom/+/84983?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email Gerrit-MessageType: newchange Gerrit-Project: flashrom Gerrit-Branch: main Gerrit-Change-Id: I5986828f3bdec583af6f7b02cbe1a9c45ed2000f Gerrit-Change-Number: 84983 Gerrit-PatchSet: 1 Gerrit-Owner: Antonio Vázquez Blanco <antoniovazquezblanco(a)gmail.com>

1 0

[S] Change in flashrom[main]: Split usbdev declarations to a separate header.
by Miklós Márton (Code Review) 03 Nov '24

03 Nov '24

Attention is currently required from: Antonio Vázquez Blanco. Miklós Márton has posted comments on this change by Antonio Vázquez Blanco. ( https://review.coreboot.org/c/flashrom/+/84982?usp=email ) Change subject: Split usbdev declarations to a separate header. ...................................................................... Patch Set 2: Code-Review+2 -- To view, visit https://review.coreboot.org/c/flashrom/+/84982?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email Gerrit-MessageType: comment Gerrit-Project: flashrom Gerrit-Branch: main Gerrit-Change-Id: I9d819ea1c5bd51289d02189c1dff367ce6d25617 Gerrit-Change-Number: 84982 Gerrit-PatchSet: 2 Gerrit-Owner: Antonio Vázquez Blanco <antoniovazquezblanco(a)gmail.com> Gerrit-Reviewer: Miklós Márton <martonmiklosqdev(a)gmail.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Antonio Vázquez Blanco <antoniovazquezblanco(a)gmail.com> Gerrit-Comment-Date: Sun, 03 Nov 2024 18:09:29 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes

1 0

[S] Change in flashrom[main]: Split usbdev declarations to a separate header.
by Antonio Vázquez Blanco (Code Review) 03 Nov '24

03 Nov '24

Attention is currently required from: Miklós Márton. Antonio Vázquez Blanco has posted comments on this change by Antonio Vázquez Blanco. ( https://review.coreboot.org/c/flashrom/+/84982?usp=email ) Change subject: Split usbdev declarations to a separate header. ...................................................................... Patch Set 2: (2 comments) Patchset: PS1: > Apart from the typo LGTM. Updated. Thank you very much! Commit Message: https://review.coreboot.org/c/flashrom/+/84982/comment/e4a04b3c_3673681f?us… : PS1, Line 7: usbvev > usbdev typo? Acknowledged -- To view, visit https://review.coreboot.org/c/flashrom/+/84982?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email Gerrit-MessageType: comment Gerrit-Project: flashrom Gerrit-Branch: main Gerrit-Change-Id: I9d819ea1c5bd51289d02189c1dff367ce6d25617 Gerrit-Change-Number: 84982 Gerrit-PatchSet: 2 Gerrit-Owner: Antonio Vázquez Blanco <antoniovazquezblanco(a)gmail.com> Gerrit-Reviewer: Miklós Márton <martonmiklosqdev(a)gmail.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Miklós Márton <martonmiklosqdev(a)gmail.com> Gerrit-Comment-Date: Sun, 03 Nov 2024 18:08:35 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Miklós Márton <martonmiklosqdev(a)gmail.com>

1 0

2024

2023

2022

2021

2020

2019

2018

2017

flashrom-gerrit November 2024