October 2022 - flashrom-gerrit

[S] Change in flashrom[master]: cli_classic.c: Make count_max_decode_exceedings() pure
by Edward O'Callaghan (Code Review) 21 Oct '22

21 Oct '22

Attention is currently required from: Edward O'Callaghan. Hello build bot (Jenkins), Angel Pons, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/flashrom/+/68479 to look at the new patch set (#3). Change subject: cli_classic.c: Make count_max_decode_exceedings() pure ...................................................................... cli_classic.c: Make count_max_decode_exceedings() pure Pass by argument the max_rom_decode structure such that the function is pure and defined upon its parameters. Note, unfortunately a itermediate step of a '_' suffix is required for the 'max_rom_decode' parameter as to not alias the global symbol within the function body. Change-Id: Ia01f77993deab68e251850008e885828e55b9462 Signed-off-by: Edward O'Callaghan <quasisec(a)google.com> --- M cli_classic.c 1 file changed, 28 insertions(+), 10 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/79/68479/3 -- To view, visit https://review.coreboot.org/c/flashrom/+/68479 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ia01f77993deab68e251850008e885828e55b9462 Gerrit-Change-Number: 68479 Gerrit-PatchSet: 3 Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-MessageType: newpatchset

1 0

[M] Change in flashrom[master]: cli_classic.c: Factor out rom bus limits exceeded validation
by Edward O'Callaghan (Code Review) 21 Oct '22

21 Oct '22

Edward O'Callaghan has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/68661 ) Change subject: cli_classic.c: Factor out rom bus limits exceeded validation ...................................................................... cli_classic.c: Factor out rom bus limits exceeded validation Cave out relevant logic relating to validating the rom decode limits fit within the common buses with certain flash-programmer pairingings into it's own function. Move the comment to the top of the new symbol while we are here and rewrite the grammar to be a bit more understandable. Change-Id: Iba1c3c8ab2edc094ede59156ae5e846900fc0777 Signed-off-by: Edward O'Callaghan <quasisec(a)google.com> --- M cli_classic.c 1 file changed, 43 insertions(+), 12 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/61/68661/1 diff --git a/cli_classic.c b/cli_classic.c index 4d327f4..19551b5 100644 --- a/cli_classic.c +++ b/cli_classic.c @@ -560,6 +560,31 @@ return limitexceeded; } +/** + * Sometimes chip and programmer combinations share more than one bus in common. + * If the rom decode limit is exceed on all relevent buses then return true unless + * overriden with a force flag, otherwise return false. + */ +static bool max_rom_decode_limitexceeded(const struct flashctx *flash, bool force) +{ + const unsigned int limitexceeded = count_max_decode_exceedings(fill_flash, &max_rom_decode); + + if (limitexceeded > 0 && !force) { + enum chipbustype commonbuses = fill_flash->mst->buses_supported & fill_flash->chip->bustype; + + if ((bitcount(commonbuses) > limitexceeded)) { + msg_pdbg("There is at least one interface available which could support the size of\n" + "the selected flash chip.\n"); + } + msg_cerr("This flash chip is too big for this programmer (--verbose/-V gives details).\n" + "Use --force/-f to override at your own risk.\n"); + + return true; + } + + return false; +} + int main(int argc, char *argv[]) { const struct flashchip *chip = NULL; @@ -1058,18 +1083,7 @@ print_chip_support_status(fill_flash->chip); - unsigned int limitexceeded = count_max_decode_exceedings(fill_flash, &max_rom_decode); - if (limitexceeded > 0 && !force) { - enum chipbustype commonbuses = fill_flash->mst->buses_supported & fill_flash->chip->bustype; - - /* Sometimes chip and programmer have more than one bus in common, - * and the limit is not exceeded on all buses. Tell the user. */ - if ((bitcount(commonbuses) > limitexceeded)) { - msg_pdbg("There is at least one interface available which could support the size of\n" - "the selected flash chip.\n"); - } - msg_cerr("This flash chip is too big for this programmer (--verbose/-V gives details).\n" - "Use --force/-f to override at your own risk.\n"); + if (max_rom_decode_limitexceeded(fill_flash, force)) { ret = 1; goto out_shutdown; } -- To view, visit https://review.coreboot.org/c/flashrom/+/68661 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Iba1c3c8ab2edc094ede59156ae5e846900fc0777 Gerrit-Change-Number: 68661 Gerrit-PatchSet: 1 Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-MessageType: newchange

1 0

[S] Change in flashrom[master]: cli_classic.c: Make count_max_decode_exceedings() pure
by Edward O'Callaghan (Code Review) 21 Oct '22

21 Oct '22

Attention is currently required from: Edward O'Callaghan. Hello build bot (Jenkins), Angel Pons, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/flashrom/+/68479 to look at the new patch set (#2). Change subject: cli_classic.c: Make count_max_decode_exceedings() pure ...................................................................... cli_classic.c: Make count_max_decode_exceedings() pure Pass by argument the max_rom_decode structure such that the function is pure and defined upon it's parameters. Note, unfortunately a itermediate step of a '_' suffix is required for the 'max_rom_decode' parameter as to not alias the global symbol within the function body. Change-Id: Ia01f77993deab68e251850008e885828e55b9462 Signed-off-by: Edward O'Callaghan <quasisec(a)google.com> --- M cli_classic.c 1 file changed, 28 insertions(+), 10 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/79/68479/2 -- To view, visit https://review.coreboot.org/c/flashrom/+/68479 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ia01f77993deab68e251850008e885828e55b9462 Gerrit-Change-Number: 68479 Gerrit-PatchSet: 2 Gerrit-Owner: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-MessageType: newpatchset

1 0

[M] Change in flashrom[master]: rayer_spi.c: Roll up programmer type search logic into func
by Edward O'Callaghan (Code Review) 21 Oct '22

21 Oct '22

