- flashrom-gerrit - mail.coreboot.org

[M] Change in flashrom[master]: block erase WIP
by Edward O'Callaghan (Code Review) 24 Nov '22

24 Nov '22

Edward O'Callaghan has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/69959 ) Change subject: block erase WIP ...................................................................... block erase WIP The idea here is to bring about modularity in the erasure stack by moving lookups into its own module. Change-Id: I9b1dc33fb8d3e98f26f7d4cd1fbb876c3c5580a2 Signed-off-by: Edward O'Callaghan <quasisec(a)google.com> --- M flashrom.c M include/chipdrivers.h M spi25.c 3 files changed, 81 insertions(+), 81 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/59/69959/1 diff --git a/flashrom.c b/flashrom.c index 2489ea3..9057b31 100644 --- a/flashrom.c +++ b/flashrom.c @@ -338,57 +338,6 @@ return extract_param(&cfg->params, param_name, ","); } -/* special unit-test hook */ -erasefunc_t *g_test_erase_injector; - -static erasefunc_t *lookup_erase_func_ptr(const struct block_eraser *const eraser) -{ - switch (eraser->block_erase) { - case SPI_BLOCK_ERASE_EMULATION: return &spi_block_erase_emulation; - case SPI_BLOCK_ERASE_20: return &spi_block_erase_20; - case SPI_BLOCK_ERASE_21: return &spi_block_erase_21; - case SPI_BLOCK_ERASE_40: return NULL; // FIXME unhandled &spi_block_erase_40; - case SPI_BLOCK_ERASE_50: return &spi_block_erase_50; - case SPI_BLOCK_ERASE_52: return &spi_block_erase_52; - case SPI_BLOCK_ERASE_53: return &spi_block_erase_53; - case SPI_BLOCK_ERASE_5C: return &spi_block_erase_5c; - case SPI_BLOCK_ERASE_60: return &spi_block_erase_60; - case SPI_BLOCK_ERASE_62: return &spi_block_erase_62; - case SPI_BLOCK_ERASE_81: return &spi_block_erase_81; - case SPI_BLOCK_ERASE_C4: return &spi_block_erase_c4; - case SPI_BLOCK_ERASE_C7: return &spi_block_erase_c7; - case SPI_BLOCK_ERASE_D7: return &spi_block_erase_d7; - case SPI_BLOCK_ERASE_D8: return &spi_block_erase_d8; - case SPI_BLOCK_ERASE_DB: return &spi_block_erase_db; - case SPI_BLOCK_ERASE_DC: return &spi_block_erase_dc; - case S25FL_BLOCK_ERASE: return &s25fl_block_erase; - case S25FS_BLOCK_ERASE_D8: return &s25fs_block_erase_d8; - case JEDEC_SECTOR_ERASE: return &erase_sector_jedec; // TODO rename to &jedec_sector_erase; - case JEDEC_BLOCK_ERASE: return &erase_block_jedec; // TODO rename to &jedec_block_erase; - case JEDEC_CHIP_BLOCK_ERASE: return &erase_chip_block_jedec; // TODO rename to &jedec_chip_block_erase; - case OPAQUE_ERASE: return &erase_opaque; // TODO rename to &opqaue_erase; - case SPI_ERASE_AT45CS_SECTOR: return &spi_erase_at45cs_sector; - case SPI_ERASE_AT45DB_BLOCK: return &spi_erase_at45db_block; - case SPI_ERASE_AT45DB_CHIP: return &spi_erase_at45db_chip; - case SPI_ERASE_AT45DB_PAGE: return &spi_erase_at45db_page; - case SPI_ERASE_AT45DB_SECTOR: return &spi_erase_at45db_sector; - case ERASE_CHIP_28SF040: return &erase_chip_28sf040; - case ERASE_SECTOR_28SF040: return &erase_sector_28sf040; - case ERASE_BLOCK_82802AB: return &erase_block_82802ab; - case ERASE_SECTOR_49LFXXXC: return &erase_sector_49lfxxxc; - case STM50_SECTOR_ERASE: return &erase_sector_stm50; // TODO rename to &stm50_sector_erase; - case EDI_CHIP_BLOCK_ERASE: return &edi_chip_block_erase; - case TEST_ERASE_INJECTOR: return g_test_erase_injector; - /* default: total function, 0 indicates no erase function set. - * We explicitly do not want a default catch-all case in the switch - * to ensure unhandled enum's are compiler warnings. - */ - case NO_BLOCK_ERASE_FUNC: return NULL; - }; - - return NULL; -} - static int check_block_eraser(const struct flashctx *flash, int k, int log) { struct block_eraser eraser = flash->chip->block_erasers[k]; diff --git a/include/chipdrivers.h b/include/chipdrivers.h index 9c9a94c..c5a0acb 100644 --- a/include/chipdrivers.h +++ b/include/chipdrivers.h @@ -22,6 +22,8 @@ #include "flash.h" /* for chipaddr and flashctx */ +erasefunc_t *lookup_erase_func_ptr(const struct block_eraser *const eraser); + /* spi.c */ int spi_aai_write(struct flashctx *flash, const uint8_t *buf, unsigned int start, unsigned int len); int spi_chip_write_256(struct flashctx *flash, const uint8_t *buf, unsigned int start, unsigned int len); @@ -37,21 +39,6 @@ int probe_spi_at25f(struct flashctx *flash); int spi_write_enable(struct flashctx *flash); int spi_write_disable(struct flashctx *flash); -int spi_block_erase_20(struct flashctx *flash, unsigned int addr, unsigned int blocklen); -int spi_block_erase_21(struct flashctx *flash, unsigned int addr, unsigned int blocklen); -int spi_block_erase_50(struct flashctx *flash, unsigned int addr, unsigned int blocklen); -int spi_block_erase_52(struct flashctx *flash, unsigned int addr, unsigned int blocklen); -int spi_block_erase_53(struct flashctx *flash, unsigned int addr, unsigned int blocklen); -int spi_block_erase_5c(struct flashctx *flash, unsigned int addr, unsigned int blocklen); -int spi_block_erase_60(struct flashctx *flash, unsigned int addr, unsigned int blocklen); -int spi_block_erase_62(struct flashctx *flash, unsigned int addr, unsigned int blocklen); -int spi_block_erase_81(struct flashctx *flash, unsigned int addr, unsigned int blocklen); -int spi_block_erase_c4(struct flashctx *flash, unsigned int addr, unsigned int blocklen); -int spi_block_erase_c7(struct flashctx *flash, unsigned int addr, unsigned int blocklen); -int spi_block_erase_d7(struct flashctx *flash, unsigned int addr, unsigned int blocklen); -int spi_block_erase_d8(struct flashctx *flash, unsigned int addr, unsigned int blocklen); -int spi_block_erase_db(struct flashctx *flash, unsigned int addr, unsigned int blocklen); -int spi_block_erase_dc(struct flashctx *flash, unsigned int addr, unsigned int blocklen); enum block_erase_func spi_get_erasefn_from_opcode(uint8_t opcode); uint8_t spi_get_opcode_from_erasefn(enum block_erase_func func); int spi_chip_write_1(struct flashctx *flash, const uint8_t *buf, unsigned int start, unsigned int len); diff --git a/spi25.c b/spi25.c index da870be..79ac01f 100644 --- a/spi25.c +++ b/spi25.c @@ -492,7 +492,7 @@ return spi_simple_write_cmd(flash, JEDEC_CE_C7, 1000 * 1000); } -int spi_block_erase_52(struct flashctx *flash, unsigned int addr, +static int spi_block_erase_52(struct flashctx *flash, unsigned int addr, unsigned int blocklen) { /* This usually takes 100-4000ms, so wait in 100ms steps. */ @@ -502,7 +502,7 @@ /* Block size is usually * 32M (one die) for Micron */ -int spi_block_erase_c4(struct flashctx *flash, unsigned int addr, unsigned int blocklen) +static int spi_block_erase_c4(struct flashctx *flash, unsigned int addr, unsigned int blocklen) { /* This usually takes 240-480s, so wait in 500ms steps. */ return spi_write_cmd(flash, JEDEC_BE_C4, false, addr, NULL, 0, 500 * 1000); @@ -513,7 +513,7 @@ * 32k for SST * 4-32k non-uniform for EON */ -int spi_block_erase_d8(struct flashctx *flash, unsigned int addr, +static int spi_block_erase_d8(struct flashctx *flash, unsigned int addr, unsigned int blocklen) { /* This usually takes 100-4000ms, so wait in 100ms steps. */ @@ -523,7 +523,7 @@ /* Block size is usually * 4k for PMC */ -int spi_block_erase_d7(struct flashctx *flash, unsigned int addr, +static int spi_block_erase_d7(struct flashctx *flash, unsigned int addr, unsigned int blocklen) { /* This usually takes 100-4000ms, so wait in 100ms steps. */ @@ -531,7 +531,7 @@ } /* Page erase (usually 256B blocks) */ -int spi_block_erase_db(struct flashctx *flash, unsigned int addr, unsigned int blocklen) +static int spi_block_erase_db(struct flashctx *flash, unsigned int addr, unsigned int blocklen) { /* This takes up to 20ms usually (on worn out devices up to the 0.5s range), so wait in 1ms steps. */ @@ -539,26 +539,26 @@ } /* Sector size is usually 4k, though Macronix eliteflash has 64k */ -int spi_block_erase_20(struct flashctx *flash, unsigned int addr, +static int spi_block_erase_20(struct flashctx *flash, unsigned int addr, unsigned int blocklen) { /* This usually takes 15-800ms, so wait in 10ms steps. */ return spi_write_cmd(flash, JEDEC_SE, false, addr, NULL, 0, 10 * 1000); } -int spi_block_erase_50(struct flashctx *flash, unsigned int addr, unsigned int blocklen) +static int spi_block_erase_50(struct flashctx *flash, unsigned int addr, unsigned int blocklen) { /* This usually takes 10ms, so wait in 1ms steps. */ return spi_write_cmd(flash, JEDEC_BE_50, false, addr, NULL, 0, 1 * 1000); } -int spi_block_erase_81(struct flashctx *flash, unsigned int addr, unsigned int blocklen) +static int spi_block_erase_81(struct flashctx *flash, unsigned int addr, unsigned int blocklen) { /* This usually takes 8ms, so wait in 1ms steps. */ return spi_write_cmd(flash, JEDEC_BE_81, false, addr, NULL, 0, 1 * 1000); } -int spi_block_erase_60(struct flashctx *flash, unsigned int addr, +static int spi_block_erase_60(struct flashctx *flash, unsigned int addr, unsigned int blocklen) { if ((addr != 0) || (blocklen != flash->chip->total_size * 1024)) { @@ -569,7 +569,7 @@ return spi_chip_erase_60(flash); } -int spi_block_erase_62(struct flashctx *flash, unsigned int addr, unsigned int blocklen) +static int spi_block_erase_62(struct flashctx *flash, unsigned int addr, unsigned int blocklen) { if ((addr != 0) || (blocklen != flash->chip->total_size * 1024)) { msg_cerr("%s called with incorrect arguments\n", @@ -579,7 +579,7 @@ return spi_chip_erase_62(flash); } -int spi_block_erase_c7(struct flashctx *flash, unsigned int addr, +static int spi_block_erase_c7(struct flashctx *flash, unsigned int addr, unsigned int blocklen) { if ((addr != 0) || (blocklen != flash->chip->total_size * 1024)) { @@ -591,33 +591,84 @@ } /* Erase 4 KB of flash with 4-bytes address from ANY mode (3-bytes or 4-bytes) */ -int spi_block_erase_21(struct flashctx *flash, unsigned int addr, unsigned int blocklen) +static int spi_block_erase_21(struct flashctx *flash, unsigned int addr, unsigned int blocklen) { /* This usually takes 15-800ms, so wait in 10ms steps. */ return spi_write_cmd(flash, 0x21, true, addr, NULL, 0, 10 * 1000); } /* Erase 32 KB of flash with 4-bytes address from ANY mode (3-bytes or 4-bytes) */ -int spi_block_erase_53(struct flashctx *flash, unsigned int addr, unsigned int blocklen) +static int spi_block_erase_53(struct flashctx *flash, unsigned int addr, unsigned int blocklen) { /* This usually takes 100-4000ms, so wait in 100ms steps. */ return spi_write_cmd(flash, 0x53, true, addr, NULL, 0, 100 * 1000); } /* Erase 32 KB of flash with 4-bytes address from ANY mode (3-bytes or 4-bytes) */ -int spi_block_erase_5c(struct flashctx *flash, unsigned int addr, unsigned int blocklen) +static int spi_block_erase_5c(struct flashctx *flash, unsigned int addr, unsigned int blocklen) { /* This usually takes 100-4000ms, so wait in 100ms steps. */ return spi_write_cmd(flash, 0x5c, true, addr, NULL, 0, 100 * 1000); } /* Erase 64 KB of flash with 4-bytes address from ANY mode (3-bytes or 4-bytes) */ -int spi_block_erase_dc(struct flashctx *flash, unsigned int addr, unsigned int blocklen) +static int spi_block_erase_dc(struct flashctx *flash, unsigned int addr, unsigned int blocklen) { /* This usually takes 100-4000ms, so wait in 100ms steps. */ return spi_write_cmd(flash, 0xdc, true, addr, NULL, 0, 100 * 1000); } +/* special unit-test hook */ +erasefunc_t *g_test_erase_injector; + +erasefunc_t *lookup_erase_func_ptr(const struct block_eraser *const eraser) +{ + switch (eraser->block_erase) { + case SPI_BLOCK_ERASE_EMULATION: return &spi_block_erase_emulation; + case SPI_BLOCK_ERASE_20: return &spi_block_erase_20; + case SPI_BLOCK_ERASE_21: return &spi_block_erase_21; + case SPI_BLOCK_ERASE_40: return NULL; // FIXME unhandled &spi_block_erase_40; + case SPI_BLOCK_ERASE_50: return &spi_block_erase_50; + case SPI_BLOCK_ERASE_52: return &spi_block_erase_52; + case SPI_BLOCK_ERASE_53: return &spi_block_erase_53; + case SPI_BLOCK_ERASE_5C: return &spi_block_erase_5c; + case SPI_BLOCK_ERASE_60: return &spi_block_erase_60; + case SPI_BLOCK_ERASE_62: return &spi_block_erase_62; + case SPI_BLOCK_ERASE_81: return &spi_block_erase_81; + case SPI_BLOCK_ERASE_C4: return &spi_block_erase_c4; + case SPI_BLOCK_ERASE_C7: return &spi_block_erase_c7; + case SPI_BLOCK_ERASE_D7: return &spi_block_erase_d7; + case SPI_BLOCK_ERASE_D8: return &spi_block_erase_d8; + case SPI_BLOCK_ERASE_DB: return &spi_block_erase_db; + case SPI_BLOCK_ERASE_DC: return &spi_block_erase_dc; + case S25FL_BLOCK_ERASE: return &s25fl_block_erase; + case S25FS_BLOCK_ERASE_D8: return &s25fs_block_erase_d8; + case JEDEC_SECTOR_ERASE: return &erase_sector_jedec; // TODO rename to &jedec_sector_erase; + case JEDEC_BLOCK_ERASE: return &erase_block_jedec; // TODO rename to &jedec_block_erase; + case JEDEC_CHIP_BLOCK_ERASE: return &erase_chip_block_jedec; // TODO rename to &jedec_chip_block_erase; + case OPAQUE_ERASE: return &erase_opaque; // TODO rename to &opqaue_erase; + case SPI_ERASE_AT45CS_SECTOR: return &spi_erase_at45cs_sector; + case SPI_ERASE_AT45DB_BLOCK: return &spi_erase_at45db_block; + case SPI_ERASE_AT45DB_CHIP: return &spi_erase_at45db_chip; + case SPI_ERASE_AT45DB_PAGE: return &spi_erase_at45db_page; + case SPI_ERASE_AT45DB_SECTOR: return &spi_erase_at45db_sector; + case ERASE_CHIP_28SF040: return &erase_chip_28sf040; + case ERASE_SECTOR_28SF040: return &erase_sector_28sf040; + case ERASE_BLOCK_82802AB: return &erase_block_82802ab; + case ERASE_SECTOR_49LFXXXC: return &erase_sector_49lfxxxc; + case STM50_SECTOR_ERASE: return &erase_sector_stm50; // TODO rename to &stm50_sector_erase; + case EDI_CHIP_BLOCK_ERASE: return &edi_chip_block_erase; + case TEST_ERASE_INJECTOR: return g_test_erase_injector; + /* default: total function, 0 indicates no erase function set. + * We explicitly do not want a default catch-all case in the switch + * to ensure unhandled enum's are compiler warnings. + */ + case NO_BLOCK_ERASE_FUNC: return NULL; + }; + + return NULL; +} + static const struct { enum block_erase_func func; uint8_t opcode; -- To view, visit https://review.coreboot.org/c/flashrom/+/69959 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I9b1dc33fb8d3e98f26f7d4cd1fbb876c3c5580a2 Gerrit-Change-Number: 69959 Gerrit-PatchSet: 1 Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-MessageType: newchange

1 0

[M] Change in flashrom[master]: tests: Redirect to real I/O to support coverage run
by Evan Benn (Code Review) 24 Nov '22

24 Nov '22

Attention is currently required from: Thomas Heijligen, Anastasia Klimchuk. Evan Benn has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/69539 ) Change subject: tests: Redirect to real I/O to support coverage run ...................................................................... Patch Set 2: (7 comments) Commit Message: https://review.coreboot.org/c/flashrom/+/69539/comment/c3076583_9792b85e PS1, Line 7: Add unmock_io.c > Redirect to real I/O to support coverage run Done https://review.coreboot.org/c/flashrom/+/69539/comment/944192b4_69a6e8fc PS1, Line 10: This is to support code coverage. > remove Done https://review.coreboot.org/c/flashrom/+/69539/comment/c4dd78db_98d053a8 PS1, Line 8: : Add functions and a struct that defer the io functions mocked by : mock_io to the real implementations. > Implement a check that redirects mock io functions to the real implementations. […] Done File tests/unmock_io.c: https://review.coreboot.org/c/flashrom/+/69539/comment/1cbb1b19_183fa8cb PS1, Line 19: unmock_io > How about `io_real.h` (and `io_real.c`)? this aligns with `io_mock` that we already have. […] Done https://review.coreboot.org/c/flashrom/+/69539/comment/33ec8082_af259f59 PS1, Line 25: (void)state; > Just drop this line, none other custom mocks are using it. […] Done https://review.coreboot.org/c/flashrom/+/69539/comment/f22a7b33_ed8bc255 PS1, Line 43: // Mock ios that defer to the real io functions. : // These exist so that code coverage can print to real files on disk. > Please format the comment as /* */ and same below Done https://review.coreboot.org/c/flashrom/+/69539/comment/b8a490d1_950baab7 PS1, Line 61: maybe_unmock_io > Let's add a comment for this function: […] the function is documented in the header -- To view, visit https://review.coreboot.org/c/flashrom/+/69539 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I0817fce6ea0f53a4c127794a0d8246504675f805 Gerrit-Change-Number: 69539 Gerrit-PatchSet: 2 Gerrit-Owner: Evan Benn <evanbenn(a)google.com> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Thomas Heijligen <src(a)posteo.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Felix Singer <felixsinger(a)posteo.net> Gerrit-Attention: Thomas Heijligen <src(a)posteo.de> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Comment-Date: Thu, 24 Nov 2022 01:59:29 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-MessageType: comment

1 0

[S] Change in flashrom[master]: tests: Detect gcov run and redirect to real I/O functions
by Evan Benn (Code Review) 24 Nov '22

24 Nov '22

Attention is currently required from: Thomas Heijligen, Anastasia Klimchuk. Evan Benn has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/69266 ) Change subject: tests: Detect gcov run and redirect to real I/O functions ...................................................................... Patch Set 11: (2 comments) Commit Message: https://review.coreboot.org/c/flashrom/+/69266/comment/70c88ad9_c9cd907f PS10, Line 7: Detect gcov file io and unmock io functions > Detect gcov run and redirect to real I/O functions Done https://review.coreboot.org/c/flashrom/+/69266/comment/c18b1b3d_b6f63753 PS10, Line 9: we need to unmock the file io > we need to use real I/O functions Done -- To view, visit https://review.coreboot.org/c/flashrom/+/69266 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: If06053ecd78e886c8f7fc55813f4b5635be78c6b Gerrit-Change-Number: 69266 Gerrit-PatchSet: 11 Gerrit-Owner: Evan Benn <evanbenn(a)google.com> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Thomas Heijligen <src(a)posteo.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Felix Singer <felixsinger(a)posteo.net> Gerrit-Attention: Thomas Heijligen <src(a)posteo.de> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Comment-Date: Thu, 24 Nov 2022 01:58:50 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-MessageType: comment

