Attention is currently required from: Felix Singer, Edward O'Callaghan.
Felix Singer has uploaded a new patch set (#3) to the change originally created by Edward O'Callaghan. ( https://review.coreboot.org/c/flashrom/+/68776 )
Change subject: tree/: Rename ERROR_FATAL to ERROR_FLASHROM_FATAL
......................................................................
tree/: Rename ERROR_FATAL to ERROR_FLASHROM_FATAL
Change-Id: I51ee789f9a1443bfff1e3c85c9b40b5023db6062
Signed-off-by: Edward O'Callaghan <quasisec(a)google.com>
---
M atavia.c
M bitbang_spi.c
M chipset_enable.c
M flashrom.c
M ichspi.c
M include/flash.h
M internal.c
M sb600spi.c
M tests/dummyflasher.c
9 files changed, 53 insertions(+), 43 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/76/68776/3
--
To view, visit https://review.coreboot.org/c/flashrom/+/68776
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I51ee789f9a1443bfff1e3c85c9b40b5023db6062
Gerrit-Change-Number: 68776
Gerrit-PatchSet: 3
Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.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: Felix Singer <felixsinger(a)posteo.net>
Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-MessageType: newpatchset
Attention is currently required from: Edward O'Callaghan.
Felix Singer has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/68777 )
Change subject: tree/: Rename ERROR_NONFATAL to ERROR_FLASHROM_NONFATAL
......................................................................
Patch Set 2: Code-Review+2
--
To view, visit https://review.coreboot.org/c/flashrom/+/68777
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I5c30fec0cebab2b7d10e2789761889abc3a14dd3
Gerrit-Change-Number: 68777
Gerrit-PatchSet: 2
Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.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: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Comment-Date: Wed, 09 Nov 2022 14:41:41 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
Attention is currently required from: Edward O'Callaghan.
Felix Singer has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/68776 )
Change subject: tree/: Rename ERROR_FATAL to ERROR_FLASHROM_FATAL
......................................................................
Patch Set 2: Code-Review+2
--
To view, visit https://review.coreboot.org/c/flashrom/+/68776
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I51ee789f9a1443bfff1e3c85c9b40b5023db6062
Gerrit-Change-Number: 68776
Gerrit-PatchSet: 2
Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.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: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Comment-Date: Wed, 09 Nov 2022 14:41:33 +0000
Gerrit-HasComments: No
Gerrit-Has-Labels: Yes
Gerrit-MessageType: comment
Evan Benn has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/69401 )
Change subject: flashrom_tester: Simplify WriteProtectState
......................................................................
flashrom_tester: Simplify WriteProtectState
We are not using push() anywhere so remove that. Drop the Mutex, rely on
callers not doing it wrong.
BUG=None
BRANCH=None
TEST=None
Change-Id: I1c4251f69b42a327383b8a99fa933f411feb9568
---
M util/flashrom_tester/src/tester.rs
1 file changed, 53 insertions(+), 187 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/01/69401/1
diff --git a/util/flashrom_tester/src/tester.rs b/util/flashrom_tester/src/tester.rs
index f2c3846..49a663c 100644
--- a/util/flashrom_tester/src/tester.rs
+++ b/util/flashrom_tester/src/tester.rs
@@ -39,9 +39,7 @@
use flashrom::FlashromError;
use flashrom::{FlashChip, Flashrom};
use serde_json::json;
-use std::mem::MaybeUninit;
use std::sync::atomic::{AtomicBool, Ordering};
-use std::sync::Mutex;
// type-signature comes from the return type of lib.rs workers.
type TestError = Box<dyn std::error::Error>;
@@ -56,7 +54,7 @@
pub cmd: &'a dyn Flashrom,
layout: LayoutSizes,
- pub wp: WriteProtectState<'a, 'static>,
+ pub wp: WriteProtectState<'a>,
/// The path to a file containing the flash contents at test start.
// TODO(pmarheine) migrate this to a PathBuf for clarity
original_flash_contents: String,
@@ -165,64 +163,33 @@
}
}
+struct WriteProtect {
+ hw: bool,
+ sw: bool,
+}
+
/// RAII handle for setting write protect in either hardware or software.
///
/// Given an instance, the state of either write protect can be modified by calling
-/// `set` or `push`. When it goes out of scope, the write protects will be returned
+/// `set`. When it goes out of scope, the write protects will be returned
/// to the state they had then it was created.
-///
-/// The lifetime `'p` on this struct is the parent state it derives from; `'static`
-/// implies it is derived from hardware, while anything else is part of a stack
-/// created by `push`ing states. An initial state is always static, and the stack
-/// forms a lifetime chain `'static -> 'p -> 'p1 -> ... -> 'pn`.
-pub struct WriteProtectState<'a, 'p> {
- /// The parent state this derives from.
- ///
- /// If it's a root (gotten via `from_hardware`), then this is Hardware and the
- /// liveness flag will be reset on drop.
- initial: InitialState<'p>,
- // Tuples are (hardware, software)
- current: (bool, bool),
+pub struct WriteProtectState<'a> {
+ current: WriteProtect,
+ initial: WriteProtect,
cmd: &'a dyn Flashrom,
fc: FlashChip,
}
-enum InitialState<'p> {
- Hardware(bool, bool),
- Previous(&'p WriteProtectState<'p, 'p>),
-}
-
-impl InitialState<'_> {
- fn get_target(&self) -> (bool, bool) {
- match self {
- InitialState::Hardware(hw, sw) => (*hw, *sw),
- InitialState::Previous(s) => s.current,
- }
- }
-}
-
-impl<'a> WriteProtectState<'a, 'static> {
+impl<'a> WriteProtectState<'a> {
/// Initialize a state from the current state of the hardware.
- ///
- /// Panics if there is already a live state derived from hardware. In such a situation the
- /// new state must be derived from the live one, or the live one must be dropped first.
pub fn from_hardware(cmd: &'a dyn Flashrom, fc: FlashChip) -> Result<Self, FlashromError> {
- let mut lock = Self::get_liveness_lock()
- .lock()
- .expect("Somebody panicked during WriteProtectState init from hardware");
- if *lock {
- drop(lock); // Don't poison the lock
- panic!("Attempted to create a new WriteProtectState when one is already live");
- }
-
let hw = Self::get_hw(cmd)?;
let sw = Self::get_sw(cmd)?;
- info!("Initial hardware write protect: HW={} SW={}", hw, sw);
+ info!("Initial write protect state: HW={} SW={}", hw, sw);
- *lock = true;
Ok(WriteProtectState {
- initial: InitialState::Hardware(hw, sw),
- current: (hw, sw),
+ current: WriteProtect { hw, sw },
+ initial: WriteProtect { hw, sw },
cmd,
fc,
})
@@ -242,9 +209,7 @@
let b = cmd.wp_status(true)?;
Ok(b)
}
-}
-impl<'a, 'p> WriteProtectState<'a, 'p> {
/// Return true if the current programmer supports setting the hardware
/// write protect.
///
@@ -253,22 +218,23 @@
self.cmd.can_control_hw_wp()
}
- /// Set the software write protect.
- pub fn set_sw(&mut self, enable: bool) -> Result<&mut Self, FlashromError> {
- info!("request={}, current={}", enable, self.current.1);
- if self.current.1 != enable {
- self.cmd.wp_toggle(/* en= */ enable)?;
- self.current.1 = enable;
+ /// Set the software write protect and check that the state is as expected.
+ pub fn set_sw(&mut self, enable: bool) -> Result<&mut Self, String> {
+ info!("request={}, current={}", enable, self.current.sw);
+ if self.current.sw != enable {
+ self.cmd
+ .wp_toggle(/* en= */ enable)
+ .map_err(|e| e.to_string())?;
}
Ok(self)
}
/// Set the hardware write protect.
pub fn set_hw(&mut self, enable: bool) -> Result<&mut Self, String> {
- if self.current.0 != enable {
- if self.can_control_hw_wp() {
+ if self.can_control_hw_wp() {
+ if self.current.hw != enable {
super::utils::toggle_hw_wp(/* dis= */ !enable)?;
- self.current.0 = enable;
+ self.current.hw = enable;
} else if enable {
info!(
"Ignoring attempt to enable hardware WP with {:?} programmer",
@@ -279,151 +245,35 @@
Ok(self)
}
- /// Stack a new write protect state on top of the current one.
- ///
- /// This is useful if you need to temporarily make a change to write protection:
- ///
- /// ```no_run
- /// # fn main() -> Result<(), Box<dyn std::error::Error>> {
- /// # let cmd: flashrom::FlashromCmd = unimplemented!();
- /// let wp = flashrom_tester::tester::WriteProtectState::from_hardware(&cmd, flashrom::FlashChip::SERVO)?;
- /// {
- /// let mut wp = wp.push();
- /// wp.set_sw(false)?;
- /// // Do something with software write protect disabled
- /// }
- /// // Now software write protect returns to its original state, even if
- /// // set_sw() failed.
- /// # Ok(())
- /// # }
- /// ```
- ///
- /// This returns a new state which restores the original when it is dropped- the new state
- /// refers to the old, so the compiler enforces that states are disposed of in the reverse
- /// order of their creation and correctly restore the original state.
- pub fn push<'p1>(&'p1 self) -> WriteProtectState<'a, 'p1> {
- WriteProtectState {
- initial: InitialState::Previous(self),
- current: self.current,
- cmd: self.cmd,
- fc: self.fc,
- }
- }
-
- fn get_liveness_lock() -> &'static Mutex<bool> {
- static INIT: std::sync::Once = std::sync::Once::new();
- /// Value becomes true when there is a live WriteProtectState derived `from_hardware`,
- /// blocking duplicate initialization.
- ///
- /// This is required because hardware access is not synchronized; it's possible to leave the
- /// hardware in an unintended state by creating a state handle from it, modifying the state,
- /// creating another handle from the hardware then dropping the first handle- then on drop
- /// of the second handle it will restore the state to the modified one rather than the initial.
- ///
- /// This flag ensures that a duplicate root state cannot be created.
- ///
- /// This is a Mutex<bool> rather than AtomicBool because acquiring the flag needs to perform
- /// several operations that may themselves fail- acquisitions must be fully synchronized.
- static mut LIVE_FROM_HARDWARE: MaybeUninit<Mutex<bool>> = MaybeUninit::uninit();
-
- unsafe {
- INIT.call_once(|| {
- LIVE_FROM_HARDWARE.as_mut_ptr().write(Mutex::new(false));
- });
- &*LIVE_FROM_HARDWARE.as_ptr()
- }
- }
-
/// Reset the hardware to what it was when this state was created, reporting errors.
///
/// This behaves exactly like allowing a state to go out of scope, but it can return
/// errors from that process rather than panicking.
pub fn close(mut self) -> Result<(), String> {
- unsafe {
- let out = self.drop_internal();
- // We just ran drop, don't do it again
- std::mem::forget(self);
- out
- }
+ let out = self.drop_internal();
+ // We just ran drop, don't do it again
+ std::mem::forget(self);
+ out
}
- /// Internal Drop impl.
- ///
- /// This is unsafe because it effectively consumes self when clearing the
- /// liveness lock. Callers must be able to guarantee that self will be forgotten
- /// if the state was constructed from hardware in order to uphold the liveness
- /// invariant (that only a single state constructed from hardware exists at any
- /// time).
- unsafe fn drop_internal(&mut self) -> Result<(), String> {
- let lock = match self.initial {
- InitialState::Hardware(_, _) => Some(
- Self::get_liveness_lock()
- .lock()
- .expect("Somebody panicked during WriteProtectState drop from hardware"),
- ),
- _ => None,
- };
- let (hw, sw) = self.initial.get_target();
-
- fn enable_str(enable: bool) -> &'static str {
- if enable {
- "en"
- } else {
- "dis"
- }
- }
-
+ /// Sets both write protects to the state they had when this state was created.
+ fn drop_internal(&mut self) -> Result<(), String> {
// Toggle both protects back to their initial states.
// Software first because we can't change it once hardware is enabled.
- if sw != self.current.1 {
- // Is the hw wp currently enabled?
- if self.current.0 {
- super::utils::toggle_hw_wp(/* dis= */ true).map_err(|e| {
- format!(
- "Failed to {}able hardware write protect: {}",
- enable_str(false),
- e
- )
- })?;
- }
- self.cmd.wp_toggle(/* en= */ sw).map_err(|e| {
- format!(
- "Failed to {}able software write protect: {}",
- enable_str(sw),
- e
- )
- })?;
+ if self.set_sw(self.initial.sw).is_err() {
+ self.set_hw(false)?;
+ self.set_sw(self.initial.sw)?;
}
+ self.set_hw(self.initial.hw)?;
- assert!(
- self.cmd.can_control_hw_wp() || (!self.current.0 && !hw),
- "HW WP must be disabled if it cannot be controlled"
- );
- if hw != self.current.0 {
- super::utils::toggle_hw_wp(/* dis= */ !hw).map_err(|e| {
- format!(
- "Failed to {}able hardware write protect: {}",
- enable_str(hw),
- e
- )
- })?;
- }
-
- if let Some(mut lock) = lock {
- // Initial state was constructed via from_hardware, now we can clear the liveness
- // lock since reset is complete.
- *lock = false;
- }
Ok(())
}
}
-impl<'a, 'p> Drop for WriteProtectState<'a, 'p> {
- /// Sets both write protects to the state they had when this state was created.
- ///
- /// Panics on error because there is no mechanism to report errors in Drop.
+impl<'a> Drop for WriteProtectState<'a> {
fn drop(&mut self) {
- unsafe { self.drop_internal() }.expect("Error while dropping WriteProtectState")
+ self.drop_internal()
+ .expect("Error while dropping WriteProtectState")
}
}
--
To view, visit https://review.coreboot.org/c/flashrom/+/69401
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I1c4251f69b42a327383b8a99fa933f411feb9568
Gerrit-Change-Number: 69401
Gerrit-PatchSet: 1
Gerrit-Owner: Evan Benn <evanbenn(a)google.com>
Gerrit-MessageType: newchange
Evan Benn has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/69400 )
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=None
BRANCH=None
TEST=None
Change-Id: Ibad559a8ff9696fd91f45bca9d9ceb6e90c41393
---
M util/flashrom_tester/src/tests.rs
1 file changed, 20 insertions(+), 4 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/00/69400/1
diff --git a/util/flashrom_tester/src/tests.rs b/util/flashrom_tester/src/tests.rs
index 1527a6e..6f2bf1e 100644
--- a/util/flashrom_tester/src/tests.rs
+++ b/util/flashrom_tester/src/tests.rs
@@ -217,12 +217,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: 1
Gerrit-Owner: Evan Benn <evanbenn(a)google.com>
Gerrit-MessageType: newchange
Attention is currently required from: Edward O'Callaghan.
Aarya has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/67473 )
Change subject: flashrom.c: Plumb 'all_skipped' global state into func param
......................................................................
Patch Set 6:
(1 comment)
Commit Message:
https://review.coreboot.org/c/flashrom/+/67473/comment/b066999e_3c9add7b
PS1, Line 12: Change-Id: Ic7990b0afebd49d2a27e89b03638b64e36c329df
: Signed-off-by: Aarya Chaumal <aarya.chaumal(a)gmail.com>
> keep these: […]
Done
--
To view, visit https://review.coreboot.org/c/flashrom/+/67473
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: Ic7990b0afebd49d2a27e89b03638b64e36c329df
Gerrit-Change-Number: 67473
Gerrit-PatchSet: 6
Gerrit-Owner: Aarya <aarya.chaumal(a)gmail.com>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
Gerrit-CC: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Comment-Date: Wed, 09 Nov 2022 05:48:33 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-MessageType: comment
Attention is currently required from: Felix Singer, Thomas Heijligen, Edward O'Callaghan, Angel Pons.
Aarya has uploaded a new patch set (#15) to the change originally created by Edward O'Callaghan. ( https://review.coreboot.org/c/flashrom/+/67093 )
Change subject: flashrom.c: Plumb 'all_skipped' global state into func param
......................................................................
flashrom.c: Plumb 'all_skipped' global state into func param
The 'all_skipped' global state can be made into a function
parameter if one just follows though the CFG.
Change-Id: I2346c869c47b48604360b0facf9313aae086c8dd
Signed-off-by: Edward O'Callaghan <quasisec(a)google.com>
Co-authored-by: Aarya Chaumal <aarya.chaumal(a)gmail.com>
---
M flashrom.c
1 file changed, 25 insertions(+), 8 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/93/67093/15
--
To view, visit https://review.coreboot.org/c/flashrom/+/67093
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I2346c869c47b48604360b0facf9313aae086c8dd
Gerrit-Change-Number: 67093
Gerrit-PatchSet: 15
Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-Reviewer: Aarya <aarya.chaumal(a)gmail.com>
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: Nikolai Artemiev <nartemiev(a)google.com>
Gerrit-Reviewer: Thomas Heijligen <src(a)posteo.de>
Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org>
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: Angel Pons <th3fanbus(a)gmail.com>
Gerrit-MessageType: newpatchset