Evan Benn has uploaded this change for review.

View Change

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>
---
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, 34 insertions(+), 24 deletions(-)

git pull ssh://review.coreboot.org:29418/flashrom refs/changes/17/69417/1

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..c948564 100644
--- a/util/flashrom_tester/flashrom/src/flashromlib.rs
+++ b/util/flashrom_tester/flashrom/src/flashromlib.rs
@@ -102,9 +102,6 @@
     }
 
     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)
     }
 
diff --git a/util/flashrom_tester/flashrom/src/lib.rs b/util/flashrom_tester/flashrom/src/lib.rs
index 11874f9..f84dea5 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,9 @@
     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 the only supported behaviour from MTD
     fn wp_toggle(&self, en: bool) -> Result<bool, FlashromError>;
 
     /// Read the whole flash to the file specified by `path`.

To view, visit change 69417. To unsubscribe, or for help writing mail filters, visit settings.