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

Change in flashrom[master]: tests: Add unit test to run init/shutdown for mec1308.c, ene_lpc.c
by Simon Glass (Code Review) 04 May '21

04 May '21

Attention is currently required from: Nico Huber, Edward O'Callaghan, Angel Pons, Anastasia Klimchuk. Simon Glass has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/51487 ) Change subject: tests: Add unit test to run init/shutdown for mec1308.c, ene_lpc.c ...................................................................... Patch Set 10: Code-Review+1 (1 comment) File tests/init_shutdown.c: https://review.coreboot.org/c/flashrom/+/51487/comment/b11241b2_4b231f69 PS10, Line 53: MEC1308_SIO_PORT1 > Nico, Simon, thank you so much for this conversation, really useful for me to read! […] Well that's fine, you are the one doing the work, after all. Yes working in untested code is not a lot of fun. Hopefully you don't get too many war stories from it! -- To view, visit https://review.coreboot.org/c/flashrom/+/51487 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I3af612defe1af3850dfc1626a208d873e3a3eddc Gerrit-Change-Number: 51487 Gerrit-PatchSet: 10 Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Simon Glass <sjg(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Attention: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Comment-Date: Tue, 04 May 2021 23:45:34 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: Yes Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de> Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org> Comment-In-Reply-To: Simon Glass <sjg(a)chromium.org> Gerrit-MessageType: comment

1 0

Change in flashrom[master]: tests: Add unit test to run init/shutdown for mec1308.c, ene_lpc.c
by Anastasia Klimchuk (Code Review) 04 May '21

04 May '21

Attention is currently required from: Nico Huber, Edward O'Callaghan, Angel Pons, Simon Glass. Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/51487 ) Change subject: tests: Add unit test to run init/shutdown for mec1308.c, ene_lpc.c ...................................................................... Patch Set 10: (1 comment) File tests/init_shutdown.c: https://review.coreboot.org/c/flashrom/+/51487/comment/d0246473_9c4eb258 PS10, Line 53: MEC1308_SIO_PORT1 > OK. […] Nico, Simon, thank you so much for this conversation, really useful for me to read! I can understand both approaches, but I am taking the side of leaving #defines in the driver code where they are (especially because I was on that side from the beginning). This point I think is really important: >>> Pulling such >>> definitions out of their compilation unit makes it easier to write >>> tests I suppose. But it also makes it harder to read/review the code. I don't want to make it harder to read/review the code. For tests, I can address read/review with comments like 0x20 /* MEC1308_DEVICE_ID_REG */ Also, how often those values change, for a given chip? Is it ~never? Please correct me if I am wrong. Another thing I want to say about >>> I don't think we should be afraid to modify the code as needed for tests. Not afraid, but need to be very very careful. It is like walking on ice... -- To view, visit https://review.coreboot.org/c/flashrom/+/51487 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I3af612defe1af3850dfc1626a208d873e3a3eddc Gerrit-Change-Number: 51487 Gerrit-PatchSet: 10 Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-CC: Simon Glass <sjg(a)chromium.org> Gerrit-Attention: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Simon Glass <sjg(a)chromium.org> Gerrit-Comment-Date: Tue, 04 May 2021 23:23:13 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de> Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org> Comment-In-Reply-To: Simon Glass <sjg(a)chromium.org> Gerrit-MessageType: comment

1 0

Change in flashrom[master]: libflashrom: Return progress state to the library user
by Anastasia Klimchuk (Code Review) 04 May '21

04 May '21

