Evan Benn has uploaded this change for review.
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 change 69401. To unsubscribe, or for help writing mail filters, visit settings.