- flashrom-gerrit - mail.coreboot.org

[M] Change in flashrom[main]: Fix FEATURE_NO_ERASE chips and add test for them
by Anastasia Klimchuk (Code Review) 16 Sep '24

16 Sep '24

Anastasia Klimchuk has submitted this change. ( https://review.coreboot.org/c/flashrom/+/84203?usp=email ) Change subject: Fix FEATURE_NO_ERASE chips and add test for them ...................................................................... Fix FEATURE_NO_ERASE chips and add test for them New check was added to `check_block_eraser` in commit 0f389aea9e630c3b21547a5dd8dbe572a8502853 but it was not handling FEATURE_NO_ERASE chips. This patch fixes processing such chips and adds test to run write and verify with dummyflasher for FEATURE_NO_ERASE chips. Ticket: https://ticket.coreboot.org/issues/553 Change-Id: I582fe00da0715e9b5e92fcc9d15d5a90a2615117 Signed-off-by: Anastasia Klimchuk <aklm(a)flashrom.org> Reviewed-on: https://review.coreboot.org/c/flashrom/+/84203 Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org> Reviewed-by: Peter Marheine <pmarheine(a)chromium.org> --- M flashrom.c M include/flash.h M tests/chip.c M tests/tests.c M tests/tests.h 5 files changed, 80 insertions(+), 7 deletions(-) Approvals: build bot (Jenkins): Verified Peter Marheine: Looks good to me, approved diff --git a/flashrom.c b/flashrom.c index 01a41c4..e2c60f4 100644 --- a/flashrom.c +++ b/flashrom.c @@ -484,14 +484,15 @@ if (flash->mst->buses_supported & BUS_SPI) { const uint8_t *opcode = spi_get_opcode_from_erasefn(eraser.block_erase); - for (int i = 0; opcode[i]; i++) { - if (!spi_probe_opcode(flash, opcode[i])) { - if (log) - msg_cdbg("block erase function and layout found " - "but SPI master doesn't support the function. "); - return 1; + if (opcode) + for (int i = 0; opcode[i]; i++) { + if (!spi_probe_opcode(flash, opcode[i])) { + if (log) + msg_cdbg("block erase function and layout found " + "but SPI master doesn't support the function. "); + return 1; + } } - } } // TODO: Once erase functions are annotated with allowed buses, check that as well. return 0; diff --git a/include/flash.h b/include/flash.h index d9a20ad..d0e55af 100644 --- a/include/flash.h +++ b/include/flash.h @@ -148,6 +148,14 @@ * other flash chips, such as the ENE KB9012 internal flash, work the opposite way. */ #define FEATURE_ERASED_ZERO (1 << 18) +/* + * Feature indicates that the chip does not require erase before writing: + * write operations can set any bit to any value without first doing an erase, + * but bulk erase operations may still be supported. + * + * EEPROMs usually behave this way (compare to Flash, which requires erase), + * for example the ST M95M02. + */ #define FEATURE_NO_ERASE (1 << 19) #define FEATURE_WRSR_EXT2 (1 << 20) diff --git a/tests/chip.c b/tests/chip.c index 3e1ae3b..604d79e 100644 --- a/tests/chip.c +++ b/tests/chip.c @@ -27,6 +27,7 @@ #include <include/test.h> #include <stdio.h> +#include <stdlib.h> #include <string.h> #include "tests.h" @@ -132,6 +133,25 @@ }}, }; +/* Chip expected to be processed with dummyflasher, so using real op functions. */ +static const struct flashchip chip_no_erase = { + .vendor = "aklm&dummyflasher", + .total_size = 16 * 1024, + .tested = TEST_OK_PREW, + .read = SPI_CHIP_READ, + .write = SPI_CHIP_WRITE256, + .page_size = 256, + .feature_bits = FEATURE_NO_ERASE | FEATURE_ERASED_ZERO, + .block_erasers = + { + { + .eraseblocks = { {16 * 1024 * 1024, 1} }, + /* Special erase fn for chips without erase capability. */ + .block_erase = SPI_BLOCK_ERASE_EMULATION, + } + }, +}; + /* Setup the struct for W25Q128.V, all values come from flashchips.c */ static const struct flashchip chip_W25Q128_V = { .vendor = "aklm&dummyflasher", @@ -384,6 +404,48 @@ free(newcontents); } +void write_chip_feature_no_erase(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); + + 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 165390a..78f1b47 100644 --- a/tests/tests.c +++ b/tests/tests.c @@ -498,6 +498,7 @@ cmocka_unit_test(read_chip_with_dummyflasher_test_success), cmocka_unit_test(write_chip_test_success), cmocka_unit_test(write_chip_with_dummyflasher_test_success), + cmocka_unit_test(write_chip_feature_no_erase), 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 8cf8be0..3536d1d 100644 --- a/tests/tests.h +++ b/tests/tests.h @@ -86,6 +86,7 @@ void read_chip_with_dummyflasher_test_success(void **state); void write_chip_test_success(void **state); void write_chip_with_dummyflasher_test_success(void **state); +void write_chip_feature_no_erase(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/+/84203?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: I582fe00da0715e9b5e92fcc9d15d5a90a2615117 Gerrit-Change-Number: 84203 Gerrit-PatchSet: 7 Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Miklós Márton <martonmiklosqdev(a)gmail.com> Gerrit-Reviewer: Peter Marheine <pmarheine(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>

1 0

[M] Change in flashrom[main]: More adequate progress
by Sergii Dmytruk (Code Review) 14 Sep '24

14 Sep '24

Attention is currently required from: Aarya, Anastasia Klimchuk, Nikolai Artemiev, Peter Marheine. Sergii Dmytruk has posted comments on this change by Sergii Dmytruk. ( https://review.coreboot.org/c/flashrom/+/84102?usp=email ) Change subject: More adequate progress ...................................................................... Patch Set 1: (7 comments) Patchset: PS1: > Sergii, thanks a lot for the patch, that's a great help. […] Sure, please go for it. I can review and provide more details as needed, just don't have time/need to finish this properly. Commit Message: https://review.coreboot.org/c/flashrom/+/84102/comment/7fa24368_d2e255aa?us… : PS1, Line 46: : CLI shares terminal with the rest of the code > What does it mean? I don't understand :( I just meant that flashrom can print log output at any point (like it prints `DONE something`) so CLI implementation can't assume that nothing interferes with its output. File dummyflasher.c: https://review.coreboot.org/c/flashrom/+/84102/comment/2af2d3e3_1145c745?us… : PS1, Line 58: unsigned long long delay_ns; > This is changing to nano seconds, is it to be able to set a more fine-grained delay? […] I think insufficient range was my first guess, before I noticed `(1000000 * 8) / freq` below. If frequency of 1Hz isn't necessary in 32-bit builds, then `long int` should be enough. 64MHz wasn't supported because `(1000000 * 8) / 64000000` results in zero delay (like any frequency above 8MHz). Type of `freq` below doesn't need to change in any case. File include/flash.h: https://review.coreboot.org/c/flashrom/+/84102/comment/74c4eb9a_2775229e?us… : PS1, Line 544: struct stage_progress { : size_t current; : size_t total; : }; > And can this go to libflashrom. […] This is not part of API, nothing in `libflashrom.h` uses this structure. https://review.coreboot.org/c/flashrom/+/84102/comment/8edbf977_85a786b5?us… : PS1, Line 587: struct stage_progress stage_progress[FLASHROM_PROGRESS_NR]; > Maybe this can go inside `struct flashrom_progress` ? Why do we need two structs on the same level, […] That would change API and ABI. This was added for internal use to keep track of progress, while `struct flashrom_progress` is a parameter for a user callback which can also be allocated on client side and changed at will, potentially messing with flashrom's view of things. File linux_mtd.c: https://review.coreboot.org/c/flashrom/+/84102/comment/4caa98f3_c3707e9c?us… : PS1, Line 298: update_progress(flash, FLASHROM_PROGRESS_ERASE, u + data->erasesize, len); > Why you removed this? Should it be […] It's for erasing which I think is handled uniformly in `erasure_layout.c`. File spi.c: https://review.coreboot.org/c/flashrom/+/84102/comment/a9799407_12dbdc0e?us… : PS1, Line 118: update_progress(flash, FLASHROM_PROGRESS_READ, start - start_address + to_read, end_address); > Why removing, should this be […] `flash->mst->spi.read` updates progress, keeping this line results in progress on reading reaching over 100%. This could be a single place to report read progress (like for erasing) and drop all similar calls in programmers, but granularity would likely be too coarse (erasing is different, there can be a command to erase the whole chip at once and you can only report it in one way: 0% -> 100%). -- To view, visit https://review.coreboot.org/c/flashrom/+/84102?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: If1e40fc97f443c4f0c0501cef11cff1f3f84c051 Gerrit-Change-Number: 84102 Gerrit-PatchSet: 1 Gerrit-Owner: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com> Gerrit-Reviewer: Aarya <aarya.chaumal(a)gmail.com> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Reviewer: Peter Marheine <pmarheine(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Aarya <aarya.chaumal(a)gmail.com> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Attention: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Attention: Peter Marheine <pmarheine(a)chromium.org> Gerrit-Comment-Date: Sat, 14 Sep 2024 16:33:11 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org>

1 0

[M] Change in flashrom[main]: More adequate progress
by Anastasia Klimchuk (Code Review) 14 Sep '24

14 Sep '24

Attention is currently required from: Aarya, Nikolai Artemiev, Peter Marheine. Anastasia Klimchuk has posted comments on this change by Sergii Dmytruk. ( https://review.coreboot.org/c/flashrom/+/84102?usp=email ) Change subject: More adequate progress ...................................................................... Patch Set 1: (1 comment) Patchset: PS1: Sergii, thanks a lot for the patch, that's a great help. You said it's fine to take over, I think I will take the offer. There are few small changes that can be extracted in separate small commits (like updating comments), I would do this first if you don't mind. This would make the patch smaller and also more focused. I added comments, mostly because I want to understand your idea better. If you could reply, that would be great, thanks! -- To view, visit https://review.coreboot.org/c/flashrom/+/84102?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: If1e40fc97f443c4f0c0501cef11cff1f3f84c051 Gerrit-Change-Number: 84102 Gerrit-PatchSet: 1 Gerrit-Owner: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com> Gerrit-Reviewer: Aarya <aarya.chaumal(a)gmail.com> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Reviewer: Peter Marheine <pmarheine(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Aarya <aarya.chaumal(a)gmail.com> Gerrit-Attention: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Attention: Peter Marheine <pmarheine(a)chromium.org> Gerrit-Comment-Date: Sat, 14 Sep 2024 13:41:55 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No

1 0

[M] Change in flashrom[main]: More adequate progress
by Anastasia Klimchuk (Code Review) 14 Sep '24

14 Sep '24

Attention is currently required from: Sergii Dmytruk. Anastasia Klimchuk has posted comments on this change by Sergii Dmytruk. ( https://review.coreboot.org/c/flashrom/+/84102?usp=email ) Change subject: More adequate progress ...................................................................... Patch Set 1: (9 comments) This change is ready for review. Patchset: PS1: A comment to fix the tests Commit Message: https://review.coreboot.org/c/flashrom/+/84102/comment/280159dd_f1975f46?us… : PS1, Line 46: : CLI shares terminal with the rest of the code What does it mean? I don't understand :( File dediprog.c: https://review.coreboot.org/c/flashrom/+/84102/comment/d434306c_36c7d7c6?us… : PS1, Line 596: 256 wow thanks for updating the comment! :) I will extract this into a separate commit File dummyflasher.c: https://review.coreboot.org/c/flashrom/+/84102/comment/62878381_3cfaaaa5?us… : PS1, Line 58: unsigned long long delay_ns; This is changing to nano seconds, is it to be able to set a more fine-grained delay? In the example script, you use `freq=64mhz`, but I think 64mhz would be possible to do even before? File en29lv640b.c: https://review.coreboot.org/c/flashrom/+/84102/comment/79d1f9ed_c73759b3?us… : PS1, Line 29: /* chunksize is 2 */ This can also be a separate commit which updates the comment only. File include/flash.h: https://review.coreboot.org/c/flashrom/+/84102/comment/7b737178_4be94ba0?us… : PS1, Line 544: struct stage_progress { : size_t current; : size_t total; : }; And can this go to libflashrom.h , where the struct flashrom_progress is already? https://review.coreboot.org/c/flashrom/+/84102/comment/9ce51976_d1b034e8?us… : PS1, Line 587: struct stage_progress stage_progress[FLASHROM_PROGRESS_NR]; Maybe this can go inside `struct flashrom_progress` ? Why do we need two structs on the same level, and they both are about progress. If they have to go together, they better be inside the same struct. File linux_mtd.c: https://review.coreboot.org/c/flashrom/+/84102/comment/135d110c_3f5a4bf3?us… : PS1, Line 298: update_progress(flash, FLASHROM_PROGRESS_ERASE, u + data->erasesize, len); Why you removed this? Should it be > update_progress(flash, FLASHROM_PROGRESS_ERASE, data->erasesize); File spi.c: https://review.coreboot.org/c/flashrom/+/84102/comment/b5251ee8_c1d8a94f?us… : PS1, Line 118: update_progress(flash, FLASHROM_PROGRESS_READ, start - start_address + to_read, end_address); Why removing, should this be > update_progress(flash, FLASHROM_PROGRESS_READ, to_read); -- To view, visit https://review.coreboot.org/c/flashrom/+/84102?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: If1e40fc97f443c4f0c0501cef11cff1f3f84c051 Gerrit-Change-Number: 84102 Gerrit-PatchSet: 1 Gerrit-Owner: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Sergii Dmytruk <sergii.dmytruk(a)3mdeb.com> Gerrit-Comment-Date: Sat, 14 Sep 2024 13:41:21 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No

1 0

[S] Change in flashrom[main]: ichspi: Restore Sector erase opcode after send command
by Anastasia Klimchuk (Code Review) 13 Sep '24

13 Sep '24

Attention is currently required from: Bill XIE, Nikolai Artemiev. Anastasia Klimchuk has posted comments on this change by Bill XIE. ( https://review.coreboot.org/c/flashrom/+/84253?usp=email ) Change subject: ichspi: Restore Sector erase opcode after send command ...................................................................... Patch Set 1: (3 comments) Patchset: PS1: Bill, thank you so much for the patch! Especially, that you provided all the details of your debugging, and logs in the ticket, it is so much useful! I think you found a real issue. I spent some time looking through everything, and I want to find the right solution (and add some more people to the patch and discuss). I understand that you made the patch and it unblocks your work, so there is no urgency from your side? It might be another week or two to discuss. Also, I added comments, but they are not necessarily for you (hope this doesn't sound too odd :) ). File ichspi.c: https://review.coreboot.org/c/flashrom/+/84253/comment/33d3cb94_84ad1e1e?us… : PS1, Line 402: static OPCODE POSSIBLE_OPCODES[] = { What is the meaning of this array ? It is called POSSIBLE_OPCODES , but for example in the logs of the linked bug, during probing, opcodes are reprogrammed to 0x83, 0x5A, 0x15, 0xAB - and none of these is in the POSSIBLE_OPCODES . https://review.coreboot.org/c/flashrom/+/84253/comment/d10bdf75_2b8c52ab?us… : PS1, Line 664: int oppos = 2; // use original JEDEC_BE_D8 offset I have questions in my head 1) Why hardcoded offset 2 is chosen to reprogram? I understand that something has to be reprogrammed. But why specifically index 2 was chosen? There is "reprogramming on the fly", but there is no "restore reprogramming", so probably the idea was that if opcode is discovered missing, it would be reprogrammed back. However, I think erase opcodes are not good candidates for reprogramming, because if one of them is missing everything still works, erase works with what's remaining available opcodes. Which breaks the idea of erase optimisations that we have now (but original code goes back to 2010, maybe it wasn't relevant at that time). If I were to pick one of the opcodes in default configuration, to be used for reprogramming, I would pick offset 4, JEDEC_REMS "Read Electronic Manufacturer Signature". I don't think it's used often, also if it's missing it will be discovered missing and reprogrammed back. Can we change offset 2 to 4 ? 2) This line was written in Oct 2010 (https://github.com/flashrom/flashrom/commit/738e252112271f63c8ad4c9a135cfe1…) , but since then the offset of JEDEC_BE_D8 has changed, see patch https://review.coreboot.org/c/flashrom/+/34689 (Aug 2019) After this patch , offset 2 is no longer JEDEC_BE_D8, but now it is JEDEC_SE. However, reprogram_opcode_on_the_fly still uses hard-coded offset 2 for re-programming. Is this fine? 3) And of course I am also wondering, how is that no one noticed this before? Is this perhaps most often the whole chip is erased? -- To view, visit https://review.coreboot.org/c/flashrom/+/84253?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: I3fc831fc072e2af9265835cb2f71bf8c222c6a64 Gerrit-Change-Number: 84253 Gerrit-PatchSet: 1 Gerrit-Owner: Bill XIE <persmule(a)hardenedlinux.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: Bill XIE <persmule(a)hardenedlinux.org> Gerrit-Attention: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Comment-Date: Fri, 13 Sep 2024 13:55:18 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No

1 0

[M] Change in flashrom[main]: Fix FEATURE_NO_ERASE chips and add test for them
by Peter Marheine (Code Review) 12 Sep '24

12 Sep '24

Attention is currently required from: Anastasia Klimchuk, Miklós Márton. Peter Marheine has posted comments on this change by Anastasia Klimchuk. ( https://review.coreboot.org/c/flashrom/+/84203?usp=email ) Change subject: Fix FEATURE_NO_ERASE chips and add test for them ...................................................................... Patch Set 6: Code-Review+2 -- To view, visit https://review.coreboot.org/c/flashrom/+/84203?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: I582fe00da0715e9b5e92fcc9d15d5a90a2615117 Gerrit-Change-Number: 84203 Gerrit-PatchSet: 6 Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Miklós Márton <martonmiklosqdev(a)gmail.com> Gerrit-Reviewer: Peter Marheine <pmarheine(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Miklós Márton <martonmiklosqdev(a)gmail.com> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Comment-Date: Thu, 12 Sep 2024 23:52:17 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes

1 0

[M] Change in flashrom[main]: Fix FEATURE_NO_ERASE chips and add test for them
by Anastasia Klimchuk (Code Review) 12 Sep '24

12 Sep '24

Attention is currently required from: Miklós Márton, Peter Marheine. Anastasia Klimchuk has posted comments on this change by Anastasia Klimchuk. ( https://review.coreboot.org/c/flashrom/+/84203?usp=email ) Change subject: Fix FEATURE_NO_ERASE chips and add test for them ...................................................................... Patch Set 6: (1 comment) File include/flash.h: https://review.coreboot.org/c/flashrom/+/84203/comment/8a6060f0_9c7715a2?us… : PS5, Line 152: * Feature indicates that the chip does not require erase before writing. : * For implementation details, see spi95.c and search for M95M02 definition in flashchips.c. > That's reasonable, but I think we can spend a few more words to include much of the explanation we'v […] Thank you so much! I updated the comment -- To view, visit https://review.coreboot.org/c/flashrom/+/84203?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: I582fe00da0715e9b5e92fcc9d15d5a90a2615117 Gerrit-Change-Number: 84203 Gerrit-PatchSet: 6 Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Miklós Márton <martonmiklosqdev(a)gmail.com> Gerrit-Reviewer: Peter Marheine <pmarheine(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Miklós Márton <martonmiklosqdev(a)gmail.com> Gerrit-Attention: Peter Marheine <pmarheine(a)chromium.org> Gerrit-Comment-Date: Thu, 12 Sep 2024 05:29:31 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Peter Marheine <pmarheine(a)chromium.org>

1 0

[M] Change in flashrom[main]: Fix FEATURE_NO_ERASE chips and add test for them
by Anastasia Klimchuk (Code Review) 12 Sep '24

12 Sep '24

Attention is currently required from: Anastasia Klimchuk, Miklós Márton. Hello Miklós Márton, Peter Marheine, build bot (Jenkins), I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/flashrom/+/84203?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: Fix FEATURE_NO_ERASE chips and add test for them ...................................................................... Fix FEATURE_NO_ERASE chips and add test for them New check was added to `check_block_eraser` in commit 0f389aea9e630c3b21547a5dd8dbe572a8502853 but it was not handling FEATURE_NO_ERASE chips. This patch fixes processing such chips and adds test to run write and verify with dummyflasher for FEATURE_NO_ERASE chips. Ticket: https://ticket.coreboot.org/issues/553 Change-Id: I582fe00da0715e9b5e92fcc9d15d5a90a2615117 Signed-off-by: Anastasia Klimchuk <aklm(a)flashrom.org> --- M flashrom.c M include/flash.h M tests/chip.c M tests/tests.c M tests/tests.h 5 files changed, 80 insertions(+), 7 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/03/84203/6 -- To view, visit https://review.coreboot.org/c/flashrom/+/84203?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email Gerrit-MessageType: newpatchset Gerrit-Project: flashrom Gerrit-Branch: main Gerrit-Change-Id: I582fe00da0715e9b5e92fcc9d15d5a90a2615117 Gerrit-Change-Number: 84203 Gerrit-PatchSet: 6 Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Miklós Márton <martonmiklosqdev(a)gmail.com> Gerrit-Reviewer: Peter Marheine <pmarheine(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Miklós Márton <martonmiklosqdev(a)gmail.com> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org>

1 0

[M] Change in flashrom[main]: Fix FEATURE_NO_ERASE chips and add test for them
by Peter Marheine (Code Review) 12 Sep '24

12 Sep '24

Attention is currently required from: Anastasia Klimchuk, Miklós Márton. Peter Marheine has posted comments on this change by Anastasia Klimchuk. ( https://review.coreboot.org/c/flashrom/+/84203?usp=email ) Change subject: Fix FEATURE_NO_ERASE chips and add test for them ...................................................................... Patch Set 5: (1 comment) File include/flash.h: https://review.coreboot.org/c/flashrom/+/84203/comment/3e988205_64795c4a?us… : PS5, Line 152: * Feature indicates that the chip does not require erase before writing. : * For implementation details, see spi95.c and search for M95M02 definition in flashchips.c. That's reasonable, but I think we can spend a few more words to include much of the explanation we've done here to minimize possible confusion. Pointing to a specific part of the code also seems error-prone (if it were to change), and this flag doesn't get used much so providing more general information seems fine: users can easily search the code for uses of this flag to find the implementation. ```suggestion * Feature indicates that the chip does not require erase before writing: * write operations can set any bit to any value without first doing an erase, * but bulk erase operations may still be supported. * * EEPROMs usually behave this way (compare to Flash, which requires erase), * for example the ST M95M02. ``` -- To view, visit https://review.coreboot.org/c/flashrom/+/84203?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: I582fe00da0715e9b5e92fcc9d15d5a90a2615117 Gerrit-Change-Number: 84203 Gerrit-PatchSet: 5 Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Miklós Márton <martonmiklosqdev(a)gmail.com> Gerrit-Reviewer: Peter Marheine <pmarheine(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Miklós Márton <martonmiklosqdev(a)gmail.com> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Comment-Date: Thu, 12 Sep 2024 04:11:02 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No

1 0

[M] Change in flashrom[main]: Fix FEATURE_NO_ERASE chips and add test for them
by Anastasia Klimchuk (Code Review) 12 Sep '24

12 Sep '24

Attention is currently required from: Miklós Márton, Peter Marheine. Anastasia Klimchuk has posted comments on this change by Anastasia Klimchuk. ( https://review.coreboot.org/c/flashrom/+/84203?usp=email ) Change subject: Fix FEATURE_NO_ERASE chips and add test for them ...................................................................... Patch Set 5: (2 comments) File flashrom.c: https://review.coreboot.org/c/flashrom/+/84203/comment/7908cf76_19f413eb?us… : PS3, Line 487: for (int i = 0; opcode[i]; i++) { > > I would say "chips that don't have erase operation capability, and no erase opcodes"? […] Thank you so much for explanations! It's so useful. I made a first attempt to add a comment for FEATURE_NO_ERASE in the header include/flash.h. But please tell me if it's a correct comment. File flashrom.c: https://review.coreboot.org/c/flashrom/+/84203/comment/22c7d77c_7399555d?us… : PS4, Line 485: !(flash->chip->feature_bits & FEATURE_NO_ERASE) > I don't think you need this check, and for a chip that doesn't need erase but supports a fast erase […] Yep, removed the check. I wasn't entirely sure which way to go, so I left it in the previous patchset (but now removed). Checking opcode for null is enough. -- To view, visit https://review.coreboot.org/c/flashrom/+/84203?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: I582fe00da0715e9b5e92fcc9d15d5a90a2615117 Gerrit-Change-Number: 84203 Gerrit-PatchSet: 5 Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Miklós Márton <martonmiklosqdev(a)gmail.com> Gerrit-Reviewer: Peter Marheine <pmarheine(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Miklós Márton <martonmiklosqdev(a)gmail.com> Gerrit-Attention: Peter Marheine <pmarheine(a)chromium.org> Gerrit-Comment-Date: Thu, 12 Sep 2024 03:34:15 +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>

1 0

2024

2023

2022

2021

2020

2019

2018

2017

flashrom-gerrit