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

[S] Change in flashrom[main]: Correct get_flash_region() to use inclusive upper bounds
by Peter Marheine (Code Review) 17 May '24

17 May '24

Attention is currently required from: Aarya. Peter Marheine has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/82496?usp=email ) Change subject: Correct get_flash_region() to use inclusive upper bounds ...................................................................... Correct get_flash_region() to use inclusive upper bounds get_flash_region() emits a struct flash_region, which uses chipoff_t for the start and end addresses of a region. chipoff_t is defined as a valid flash address, so it was wrong to be setting the end address to start + len; this is clearly wrong in the case where there is a single region because setting end to the flash size generates an address that is beyond the end of the chip (due to zero-indexing). This changes the one actual implementation of .get_region in ichspi.c to use inclusive upper bounds, and corrects all callers of get_flash_region() to treat the upper bounds as inclusive. Overall this reduces complexity slightly by removing more downward adjustments by 1 than it needs to add upward adjustments. Change-Id: Ia0ca867aef33b598c2697fc264807fa5e29683ec --- M erasure_layout.c M flashrom.c M ichspi.c 3 files changed, 20 insertions(+), 20 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/96/82496/1 diff --git a/erasure_layout.c b/erasure_layout.c index 1dd6b60..ef7987a 100644 --- a/erasure_layout.c +++ b/erasure_layout.c @@ -316,13 +316,13 @@ struct flash_region region; get_flash_region(flashctx, addr, &region); - len = min(start_addr + block_len, region.end) - addr; + len = min(start_addr + block_len, region.end + 1) - addr; if (region.write_prot) { msg_gdbg("%s: cannot erase inside %s " "region (%#08"PRIx32"..%#08"PRIx32"), skipping range (%#08x..%#08x).\n", __func__, region.name, - region.start, region.end - 1, + region.start, region.end, addr, addr + len - 1); free(region.name); continue; @@ -331,7 +331,7 @@ msg_gdbg("%s: %s region (%#08"PRIx32"..%#08"PRIx32") is " "writable, erasing range (%#08x..%#08x).\n", __func__, region.name, - region.start, region.end - 1, + region.start, region.end, addr, addr + len - 1); free(region.name); diff --git a/flashrom.c b/flashrom.c index a056929..a8ad3f0 100644 --- a/flashrom.c +++ b/flashrom.c @@ -379,7 +379,7 @@ } else { region->name = strdup(""); region->start = 0; - region->end = flashrom_flash_getsize(flash); + region->end = flashrom_flash_getsize(flash) - 1; region->read_prot = false; region->write_prot = false; } @@ -388,12 +388,12 @@ int check_for_unwritable_regions(const struct flashctx *flash, unsigned int start, unsigned int len) { struct flash_region region; - for (unsigned int addr = start; addr < start + len; addr = region.end) { + for (unsigned int addr = start; addr < start + len; addr = region.end + 1) { get_flash_region(flash, addr, &region); if (region.write_prot) { msg_gerr("%s: cannot write/erase inside %s region (%#08"PRIx32"..%#08"PRIx32").\n", - __func__, region.name, region.start, region.end - 1); + __func__, region.name, region.start, region.end); free(region.name); return -1; } @@ -596,14 +596,14 @@ struct flash_region region; get_flash_region(flash, addr, &region); - read_len = min(start + len, region.end) - addr; + read_len = min(start + len, region.end + 1) - addr; uint8_t *rbuf = buf + addr - start; if (region.read_prot) { if (flash->flags.skip_unreadable_regions) { msg_gdbg("%s: cannot read inside %s region (%#08"PRIx32"..%#08"PRIx32"), " "filling (%#08x..%#08x) with erased value instead.\n", - __func__, region.name, region.start, region.end - 1, + __func__, region.name, region.start, region.end, addr, addr + read_len - 1); free(region.name); @@ -612,12 +612,12 @@ } msg_gerr("%s: cannot read inside %s region (%#08"PRIx32"..%#08"PRIx32").\n", - __func__, region.name, region.start, region.end - 1); + __func__, region.name, region.start, region.end); free(region.name); return -1; } msg_gdbg("%s: %s region (%#08"PRIx32"..%#08"PRIx32") is readable, reading range (%#08x..%#08x).\n", - __func__, region.name, region.start, region.end - 1, addr, addr + read_len - 1); + __func__, region.name, region.start, region.end, addr, addr + read_len - 1); free(region.name); read_func_t *read_func = lookup_read_func_ptr(flash->chip); @@ -665,25 +665,25 @@ for (size_t addr = start; addr < start + len; addr += read_len) { struct flash_region region; get_flash_region(flash, addr, &region); - read_len = min(start + len, region.end) - addr; + read_len = min(start + len, region.end + 1) - addr; if ((region.write_prot && flash->flags.skip_unwritable_regions) || (region.read_prot && flash->flags.skip_unreadable_regions)) { msg_gdbg("%s: Skipping verification of %s region (%#08"PRIx32"..%#08"PRIx32")\n", - __func__, region.name, region.start, region.end - 1); + __func__, region.name, region.start, region.end); free(region.name); continue; } if (region.read_prot) { msg_gerr("%s: Verification imposible because %s region (%#08"PRIx32"..%#08"PRIx32") is unreadable.\n", - __func__, region.name, region.start, region.end - 1); + __func__, region.name, region.start, region.end); free(region.name); goto out_free; } msg_gdbg("%s: Verifying %s region (%#08"PRIx32"..%#08"PRIx32")\n", - __func__, region.name, region.start, region.end - 1); + __func__, region.name, region.start, region.end); free(region.name); ret = read_flash(flash, readbuf, addr, read_len); @@ -1046,18 +1046,18 @@ struct flash_region region; get_flash_region(flash, addr, &region); - write_len = min(start + len, region.end) - addr; + write_len = min(start + len, region.end + 1) - addr; const uint8_t *rbuf = buf + addr - start; if (region.write_prot) { msg_gdbg("%s: cannot write inside %s region (%#08"PRIx32"..%#08"PRIx32"), skipping (%#08x..%#08x).\n", - __func__, region.name, region.start, region.end - 1, addr, addr + write_len - 1); + __func__, region.name, region.start, region.end, addr, addr + write_len - 1); free(region.name); continue; } msg_gdbg("%s: %s region (%#08"PRIx32"..%#08"PRIx32") is writable, writing range (%#08x..%#08x).\n", - __func__, region.name, region.start, region.end - 1, addr, addr + write_len - 1); + __func__, region.name, region.start, region.end, addr, addr + write_len - 1); write_func_t *write_func = lookup_write_func_ptr(flash->chip); int ret = write_func(flash, rbuf, addr, write_len); @@ -1511,7 +1511,7 @@ struct flash_region region; get_flash_region(flashctx, addr, &region); - len = min(info->erase_start + erase_len, region.end) - addr; + len = min(info->erase_start + erase_len, region.end + 1) - addr; if (region.write_prot) { msg_gdbg("%s: cannot erase inside %s region (%#08"PRIx32"..%#08"PRIx32"), skipping range (%#08x..%#08x).\n", diff --git a/ichspi.c b/ichspi.c index bee5ec9..249e966 100644 --- a/ichspi.c +++ b/ichspi.c @@ -1451,12 +1451,12 @@ * fd_regions[i] ends before addr, constrain * region->start so that it does not overlap. */ - region->start = max(region->start, limit + 1); + region->start = max(region->start, limit); } else { /* fd_regions[i] contains addr, copy to *region. */ name = fd_regions[i].name; region->start = base; - region->end = limit + 1; + region->end = limit; region->read_prot = (level == LOCKED) || (level == READ_PROT); region->write_prot = (level == LOCKED) || (level == WRITE_PROT); break; -- To view, visit https://review.coreboot.org/c/flashrom/+/82496?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: Ia0ca867aef33b598c2697fc264807fa5e29683ec Gerrit-Change-Number: 82496 Gerrit-PatchSet: 1 Gerrit-Owner: Peter Marheine <pmarheine(a)chromium.org> Gerrit-Reviewer: Aarya <aarya.chaumal(a)gmail.com> Gerrit-Attention: Aarya <aarya.chaumal(a)gmail.com> Gerrit-MessageType: newchange

1 0

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

17 May '24

Attention is currently required from: Aarya, Alexander Goncharov, Anastasia Klimchuk, Nikolai Artemiev. Hello Aarya, Alexander Goncharov, Anastasia Klimchuk, Nikolai Artemiev, build bot (Jenkins), I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/flashrom/+/82393?usp=email to look at the new patch set (#7). The following approvals got outdated and were removed: Verified+1 by build bot (Jenkins) Change subject: erasure_layout: Fix get_flash_region bug ...................................................................... erasure_layout: Fix get_flash_region bug When flash regions are protected, erase could incorrectly erase regions which were meant to be protected by requesting the correct size but using an erase opcode with coarser granularity than desired (for instance using a 16-byte erase command while attempting to erase only 8 bytes). This fixes that by exchanging the nesting of the loops over erase blocks and flash regions. Old: - Select erasefns - Loop over blocks to erase for each selected erasefn - Loop over programmer flash regions within erase block - Erase regions (may fail since selected erasefn will be too big if flash region is smaller than erase block) New: - Loop over programmer flash regions within erase block - Select erasefns within programer flash region - Loop over blocks to erase for each selected erasefn - Erase regions Eraser selection and erasing has also been factored out into a helper function to manage nesting depth. TEST=New test cases pass, whereas some of them fail without the changes to erasure_layout.c BUG=https://ticket.coreboot.org/issues/525 Change-Id: Ic5bf9d0f0e4a94c48d6f6e74e3cb9cccdc7adec9 Co-authored-by: Nikolai Artemiev <nartemiev(a)google.com> Co-authored-by: Anastasia Klimchuk <aklm(a)flashrom.org> Signed-off-by: Nikolai Artemiev <nartemiev(a)google.com> Signed-off-by: Anastasia Klimchuk <aklm(a)flashrom.org> Signed-off-by: Peter Marheine <pmarheine(a)chromium.org> --- M erasure_layout.c M tests/erase_func_algo.c M tests/tests.c M tests/tests.h 4 files changed, 450 insertions(+), 96 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/93/82393/7 -- To view, visit https://review.coreboot.org/c/flashrom/+/82393?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: main Gerrit-Change-Id: Ic5bf9d0f0e4a94c48d6f6e74e3cb9cccdc7adec9 Gerrit-Change-Number: 82393 Gerrit-PatchSet: 7 Gerrit-Owner: Peter Marheine <pmarheine(a)chromium.org> Gerrit-Reviewer: Aarya <aarya.chaumal(a)gmail.com> Gerrit-Reviewer: Alexander Goncharov <chat(a)joursoir.net> 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: Aarya <aarya.chaumal(a)gmail.com> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Attention: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Attention: Alexander Goncharov <chat(a)joursoir.net> Gerrit-MessageType: newpatchset

1 0

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

17 May '24

Attention is currently required from: Aarya, Alexander Goncharov, Anastasia Klimchuk, Nikolai Artemiev. Peter Marheine has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/82393?usp=email ) Change subject: erasure_layout: Fix get_flash_region bug ...................................................................... Patch Set 6: (7 comments) File erasure_layout.c: https://review.coreboot.org/c/flashrom/+/82393/comment/f4b3418e_02355139 : PS5, Line 240: > May I ask you to also add doc comment here , most importantly to say that region_end is inclusive I'm a little hesitant to, because `chipoff_t` implies inclusive endpoints. We could just as easily add the same comment to every use of that type. https://review.coreboot.org/c/flashrom/+/82393/comment/4e8f44f2_39889a85 : PS5, Line 320: * @param region_end end address of the region > I know it wasn't here before, but let's document it here that end is inclusive. As above, `chipoff_t` implies this. > Also, maybe you can check with your own eyes that region end is inclusive? I checked (by the callsites of erase_write) but just in case! While working on this I hacked debug logging to be included while running tests, which helped me identify where things were off by one; I'm confident that it's correct now, since the debug logs had consistent addresses. https://review.coreboot.org/c/flashrom/+/82393/comment/621d1b03_b21976b5 : PS5, Line 369: len = min(region_end, region.end - 1) - addr + 1; > Maybe add a comment above this line that […] I actually think `ich_get_region` (the only existing implementation of `get_region`) is wrong here, because `flash_region` uses `chipoff_t` but initializes `end` to the size of the flash which is not a valid `chipoff_t`. Both should be inclusive. On further investigation, it looks like `get_flash_region` has always misused `chipoff_t` so it and all of its users need to be fixed. File tests/erase_func_algo.c: https://review.coreboot.org/c/flashrom/+/82393/comment/83609822_7ee3c068 : PS5, Line 1042: _tagged > Maybe without `_tagged` ? the rest of functions have suffix, so there will be no clash Done https://review.coreboot.org/c/flashrom/+/82393/comment/c872ca8c_743f90e9 : PS5, Line 1082: #define ERASE_PROTECTED(i) static int block_erase_chip_with_protected_region_ ## i \ > That's what I was thinking for the previous patch, a macro loop! :) Acknowledged https://review.coreboot.org/c/flashrom/+/82393/comment/4aef587f_0b15267e : PS5, Line 1092: void erase_unwritable_regions_skipflag_on_test_success(void **state) > I think this qualifies for a comment: […] Done https://review.coreboot.org/c/flashrom/+/82393/comment/52e2344b_0e7c7834 : PS5, Line 1172: void erase_unwritable_regions_skipflag_off_test_success(void **state) : { : // flashrom_flag_set(&flashctx, FLASHROM_FLAG_SKIP_UNWRITABLE_REGIONS, false); : } > I meant to write this test too, but didn't finish it. […] Done -- To view, visit https://review.coreboot.org/c/flashrom/+/82393?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: main Gerrit-Change-Id: Ic5bf9d0f0e4a94c48d6f6e74e3cb9cccdc7adec9 Gerrit-Change-Number: 82393 Gerrit-PatchSet: 6 Gerrit-Owner: Peter Marheine <pmarheine(a)chromium.org> Gerrit-Reviewer: Aarya <aarya.chaumal(a)gmail.com> Gerrit-Reviewer: Alexander Goncharov <chat(a)joursoir.net> 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: Aarya <aarya.chaumal(a)gmail.com> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Attention: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Attention: Alexander Goncharov <chat(a)joursoir.net> Gerrit-Comment-Date: Fri, 17 May 2024 03:47:25 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-MessageType: comment

1 0

[M] Change in flashrom[main]: erasure_layout: don't copy region buffers if they're null/zero-size
by Anastasia Klimchuk (Code Review) 17 May '24

17 May '24

Anastasia Klimchuk has submitted this change. ( https://review.coreboot.org/c/flashrom/+/81548?usp=email ) Change subject: erasure_layout: don't copy region buffers if they're null/zero-size ...................................................................... erasure_layout: don't copy region buffers if they're null/zero-size memcpy() function expects 2nd parameter to be non-null. Make sure that the pointer is non-null before passing it to the function. Also move allocations under if conditions to avoid allocating memory for a potentially 0 size. Found-by: scan-build, clang v17.0.6 Signed-off-by: Alexander Goncharov <chat(a)joursoir.net> Change-Id: I99aad4119f9c11bea75e3419926cc833bc1f68c5 Reviewed-on: https://review.coreboot.org/c/flashrom/+/81548 Reviewed-by: Anastasia Klimchuk <aklm(a)chromium.org> Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org> --- M erasure_layout.c 1 file changed, 30 insertions(+), 24 deletions(-) Approvals: Anastasia Klimchuk: Looks good to me, approved build bot (Jenkins): Verified diff --git a/erasure_layout.c b/erasure_layout.c index 1dd6b60..e9bfa86 100644 --- a/erasure_layout.c +++ b/erasure_layout.c @@ -260,29 +260,31 @@ align_region(erase_layout, flashctx, &region_start, &region_end); uint8_t *old_start_buf = NULL, *old_end_buf = NULL; - 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; - } + const size_t start_buf_len = old_start - region_start; + const size_t end_buf_len = region_end - old_end; - if (old_start - region_start) { - read_flash(flashctx, curcontents + region_start, region_start, old_start - region_start); - memcpy(old_start_buf, newcontents + region_start, old_start - region_start); - memcpy(newcontents + region_start, curcontents + region_start, old_start - region_start); + if (start_buf_len) { + old_start_buf = (uint8_t *)malloc(start_buf_len); + if (!old_start_buf) { + msg_cerr("Not enough memory!\n"); + ret = -1; + goto _end; + } + read_flash(flashctx, curcontents + region_start, region_start, start_buf_len); + memcpy(old_start_buf, newcontents + region_start, start_buf_len); + memcpy(newcontents + region_start, curcontents + region_start, start_buf_len); } - if (region_end - old_end) { + if (end_buf_len) { chipoff_t end_offset = old_end + 1; - read_flash(flashctx, curcontents + end_offset, end_offset, region_end - old_end); - memcpy(old_end_buf, newcontents + end_offset, region_end - old_end); - memcpy(newcontents + end_offset, curcontents + end_offset, region_end - old_end); + old_end_buf = (uint8_t *)malloc(end_buf_len); + if (!old_end_buf) { + msg_cerr("Not enough memory!\n"); + ret = -1; + goto _end; + } + read_flash(flashctx, curcontents + end_offset, end_offset, end_buf_len); + memcpy(old_end_buf, newcontents + end_offset, end_buf_len); + memcpy(newcontents + end_offset, curcontents + end_offset, end_buf_len); } // select erase functions @@ -381,11 +383,15 @@ } _end: - memcpy(newcontents + region_start, old_start_buf, old_start - region_start); - memcpy(newcontents + old_end, old_end_buf, region_end - old_end); + if (old_start_buf) { + memcpy(newcontents + region_start, old_start_buf, start_buf_len); + free(old_start_buf); + } - free(old_start_buf); - free(old_end_buf); + if (old_end_buf) { + memcpy(newcontents + old_end, old_end_buf, end_buf_len); + free(old_end_buf); + } msg_cinfo("Erase/write done from %"PRIx32" to %"PRIx32"\n", region_start, region_end); return ret; -- 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: 4 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-MessageType: merged

1 0

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

17 May '24

Attention is currently required from: Anastasia Klimchuk, Nikolai Artemiev, Thomas Heijligen. Peter Marheine has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/82251?usp=email ) Change subject: tests/erase: record the opcode for each erase ...................................................................... Patch Set 5: (4 comments) File flashrom.c: https://review.coreboot.org/c/flashrom/+/82251/comment/ddd3453b_cee6c526 : PS5, Line 407: 5 > Can this be a macro in flash. […] Done. I also moved the declaration of this into flash.h, alongside the other `g_test_*_injector`s so it should trigger a compile-time error if somebody changes the definition without also changing the declaration. File include/flash.h: https://review.coreboot.org/c/flashrom/+/82251/comment/015dbef3_8ea1a89d : PS5, Line 325: TEST_ERASE_INJECTOR > Maybe remove this value? I know some existing tests are using it, but they can be adjusted. […] Done File tests/erase_func_algo.c: https://review.coreboot.org/c/flashrom/+/82251/comment/d462a9bc_73829dd4 : PS5, Line 198: block_erase_chip, block_erase_chip, block_erase_chip, block_erase_chip, block_erase_chip > What if […] I had avoided doing that just because it requires updating all of the existing test cases, but done now. It does seem worth verifying that, now that we have the capability. File tests/meson.build: https://review.coreboot.org/c/flashrom/+/82251/comment/c873eb11_21789d04 : PS5, Line 127: '-DFLASHROM_TEST', > Just to test , have you built with this option undefined? […] Yes, building the regular flashrom binary still works. I added this because it may not be clear that the features it currently guards are intended only to enable unit testing. Guarding them with a symbol that we only define when building tests makes that clearer, and doesn't add much complexity. -- To view, visit https://review.coreboot.org/c/flashrom/+/82251?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: main Gerrit-Change-Id: I3983fe42c2e7f06668a1bd20d2db7fafa93b8043 Gerrit-Change-Number: 82251 Gerrit-PatchSet: 5 Gerrit-Owner: Peter Marheine <pmarheine(a)chromium.org> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Reviewer: 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: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Comment-Date: Fri, 17 May 2024 00:47:38 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-MessageType: comment

1 0

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

17 May '24

Attention is currently required from: Nikolai Artemiev, Peter Marheine, Thomas Heijligen. Hello Anastasia Klimchuk, Nikolai Artemiev, Thomas Heijligen, build bot (Jenkins), I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/flashrom/+/82251?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: tests/erase: record the opcode for each erase ...................................................................... tests/erase: record the opcode for each erase This allows tests to verify that the correct opcode is used when erasing, which is required to unit-test the fix to issue #525 where in some situations an incorrect erase opcode will be used. BUG=https://ticket.coreboot.org/issues/525 Change-Id: I3983fe42c2e7f06668a1bd20d2db7fafa93b8043 Signed-off-by: Peter Marheine <pmarheine(a)chromium.org> --- M flashrom.c M include/flash.h M tests/chip.c M tests/erase_func_algo.c M tests/meson.build 5 files changed, 155 insertions(+), 58 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/51/82251/6 -- To view, visit https://review.coreboot.org/c/flashrom/+/82251?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: main Gerrit-Change-Id: I3983fe42c2e7f06668a1bd20d2db7fafa93b8043 Gerrit-Change-Number: 82251 Gerrit-PatchSet: 6 Gerrit-Owner: Peter Marheine <pmarheine(a)chromium.org> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Reviewer: 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: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Attention: Peter Marheine <pmarheine(a)chromium.org> Gerrit-MessageType: newpatchset

1 0

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

17 May '24

Attention is currently required from: Aarya, Alexander Goncharov, Nikolai Artemiev, Peter Marheine. Hello Aarya, Alexander Goncharov, Anastasia Klimchuk, Nikolai Artemiev, build bot (Jenkins), I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/flashrom/+/82393?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: erasure_layout: Fix get_flash_region bug ...................................................................... erasure_layout: Fix get_flash_region bug When flash regions are protected, erase could incorrectly erase regions which were meant to be protected by requesting the correct size but using an erase opcode with coarser granularity than desired (for instance using a 16-byte erase command while attempting to erase only 8 bytes). This fixes that by exchanging the nesting of the loops over erase blocks and flash regions. Old: - Select erasefns - Loop over blocks to erase for each selected erasefn - Loop over programmer flash regions within erase block - Erase regions (may fail since selected erasefn will be too big if flash region is smaller than erase block) New: - Loop over programmer flash regions within erase block - Select erasefns within programer flash region - Loop over blocks to erase for each selected erasefn - Erase regions Eraser selection and erasing has also been factored out into a helper function to manage nesting depth. TEST=New test cases pass, whereas some of them fail without the changes to erasure_layout.c BUG=https://ticket.coreboot.org/issues/525 Change-Id: Ic5bf9d0f0e4a94c48d6f6e74e3cb9cccdc7adec9 Co-authored-by: Nikolai Artemiev <nartemiev(a)google.com> Co-authored-by: Anastasia Klimchuk <aklm(a)flashrom.org> Signed-off-by: Nikolai Artemiev <nartemiev(a)google.com> Signed-off-by: Anastasia Klimchuk <aklm(a)flashrom.org> Signed-off-by: Peter Marheine <pmarheine(a)chromium.org> --- M erasure_layout.c M tests/erase_func_algo.c M tests/tests.c M tests/tests.h 4 files changed, 441 insertions(+), 96 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/93/82393/6 -- To view, visit https://review.coreboot.org/c/flashrom/+/82393?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: main Gerrit-Change-Id: Ic5bf9d0f0e4a94c48d6f6e74e3cb9cccdc7adec9 Gerrit-Change-Number: 82393 Gerrit-PatchSet: 6 Gerrit-Owner: Peter Marheine <pmarheine(a)chromium.org> Gerrit-Reviewer: Aarya <aarya.chaumal(a)gmail.com> Gerrit-Reviewer: Alexander Goncharov <chat(a)joursoir.net> 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: Aarya <aarya.chaumal(a)gmail.com> Gerrit-Attention: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Attention: Peter Marheine <pmarheine(a)chromium.org> Gerrit-Attention: Alexander Goncharov <chat(a)joursoir.net> Gerrit-MessageType: newpatchset

1 0

[L] Change in flashrom[main]: erasure_layout: Fix get_flash_region bug
by Anastasia Klimchuk (Code Review) 16 May '24

16 May '24

Attention is currently required from: Aarya, Alexander Goncharov, Nikolai Artemiev, Peter Marheine. Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/82393?usp=email ) Change subject: erasure_layout: Fix get_flash_region bug ...................................................................... Patch Set 5: (8 comments) File erasure_layout.c: https://review.coreboot.org/c/flashrom/+/82393/comment/c57584b4_2953c93e : PS5, Line 240: May I ask you to also add doc comment here , most importantly to say that region_end is inclusive https://review.coreboot.org/c/flashrom/+/82393/comment/4ed42d95_b8c1c458 : PS5, Line 320: * @param region_end end address of the region I know it wasn't here before, but let's document it here that *end is inclusive*. those 1-offsets are tricky, but also it will be useful for future readers Also, maybe you can check with your own eyes that region end is inclusive? I checked (by the callsites of erase_write) but just in case! https://review.coreboot.org/c/flashrom/+/82393/comment/970afeab_ee925839 : PS5, Line 335: region_end - region_start If region end is inclusive, this needs to be +1 byte... am I right? (because the 3rd arg is length) It seems we can add one more test case for this, a test case with protected region of 1 last byte https://review.coreboot.org/c/flashrom/+/82393/comment/8f01d70f_1f1c50a9 : PS5, Line 369: len = min(region_end, region.end - 1) - addr + 1; Maybe add a comment above this line that region_end is inclusive, but region.end is exclusive File tests/erase_func_algo.c: https://review.coreboot.org/c/flashrom/+/82393/comment/992a1efb_d6f8d0e4 : PS5, Line 1042: _tagged Maybe without `_tagged` ? the rest of functions have suffix, so there will be no clash https://review.coreboot.org/c/flashrom/+/82393/comment/8a96caaa_9405bcde : PS5, Line 1082: #define ERASE_PROTECTED(i) static int block_erase_chip_with_protected_region_ ## i \ That's what I was thinking for the previous patch, a macro loop! :) https://review.coreboot.org/c/flashrom/+/82393/comment/0f05981b_e52b1ed9 : PS5, Line 1092: void erase_unwritable_regions_skipflag_on_test_success(void **state) I think this qualifies for a comment: This test that runs through the test cases with protected regions on the chip, and with a flag set to skip (i.e. ignore) protected regions. Ignoring means they are left as is, and no opcodes should be sent to any memory blocks inside protected regions. feel free to improve if you want! https://review.coreboot.org/c/flashrom/+/82393/comment/7971c1d9_07e608ab : PS5, Line 1172: void erase_unwritable_regions_skipflag_off_test_success(void **state) : { : // flashrom_flag_set(&flashctx, FLASHROM_FLAG_SKIP_UNWRITABLE_REGIONS, false); : } I meant to write this test too, but didn't finish it. Probably better to remove from this patch, and I will add it later -- To view, visit https://review.coreboot.org/c/flashrom/+/82393?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: main Gerrit-Change-Id: Ic5bf9d0f0e4a94c48d6f6e74e3cb9cccdc7adec9 Gerrit-Change-Number: 82393 Gerrit-PatchSet: 5 Gerrit-Owner: Peter Marheine <pmarheine(a)chromium.org> Gerrit-Reviewer: Aarya <aarya.chaumal(a)gmail.com> Gerrit-Reviewer: Alexander Goncharov <chat(a)joursoir.net> 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: Aarya <aarya.chaumal(a)gmail.com> Gerrit-Attention: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Attention: Peter Marheine <pmarheine(a)chromium.org> Gerrit-Attention: Alexander Goncharov <chat(a)joursoir.net> Gerrit-Comment-Date: Thu, 16 May 2024 12:05:24 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

[S] Change in flashrom[main]: Add documentation for pico-serprog
by Sydney (Code Review) 16 May '24

16 May '24

Attention is currently required from: Anastasia Klimchuk, J. Neuschäfer, Riku Viitanen, Thomas Heijligen. Sydney has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/82229?usp=email ) Change subject: Add documentation for pico-serprog ...................................................................... Patch Set 4: (3 comments) Patchset: PS4: I applied your suggestions on wording and split the sentence into two to make them less ambiguous. Please have a look again if everything is fine now. File doc/supported_hw/supported_prog/serprog/overview.rst: https://review.coreboot.org/c/flashrom/+/82229/comment/c48f1124_0494246b : PS3, Line 94: over PIO > "over" can be misunderstood. […] Done https://review.coreboot.org/c/flashrom/+/82229/comment/bd805fc4_400fd6e9 : PS3, Line 95: pinouts but implements USB descriptors allowing custom udev-rules. : > Nothing wrong with what this _says_, but what it _implies_ is a bit confusing. […] Done -- To view, visit https://review.coreboot.org/c/flashrom/+/82229?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: I457dfec52f89997f64b6c276c50b329359d61b77 Gerrit-Change-Number: 82229 Gerrit-PatchSet: 4 Gerrit-Owner: Sydney <git(a)funkeleinhorn.com> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: J. Neuschäfer <j.neuschaefer(a)gmx.net> Gerrit-Reviewer: Riku Viitanen <riku.viitanen(a)protonmail.com> Gerrit-Reviewer: Thomas Heijligen <src(a)posteo.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Riku Viitanen <riku.viitanen(a)protonmail.com> Gerrit-Attention: J. Neuschäfer <j.neuschaefer(a)gmx.net> Gerrit-Attention: Thomas Heijligen <src(a)posteo.de> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Comment-Date: Thu, 16 May 2024 11:14:42 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: J. Neuschäfer <j.neuschaefer(a)gmx.net> Gerrit-MessageType: comment