Attention is currently required from: Richard Hughes, Angel Pons, Patrick Rudolph. Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/49643 ) Change subject: libflashrom: Return progress state to the library user ...................................................................... Patch Set 6: (3 comments) Commit Message: https://review.coreboot.org/c/flashrom/+/49643/comment/c325d5b5_1fa33580 PS6, Line 9: Projects using libflashrom like fwupd expect the user to wait for the operation : to complete. To avoid the user thinking the process has "hung" or "got stuck" : report back the progress complete of the erase, write and read operations That's good, thank you! One thing left, limit for commit message lines is 72 chars, look like this one is over the limit and lines are broken. File libflashrom.h: https://review.coreboot.org/c/flashrom/+/49643/comment/aa2abe69_510ddc5a PS4, Line 106: void * > Is there a reason why last argument doesn't have a name here? all other arguments have names. Done File tests/spi25.c: https://review.coreboot.org/c/flashrom/+/49643/comment/3e8f7ef7_67640bba PS4, Line 54: size_t current, > Thank you for the test! Is there any way to assert what current value is? it should change (unlike t […] Done -- To view, visit https://review.coreboot.org/c/flashrom/+/49643 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I7197572bb7f19e3bdb2bde855d70a0f50fd3854c Gerrit-Change-Number: 49643 Gerrit-PatchSet: 6 Gerrit-Owner: Richard Hughes <richard(a)hughsie.com> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Daniel Campello <campello(a)chromium.org> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Patrick Rudolph <siro(a)das-labor.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-CC: Richard Hughes <hughsient(a)gmail.com> Gerrit-CC: Thomas Heijligen <src(a)posteo.de> Gerrit-Attention: Richard Hughes <richard(a)hughsie.com> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Patrick Rudolph <siro(a)das-labor.org> Gerrit-Comment-Date: Tue, 04 May 2021 22:50:54 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-MessageType: comment

1 0

Change in flashrom[master]: cli_classic.c: add -x option for do_extract()
by Daniel Campello (Code Review) 04 May '21

04 May '21

Daniel Campello has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/52450 ) Change subject: cli_classic.c: add -x option for do_extract() ...................................................................... Patch Set 12: (1 comment) Patchset: PS12: > Daniel, I just realised after hitting submit this is missing the manpage update. […] CB:52892 -- To view, visit https://review.coreboot.org/c/flashrom/+/52450 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I8c69223fa92cf5b50abe070f1ab9f19d3b42f6ff Gerrit-Change-Number: 52450 Gerrit-PatchSet: 12 Gerrit-Owner: Daniel Campello <campello(a)chromium.org> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Sam McNally <sammc(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-Comment-Date: Tue, 04 May 2021 17:46:59 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-MessageType: comment

1 0

Change in flashrom[master]: flashrom.8.tmpl: Add man entry for --extract
by Daniel Campello (Code Review) 04 May '21

04 May '21

