Fix D-Bus error from callback method invoked from async thread
Noticed D-bus method invoked as part of the callback method
invoked from async thread returns error
Switching to use sd_event loop timer for callback after timer
expiry
Resolves openbmc/openbmc#3130
Change-Id: Ibe87a6b3aa179cc887593c7dea635c11d9ea844c
Signed-off-by: Marri Devender Rao <devenrao@in.ibm.com>
diff --git a/chassishandler.cpp b/chassishandler.cpp
index 2e892b9..3092e7c 100644
--- a/chassishandler.cpp
+++ b/chassishandler.cpp
@@ -36,13 +36,15 @@
#include <xyz/openbmc_project/State/PowerOnHours/server.hpp>
#include "config.h"
-
+#include "timer.hpp"
//Defines
#define SET_PARM_VERSION 0x01
#define SET_PARM_BOOT_FLAGS_PERMANENT 0x40 //boot flags data1 7th bit on
#define SET_PARM_BOOT_FLAGS_VALID_ONE_TIME 0x80 //boot flags data1 8th bit on
#define SET_PARM_BOOT_FLAGS_VALID_PERMANENT 0xC0 //boot flags data1 7 & 8 bit on
+std::unique_ptr<phosphor::ipmi::Timer> identifyTimer = nullptr;
+
constexpr size_t SIZE_MAC = 18;
constexpr size_t SIZE_BOOT_OPTION = (uint8_t)BootOptionResponseSize::
OPAL_NETWORK_SETTINGS;//Maximum size of the boot option parametrs
@@ -63,6 +65,10 @@
static constexpr size_t ADDRTYPE_OFFSET = 16;
static constexpr size_t IPADDR_OFFSET = 17;
+static constexpr size_t encIdentifyObjectsSize = 1;
+static constexpr size_t chassisIdentifyReqLength = 2;
+static constexpr size_t identifyIntervalPos = 0;
+static constexpr size_t forceIdentifyPos = 1;
void register_netfn_chassis_functions() __attribute__((constructor));
@@ -1073,32 +1079,13 @@
return ( (rc < 0) ? IPMI_CC_INVALID : IPMI_CC_OK);
}
-ipmi_ret_t ipmi_chassis_identify(ipmi_netfn_t netfn, ipmi_cmd_t cmd,
- ipmi_request_t request,
- ipmi_response_t response,
- ipmi_data_len_t data_len,
- ipmi_context_t context)
+/** @brief Return D-Bus connection string to enclosure identify LED object
+ *
+ * @param[in, out] connection - connection to D-Bus object
+ * @return a IPMI return code
+ */
+std::string getEnclosureIdentifyConnection()
{
- static std::atomic_size_t currentCallerId(0);
- static std::unique_ptr<std::future<void>> future;
- static std::condition_variable condition;
- static std::mutex timeoutMutex;
-
- if (*data_len > 2)
- {
- return IPMI_CC_REQ_DATA_LEN_INVALID;
- }
- uint8_t identifyInterval = *data_len > 0 ?
- (static_cast<uint8_t*>(request))[0] :
- DEFAULT_IDENTIFY_TIME_OUT;
- bool forceIdentify =
- *data_len == 2 ? (static_cast<uint8_t*>(request))[1] & 0x01 : false;
-
- currentCallerId++;
-
- // stop any threads currently running
- condition.notify_all();
-
// lookup enclosure_identify group owner(s) in mapper
auto mapperCall = chassis::internal::dbus.new_method_call(
ipmi::MAPPER_BUS_NAME,
@@ -1116,64 +1103,106 @@
if (mapperReply.is_method_error())
{
log<level::ERR>("Chassis Identify: Error communicating to mapper.");
- return IPMI_CC_RESPONSE_ERROR;
+ elog<InternalFailure>();
}
std::vector<std::pair<std::string, std::vector<std::string>>> mapperResp;
mapperReply.read(mapperResp);
- for (auto& object : mapperResp)
+ if (mapperResp.size() != encIdentifyObjectsSize)
{
- std::string& connection = object.first;
+ log<level::ERR>("Invalid number of enclosure identify objects.",
+ entry("ENC_IDENTITY_OBJECTS_SIZE=%d", mapperResp.size()));
+ elog<InternalFailure>();
+ }
+ auto pair = mapperResp[encIdentifyObjectsSize - 1];
+ return pair.first;
+}
- if (identifyInterval || forceIdentify)
+/** @brief Turn On/Off enclosure identify LED
+ *
+ * @param[in] flag - true to turn on LED, false to turn off
+ * @return a IPMI return code
+ */
+void enclosureIdentifyLed(bool flag)
+{
+ using namespace chassis::internal;
+ std::string connection = std::move(getEnclosureIdentifyConnection());
+ auto led = dbus.new_method_call(connection.c_str(),
+ identify_led_object_name, "org.freedesktop.DBus.Properties", "Set");
+ led.append("xyz.openbmc_project.Led.Group", "Asserted",
+ sdbusplus::message::variant<bool>(flag));
+ auto ledReply = dbus.call(led);
+ if (ledReply.is_method_error())
+ {
+ log<level::ERR>("Chassis Identify: Error Setting State On/Off\n",
+ entry("LED_STATE=%d", flag));
+ elog<InternalFailure>();
+ }
+}
+
+/** @brief Callback method to turn off LED
+ */
+void enclosureIdentifyLedOff()
+{
+ try
+ {
+ enclosureIdentifyLed(false);
+ }
+ catch (const InternalFailure& e)
+ {
+ report<InternalFailure>();
+ }
+}
+
+/** @brief Create timer to turn on and off the enclosure LED
+ */
+void createIdentifyTimer()
+{
+ if (!identifyTimer)
+ {
+ identifyTimer = std::make_unique<phosphor::ipmi::Timer>(
+ ipmid_get_sd_event_connection(), enclosureIdentifyLedOff);
+ }
+}
+
+ipmi_ret_t ipmi_chassis_identify(ipmi_netfn_t netfn, ipmi_cmd_t cmd,
+ ipmi_request_t request,
+ ipmi_response_t response,
+ ipmi_data_len_t data_len,
+ ipmi_context_t context)
+{
+ if (*data_len > chassisIdentifyReqLength)
+ {
+ return IPMI_CC_REQ_DATA_LEN_INVALID;
+ }
+ uint8_t identifyInterval = *data_len > identifyIntervalPos ?
+ (static_cast<uint8_t*>(request))[identifyIntervalPos] :
+ DEFAULT_IDENTIFY_TIME_OUT;
+ bool forceIdentify = (*data_len == chassisIdentifyReqLength) ?
+ (static_cast<uint8_t*>(request))[forceIdentifyPos] & 0x01 : false;
+ if (identifyInterval || forceIdentify)
+ {
+ // stop the timer if already started, for force identify we should
+ // not turn off LED
+ identifyTimer->setTimer(SD_EVENT_OFF);
+ try
{
- auto ledOn = chassis::internal::dbus.new_method_call(
- connection.c_str(),
- identify_led_object_name,
- "org.freedesktop.DBus.Properties", "Set");
- ledOn.append(
- "xyz.openbmc_project.Led.Group", "Asserted",
- sdbusplus::message::variant<bool>(
- true));
- auto ledReply = chassis::internal::dbus.call(ledOn);
- if (ledReply.is_method_error())
- {
- log<level::ERR>("Chassis Identify: Error Setting State On\n");
- return IPMI_CC_RESPONSE_ERROR;
- }
- if (forceIdentify)
- {
- return IPMI_CC_OK;
- }
+ enclosureIdentifyLed(true);
+ }
+ catch (const InternalFailure& e)
+ {
+ report<InternalFailure>();
+ return IPMI_CC_RESPONSE_ERROR;
}
- size_t threadCallerId = currentCallerId;
- future = std::make_unique<std::future<void>>(
- std::async(std::launch::async,
- [connection,
- identifyInterval,
- threadCallerId]
+ if (forceIdentify)
{
- std::unique_lock<std::mutex> lock(timeoutMutex);
- if (condition.wait_for(lock,
- std::chrono::seconds(identifyInterval),
- [&threadCallerId]{return currentCallerId != threadCallerId;}))
- {
- return; // another thread started.
- }
- auto ledOff = chassis::internal::dbus.new_method_call(
- connection.c_str(),
- identify_led_object_name,
- "org.freedesktop.DBus.Properties", "Set");
- ledOff.append("xyz.openbmc_project.Led.Group", "Asserted",
- sdbusplus::message::variant<bool>(
- false));
- auto ledReply = chassis::internal::dbus.call(ledOff);
- if (ledReply.is_method_error())
- {
- log<level::ERR>("Chassis Identify: Error Setting State Off\n");
- }
- }));
+ return IPMI_CC_OK;
+ }
+ // start the timer
+ auto time = std::chrono::duration_cast<std::chrono::microseconds>(
+ std::chrono::seconds(identifyInterval));
+ identifyTimer->startTimer(time);
}
return IPMI_CC_OK;
}
@@ -1575,6 +1604,8 @@
void register_netfn_chassis_functions()
{
+ createIdentifyTimer();
+
// <Wildcard Command>
ipmi_register_callback(NETFUN_CHASSIS, IPMI_CMD_WILDCARD, NULL, ipmi_chassis_wildcard,
PRIVILEGE_USER);