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) 27 May '24

27 May '24

Attention is currently required from: Aarya, Anastasia Klimchuk, Nikolai Artemiev. Hello Aarya, Anastasia Klimchuk, Nikolai Artemiev, build bot (Jenkins), I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/flashrom/+/82496?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: 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 Signed-off-by: Peter Marheine <pmarheine(a)chromium.org> --- M erasure_layout.c M flashrom.c M ichspi.c 3 files changed, 22 insertions(+), 22 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/96/82496/6 -- 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: newpatchset Gerrit-Project: flashrom Gerrit-Branch: main Gerrit-Change-Id: Ia0ca867aef33b598c2697fc264807fa5e29683ec Gerrit-Change-Number: 82496 Gerrit-PatchSet: 6 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: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Hsuan-ting Chen <roccochen(a)google.com> Gerrit-Attention: Aarya <aarya.chaumal(a)gmail.com> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Attention: Nikolai Artemiev <nartemiev(a)google.com>

1 0

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

27 May '24

Attention is currently required from: Aarya, Alexander Goncharov, Anastasia Klimchuk, Nikolai Artemiev. Peter Marheine 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 12: (8 comments) File tests/erase_func_algo.c: https://review.coreboot.org/c/flashrom/+/82393/comment/1792f48b_a2cb24cb?us… : PS9, Line 766: 16 > To be consistent with your previous patch, and also I think tests are partially a documentation (a s […] Done https://review.coreboot.org/c/flashrom/+/82393/comment/11d54903_58ed26e6?us… : PS9, Line 953: const size_t num_unparameterized = 1; > Do you think this is needed? It's the same name and the same function. […] I don't understand what you're suggesting as an alternative. All of the tests need to be allocated in one block, and since this value is used in two locations (the size for memcpy below as well as generating the total number of tests) it seemed less error-prone to put the 1 in as a constant: this way you're less likely to forget to increase the allocation size if you add another unparameterized case. https://review.coreboot.org/c/flashrom/+/82393/comment/98801773_4b44b3a6?us… : PS9, Line 973: .name = "erase failure for unskipped unwritable regions", > I had the trouble with test names, because I wanted an index (a test case #) in the name. […] The key with this test is that it expects `flashrom_flash_erase` to fail, whereas the other tests (with `SKIP_UNWRITABLE_REGIONS` enabled) require it to succeed. https://review.coreboot.org/c/flashrom/+/82393/comment/381cf412_d9796d9e?us… : PS9, Line 1103: 16 > This is `END_PROTECTED_REGION + 1`, just for perfection […] Done https://review.coreboot.org/c/flashrom/+/82393/comment/27425dc9_70d4251d?us… : PS9, Line 1138: < > <= […] Done https://review.coreboot.org/c/flashrom/+/82393/comment/cd5f27f4_e0442904?us… : PS9, Line 1140: < > also here <= I realized there was another issue with this comparison: it didn't handle the case where the block to be erased completely contains the protected region but neither endpoint is inside it. That doesn't affect the tests right now, but I added a check for that as well. File tests/tests.h: https://review.coreboot.org/c/flashrom/+/82393/comment/40e2456a_0f17e15c?us… : PS9, Line 113: void erase_unwritable_regions_skipflag_off_test_success(void **state); > this also can be removed from this patch Done https://review.coreboot.org/c/flashrom/+/82393/comment/ff54bfe2_4fd50433?us… : PS9, Line 114: erase_unwritable_regions_skipflag_on_test_success > I am wondering maybe this is also not needed. Because the function which is called in tests. […] 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?usp=email Gerrit-MessageType: comment Gerrit-Project: flashrom Gerrit-Branch: main Gerrit-Change-Id: Ic5bf9d0f0e4a94c48d6f6e74e3cb9cccdc7adec9 Gerrit-Change-Number: 82393 Gerrit-PatchSet: 12 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: Mon, 27 May 2024 01:15:55 +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 Peter Marheine (Code Review) 27 May '24

