Edward O'Callaghan has submitted this change. ( https://review.coreboot.org/c/flashrom/+/69417 )
Change subject: flashrom_tester: Change the wp_toggle semantics ......................................................................
flashrom_tester: Change the wp_toggle semantics
wp_toggle and wp_range had some confusing behaviour where enabling wp would set a range, but disabling wp would not unset the range (explicitly). This was a way to workaround the MTD kernel driver semantics. Now make things very explicit, enabling software write protect will set the range to the whole chip. Disabling write protect will set the range to 0,0. This makes all drivers behave the same as MTD, and documents the exact behaviour explicitly.
BUG=b:244663741 BRANCH=None TEST=flashrom_tester --libflashrom host # MTD and non-MTD TEST=flashrom_tester --flashrom_binary # MTD and non-MTD
Change-Id: Ia01d612d988e6580a7c5f0fd448ccc319ce9b181 Signed-off-by: Evan Benn evanbenn@chromium.org Reviewed-on: https://review.coreboot.org/c/flashrom/+/69417 Tested-by: build bot (Jenkins) no-reply@coreboot.org Reviewed-by: Edward O'Callaghan quasisec@chromium.org --- M util/flashrom_tester/flashrom/src/cmd.rs M util/flashrom_tester/flashrom/src/flashromlib.rs M util/flashrom_tester/flashrom/src/lib.rs 3 files changed, 40 insertions(+), 25 deletions(-)
Approvals: build bot (Jenkins): Verified Edward O'Callaghan: Looks good to me, approved
diff --git a/util/flashrom_tester/flashrom/src/cmd.rs b/util/flashrom_tester/flashrom/src/cmd.rs index 4078910..458f054 100644 --- a/util/flashrom_tester/flashrom/src/cmd.rs +++ b/util/flashrom_tester/flashrom/src/cmd.rs @@ -149,11 +149,12 @@ Ok(true) }
- fn wp_range(&self, range: (i64, i64), wp_enable: bool) -> Result<bool, FlashromError> { + fn wp_range(&self, range: (i64, i64), en: bool) -> Result<bool, FlashromError> { let opts = FlashromOpt { wp_opt: WPOpt { + enable: en, + disable: !en, range: Some(range), - enable: wp_enable, ..Default::default() }, ..Default::default() @@ -200,28 +201,14 @@ }
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) + (0, rom_sz) // (start, len) } else { - None + (0, 0) }; - - let opts = FlashromOpt { - wp_opt: WPOpt { - range, - enable: en, - disable: !en, - ..Default::default() - }, - ..Default::default() - }; - - self.dispatch(opts, "wp_toggle")?; - + self.wp_range(range, en)?; + let status = if en { "en" } else { "dis" }; match self.wp_status(true) { Ok(_ret) => { info!("Successfully {}abled write-protect", status); diff --git a/util/flashrom_tester/flashrom/src/flashromlib.rs b/util/flashrom_tester/flashrom/src/flashromlib.rs index 3036394..6cdd55d 100644 --- a/util/flashrom_tester/flashrom/src/flashromlib.rs +++ b/util/flashrom_tester/flashrom/src/flashromlib.rs @@ -102,10 +102,8 @@ }
fn wp_toggle(&self, en: bool) -> Result<bool, FlashromError> { - // TODO why does the cmd impl not do this? - // for cmd, range is only set for enable - // and disable is not sent for the wp_range command - self.wp_range((0, self.get_size()?), en) + let range = if en { (0, self.get_size()?) } else { (0, 0) }; + self.wp_range(range, en) }
fn read_into_file(&self, path: &Path) -> Result<(), FlashromError> { diff --git a/util/flashrom_tester/flashrom/src/lib.rs b/util/flashrom_tester/flashrom/src/lib.rs index 11874f9..90e40e2 100644 --- a/util/flashrom_tester/flashrom/src/lib.rs +++ b/util/flashrom_tester/flashrom/src/lib.rs @@ -133,7 +133,7 @@ /// Write only a region of the flash. fn write_file_with_layout(&self, rws: &ROMWriteSpecifics) -> Result<bool, FlashromError>;
- /// Set write protect status for a range. + /// Set write protect status and range. fn wp_range(&self, range: (i64, i64), wp_enable: bool) -> Result<bool, FlashromError>;
/// Read the write protect regions for the flash. @@ -143,6 +143,10 @@ fn wp_status(&self, en: bool) -> Result<bool, FlashromError>;
/// Set write protect status. + /// If en=true sets wp_range to the whole chip (0,getsize()). + /// If en=false sets wp_range to (0,0). + /// This is due to the MTD driver, which requires wp enable to use a range + /// length != 0 and wp disable to have the range 0,0. fn wp_toggle(&self, en: bool) -> Result<bool, FlashromError>;
/// Read the whole flash to the file specified by `path`.