Attention is currently required from: Thomas Heijligen, Edward O'Callaghan.
Stefan Reinauer has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/72607 )
Change subject: jedec.c: Move printlock stuff into printlock.c
......................................................................
Patch Set 1: Code-Review+2
--
To view, visit https://review.coreboot.org/c/flashrom/+/72607
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: Iacaa16c81e141aac30feb6871700c4fdc9eec8e9
Gerrit-Change-Number: 72607
Gerrit-PatchSet: 1
Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Reviewer: Stefan Reinauer <stefan.reinauer(a)coreboot.org>
Gerrit-Reviewer: Thomas Heijligen <src(a)posteo.de>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Thomas Heijligen <src(a)posteo.de>
Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Comment-Date: Tue, 31 Jan 2023 22:33:52 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
Attention is currently required from: Thomas Heijligen, Edward O'Callaghan.
Stefan Reinauer has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/72607 )
Change subject: jedec.c: Move printlock stuff into printlock.c
......................................................................
Patch Set 1:
(1 comment)
File printlock.c:
https://review.coreboot.org/c/flashrom/+/72607/comment/a0485b4f_3d4d5578
PS1, Line 4: * Copyright (C) 2000 Silicon Integrated System Corporation
: * Copyright (C) 2006 Giampiero Giancipoli <gianci(a)email.it>
: * Copyright (C) 2006 coresystems GmbH <info(a)coresystems.de>
: * Copyright (C) 2007-2012 Carl-Daniel Hailfinger
: * Copyright (C) 2009 Sean Nelson <audiohacked(a)gmail.com>
: * Copyright (C) 2014 Stefan Tauner
Is this list correct for this new file?
--
To view, visit https://review.coreboot.org/c/flashrom/+/72607
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: Iacaa16c81e141aac30feb6871700c4fdc9eec8e9
Gerrit-Change-Number: 72607
Gerrit-PatchSet: 1
Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Reviewer: Stefan Reinauer <stefan.reinauer(a)coreboot.org>
Gerrit-Reviewer: Thomas Heijligen <src(a)posteo.de>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Thomas Heijligen <src(a)posteo.de>
Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Comment-Date: Tue, 31 Jan 2023 22:33:50 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
Attention is currently required from: Edward O'Callaghan.
Stefan Reinauer has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/72502 )
Change subject: jedec.c: Consolidate op,operand sequencing into func
......................................................................
Patch Set 4: Code-Review+2
--
To view, visit https://review.coreboot.org/c/flashrom/+/72502
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I9a1b211b49777f6d7f62a052975c1759e036c8df
Gerrit-Change-Number: 72502
Gerrit-PatchSet: 4
Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Reviewer: Stefan Reinauer <stefan.reinauer(a)coreboot.org>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Comment-Date: Tue, 31 Jan 2023 22:32:19 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
Attention is currently required from: Edward O'Callaghan.
Stefan Reinauer has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/72502 )
Change subject: jedec.c: Consolidate op,operand sequencing into func
......................................................................
Patch Set 4:
(1 comment)
File jedec.c:
https://review.coreboot.org/c/flashrom/+/72502/comment/d8f4376e_554c7968
PS4, Line 325: toggle_ready_jedec_slow(flash, bios);
It seems that toggle_jedec_slow could dereference flash->virtual_memory itself here.
--
To view, visit https://review.coreboot.org/c/flashrom/+/72502
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I9a1b211b49777f6d7f62a052975c1759e036c8df
Gerrit-Change-Number: 72502
Gerrit-PatchSet: 4
Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Reviewer: Stefan Reinauer <stefan.reinauer(a)coreboot.org>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Comment-Date: Tue, 31 Jan 2023 22:32:07 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
Attention is currently required from: Edward O'Callaghan.
Stefan Reinauer has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/72606 )
Change subject: jedec.c: Trivial code style fix
......................................................................
Patch Set 1: Code-Review+2
--
To view, visit https://review.coreboot.org/c/flashrom/+/72606
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I42ebdda07512d0a84a6bd6d0630f96c40f039259
Gerrit-Change-Number: 72606
Gerrit-PatchSet: 1
Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Reviewer: Stefan Reinauer <stefan.reinauer(a)coreboot.org>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Comment-Date: Tue, 31 Jan 2023 19:10:30 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
Attention is currently required from: Edward O'Callaghan.
Stefan Reinauer has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/72504 )
Change subject: jedec.c: Collapse probe_jedec() indirection
......................................................................
Patch Set 1: Code-Review+2
--
To view, visit https://review.coreboot.org/c/flashrom/+/72504
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I57c27bcf25c5d9ce10fb9c74d9be6ab3544ac7ba
Gerrit-Change-Number: 72504
Gerrit-PatchSet: 1
Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Reviewer: Stefan Reinauer <stefan.reinauer(a)coreboot.org>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Comment-Date: Tue, 31 Jan 2023 19:10:08 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
Attention is currently required from: Miklós Márton, Anastasia Klimchuk.
Alexander Goncharov has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/72154 )
Change subject: ni845x_spi: pass global state through func params
......................................................................
Patch Set 2:
(1 comment)
Patchset:
PS2:
This programmer requires a special environment to run, and unfortunately, it only works on Windows. Miklós, could I ask you to compile the code of this chain and test it on the hardware to make sure that everything is OK?
--
To view, visit https://review.coreboot.org/c/flashrom/+/72154
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I5daeb0839a4cc18b82d38cc06eeba88a619bec61
Gerrit-Change-Number: 72154
Gerrit-PatchSet: 2
Gerrit-Owner: Alexander Goncharov <chat(a)joursoir.net>
Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Reviewer: Miklós Márton <martonmiklosqdev(a)gmail.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-Attention: Miklós Márton <martonmiklosqdev(a)gmail.com>
Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Comment-Date: Tue, 31 Jan 2023 07:23:18 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Gerrit-MessageType: comment
Edward O'Callaghan has submitted this change. ( https://review.coreboot.org/c/flashrom/+/71971 )
Change subject: flashrom_tester: Simplify wp_toggle_test and rename
......................................................................
flashrom_tester: Simplify wp_toggle_test and rename
Rename wp_toggle_test to wp_region_list_test. Make it test one thing,
wp_list. lock_test does the test of set_hw/set_sw functionality so that
was redundant here.
BUG=b:235916336
BRANCH=None
TEST=None
Change-Id: I314aa8d9708c39cc162a8d5e95ca3e019c2fb5b8
Signed-off-by: Evan Benn <evanbenn(a)chromium.org>
Reviewed-on: https://review.coreboot.org/c/flashrom/+/71971
Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org>
Reviewed-by: Peter Marheine <pmarheine(a)chromium.org>
Reviewed-by: Edward O'Callaghan <quasisec(a)chromium.org>
---
M util/flashrom_tester/src/tests.rs
1 file changed, 24 insertions(+), 5 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 1f65b04..8369a94 100644
--- a/util/flashrom_tester/src/tests.rs
+++ b/util/flashrom_tester/src/tests.rs
@@ -97,7 +97,7 @@
&("Get_device_name", get_device_name_test),
&("Coreboot_ELOG_sanity", elog_sanity_test),
&("Host_is_ChromeOS", host_is_chrome_test),
- &("Toggle_WP", wp_toggle_test),
+ &("WP_Region_List", wp_region_list_test),
&("Erase_and_Write", erase_write_test),
&("Fail_to_verify", verify_fail_test),
&("Lock", lock_test),
@@ -160,14 +160,11 @@
/// List the write-protectable regions of flash.
/// NOTE: This is not strictly a 'test' as it is allowed to fail on some platforms.
/// However, we will warn when it does fail.
-fn wp_toggle_test(env: &mut TestEnv) -> TestResult {
+fn wp_region_list_test(env: &mut TestEnv) -> TestResult {
match env.cmd.wp_list() {
Ok(list_str) => info!("\n{}", list_str),
Err(e) => warn!("{}", e),
};
- // Fails if unable to set either one
- env.wp.set_hw(false)?;
- env.wp.set_sw(false)?;
Ok(())
}
--
To view, visit https://review.coreboot.org/c/flashrom/+/71971
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I314aa8d9708c39cc162a8d5e95ca3e019c2fb5b8
Gerrit-Change-Number: 71971
Gerrit-PatchSet: 2
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
Edward O'Callaghan has submitted this change. ( https://review.coreboot.org/c/flashrom/+/71970 )
Change subject: flashrom_tester: Add a description of each test
......................................................................
flashrom_tester: Add a description of each test
BUG=b:235916336
BRANCH=None
TEST=cargo doc --open --no-deps --document-private-items
Change-Id: Iacd23e5ac4635eee19f99d849c6e58c5a193f904
Signed-off-by: Evan Benn <evanbenn(a)chromium.org>
Reviewed-on: https://review.coreboot.org/c/flashrom/+/71970
Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org>
Reviewed-by: Peter Marheine <pmarheine(a)chromium.org>
Reviewed-by: Edward O'Callaghan <quasisec(a)chromium.org>
---
M util/flashrom_tester/src/tests.rs
1 file changed, 31 insertions(+), 7 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 3e39e4d..1f65b04 100644
--- a/util/flashrom_tester/src/tests.rs
+++ b/util/flashrom_tester/src/tests.rs
@@ -150,16 +150,17 @@
Ok(())
}
+/// Query the programmer and chip name.
+/// Success means we got something back, which is good enough.
fn get_device_name_test(env: &mut TestEnv) -> TestResult {
- // Success means we got something back, which is good enough.
env.cmd.name()?;
Ok(())
}
+/// List the write-protectable regions of flash.
+/// NOTE: This is not strictly a 'test' as it is allowed to fail on some platforms.
+/// However, we will warn when it does fail.
fn wp_toggle_test(env: &mut TestEnv) -> TestResult {
- // NOTE: This is not strictly a 'test' as it is allowed to fail on some platforms.
- // However, we will warn when it does fail.
- // List the write-protected regions of flash.
match env.cmd.wp_list() {
Ok(list_str) => info!("\n{}", list_str),
Err(e) => warn!("{}", e),
@@ -170,6 +171,7 @@
Ok(())
}
+/// Verify that enabling hardware and software write protect prevents chip erase.
fn erase_write_test(env: &mut TestEnv) -> TestResult {
if !env.is_golden() {
info!("Memory has been modified; reflashing to ensure erasure can be detected");
@@ -196,6 +198,7 @@
Ok(())
}
+/// Verify that enabling hardware write protect prevents disabling software write protect.
fn lock_test(env: &mut TestEnv) -> TestResult {
if !env.wp.can_control_hw_wp() {
return Err("Lock test requires ability to control hardware write protect".into());
@@ -214,10 +217,10 @@
Ok(())
}
+/// Check that the elog contains *something*, as an indication that Coreboot
+/// is actually able to write to the Flash. This only makes sense for chips
+/// running Coreboot, which we assume is just host.
fn elog_sanity_test(env: &mut TestEnv) -> TestResult {
- // Check that the elog contains *something*, as an indication that Coreboot
- // is actually able to write to the Flash. This only makes sense for chips
- // running Coreboot, which we assume is just host.
if env.chip_type() != FlashChip::HOST {
info!("Skipping ELOG sanity check for non-host chip");
return Ok(());
@@ -241,6 +244,7 @@
Ok(())
}
+/// Check that we are running ChromiumOS.
fn host_is_chrome_test(_env: &mut TestEnv) -> TestResult {
let release_info = if let Ok(f) = File::open("/etc/os-release") {
let buf = std::io::BufReader::new(f);
@@ -266,6 +270,7 @@
}
}
+/// Verify that software write protect for a range protects only the requested range.
fn partial_lock_test(section: LayoutNames) -> impl Fn(&mut TestEnv) -> TestResult {
move |env: &mut TestEnv| {
// Need a clean image for verification
@@ -310,6 +315,7 @@
}
}
+/// Check that flashrom 'verify' will fail if the provided data does not match the chip data.
fn verify_fail_test(env: &mut TestEnv) -> TestResult {
// Comparing the flash contents to random data says they're not the same.
match env.verify(env.random_data_file()) {
--
To view, visit https://review.coreboot.org/c/flashrom/+/71970
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: Iacd23e5ac4635eee19f99d849c6e58c5a193f904
Gerrit-Change-Number: 71970
Gerrit-PatchSet: 2
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