June 2022 - flashrom-gerrit - mail.coreboot.org

Change in flashrom[master]: libflashrom.map: Add missing functions from libflashrom.h
by Evan Benn (Code Review) June 1, 2022

June 1, 2022

Attention is currently required from: Felix Singer, Nikolai Artemiev. Evan Benn has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/64851 ) Change subject: libflashrom.map: Add missing functions from libflashrom.h ...................................................................... Patch Set 2: (2 comments) Commit Message: https://review.coreboot.org/c/flashrom/+/64851/comment/094bc8d5_b16e4484 PS1, Line 7: Add all functions from header > Suggestion: […] Done https://review.coreboot.org/c/flashrom/+/64851/comment/4a3fb9b3_fc0e63da PS1, Line 9: I added all : functioned declared in the header. > Please use imperative. Suggestion: […] Done -- To view, visit https://review.coreboot.org/c/flashrom/+/64851 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ic90b3c20780d3a07b00bfca82d23d44c4fa6f22f Gerrit-Change-Number: 64851 Gerrit-PatchSet: 2 Gerrit-Owner: Evan Benn <evanbenn(a)google.com> Gerrit-Reviewer: Nikolai Artemiev <nartemiev(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Felix Singer <felixsinger(a)posteo.net> Gerrit-Attention: Felix Singer <felixsinger(a)posteo.net> Gerrit-Attention: Nikolai Artemiev <nartemiev(a)chromium.org> Gerrit-Comment-Date: Wed, 01 Jun 2022 04:26:49 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Felix Singer <felixsinger(a)posteo.net> Gerrit-MessageType: comment

1 0

Change in flashrom[master]: libflashrom.map: Add missing functions from libflashrom.h
by Evan Benn (Code Review) June 1, 2022

June 1, 2022

Attention is currently required from: Nikolai Artemiev, Evan Benn. Hello build bot (Jenkins), Nikolai Artemiev, I'd like you to reexamine a change. Please visit https://review.coreboot.org/c/flashrom/+/64851 to look at the new patch set (#2). Change subject: libflashrom.map: Add missing functions from libflashrom.h ...................................................................... libflashrom.map: Add missing functions from libflashrom.h Some functions were not being defined, namely flashrom_wp_*. Thus, add all missing functions from libflashrom.h header file. Change-Id: Ic90b3c20780d3a07b00bfca82d23d44c4fa6f22f Signed-off-by: Evan Benn <evanbenn(a)chromium.org> --- M libflashrom.map 1 file changed, 18 insertions(+), 1 deletion(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/51/64851/2 -- To view, visit https://review.coreboot.org/c/flashrom/+/64851 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ic90b3c20780d3a07b00bfca82d23d44c4fa6f22f Gerrit-Change-Number: 64851 Gerrit-PatchSet: 2 Gerrit-Owner: Evan Benn <evanbenn(a)google.com> Gerrit-Reviewer: Nikolai Artemiev <nartemiev(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Felix Singer <felixsinger(a)posteo.net> Gerrit-Attention: Nikolai Artemiev <nartemiev(a)chromium.org> Gerrit-Attention: Evan Benn <evanbenn(a)google.com> Gerrit-MessageType: newpatchset

1 0

Change in flashrom[master]: libflashrom.map: Add all functions from header
by Felix Singer (Code Review) June 1, 2022

June 1, 2022

Attention is currently required from: Nikolai Artemiev, Evan Benn. Felix Singer has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/64851 ) Change subject: libflashrom.map: Add all functions from header ...................................................................... Patch Set 1: (2 comments) Commit Message: https://review.coreboot.org/c/flashrom/+/64851/comment/ac02c284_7b06f337 PS1, Line 7: Add all functions from header Suggestion: Add missing functions from libflashrom.h https://review.coreboot.org/c/flashrom/+/64851/comment/88edb69e_bf75e406 PS1, Line 9: I added all : functioned declared in the header. Please use imperative. Suggestion: Thus, add all missing functions from libflashrom.h header file. -- To view, visit https://review.coreboot.org/c/flashrom/+/64851 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ic90b3c20780d3a07b00bfca82d23d44c4fa6f22f Gerrit-Change-Number: 64851 Gerrit-PatchSet: 1 Gerrit-Owner: Evan Benn <evanbenn(a)google.com> Gerrit-Reviewer: Nikolai Artemiev <nartemiev(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Felix Singer <felixsinger(a)posteo.net> Gerrit-Attention: Nikolai Artemiev <nartemiev(a)chromium.org> Gerrit-Attention: Evan Benn <evanbenn(a)google.com> Gerrit-Comment-Date: Wed, 01 Jun 2022 04:19:54 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

Change in flashrom[master]: libflashrom.map: Add all functions from header
by Evan Benn (Code Review) June 1, 2022

June 1, 2022

Attention is currently required from: Nikolai Artemiev. Evan Benn has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/64851 ) Change subject: libflashrom.map: Add all functions from header ...................................................................... Patch Set 1: (1 comment) Patchset: PS1: I think its quite plausible that some of these functioned were intentionally omitted. But I just added all of them and we'll see how the review goes. -- To view, visit https://review.coreboot.org/c/flashrom/+/64851 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ic90b3c20780d3a07b00bfca82d23d44c4fa6f22f Gerrit-Change-Number: 64851 Gerrit-PatchSet: 1 Gerrit-Owner: Evan Benn <evanbenn(a)google.com> Gerrit-Reviewer: Nikolai Artemiev <nartemiev(a)chromium.org> Gerrit-Attention: Nikolai Artemiev <nartemiev(a)chromium.org> Gerrit-Comment-Date: Wed, 01 Jun 2022 03:50:56 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

