November 2022 - flashrom-gerrit

[S] Change in flashrom[master]: flashrom_tester: lock_test: Make the logic clear and explicit
by Edward O'Callaghan (Code Review) 24 Nov '22

24 Nov '22

Edward O'Callaghan has submitted this change. ( https://review.coreboot.org/c/flashrom/+/69400 ) ( 4 is the latest approved patch-set. No files were changed between the latest approved patch-set and the submitted one. )Change subject: flashrom_tester: lock_test: Make the logic clear and explicit ...................................................................... flashrom_tester: lock_test: Make the logic clear and explicit Document clearly what the test is doing and expects. Do not use the push() function as it confuses the logs in the case of error. BUG=b:258357944 BRANCH=None TEST=flashrom_tester --libflashrom host Lock Change-Id: Ibad559a8ff9696fd91f45bca9d9ceb6e90c41393 Signed-off-by: Evan Benn <evanbenn(a)chromium.org> Reviewed-on: https://review.coreboot.org/c/flashrom/+/69400 Reviewed-by: Peter Marheine <pmarheine(a)chromium.org> Reviewed-by: Edward O'Callaghan <quasisec(a)chromium.org> Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org> --- M util/flashrom_tester/src/tests.rs 1 file changed, 25 insertions(+), 4 deletions(-) Approvals: build bot (Jenkins): Verified Edward O'Callaghan: Looks good to me, approved Peter Marheine: Looks good to me, but someone else must approve diff --git a/util/flashrom_tester/src/tests.rs b/util/flashrom_tester/src/tests.rs index af34208..5cbe9d2 100644 --- a/util/flashrom_tester/src/tests.rs +++ b/util/flashrom_tester/src/tests.rs @@ -199,12 +199,12 @@ } env.wp.set_hw(false)?.set_sw(true)?; - // Toggling software WP off should work when hardware is off. - // Then enable again for another go. - env.wp.push().set_sw(false)?; + // Toggling software WP off should work when hardware WP is off. + // Then enable software WP again for the next test. + env.wp.set_sw(false)?.set_sw(true)?; + // Toggling software WP off should not work when hardware WP is on. env.wp.set_hw(true)?; - // Clearing should fail when hardware is enabled if env.wp.set_sw(false).is_ok() { return Err("Software WP was reset despite hardware WP being enabled".into()); } -- To view, visit https://review.coreboot.org/c/flashrom/+/69400 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ibad559a8ff9696fd91f45bca9d9ceb6e90c41393 Gerrit-Change-Number: 69400 Gerrit-PatchSet: 6 Gerrit-Owner: Evan Benn <evanbenn(a)google.com> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Peter Marheine <pmarheine(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-MessageType: merged

1 0

[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: 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: 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

2024

2023

2022

2021

2020

2019

2018

2017

flashrom-gerrit November 2022