1 0

[M] Change in flashrom[master]: tests: Detect llvm coverage run and redirect to real I/O functions
by Evan Benn (Code Review) 24 Nov '22

24 Nov '22

Attention is currently required from: Anastasia Klimchuk. Evan Benn has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/69267 ) Change subject: tests: Detect llvm coverage run and redirect to real I/O functions ...................................................................... Patch Set 11: (8 comments) Commit Message: https://review.coreboot.org/c/flashrom/+/69267/comment/5924adf4_e81e7089 PS10, Line 7: tests: Detect llvm coverage file io and unmock io functions > Could you please re-use my comments for commit message from the previous patch into here, too? thank […] Done https://review.coreboot.org/c/flashrom/+/69267/comment/4c12222e_50f8d393 PS10, Line 7: tests: Detect llvm coverage file io and unmock io functions > Could you please re-use my comments for commit message from the previous patch into here, too? thank […] Done File Documentation/building.md: https://review.coreboot.org/c/flashrom/+/69267/comment/a0b1b637_d8cb1513 PS10, Line 52: : #### gcc / gcov > This line needs to be in a previous patch, where the section is added Done https://review.coreboot.org/c/flashrom/+/69267/comment/639d2953_cecefdb2 PS10, Line 52: : #### gcc / gcov > This line needs to be in a previous patch, where the section is added Done File tests/unmock_io.c: https://review.coreboot.org/c/flashrom/+/69267/comment/6a3bbc0d_f2448b42 PS10, Line 32: (void)state; > can be dropped Done https://review.coreboot.org/c/flashrom/+/69267/comment/ccfad5f8_0fcaea72 PS10, Line 32: (void)state; > can be dropped Done https://review.coreboot.org/c/flashrom/+/69267/comment/d71111e7_6de1a74a PS10, Line 79: ".gcda" > Can this go into macro at the beginning of the file? […] only used here so i made a variable same diff https://review.coreboot.org/c/flashrom/+/69267/comment/2da79c58_a150013a PS10, Line 79: ".gcda" > Can this go into macro at the beginning of the file? […] Done -- To view, visit https://review.coreboot.org/c/flashrom/+/69267 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I21cc1d631e92fa19006b967e85676f108e80b307 Gerrit-Change-Number: 69267 Gerrit-PatchSet: 11 Gerrit-Owner: Evan Benn <evanbenn(a)google.com> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Felix Singer <felixsinger(a)posteo.net> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Comment-Date: Thu, 24 Nov 2022 01:58:32 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-MessageType: comment

