From 6706c1cc30f4f98040d6628f9f15c4513eef2d21 Mon Sep 17 00:00:00 2001 From: Marri Devender Rao Date: Mon, 14 May 2018 00:29:38 -0500 Subject: 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 --- chassishandler.cpp | 179 +++++++++++++++++++++++++++++++---------------------- 1 file changed, 105 insertions(+), 74 deletions(-) diff --git a/chassishandler.cpp b/chassishandler.cpp index 2e892b9..3092e7c 100644 --- a/chassishandler.cpp +++ b/chassishandler.cpp @@ -36,13 +36,15 @@ #include #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 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 MAC_OFFSET = 9; 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 @@ ipmi_ret_t ipmi_chassis_control(ipmi_netfn_t netfn, ipmi_cmd_t cmd, 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> 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(request))[0] : - DEFAULT_IDENTIFY_TIME_OUT; - bool forceIdentify = - *data_len == 2 ? (static_cast(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 @@ ipmi_ret_t ipmi_chassis_identify(ipmi_netfn_t netfn, ipmi_cmd_t cmd, if (mapperReply.is_method_error()) { log("Chassis Identify: Error communicating to mapper."); - return IPMI_CC_RESPONSE_ERROR; + elog(); } std::vector>> mapperResp; mapperReply.read(mapperResp); - for (auto& object : mapperResp) + if (mapperResp.size() != encIdentifyObjectsSize) + { + log("Invalid number of enclosure identify objects.", + entry("ENC_IDENTITY_OBJECTS_SIZE=%d", mapperResp.size())); + elog(); + } + auto pair = mapperResp[encIdentifyObjectsSize - 1]; + return pair.first; +} + +/** @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(flag)); + auto ledReply = dbus.call(led); + if (ledReply.is_method_error()) + { + log("Chassis Identify: Error Setting State On/Off\n", + entry("LED_STATE=%d", flag)); + elog(); + } +} + +/** @brief Callback method to turn off LED + */ +void enclosureIdentifyLedOff() +{ + try + { + enclosureIdentifyLed(false); + } + catch (const InternalFailure& e) + { + report(); + } +} + +/** @brief Create timer to turn on and off the enclosure LED + */ +void createIdentifyTimer() +{ + if (!identifyTimer) { - std::string& connection = object.first; + identifyTimer = std::make_unique( + ipmid_get_sd_event_connection(), enclosureIdentifyLedOff); + } +} - if (identifyInterval || forceIdentify) +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(request))[identifyIntervalPos] : + DEFAULT_IDENTIFY_TIME_OUT; + bool forceIdentify = (*data_len == chassisIdentifyReqLength) ? + (static_cast(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( - true)); - auto ledReply = chassis::internal::dbus.call(ledOn); - if (ledReply.is_method_error()) - { - log("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(); + return IPMI_CC_RESPONSE_ERROR; } - size_t threadCallerId = currentCallerId; - future = std::make_unique>( - std::async(std::launch::async, - [connection, - identifyInterval, - threadCallerId] + if (forceIdentify) { - std::unique_lock 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( - false)); - auto ledReply = chassis::internal::dbus.call(ledOff); - if (ledReply.is_method_error()) - { - log("Chassis Identify: Error Setting State Off\n"); - } - })); + return IPMI_CC_OK; + } + // start the timer + auto time = std::chrono::duration_cast( + std::chrono::seconds(identifyInterval)); + identifyTimer->startTimer(time); } return IPMI_CC_OK; } @@ -1575,6 +1604,8 @@ ipmi_ret_t ipmiGetPOHCounter(ipmi_netfn_t netfn, ipmi_cmd_t cmd, void register_netfn_chassis_functions() { + createIdentifyTimer(); + // ipmi_register_callback(NETFUN_CHASSIS, IPMI_CMD_WILDCARD, NULL, ipmi_chassis_wildcard, PRIVILEGE_USER); -- cgit v1.2.1