Attention is currently required from: Felix Singer, Angel Pons. Edward O'Callaghan has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/68238 ) Change subject: rayer_spi.c: Roll up programmer type search logic into func ...................................................................... Patch Set 4: (1 comment) File rayer_spi.c: https://review.coreboot.org/c/flashrom/+/68238/comment/a6db5cc2_1b056f86 PS2, Line 276: const struct rayer_programmer rayer_spi_types[] = { > Good question. […] Resolved? -- To view, visit https://review.coreboot.org/c/flashrom/+/68238 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Id226ea61132ecc30fd8696e1d8ea50373e752cac Gerrit-Change-Number: 68238 Gerrit-PatchSet: 4 Gerrit-Owner: Edward O'Callaghan <quasisec(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: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Comment-Date: Fri, 21 Oct 2022 04:28:31 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Edward O'Callaghan <quasisec(a)chromium.org> Comment-In-Reply-To: Angel Pons <th3fanbus(a)gmail.com> Gerrit-MessageType: comment

1 0

[M] Change in flashrom[master]: flashrom_tester: Use Path type and tempdir for test data
by Evan Benn (Code Review) 21 Oct '22

21 Oct '22

Evan Benn has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/68643 ) Change subject: flashrom_tester: Use Path type and tempdir for test data ...................................................................... flashrom_tester: Use Path type and tempdir for test data Use tempdir crate to create a tempdir that is deleted when the tester closes. Update many str to Path. BUG=b:243460685 BRANCH=None TEST=/usr/bin/flashrom_tester --flashrom_binary /usr/sbin/flashrom host Change-Id: I4a0b25c8c70fa6dfd682fba3db7d3bf06b2eb7ac Signed-off-by: Evan Benn <evanbenn(a)chromium.org> --- M util/flashrom_tester/Cargo.toml M util/flashrom_tester/flashrom/src/cmd.rs M util/flashrom_tester/flashrom/src/flashromlib.rs M util/flashrom_tester/flashrom/src/lib.rs M util/flashrom_tester/src/rand_util.rs M util/flashrom_tester/src/tester.rs M util/flashrom_tester/src/tests.rs 7 files changed, 140 insertions(+), 94 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/43/68643/1 diff --git a/util/flashrom_tester/Cargo.toml b/util/flashrom_tester/Cargo.toml index b57f04e..364a223 100644 --- a/util/flashrom_tester/Cargo.toml +++ b/util/flashrom_tester/Cargo.toml @@ -23,6 +23,7 @@ rand = "0.6.4" serde_json = "1" sys-info = "0.9" +tempdir = "0.3.7" [build-dependencies] built = { version = "0.5", features = ["chrono"] } diff --git a/util/flashrom_tester/flashrom/src/cmd.rs b/util/flashrom_tester/flashrom/src/cmd.rs index faf597d..4078910 100644 --- a/util/flashrom_tester/flashrom/src/cmd.rs +++ b/util/flashrom_tester/flashrom/src/cmd.rs @@ -35,15 +35,19 @@ use crate::{FlashChip, FlashromError, ROMWriteSpecifics}; -use std::process::Command; +use std::{ + ffi::{OsStr, OsString}, + path::Path, + process::Command, +}; #[derive(Default)] pub struct FlashromOpt<'a> { pub wp_opt: WPOpt, pub io_opt: IOOpt<'a>, - pub layout: Option<&'a str>, // -l <file> - pub image: Option<&'a str>, // -i <name> + pub layout: Option<&'a Path>, // -l <file> + pub image: Option<&'a str>, // -i <name> pub flash_name: bool, // --flash-name pub verbose: bool, // -V @@ -60,11 +64,11 @@ #[derive(Default)] pub struct IOOpt<'a> { - pub read: Option<&'a str>, // -r <file> - pub write: Option<&'a str>, // -w <file> - pub verify: Option<&'a str>, // -v <file> - pub erase: bool, // -E - pub region: Option<(&'a str, &'a str)>, // --image + pub read: Option<&'a Path>, // -r <file> + pub write: Option<&'a Path>, // -w <file> + pub verify: Option<&'a Path>, // -v <file> + pub erase: bool, // -E + pub region: Option<(&'a str, &'a Path)>, // --image } #[derive(PartialEq, Eq, Debug)] @@ -227,7 +231,7 @@ } } - fn read_into_file(&self, path: &str) -> Result<(), FlashromError> { + fn read_into_file(&self, path: &Path) -> Result<(), FlashromError> { let opts = FlashromOpt { io_opt: IOOpt { read: Some(path), @@ -240,7 +244,7 @@ Ok(()) } - fn read_region_into_file(&self, path: &str, region: &str) -> Result<(), FlashromError> { + fn read_region_into_file(&self, path: &Path, region: &str) -> Result<(), FlashromError> { let opts = FlashromOpt { io_opt: IOOpt { region: Some((region, path)), @@ -253,7 +257,7 @@ Ok(()) } - fn write_from_file(&self, path: &str) -> Result<(), FlashromError> { + fn write_from_file(&self, path: &Path) -> Result<(), FlashromError> { let opts = FlashromOpt { io_opt: IOOpt { write: Some(path), @@ -266,7 +270,7 @@ Ok(()) } - fn verify_from_file(&self, path: &str) -> Result<(), FlashromError> { + fn verify_from_file(&self, path: &Path) -> Result<(), FlashromError> { let opts = FlashromOpt { io_opt: IOOpt { verify: Some(path), @@ -297,8 +301,8 @@ } } -fn flashrom_decode_opts(opts: FlashromOpt) -> Vec<String> { - let mut params = Vec::<String>::new(); +fn flashrom_decode_opts(opts: FlashromOpt) -> Vec<OsString> { + let mut params = Vec::<OsString>::new(); // ------------ WARNING !!! ------------ // each param must NOT contain spaces! @@ -307,58 +311,62 @@ // wp_opt if opts.wp_opt.range.is_some() { let (x0, x1) = opts.wp_opt.range.unwrap(); - params.push("--wp-range".to_string()); - params.push(hex_range_string(x0, x1)); + params.push("--wp-range".into()); + params.push(hex_range_string(x0, x1).into()); } if opts.wp_opt.status { - params.push("--wp-status".to_string()); + params.push("--wp-status".into()); } else if opts.wp_opt.list { - params.push("--wp-list".to_string()); + params.push("--wp-list".into()); } else if opts.wp_opt.enable { - params.push("--wp-enable".to_string()); + params.push("--wp-enable".into()); } else if opts.wp_opt.disable { - params.push("--wp-disable".to_string()); + params.push("--wp-disable".into()); } // io_opt if let Some((region, path)) = opts.io_opt.region { - params.push("--image".to_string()); - params.push(format!("{}:{}", region, path)); - params.push("-r".to_string()); + params.push("--image".into()); + let mut p = OsString::new(); + p.push(region); + p.push(":"); + p.push(path); + params.push(p); + params.push("-r".into()); } else if opts.io_opt.read.is_some() { - params.push("-r".to_string()); - params.push(opts.io_opt.read.unwrap().to_string()); + params.push("-r".into()); + params.push(opts.io_opt.read.unwrap().into()); } else if opts.io_opt.write.is_some() { - params.push("-w".to_string()); - params.push(opts.io_opt.write.unwrap().to_string()); + params.push("-w".into()); + params.push(opts.io_opt.write.unwrap().into()); } else if opts.io_opt.verify.is_some() { - params.push("-v".to_string()); - params.push(opts.io_opt.verify.unwrap().to_string()); + params.push("-v".into()); + params.push(opts.io_opt.verify.unwrap().into()); } else if opts.io_opt.erase { - params.push("-E".to_string()); + params.push("-E".into()); } // misc_opt if opts.layout.is_some() { - params.push("-l".to_string()); - params.push(opts.layout.unwrap().to_string()); + params.push("-l".into()); + params.push(opts.layout.unwrap().into()); } if opts.image.is_some() { - params.push("-i".to_string()); - params.push(opts.image.unwrap().to_string()); + params.push("-i".into()); + params.push(opts.image.unwrap().into()); } if opts.flash_name { - params.push("--flash-name".to_string()); + params.push("--flash-name".into()); } if opts.verbose { - params.push("-V".to_string()); + params.push("-V".into()); } params } -fn flashrom_dispatch<S: AsRef<str>>( +fn flashrom_dispatch<S: AsRef<OsStr>>( path: &str, params: &[S], fc: FlashChip, @@ -366,7 +374,7 @@ ) -> Result<(String, String), FlashromError> { // from man page: // ' -p, --programmer <name>[:parameter[,parameter[,parameter]]] ' - let mut args: Vec<&str> = vec!["-p", FlashChip::to(fc)]; + let mut args: Vec<&OsStr> = vec![OsStr::new("-p"), OsStr::new(FlashChip::to(fc))]; args.extend(params.iter().map(S::as_ref)); info!("flashrom_dispatch() running: {} {:?}", path, args); @@ -454,6 +462,8 @@ #[cfg(test)] mod tests { + use std::path::Path; + use super::flashrom_decode_opts; use super::{FlashromOpt, IOOpt, WPOpt}; @@ -515,21 +525,21 @@ test_io_opt( IOOpt { - read: Some("foo.bin"), + read: Some(Path::new("foo.bin")), ..Default::default() }, &["-r", "foo.bin"], ); test_io_opt( IOOpt { - write: Some("bar.bin"), + write: Some(Path::new("bar.bin")), ..Default::default() }, &["-w", "bar.bin"], ); test_io_opt( IOOpt { - verify: Some("/tmp/baz.bin"), + verify: Some(Path::new("/tmp/baz.bin")), ..Default::default() }, &["-v", "/tmp/baz.bin"], @@ -548,7 +558,7 @@ //use Default::default; assert_eq!( flashrom_decode_opts(FlashromOpt { - layout: Some("TestLayout"), + layout: Some(Path::new("TestLayout")), ..Default::default() }), &["-l", "TestLayout"] diff --git a/util/flashrom_tester/flashrom/src/flashromlib.rs b/util/flashrom_tester/flashrom/src/flashromlib.rs index d20dbb2..3036394 100644 --- a/util/flashrom_tester/flashrom/src/flashromlib.rs +++ b/util/flashrom_tester/flashrom/src/flashromlib.rs @@ -34,7 +34,7 @@ use libflashrom::{Chip, Programmer}; -use std::{cell::RefCell, convert::TryFrom, fs}; +use std::{cell::RefCell, convert::TryFrom, fs, path::Path}; use crate::{FlashChip, FlashromError, ROMWriteSpecifics}; @@ -108,13 +108,13 @@ self.wp_range((0, self.get_size()?), en) } - fn read_into_file(&self, path: &str) -> Result<(), FlashromError> { + fn read_into_file(&self, path: &Path) -> Result<(), FlashromError> { let buf = self.flashrom.borrow_mut().image_read(None)?; fs::write(path, buf).map_err(|error| error.to_string())?; Ok(()) } - fn read_region_into_file(&self, path: &str, region: &str) -> Result<(), FlashromError> { + fn read_region_into_file(&self, path: &Path, region: &str) -> Result<(), FlashromError> { let mut layout = self.flashrom.borrow_mut().layout_read_fmap_from_rom()?; layout.include_region(region)?; let range = layout.get_region_range(region)?; @@ -123,7 +123,7 @@ Ok(()) } - fn write_from_file(&self, path: &str) -> Result<(), FlashromError> { + fn write_from_file(&self, path: &Path) -> Result<(), FlashromError> { let mut buf = fs::read(path).map_err(|error| error.to_string())?; self.flashrom.borrow_mut().image_write(&mut buf, None)?; Ok(()) @@ -143,7 +143,7 @@ Ok(true) } - fn verify_from_file(&self, path: &str) -> Result<(), FlashromError> { + fn verify_from_file(&self, path: &Path) -> Result<(), FlashromError> { let buf = fs::read(path).map_err(|error| error.to_string())?; self.flashrom.borrow_mut().image_verify(&buf, None)?; Ok(()) diff --git a/util/flashrom_tester/flashrom/src/lib.rs b/util/flashrom_tester/flashrom/src/lib.rs index 7e245ae..11874f9 100644 --- a/util/flashrom_tester/flashrom/src/lib.rs +++ b/util/flashrom_tester/flashrom/src/lib.rs @@ -39,7 +39,7 @@ mod cmd; mod flashromlib; -use std::{error, fmt}; +use std::{error, fmt, path::Path}; pub use cmd::{dut_ctrl_toggle_wp, FlashromCmd}; pub use flashromlib::FlashromLib; @@ -118,8 +118,8 @@ } pub struct ROMWriteSpecifics<'a> { - pub layout_file: Option<&'a str>, - pub write_file: Option<&'a str>, + pub layout_file: Option<&'a Path>, + pub write_file: Option<&'a Path>, pub name_file: Option<&'a str>, } @@ -146,16 +146,16 @@ fn wp_toggle(&self, en: bool) -> Result<bool, FlashromError>; /// Read the whole flash to the file specified by `path`. - fn read_into_file(&self, path: &str) -> Result<(), FlashromError>; + fn read_into_file(&self, path: &Path) -> Result<(), FlashromError>; /// Read only a region of the flash. - fn read_region_into_file(&self, path: &str, region: &str) -> Result<(), FlashromError>; + fn read_region_into_file(&self, path: &Path, region: &str) -> Result<(), FlashromError>; /// Write the whole flash to the file specified by `path`. - fn write_from_file(&self, path: &str) -> Result<(), FlashromError>; + fn write_from_file(&self, path: &Path) -> Result<(), FlashromError>; /// Verify the whole flash against the file specified by `path`. - fn verify_from_file(&self, path: &str) -> Result<(), FlashromError>; + fn verify_from_file(&self, path: &Path) -> Result<(), FlashromError>; /// Erase the whole flash. fn erase(&self) -> Result<(), FlashromError>; diff --git a/util/flashrom_tester/src/rand_util.rs b/util/flashrom_tester/src/rand_util.rs index 1b0912b..a040c89 100644 --- a/util/flashrom_tester/src/rand_util.rs +++ b/util/flashrom_tester/src/rand_util.rs @@ -36,10 +36,11 @@ use std::fs::File; use std::io::prelude::*; use std::io::BufWriter; +use std::path::Path; use rand::prelude::*; -pub fn gen_rand_testdata(path: &str, size: usize) -> std::io::Result<()> { +pub fn gen_rand_testdata(path: &Path, size: usize) -> std::io::Result<()> { let mut buf = BufWriter::new(File::create(path)?); let mut a: Vec<u8> = vec![0; size]; @@ -58,8 +59,8 @@ fn gen_rand_testdata() { use super::gen_rand_testdata; - let path0 = "/tmp/idk_test00"; - let path1 = "/tmp/idk_test01"; + let path0 = Path::new("/tmp/idk_test00"); + let path1 = Path::new("/tmp/idk_test01"); let sz = 1024; gen_rand_testdata(path0, sz).unwrap(); diff --git a/util/flashrom_tester/src/tester.rs b/util/flashrom_tester/src/tester.rs index f2c3846..8324ade 100644 --- a/util/flashrom_tester/src/tester.rs +++ b/util/flashrom_tester/src/tester.rs @@ -39,9 +39,14 @@ use flashrom::FlashromError; use flashrom::{FlashChip, Flashrom}; 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; +use tempdir::TempDir; // type-signature comes from the return type of lib.rs workers. type TestError = Box<dyn std::error::Error>; @@ -58,23 +63,32 @@ pub wp: WriteProtectState<'a, 'static>, /// 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, + original_flash_contents: PathBuf, /// The path to a file containing flash-sized random data - // TODO(pmarheine) make this a PathBuf too - random_data: String, + random_data: PathBuf, + /// The path to a file containing layout data. + pub layout_file: PathBuf, + /// A directory for temporary test data, including the above files. + pub tmp_dir: TempDir, } impl<'a> TestEnv<'a> { - pub fn create(chip_type: FlashChip, cmd: &'a dyn Flashrom) -> Result<Self, FlashromError> { + pub fn create( + chip_type: FlashChip, + cmd: &'a dyn Flashrom, + print_layout: bool, + ) -> Result<Self, FlashromError> { let rom_sz = cmd.get_size()?; + let tmp_dir = TempDir::new("flashrom_tester").map_err(|e| e.to_string())?; let out = TestEnv { chip_type, cmd, layout: utils::get_layout_sizes(rom_sz)?, wp: WriteProtectState::from_hardware(cmd, chip_type)?, - original_flash_contents: "/tmp/flashrom_tester_golden.bin".into(), - random_data: "/tmp/random_content.bin".into(), + original_flash_contents: tmp_dir.path().join("flashrom_tester_golden.bin"), + random_data: tmp_dir.path().join("random_content.bin"), + layout_file: create_layout_file(rom_sz, tmp_dir.path(), print_layout), + tmp_dir, }; info!("Stashing golden image for verification/recovery on completion"); @@ -113,7 +127,7 @@ /// Return the path to a file that contains random data and is the same size /// as the flash chip. - pub fn random_data_file(&self) -> &str { + pub fn random_data_file(&self) -> &Path { &self.random_data } @@ -147,7 +161,7 @@ /// path. /// /// Returns Err if they are not the same. - pub fn verify(&self, contents_path: &str) -> Result<(), FlashromError> { + pub fn verify(&self, contents_path: &Path) -> Result<(), FlashromError> { self.cmd.verify_from_file(contents_path)?; Ok(()) } @@ -487,17 +501,38 @@ } } +fn create_layout_file(rom_sz: i64, tmp_dir: &Path, print_layout: bool) -> PathBuf { + info!("Calculate ROM partition sizes & Create the layout file."); + let layout_sizes = utils::get_layout_sizes(rom_sz).expect("Could not partition rom"); + + let layout_file = tmp_dir.join("layout.file"); + let mut f = File::create(&layout_file).expect("Could not create layout file"); + let mut buf: Vec<u8> = vec![]; + utils::construct_layout_file(&mut buf, &layout_sizes).expect("Could not construct layout file"); + + f.write_all(&buf).expect("Writing layout file failed"); + if print_layout { + info!( + "Dumping layout file as requested:\n{}", + String::from_utf8_lossy(&buf) + ); + } + layout_file +} + pub fn run_all_tests<T, TS>( chip: FlashChip, cmd: &dyn Flashrom, ts: TS, terminate_flag: Option<&AtomicBool>, + print_layout: bool, ) -> Vec<(String, (TestConclusion, Option<TestError>))> where T: TestCase + Copy, TS: IntoIterator<Item = T>, { - let mut env = TestEnv::create(chip, cmd).expect("Failed to set up test environment"); + let mut env = + TestEnv::create(chip, cmd, print_layout).expect("Failed to set up test environment"); let mut results = Vec::new(); for t in ts { diff --git a/util/flashrom_tester/src/tests.rs b/util/flashrom_tester/src/tests.rs index 1527a6e..94fe14d 100644 --- a/util/flashrom_tester/src/tests.rs +++ b/util/flashrom_tester/src/tests.rs @@ -40,12 +40,9 @@ use std::collections::{HashMap, HashSet}; use std::convert::TryInto; use std::fs::{self, File}; -use std::io::{BufRead, Write}; +use std::io::BufRead; use std::sync::atomic::AtomicBool; -const LAYOUT_FILE: &str = "/tmp/layout.file"; -const ELOG_FILE: &str = "/tmp/elog.file"; - /// Iterate over tests, yielding only those tests with names matching filter_names. /// /// If filter_names is None, all tests will be run. None is distinct from Some(∅); @@ -91,23 +88,6 @@ ) -> Result<(), Box<dyn std::error::Error>> { utils::ac_power_warning(); - info!("Calculate ROM partition sizes & Create the layout file."); - let rom_sz: i64 = cmd.get_size()?; - let layout_sizes = utils::get_layout_sizes(rom_sz)?; - { - let mut f = File::create(LAYOUT_FILE)?; - let mut buf: Vec<u8> = vec![]; - utils::construct_layout_file(&mut buf, &layout_sizes)?; - - f.write_all(&buf)?; - if print_layout { - info!( - "Dumping layout file as requested:\n{}", - String::from_utf8_lossy(&buf) - ); - } - } - info!("Record crossystem information.\n{}", crossystem); // Register tests to run: @@ -144,7 +124,7 @@ // ------------------------. // Run all the tests and collate the findings: - let results = tester::run_all_tests(fc, cmd, tests, terminate_flag); + let results = tester::run_all_tests(fc, cmd, tests, terminate_flag, print_layout); // Any leftover filtered names were specified to be run but don't exist for leftover in filter_names.iter().flatten() { @@ -241,15 +221,17 @@ env.ensure_golden()?; const ELOG_RW_REGION_NAME: &str = "RW_ELOG"; + let elog_file = env.tmp_dir.path().join("elog.file"); + env.cmd - .read_region_into_file(ELOG_FILE, ELOG_RW_REGION_NAME)?; + .read_region_into_file(&elog_file, ELOG_RW_REGION_NAME)?; // Just checking for the magic numer // TODO: improve this test to read the events - if fs::metadata(ELOG_FILE)?.len() < 4 { + if fs::metadata(&elog_file)?.len() < 4 { return Err("ELOG contained no data".into()); } - let data = fs::read(ELOG_FILE)?; + let data = fs::read(&elog_file)?; if u32::from_le_bytes(data[0..4].try_into()?) != 0x474f4c45 { return Err("ELOG had bad magic number".into()); } @@ -295,7 +277,7 @@ // Check that we cannot write to the protected region. let rws = flashrom::ROMWriteSpecifics { - layout_file: Some(LAYOUT_FILE), + layout_file: Some(&env.layout_file), write_file: Some(env.random_data_file()), name_file: Some(wp_section_name), }; @@ -313,7 +295,7 @@ let (non_wp_section_name, _, _) = utils::layout_section(env.layout(), section.get_non_overlapping_section()); let rws = flashrom::ROMWriteSpecifics { - layout_file: Some(LAYOUT_FILE), + layout_file: Some(&env.layout_file), write_file: Some(env.random_data_file()), name_file: Some(non_wp_section_name), }; -- To view, visit https://review.coreboot.org/c/flashrom/+/68643 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I4a0b25c8c70fa6dfd682fba3db7d3bf06b2eb7ac Gerrit-Change-Number: 68643 Gerrit-PatchSet: 1 Gerrit-Owner: Evan Benn <evanbenn(a)google.com> Gerrit-MessageType: newchange

