Edward O'Callaghan has submitted this change. ( https://review.coreboot.org/c/flashrom/+/69402 )
(
6 is the latest approved patch-set.
No files were changed between the latest approved patch-set and the submitted one.
)Change subject: flashrom_tester: Check the WP state when setting
......................................................................
flashrom_tester: Check the WP state when setting
Check that the hardware and software WP state are as expected in the
setter methods.
BUG=b:244663741
BRANCH=None
TEST=flashrom_tester --libflashrom host
Change-Id: Ie7f90ab478dca6f92eaa0908443e3cb156ea0319
Signed-off-by: Evan Benn <evanbenn(a)chromium.org>
Reviewed-on: https://review.coreboot.org/c/flashrom/+/69402
Reviewed-by: Peter Marheine <pmarheine(a)chromium.org>
Reviewed-by: Edward O'Callaghan <quasisec(a)chromium.org>
Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org>
---
M util/flashrom_tester/src/tester.rs
1 file changed, 46 insertions(+), 9 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/tester.rs b/util/flashrom_tester/src/tester.rs
index c43c7d2..bedccaf 100644
--- a/util/flashrom_tester/src/tester.rs
+++ b/util/flashrom_tester/src/tester.rs
@@ -229,28 +229,44 @@
/// 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);
+ info!("set_sw 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)
+ if Self::get_sw(self.cmd).map_err(|e| e.to_string())? != enable {
+ Err(format!(
+ "Software write protect did not change state to {} when requested",
+ enable
+ ))
+ } else {
+ self.current.sw = enable;
+ Ok(self)
+ }
}
- /// Set the hardware write protect.
+ /// Set the hardware write protect if supported and check that the state is as expected.
pub fn set_hw(&mut self, enable: bool) -> Result<&mut Self, String> {
+ info!("set_hw request={}, current={}", enable, self.current.hw);
if self.can_control_hw_wp() {
if self.current.hw != enable {
super::utils::toggle_hw_wp(/* dis= */ !enable)?;
- self.current.hw = enable;
- } else if enable {
- info!(
- "Ignoring attempt to enable hardware WP with {:?} programmer",
- self.fc
- );
}
+ // toggle_hw_wp does check this, but we might not have called toggle_hw_wp so check again.
+ if Self::get_hw(self.cmd)? != enable {
+ return Err(format!(
+ "Hardware write protect did not change state to {} when requested",
+ enable
+ ));
+ }
+ } else {
+ info!(
+ "Ignoring attempt to set hardware WP with {:?} programmer",
+ self.fc
+ );
}
+ self.current.hw = enable;
Ok(self)
}
--
To view, visit https://review.coreboot.org/c/flashrom/+/69402
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: Ie7f90ab478dca6f92eaa0908443e3cb156ea0319
Gerrit-Change-Number: 69402
Gerrit-PatchSet: 8
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-CC: Nikolai Artemiev <nartemiev(a)chromium.org>
Gerrit-MessageType: merged
Edward O'Callaghan has submitted this change. ( https://review.coreboot.org/c/flashrom/+/69401 )
(
5 is the latest approved patch-set.
No files were changed between the latest approved patch-set and the submitted one.
)Change subject: flashrom_tester: Simplify WriteProtectState
......................................................................
flashrom_tester: Simplify WriteProtectState
Remove the WriteProtectState 'stack' implementation and the push
function. This functionality allowed states to be stacked and then
automatically unrolled via RAII lifetimes. This was useful. However this
unrolling could make errors in a flashrom_tester run much harder to
understand, as the actual failure would be followed by multiple write
protect calls that could subsequently fail, potentially causing panicing
inside the panic handler and the process to be hard aborted and the
restore golden image function would not be run.
The new approach is to prefer code simplicity. Ideally this makes errors
easier to diagnose from logs. To that end the lifetime has been
simplified. The stack has been removed. The mutex has been removed.
This means tests may not be running in the same environment they were
previously. However if they continue to specify their requirements with
set_sw and set_hw there will be no difference and the errors will be
clear.
BUG=b:259494812
BRANCH=None
TEST=flashrom_tester --libflashrom host
Change-Id: I1c4251f69b42a327383b8a99fa933f411feb9568
Signed-off-by: Evan Benn <evanbenn(a)chromium.org>
Reviewed-on: https://review.coreboot.org/c/flashrom/+/69401
Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org>
Reviewed-by: Edward O'Callaghan <quasisec(a)chromium.org>
---
M util/flashrom_tester/src/tester.rs
1 file changed, 71 insertions(+), 187 deletions(-)
Approvals:
build bot (Jenkins): Verified
Edward O'Callaghan: Looks good to me, approved
diff --git a/util/flashrom_tester/src/tester.rs b/util/flashrom_tester/src/tester.rs
index 323b536..c43c7d2 100644
--- a/util/flashrom_tester/src/tester.rs
+++ b/util/flashrom_tester/src/tester.rs
@@ -41,11 +41,9 @@
use serde_json::json;
use std::fs::File;
use std::io::Write;
-use std::mem::MaybeUninit;
use std::path::Path;
use std::path::PathBuf;
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>;
@@ -60,7 +58,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.
original_flash_contents: PathBuf,
/// The path to a file containing flash-sized random data
@@ -174,64 +172,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,
})
@@ -251,9 +218,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.
///
@@ -262,22 +227,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",
@@ -288,151 +254,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: 7
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/+/69400 )
(
4 is the latest approved patch-set.
No files were changed between the latest approved patch-set and the submitted one.
)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=b:258357944
BRANCH=None
TEST=flashrom_tester --libflashrom host Lock
Change-Id: Ibad559a8ff9696fd91f45bca9d9ceb6e90c41393
Signed-off-by: Evan Benn <evanbenn(a)chromium.org>
Reviewed-on: https://review.coreboot.org/c/flashrom/+/69400
Reviewed-by: Peter Marheine <pmarheine(a)chromium.org>
Reviewed-by: Edward O'Callaghan <quasisec(a)chromium.org>
Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org>
---
M util/flashrom_tester/src/tests.rs
1 file changed, 25 insertions(+), 4 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 af34208..5cbe9d2 100644
--- a/util/flashrom_tester/src/tests.rs
+++ b/util/flashrom_tester/src/tests.rs
@@ -199,12 +199,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: 6
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 uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/69959 )
Change subject: block erase WIP
......................................................................
block erase WIP
The idea here is to bring about modularity in the erasure stack
by moving lookups into its own module.
Change-Id: I9b1dc33fb8d3e98f26f7d4cd1fbb876c3c5580a2
Signed-off-by: Edward O'Callaghan <quasisec(a)google.com>
---
M flashrom.c
M include/chipdrivers.h
M spi25.c
3 files changed, 81 insertions(+), 81 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/59/69959/1
diff --git a/flashrom.c b/flashrom.c
index 2489ea3..9057b31 100644
--- a/flashrom.c
+++ b/flashrom.c
@@ -338,57 +338,6 @@
return extract_param(&cfg->params, param_name, ",");
}
-/* special unit-test hook */
-erasefunc_t *g_test_erase_injector;
-
-static erasefunc_t *lookup_erase_func_ptr(const struct block_eraser *const eraser)
-{
- switch (eraser->block_erase) {
- case SPI_BLOCK_ERASE_EMULATION: return &spi_block_erase_emulation;
- case SPI_BLOCK_ERASE_20: return &spi_block_erase_20;
- case SPI_BLOCK_ERASE_21: return &spi_block_erase_21;
- case SPI_BLOCK_ERASE_40: return NULL; // FIXME unhandled &spi_block_erase_40;
- case SPI_BLOCK_ERASE_50: return &spi_block_erase_50;
- case SPI_BLOCK_ERASE_52: return &spi_block_erase_52;
- case SPI_BLOCK_ERASE_53: return &spi_block_erase_53;
- case SPI_BLOCK_ERASE_5C: return &spi_block_erase_5c;
- case SPI_BLOCK_ERASE_60: return &spi_block_erase_60;
- case SPI_BLOCK_ERASE_62: return &spi_block_erase_62;
- case SPI_BLOCK_ERASE_81: return &spi_block_erase_81;
- case SPI_BLOCK_ERASE_C4: return &spi_block_erase_c4;
- case SPI_BLOCK_ERASE_C7: return &spi_block_erase_c7;
- case SPI_BLOCK_ERASE_D7: return &spi_block_erase_d7;
- case SPI_BLOCK_ERASE_D8: return &spi_block_erase_d8;
- case SPI_BLOCK_ERASE_DB: return &spi_block_erase_db;
- case SPI_BLOCK_ERASE_DC: return &spi_block_erase_dc;
- case S25FL_BLOCK_ERASE: return &s25fl_block_erase;
- case S25FS_BLOCK_ERASE_D8: return &s25fs_block_erase_d8;
- case JEDEC_SECTOR_ERASE: return &erase_sector_jedec; // TODO rename to &jedec_sector_erase;
- case JEDEC_BLOCK_ERASE: return &erase_block_jedec; // TODO rename to &jedec_block_erase;
- case JEDEC_CHIP_BLOCK_ERASE: return &erase_chip_block_jedec; // TODO rename to &jedec_chip_block_erase;
- case OPAQUE_ERASE: return &erase_opaque; // TODO rename to &opqaue_erase;
- case SPI_ERASE_AT45CS_SECTOR: return &spi_erase_at45cs_sector;
- case SPI_ERASE_AT45DB_BLOCK: return &spi_erase_at45db_block;
- case SPI_ERASE_AT45DB_CHIP: return &spi_erase_at45db_chip;
- case SPI_ERASE_AT45DB_PAGE: return &spi_erase_at45db_page;
- case SPI_ERASE_AT45DB_SECTOR: return &spi_erase_at45db_sector;
- case ERASE_CHIP_28SF040: return &erase_chip_28sf040;
- case ERASE_SECTOR_28SF040: return &erase_sector_28sf040;
- case ERASE_BLOCK_82802AB: return &erase_block_82802ab;
- case ERASE_SECTOR_49LFXXXC: return &erase_sector_49lfxxxc;
- case STM50_SECTOR_ERASE: return &erase_sector_stm50; // TODO rename to &stm50_sector_erase;
- case EDI_CHIP_BLOCK_ERASE: return &edi_chip_block_erase;
- case TEST_ERASE_INJECTOR: return g_test_erase_injector;
- /* default: total function, 0 indicates no erase function set.
- * We explicitly do not want a default catch-all case in the switch
- * to ensure unhandled enum's are compiler warnings.
- */
- case NO_BLOCK_ERASE_FUNC: return NULL;
- };
-
- return NULL;
-}
-
static int check_block_eraser(const struct flashctx *flash, int k, int log)
{
struct block_eraser eraser = flash->chip->block_erasers[k];
diff --git a/include/chipdrivers.h b/include/chipdrivers.h
index 9c9a94c..c5a0acb 100644
--- a/include/chipdrivers.h
+++ b/include/chipdrivers.h
@@ -22,6 +22,8 @@
#include "flash.h" /* for chipaddr and flashctx */
+erasefunc_t *lookup_erase_func_ptr(const struct block_eraser *const eraser);
+
/* spi.c */
int spi_aai_write(struct flashctx *flash, const uint8_t *buf, unsigned int start, unsigned int len);
int spi_chip_write_256(struct flashctx *flash, const uint8_t *buf, unsigned int start, unsigned int len);
@@ -37,21 +39,6 @@
int probe_spi_at25f(struct flashctx *flash);
int spi_write_enable(struct flashctx *flash);
int spi_write_disable(struct flashctx *flash);
-int spi_block_erase_20(struct flashctx *flash, unsigned int addr, unsigned int blocklen);
-int spi_block_erase_21(struct flashctx *flash, unsigned int addr, unsigned int blocklen);
-int spi_block_erase_50(struct flashctx *flash, unsigned int addr, unsigned int blocklen);
-int spi_block_erase_52(struct flashctx *flash, unsigned int addr, unsigned int blocklen);
-int spi_block_erase_53(struct flashctx *flash, unsigned int addr, unsigned int blocklen);
-int spi_block_erase_5c(struct flashctx *flash, unsigned int addr, unsigned int blocklen);
-int spi_block_erase_60(struct flashctx *flash, unsigned int addr, unsigned int blocklen);
-int spi_block_erase_62(struct flashctx *flash, unsigned int addr, unsigned int blocklen);
-int spi_block_erase_81(struct flashctx *flash, unsigned int addr, unsigned int blocklen);
-int spi_block_erase_c4(struct flashctx *flash, unsigned int addr, unsigned int blocklen);
-int spi_block_erase_c7(struct flashctx *flash, unsigned int addr, unsigned int blocklen);
-int spi_block_erase_d7(struct flashctx *flash, unsigned int addr, unsigned int blocklen);
-int spi_block_erase_d8(struct flashctx *flash, unsigned int addr, unsigned int blocklen);
-int spi_block_erase_db(struct flashctx *flash, unsigned int addr, unsigned int blocklen);
-int spi_block_erase_dc(struct flashctx *flash, unsigned int addr, unsigned int blocklen);
enum block_erase_func spi_get_erasefn_from_opcode(uint8_t opcode);
uint8_t spi_get_opcode_from_erasefn(enum block_erase_func func);
int spi_chip_write_1(struct flashctx *flash, const uint8_t *buf, unsigned int start, unsigned int len);
diff --git a/spi25.c b/spi25.c
index da870be..79ac01f 100644
--- a/spi25.c
+++ b/spi25.c
@@ -492,7 +492,7 @@
return spi_simple_write_cmd(flash, JEDEC_CE_C7, 1000 * 1000);
}
-int spi_block_erase_52(struct flashctx *flash, unsigned int addr,
+static int spi_block_erase_52(struct flashctx *flash, unsigned int addr,
unsigned int blocklen)
{
/* This usually takes 100-4000ms, so wait in 100ms steps. */
@@ -502,7 +502,7 @@
/* Block size is usually
* 32M (one die) for Micron
*/
-int spi_block_erase_c4(struct flashctx *flash, unsigned int addr, unsigned int blocklen)
+static int spi_block_erase_c4(struct flashctx *flash, unsigned int addr, unsigned int blocklen)
{
/* This usually takes 240-480s, so wait in 500ms steps. */
return spi_write_cmd(flash, JEDEC_BE_C4, false, addr, NULL, 0, 500 * 1000);
@@ -513,7 +513,7 @@
* 32k for SST
* 4-32k non-uniform for EON
*/
-int spi_block_erase_d8(struct flashctx *flash, unsigned int addr,
+static int spi_block_erase_d8(struct flashctx *flash, unsigned int addr,
unsigned int blocklen)
{
/* This usually takes 100-4000ms, so wait in 100ms steps. */
@@ -523,7 +523,7 @@
/* Block size is usually
* 4k for PMC
*/
-int spi_block_erase_d7(struct flashctx *flash, unsigned int addr,
+static int spi_block_erase_d7(struct flashctx *flash, unsigned int addr,
unsigned int blocklen)
{
/* This usually takes 100-4000ms, so wait in 100ms steps. */
@@ -531,7 +531,7 @@
}
/* Page erase (usually 256B blocks) */
-int spi_block_erase_db(struct flashctx *flash, unsigned int addr, unsigned int blocklen)
+static int spi_block_erase_db(struct flashctx *flash, unsigned int addr, unsigned int blocklen)
{
/* This takes up to 20ms usually (on worn out devices
up to the 0.5s range), so wait in 1ms steps. */
@@ -539,26 +539,26 @@
}
/* Sector size is usually 4k, though Macronix eliteflash has 64k */
-int spi_block_erase_20(struct flashctx *flash, unsigned int addr,
+static int spi_block_erase_20(struct flashctx *flash, unsigned int addr,
unsigned int blocklen)
{
/* This usually takes 15-800ms, so wait in 10ms steps. */
return spi_write_cmd(flash, JEDEC_SE, false, addr, NULL, 0, 10 * 1000);
}
-int spi_block_erase_50(struct flashctx *flash, unsigned int addr, unsigned int blocklen)
+static int spi_block_erase_50(struct flashctx *flash, unsigned int addr, unsigned int blocklen)
{
/* This usually takes 10ms, so wait in 1ms steps. */
return spi_write_cmd(flash, JEDEC_BE_50, false, addr, NULL, 0, 1 * 1000);
}
-int spi_block_erase_81(struct flashctx *flash, unsigned int addr, unsigned int blocklen)
+static int spi_block_erase_81(struct flashctx *flash, unsigned int addr, unsigned int blocklen)
{
/* This usually takes 8ms, so wait in 1ms steps. */
return spi_write_cmd(flash, JEDEC_BE_81, false, addr, NULL, 0, 1 * 1000);
}
-int spi_block_erase_60(struct flashctx *flash, unsigned int addr,
+static int spi_block_erase_60(struct flashctx *flash, unsigned int addr,
unsigned int blocklen)
{
if ((addr != 0) || (blocklen != flash->chip->total_size * 1024)) {
@@ -569,7 +569,7 @@
return spi_chip_erase_60(flash);
}
-int spi_block_erase_62(struct flashctx *flash, unsigned int addr, unsigned int blocklen)
+static int spi_block_erase_62(struct flashctx *flash, unsigned int addr, unsigned int blocklen)
{
if ((addr != 0) || (blocklen != flash->chip->total_size * 1024)) {
msg_cerr("%s called with incorrect arguments\n",
@@ -579,7 +579,7 @@
return spi_chip_erase_62(flash);
}
-int spi_block_erase_c7(struct flashctx *flash, unsigned int addr,
+static int spi_block_erase_c7(struct flashctx *flash, unsigned int addr,
unsigned int blocklen)
{
if ((addr != 0) || (blocklen != flash->chip->total_size * 1024)) {
@@ -591,33 +591,84 @@
}
/* Erase 4 KB of flash with 4-bytes address from ANY mode (3-bytes or 4-bytes) */
-int spi_block_erase_21(struct flashctx *flash, unsigned int addr, unsigned int blocklen)
+static int spi_block_erase_21(struct flashctx *flash, unsigned int addr, unsigned int blocklen)
{
/* This usually takes 15-800ms, so wait in 10ms steps. */
return spi_write_cmd(flash, 0x21, true, addr, NULL, 0, 10 * 1000);
}
/* Erase 32 KB of flash with 4-bytes address from ANY mode (3-bytes or 4-bytes) */
-int spi_block_erase_53(struct flashctx *flash, unsigned int addr, unsigned int blocklen)
+static int spi_block_erase_53(struct flashctx *flash, unsigned int addr, unsigned int blocklen)
{
/* This usually takes 100-4000ms, so wait in 100ms steps. */
return spi_write_cmd(flash, 0x53, true, addr, NULL, 0, 100 * 1000);
}
/* Erase 32 KB of flash with 4-bytes address from ANY mode (3-bytes or 4-bytes) */
-int spi_block_erase_5c(struct flashctx *flash, unsigned int addr, unsigned int blocklen)
+static int spi_block_erase_5c(struct flashctx *flash, unsigned int addr, unsigned int blocklen)
{
/* This usually takes 100-4000ms, so wait in 100ms steps. */
return spi_write_cmd(flash, 0x5c, true, addr, NULL, 0, 100 * 1000);
}
/* Erase 64 KB of flash with 4-bytes address from ANY mode (3-bytes or 4-bytes) */
-int spi_block_erase_dc(struct flashctx *flash, unsigned int addr, unsigned int blocklen)
+static int spi_block_erase_dc(struct flashctx *flash, unsigned int addr, unsigned int blocklen)
{
/* This usually takes 100-4000ms, so wait in 100ms steps. */
return spi_write_cmd(flash, 0xdc, true, addr, NULL, 0, 100 * 1000);
}
+/* special unit-test hook */
+erasefunc_t *g_test_erase_injector;
+
+erasefunc_t *lookup_erase_func_ptr(const struct block_eraser *const eraser)
+{
+ switch (eraser->block_erase) {
+ case SPI_BLOCK_ERASE_EMULATION: return &spi_block_erase_emulation;
+ case SPI_BLOCK_ERASE_20: return &spi_block_erase_20;
+ case SPI_BLOCK_ERASE_21: return &spi_block_erase_21;
+ case SPI_BLOCK_ERASE_40: return NULL; // FIXME unhandled &spi_block_erase_40;
+ case SPI_BLOCK_ERASE_50: return &spi_block_erase_50;
+ case SPI_BLOCK_ERASE_52: return &spi_block_erase_52;
+ case SPI_BLOCK_ERASE_53: return &spi_block_erase_53;
+ case SPI_BLOCK_ERASE_5C: return &spi_block_erase_5c;
+ case SPI_BLOCK_ERASE_60: return &spi_block_erase_60;
+ case SPI_BLOCK_ERASE_62: return &spi_block_erase_62;
+ case SPI_BLOCK_ERASE_81: return &spi_block_erase_81;
+ case SPI_BLOCK_ERASE_C4: return &spi_block_erase_c4;
+ case SPI_BLOCK_ERASE_C7: return &spi_block_erase_c7;
+ case SPI_BLOCK_ERASE_D7: return &spi_block_erase_d7;
+ case SPI_BLOCK_ERASE_D8: return &spi_block_erase_d8;
+ case SPI_BLOCK_ERASE_DB: return &spi_block_erase_db;
+ case SPI_BLOCK_ERASE_DC: return &spi_block_erase_dc;
+ case S25FL_BLOCK_ERASE: return &s25fl_block_erase;
+ case S25FS_BLOCK_ERASE_D8: return &s25fs_block_erase_d8;
+ case JEDEC_SECTOR_ERASE: return &erase_sector_jedec; // TODO rename to &jedec_sector_erase;
+ case JEDEC_BLOCK_ERASE: return &erase_block_jedec; // TODO rename to &jedec_block_erase;
+ case JEDEC_CHIP_BLOCK_ERASE: return &erase_chip_block_jedec; // TODO rename to &jedec_chip_block_erase;
+ case OPAQUE_ERASE: return &erase_opaque; // TODO rename to &opqaue_erase;
+ case SPI_ERASE_AT45CS_SECTOR: return &spi_erase_at45cs_sector;
+ case SPI_ERASE_AT45DB_BLOCK: return &spi_erase_at45db_block;
+ case SPI_ERASE_AT45DB_CHIP: return &spi_erase_at45db_chip;
+ case SPI_ERASE_AT45DB_PAGE: return &spi_erase_at45db_page;
+ case SPI_ERASE_AT45DB_SECTOR: return &spi_erase_at45db_sector;
+ case ERASE_CHIP_28SF040: return &erase_chip_28sf040;
+ case ERASE_SECTOR_28SF040: return &erase_sector_28sf040;
+ case ERASE_BLOCK_82802AB: return &erase_block_82802ab;
+ case ERASE_SECTOR_49LFXXXC: return &erase_sector_49lfxxxc;
+ case STM50_SECTOR_ERASE: return &erase_sector_stm50; // TODO rename to &stm50_sector_erase;
+ case EDI_CHIP_BLOCK_ERASE: return &edi_chip_block_erase;
+ case TEST_ERASE_INJECTOR: return g_test_erase_injector;
+ /* default: total function, 0 indicates no erase function set.
+ * We explicitly do not want a default catch-all case in the switch
+ * to ensure unhandled enum's are compiler warnings.
+ */
+ case NO_BLOCK_ERASE_FUNC: return NULL;
+ };
+
+ return NULL;
+}
+
static const struct {
enum block_erase_func func;
uint8_t opcode;
--
To view, visit https://review.coreboot.org/c/flashrom/+/69959
To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings
Gerrit-Project: flashrom
Gerrit-Branch: master
Gerrit-Change-Id: I9b1dc33fb8d3e98f26f7d4cd1fbb876c3c5580a2
Gerrit-Change-Number: 69959
Gerrit-PatchSet: 1
Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org>
Gerrit-MessageType: newchange
Attention is currently required from: Thomas Heijligen, Anastasia Klimchuk.
Evan Benn has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/69539 )
Change subject: tests: Redirect to real I/O to support coverage run
......................................................................
Patch Set 2:
(7 comments)
Commit Message:
https://review.coreboot.org/c/flashrom/+/69539/comment/c3076583_9792b85e
PS1, Line 7: Add unmock_io.c
> Redirect to real I/O to support coverage run
Done
https://review.coreboot.org/c/flashrom/+/69539/comment/944192b4_69a6e8fc
PS1, Line 10: This is to support code coverage.
> remove
Done
https://review.coreboot.org/c/flashrom/+/69539/comment/c4dd78db_98d053a8
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. […]
Done
File tests/unmock_io.c:
https://review.coreboot.org/c/flashrom/+/69539/comment/1cbb1b19_183fa8cb
PS1, Line 19: unmock_io
> How about `io_real.h` (and `io_real.c`)? this aligns with `io_mock` that we already have. […]
Done
https://review.coreboot.org/c/flashrom/+/69539/comment/33ec8082_af259f59
PS1, Line 25: (void)state;
> Just drop this line, none other custom mocks are using it. […]
Done
https://review.coreboot.org/c/flashrom/+/69539/comment/f22a7b33_ed8bc255
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
Done
https://review.coreboot.org/c/flashrom/+/69539/comment/b8a490d1_950baab7
PS1, Line 61: maybe_unmock_io
> Let's add a comment for this function: […]
the function is documented in the header
--
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: 2
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: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Comment-Date: Thu, 24 Nov 2022 01:59:29 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-MessageType: comment
Attention is currently required from: Thomas Heijligen, Anastasia Klimchuk.
Evan Benn has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/69266 )
Change subject: tests: Detect gcov run and redirect to real I/O functions
......................................................................
Patch Set 11:
(2 comments)
Commit Message:
https://review.coreboot.org/c/flashrom/+/69266/comment/70c88ad9_c9cd907f
PS10, Line 7: Detect gcov file io and unmock io functions
> Detect gcov run and redirect to real I/O functions
Done
https://review.coreboot.org/c/flashrom/+/69266/comment/c18b1b3d_b6f63753
PS10, Line 9: we need to unmock the file io
> we need to use real I/O functions
Done
--
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: 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: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Comment-Date: Thu, 24 Nov 2022 01:58:50 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-MessageType: comment
Attention is currently required from: Thomas Heijligen, Anastasia Klimchuk.
Evan Benn 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 12:
(4 comments)
Commit Message:
https://review.coreboot.org/c/flashrom/+/69268/comment/a533f182_43fd0e6b
PS11, Line 14: TEST=meson test; ninja llvm-cov-tests
> Can you test scenario when feature disables? when tests disabled? […]
I ran it for all the test_build configs with coverage enabled, anad with and without tests.
Patchset:
PS11:
> I dont mind doing that, but is it necessary? The script is in the repo. […]
Done
File meson.build:
https://review.coreboot.org/c/flashrom/+/69268/comment/2e20ae40_5b7222ce
PS5, Line 534: if get_option('llvm_cov')
> I am confused, but what does it mean, coverage without running tests?
code coverage is an independent concept from unit tests. you can collect code coverage on a run of the CLI. This would be useful for integration tests, or for debugging in general.
File meson.build:
https://review.coreboot.org/c/flashrom/+/69268/comment/5eee5dd1_8810e07e
PS11, Line 614: run_target('llvm-cov-cli', command : ['scripts/llvm-cov', classic_cli])
> This should also be guarded
Done
--
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: 12
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: Anastasia Klimchuk <aklm(a)chromium.org>
Gerrit-Comment-Date: Thu, 24 Nov 2022 01:57:51 +0000
Gerrit-HasComments: Yes
Gerrit-Has-Labels: No
Comment-In-Reply-To: Thomas Heijligen <src(a)posteo.de>
Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org>
Comment-In-Reply-To: Evan Benn <evanbenn(a)google.com>
Gerrit-MessageType: comment
Attention is currently required from: Thomas Heijligen, Evan Benn.
Hello build bot (Jenkins), Thomas Heijligen, Anastasia Klimchuk,
I'd like you to reexamine a change. Please visit
https://review.coreboot.org/c/flashrom/+/69268
to look at the new patch set (#12).
Change subject: tests: Add llvm-cov option and run target for code coverage
......................................................................
tests: Add llvm-cov option and run target for code coverage
Code coverage can be requested with -Dllvm_cov and run with ninja
llvm-cov-tests or llvm-cov-cli.
BUG=b:187647884
BRANCH=None
TEST=meson test; ninja llvm-cov-tests
TEST=ran test_build.sh with coverage enabled
Change-Id: Id6c73bff46e7b88d425956a80def97082b201f56
Signed-off-by: Evan Benn <evanbenn(a)chromium.org>
---
M Documentation/building.md
M meson.build
M meson_options.txt
A scripts/llvm-cov
M tests/meson.build
5 files changed, 43 insertions(+), 9 deletions(-)
git pull ssh://review.coreboot.org:29418/flashrom refs/changes/68/69268/12
--
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: 12
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-MessageType: newpatchset