- flashrom-gerrit - mail.coreboot.org

[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, Evan Benn. Hello build bot (Jenkins), Thomas Heijligen, Edward O'Callaghan, Anastasia Klimchuk, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/flashrom/+/69620 to look at the new patch set (#7). Change subject: tests: Add selfcheck to unit tests ...................................................................... tests: Add selfcheck to unit tests Add unit tests for programmer_table, flashchips, and board_matches structs. The tests are derived from the selfcheck function, checking that the required fields have been filled in. BUG=b:140595239 BRANCH=None TEST=meson test Change-Id: I41cd014d9bf909296b6c28e3e00548e6883ff41a Signed-off-by: Evan Benn <evanbenn(a)chromium.org> --- M board_enable.c M include/programmer.h M tests/meson.build A tests/selfcheck.c M tests/tests.c M tests/tests.h 6 files changed, 213 insertions(+), 0 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/20/69620/7 -- 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: 7 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: 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 Anastasia Klimchuk (Code Review) 24 Nov '22

24 Nov '22

Attention is currently required from: Thomas Heijligen, Evan Benn. Anastasia Klimchuk 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 11: (2 comments) Commit Message: https://review.coreboot.org/c/flashrom/+/69268/comment/6b315a32_75a571f8 PS11, Line 14: TEST=meson test; ninja llvm-cov-tests Can you test scenario when feature disables? when tests disabled? Obviously, when feature disabled then coverage is not measured. The point of testing is to check flashrom builds and runs normally (without coverage and/or tests). And please add those to the test scenarios in commit message. Thanks! File meson.build: https://review.coreboot.org/c/flashrom/+/69268/comment/12518494_58fff455 PS5, Line 534: if get_option('llvm_cov') > coverage can be used independently of unit tests I am confused, but what does it mean, coverage without running tests? -- 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: 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-Comment-Date: Thu, 24 Nov 2022 00:41:22 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No 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]: tree/: Convert unlock func ptr into enumerate values
by Edward O'Callaghan (Code Review) 24 Nov '22

24 Nov '22

Attention is currently required from: Felix Singer, Angel Pons, Liam Flaherty. Hello Felix Singer, build bot (Jenkins), Angel Pons, Liam Flaherty, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/flashrom/+/69933 to look at the new patch set (#3). Change subject: tree/: Convert unlock func ptr into enumerate values ...................................................................... tree/: Convert unlock func ptr into enumerate values By converting the blockprotect unlock function pointer within the flashchip struct into enum values allows for the flashchips db to be turn into pure, declarative data. A nice side-effect of this is to reduce link-time symbol space of chipdrivers and increase modularity of the spi25_statusreg.c and related implementations. BUG=none TEST=ninja test. Change-Id: Ie5c5db1b09d07e1a549990d6f5a622fae4c83233 Signed-off-by: Edward O'Callaghan <quasisec(a)google.com> --- M 82802ab.c M flashrom.c M include/chipdrivers.h M include/flash.h M jedec.c M spi25_statusreg.c M tests/chip.c M tests/chip_wp.c 8 files changed, 151 insertions(+), 73 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/33/69933/3 -- To view, visit https://review.coreboot.org/c/flashrom/+/69933 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ie5c5db1b09d07e1a549990d6f5a622fae4c83233 Gerrit-Change-Number: 69933 Gerrit-PatchSet: 3 Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Felix Singer <felixsinger(a)posteo.net> Gerrit-Reviewer: Liam Flaherty <liamflaherty(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Felix Singer <felixsinger(a)posteo.net> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Liam Flaherty <liamflaherty(a)chromium.org> Gerrit-MessageType: newpatchset

1 0

[M] Change in flashrom[master]: tests: Detect llvm coverage file io and unmock io functions
by Anastasia Klimchuk (Code Review) 24 Nov '22

24 Nov '22

Attention is currently required from: Evan Benn. Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/69267 ) Change subject: tests: Detect llvm coverage file io and unmock io functions ...................................................................... Patch Set 10: (4 comments) Commit Message: https://review.coreboot.org/c/flashrom/+/69267/comment/f3e08fd3_587611ed 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? thanks! File Documentation/building.md: https://review.coreboot.org/c/flashrom/+/69267/comment/2097605a_aee8f9b5 PS10, Line 52: : #### gcc / gcov This line needs to be in a previous patch, where the section is added File tests/unmock_io.c: https://review.coreboot.org/c/flashrom/+/69267/comment/a4bc875d_4582a185 PS10, Line 32: (void)state; can be dropped https://review.coreboot.org/c/flashrom/+/69267/comment/e75d9d4c_14adf93e PS10, Line 79: ".gcda" Can this go into macro at the beginning of the file? GCOV_COVERAGE_FILE LLVM_COVERAGE_FILE -- 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: 10 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-Comment-Date: Thu, 24 Nov 2022 00:22:14 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

[S] Change in flashrom[master]: tests: Detect gcov file io and unmock io functions
by Anastasia Klimchuk (Code Review) 24 Nov '22

24 Nov '22

Attention is currently required from: Thomas Heijligen, Evan Benn. Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/69266 ) Change subject: tests: Detect gcov file io and unmock io functions ...................................................................... Patch Set 10: (2 comments) Commit Message: https://review.coreboot.org/c/flashrom/+/69266/comment/6b019351_1d99152c PS10, Line 7: Detect gcov file io and unmock io functions Detect gcov run and redirect to real I/O functions https://review.coreboot.org/c/flashrom/+/69266/comment/615a376c_626a43df PS10, Line 9: we need to unmock the file io we need to use real I/O functions -- 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: 10 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-Comment-Date: Thu, 24 Nov 2022 00:18:40 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

