gpio-health-monitor: Patch to fix crash issue
This patch fixes a crash that occurred when multiple GPIO lines asserted
simultaneously by refactoring D-Bus handling to use one shared
connection instead of creating new ones for every event. It also prevents
GPIO line handle leaks by adding missing gpiod_line_release calls in the
destructor and relevant error paths.
0003-phosphor-health-gpio-monitor-Use-shared-D-Bus-connec.patch
Tested: details in the patch.
Fusion-Link: fusion2 N/A
Google-Bug-Id: 427175539
Change-Id: Ib72c70c9750d9a528b593ceeba064f0202035c2b
Signed-off-by: Vikram Gara <vikramgara@google.com>
diff --git a/recipes-phosphor/gpio/health-monitor/0003-phosphor-health-gpio-monitor-Use-shared-D-Bus-connec.patch b/recipes-phosphor/gpio/health-monitor/0003-phosphor-health-gpio-monitor-Use-shared-D-Bus-connec.patch
new file mode 100644
index 0000000..3639319
--- /dev/null
+++ b/recipes-phosphor/gpio/health-monitor/0003-phosphor-health-gpio-monitor-Use-shared-D-Bus-connec.patch
@@ -0,0 +1,246 @@
+From 3c878b9f7759b74a310b13ddabb591937e381f47 Mon Sep 17 00:00:00 2001
+From: Vikram Gara <vikramgara@google.com>
+Date: Tue, 2 Sep 2025 22:12:01 +0000
+Subject: [PATCH] phosphor-health-gpio-monitor: Use shared D-Bus connection to
+ fix crash
+
+The previous implementation created a new `boost::asio::io_context` and
+`sdbusplus::asio::connection` within the `gpioEventHandler` for every
+GPIO event. This is inefficient and leads to a crash, likely due to
+resource exhaustion or race conditions.
+
+This patch refactors the D-Bus connection handling to fix the crash
+and improve resource management:
+
+- A single `sdbusplus::asio::connection` is now created as a
+ `std::shared_ptr` in `main()` and is shared across all `GpioMonitor`
+ instances.
+- The `GpioMonitor` class now stores this shared pointer and uses it
+ for making asynchronous D-Bus calls, avoiding the creation of new
+ connections in the event handler.
+- The D-Bus method to update the health status is now only called when
+ the health state actually changes, reducing unnecessary D-Bus traffic.
+
+Additionally, this patch includes resource management improvements:
+- A destructor is added to `GpioMonitor` to ensure `gpiod_line_release`
+ is called, preventing GPIO line handle leaks.
+- A missing `gpiod_line_release` call in an error path within
+ `createHealthObject` is added.
+- Add mutex lock to prevent race condition between main loop and D-Bus
+ signal handler when creating HealthMonitor objects
+
+Tested:
+ Simulated two gpio line assertions at the same time using mimic
+ did not see any crash with the change.
+ http://paste.googleplex.com/4763735394746368#l=199
+Signed-off-by: Vikram Gara <vikramgara@google.com>
+---
+ gpioHealthMon.cpp | 36 +++++++++++++++++-------------------
+ gpioHealthMon.hpp | 25 ++++++++++++++++++++-----
+ gpioHealthMonMain.cpp | 18 +++++++++++-------
+ 3 files changed, 48 insertions(+), 31 deletions(-)
+
+diff --git a/gpioHealthMon.cpp b/gpioHealthMon.cpp
+index b7e90d4..99503ff 100644
+--- a/gpioHealthMon.cpp
++++ b/gpioHealthMon.cpp
+@@ -17,7 +17,6 @@
+ #include "gpioHealthMon.hpp"
+
+ #include <boost/asio/io_context.hpp>
+-#include <sdbusplus/asio/connection.hpp>
+ #include <sdbusplus/bus.hpp>
+
+ #include <iostream>
+@@ -65,24 +64,23 @@ void GpioMonitor::gpioEventHandler()
+ lg2::info("{GPIO} Deasserted", "GPIO", gpioLineMsg);
+ isRising = false;
+ }
+- currentHealth = (isRising == healthyPolarity);
+-
+- // Creating io_context
+- boost::asio::io_context io;
+-
+- // Creating shared connection
+- auto conn = sdbusplus::asio::connection(io);
+-
+- // Making the async D-Bus method call
+- conn.async_method_call([](boost::system::error_code ec) {
+- if (ec)
+- {
+- lg2::error("error with async_method_call: {ERROR}", "ERROR",
+- ec.message());
+- return;
+- }
+- }, phosphor::gpio::DBUS_SERVICE, objectPath, DBUS_INTERFACE,
+- "updateHealthStatus", currentHealth);
++
++ bool newHealth = (isRising == healthyPolarity);
++ if (newHealth != currentHealth)
++ {
++ currentHealth = newHealth;
++ // Making the async D-Bus method call
++ conn->async_method_call(
++ [](boost::system::error_code ec) {
++ if (ec)
++ {
++ lg2::error("error with async_method_call: {ERROR}", "ERROR",
++ ec.message());
++ }
++ },
++ phosphor::gpio::DBUS_SERVICE, objectPath, DBUS_INTERFACE,
++ "updateHealthStatus", currentHealth);
++ }
+
+ /* Schedule a wait event */
+ scheduleEventHandler();
+diff --git a/gpioHealthMon.hpp b/gpioHealthMon.hpp
+index 894a817..72e7c13 100644
+--- a/gpioHealthMon.hpp
++++ b/gpioHealthMon.hpp
+@@ -4,9 +4,11 @@
+
+ #include <boost/asio/io_context.hpp>
+ #include <boost/asio/posix/stream_descriptor.hpp>
++#include <sdbusplus/asio/connection.hpp>
+ #include <phosphor-logging/lg2.hpp>
+
+ #include <map>
++#include <memory>
+ #include <vector>
+
+ namespace phosphor
+@@ -26,7 +28,14 @@ class GpioMonitor
+ {
+ public:
+ GpioMonitor() = delete;
+- ~GpioMonitor() = default;
++ ~GpioMonitor()
++ {
++ if (gpioLine)
++ {
++ // Release the GPIO line
++ gpiod_line_release(gpioLine);
++ }
++ }
+ GpioMonitor(const GpioMonitor&) = delete;
+ GpioMonitor& operator=(const GpioMonitor&) = delete;
+ GpioMonitor(GpioMonitor&&) = delete;
+@@ -35,18 +44,21 @@ class GpioMonitor
+ /** @brief Constructs GpioMonitor object.
+ *
+ * @param[in] line - GPIO line from libgpiod
+- * @param[in] config - configuration of line with event
+- * @param[in] io - io service
++ * @param[in] config - Configuration of line with event
++ * @param[in] conn - D-Bus connection
+ * @param[in] lineMsg - GPIO line message to be used for log
+ * @param[in] objPath - D-Bus service path for this GPIO
+ * @param[in] healthyPolarity - Definition of healthy event
+ * @param[in] currentHealth - Current GPIO status
++ * @param[in] entityPath - Entity path for the GPIO
+ */
+ GpioMonitor(gpiod_line* line, gpiod_line_request_config& config,
+- boost::asio::io_context& io, const std::string& lineMsg,
++ const std::shared_ptr<sdbusplus::asio::connection>& conn,
++ const std::string& lineMsg,
+ std::string& objPath, bool& healthyPolarity,
+ bool& currentHealth, std::string entityPath) :
+- gpioLine(line), gpioConfig(config), gpioEventDescriptor(io),
++ gpioLine(line), gpioConfig(config),
++ gpioEventDescriptor(conn->get_io_context()), conn(conn),
+ gpioLineMsg(lineMsg), objectPath(objPath),
+ healthyPolarity(healthyPolarity), currentHealth(currentHealth),
+ entityPath(entityPath)
+@@ -69,6 +81,9 @@ class GpioMonitor
+ /** @brief GPIO event descriptor */
+ boost::asio::posix::stream_descriptor gpioEventDescriptor;
+
++ /** @brief D-Bus connection. */
++ std::shared_ptr<sdbusplus::asio::connection> conn;
++
+ /** @brief GPIO line name message */
+ std::string gpioLineMsg;
+
+diff --git a/gpioHealthMonMain.cpp b/gpioHealthMonMain.cpp
+index 1d84d5d..f26dc75 100644
+--- a/gpioHealthMonMain.cpp
++++ b/gpioHealthMonMain.cpp
+@@ -108,7 +108,8 @@ std::set<std::string> exist;
+ std::mutex handlerMutex;
+
+ bool createHealthObject(sdbusplus::asio::connection& conn,
+- const std::string& objPath, sdbusplus::bus_t& bus)
++ const std::string& objPath, sdbusplus::bus_t& bus,
++ const std::shared_ptr<sdbusplus::asio::connection>& sharedConn)
+ {
+ /* Call GetAll method call */
+ std::vector<std::string> targetInterfaces = {
+@@ -186,8 +187,10 @@ bool createHealthObject(sdbusplus::asio::connection& conn,
+ if (gpioValue < 0)
+ {
+ lg2::error("Failed to read current GPIO value");
++ gpiod_line_release(line);
+ return false;
+ }
++
+ gpiod_line_release(line);
+
+ /* Get event to be monitored, if it is not defined then
+@@ -212,7 +215,7 @@ bool createHealthObject(sdbusplus::asio::connection& conn,
+ limit = std::get<uint64_t>(properties["LimitMonitoring"]);
+
+ gpios.insert(std::make_unique<phosphor::gpio::GpioMonitor>(
+- line, config, conn.get_io_context(), lineMsg, gpioObjectPath,
++ line, config, sharedConn, lineMsg, gpioObjectPath,
+ healthyPolarity, currentHealthy, objPath));
+
+ dbusObjects.push_back(std::make_unique<GetStatus>(
+@@ -221,20 +224,20 @@ bool createHealthObject(sdbusplus::asio::connection& conn,
+ }
+
+ std::unique_ptr<sdbusplus::bus::match_t>
+- setupPropertiesChangedMatches(sdbusplus::asio::connection& conn, sdbusplus::bus_t& bus)
++ setupPropertiesChangedMatches(const std::shared_ptr<sdbusplus::asio::connection>& conn, sdbusplus::bus_t& bus)
+ {
+ lg2::info("Begin to listen");
+ std::string match_expression =
+ "type='signal',member='PropertiesChanged',path_namespace='/xyz/openbmc_project/inventory',arg0namespace='xyz.openbmc_project.Configuration.HealthMonitor'";
+ auto match = std::make_unique<sdbusplus::bus::match_t>(
+ bus, match_expression,
+- [&conn, &bus](sdbusplus::message_t& message) {
++ [conn, &bus](sdbusplus::message_t& message) {
+ std::lock_guard<std::mutex> lock(handlerMutex);
+ const auto* path = message.get_path();
+ if (exist.find(path) != exist.end())
+ return;
+ exist.insert(path);
+- if (createHealthObject(conn, path, bus) == false)
++ if (createHealthObject(*conn, path, bus, conn) == false)
+ {
+ lg2::error("Failed to create health object for {NAME}.", "NAME",
+ path);
+@@ -310,17 +313,18 @@ int main()
+
+ for (const auto& objPath : objPaths)
+ {
++ std::lock_guard<std::mutex> lock(handlerMutex);
+ if (exist.find(objPath) != exist.end())
+ continue;
+ exist.insert(objPath);
+- if (createHealthObject(*conn, objPath, bus) == false)
++ if (createHealthObject(*conn, objPath, bus, conn) == false)
+ {
+ lg2::error("Failed to create health object for {NAME}.", "NAME",
+ objPath);
+ continue;
+ }
+ }
+- auto match = setupPropertiesChangedMatches(*conn, bus);
++ auto match = setupPropertiesChangedMatches(conn, bus);
+ io.run();
+
+ return 0;
+--
+2.51.0.384.g4c02a37b29-goog
+