Daniel Campello has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/52892 ) Change subject: flashrom.8.tmpl: Add man entry for --extract ...................................................................... flashrom.8.tmpl: Add man entry for --extract This is a follow up change of CB:52450 Signed-off-by: Daniel Campello <campello(a)chromium.org> Change-Id: Icc068f5545b6f30ac390b7b815a31e2d61bf4789 --- M flashrom.8.tmpl 1 file changed, 5 insertions(+), 1 deletion(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/92/52892/1 diff --git a/flashrom.8.tmpl b/flashrom.8.tmpl index 4634539..4ae7e71 100644 --- a/flashrom.8.tmpl +++ b/flashrom.8.tmpl @@ -47,7 +47,7 @@ .B flashrom \fR[\fB\-h\fR|\fB\-R\fR|\fB\-L\fR|\fB\-z\fR| \fB\-p\fR <programmername>[:<parameters>] [\fB\-c\fR <chipname>] (\fB\-\-flash\-name\fR|\fB\-\-flash\-size\fR| - [\fB\-E\fR|\fB\-r\fR <file>|\fB\-w\fR <file>|\fB\-v\fR <file>] + [\fB\-E\fR|\fB\-x\fR|\fB\-r\fR <file>|\fB\-w\fR <file>|\fB\-v\fR <file>] [(\fB\-l\fR <file>|\fB\-\-ifd|\fB \-\-fmap\fR|\fB\-\-fmap-file\fR <file>) [\fB\-i\fR <image>[:<file>]]] [\fB\-n\fR] [\fB\-N\fR] [\fB\-f\fR])] @@ -136,6 +136,10 @@ .B "\-E, \-\-erase" Erase the flash ROM chip. .TP +.B "\-x, \-\-extract" +Extract every region defined on the layout from flash ROM chip to a +file with the same name as the extracted region. +.TP .B "\-V, \-\-verbose" More verbose output. This option can be supplied multiple times (max. 3 times, i.e. -- To view, visit https://review.coreboot.org/c/flashrom/+/52892 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Icc068f5545b6f30ac390b7b815a31e2d61bf4789 Gerrit-Change-Number: 52892 Gerrit-PatchSet: 1 Gerrit-Owner: Daniel Campello <campello(a)chromium.org> Gerrit-MessageType: newchange

1 0

Change in flashrom[master]: CHROMIUM: cli_classic.c: Remove deprecated options
by Daniel Campello (Code Review) 04 May '21

04 May '21

Daniel Campello has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/52890 ) Change subject: CHROMIUM: cli_classic.c: Remove deprecated options ...................................................................... CHROMIUM: cli_classic.c: Remove deprecated options This change removes --fast-verify and --diff options as there are other options that implement the same logic. It also removes --ignore-fmap as this is implicetely computed based on other supplied options by the user. BRANCH=none BUG=b:162798062,b:186479007 TEST=tryjobs Original-Signed-off-by: Daniel Campello <campello(a)chromium.org> Original-Cq-Depend: chromium:2854174, chrome-internal:3789445, chromium:2854014 Original-Change-Id: I841a56d8726644cedd7d616ddfd5656b92dd7e59 Original-Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/third_party/flashrom/… Original-Reviewed-by: Edward O'Callaghan <quasisec(a)chromium.org> (cherry picked from commit 2e2a5e449229e9c9604235a98b56e5dd29bf25cf) Change-Id: Ib329f3adb59ce1848d1540844d64b968f49eb22a --- M util/flashrom_tester/flashrom/src/cmd.rs M util/flashrom_tester/flashrom/src/lib.rs 2 files changed, 3 insertions(+), 10 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/90/52890/1 diff --git a/util/flashrom_tester/flashrom/src/cmd.rs b/util/flashrom_tester/flashrom/src/cmd.rs index 8775ed4..37f8a72 100644 --- a/util/flashrom_tester/flashrom/src/cmd.rs +++ b/util/flashrom_tester/flashrom/src/cmd.rs @@ -129,9 +129,6 @@ if opts.flash_name { params.push("--flash-name".to_string()); } - if opts.ignore_fmap { - params.push("--ignore-fmap".to_string()); - } if opts.verbose { params.push("-V".to_string()); } @@ -325,11 +322,10 @@ assert_eq!( flashrom_decode_opts(FlashromOpt { flash_name: true, - ignore_fmap: true, verbose: true, ..Default::default() }), - &["--flash-name", "--ignore-fmap", "-V"] + &["--flash-name", "-V"] ); } diff --git a/util/flashrom_tester/flashrom/src/lib.rs b/util/flashrom_tester/flashrom/src/lib.rs index 734e3ff..75797d3 100644 --- a/util/flashrom_tester/flashrom/src/lib.rs +++ b/util/flashrom_tester/flashrom/src/lib.rs @@ -91,9 +91,8 @@ pub layout: Option<&'a str>, // -l <file> pub image: Option<&'a str>, // -i <name> - pub flash_name: bool, // --flash-name - pub ignore_fmap: bool, // --ignore-fmap - pub verbose: bool, // -V + pub flash_name: bool, // --flash-name + pub verbose: bool, // -V } #[derive(Default)] @@ -183,8 +182,6 @@ layout: rws.layout_file, image: rws.name_file, - ignore_fmap: true, - ..Default::default() }; -- To view, visit https://review.coreboot.org/c/flashrom/+/52890 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ib329f3adb59ce1848d1540844d64b968f49eb22a Gerrit-Change-Number: 52890 Gerrit-PatchSet: 1 Gerrit-Owner: Daniel Campello <campello(a)chromium.org> Gerrit-MessageType: newchange

1 0

Change in flashrom[master]: CHROMIUM: flashrom_tester: Drop nix dependency
by Daniel Campello (Code Review) 04 May '21

04 May '21

Daniel Campello has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/52889 ) Change subject: CHROMIUM: flashrom_tester: Drop nix dependency ...................................................................... CHROMIUM: flashrom_tester: Drop nix dependency We can just use the libc functions directly. This is exactly what nix does anyway. BUG=none TEST=unit tests BRANCH=none Original-Change-Id: I45c02f0c71d164bd8f504fe2b8d3acd54e0d5704 Original-Signed-off-by: Chirantan Ekbote <chirantan(a)chromium.org> Original-Reviewed-on: https://chromium-review.googlesource.com/c/chromiumos/third_party/flashrom/… Original-Reviewed-by: Edward O'Callaghan <quasisec(a)chromium.org> Original-Reviewed-by: Allen Webb <allenwebb(a)google.com> Original-Commit-Queue: Allen Webb <allenwebb(a)google.com> (cherry picked from commit 1ba7dbe83e01d270b6d8d597a079ea3bfeb2117e) Change-Id: Iea61c65efb04da9cd0bc0bd85a34fc10912ea87b --- M util/flashrom_tester/Cargo.toml M util/flashrom_tester/src/main.rs 2 files changed, 13 insertions(+), 7 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/89/52889/1 diff --git a/util/flashrom_tester/Cargo.toml b/util/flashrom_tester/Cargo.toml index 0898d3c..e7a5820 100644 --- a/util/flashrom_tester/Cargo.toml +++ b/util/flashrom_tester/Cargo.toml @@ -18,8 +18,8 @@ chrono = { version = "0.4", optional = true } clap = { version = "2.33", default-features = false, optional = true } flashrom = { path = "flashrom/" } +libc = "0.2" log = { version = "0.4", features = ["std"] } -nix = "0.14.1" rand = "0.6.4" serde_json = "1" sys-info = "0.5.7" diff --git a/util/flashrom_tester/src/main.rs b/util/flashrom_tester/src/main.rs index e589ee1..80484d2 100644 --- a/util/flashrom_tester/src/main.rs +++ b/util/flashrom_tester/src/main.rs @@ -152,12 +152,11 @@ /// Once a signal is trapped, the default behavior is restored (terminating /// the process) for future signals. fn handle_sigint() -> &'static AtomicBool { - use nix::libc::c_int; - use nix::sys::signal::{self, SigHandler, Signal}; + use libc::c_int; use std::sync::atomic::Ordering; unsafe { - let _ = signal::signal(Signal::SIGINT, SigHandler::Handler(sigint_handler)); + let _ = libc::signal(libc::SIGINT, sigint_handler as libc::sighandler_t); } static TERMINATE_FLAG: AtomicBool = AtomicBool::new(false); @@ -169,10 +168,17 @@ test, or press ^C again to exit immediately (possibly bricking your machine). "; - // Use raw write() because signal-safety is a very hard problem - let _ = nix::unistd::write(STDERR_FILENO, MESSAGE); + // Use raw write() because signal-safety is a very hard problem. Safe because this doesn't + // modify any memory. + let _ = unsafe { + libc::write( + STDERR_FILENO, + MESSAGE.as_ptr() as *const libc::c_void, + MESSAGE.len() as libc::size_t, + ) + }; unsafe { - let _ = signal::signal(Signal::SIGINT, SigHandler::SigDfl); + let _ = libc::signal(libc::SIGINT, libc::SIG_DFL); } TERMINATE_FLAG.store(true, Ordering::Release); } -- To view, visit https://review.coreboot.org/c/flashrom/+/52889 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Iea61c65efb04da9cd0bc0bd85a34fc10912ea87b Gerrit-Change-Number: 52889 Gerrit-PatchSet: 1 Gerrit-Owner: Daniel Campello <campello(a)chromium.org> Gerrit-MessageType: newchange