[M] Change in flashrom[master]: tree/: Convert unlock func ptr into enumerate values
by Edward O'Callaghan (Code Review) 24 Nov '22

24 Nov '22

Attention is currently required from: Felix Singer, Angel Pons, Liam Flaherty. Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/69933 ) Change subject: tree/: Convert unlock func ptr into enumerate values ...................................................................... Patch Set 2: This change is ready for review. -- To view, visit https://review.coreboot.org/c/flashrom/+/69933 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ie5c5db1b09d07e1a549990d6f5a622fae4c83233 Gerrit-Change-Number: 69933 Gerrit-PatchSet: 2 Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Felix Singer <felixsinger(a)posteo.net> Gerrit-Reviewer: Liam Flaherty <liamflaherty(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Felix Singer <felixsinger(a)posteo.net> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Liam Flaherty <liamflaherty(a)chromium.org> Gerrit-Comment-Date: Thu, 24 Nov 2022 00:16:35 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

[M] Change in flashrom[master]: tests: Add unmock_io.c
by Anastasia Klimchuk (Code Review) 24 Nov '22

24 Nov '22

Attention is currently required from: Thomas Heijligen, Evan Benn. Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/69539 ) Change subject: tests: Add unmock_io.c ...................................................................... Patch Set 1: (7 comments) Commit Message: https://review.coreboot.org/c/flashrom/+/69539/comment/400974c9_c8d84192 PS1, Line 7: Add unmock_io.c Redirect to real I/O to support coverage run https://review.coreboot.org/c/flashrom/+/69539/comment/1eef9953_5b36b3e7 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. Real I/O functions are needed for the coverage tool to be able to create and write files. https://review.coreboot.org/c/flashrom/+/69539/comment/ad864346_2eddbbaa PS1, Line 10: This is to support code coverage. remove File tests/unmock_io.c: https://review.coreboot.org/c/flashrom/+/69539/comment/cbdaff95_219de286 PS1, Line 19: unmock_io How about `io_real.h` (and `io_real.c`)? this aligns with `io_mock` that we already have. Function names are all fine, I am specifically about file names https://review.coreboot.org/c/flashrom/+/69539/comment/eefc0c06_762ea4ef PS1, Line 25: (void)state; Just drop this line, none other custom mocks are using it. This line is used in test methods (there are none in this file). https://review.coreboot.org/c/flashrom/+/69539/comment/4a6d12f9_310934f6 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 https://review.coreboot.org/c/flashrom/+/69539/comment/bc915c46_9502c822 PS1, Line 61: maybe_unmock_io Let's add a comment for this function: > Detects a coverage run and registers real I/O functions instead of mocks so that coverage tools can create and write real files and generate a coverage report. -- 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: 1 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-Comment-Date: Thu, 24 Nov 2022 00:12:45 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

[M] Change in flashrom[master]: tests: Mock the mode_t variant of open
by Anastasia Klimchuk (Code Review) 24 Nov '22

24 Nov '22

