commit db76d49cf7740500db4e677046a9175e0b3f173e
author Brandon Kim <brandonkim@google.com> Mon Jun 17 11:53:04 2019 -0700
committer Brandon Kim <brandonkim@google.com> Tue Jun 18 10:27:45 2019 -0700
tree 0e7457c4754ba3302c98f5d5f41c44a1f349072d
parent d0ce7925d77c56c142f64b2c6ae55f0020c06ba8

sensor: Implement RAII object for GPIO unlock and lock

There is a bug where GPIO lock may not be called if there is an
exception after GPIO unlock. Adding an RAII object for GPIO lock
and replacing the current usage of gpioUnlock, gpioLock in
sensor.cpp and mainloop.cpp.

Bug: openbmc/phosphor-hwmon#11
Signed-off-by: Brandon Kim <brandonkim@google.com>
Change-Id: Ica094e716a6ff9dc99165651f83fc496d5ed5a17

mainloop.cpp

diff --git a/mainloop.cpp b/mainloop.cpp
index 7f446a7..f99decb 100644
--- a/mainloop.cpp
+++ b/mainloop.cpp
@@ -397,14 +397,15 @@
                 std::unique_ptr<sensor::Sensor>& sensor =
                     _sensorObjects[i.first];
 
-                sensor->unlockGpio();
+                {
+                    // RAII object for GPIO unlock / lock
+                    sensor::GpioLock gpioLock(sensor->getGpio());
 
-                value = _ioAccess.read(i.first.first, i.first.second, input,
-                                       hwmonio::retries, hwmonio::delay);
+                    value = _ioAccess.read(i.first.first, i.first.second, input,
+                                           hwmonio::retries, hwmonio::delay);
 
-                sensor->lockGpio();
-
-                value = sensor->adjustValue(value);
+                    value = sensor->adjustValue(value);
+                }
 
                 for (auto& iface : obj)
                 {

sensor.cpp

diff --git a/sensor.cpp b/sensor.cpp
index d45f23a..89f8330 100644
--- a/sensor.cpp
+++ b/sensor.cpp
@@ -146,7 +146,8 @@
     // its status is functional, read the input value.
     if (!statusIface || (statusIface && statusIface->functional()))
     {
-        unlockGpio();
+        // RAII object for GPIO unlock / lock
+        GpioLock gpioLock(getGpio());
 
         // Retry for up to a second if device is busy
         // or has a transient error.
@@ -154,7 +155,6 @@
                               hwmon::entry::cinput, std::get<size_t>(retryIO),
                               std::get<std::chrono::milliseconds>(retryIO));
 
-        lockGpio();
         val = adjustValue(val);
     }
 
@@ -243,7 +243,17 @@
     return iface;
 }
 
-void Sensor::unlockGpio()
+GpioLock::GpioLock(const gpioplus::HandleInterface* handle) : _handle(handle)
+{
+    unlockGpio();
+}
+
+GpioLock::~GpioLock()
+{
+    lockGpio();
+}
+
+void GpioLock::unlockGpio()
 {
     if (_handle)
     {
@@ -252,7 +262,7 @@
     }
 }
 
-void Sensor::lockGpio()
+void GpioLock::lockGpio()
 {
     if (_handle)
     {

sensor.hpp

diff --git a/sensor.hpp b/sensor.hpp
index f8069f3..2a53af8 100644
--- a/sensor.hpp
+++ b/sensor.hpp
@@ -105,16 +105,6 @@
     std::shared_ptr<StatusObject> addStatus(ObjectInfo& info);
 
     /**
-     * @brief Unlock the gpio, set to high if relevant.
-     */
-    void unlockGpio();
-
-    /**
-     * @brief Lock the gpio, set to low if relevant.
-     */
-    void lockGpio();
-
-    /**
      * @brief Get the scale from the sensor.
      *
      * @return - Scale value
@@ -124,6 +114,16 @@
         return _scale;
     }
 
+    /**
+     * @brief Get the GPIO handle from the sensor.
+     *
+     * @return - Pointer to the GPIO handle interface, can be nullptr.
+     */
+    inline const gpioplus::HandleInterface* getGpio(void)
+    {
+        return _handle.get();
+    }
+
   private:
     /** @brief Sensor object's identifiers */
     SensorSet::key_type _sensor;
@@ -140,11 +140,52 @@
     /** @brief Optional pointer to GPIO handle. */
     std::unique_ptr<gpioplus::HandleInterface> _handle;
 
-    /** @brief default pause after unlocking gpio. */
-    static constexpr std::chrono::milliseconds _pause{500};
-
     /** @brief sensor scale from configuration. */
     int64_t _scale;
 };
 
+/** @class GpioLock
+ *  @brief RAII class for GPIO unlock and lock
+ *  @details Create this object in the stack to unlock the GPIO. GPIO will
+ *  be locked by the destructor when we go out of scope.
+ */
+class GpioLock
+{
+  public:
+    GpioLock() = delete;
+    GpioLock(const GpioLock&) = delete;
+    GpioLock(GpioLock&&) = default;
+    GpioLock& operator=(const GpioLock&) = delete;
+    GpioLock& operator=(GpioLock&&) = default;
+
+    /**
+     * @brief Constructs GpioLock RAII object. Unlocks the GPIO.
+     *
+     * @param[in] handle - Pointer to a GPIO handle interface, can be nullptr.
+     */
+    explicit GpioLock(const gpioplus::HandleInterface* handle);
+
+    /**
+     * @brief Destructs GpioLock RAII object. Locks the GPIO.
+     */
+    ~GpioLock();
+
+    /**
+     * @brief Unlock the gpio, set to high if relevant.
+     */
+    void unlockGpio();
+
+    /**
+     * @brief Lock the gpio, set to low if relevant.
+     */
+    void lockGpio();
+
+  private:
+    /** @brief Pointer to GPIO handle. */
+    const gpioplus::HandleInterface* _handle;
+
+    /** @brief default pause after unlocking gpio. */
+    static constexpr std::chrono::milliseconds _pause{500};
+};
+
 } // namespace sensor