1 0

[M] Change in flashrom[master]: tests: Add llvm-cov option and run target for code coverage
by Evan Benn (Code Review) 24 Nov '22

24 Nov '22

Attention is currently required from: Thomas Heijligen, Anastasia Klimchuk. Evan Benn has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/69268 ) Change subject: tests: Add llvm-cov option and run target for code coverage ...................................................................... Patch Set 12: (4 comments) Commit Message: https://review.coreboot.org/c/flashrom/+/69268/comment/a533f182_43fd0e6b PS11, Line 14: TEST=meson test; ninja llvm-cov-tests > Can you test scenario when feature disables? when tests disabled? […] I ran it for all the test_build configs with coverage enabled, anad with and without tests. Patchset: PS11: > I dont mind doing that, but is it necessary? The script is in the repo. […] Done File meson.build: https://review.coreboot.org/c/flashrom/+/69268/comment/2e20ae40_5b7222ce PS5, Line 534: if get_option('llvm_cov') > I am confused, but what does it mean, coverage without running tests? code coverage is an independent concept from unit tests. you can collect code coverage on a run of the CLI. This would be useful for integration tests, or for debugging in general. File meson.build: https://review.coreboot.org/c/flashrom/+/69268/comment/5eee5dd1_8810e07e PS11, Line 614: run_target('llvm-cov-cli', command : ['scripts/llvm-cov', classic_cli]) > This should also be guarded Done -- To view, visit https://review.coreboot.org/c/flashrom/+/69268 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Id6c73bff46e7b88d425956a80def97082b201f56 Gerrit-Change-Number: 69268 Gerrit-PatchSet: 12 Gerrit-Owner: Evan Benn <evanbenn(a)google.com> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Thomas Heijligen <src(a)posteo.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Felix Singer <felixsinger(a)posteo.net> Gerrit-Attention: Thomas Heijligen <src(a)posteo.de> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Comment-Date: Thu, 24 Nov 2022 01:57:51 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Thomas Heijligen <src(a)posteo.de> Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org> Comment-In-Reply-To: Evan Benn <evanbenn(a)google.com> Gerrit-MessageType: comment

