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

Change in flashrom[master]: libflashrom.map: Add missing symbols
by Edward O'Callaghan (Code Review) 07 Apr '25

07 Apr '25

Edward O'Callaghan has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/38993 ) Change subject: libflashrom.map: Add missing symbols ...................................................................... libflashrom.map: Add missing symbols The following symbols were missing at link-time from libflashrom: supported_flash_chips; supported_boards; supported_chipsets; Change-Id: I299928d62788c0abfe7f6cdd3e8ad8be0c49e45e Signed-off-by: Edward O'Callaghan <quasisec(a)google.com> --- M libflashrom.map 1 file changed, 3 insertions(+), 0 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/93/38993/1 diff --git a/libflashrom.map b/libflashrom.map index d6dd24d..9430775 100644 --- a/libflashrom.map +++ b/libflashrom.map @@ -25,6 +25,9 @@ flashrom_set_log_callback; flashrom_shutdown; flashrom_supported_programmers; + flashrom_supported_flash_chips; + flashrom_supported_boards; + flashrom_supported_chipsets; flashrom_system_info; flashrom_version_info; local: *; -- To view, visit https://review.coreboot.org/c/flashrom/+/38993 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I299928d62788c0abfe7f6cdd3e8ad8be0c49e45e Gerrit-Change-Number: 38993 Gerrit-PatchSet: 1 Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-MessageType: newchange

4 3

Change in flashrom[master]: tests/: Add programmer enumerate correspondence test
by Edward O'Callaghan (Code Review) 21 Sep '24

21 Sep '24