27 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 (#12). 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, 490 insertions(+), 96 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/93/82393/12 -- 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: 12 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

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

27 May '24

Attention is currently required from: Anastasia Klimchuk, Nikolai Artemiev, Thomas Heijligen. Peter Marheine has posted comments on this change by Peter Marheine. ( https://review.coreboot.org/c/flashrom/+/82251?usp=email ) Change subject: tests/erase: record the opcode for each erase ...................................................................... Patch Set 8: (4 comments) File include/flash.h: https://review.coreboot.org/c/flashrom/+/82251/comment/883d5d1c_3edc3fd7?us… : PS6, Line 334: /* special cases must come last. */ > I think this comment better go under FLASHROM_TEST, otherwise it is orphaned in the non-test code. […] Done https://review.coreboot.org/c/flashrom/+/82251/comment/d4fad8a0_ef0e589d?us… : PS6, Line 335: #ifndef FLASHROM_TEST : }; : #else : TEST_ERASE_INJECTOR_1, : TEST_ERASE_INJECTOR_2, : TEST_ERASE_INJECTOR_3, : TEST_ERASE_INJECTOR_4, : TEST_ERASE_INJECTOR_5, : }; : : #define NUM_TEST_ERASE_INJECTORS 5 : extern erasefunc_t *g_test_erase_injector[NUM_TEST_ERASE_INJECTORS]; : #endif > This style is different from the others (read and write), ifndef vs ifdef and I know it saves few li […] Yeah, that seems worthwhile. Doubling up the closing braces could probably confuse editors easily. Done. File tests/erase_func_algo.c: https://review.coreboot.org/c/flashrom/+/82251/comment/482e6239_0f0213e5?us… : PS6, Line 205: block_erase_chip_1, : block_erase_chip_2, : block_erase_chip_3, : block_erase_chip_4, : block_erase_chip_5, > I think it can help readers, to add per-line comment, since the suffixes (1, 2, 3, 4, 5) do not corr […] Done https://review.coreboot.org/c/flashrom/+/82251/comment/2f9ea4bf_3248e877?us… : PS6, Line 649: {0xf, 0x1, TEST_ERASE_INJECTOR_1}, : {0x8, 0x1, TEST_ERASE_INJECTOR_1}, : {0x9, 0x1, TEST_ERASE_INJECTOR_1}, : {0xa, 0x1, TEST_ERASE_INJECTOR_1}, : {0xb, 0x1, TEST_ERASE_INJECTOR_1}, : {0xc, 0x1, TEST_ERASE_INJECTOR_1}, : {0xd, 0x1, TEST_ERASE_INJECTOR_1}, : {0xe, 0x1, TEST_ERASE_INJECTOR_1}, : {0x3, 0x1, TEST_ERASE_INJECTOR_1}, : {0x4, 0x1, TEST_ERASE_INJECTOR_1}, : {0x5, 0x1, TEST_ERASE_INJECTOR_1}, : {0x6, 0x1, TEST_ERASE_INJECTOR_1}, : {0x7, 0x1, TEST_ERASE_INJECTOR_1}, : {0x0, 0x1, TEST_ERASE_INJECTOR_1}, : {0x1, 0x1, TEST_ERASE_INJECTOR_1}, > These lines have trailing whitespace (shows as bright pink in gerrit :)) Done -- 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?usp=email Gerrit-MessageType: comment Gerrit-Project: flashrom Gerrit-Branch: main Gerrit-Change-Id: I3983fe42c2e7f06668a1bd20d2db7fafa93b8043 Gerrit-Change-Number: 82251 Gerrit-PatchSet: 8 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: Mon, 27 May 2024 00:28:07 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org>

1 0

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

27 May '24

Attention is currently required from: Aarya, Anastasia Klimchuk, Nikolai Artemiev. Hello Aarya, Anastasia Klimchuk, Nikolai Artemiev, build bot (Jenkins), I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/flashrom/+/82496?usp=email to look at the new patch set (#5). The following approvals got outdated and were removed: Verified-1 by build bot (Jenkins) Change subject: 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 Signed-off-by: Peter Marheine <pmarheine(a)chromium.org> --- M erasure_layout.c M flashrom.c M ichspi.c 3 files changed, 21 insertions(+), 21 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/96/82496/5 -- 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: newpatchset Gerrit-Project: flashrom Gerrit-Branch: main Gerrit-Change-Id: Ia0ca867aef33b598c2697fc264807fa5e29683ec Gerrit-Change-Number: 82496 Gerrit-PatchSet: 5 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: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Hsuan-ting Chen <roccochen(a)google.com> Gerrit-Attention: Aarya <aarya.chaumal(a)gmail.com> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Attention: Nikolai Artemiev <nartemiev(a)google.com>

1 0

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

27 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 (#11). 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, 489 insertions(+), 96 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/93/82393/11 -- 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: 11 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

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

27 May '24

Attention is currently required from: Aarya, Anastasia Klimchuk, Nikolai Artemiev. Peter Marheine has posted comments on this change by Peter Marheine. ( https://review.coreboot.org/c/flashrom/+/82496?usp=email ) Change subject: Correct get_flash_region() to use inclusive upper bounds ...................................................................... Patch Set 4: (3 comments) File flashrom.c: https://review.coreboot.org/c/flashrom/+/82496/comment/65a8f8c6_37450783?us… : PS2, Line 1518: region.end - 1 > `region. […] Done https://review.coreboot.org/c/flashrom/+/82496/comment/184f6c4c_0bc70ec3?us… : PS2, Line 1524: region.end - 1 > also `region. […] Done File ichspi.c: https://review.coreboot.org/c/flashrom/+/82496/comment/7694afec_9848bf71?us… : PS2, Line 1454: limit > Why you are changing this? I understand why region end changes, but why region start? […] Good catch; it should still be +1 here. The regions in this function also use inclusive bounds so converting from the top to bottom needs an adjustment, but I probably failed to think that through completely. -- 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: comment Gerrit-Project: flashrom Gerrit-Branch: main Gerrit-Change-Id: Ia0ca867aef33b598c2697fc264807fa5e29683ec Gerrit-Change-Number: 82496 Gerrit-PatchSet: 4 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: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Hsuan-ting Chen <roccochen(a)google.com> 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-Comment-Date: Mon, 27 May 2024 00:17:00 +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]: Correct get_flash_region() to use inclusive upper bounds
by Peter Marheine (Code Review) 27 May '24

27 May '24

Attention is currently required from: Aarya, Nikolai Artemiev, Peter Marheine. Hello Aarya, Anastasia Klimchuk, Nikolai Artemiev, build bot (Jenkins), I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/flashrom/+/82496?usp=email to look at the new patch set (#4). The following approvals got outdated and were removed: Verified-1 by build bot (Jenkins) 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 M tests/erase_func_algo.c 4 files changed, 25 insertions(+), 25 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/96/82496/4 -- 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: newpatchset Gerrit-Project: flashrom Gerrit-Branch: main Gerrit-Change-Id: Ia0ca867aef33b598c2697fc264807fa5e29683ec Gerrit-Change-Number: 82496 Gerrit-PatchSet: 4 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: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Hsuan-ting Chen <roccochen(a)google.com> Gerrit-Attention: Aarya <aarya.chaumal(a)gmail.com> Gerrit-Attention: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Attention: Peter Marheine <pmarheine(a)chromium.org>

1 0

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

26 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 (#7). 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, 157 insertions(+), 58 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/51/82251/7 -- 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?usp=email Gerrit-MessageType: newpatchset Gerrit-Project: flashrom Gerrit-Branch: main Gerrit-Change-Id: I3983fe42c2e7f06668a1bd20d2db7fafa93b8043 Gerrit-Change-Number: 82251 Gerrit-PatchSet: 7 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>

1 0

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

26 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 (#10). 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/10 -- 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: 10 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

2024

2023

2022

2021

2020

2019

2018

2017

flashrom-gerrit May 2024