1 0

Change in flashrom[master]: tests: Add unit test to run init/shutdown for mec1308.c, ene_lpc.c
by Simon Glass (Code Review) 04 May '21

04 May '21

Attention is currently required from: Nico Huber, Edward O'Callaghan, Angel Pons, Anastasia Klimchuk. Simon Glass has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/51487 ) Change subject: tests: Add unit test to run init/shutdown for mec1308.c, ene_lpc.c ...................................................................... Patch Set 10: (1 comment) File tests/init_shutdown.c: https://review.coreboot.org/c/flashrom/+/51487/comment/ebec52fb_31e6d7f2 PS10, Line 53: MEC1308_SIO_PORT1 > > Yes, that's right... […] OK. From my POV I don't see much need to test that the enums/#defines in a header file correspond with values in a test...but I can see situations where it might be useful, such as when testing black-box code. -- To view, visit https://review.coreboot.org/c/flashrom/+/51487 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I3af612defe1af3850dfc1626a208d873e3a3eddc Gerrit-Change-Number: 51487 Gerrit-PatchSet: 10 Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-CC: Simon Glass <sjg(a)chromium.org> Gerrit-Attention: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Comment-Date: Tue, 04 May 2021 16:42:22 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de> Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org> Comment-In-Reply-To: Simon Glass <sjg(a)chromium.org> Gerrit-MessageType: comment