Change in flashrom[master]: libflashrom.map: Add all functions from header
by Evan Benn (Code Review) June 1, 2022

June 1, 2022

Evan Benn has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/64851 ) Change subject: libflashrom.map: Add all functions from header ...................................................................... libflashrom.map: Add all functions from header Some functions were not being defined, namely flashrom_wp_*. I added all functioned declared in the header. Change-Id: Ic90b3c20780d3a07b00bfca82d23d44c4fa6f22f Signed-off-by: Evan Benn <evanbenn(a)chromium.org> --- M libflashrom.map 1 file changed, 18 insertions(+), 1 deletion(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/51/64851/1 diff --git a/libflashrom.map b/libflashrom.map index b3af70b..e3c7a76 100644 --- a/libflashrom.map +++ b/libflashrom.map @@ -5,17 +5,19 @@ flashrom_data_free; flashrom_flag_get; flashrom_flag_set; - flashrom_flashchip_info; flashrom_flash_erase; flashrom_flash_getsize; flashrom_flash_probe; flashrom_flash_release; + flashrom_flashchip_info; flashrom_image_read; flashrom_image_verify; flashrom_image_write; flashrom_init; + flashrom_layout_add_region; flashrom_layout_get_region_range; flashrom_layout_include_region; + flashrom_layout_new; flashrom_layout_read_fmap_from_buffer; flashrom_layout_read_fmap_from_rom; flashrom_layout_read_from_ifd; @@ -26,6 +28,21 @@ flashrom_set_log_callback; flashrom_set_progress_callback; flashrom_shutdown; + flashrom_supported_boards; + flashrom_supported_chipsets; + flashrom_supported_flash_chips; flashrom_version_info; + flashrom_wp_cfg_new; + flashrom_wp_cfg_release; + flashrom_wp_get_available_ranges; + flashrom_wp_get_mode; + flashrom_wp_get_range; + flashrom_wp_ranges_get_count; + flashrom_wp_ranges_get_range; + flashrom_wp_ranges_release; + flashrom_wp_read_cfg; + flashrom_wp_set_mode; + flashrom_wp_set_range; + flashrom_wp_write_cfg; local: *; }; -- To view, visit https://review.coreboot.org/c/flashrom/+/64851 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ic90b3c20780d3a07b00bfca82d23d44c4fa6f22f Gerrit-Change-Number: 64851 Gerrit-PatchSet: 1 Gerrit-Owner: Evan Benn <evanbenn(a)google.com> Gerrit-MessageType: newchange

1 0

Change in flashrom[master]: flashrom_tester: Use Flashrom trait instead of struct FlashromCmd
by Evan Benn (Code Review) June 1, 2022

June 1, 2022

Attention is currently required from: Peter Marheine. Evan Benn has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/64849 ) Change subject: flashrom_tester: Use Flashrom trait instead of struct FlashromCmd ...................................................................... Patch Set 1: (1 comment) File util/flashrom_tester/src/tester.rs: https://review.coreboot.org/c/flashrom/+/64849/comment/1a9ea5a3_3e22d98e PS1, Line 55: pub cmd: &'a dyn Flashrom, The other option suggested by Peter is to take ownership here, with `pub cmd: Box<dyn Flashrom>` I had a go at this. I'm not expert in rust enough to make it work; many changes to the WriteProtectState lifetime annotations and 'Sized' trait compilation errors make it tricky. -- To view, visit https://review.coreboot.org/c/flashrom/+/64849 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ie2b4e7e91d69043fd50d1c57f6585fc9946fab10 Gerrit-Change-Number: 64849 Gerrit-PatchSet: 1 Gerrit-Owner: Evan Benn <evanbenn(a)google.com> Gerrit-Reviewer: Peter Marheine <pmarheine(a)chromium.org> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-Attention: Peter Marheine <pmarheine(a)chromium.org> Gerrit-Comment-Date: Wed, 01 Jun 2022 03:15:26 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

Change in flashrom[master]: flashrom_tester: Use Flashrom trait instead of struct FlashromCmd
by Evan Benn (Code Review) June 1, 2022

June 1, 2022

Attention is currently required from: Peter Marheine. Evan Benn has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/64849 ) Change subject: flashrom_tester: Use Flashrom trait instead of struct FlashromCmd ...................................................................... Patch Set 1: (1 comment) Patchset: PS1: There is some further WIP work based on this here: https://chromium-review.googlesource.com/c/chromiumos/third_party/flashrom/… -- To view, visit https://review.coreboot.org/c/flashrom/+/64849 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ie2b4e7e91d69043fd50d1c57f6585fc9946fab10 Gerrit-Change-Number: 64849 Gerrit-PatchSet: 1 Gerrit-Owner: Evan Benn <evanbenn(a)google.com> Gerrit-Reviewer: Peter Marheine <pmarheine(a)chromium.org> Gerrit-Attention: Peter Marheine <pmarheine(a)chromium.org> Gerrit-Comment-Date: Wed, 01 Jun 2022 03:11:44 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Gerrit-MessageType: comment