1 0

[M] Change in flashrom[master]: tests: Redirect to real I/O to support coverage run
by Evan Benn (Code Review) 24 Nov '22

24 Nov '22

Attention is currently required from: Thomas Heijligen, Evan Benn. Hello build bot (Jenkins), Thomas Heijligen, Anastasia Klimchuk, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/flashrom/+/69539 to look at the new patch set (#2). Change subject: tests: Redirect to real I/O to support coverage run ...................................................................... tests: Redirect to real I/O to support coverage run Implement a check that redirects mock io functions to the real implementations. Real I/O functions are needed for the coverage tool to be able to create and write files. BUG=None BRANCH=None TEST=None Change-Id: I0817fce6ea0f53a4c127794a0d8246504675f805 Signed-off-by: Evan Benn <evanbenn(a)chromium.org> --- A tests/io_real.c A tests/io_real.h M tests/meson.build M tests/wraps.h 4 files changed, 104 insertions(+), 0 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/39/69539/2 -- To view, visit https://review.coreboot.org/c/flashrom/+/69539 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I0817fce6ea0f53a4c127794a0d8246504675f805 Gerrit-Change-Number: 69539 Gerrit-PatchSet: 2 Gerrit-Owner: Evan Benn <evanbenn(a)google.com> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Thomas Heijligen <src(a)posteo.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Felix Singer <felixsinger(a)posteo.net> Gerrit-Attention: Thomas Heijligen <src(a)posteo.de> Gerrit-Attention: Evan Benn <evanbenn(a)google.com> Gerrit-MessageType: newpatchset

1 0

[M] Change in flashrom[master]: tests: Add llvm-cov option and run target for code coverage
by Evan Benn (Code Review) 24 Nov '22

24 Nov '22

Attention is currently required from: Thomas Heijligen, Evan Benn. Hello build bot (Jenkins), Thomas Heijligen, Anastasia Klimchuk, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/flashrom/+/69268 to look at the new patch set (#12). Change subject: tests: Add llvm-cov option and run target for code coverage ...................................................................... tests: Add llvm-cov option and run target for code coverage Code coverage can be requested with -Dllvm_cov and run with ninja llvm-cov-tests or llvm-cov-cli. BUG=b:187647884 BRANCH=None TEST=meson test; ninja llvm-cov-tests TEST=ran test_build.sh with coverage enabled Change-Id: Id6c73bff46e7b88d425956a80def97082b201f56 Signed-off-by: Evan Benn <evanbenn(a)chromium.org> --- M Documentation/building.md M meson.build M meson_options.txt A scripts/llvm-cov M tests/meson.build 5 files changed, 43 insertions(+), 9 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/68/69268/12 -- To view, visit https://review.coreboot.org/c/flashrom/+/69268 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Id6c73bff46e7b88d425956a80def97082b201f56 Gerrit-Change-Number: 69268 Gerrit-PatchSet: 12 Gerrit-Owner: Evan Benn <evanbenn(a)google.com> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Thomas Heijligen <src(a)posteo.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Felix Singer <felixsinger(a)posteo.net> Gerrit-Attention: Thomas Heijligen <src(a)posteo.de> Gerrit-Attention: Evan Benn <evanbenn(a)google.com> Gerrit-MessageType: newpatchset

1 0

[M] Change in flashrom[master]: tests: Detect llvm coverage run and redirect to real I/O functions
by Evan Benn (Code Review) 24 Nov '22

24 Nov '22

Attention is currently required from: Evan Benn. Hello build bot (Jenkins), Anastasia Klimchuk, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/flashrom/+/69267 to look at the new patch set (#11). Change subject: tests: Detect llvm coverage run and redirect to real I/O functions ...................................................................... tests: Detect llvm coverage run and redirect to real I/O functions Code coverage writes data to disk, we need to use real io functions at this point so that the data is really written. BUG=b:187647884 BRANCH=None TEST=llvm-profdata merge -sparse default.profraw -o default.profdata TEST=llvm-cov show ./flashrom_unit_tests -instr-profile=default.profdata --format=html --output-dir=. Change-Id: I21cc1d631e92fa19006b967e85676f108e80b307 Signed-off-by: Evan Benn <evanbenn(a)chromium.org> --- M Documentation/building.md M tests/io_real.c M tests/tests.c M tests/wraps.h 4 files changed, 50 insertions(+), 1 deletion(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/67/69267/11 -- To view, visit https://review.coreboot.org/c/flashrom/+/69267 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I21cc1d631e92fa19006b967e85676f108e80b307 Gerrit-Change-Number: 69267 Gerrit-PatchSet: 11 Gerrit-Owner: Evan Benn <evanbenn(a)google.com> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Felix Singer <felixsinger(a)posteo.net> Gerrit-Attention: Evan Benn <evanbenn(a)google.com> Gerrit-MessageType: newpatchset

1 0

[S] Change in flashrom[master]: tests: Detect gcov run and redirect to real I/O functions
by Evan Benn (Code Review) 24 Nov '22

24 Nov '22

Attention is currently required from: Thomas Heijligen, Evan Benn. Hello build bot (Jenkins), Thomas Heijligen, Anastasia Klimchuk, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/flashrom/+/69266 to look at the new patch set (#11). Change subject: tests: Detect gcov run and redirect to real I/O functions ...................................................................... tests: Detect gcov run and redirect to real I/O functions Code coverage writes data to disk, we need to use real io functions at this point so that the data is really written. BUG=b:187647884 BRANCH=None TEST=meson test TEST=ninja coverage Change-Id: If06053ecd78e886c8f7fc55813f4b5635be78c6b Signed-off-by: Evan Benn <evanbenn(a)chromium.org> --- M Documentation/building.md M tests/tests.c 2 files changed, 32 insertions(+), 0 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/66/69266/11 -- To view, visit https://review.coreboot.org/c/flashrom/+/69266 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: If06053ecd78e886c8f7fc55813f4b5635be78c6b Gerrit-Change-Number: 69266 Gerrit-PatchSet: 11 Gerrit-Owner: Evan Benn <evanbenn(a)google.com> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Thomas Heijligen <src(a)posteo.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Felix Singer <felixsinger(a)posteo.net> Gerrit-Attention: Thomas Heijligen <src(a)posteo.de> Gerrit-Attention: Evan Benn <evanbenn(a)google.com> Gerrit-MessageType: newpatchset

1 0

[M] Change in flashrom[master]: tests: Add selfcheck to unit tests
by Evan Benn (Code Review) 24 Nov '22

24 Nov '22

Attention is currently required from: Felix Singer, Thomas Heijligen, Edward O'Callaghan, Anastasia Klimchuk. Evan Benn has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/69620 ) Change subject: tests: Add selfcheck to unit tests ...................................................................... Patch Set 6: (15 comments) Commit Message: https://review.coreboot.org/c/flashrom/+/69620/comment/bcb98b22_522cba6c PS6, Line 7: Convert > Replace Convert with Add. […] Done File board_enable.c: https://review.coreboot.org/c/flashrom/+/69620/comment/f1d180e1_08c13711 PS6, Line 2527: const size_t board_matches_size = ARRAY_SIZE(board_matches); > Why this can't be in the test code? the size of the array is only known in the translation unit it is defined in File tests/meson.build: https://review.coreboot.org/c/flashrom/+/69620/comment/f6d42ec1_2eaba2ae PS6, Line 25: well_formed > I would rename all occurrences of "well_formed" into "selfcheck": file names, test methods names etc […] Done File tests/well_formed_tables.c: https://review.coreboot.org/c/flashrom/+/69620/comment/dcdd942e_02a36d86 PS6, Line 23: name > Maybe rename to prog_name? its only prog_name for one of the callsites https://review.coreboot.org/c/flashrom/+/69620/comment/22ff445a_ce150d65 PS6, Line 26: is false > is false or NULL Done https://review.coreboot.org/c/flashrom/+/69620/comment/6c50d1e5_8d6c15aa PS6, Line 26: #assertion > I assume this would print "p is false" which can be confusing. […] #assertion is copied from what normal cmocka assertions print. We can do better. https://review.coreboot.org/c/flashrom/+/69620/comment/2ff04049_0534353d PS6, Line 24: do { \ : if (!(assertion)) \ : fail_msg(#assertion " is false for index:%zu name:%s", (index), \ : (name) ? (name) : "unknown"); \ : } while (0) > I was also confused in the beginning, but then I thought this is probably a trick to avoid swallowin […] yes its the standard semicolon trick for macros. So you can call it like a function. https://review.coreboot.org/c/flashrom/+/69620/comment/db31623b_d14b0e8a PS6, Line 33: (void)state; /* unused */ > Add a new line after `(void)state; /* unused */` and the same for all other test methods. Done https://review.coreboot.org/c/flashrom/+/69620/comment/3f408137_585db4a4 PS6, Line 41: if (strcmp("internal", p->name) != 0) : assert_table(p->devs.note, i, p->name); > Original code has this check for all programmers with p->type == OTHER […] the original code does this check for all types except 'default', and for default it does an unconditional assertion. I replaced that assertion with assert(p->type), although that is not as good as it no longer checks for non-valid values of the enum. I added that back in. https://review.coreboot.org/c/flashrom/+/69620/comment/45140d0d_f5bc04d8 PS6, Line 51: assert_false(flashchips_size <= 1 || flashchips[flashchips_size - 1].name != NULL); > Break down into two `assert_true`s Done https://review.coreboot.org/c/flashrom/+/69620/comment/b2bc0543_4d49ff81 PS6, Line 58: } > You missed `selfcheck_eraseblocks` functionality? […] Done https://review.coreboot.org/c/flashrom/+/69620/comment/43193fb6_5e543a55 PS6, Line 66: assert_false(board_matches_size <= 1 : || board_matches[board_matches_size - 1].vendor_name != NULL); > Break down into two `assert_true`s Done https://review.coreboot.org/c/flashrom/+/69620/comment/83248581_22f7bf60 PS6, Line 71: assert_table(b->board_name, i, b->board_name); : assert_table(b->board_name, i, b->board_name); > The same line twice. […] Done https://review.coreboot.org/c/flashrom/+/69620/comment/4da57f91_eb0b43fc PS6, Line 83: SKIP_TEST > Indent with tab Done https://review.coreboot.org/c/flashrom/+/69620/comment/2b68e0b2_33a8f7b6 PS6, Line 84: // CONFIG_INTERNAL > /* comment */ Done -- To view, visit https://review.coreboot.org/c/flashrom/+/69620 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I41cd014d9bf909296b6c28e3e00548e6883ff41a Gerrit-Change-Number: 69620 Gerrit-PatchSet: 6 Gerrit-Owner: Evan Benn <evanbenn(a)google.com> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Thomas Heijligen <src(a)posteo.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Felix Singer <felixsinger(a)posteo.net> Gerrit-Attention: Felix Singer <felixsinger(a)posteo.net> Gerrit-Attention: Thomas Heijligen <src(a)posteo.de> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Comment-Date: Thu, 24 Nov 2022 01:09:45 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Felix Singer <felixsinger(a)posteo.net> Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-MessageType: comment

1 0

2024

2023

2022

2021

2020

2019

2018

2017

flashrom-gerrit