1 0

[M] Change in flashrom[master]: tests: Add prefix to io_mock functions not to clash with macros
by Peter Marheine (Code Review) 20 Oct '22

20 Oct '22

Attention is currently required from: Felix Singer, Thomas Heijligen, Anastasia Klimchuk. Peter Marheine has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/68433 ) Change subject: tests: Add prefix to io_mock functions not to clash with macros ...................................................................... Patch Set 2: Code-Review+1 -- To view, visit https://review.coreboot.org/c/flashrom/+/68433 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I7998a8fb1b9e65621e12adbfab5460a245d5606b Gerrit-Change-Number: 68433 Gerrit-PatchSet: 2 Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Felix Singer <felixsinger(a)posteo.net> Gerrit-Reviewer: Peter Marheine <pmarheine(a)chromium.org> 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: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Comment-Date: Thu, 20 Oct 2022 23:29:00 +0000 Gerrit-HasComments: No Gerrit-Has-Labels: Yes Gerrit-MessageType: comment

1 0

[M] Change in flashrom[1.0.x]: ichspi: Unify timeouts across all SPI operations to 30s
by Felix Singer (Code Review) 20 Oct '22

20 Oct '22

Felix Singer has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/68640 ) Change subject: ichspi: Unify timeouts across all SPI operations to 30s ...................................................................... ichspi: Unify timeouts across all SPI operations to 30s Note: This patch was backported from the master branch and it was modified so that it can be applied on the 1.0.x branch. `ich_hwseq_wait_for_cycle_complete()` drops taking `timeout` as argument in favor of a fixed timeout of `30 seconds` for any given SPI operation as recommended by the SPI programming guide. Document: Alder Lake-P Client Platform SPI Programming Guide Rev 1.30 (supporting document for multi-master accessing the SPI Flash device.) Refer to below section to understand the problem in more detail and SPI operation timeout recommendation from Intel in multi-master scenarios. On Intel Chipsets that support multi-mastering access of the SPI flash may run into a timeout failure when the operation initiated from a single master just follows the SPI operational timeout recommendation as per the vendor datasheet (example: winbond spiflash W25Q256JV-DTR specification, table 9.7). In the multi-master SPI accessing scenario using hardware sequencing operation, it's impossible to know the actual status of the SPI bus prior to individual master starting the operation (SPI Cycle In Progress a.k.a SCIP bit represents the status of SPI operation on individual master). Thus, any SPI operation triggered in multi-master environment might need to account a worst case scenario where the most time consuming operation might have occupied the SPI bus from a master and an operation initiated by another master just timed out. Here is the timeout calculation for any hardware sequencing operation: Worst Case Operational Delay = (Maximum Time consumed by a SPI operation + Any marginal adjustment) Timeout Recommendation for Hardware Sequencing Operation = ((Worst Case Operational Delay) * (#No. Of SPI Master - 1) + Current Operational latency) Assume, on Intel platform with 6 SPI master like, Host CPU, CSE, EC, GbE and other reserved etc, hence, the Timeout Calculation for SPI erase Operation would look like as below: Maximum Time consumed by a SPI Operation = 5 seconds Worst Case Operational Delay = 5 seconds Timeout Recommendation for Hardware Seq Operation = 5 seconds * (6 - 1) + 5 seconds = 30 seconds BUG=b:223630977 TEST=Able to perform read/write/erase operation on PCH 600 series chipset (board name: Brya). Original-Signed-off-by: Subrata Banik <subratabanik(a)google.com> Original-Change-Id: Ifa910dea794175d8ee2ad277549e5a0d69cba45b Original-Reviewed-on: https://review.coreboot.org/c/flashrom/+/62867 Original-Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org> Original-Reviewed-by: Anastasia Klimchuk <aklm(a)chromium.org> Original-Reviewed-by: Arthur Heymans <arthur(a)aheymans.xyz> Original-Reviewed-by: Edward O'Callaghan <quasisec(a)chromium.org> Change-Id: Icb632d28c2c8420b9e47a8a0abd0dbf84dc200eb Signed-off-by: Felix Singer <felixsinger(a)posteo.net> --- M ichspi.c 1 file changed, 85 insertions(+), 11 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/40/68640/1 diff --git a/ichspi.c b/ichspi.c index c7bda92..af201a4 100644 --- a/ichspi.c +++ b/ichspi.c @@ -1251,20 +1251,24 @@ Resets all error flags in HSFS. Returns 0 if the cycle completes successfully without errors within timeout us, 1 on errors. */ -static int ich_hwseq_wait_for_cycle_complete(unsigned int timeout, - unsigned int len) +static int ich_hwseq_wait_for_cycle_complete(unsigned int len) { + /* + * The SPI bus may be busy due to performing operations from other masters, hence + * introduce the long timeout of 30s to cover the worst case scenarios as well. + */ + unsigned int timeout_us = 30 * 1000 * 1000; uint16_t hsfs; uint32_t addr; - timeout /= 8; /* scale timeout duration to counter */ + timeout_us /= 8; /* scale timeout duration to counter */ while ((((hsfs = REGREAD16(ICH9_REG_HSFS)) & (HSFS_FDONE | HSFS_FCERR)) == 0) && - --timeout) { + --timeout_us) { programmer_delay(8); } REGWRITE16(ICH9_REG_HSFS, REGREAD16(ICH9_REG_HSFS)); - if (!timeout) { + if (!timeout_us) { addr = REGREAD32(ICH9_REG_FADDR) & hwseq_data.addr_mask; msg_perr("Timeout error between offset 0x%08x and " "0x%08x (= 0x%08x + %d)!\n", @@ -1347,7 +1351,6 @@ { uint32_t erase_block; uint16_t hsfc; - uint32_t timeout = 5000 * 1000; /* 5 s for max 64 kB */ erase_block = ich_hwseq_get_erase_block_size(addr); if (len != erase_block) { @@ -1387,7 +1390,7 @@ prettyprint_ich9_reg_hsfc(hsfc); REGWRITE16(ICH9_REG_HSFC, hsfc); - if (ich_hwseq_wait_for_cycle_complete(timeout, len)) + if (ich_hwseq_wait_for_cycle_complete(len)) return -1; return 0; } @@ -1396,7 +1399,6 @@ unsigned int addr, unsigned int len) { uint16_t hsfc; - uint16_t timeout = 100 * 60; uint8_t block_len; if (addr + len > flash->chip->total_size * 1024) { @@ -1424,7 +1426,7 @@ hsfc |= HSFC_FGO; /* start */ REGWRITE16(ICH9_REG_HSFC, hsfc); - if (ich_hwseq_wait_for_cycle_complete(timeout, block_len)) + if (ich_hwseq_wait_for_cycle_complete(block_len)) return 1; ich_read_data(buf, block_len, ICH9_REG_FDATA0); addr += block_len; @@ -1437,7 +1439,6 @@ static int ich_hwseq_write(struct flashctx *flash, const uint8_t *buf, unsigned int addr, unsigned int len) { uint16_t hsfc; - uint16_t timeout = 100 * 60; uint8_t block_len; if (addr + len > flash->chip->total_size * 1024) { @@ -1466,7 +1467,7 @@ hsfc |= HSFC_FGO; /* start */ REGWRITE16(ICH9_REG_HSFC, hsfc); - if (ich_hwseq_wait_for_cycle_complete(timeout, block_len)) + if (ich_hwseq_wait_for_cycle_complete(block_len)) return -1; addr += block_len; buf += block_len; -- To view, visit https://review.coreboot.org/c/flashrom/+/68640 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: 1.0.x Gerrit-Change-Id: Icb632d28c2c8420b9e47a8a0abd0dbf84dc200eb Gerrit-Change-Number: 68640 Gerrit-PatchSet: 1 Gerrit-Owner: Felix Singer <felixsinger(a)posteo.net> Gerrit-CC: Subrata Banik <subratabanik(a)google.com> Gerrit-MessageType: newchange

1 0

[M] Change in flashrom[1.1.x]: ichspi: Unify timeouts across all SPI operations to 30s
by Felix Singer (Code Review) 20 Oct '22

20 Oct '22

Felix Singer has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/68639 ) Change subject: ichspi: Unify timeouts across all SPI operations to 30s ...................................................................... ichspi: Unify timeouts across all SPI operations to 30s Note: This patch was backported from the master branch and it was modified so that it can be applied on the 1.1.x branch. `ich_hwseq_wait_for_cycle_complete()` drops taking `timeout` as argument in favor of a fixed timeout of `30 seconds` for any given SPI operation as recommended by the SPI programming guide. Document: Alder Lake-P Client Platform SPI Programming Guide Rev 1.30 (supporting document for multi-master accessing the SPI Flash device.) Refer to below section to understand the problem in more detail and SPI operation timeout recommendation from Intel in multi-master scenarios. On Intel Chipsets that support multi-mastering access of the SPI flash may run into a timeout failure when the operation initiated from a single master just follows the SPI operational timeout recommendation as per the vendor datasheet (example: winbond spiflash W25Q256JV-DTR specification, table 9.7). In the multi-master SPI accessing scenario using hardware sequencing operation, it's impossible to know the actual status of the SPI bus prior to individual master starting the operation (SPI Cycle In Progress a.k.a SCIP bit represents the status of SPI operation on individual master). Thus, any SPI operation triggered in multi-master environment might need to account a worst case scenario where the most time consuming operation might have occupied the SPI bus from a master and an operation initiated by another master just timed out. Here is the timeout calculation for any hardware sequencing operation: Worst Case Operational Delay = (Maximum Time consumed by a SPI operation + Any marginal adjustment) Timeout Recommendation for Hardware Sequencing Operation = ((Worst Case Operational Delay) * (#No. Of SPI Master - 1) + Current Operational latency) Assume, on Intel platform with 6 SPI master like, Host CPU, CSE, EC, GbE and other reserved etc, hence, the Timeout Calculation for SPI erase Operation would look like as below: Maximum Time consumed by a SPI Operation = 5 seconds Worst Case Operational Delay = 5 seconds Timeout Recommendation for Hardware Seq Operation = 5 seconds * (6 - 1) + 5 seconds = 30 seconds BUG=b:223630977 TEST=Able to perform read/write/erase operation on PCH 600 series chipset (board name: Brya). Original-Signed-off-by: Subrata Banik <subratabanik(a)google.com> Original-Change-Id: Ifa910dea794175d8ee2ad277549e5a0d69cba45b Original-Reviewed-on: https://review.coreboot.org/c/flashrom/+/62867 Original-Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org> Original-Reviewed-by: Anastasia Klimchuk <aklm(a)chromium.org> Original-Reviewed-by: Arthur Heymans <arthur(a)aheymans.xyz> Original-Reviewed-by: Edward O'Callaghan <quasisec(a)chromium.org> Change-Id: Ifb8e65ee7707c4a89c3a33d041f1957f419a32b2 Signed-off-by: Felix Singer <felixsinger(a)posteo.net> --- M ichspi.c 1 file changed, 85 insertions(+), 11 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/39/68639/1 diff --git a/ichspi.c b/ichspi.c index 0f1470d..6cf6b5f 100644 --- a/ichspi.c +++ b/ichspi.c @@ -1265,20 +1265,24 @@ Resets all error flags in HSFS. Returns 0 if the cycle completes successfully without errors within timeout us, 1 on errors. */ -static int ich_hwseq_wait_for_cycle_complete(unsigned int timeout, - unsigned int len) +static int ich_hwseq_wait_for_cycle_complete(unsigned int len) { + /* + * The SPI bus may be busy due to performing operations from other masters, hence + * introduce the long timeout of 30s to cover the worst case scenarios as well. + */ + unsigned int timeout_us = 30 * 1000 * 1000; uint16_t hsfs; uint32_t addr; - timeout /= 8; /* scale timeout duration to counter */ + timeout_us /= 8; /* scale timeout duration to counter */ while ((((hsfs = REGREAD16(ICH9_REG_HSFS)) & (HSFS_FDONE | HSFS_FCERR)) == 0) && - --timeout) { + --timeout_us) { programmer_delay(8); } REGWRITE16(ICH9_REG_HSFS, REGREAD16(ICH9_REG_HSFS)); - if (!timeout) { + if (!timeout_us) { addr = REGREAD32(ICH9_REG_FADDR) & hwseq_data.addr_mask; msg_perr("Timeout error between offset 0x%08x and " "0x%08x (= 0x%08x + %d)!\n", @@ -1361,7 +1365,6 @@ { uint32_t erase_block; uint16_t hsfc; - uint32_t timeout = 5000 * 1000; /* 5 s for max 64 kB */ erase_block = ich_hwseq_get_erase_block_size(addr); if (len != erase_block) { @@ -1401,7 +1404,7 @@ prettyprint_ich9_reg_hsfc(hsfc); REGWRITE16(ICH9_REG_HSFC, hsfc); - if (ich_hwseq_wait_for_cycle_complete(timeout, len)) + if (ich_hwseq_wait_for_cycle_complete(len)) return -1; return 0; } @@ -1410,7 +1413,6 @@ unsigned int addr, unsigned int len) { uint16_t hsfc; - uint16_t timeout = 100 * 60; uint8_t block_len; if (addr + len > flash->chip->total_size * 1024) { @@ -1438,7 +1440,7 @@ hsfc |= HSFC_FGO; /* start */ REGWRITE16(ICH9_REG_HSFC, hsfc); - if (ich_hwseq_wait_for_cycle_complete(timeout, block_len)) + if (ich_hwseq_wait_for_cycle_complete(block_len)) return 1; ich_read_data(buf, block_len, ICH9_REG_FDATA0); addr += block_len; @@ -1451,7 +1453,6 @@ static int ich_hwseq_write(struct flashctx *flash, const uint8_t *buf, unsigned int addr, unsigned int len) { uint16_t hsfc; - uint16_t timeout = 100 * 60; uint8_t block_len; if (addr + len > flash->chip->total_size * 1024) { @@ -1480,7 +1481,7 @@ hsfc |= HSFC_FGO; /* start */ REGWRITE16(ICH9_REG_HSFC, hsfc); - if (ich_hwseq_wait_for_cycle_complete(timeout, block_len)) + if (ich_hwseq_wait_for_cycle_complete(block_len)) return -1; addr += block_len; buf += block_len; -- To view, visit https://review.coreboot.org/c/flashrom/+/68639 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: 1.1.x Gerrit-Change-Id: Ifb8e65ee7707c4a89c3a33d041f1957f419a32b2 Gerrit-Change-Number: 68639 Gerrit-PatchSet: 1 Gerrit-Owner: Felix Singer <felixsinger(a)posteo.net> Gerrit-CC: Subrata Banik <subratabanik(a)google.com> Gerrit-MessageType: newchange

1 0

[M] Change in flashrom[1.1.x]: ichspi: Unify timeouts across all SPI operations to 30s
by Felix Singer (Code Review) 20 Oct '22

20 Oct '22

Attention is currently required from: Subrata Banik, Edward O'Callaghan, Arthur Heymans, Anastasia Klimchuk. Hello build bot (Jenkins), Subrata Banik, Edward O'Callaghan, Arthur Heymans, Anastasia Klimchuk, I'd like you to do a code review. Please visit https://review.coreboot.org/c/flashrom/+/68598 to review the following change. Change subject: ichspi: Unify timeouts across all SPI operations to 30s ...................................................................... ichspi: Unify timeouts across all SPI operations to 30s Note: This patch was backported from the master branch and it was modified so that it can be applied on the 1.1.x branch. `ich_hwseq_wait_for_cycle_complete()` drops taking `timeout` as argument in favor of a fixed timeout of `30 seconds` for any given SPI operation as recommended by the SPI programming guide. Document: Alder Lake-P Client Platform SPI Programming Guide Rev 1.30 (supporting document for multi-master accessing the SPI Flash device.) Refer to below section to understand the problem in more detail and SPI operation timeout recommendation from Intel in multi-master scenarios. On Intel Chipsets that support multi-mastering access of the SPI flash may run into a timeout failure when the operation initiated from a single master just follows the SPI operational timeout recommendation as per the vendor datasheet (example: winbond spiflash W25Q256JV-DTR specification, table 9.7). In the multi-master SPI accessing scenario using hardware sequencing operation, it's impossible to know the actual status of the SPI bus prior to individual master starting the operation (SPI Cycle In Progress a.k.a SCIP bit represents the status of SPI operation on individual master). Thus, any SPI operation triggered in multi-master environment might need to account a worst case scenario where the most time consuming operation might have occupied the SPI bus from a master and an operation initiated by another master just timed out. Here is the timeout calculation for any hardware sequencing operation: Worst Case Operational Delay = (Maximum Time consumed by a SPI operation + Any marginal adjustment) Timeout Recommendation for Hardware Sequencing Operation = ((Worst Case Operational Delay) * (#No. Of SPI Master - 1) + Current Operational latency) Assume, on Intel platform with 6 SPI master like, Host CPU, CSE, EC, GbE and other reserved etc, hence, the Timeout Calculation for SPI erase Operation would look like as below: Maximum Time consumed by a SPI Operation = 5 seconds Worst Case Operational Delay = 5 seconds Timeout Recommendation for Hardware Seq Operation = 5 seconds * (6 - 1) + 5 seconds = 30 seconds BUG=b:223630977 TEST=Able to perform read/write/erase operation on PCH 600 series chipset (board name: Brya). Signed-off-by: Subrata Banik <subratabanik(a)google.com> Change-Id: Ifa910dea794175d8ee2ad277549e5a0d69cba45b Reviewed-on: https://review.coreboot.org/c/flashrom/+/62867 Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org> Reviewed-by: Anastasia Klimchuk <aklm(a)chromium.org> Reviewed-by: Arthur Heymans <arthur(a)aheymans.xyz> Reviewed-by: Edward O'Callaghan <quasisec(a)chromium.org> --- M ichspi.c 1 file changed, 83 insertions(+), 11 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/98/68598/1 diff --git a/ichspi.c b/ichspi.c index 0f1470d..6cf6b5f 100644 --- a/ichspi.c +++ b/ichspi.c @@ -1265,20 +1265,24 @@ Resets all error flags in HSFS. Returns 0 if the cycle completes successfully without errors within timeout us, 1 on errors. */ -static int ich_hwseq_wait_for_cycle_complete(unsigned int timeout, - unsigned int len) +static int ich_hwseq_wait_for_cycle_complete(unsigned int len) { + /* + * The SPI bus may be busy due to performing operations from other masters, hence + * introduce the long timeout of 30s to cover the worst case scenarios as well. + */ + unsigned int timeout_us = 30 * 1000 * 1000; uint16_t hsfs; uint32_t addr; - timeout /= 8; /* scale timeout duration to counter */ + timeout_us /= 8; /* scale timeout duration to counter */ while ((((hsfs = REGREAD16(ICH9_REG_HSFS)) & (HSFS_FDONE | HSFS_FCERR)) == 0) && - --timeout) { + --timeout_us) { programmer_delay(8); } REGWRITE16(ICH9_REG_HSFS, REGREAD16(ICH9_REG_HSFS)); - if (!timeout) { + if (!timeout_us) { addr = REGREAD32(ICH9_REG_FADDR) & hwseq_data.addr_mask; msg_perr("Timeout error between offset 0x%08x and " "0x%08x (= 0x%08x + %d)!\n", @@ -1361,7 +1365,6 @@ { uint32_t erase_block; uint16_t hsfc; - uint32_t timeout = 5000 * 1000; /* 5 s for max 64 kB */ erase_block = ich_hwseq_get_erase_block_size(addr); if (len != erase_block) { @@ -1401,7 +1404,7 @@ prettyprint_ich9_reg_hsfc(hsfc); REGWRITE16(ICH9_REG_HSFC, hsfc); - if (ich_hwseq_wait_for_cycle_complete(timeout, len)) + if (ich_hwseq_wait_for_cycle_complete(len)) return -1; return 0; } @@ -1410,7 +1413,6 @@ unsigned int addr, unsigned int len) { uint16_t hsfc; - uint16_t timeout = 100 * 60; uint8_t block_len; if (addr + len > flash->chip->total_size * 1024) { @@ -1438,7 +1440,7 @@ hsfc |= HSFC_FGO; /* start */ REGWRITE16(ICH9_REG_HSFC, hsfc); - if (ich_hwseq_wait_for_cycle_complete(timeout, block_len)) + if (ich_hwseq_wait_for_cycle_complete(block_len)) return 1; ich_read_data(buf, block_len, ICH9_REG_FDATA0); addr += block_len; @@ -1451,7 +1453,6 @@ static int ich_hwseq_write(struct flashctx *flash, const uint8_t *buf, unsigned int addr, unsigned int len) { uint16_t hsfc; - uint16_t timeout = 100 * 60; uint8_t block_len; if (addr + len > flash->chip->total_size * 1024) { @@ -1480,7 +1481,7 @@ hsfc |= HSFC_FGO; /* start */ REGWRITE16(ICH9_REG_HSFC, hsfc); - if (ich_hwseq_wait_for_cycle_complete(timeout, block_len)) + if (ich_hwseq_wait_for_cycle_complete(block_len)) return -1; addr += block_len; buf += block_len; -- To view, visit https://review.coreboot.org/c/flashrom/+/68598 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: 1.1.x Gerrit-Change-Id: Ifa910dea794175d8ee2ad277549e5a0d69cba45b Gerrit-Change-Number: 68598 Gerrit-PatchSet: 1 Gerrit-Owner: Felix Singer <felixsinger(a)posteo.net> Gerrit-Reviewer: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Subrata Banik <subratabanik(a)google.com> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Subrata Banik <subratabanik(a)google.com> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Attention: Arthur Heymans <arthur(a)aheymans.xyz> Gerrit-Attention: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-MessageType: newchange

1 0

[M] Change in flashrom[1.2.x]: ichspi: Unify timeouts across all SPI operations to 30s
by Felix Singer (Code Review) 20 Oct '22

20 Oct '22

Felix Singer has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/68597 ) Change subject: ichspi: Unify timeouts across all SPI operations to 30s ...................................................................... ichspi: Unify timeouts across all SPI operations to 30s Note: This patch was backported from the master branch and it was modified so that it can be applied on the 1.2.x branch. `ich_hwseq_wait_for_cycle_complete()` drops taking `timeout` as argument in favor of a fixed timeout of `30 seconds` for any given SPI operation as recommended by the SPI programming guide. Document: Alder Lake-P Client Platform SPI Programming Guide Rev 1.30 (supporting document for multi-master accessing the SPI Flash device.) Refer to below section to understand the problem in more detail and SPI operation timeout recommendation from Intel in multi-master scenarios. On Intel Chipsets that support multi-mastering access of the SPI flash may run into a timeout failure when the operation initiated from a single master just follows the SPI operational timeout recommendation as per the vendor datasheet (example: winbond spiflash W25Q256JV-DTR specification, table 9.7). In the multi-master SPI accessing scenario using hardware sequencing operation, it's impossible to know the actual status of the SPI bus prior to individual master starting the operation (SPI Cycle In Progress a.k.a SCIP bit represents the status of SPI operation on individual master). Thus, any SPI operation triggered in multi-master environment might need to account a worst case scenario where the most time consuming operation might have occupied the SPI bus from a master and an operation initiated by another master just timed out. Here is the timeout calculation for any hardware sequencing operation: Worst Case Operational Delay = (Maximum Time consumed by a SPI operation + Any marginal adjustment) Timeout Recommendation for Hardware Sequencing Operation = ((Worst Case Operational Delay) * (#No. Of SPI Master - 1) + Current Operational latency) Assume, on Intel platform with 6 SPI master like, Host CPU, CSE, EC, GbE and other reserved etc, hence, the Timeout Calculation for SPI erase Operation would look like as below: Maximum Time consumed by a SPI Operation = 5 seconds Worst Case Operational Delay = 5 seconds Timeout Recommendation for Hardware Seq Operation = 5 seconds * (6 - 1) + 5 seconds = 30 seconds BUG=b:223630977 TEST=Able to perform read/write/erase operation on PCH 600 series chipset (board name: Brya). Original-Signed-off-by: Subrata Banik <subratabanik(a)google.com> Original-Change-Id: Ifa910dea794175d8ee2ad277549e5a0d69cba45b Original-Reviewed-on: https://review.coreboot.org/c/flashrom/+/62867 Original-Tested-by: build bot (Jenkins) <no-reply(a)coreboot.org> Original-Reviewed-by: Anastasia Klimchuk <aklm(a)chromium.org> Original-Reviewed-by: Arthur Heymans <arthur(a)aheymans.xyz> Original-Reviewed-by: Edward O'Callaghan <quasisec(a)chromium.org> Change-Id: I588b02da78fe26327d44bee455afb8272b34ed16 Signed-off-by: Felix Singer <felixsinger(a)posteo.net> --- M ichspi.c 1 file changed, 85 insertions(+), 11 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/97/68597/1 diff --git a/ichspi.c b/ichspi.c index 12ee126..725bfb3 100644 --- a/ichspi.c +++ b/ichspi.c @@ -1282,20 +1282,24 @@ Resets all error flags in HSFS. Returns 0 if the cycle completes successfully without errors within timeout us, 1 on errors. */ -static int ich_hwseq_wait_for_cycle_complete(unsigned int timeout, - unsigned int len) +static int ich_hwseq_wait_for_cycle_complete(unsigned int len) { + /* + * The SPI bus may be busy due to performing operations from other masters, hence + * introduce the long timeout of 30s to cover the worst case scenarios as well. + */ + unsigned int timeout_us = 30 * 1000 * 1000; uint16_t hsfs; uint32_t addr; - timeout /= 8; /* scale timeout duration to counter */ + timeout_us /= 8; /* scale timeout duration to counter */ while ((((hsfs = REGREAD16(ICH9_REG_HSFS)) & (HSFS_FDONE | HSFS_FCERR)) == 0) && - --timeout) { + --timeout_us) { programmer_delay(8); } REGWRITE16(ICH9_REG_HSFS, REGREAD16(ICH9_REG_HSFS)); - if (!timeout) { + if (!timeout_us) { addr = REGREAD32(ICH9_REG_FADDR) & hwseq_data.addr_mask; msg_perr("Timeout error between offset 0x%08x and " "0x%08x (= 0x%08x + %d)!\n", @@ -1378,7 +1382,6 @@ { uint32_t erase_block; uint16_t hsfc; - uint32_t timeout = 5000 * 1000; /* 5 s for max 64 kB */ erase_block = ich_hwseq_get_erase_block_size(addr); if (len != erase_block) { @@ -1418,7 +1421,7 @@ prettyprint_ich9_reg_hsfc(hsfc); REGWRITE16(ICH9_REG_HSFC, hsfc); - if (ich_hwseq_wait_for_cycle_complete(timeout, len)) + if (ich_hwseq_wait_for_cycle_complete(len)) return -1; return 0; } @@ -1427,7 +1430,6 @@ unsigned int addr, unsigned int len) { uint16_t hsfc; - uint16_t timeout = 100 * 60; uint8_t block_len; if (addr + len > flash->chip->total_size * 1024) { @@ -1455,7 +1457,7 @@ hsfc |= HSFC_FGO; /* start */ REGWRITE16(ICH9_REG_HSFC, hsfc); - if (ich_hwseq_wait_for_cycle_complete(timeout, block_len)) + if (ich_hwseq_wait_for_cycle_complete(block_len)) return 1; ich_read_data(buf, block_len, ICH9_REG_FDATA0); addr += block_len; @@ -1468,7 +1470,6 @@ static int ich_hwseq_write(struct flashctx *flash, const uint8_t *buf, unsigned int addr, unsigned int len) { uint16_t hsfc; - uint16_t timeout = 100 * 60; uint8_t block_len; if (addr + len > flash->chip->total_size * 1024) { @@ -1497,7 +1498,7 @@ hsfc |= HSFC_FGO; /* start */ REGWRITE16(ICH9_REG_HSFC, hsfc); - if (ich_hwseq_wait_for_cycle_complete(timeout, block_len)) + if (ich_hwseq_wait_for_cycle_complete(block_len)) return -1; addr += block_len; buf += block_len; -- To view, visit https://review.coreboot.org/c/flashrom/+/68597 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: 1.2.x Gerrit-Change-Id: I588b02da78fe26327d44bee455afb8272b34ed16 Gerrit-Change-Number: 68597 Gerrit-PatchSet: 1 Gerrit-Owner: Felix Singer <felixsinger(a)posteo.net> Gerrit-CC: Subrata Banik <subratabanik(a)google.com> Gerrit-MessageType: newchange

1 0

2024

2023

2022

2021

2020

2019

2018

2017

flashrom-gerrit October 2022