1 0

[S] Change in flashrom[main]: Add documentation for pico-serprog
by Sydney (Code Review) 16 May '24

16 May '24

Attention is currently required from: Anastasia Klimchuk, Riku Viitanen, Sydney, Thomas Heijligen. Hello Anastasia Klimchuk, J. Neuschäfer, Riku Viitanen, Thomas Heijligen, build bot (Jenkins), I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/flashrom/+/82229?usp=email to look at the new patch set (#4). The following approvals got outdated and were removed: Code-Review+1 by Anastasia Klimchuk, Verified+1 by build bot (Jenkins) Change subject: Add documentation for pico-serprog ...................................................................... Add documentation for pico-serprog This commit adds documentation for pico-serprog by stacksmashing: https://github.com/stacksmashing/pico-serprog and its fork by Riku_V: https://codeberg.org/Riku_V/pico-serprog to the serprog overview page. Change-Id: I457dfec52f89997f64b6c276c50b329359d61b77 Signed-off-by: Funkeleinhorn <git(a)funkeleinhorn.com> --- M doc/supported_hw/supported_prog/serprog/overview.rst 1 file changed, 12 insertions(+), 0 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/29/82229/4 -- To view, visit https://review.coreboot.org/c/flashrom/+/82229?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: I457dfec52f89997f64b6c276c50b329359d61b77 Gerrit-Change-Number: 82229 Gerrit-PatchSet: 4 Gerrit-Owner: Sydney <git(a)funkeleinhorn.com> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: J. Neuschäfer <j.neuschaefer(a)gmx.net> Gerrit-Reviewer: Riku Viitanen <riku.viitanen(a)protonmail.com> Gerrit-Reviewer: Thomas Heijligen <src(a)posteo.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Riku Viitanen <riku.viitanen(a)protonmail.com> Gerrit-Attention: Thomas Heijligen <src(a)posteo.de> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Attention: Sydney <git(a)funkeleinhorn.com> Gerrit-MessageType: newpatchset

1 0

2024

2023

2022

2021

2020

2019

2018

2017

flashrom-gerrit May 2024