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>

3 files changed, 14 insertions, 25 deletions

diff --git a/util/flashrom_tester/flashrom/src/cmd.rs b/util/flashrom_tester/flashrom/src/cmd.rs
index 40789100..458f0540 100644
--- a/util/flashrom_tester/flashrom/src/cmd.rs
+++ b/util/flashrom_tester/flashrom/src/cmd.rs
@@ -149,11 +149,12 @@ impl crate::Flashrom for FlashromCmd {
         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 @@ impl crate::Flashrom for FlashromCmd {
     }
 
     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 3036394d..6cdd55d0 100644
--- a/util/flashrom_tester/flashrom/src/flashromlib.rs
+++ b/util/flashrom_tester/flashrom/src/flashromlib.rs
@@ -102,10 +102,8 @@ impl crate::Flashrom for FlashromLib {
     }
 
     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 11874f91..90e40e2c 100644
--- a/util/flashrom_tester/flashrom/src/lib.rs
+++ b/util/flashrom_tester/flashrom/src/lib.rs
@@ -133,7 +133,7 @@ pub trait Flashrom {
     /// 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 @@ pub trait Flashrom {
     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`.