Attention is currently required from: Evan Benn. Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/69263 ) Change subject: tests: Mock the mode_t variant of open ...................................................................... Patch Set 7: (2 comments) File tests/io_mock.h: https://review.coreboot.org/c/flashrom/+/69263/comment/73dd77c8_11a3484d PS4, Line 37: #include <fcntl.h> > this one is posix, and posix says: `The <fcntl.h> header shall define the mode_t` […] I will try to test File tests/tests.c: https://review.coreboot.org/c/flashrom/+/69263/comment/5344f4cd_5e133b3f PS4, Line 103: mode_t mode = 0; : if (flags & O_CREAT) { : va_list ap; : va_start(ap, flags); : mode = va_arg(ap, mode_t); : va_end(ap); : } > It has to be in the varargs function. […] open64 and open64_2 are needed for some of the environments, so we can't remove them. Let's keep as is then. -- To view, visit https://review.coreboot.org/c/flashrom/+/69263 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I8c134e6d36a248d0f51985e389085a9e585fb83d Gerrit-Change-Number: 69263 Gerrit-PatchSet: 7 Gerrit-Owner: Evan Benn <evanbenn(a)google.com> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> 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-Comment-Date: Wed, 23 Nov 2022 23:51:19 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No 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

[S] Change in flashrom[master]: flashrom_tester: Fix unit test error
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/+/69765 ) Change subject: flashrom_tester: Fix unit test error ...................................................................... flashrom_tester: Fix unit test error Commit 065366d (flashrom_tester: Change timestamp to UTC microsecond) changed the time format, breaking the unit test. This patch fixes the unit test. BUG=None BRANCH=None TEST=cargo test Change-Id: Iba42a9026b41ddb61bb704aa1c26783cd251d787 Signed-off-by: Evan Benn <evanbenn(a)chromium.org> Reviewed-on: https://review.coreboot.org/c/flashrom/+/69765 Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org> Reviewed-by: Angel Pons <th3fanbus(a)gmail.com> Reviewed-by: Peter Marheine <pmarheine(a)chromium.org> Reviewed-by: Edward O'Callaghan <quasisec(a)chromium.org> --- M util/flashrom_tester/src/logger.rs 1 file changed, 26 insertions(+), 2 deletions(-) Approvals: build bot (Jenkins): Verified Angel Pons: Looks good to me, approved 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/logger.rs b/util/flashrom_tester/src/logger.rs index fb26bde..b700c19 100644 --- a/util/flashrom_tester/src/logger.rs +++ b/util/flashrom_tester/src/logger.rs @@ -153,9 +153,10 @@ assert_eq!(&buf[..5], "\x1b[35m"); // Time is difficult to test, assume it's formatted okay + // Split on the UTC timezone char assert_eq!( - &buf[24..], - " \x1b[33m[ INFO ]\x1b[0m Test message at INFO\n" + buf.split_once("Z ").unwrap().1, + "\x1b[33m[ INFO ]\x1b[0m Test message at INFO\n" ); } -- To view, visit https://review.coreboot.org/c/flashrom/+/69765 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Iba42a9026b41ddb61bb704aa1c26783cd251d787 Gerrit-Change-Number: 69765 Gerrit-PatchSet: 3 Gerrit-Owner: Evan Benn <evanbenn(a)google.com> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.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]: flashchips: Add support for XMC XM25QH128A.
by Anastasia Klimchuk (Code Review) 24 Nov '22

24 Nov '22

Attention is currently required from: foss(a)volatilesystems.org, Angel Pons. Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/69309 ) Change subject: flashchips: Add support for XMC XM25QH128A. ...................................................................... Patch Set 2: (1 comment) Patchset: PS1: > Thought it might have been better to wait for clarity on the commit message before doing that, but I […] Usually the rule of thumb is: if you mark comments as resolved, you need to upload new patchset which demonstrates that you indeed resolved the comments. Otherwise how do we know what you have in local repo? :) Gerrit allows to view "incremental" diffs between patchsets, which can be very handy. Also it is entirely normal to resolve some of the comments (not all of them), and for the remaining ones ask for clarification, not a problem! -- To view, visit https://review.coreboot.org/c/flashrom/+/69309 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Iced40403c6694a55fd648ea2785cdcba21712234 Gerrit-Change-Number: 69309 Gerrit-PatchSet: 2 Gerrit-Owner: foss(a)volatilesystems.org Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Felix Singer <felixsinger(a)posteo.net> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: foss(a)volatilesystems.org Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Comment-Date: Wed, 23 Nov 2022 23:07:00 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: foss(a)volatilesystems.org 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