1 0

Change in flashrom[master]: tests: Add unit test to run init/shutdown for mec1308.c, ene_lpc.c
by Nico Huber (Code Review) 04 May '21

04 May '21

Attention is currently required from: Edward O'Callaghan, Angel Pons, Anastasia Klimchuk, Simon Glass. Nico Huber has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/51487 ) Change subject: tests: Add unit test to run init/shutdown for mec1308.c, ene_lpc.c ...................................................................... Patch Set 10: (1 comment) File tests/init_shutdown.c: https://review.coreboot.org/c/flashrom/+/51487/comment/fefaa940_9eceecb0 PS10, Line 53: MEC1308_SIO_PORT1 > Yes, that's right...but are we trying to check that the header file has the right constant values? To me a more useful test is that they 'come through' or are dealt with as expected. I don't know. FWIW, we have not specified what should be tested to what extend. With wrong numbers, it wouldn't work, I suppose, so seems worth to test them too? > > If the definitions are considered private, they could go in a header file in the same directory as the driver, perhaps with a _private suffix? Then, just the test can include them Yeah, that would avoid any confusion with an API. -- To view, visit https://review.coreboot.org/c/flashrom/+/51487 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I3af612defe1af3850dfc1626a208d873e3a3eddc Gerrit-Change-Number: 51487 Gerrit-PatchSet: 10 Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-CC: Simon Glass <sjg(a)chromium.org> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Attention: Simon Glass <sjg(a)chromium.org> Gerrit-Comment-Date: Tue, 04 May 2021 16:39:35 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de> Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org> Comment-In-Reply-To: Simon Glass <sjg(a)chromium.org> Gerrit-MessageType: comment

1 0

Change in flashrom[master]: tests: Add unit test to run init/shutdown for mec1308.c, ene_lpc.c
by Simon Glass (Code Review) 04 May '21

04 May '21

Attention is currently required from: Nico Huber, Edward O'Callaghan, Angel Pons, Anastasia Klimchuk. Simon Glass has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/51487 ) Change subject: tests: Add unit test to run init/shutdown for mec1308.c, ene_lpc.c ...................................................................... Patch Set 10: (1 comment) File tests/init_shutdown.c: https://review.coreboot.org/c/flashrom/+/51487/comment/44701e00_7dd46aae PS10, Line 53: MEC1308_SIO_PORT1 > It depends much on what one wants to test. Sharing the numbers with the […] Yes, that's right...but are we trying to check that the header file has the right constant values? To me a more useful test is that they 'come through' or are dealt with as expected. If the definitions are considered private, they could go in a header file in the same directory as the driver, perhaps with a _private suffix? Then, just the test can include them -- To view, visit https://review.coreboot.org/c/flashrom/+/51487 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I3af612defe1af3850dfc1626a208d873e3a3eddc Gerrit-Change-Number: 51487 Gerrit-PatchSet: 10 Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-CC: Simon Glass <sjg(a)chromium.org> Gerrit-Attention: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Comment-Date: Tue, 04 May 2021 16:15:26 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Nico Huber <nico.h(a)gmx.de> Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org> Comment-In-Reply-To: Simon Glass <sjg(a)chromium.org> Gerrit-MessageType: comment

1 0

2024

2023

2022

2021

2020

2019

2018

2017

flashrom-gerrit May 2021