Edward O'Callaghan has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/47356 ) Change subject: tests/: Add programmer enumerate correspondence test ...................................................................... tests/: Add programmer enumerate correspondence test The enumerate defines within programmer.h must be aligned with the programmer dispatch table within flashrom.c. The following unit-tests tries to improve coverage here to avoid issues like we saw when ENE and MEC1308 were upstreamed. BUG=none BRANCH=none TEST=meson test Change-Id: I13efc4561f743edf51326d4b408be8715b630377 Signed-off-by: Edward O'Callaghan <quasisec(a)google.com> --- M tests/flashrom.c M tests/tests.c M tests/tests.h 3 files changed, 94 insertions(+), 0 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/56/47356/1 diff --git a/tests/flashrom.c b/tests/flashrom.c index 50464dd..9180922 100644 --- a/tests/flashrom.c +++ b/tests/flashrom.c @@ -15,6 +15,47 @@ #include <include/test.h> +/* must be before programmer.h include */ +#define CONFIG_INTERNAL 1 +#define CONFIG_DUMMY 1 +#define CONFIG_MEC1308 1 +#define CONFIG_NIC3COM 1 +#define CONFIG_NICREALTEK 1 +#define CONFIG_NICNATSEMI 1 +#define CONFIG_GFXNVIDIA 1 +#define CONFIG_RAIDEN 1 +#define CONFIG_DRKAISER 1 +#define CONFIG_SATASII 1 +#define CONFIG_ATAHPT 1 +#define CONFIG_ATAVIA 1 +#define CONFIG_ATAPROMISE 1 +#define CONFIG_IT8212 1 +#define CONFIG_FT2232_SPI 1 +#define CONFIG_SERPROG 1 +#define CONFIG_BUSPIRATE_SPI 1 +#define CONFIG_DEDIPROG 1 +#define CONFIG_DEVELOPERBOX_SPI 1 +#define CONFIG_ENE_LPC 1 +#define CONFIG_RAYER_SPI 1 +#define CONFIG_PONY_SPI 1 +#define CONFIG_NICINTEL 1 +#define CONFIG_NICINTEL_SPI 1 +#define CONFIG_NICINTEL_EEPROM 1 +#define CONFIG_OGP_SPI 1 +#define CONFIG_SATAMV 1 +#define CONFIG_LINUX_MTD 1 +#define CONFIG_LINUX_SPI 1 +#define CONFIG_LSPCON_I2C_SPI 1 +#define CONFIG_REALTEK_MST_I2C_SPI 1 +#define CONFIG_USBBLASTER_SPI 1 +#define CONFIG_MSTARDDC_SPI 1 +#define CONFIG_PICKIT2_SPI 1 +#define CONFIG_CH341A_SPI 1 +#define CONFIG_DIGILENT_SPI 1 +#define CONFIG_JLINK_SPI 1 +#define CONFIG_NI845X_SPI 1 +#define CONFIG_STLINKV3_SPI 1 + #include "programmer.h" void flashbuses_to_text_test_success(void **state) @@ -49,3 +90,54 @@ bustype = BUS_NONE; assert_string_equal(flashbuses_to_text(bustype), "None"); } + +void validate_prog_enumerates_test_success(void **state) +{ + (void) state; /* unused */ + + enum programmer prog = PROGRAMMER_INTERNAL; + const char *prog_names[] = { + "internal", + "dummy", + "mec1308", + "nic3com", + "nicrealtek", + "nicnatsemi", + "gfxnvidia", + "raiden_debug", + "drkaiser", + "satasii", + "atahpt", + "atavia", + "atapromise", + "it8212", + "ft2232_spi", + "serprog", + "buspirate_spi", + "dediprog", + "developerbox", + "ene_lpc", + "rayer_spi", + "pony_spi", + "nicintel", + "nicintel_spi", + "nicintel_eeprom", + "ogp_spi", + "satamv", + "linux_mtd", + "linux_spi", + "lspcon_i2c_spi", + "realtek_mst_i2c_spi", + "usbblaster_spi", + "mstarddc_spi", + "pickit2_spi", + "ch341a_spi", + "digilent_spi", + "jlink_spi", + "ni845x_spi", + "stlinkv3_spi", + }; + for (; prog < PROGRAMMER_INVALID; prog++) { + assert_string_equal(programmer_table[prog].name, prog_names[prog]); + } +} diff --git a/tests/tests.c b/tests/tests.c index 3124607..d2a22a5 100644 --- a/tests/tests.c +++ b/tests/tests.c @@ -46,6 +46,7 @@ const struct CMUnitTest flashrom_tests[] = { cmocka_unit_test(flashbuses_to_text_test_success), + cmocka_unit_test(validate_prog_enumerates_test_success), }; ret |= cmocka_run_group_tests_name("flashrom.c tests", flashrom_tests, NULL, NULL); diff --git a/tests/tests.h b/tests/tests.h index cb905fd..89fc7fa 100644 --- a/tests/tests.h +++ b/tests/tests.h @@ -27,6 +27,7 @@ /* flashrom.c */ void flashbuses_to_text_test_success(void **state); +void validate_prog_enumerates_test_success(void **state); /* spi25.c */ void spi_write_enable_test_success(void **state); -- To view, visit https://review.coreboot.org/c/flashrom/+/47356 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I13efc4561f743edf51326d4b408be8715b630377 Gerrit-Change-Number: 47356 Gerrit-PatchSet: 1 Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-MessageType: newchange

6 8

Change in flashrom[master]: Makefile: Check for last line only from preprocessed output
by Khem Raj (Code Review) 01 Aug '24

01 Aug '24