1 0

Change in flashrom[master]: flashrom_tester: Use Flashrom trait instead of struct FlashromCmd
by Evan Benn (Code Review) June 1, 2022

June 1, 2022

Evan Benn has uploaded this change for review. ( https://review.coreboot.org/c/flashrom/+/64849 ) Change subject: flashrom_tester: Use Flashrom trait instead of struct FlashromCmd ...................................................................... flashrom_tester: Use Flashrom trait instead of struct FlashromCmd To allow FlashromCmd to be reimplemented with libflashrom move all functions to the trait, allowing users to deal with the trait instad of the concrete type. Change-Id: Ie2b4e7e91d69043fd50d1c57f6585fc9946fab10 Signed-off-by: Evan Benn <evanbenn(a)chromium.org> --- M util/flashrom_tester/flashrom/src/cmd.rs M util/flashrom_tester/flashrom/src/lib.rs M util/flashrom_tester/src/tester.rs M util/flashrom_tester/src/tests.rs 4 files changed, 311 insertions(+), 296 deletions(-) git pull ssh://review.coreboot.org:29418/flashrom refs/changes/49/64849/1 diff --git a/util/flashrom_tester/flashrom/src/cmd.rs b/util/flashrom_tester/flashrom/src/cmd.rs index 37f8a72..e80b64f 100644 --- a/util/flashrom_tester/flashrom/src/cmd.rs +++ b/util/flashrom_tester/flashrom/src/cmd.rs @@ -33,10 +33,39 @@ // Software Foundation. // -use crate::{FlashChip, FlashromError, FlashromOpt}; +use crate::{FlashChip, FlashromError, ROMWriteSpecifics}; use std::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 flash_name: bool, // --flash-name + pub verbose: bool, // -V +} + +#[derive(Default)] +pub struct WPOpt { + pub range: Option<(i64, i64)>, // --wp-range x0 x1 + pub status: bool, // --wp-status + pub list: bool, // --wp-list + pub enable: bool, // --wp-enable + pub disable: bool, // --wp-disable +} + +#[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 +} + #[derive(PartialEq, Debug)] pub struct FlashromCmd { pub path: String, @@ -65,6 +94,13 @@ } } +impl FlashromCmd { + fn dispatch(&self, fropt: FlashromOpt) -> Result<(Vec<u8>, Vec<u8>), FlashromError> { + let params = flashrom_decode_opts(fropt); + flashrom_dispatch(self.path.as_str(), &params, self.fc) + } +} + impl crate::Flashrom for FlashromCmd { fn get_size(&self) -> Result<i64, FlashromError> { let (stdout, _) = flashrom_dispatch(self.path.as_str(), &["--flash-size"], self.fc)?; @@ -73,9 +109,210 @@ flashrom_extract_size(&sz) } - fn dispatch(&self, fropt: FlashromOpt) -> Result<(Vec<u8>, Vec<u8>), FlashromError> { - let params = flashrom_decode_opts(fropt); - flashrom_dispatch(self.path.as_str(), &params, self.fc) + fn name(&self) -> Result<(String, String), FlashromError> { + let opts = FlashromOpt { + io_opt: IOOpt { + ..Default::default() + }, + + flash_name: true, + + ..Default::default() + }; + + let (stdout, stderr) = self.dispatch(opts)?; + let output = String::from_utf8_lossy(stdout.as_slice()); + let eoutput = String::from_utf8_lossy(stderr.as_slice()); + debug!("name()'stdout: {:#?}.", output); + debug!("name()'stderr: {:#?}.", eoutput); + + match extract_flash_name(&output) { + None => Err("Didn't find chip vendor/name in flashrom output".into()), + Some((vendor, name)) => Ok((vendor.into(), name.into())), + } + } + fn write_file_with_layout(&self, rws: &ROMWriteSpecifics) -> Result<bool, FlashromError> { + let opts = FlashromOpt { + io_opt: IOOpt { + write: rws.write_file, + ..Default::default() + }, + + layout: rws.layout_file, + image: rws.name_file, + + ..Default::default() + }; + + let (stdout, stderr) = self.dispatch(opts)?; + let output = String::from_utf8_lossy(stdout.as_slice()); + let eoutput = String::from_utf8_lossy(stderr.as_slice()); + debug!("write_file_with_layout()'stdout:\n{}.", output); + debug!("write_file_with_layout()'stderr:\n{}.", eoutput); + Ok(true) + } + fn wp_range(&self, range: (i64, i64), wp_enable: bool) -> Result<bool, FlashromError> { + let opts = FlashromOpt { + wp_opt: WPOpt { + range: Some(range), + enable: wp_enable, + ..Default::default() + }, + ..Default::default() + }; + + let (stdout, stderr) = self.dispatch(opts)?; + let output = String::from_utf8_lossy(stdout.as_slice()); + let eoutput = String::from_utf8_lossy(stderr.as_slice()); + debug!("wp_range()'stdout:\n{}.", output); + debug!("wp_range()'stderr:\n{}.", eoutput); + Ok(true) + } + + fn wp_list(&self) -> Result<String, FlashromError> { + let opts = FlashromOpt { + wp_opt: WPOpt { + list: true, + ..Default::default() + }, + ..Default::default() + }; + + let (stdout, _) = self.dispatch(opts)?; + let output = String::from_utf8_lossy(stdout.as_slice()); + if output.len() == 0 { + return Err( + "wp_list isn't supported on platforms using the Linux kernel SPI driver wp_list" + .into(), + ); + } + Ok(output.to_string()) + } + + fn wp_status(&self, en: bool) -> Result<bool, FlashromError> { + let status = if en { "en" } else { "dis" }; + info!("See if chip write protect is {}abled", status); + + let opts = FlashromOpt { + wp_opt: WPOpt { + status: true, + ..Default::default() + }, + ..Default::default() + }; + + let (stdout, _) = self.dispatch(opts)?; + let output = String::from_utf8_lossy(stdout.as_slice()); + + debug!("wp_status():\n{}", output); + + let s = std::format!("write protect is {}abled", status); + Ok(output.contains(&s)) + } + + fn wp_toggle(&self, en: bool) -> Result<bool, FlashromError> { + let status = if en { "en" } else { "dis" }; + + // For MTD, --wp-range and --wp-enable must be used simultaneously. + let range = if en { + let rom_sz: i64 = self.get_size()?; + Some((0, rom_sz)) // (start, len) + } else { + None + }; + + let opts = FlashromOpt { + wp_opt: WPOpt { + range: range, + enable: en, + disable: !en, + ..Default::default() + }, + ..Default::default() + }; + + let (stdout, stderr) = self.dispatch(opts)?; + let output = String::from_utf8_lossy(stdout.as_slice()); + let eoutput = String::from_utf8_lossy(stderr.as_slice()); + + debug!("wp_toggle()'stdout:\n{}.", output); + debug!("wp_toggle()'stderr:\n{}.", eoutput); + + match self.wp_status(true) { + Ok(_ret) => { + info!("Successfully {}abled write-protect", status); + Ok(true) + } + Err(e) => Err(format!("Cannot {}able write-protect: {}", status, e)), + } + } + + fn read(&self, path: &str) -> Result<(), FlashromError> { + let opts = FlashromOpt { + io_opt: IOOpt { + read: Some(path), + ..Default::default() + }, + ..Default::default() + }; + + let (stdout, _) = self.dispatch(opts)?; + let output = String::from_utf8_lossy(stdout.as_slice()); + debug!("read():\n{}", output); + Ok(()) + } + + fn write(&self, path: &str) -> Result<(), FlashromError> { + let opts = FlashromOpt { + io_opt: IOOpt { + write: Some(path), + ..Default::default() + }, + ..Default::default() + }; + + let (stdout, _) = self.dispatch(opts)?; + let output = String::from_utf8_lossy(stdout.as_slice()); + debug!("write():\n{}", output); + Ok(()) + } + + fn verify(&self, path: &str) -> Result<(), FlashromError> { + let opts = FlashromOpt { + io_opt: IOOpt { + verify: Some(path), + ..Default::default() + }, + ..Default::default() + }; + + let (stdout, _) = self.dispatch(opts)?; + let output = String::from_utf8_lossy(stdout.as_slice()); + debug!("verify():\n{}", output); + Ok(()) + } + + fn erase(&self) -> Result<(), FlashromError> { + let opts = FlashromOpt { + io_opt: IOOpt { + erase: true, + ..Default::default() + }, + ..Default::default() + }; + + let (stdout, _) = self.dispatch(opts)?; + let output = String::from_utf8_lossy(stdout.as_slice()); + debug!("erase():\n{}", output); + Ok(()) + } + + fn can_control_hw_wp(&self) -> bool { + self.fc.can_control_hw_wp() + } + + fn programmer_name(&self) -> String { + FlashChip::to(self.fc).to_string() } } @@ -209,6 +446,29 @@ format!("{:#08X},{:#08X}", s, l).to_string() } +/// Get a flash vendor and name from the first matching line of flashrom output. +/// +/// The target line looks like 'vendor="foo" name="bar"', as output by flashrom --flash-name. +/// This is usually the last line of output. +fn extract_flash_name(stdout: &str) -> Option<(&str, &str)> { + for line in stdout.lines() { + if !line.starts_with("vendor=\"") { + continue; + } + + let tail = line.trim_start_matches("vendor=\""); + let mut split = tail.splitn(2, "\" name=\""); + let vendor = split.next(); + let name = split.next().map(|s| s.trim_end_matches('"')); + + match (vendor, name) { + (Some(v), Some(n)) => return Some((v, n)), + _ => continue, + } + } + None +} + #[cfg(test)] mod tests { use super::flashrom_decode_opts; diff --git a/util/flashrom_tester/flashrom/src/lib.rs b/util/flashrom_tester/flashrom/src/lib.rs index 75797d3..e255c9b 100644 --- a/util/flashrom_tester/flashrom/src/lib.rs +++ b/util/flashrom_tester/flashrom/src/lib.rs @@ -83,273 +83,26 @@ pub type FlashromError = String; -#[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 flash_name: bool, // --flash-name - pub verbose: bool, // -V -} - -#[derive(Default)] -pub struct WPOpt { - pub range: Option<(i64, i64)>, // --wp-range x0 x1 - pub status: bool, // --wp-status - pub list: bool, // --wp-list - pub enable: bool, // --wp-enable - pub disable: bool, // --wp-disable -} - -#[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 trait Flashrom { - fn get_size(&self) -> Result<i64, FlashromError>; - fn dispatch(&self, fropt: FlashromOpt) -> Result<(Vec<u8>, Vec<u8>), FlashromError>; -} - -pub fn name(cmd: &cmd::FlashromCmd) -> Result<(String, String), FlashromError> { - let opts = FlashromOpt { - io_opt: IOOpt { - ..Default::default() - }, - - flash_name: true, - - ..Default::default() - }; - - let (stdout, stderr) = cmd.dispatch(opts)?; - let output = String::from_utf8_lossy(stdout.as_slice()); - let eoutput = String::from_utf8_lossy(stderr.as_slice()); - debug!("name()'stdout: {:#?}.", output); - debug!("name()'stderr: {:#?}.", eoutput); - - match extract_flash_name(&output) { - None => Err("Didn't find chip vendor/name in flashrom output".into()), - Some((vendor, name)) => Ok((vendor.into(), name.into())), - } -} - -/// Get a flash vendor and name from the first matching line of flashrom output. -/// -/// The target line looks like 'vendor="foo" name="bar"', as output by flashrom --flash-name. -/// This is usually the last line of output. -fn extract_flash_name(stdout: &str) -> Option<(&str, &str)> { - for line in stdout.lines() { - if !line.starts_with("vendor=\"") { - continue; - } - - let tail = line.trim_start_matches("vendor=\""); - let mut split = tail.splitn(2, "\" name=\""); - let vendor = split.next(); - let name = split.next().map(|s| s.trim_end_matches('"')); - - match (vendor, name) { - (Some(v), Some(n)) => return Some((v, n)), - _ => continue, - } - } - None -} - pub struct ROMWriteSpecifics<'a> { pub layout_file: Option<&'a str>, pub write_file: Option<&'a str>, pub name_file: Option<&'a str>, } -pub fn write_file_with_layout( - cmd: &cmd::FlashromCmd, - rws: &ROMWriteSpecifics, -) -> Result<bool, FlashromError> { - let opts = FlashromOpt { - io_opt: IOOpt { - write: rws.write_file, - ..Default::default() - }, - - layout: rws.layout_file, - image: rws.name_file, - - ..Default::default() - }; - - let (stdout, stderr) = cmd.dispatch(opts)?; - let output = String::from_utf8_lossy(stdout.as_slice()); - let eoutput = String::from_utf8_lossy(stderr.as_slice()); - debug!("write_file_with_layout()'stdout:\n{}.", output); - debug!("write_file_with_layout()'stderr:\n{}.", eoutput); - Ok(true) -} - -pub fn wp_range( - cmd: &cmd::FlashromCmd, - range: (i64, i64), - wp_enable: bool, -) -> Result<bool, FlashromError> { - let opts = FlashromOpt { - wp_opt: WPOpt { - range: Some(range), - enable: wp_enable, - ..Default::default() - }, - ..Default::default() - }; - - let (stdout, stderr) = cmd.dispatch(opts)?; - let output = String::from_utf8_lossy(stdout.as_slice()); - let eoutput = String::from_utf8_lossy(stderr.as_slice()); - debug!("wp_range()'stdout:\n{}.", output); - debug!("wp_range()'stderr:\n{}.", eoutput); - Ok(true) -} - -pub fn wp_list(cmd: &cmd::FlashromCmd) -> Result<String, FlashromError> { - let opts = FlashromOpt { - wp_opt: WPOpt { - list: true, - ..Default::default() - }, - ..Default::default() - }; - - let (stdout, _) = cmd.dispatch(opts)?; - let output = String::from_utf8_lossy(stdout.as_slice()); - if output.len() == 0 { - return Err( - "wp_list isn't supported on platforms using the Linux kernel SPI driver wp_list".into(), - ); - } - Ok(output.to_string()) -} - -pub fn wp_status(cmd: &cmd::FlashromCmd, en: bool) -> Result<bool, FlashromError> { - let status = if en { "en" } else { "dis" }; - info!("See if chip write protect is {}abled", status); - - let opts = FlashromOpt { - wp_opt: WPOpt { - status: true, - ..Default::default() - }, - ..Default::default() - }; - - let (stdout, _) = cmd.dispatch(opts)?; - let output = String::from_utf8_lossy(stdout.as_slice()); - - debug!("wp_status():\n{}", output); - - let s = std::format!("write protect is {}abled", status); - Ok(output.contains(&s)) -} - -pub fn wp_toggle(cmd: &cmd::FlashromCmd, en: bool) -> Result<bool, FlashromError> { - let status = if en { "en" } else { "dis" }; - - // For MTD, --wp-range and --wp-enable must be used simultaneously. - let range = if en { - let rom_sz: i64 = cmd.get_size()?; - Some((0, rom_sz)) // (start, len) - } else { - None - }; - - let opts = FlashromOpt { - wp_opt: WPOpt { - range: range, - enable: en, - disable: !en, - ..Default::default() - }, - ..Default::default() - }; - - let (stdout, stderr) = cmd.dispatch(opts)?; - let output = String::from_utf8_lossy(stdout.as_slice()); - let eoutput = String::from_utf8_lossy(stderr.as_slice()); - - debug!("wp_toggle()'stdout:\n{}.", output); - debug!("wp_toggle()'stderr:\n{}.", eoutput); - - match wp_status(&cmd, true) { - Ok(_ret) => { - info!("Successfully {}abled write-protect", status); - Ok(true) - } - Err(e) => Err(format!("Cannot {}able write-protect: {}", status, e)), - } -} - -pub fn read(cmd: &cmd::FlashromCmd, path: &str) -> Result<(), FlashromError> { - let opts = FlashromOpt { - io_opt: IOOpt { - read: Some(path), - ..Default::default() - }, - ..Default::default() - }; - - let (stdout, _) = cmd.dispatch(opts)?; - let output = String::from_utf8_lossy(stdout.as_slice()); - debug!("read():\n{}", output); - Ok(()) -} - -pub fn write(cmd: &cmd::FlashromCmd, path: &str) -> Result<(), FlashromError> { - let opts = FlashromOpt { - io_opt: IOOpt { - write: Some(path), - ..Default::default() - }, - ..Default::default() - }; - - let (stdout, _) = cmd.dispatch(opts)?; - let output = String::from_utf8_lossy(stdout.as_slice()); - debug!("write():\n{}", output); - Ok(()) -} - -pub fn verify(cmd: &cmd::FlashromCmd, path: &str) -> Result<(), FlashromError> { - let opts = FlashromOpt { - io_opt: IOOpt { - verify: Some(path), - ..Default::default() - }, - ..Default::default() - }; - - let (stdout, _) = cmd.dispatch(opts)?; - let output = String::from_utf8_lossy(stdout.as_slice()); - debug!("verify():\n{}", output); - Ok(()) -} - -pub fn erase(cmd: &cmd::FlashromCmd) -> Result<(), FlashromError> { - let opts = FlashromOpt { - io_opt: IOOpt { - erase: true, - ..Default::default() - }, - ..Default::default() - }; - - let (stdout, _) = cmd.dispatch(opts)?; - let output = String::from_utf8_lossy(stdout.as_slice()); - debug!("erase():\n{}", output); - Ok(()) +pub trait Flashrom { + fn get_size(&self) -> Result<i64, FlashromError>; + fn name(&self) -> Result<(String, String), FlashromError>; + fn write_file_with_layout(&self, rws: &ROMWriteSpecifics) -> Result<bool, FlashromError>; + fn wp_range(&self, range: (i64, i64), wp_enable: bool) -> Result<bool, FlashromError>; + fn wp_list(&self) -> Result<String, FlashromError>; + fn wp_status(&self, en: bool) -> Result<bool, FlashromError>; + fn wp_toggle(&self, en: bool) -> Result<bool, FlashromError>; + fn read(&self, path: &str) -> Result<(), FlashromError>; + fn write(&self, path: &str) -> Result<(), FlashromError>; + fn verify(&self, path: &str) -> Result<(), FlashromError>; + fn erase(&self) -> Result<(), FlashromError>; + fn can_control_hw_wp(&self) -> bool; + fn programmer_name(&self) -> String; } #[cfg(test)] diff --git a/util/flashrom_tester/src/tester.rs b/util/flashrom_tester/src/tester.rs index 3150a43..452f791 100644 --- a/util/flashrom_tester/src/tester.rs +++ b/util/flashrom_tester/src/tester.rs @@ -36,7 +36,7 @@ use super::rand_util; use super::types; use super::utils::{self, LayoutSizes}; -use flashrom::{FlashChip, Flashrom, FlashromCmd}; +use flashrom::{FlashChip, Flashrom}; use serde_json::json; use std::mem::MaybeUninit; use std::sync::atomic::{AtomicBool, Ordering}; @@ -52,7 +52,7 @@ /// /// Where possible, prefer to use methods on the TestEnv rather than delegating /// to the raw flashrom functions. - pub cmd: &'a FlashromCmd, + pub cmd: &'a dyn Flashrom, layout: LayoutSizes, pub wp: WriteProtectState<'a, 'static>, @@ -65,7 +65,7 @@ } impl<'a> TestEnv<'a> { - pub fn create(chip_type: FlashChip, cmd: &'a FlashromCmd) -> Result<Self, String> { + pub fn create(chip_type: FlashChip, cmd: &'a impl Flashrom) -> Result<Self, String> { let rom_sz = cmd.get_size()?; let out = TestEnv { chip_type: chip_type, @@ -77,8 +77,8 @@ }; info!("Stashing golden image for verification/recovery on completion"); - flashrom::read(&out.cmd, &out.original_flash_contents)?; - flashrom::verify(&out.cmd, &out.original_flash_contents)?; + out.cmd.read(&out.original_flash_contents)?; + out.cmd.verify(&out.original_flash_contents)?; info!("Generating random flash-sized data"); rand_util::gen_rand_testdata(&out.random_data, rom_sz as usize) @@ -123,19 +123,19 @@ /// Return true if the current Flash contents are the same as the golden image /// that was present at the start of testing. pub fn is_golden(&self) -> bool { - flashrom::verify(&self.cmd, &self.original_flash_contents).is_ok() + self.cmd.verify(&self.original_flash_contents).is_ok() } /// Do whatever is necessary to make the current Flash contents the same as they /// were at the start of testing. pub fn ensure_golden(&mut self) -> Result<(), String> { self.wp.set_hw(false)?.set_sw(false)?; - flashrom::write(&self.cmd, &self.original_flash_contents) + self.cmd.write(&self.original_flash_contents) } /// Attempt to erase the flash. pub fn erase(&self) -> Result<(), String> { - flashrom::erase(self.cmd) + self.cmd.erase() } /// Verify that the current Flash contents are the same as the file at the given @@ -143,11 +143,11 @@ /// /// Returns Err if they are not the same. pub fn verify(&self, contents_path: &str) -> Result<(), String> { - flashrom::verify(self.cmd, contents_path) + self.cmd.verify(contents_path) } } -impl Drop for TestEnv<'_> { +impl<'a> Drop for TestEnv<'a> { fn drop(&mut self) { info!("Verifying flash remains unmodified"); if !self.is_golden() { @@ -177,7 +177,7 @@ initial: InitialState<'p>, // Tuples are (hardware, software) current: (bool, bool), - cmd: &'a FlashromCmd, + cmd: &'a dyn Flashrom, } enum InitialState<'p> { @@ -199,7 +199,7 @@ /// /// 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 FlashromCmd) -> Result<Self, String> { + pub fn from_hardware(cmd: &'a impl Flashrom) -> Result<Self, String> { let mut lock = Self::get_liveness_lock() .lock() .expect("Somebody panicked during WriteProtectState init from hardware"); @@ -221,8 +221,8 @@ } /// Get the actual hardware write protect state. - fn get_hw(cmd: &FlashromCmd) -> Result<bool, String> { - if cmd.fc.can_control_hw_wp() { + fn get_hw(cmd: &impl Flashrom) -> Result<bool, String> { + if cmd.can_control_hw_wp() { super::utils::get_hardware_wp() } else { Ok(false) @@ -230,8 +230,8 @@ } /// Get the actual software write protect state. - fn get_sw(cmd: &FlashromCmd) -> Result<bool, String> { - flashrom::wp_status(cmd, true) + fn get_sw(cmd: &impl Flashrom) -> Result<bool, String> { + cmd.wp_status(true) } } @@ -241,14 +241,14 @@ /// /// If false, calls to set_hw() will do nothing. pub fn can_control_hw_wp(&self) -> bool { - self.cmd.fc.can_control_hw_wp() + self.cmd.can_control_hw_wp() } /// Set the software write protect. pub fn set_sw(&mut self, enable: bool) -> Result<&mut Self, String> { info!("request={}, current={}", enable, self.current.1); if self.current.1 != enable { - flashrom::wp_toggle(self.cmd, /* en= */ enable)?; + self.cmd.wp_toggle(/* en= */ enable)?; self.current.1 = enable; } Ok(self) @@ -263,7 +263,7 @@ } else if enable { info!( "Ignoring attempt to enable hardware WP with {:?} programmer", - self.cmd.fc + self.cmd.programmer_name() ); } } @@ -376,7 +376,7 @@ ) })?; } - flashrom::wp_toggle(self.cmd, /* en= */ sw).map_err(|e| { + self.cmd.wp_toggle(/* en= */ sw).map_err(|e| { format!( "Failed to {}able software write protect: {}", enable_str(sw), @@ -386,7 +386,7 @@ } assert!( - self.cmd.fc.can_control_hw_wp() || (!self.current.0 && !hw), + 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 { @@ -479,7 +479,7 @@ pub fn run_all_tests<T, TS>( chip: FlashChip, - cmd: &FlashromCmd, + cmd: impl Flashrom, ts: TS, terminate_flag: Option<&AtomicBool>, ) -> Vec<(String, (TestConclusion, Option<TestError>))> @@ -487,7 +487,7 @@ 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).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 86c0da5..e9cf8a5 100644 --- a/util/flashrom_tester/src/tests.rs +++ b/util/flashrom_tester/src/tests.rs @@ -142,18 +142,20 @@ }; let tests = filter_tests(tests, &mut filter_names); + let chip_name = cmd + .name() + .map(|x| format!("vendor=\"{}\" name=\"{}\"", x.0, x.1)) + .unwrap_or("<Unknown chip>".into()); + // ------------------------. // 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); // Any leftover filtered names were specified to be run but don't exist for leftover in filter_names.iter().flatten() { warn!("No test matches filter name \"{}\"", leftover); } - let chip_name = flashrom::name(&cmd) - .map(|x| format!("vendor=\"{}\" name=\"{}\"", x.0, x.1)) - .unwrap_or("<Unknown chip>".into()); let os_rel = sys_info::os_release().unwrap_or("<Unknown OS>".to_string()); let system_info = cros_sysinfo::system_info().unwrap_or("<Unknown System>".to_string()); let bios_info = cros_sysinfo::bios_info().unwrap_or("<Unknown BIOS>".to_string()); @@ -170,7 +172,7 @@ fn get_device_name_test(env: &mut TestEnv) -> TestResult { // Success means we got something back, which is good enough. - flashrom::name(env.cmd)?; + env.cmd.name()?; Ok(()) } @@ -178,7 +180,7 @@ // NOTE: This is not strictly a 'test' as it is allowed to fail on some platforms. // However, we will warn when it does fail. // List the write-protected regions of flash. - match flashrom::wp_list(env.cmd) { + match env.cmd.wp_list() { Ok(list_str) => info!("\n{}", list_str), Err(e) => warn!("{}", e), }; @@ -289,7 +291,7 @@ // Disable software WP so we can do range protection, but hardware WP // must remain enabled for (most) range protection to do anything. env.wp.set_hw(false)?.set_sw(false)?; - flashrom::wp_range(env.cmd, (start, len), true)?; + env.cmd.wp_range((start, len), true)?; env.wp.set_hw(true)?; let rws = flashrom::ROMWriteSpecifics { @@ -297,7 +299,7 @@ write_file: Some(env.random_data_file()), name_file: Some(name), }; - if flashrom::write_file_with_layout(env.cmd, &rws).is_ok() { + if env.cmd.write_file_with_layout(&rws).is_ok() { return Err( "Section should be locked, should not have been overwritable with random data" .into(), -- To view, visit https://review.coreboot.org/c/flashrom/+/64849 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: Ie2b4e7e91d69043fd50d1c57f6585fc9946fab10 Gerrit-Change-Number: 64849 Gerrit-PatchSet: 1 Gerrit-Owner: Evan Benn <evanbenn(a)google.com> Gerrit-MessageType: newchange

1 0

Change in flashrom[master]: programmer: Introduce default shutdown function
by Anastasia Klimchuk (Code Review) June 1, 2022

June 1, 2022

Attention is currently required from: Nico Huber, Edward O'Callaghan, Angel Pons, Joursoir. Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/54890 ) Change subject: programmer: Introduce default shutdown function ...................................................................... Patch Set 6: (1 comment) File flashrom.c: https://review.coreboot.org/c/flashrom/+/54890/comment/4a991178_7bcfd1f7 PS5, Line 104: function; > Do I right understand that you mean checking in register_*_master() and not in > register_shutdown()? Actually I wasn't thinking which place (out of these two) is the best one. But we don't have to decide right now. The question was for future, when (and if) we decide that shutdown function needs to be required - at the moment it is not required. It would be great to summarize this thread... Edward, what do you think about Angel's and my replies above? It was almost a year ago, but I still think in the same way :) There are few other comments, but that's essentially the same question. -- To view, visit https://review.coreboot.org/c/flashrom/+/54890 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I8b20717ba549e12edffbc5d1643ff064a4f0c517 Gerrit-Change-Number: 54890 Gerrit-PatchSet: 6 Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Joursoir <chat(a)joursoir.net> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: build bot (Jenkins) <no-reply(a)coreboot.org> Gerrit-CC: Paul Menzel <paulepanter(a)mailbox.org> Gerrit-CC: Thomas Heijligen <src(a)posteo.de> Gerrit-Attention: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Joursoir <chat(a)joursoir.net> Gerrit-Comment-Date: Wed, 01 Jun 2022 02:23:46 +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> Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org> Comment-In-Reply-To: Joursoir <chat(a)joursoir.net> Gerrit-MessageType: comment

