[flashrom-gerrit] [M] Change in flashrom[master]: flashrom_tester: Check the WP state when setting

[Edward O'Callaghan (Code Review)]

Edward O'Callaghan has submitted this change. ( https://review.coreboot.org/c/flashrom/+/69402 )

(

6 is the latest approved patch-set.
No files were changed between the latest approved patch-set and the submitted one.
 )Change subject: flashrom_tester: Check the WP state when setting
......................................................................

flashrom_tester: Check the WP state when setting

Check that the hardware and software WP state are as expected in the
setter methods.

BUG=b:244663741
BRANCH=None
TEST=flashrom_tester --libflashrom host

Change-Id: Ie7f90ab478dca6f92eaa0908443e3cb156ea0319
Signed-off-by: Evan Benn 
Reviewed-on: https://review.coreboot.org/c/flashrom/+/69402
Reviewed-by: Peter Marheine 
Reviewed-by: Edward O'Callaghan 
Tested-by: build bot (Jenkins) 
---
M util/flashrom_tester/src/tester.rs
1 file changed, 46 insertions(+), 9 deletions(-)

Approvals:
  build bot (Jenkins): Verified
  Edward O'Callaghan: Looks good to me, approved
  Peter Marheine: Looks good to me, but someone else must approve

diff --git a/util/flashrom_tester/src/tester.rs b/util/flashrom_tester/src/tester.rs
index c43c7d2..bedccaf 100644
--- a/util/flashrom_tester/src/tester.rs
+++ b/util/flashrom_tester/src/tester.rs
@@ -229,28 +229,44 @@

/// Set the software write protect and check that the state is as expected.
     pub fn set_sw(&mut self, enable: bool) -> Result<&mut Self, String> {
-        info!("request={}, current={}", enable, self.current.sw);
+        info!("set_sw request={}, current={}", enable, self.current.sw);
         if self.current.sw != enable {
             self.cmd
                 .wp_toggle(/* en= */ enable)
                 .map_err(|e| e.to_string())?;
         }
-        Ok(self)
+        if Self::get_sw(self.cmd).map_err(|e| e.to_string())? != enable {
+            Err(format!(
+                "Software write protect did not change state to {} when requested",
+                enable
+            ))
+        } else {
+            self.current.sw = enable;
+            Ok(self)
+        }
     }

-    /// Set the hardware write protect.
+    /// Set the hardware write protect if supported and check that the state is as expected.
     pub fn set_hw(&mut self, enable: bool) -> Result<&mut Self, String> {
+        info!("set_hw request={}, current={}", enable, self.current.hw);
         if self.can_control_hw_wp() {
             if self.current.hw != enable {
                 super::utils::toggle_hw_wp(/* dis= */ !enable)?;
-                self.current.hw = enable;
-            } else if enable {
-                info!(
-                    "Ignoring attempt to enable hardware WP with {:?} programmer",
-                    self.fc
-                );
             }
+            // toggle_hw_wp does check this, but we might not have called toggle_hw_wp so check again.
+            if Self::get_hw(self.cmd)? != enable {
+                return Err(format!(
+                    "Hardware write protect did not change state to {} when requested",
+                    enable
+                ));
+            }
+        } else {
+            info!(
+                "Ignoring attempt to set hardware WP with {:?} programmer",
+                self.fc
+            );
         }
+        self.current.hw = enable;
         Ok(self)
     }