Khem Raj has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/43770 ) Change subject: Makefile: Check for last line only from preprocessed output ...................................................................... Makefile: Check for last line only from preprocessed output This started to fail with glibc 2.32 since glibc added additional attributes to functions in signal.h therefore existing regexp started to fail as it is not able to handle these functions e.g. extern int siginterrupt (int __sig, int __interrupt) __attribute__ ((__nothrow__ , __leaf__)) __attribute__ ((__deprecated__ ("Use sigaction with SA_RESTART instead"))); grep -v '^\#' | grep '"' | cut -f 2 -d'"' bit outside of fd_set selected Use sigaction with SA_RESTART instead arm So changing it to tail -1 | grep '"' | cut -f 2 -d'"' arm Produces the expected result, this was hidden until now Signed-off-by: Khem Raj <raj.khem(a)gmail.com> Change-Id: I123a046e142d54632f12d54e2aa09b0928c02b91 --- M Makefile 1 file changed, 3 insertions(+), 3 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/70/43770/1 diff --git a/Makefile b/Makefile index 803529f..3795681 100644 --- a/Makefile +++ b/Makefile @@ -106,7 +106,7 @@ # IMPORTANT: The following line must be placed before TARGET_OS is ever used # (of course), but should come after any lines setting CC because the line # below uses CC itself. -override TARGET_OS := $(strip $(call debug_shell,$(CC) $(CPPFLAGS) -E os.h 2>/dev/null | grep -v '^\#' | grep '"' | cut -f 2 -d'"')) +override TARGET_OS := $(strip $(call debug_shell,$(CC) $(CPPFLAGS) -E os.h 2>/dev/null | tail -1 | grep '"' | cut -f 2 -d'"')) ifeq ($(TARGET_OS), Darwin) override CPPFLAGS += -I/opt/local/include -I/usr/local/include @@ -460,8 +460,8 @@ # IMPORTANT: The following line must be placed before ARCH is ever used # (of course), but should come after any lines setting CC because the line # below uses CC itself. -override ARCH := $(strip $(call debug_shell,$(CC) $(CPPFLAGS) -E archtest.c 2>/dev/null | grep -v '^\#' | grep '"' | cut -f 2 -d'"')) -override ENDIAN := $(strip $(call debug_shell,$(CC) $(CPPFLAGS) -E endiantest.c 2>/dev/null | grep -v '^\#')) +override ARCH := $(strip $(call debug_shell,$(CC) $(CPPFLAGS) -E archtest.c 2>/dev/null | tail -1 | grep '"' | cut -f 2 -d'"')) +override ENDIAN := $(strip $(call debug_shell,$(CC) $(CPPFLAGS) -E endiantest.c 2>/dev/null | tail -1)) # Disable the internal programmer on unsupported architectures (everything but x86 and mipsel) ifneq ($(ARCH)-little, $(filter $(ARCH),x86 mips)-$(ENDIAN)) -- To view, visit https://review.coreboot.org/c/flashrom/+/43770 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I123a046e142d54632f12d54e2aa09b0928c02b91 Gerrit-Change-Number: 43770 Gerrit-PatchSet: 1 Gerrit-Owner: Khem Raj Gerrit-MessageType: newchange

4 5

Change in flashrom[master]: Add -- to make clean
by Ryan O'Leary (Code Review) 01 Aug '24

01 Aug '24

Ryan O'Leary has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/37776 ) Change subject: Add -- to make clean ...................................................................... Add -- to make clean Somehow, I ended up with a file name "-c.d". I probably made it by mistake. This change lets "make clean" be a bit more resilient to files such as these. Change-Id: I2517ffac975f3df75f706350a07f189a98a11b7c Signed-off-by: Ryan O'Leary <ryanoleary(a)google.com> --- M Makefile 1 file changed, 1 insertion(+), 1 deletion(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/76/37776/1 diff --git a/Makefile b/Makefile index 518d41b..fa66910 100644 --- a/Makefile +++ b/Makefile @@ -1129,7 +1129,7 @@ # This includes all frontends and libflashrom. # We don't use EXEC_SUFFIX here because we want to clean everything. clean: - rm -f $(PROGRAM) $(PROGRAM).exe libflashrom.a *.o *.d $(PROGRAM).8 $(PROGRAM).8.html $(BUILD_DETAILS_FILE) + rm -f -- $(PROGRAM) $(PROGRAM).exe libflashrom.a *.o *.d $(PROGRAM).8 $(PROGRAM).8.html $(BUILD_DETAILS_FILE) @+$(MAKE) -C util/ich_descriptors_tool/ clean distclean: clean -- To view, visit https://review.coreboot.org/c/flashrom/+/37776 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I2517ffac975f3df75f706350a07f189a98a11b7c Gerrit-Change-Number: 37776 Gerrit-PatchSet: 1 Gerrit-Owner: Ryan O'Leary <ryanoleary(a)google.com> Gerrit-MessageType: newchange

6 8

[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

2025

2024

2023

2022

2021

2020

2019

2018

2017

flashrom-gerrit May 2024