1 0

Change in flashrom[master]: dummyflasher: Wire variable size feature via opaque infra
by Anastasia Klimchuk (Code Review) June 1, 2022

June 1, 2022

Attention is currently required from: Nico Huber, Angel Pons, Nikolai Artemiev. Anastasia Klimchuk has posted comments on this change. ( https://review.coreboot.org/c/flashrom/+/64488 ) Change subject: dummyflasher: Wire variable size feature via opaque infra ...................................................................... Patch Set 4: (2 comments) Commit Message: https://review.coreboot.org/c/flashrom/+/64488/comment/174f2492_e8a1e301 PS2, Line 20: TEST > A comment for myself: check with the users of "variable size" feature to make sure their stuff does […] It's all good! So I am closing this comment. Patchset: PS4: Testing done, and I rebased the patch on the latest. I also ran all the scenarios listed in commit message, and all of them work successfully. So, ready! :) -- To view, visit https://review.coreboot.org/c/flashrom/+/64488 To unsubscribe, or for help writing mail filters, visit https://review.coreboot.org/settings Gerrit-Project: flashrom Gerrit-Branch: master Gerrit-Change-Id: I76402bfdf8b1a75489e4509fec92c9a777d0cf58 Gerrit-Change-Number: 64488 Gerrit-PatchSet: 4 Gerrit-Owner: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-Reviewer: Edward O'Callaghan <quasisec(a)chromium.org> Gerrit-Reviewer: Nico Huber <nico.h(a)gmx.de> Gerrit-Reviewer: Nikolai Artemiev <nartemiev(a)google.com> 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-CC: Angel Pons <th3fanbus(a)gmail.com> Gerrit-CC: Felix Singer <felixsinger(a)posteo.net> Gerrit-CC: Namyoon Woo <namyoon(a)google.com> Gerrit-Attention: Nico Huber <nico.h(a)gmx.de> Gerrit-Attention: Angel Pons <th3fanbus(a)gmail.com> Gerrit-Attention: Nikolai Artemiev <nartemiev(a)google.com> Gerrit-Comment-Date: Wed, 01 Jun 2022 01:33:15 +0000 Gerrit-HasComments: Yes Gerrit-Has-Labels: No Comment-In-Reply-To: Anastasia Klimchuk <aklm(a)chromium.org> Gerrit-MessageType: comment

1 0