- flashrom-gerrit - mail.coreboot.org

[S] Change in flashrom[main]: flashchips: Add XTX XT25F128B
by Anastasia Klimchuk (Code Review) 01 Jun '24

01 Jun '24

Anastasia Klimchuk has submitted this change. ( https://review.coreboot.org/c/flashrom/+/82676?usp=email ) Change subject: flashchips: Add XTX XT25F128B ...................................................................... flashchips: Add XTX XT25F128B XTX XT25F128B is 128M-bit version of XT25F64B. Tested probe, read, erase and write with FT232H. Datasheet: https://wmsc.lcsc.com/wmsc/upload/file/pdf/v2/lcsc/2304140030_XTX-XT25F128B… Change-Id: I37084bd66bc7a8f93d6533ab0d67aa2528786299 Signed-off-by: Robert Marko <robert.marko(a)sartura.hr> Reviewed-on: https://review.coreboot.org/c/flashrom/+/82676 Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org> Reviewed-by: Anastasia Klimchuk <aklm(a)chromium.org> --- M flashchips.c M include/flashchips.h 2 files changed, 41 insertions(+), 0 deletions(-) Approvals: build bot (Jenkins): Verified Anastasia Klimchuk: Looks good to me, approved diff --git a/flashchips.c b/flashchips.c index 2065746..34e610a 100644 --- a/flashchips.c +++ b/flashchips.c @@ -22178,6 +22178,46 @@ }, { + .vendor = "XTX Technology Limited", + .name = "XT25F128B", + .bustype = BUS_SPI, + .manufacture_id = XTX_ID, + .model_id = XTX_XT25F128B, + .total_size = 16 * 1024, + .page_size = 256, + /* Supports SFDP */ + /* OTP: 4 x 256 bytes */ + .feature_bits = FEATURE_WRSR_WREN | FEATURE_OTP | FEATURE_QPI, + .tested = TEST_OK_PREW, + .probe = PROBE_SPI_RDID, + .probe_timing = TIMING_ZERO, + .block_erasers = + { + { + .eraseblocks = { {4 * 1024, 4096} }, + .block_erase = SPI_BLOCK_ERASE_20, + }, { + .eraseblocks = { {32 * 1024, 512} }, + .block_erase = SPI_BLOCK_ERASE_52, + }, { + .eraseblocks = { {64 * 1024, 256} }, + .block_erase = SPI_BLOCK_ERASE_D8, + }, { + .eraseblocks = { {16 * 1024 * 1024, 1} }, + .block_erase = SPI_BLOCK_ERASE_60, + }, { + .eraseblocks = { {16 * 1024 * 1024, 1} }, + .block_erase = SPI_BLOCK_ERASE_C7, + } + }, + .printlock = SPI_PRETTYPRINT_STATUS_REGISTER_BP4_SRWD, + .unlock = SPI_DISABLE_BLOCKPROTECT_BP4_SRWD, + .write = SPI_CHIP_WRITE256, + .read = SPI_CHIP_READ, + .voltage = {2700, 3600}, + }, + + { .vendor = "Zetta Device", .name = "ZD25D20", .bustype = BUS_SPI, diff --git a/include/flashchips.h b/include/flashchips.h index b8a58b2..d8e8bf1 100644 --- a/include/flashchips.h +++ b/include/flashchips.h @@ -1061,6 +1061,7 @@ #define XTX_ID 0x0B /* XTX Technology Limited */ #define XTX_XT25F02E 0x4012 #define XTX_XT25F64B 0x4017 +#define XTX_XT25F128B 0x4018 #define ZETTA_ID 0xBA /* Zetta Device */ #define ZETTA_ZD25D20 0x2012 -- To view, visit https://review.coreboot.org/c/flashrom/+/82676?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: I37084bd66bc7a8f93d6533ab0d67aa2528786299 Gerrit-Change-Number: 82676 Gerrit-PatchSet: 2 Gerrit-Owner: Robert Marko <robimarko(a)gmail.com> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Reviewer: Stefan Reinauer <stefan.reinauer(a)coreboot.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>

1 0

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

31 May '24

Attention is currently required from: Aarya, Alexander Goncharov, Nikolai Artemiev, Peter Marheine. Anastasia Klimchuk has posted comments on this change by Peter Marheine. ( https://review.coreboot.org/c/flashrom/+/82393?usp=email ) Change subject: erasure_layout: Fix get_flash_region bug ...................................................................... Patch Set 14: (1 comment) File erasure_layout.c: https://review.coreboot.org/c/flashrom/+/82393/comment/fecea642_135db866?us… : PS13, Line 368: for (unsigned int addr = region_start; addr <= region_end; addr += len) { > I have changed tests, and looked into debugging messages, and now I have a piece of mind that the bu […] I made CB:82723 to illustrate, the issue repro on head Now the question is, how urgent the issue is. The memory is erased correctly, but not in an optimal way. Peter what do you think? (also for this latest patchset, Jenkins is not adding a comment, but the results are here https://qa.coreboot.org/job/flashrom_gerrit/11678/console) -- 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?usp=email Gerrit-MessageType: comment Gerrit-Project: flashrom Gerrit-Branch: main Gerrit-Change-Id: Ic5bf9d0f0e4a94c48d6f6e74e3cb9cccdc7adec9 Gerrit-Change-Number: 82393 Gerrit-PatchSet: 14 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: Fri, 31 May 2024 11:55:32 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org>

1 0

[XS] Change in flashrom[main]: [WIP] Erase blocks are selected smaller than expected
by Anastasia Klimchuk (Code Review) 31 May '24

31 May '24

Anastasia Klimchuk has posted comments on this change by Anastasia Klimchuk. ( https://review.coreboot.org/c/flashrom/+/82723?usp=email ) Change subject: [WIP] Erase blocks are selected smaller than expected ...................................................................... Patch Set 1: (1 comment) Patchset: PS1: Running this on head, erase blocks are more granular than expected, specifically: 1017 [ RUN ] Erase test case #6 1018 Creating layout ... done 1019 Dummyflasher initialising with param=""... done 1020 Erase test case #6 started. 1021 Eraser called with blockaddr=0xf, blocklen=0x1 1022 read_chip called with start=0xf, len=0x1 1023 Eraser called with blockaddr=0xe, blocklen=0x1 1024 read_chip called with start=0xe, len=0x1 1025 Eraser called with blockaddr=0x6, blocklen=0x2 1026 read_chip called with start=0x6, len=0x2 1027 Eraser called with blockaddr=0xc, blocklen=0x2 1028 read_chip called with start=0xc, len=0x2 1029 Eraser called with blockaddr=0x8, blocklen=0x4 1030 read_chip called with start=0x8, len=0x4 1031 Eraser called with blockaddr=0x2, blocklen=0x2 1032 read_chip called with start=0x2, len=0x2 1033 Eraser called with blockaddr=0x4, blocklen=0x2 1034 read_chip called with start=0x4, len=0x2 1035 Eraser called with blockaddr=0x0, blocklen=0x2 1036 read_chip called with start=0x0, len=0x2 1037 Erase test case #6 returned 0. 1038 Erased chip memory state for Erase test case #6 is CORRECT 1039 Eraseblocks order of invocation for Erase test case #6 is WRONG 1040 Eraseblocks number of invocations for Erase test case #6 is WRONG, expected 4 actual 8 1021 Eraser called with blockaddr=0xf, blocklen=0x1 1023 Eraser called with blockaddr=0xe, blocklen=0x1 -> this could be one eraseblock of size 2 1031 Eraser called with blockaddr=0x2, blocklen=0x2 1033 Eraser called with blockaddr=0x4, blocklen=0x2 -> this could be one eraseblock of size 4 -- To view, visit https://review.coreboot.org/c/flashrom/+/82723?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: I47851d2b6106075111babf8c3535e5394cbbe0f9 Gerrit-Change-Number: 82723 Gerrit-PatchSet: 1 Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Comment-Date: Fri, 31 May 2024 11:51:58 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No

1 0

[XS] Change in flashrom[main]: [WIP] Erase blocks are selected smaller than expected
by Anastasia Klimchuk (Code Review) 31 May '24

31 May '24

Anastasia Klimchuk has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/82723?usp=email ) Change subject: [WIP] Erase blocks are selected smaller than expected ...................................................................... [WIP] Erase blocks are selected smaller than expected Change-Id: I47851d2b6106075111babf8c3535e5394cbbe0f9 Signed-off-by: Anastasia Klimchuk <aklm(a)flashrom.org> --- M tests/erase_func_algo.c 1 file changed, 5 insertions(+), 4 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/23/82723/1 diff --git a/tests/erase_func_algo.c b/tests/erase_func_algo.c index b0969c2..83f27b6 100644 --- a/tests/erase_func_algo.c +++ b/tests/erase_func_algo.c @@ -415,11 +415,11 @@ * Test case #6 * * Initial vs written: 7 bytes the same, 1 bytes different, 8 bytes the same. - * One layout region covering the whole chip. + * Layout regions of size 2+4+10b. * Chip with eraseblocks 1, 2, 4, 8, 16. */ .chip = &chip_1_2_4_8_16, - .regions = {{0, MIN_REAL_CHIP_SIZE - 1, "whole chip"}}, + .regions = {{0, 1, "reg1"}, {2, 5, "reg2"}, {6, 15, "reg3"}, {15, MIN_REAL_CHIP_SIZE - 1, "tail"}}, .initial_buf = {0x1, 0x2, 0x3, 0x4, 0x5, 0x6, 0x7, 0x1d, 0xa1, 0xa2, 0xa3, 0xa4, 0xa5, 0xa6, 0xa7, 0xa8}, .erased_buf = {ERASE_VALUE, ERASE_VALUE, ERASE_VALUE, ERASE_VALUE, @@ -428,8 +428,9 @@ ERASE_VALUE, ERASE_VALUE, ERASE_VALUE, ERASE_VALUE}, .written_buf = {0x1, 0x2, 0x3, 0x4, 0x5, 0x6, 0x7, 0xdd, 0xa1, 0xa2, 0xa3, 0xa4, 0xa5, 0xa6, 0xa7, 0xa8}, - .eraseblocks_expected = {{0x0, 0x10, TEST_ERASE_INJECTOR_5}}, - .eraseblocks_expected_ind = 1, + .eraseblocks_expected = {{0x6, 0x8, TEST_ERASE_INJECTOR_4}, {0xe, 0x2, TEST_ERASE_INJECTOR_2}, + {0x2, 0x4, TEST_ERASE_INJECTOR_3}, {0x0, 0x2, TEST_ERASE_INJECTOR_2}}, + .eraseblocks_expected_ind = 4, .erase_test_name = "Erase test case #6", .write_test_name = "Write test case #6", }, { -- To view, visit https://review.coreboot.org/c/flashrom/+/82723?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: I47851d2b6106075111babf8c3535e5394cbbe0f9 Gerrit-Change-Number: 82723 Gerrit-PatchSet: 1 Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org>

1 0

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

31 May '24

Attention is currently required from: Aarya, Alexander Goncharov, Nikolai Artemiev, Peter Marheine. Anastasia Klimchuk has posted comments on this change by Peter Marheine. ( https://review.coreboot.org/c/flashrom/+/82393?usp=email ) Change subject: erasure_layout: Fix get_flash_region bug ...................................................................... Patch Set 14: (2 comments) File erasure_layout.c: https://review.coreboot.org/c/flashrom/+/82393/comment/6598daea_c861aedb?us… : PS13, Line 368: for (unsigned int addr = region_start; addr <= region_end; addr += len) { > I looked at this loop and got a bit suspicious. […] I have changed tests, and looked into debugging messages, and now I have a piece of mind that the bug that I suspected is not present :) Happy ending! However after modifying tests I found something else suspicious: now "Erase protected region test case #3" behaves not as I expect. Specifically, there is a 4byte layout region, outside of protected area. I expect it to be erased by 4byte eraser, instead it is erased by 2b + 2b two erasers. The end state of memory is correct, but the behaviour is not optimal. Maybe that's a double happy ending, if I found a different bug :D I uploaded the patchset 14 to have a history of changes, but now I will try to debug "Erase protected region test case #3". File tests/erase_func_algo.c: https://review.coreboot.org/c/flashrom/+/82393/comment/4df28d74_823d2acc?us… : PS13, Line 766: : #define START_PROTECTED_REGION 8 : #define END_PROTECTED_REGION 15 > This was what I thought in the other comment: have this both in the middle of a chip (5-12, somethin […] Done (protected region spans bytes 6-13) -- 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?usp=email Gerrit-MessageType: comment Gerrit-Project: flashrom Gerrit-Branch: main Gerrit-Change-Id: Ic5bf9d0f0e4a94c48d6f6e74e3cb9cccdc7adec9 Gerrit-Change-Number: 82393 Gerrit-PatchSet: 14 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: Fri, 31 May 2024 10:09:28 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org>

1 0

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

31 May '24

Attention is currently required from: Aarya, Alexander Goncharov, Nikolai Artemiev, Peter Marheine. Anastasia Klimchuk has uploaded a new patch set (#14) to the change originally created by Peter Marheine. ( https://review.coreboot.org/c/flashrom/+/82393?usp=email ) 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, 495 insertions(+), 96 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/93/82393/14 -- 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?usp=email Gerrit-MessageType: newpatchset Gerrit-Project: flashrom Gerrit-Branch: main Gerrit-Change-Id: Ic5bf9d0f0e4a94c48d6f6e74e3cb9cccdc7adec9 Gerrit-Change-Number: 82393 Gerrit-PatchSet: 14 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>

1 0

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

31 May '24

Peter Marheine has submitted this change. ( 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. TEST=on yaviks, `flashrom -V -x` prints equivalent messages about "x region (0xZZZZ..0xZZZZ) is readable" before and after this patch. Change-Id: Ia0ca867aef33b598c2697fc264807fa5e29683ec Signed-off-by: Peter Marheine <pmarheine(a)chromium.org> Reviewed-on: https://review.coreboot.org/c/flashrom/+/82496 Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org> Reviewed-by: Hsuan-ting Chen <roccochen(a)google.com> Reviewed-by: Anastasia Klimchuk <aklm(a)chromium.org> --- M erasure_layout.c M flashrom.c M ichspi.c 3 files changed, 22 insertions(+), 22 deletions(-) Approvals: build bot (Jenkins): Verified Anastasia Klimchuk: Looks good to me, approved Hsuan-ting Chen: Looks good to me, but someone else must approve diff --git a/erasure_layout.c b/erasure_layout.c index e9bfa86..d15594d 100644 --- a/erasure_layout.c +++ b/erasure_layout.c @@ -318,13 +318,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; @@ -333,7 +333,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..dbf06cf 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,17 +1511,17 @@ 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", - __func__, region.name, region.start, region.end - 1, addr, addr + len - 1); + __func__, region.name, region.start, region.end, addr, addr + len - 1); free(region.name); continue; } msg_gdbg("%s: %s region (%#08"PRIx32"..%#08"PRIx32") is writable, erasing range (%#08x..%#08x).\n", - __func__, region.name, region.start, region.end - 1, addr, addr + len - 1); + __func__, region.name, region.start, region.end, addr, addr + len - 1); free(region.name); if (erasefn(flashctx, addr, len)) diff --git a/ichspi.c b/ichspi.c index bef43b9..cc56e0c 100644 --- a/ichspi.c +++ b/ichspi.c @@ -1436,7 +1436,7 @@ region->read_prot = false; region->write_prot = false; region->start = 0; - region->end = flashrom_flash_getsize(flash); + region->end = flashrom_flash_getsize(flash) - 1; for (ssize_t i = 0; i < nr; i++) { uint32_t base = fd_regions[i].base; @@ -1459,7 +1459,7 @@ /* 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?usp=email Gerrit-MessageType: merged Gerrit-Project: flashrom Gerrit-Branch: main Gerrit-Change-Id: Ia0ca867aef33b598c2697fc264807fa5e29683ec Gerrit-Change-Number: 82496 Gerrit-PatchSet: 8 Gerrit-Owner: Peter Marheine <pmarheine(a)chromium.org> Gerrit-Reviewer: Aarya <aarya.chaumal(a)gmail.com> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Hsuan-ting Chen <roccochen(a)google.com> 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>

1 0

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

31 May '24

Attention is currently required from: Paul Menzel, ZhiYuanNJ. Anastasia Klimchuk has posted comments on this change by ZhiYuanNJ. ( https://review.coreboot.org/c/flashrom/+/82193?usp=email ) Change subject: ch347_spi: Add driver support for CH347F packaging ...................................................................... Patch Set 13: (1 comment) Patchset: PS13: > My SUBMIT button is in an unreachable state, prompting:You don't have permission to submit change 82 […] Yes, this is normal, there is a limited group of core maintainers who can submit patches. You can read more on our guidelines: https://flashrom.org/dev_guide/development_guide.html#merging-patches In this case, you are all good, your patch is ready. It will be submitted in few days. Thank you for your work! -- To view, visit https://review.coreboot.org/c/flashrom/+/82193?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email Gerrit-MessageType: comment Gerrit-Project: flashrom Gerrit-Branch: main Gerrit-Change-Id: I693baf1a0d9dc20757f56fba626b5f5ad20f71dd Gerrit-Change-Number: 82193 Gerrit-PatchSet: 13 Gerrit-Owner: ZhiYuanNJ Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Nicholas Chin <nic.c3.14(a)gmail.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: ZhiYuanNJ Gerrit-Comment-Date: Fri, 31 May 2024 02:44:08 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: ZhiYuanNJ

1 0

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

31 May '24

Attention is currently required from: Paul Menzel. ZhiYuanNJ has posted comments on this change by ZhiYuanNJ. ( https://review.coreboot.org/c/flashrom/+/82193?usp=email ) Change subject: ch347_spi: Add driver support for CH347F packaging ...................................................................... Patch Set 13: (1 comment) Patchset: PS13: My SUBMIT button is in an unreachable state, prompting:You don't have permission to submit change 82193 Is this a normal phenomenon? -- To view, visit https://review.coreboot.org/c/flashrom/+/82193?usp=email To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings?usp=email Gerrit-MessageType: comment Gerrit-Project: flashrom Gerrit-Branch: main Gerrit-Change-Id: I693baf1a0d9dc20757f56fba626b5f5ad20f71dd Gerrit-Change-Number: 82193 Gerrit-PatchSet: 13 Gerrit-Owner: ZhiYuanNJ Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Nicholas Chin <nic.c3.14(a)gmail.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Comment-Date: Fri, 31 May 2024 02:29:42 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No

1 0

[XS] Change in flashrom[main]: Add clarification to struct flash_region on chipoff_t
by Peter Marheine (Code Review) 31 May '24

31 May '24

Peter Marheine has submitted this change. ( https://review.coreboot.org/c/flashrom/+/82677?usp=email ) ( 1 is the latest approved patch-set. No files were changed between the latest approved patch-set and the submitted one. )Change subject: Add clarification to struct flash_region on chipoff_t ...................................................................... Add clarification to struct flash_region on chipoff_t Although chipoff_t is fairly clearly documented on its own, it seems fairly frequent that developers will treat the end address of a flash region as an exclusive upper bound rather than the inclusive one it should be; for example CB:82496 fixes an incorrect use that affected multiple sites, and CB:73571 stemmed from a similar cause. Add a clarifying comment to call attention to this, to help programmers avoid making similar mistakes in the future. Change-Id: I80b61a87ca31bd5a116224aadb4e211ee6841e1f Signed-off-by: Peter Marheine <pmarheine(a)chromium.org> Reviewed-on: https://review.coreboot.org/c/flashrom/+/82677 Reviewed-by: Hsuan-ting Chen <roccochen(a)google.com> Reviewed-by: Anastasia Klimchuk <aklm(a)chromium.org> Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org> --- M include/layout.h 1 file changed, 5 insertions(+), 0 deletions(-) Approvals: Anastasia Klimchuk: Looks good to me, approved build bot (Jenkins): Verified Hsuan-ting Chen: Looks good to me, but someone else must approve diff --git a/include/layout.h b/include/layout.h index 5614bea..d03a15b 100644 --- a/include/layout.h +++ b/include/layout.h @@ -37,6 +37,11 @@ struct flash_region { char *name; + /* + * Note that because start and end are chipoff_t, end is an inclusive upper + * bound: the length of a region is (end - start + 1) bytes and it is + * impossible to represent a region with zero length. + */ chipoff_t start; chipoff_t end; bool read_prot; -- To view, visit https://review.coreboot.org/c/flashrom/+/82677?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: I80b61a87ca31bd5a116224aadb4e211ee6841e1f Gerrit-Change-Number: 82677 Gerrit-PatchSet: 3 Gerrit-Owner: Peter Marheine <pmarheine(a)chromium.org> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Hsuan-ting Chen <roccochen(a)google.com> Gerrit-Reviewer: Peter Marheine <pmarheine(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>

1 0

2025

2024

2023

2022

2021

2020

2019

2018

2017

flashrom-gerrit