nvmesensor: implement deferred setup and add unit tests Implement deferred MCTP setup in MctpReactorDevice to handle out-of-order events between Entity Manager and mctpd. Add callback mechanism in MctpUtil to notify about endpoint events. Refine I2C extraction in NVMeSensorMain.cpp. Add unit tests in test_MctpUtil.cpp to verify deferred setup and removal notifications. Update tests/meson.build to link against required sources. Tested: Verified by running unit tests in Docker (all 14 tests passed). Google-Bug-Id: 505231613 Change-Id: Id6b86e400071aefbc5221e64fbb75ab3d5f4207c
diff --git a/src/MctpReactorDevice.cpp b/src/MctpReactorDevice.cpp index 3b979b8..123ca4a 100644 --- a/src/MctpReactorDevice.cpp +++ b/src/MctpReactorDevice.cpp
@@ -122,25 +122,71 @@ MctpReactorDevice::MctpReactorDevice( const std::shared_ptr<sdbusplus::asio::connection>& connection, - const std::string& deviceObjectPath, const std::string& endpointPath, + const std::string& deviceObjectPath, const BusInfo& busInfo, std::optional<int> busNumber, std::optional<int> address) : connection(connection), deviceObjectPath(deviceObjectPath), - resolvedEndpointPath(endpointPath), prettyObjectName(extractSuffixFromObjectPath(deviceObjectPath)), isI2cAccessible(busNumber.has_value() && address.has_value()), i2cBusNumber(isI2cAccessible ? busNumber.value() : -1), - i2cAddress(isI2cAccessible ? address.value() : -1) + i2cAddress(isI2cAccessible ? address.value() : -1), targetBusInfo(busInfo) {} void MctpReactorDevice::setup( std::function<void(const std::error_code& ec, const std::shared_ptr<MctpEndpoint>& ep)>&& action) { - if (!resolvedEndpointPath.empty()) + if (!targetBusInfo.empty()) { - finaliseEndpoint(resolvedEndpointPath, std::move(action)); + if (!callbackToken) + { + auto weak = weak_from_this(); + callbackToken = registerMctpEndpointCallback( + [weak](const std::string& epPath, + const SensorData& epConfig [[maybe_unused]], + bool isRemoved) { + auto self = weak.lock(); + if (!self) + { + return; + } + + if (isRemoved) + { + if (self->resolvedEndpointPath == epPath) + { + self->endpointRemoved(); + } + } + else + { + if (self->savedAction) + { + self->setup(std::move(self->savedAction)); + } + } + }); + } + + for (const auto& [epPath, epConfig] : mctpEndpointConfigMap) + { + BusInfo epBusInfo = extractBusInfo(epConfig); + if (epBusInfo == targetBusInfo) + { + finaliseEndpoint(epPath, std::move(action)); + return; + } + } + + if (savedAction) + { + savedAction(std::make_error_code(std::errc::operation_canceled), + nullptr); + } + savedAction = std::move(action); return; } + + // Fallback to old static path auto onGetPropertyReturned = [weak{weak_from_this()}, action{std::move(action)}]( const boost::system::error_code& ec, @@ -184,12 +230,27 @@ } } +MctpReactorDevice::~MctpReactorDevice() +{ + remove(); +} + void MctpReactorDevice::remove() { - if (endpoint) + if (callbackToken) { - endpoint->remove(); + unregisterMctpEndpointCallback(*callbackToken); + callbackToken.reset(); } + + if (savedAction) + { + savedAction(std::make_error_code(std::errc::operation_canceled), + nullptr); + savedAction = nullptr; + } + + endpointRemoved(); } std::string MctpReactorDevice::describe() const @@ -224,14 +285,19 @@ std::function<void(const std::error_code& ec, const std::shared_ptr<MctpEndpoint>& ep)>&& action) { - const auto matchSpec = - std::string(sdbusplus::bus::match::rules::interfacesRemoved()) - .append( - sdbusplus::bus::match::rules::argNpath(0, endpointObjectPath)); - removeMatch = std::make_unique<sdbusplus::bus::match_t>( - *connection, matchSpec, - std::bind_front(MctpReactorDevice::onEndpointInterfacesRemoved, - weak_from_this(), endpointObjectPath)); + resolvedEndpointPath = endpointObjectPath; + + if (targetBusInfo.empty()) + { + const auto matchSpec = + std::string(sdbusplus::bus::match::rules::interfacesRemoved()) + .append(sdbusplus::bus::match::rules::argNpath( + 0, endpointObjectPath)); + removeMatch = std::make_unique<sdbusplus::bus::match_t>( + *connection, matchSpec, + std::bind_front(MctpReactorDevice::onEndpointInterfacesRemoved, + weak_from_this(), endpointObjectPath)); + } const auto networkAndEid = extractNetworkAndEid(endpointObjectPath); if (!networkAndEid) @@ -251,7 +317,9 @@ if (endpoint) { removeMatch.reset(); - endpoint->remove(); + + endpoint->removed(); + endpoint.reset(); } }
diff --git a/src/MctpReactorDevice.hpp b/src/MctpReactorDevice.hpp index 321fcdb..212a355 100644 --- a/src/MctpReactorDevice.hpp +++ b/src/MctpReactorDevice.hpp
@@ -1,6 +1,7 @@ #pragma once #include "MctpEndpoint.hpp" +#include "MctpUtil.hpp" #include <sdbusplus/asio/connection.hpp> #include <sdbusplus/bus/match.hpp> @@ -27,11 +28,11 @@ std::optional<int> address); MctpReactorDevice( const std::shared_ptr<sdbusplus::asio::connection>& connection, - const std::string& deviceObjectPath, const std::string& endpointPath, + const std::string& deviceObjectPath, const BusInfo& busInfo, std::optional<int> busNumber, std::optional<int> address); MctpReactorDevice(const MctpDevice& other) = delete; MctpReactorDevice(MctpDevice&& other) = delete; - ~MctpReactorDevice() override = default; + ~MctpReactorDevice() override; void setup(std::function<void(const std::error_code& ec, const std::shared_ptr<MctpEndpoint>& ep)>&& @@ -53,11 +54,17 @@ std::shared_ptr<sdbusplus::asio::connection> connection; const std::string deviceObjectPath; - const std::string resolvedEndpointPath; + std::string resolvedEndpointPath; const std::string prettyObjectName; // Suffix of deviceObjectPath. const bool isI2cAccessible; const int i2cBusNumber; // Invalid if isI2cAccessible is false. const int i2cAddress; // Invalid if isI2cAccessible is false. - std::shared_ptr<MctpEndpoint> endpoint; + std::shared_ptr<MctpdEndpoint> endpoint; std::unique_ptr<sdbusplus::bus::match_t> removeMatch; + + BusInfo targetBusInfo; + std::optional<MctpCallbackToken> callbackToken; + std::function<void(const std::error_code&, + const std::shared_ptr<MctpEndpoint>&)> + savedAction; };
diff --git a/src/MctpUtil.cpp b/src/MctpUtil.cpp index 7530c97..b28c39b 100644 --- a/src/MctpUtil.cpp +++ b/src/MctpUtil.cpp
@@ -33,6 +33,9 @@ static std::map<std::string, EndpointState> endpointStates; static uint8_t filterMsgType = 0; +static std::map<MctpCallbackToken, MctpEndpointCallback> mctpEndpointCallbacks; +static MctpCallbackToken nextCallbackToken = 1; + static std::unique_ptr<sdbusplus::bus::match_t> associationMatch = nullptr; static std::unique_ptr<sdbusplus::bus::match_t> associationRemoveMatch = nullptr; @@ -224,6 +227,11 @@ mctpEndpointConfigMap[endpointPath] = objIt->second; lg2::info("DEBUG: Map populated, new size: {SIZE}", "SIZE", mctpEndpointConfigMap.size()); + + for (const auto& [token, cb] : mctpEndpointCallbacks) + { + cb(endpointPath, objIt->second, false); + } } if (lastState == State::Add) @@ -379,6 +387,11 @@ lg2::info("Removing MCTP endpoint {ENDPOINT} from map", "ENDPOINT", endpointPath); mctpEndpointConfigMap.erase(it); + + for (const auto& [token, cb] : mctpEndpointCallbacks) + { + cb(endpointPath, {}, true); + } } } }); @@ -550,3 +563,20 @@ associationMatch.reset(); associationRemoveMatch.reset(); } + +MctpCallbackToken registerMctpEndpointCallback(MctpEndpointCallback&& cb) +{ + MctpCallbackToken token = nextCallbackToken++; + mctpEndpointCallbacks[token] = std::move(cb); + return token; +} + +void unregisterMctpEndpointCallback(MctpCallbackToken token) +{ + mctpEndpointCallbacks.erase(token); +} + +void unregisterAllMctpEndpointCallbacks() +{ + mctpEndpointCallbacks.clear(); +}
diff --git a/src/MctpUtil.hpp b/src/MctpUtil.hpp index 8f6d25c..5180e6b 100644 --- a/src/MctpUtil.hpp +++ b/src/MctpUtil.hpp
@@ -22,8 +22,19 @@ #include "Utils.hpp" +#include <functional> + extern std::map<std::string, SensorData> mctpEndpointConfigMap; +using MctpEndpointCallback = + std::function<void(const std::string& endpointPath, + const SensorData& endpointConfig, bool isRemoved)>; +using MctpCallbackToken = size_t; + +MctpCallbackToken registerMctpEndpointCallback(MctpEndpointCallback&& cb); +void unregisterMctpEndpointCallback(MctpCallbackToken token); +void unregisterAllMctpEndpointCallbacks(); + void setupMctpEndpointListener( const std::shared_ptr<sdbusplus::asio::connection>& conn, MctpMessageType msgType);
diff --git a/src/NVMeSensorMain.cpp b/src/NVMeSensorMain.cpp index 90bb362..2821207 100644 --- a/src/NVMeSensorMain.cpp +++ b/src/NVMeSensorMain.cpp
@@ -372,28 +372,9 @@ std::shared_ptr<MctpDevice> mctpDev; if (*nvmeProtocol == "mi_mctp") { - std::string endpointPath; - for (const auto& [epPath, epConfig] : mctpEndpointConfigMap) - { - BusInfo epBusInfo = extractBusInfo(epConfig); - if (epBusInfo == busInfo) - { - endpointPath = epPath; - break; - } - } - - if (endpointPath.empty()) - { - lg2::warning( - "No matching MCTP endpoint found for BusInfo on {PATH}", - "PATH", nvmeObjectPath.str); - continue; - } - mctpDev = std::make_shared<MctpReactorDevice>( - dbusConnection, nvmeObjectPath.str, endpointPath, - busNumber, address); + dbusConnection, nvmeObjectPath.str, busInfo, busNumber, + address); } else if (!isMctpReactorDevice) {
diff --git a/tests/meson.build b/tests/meson.build index 455d538..9350ba4 100644 --- a/tests/meson.build +++ b/tests/meson.build
@@ -104,7 +104,9 @@ 'test_mctp_util', 'test_MctpUtil.cpp', '../src/MctpUtil.cpp', - cpp_args: ['-UBOOST_ASIO_NO_DEPRECATED', '-UBOOST_ASIO_DISABLE_THREADS', '-UBOOST_ASIO_HAS_IO_URING', '-DBUILDDIR='+ meson.current_build_dir(), '-DTEST_SRC_DIR="' + meson.current_source_dir() + '"'], + '../src/MctpReactorDevice.cpp', + '../src/MctpEndpoint.cpp', + cpp_args: ['-UBOOST_ASIO_NO_DEPRECATED', '-UBOOST_ASIO_DISABLE_THREADS', '-UBOOST_ASIO_HAS_IO_URING', '-DTEST_SRC_DIR="' + meson.current_source_dir() + '"'], dependencies: [ut_deps_list, nlohmann_json], implicit_include_directories: false, include_directories: '../src',
diff --git a/tests/test_MctpUtil.cpp b/tests/test_MctpUtil.cpp index a743d07..3cbc58f 100644 --- a/tests/test_MctpUtil.cpp +++ b/tests/test_MctpUtil.cpp
@@ -1005,6 +1005,230 @@ EXPECT_EQ(itUnsupported, mctpEndpointConfigMap.end()); } +// 11. MctpReactorDevice creation BEFORE mctpEndpointConfigMap population +// (Deferred Setup) +TEST_F(MctpUtilTest, MctpReactorDevice_DeferredSetup_USB) +{ + std::string endpointPath = + "/au/com/codeconstruct/mctp1/networks/1/endpoints/11"; + std::string emConfigPath = + "/xyz/openbmc_project/inventory/system/board/MockBoard/mctp_device"; + + setupMctpEndpointListener(conn, MctpMessageType::NVME_MI); + + // Create BusInfo for target device (matching what mock_mctpd.py returns for + // USB) + BusInfo targetBusInfo; + targetBusInfo["BusType"] = "USB"; + targetBusInfo["Port"] = "1.2.5"; + targetBusInfo["Configuration"] = "1"; + + // Create MctpReactorDevice + auto mctpDev = std::make_shared<MctpReactorDevice>( + conn, emConfigPath, targetBusInfo, std::nullopt, std::nullopt); + + bool setupCompleted = false; + mctpDev->setup([&](const std::error_code& ec, + const std::shared_ptr<MctpEndpoint>& ep) { + setupCompleted = true; + EXPECT_FALSE(ec); + EXPECT_NE(ep, nullptr); + }); + + // Verify it didn't complete immediately + io.poll(); + EXPECT_FALSE(setupCompleted); + + // Trigger Add event via D-Bus! + std::string addCmd = + "busctl call au.com.codeconstruct.MCTP1 /au/com/codeconstruct/mctp1 com.example.Control TriggerAdd ss \"" + + endpointPath + "\" \"" + emConfigPath + "\""; + runCmd(addCmd); + + // Wait for setup to complete (up to 5 seconds) + for (int i = 0; i < 50; i++) + { + io.poll(); + usleep(100000); // 100ms + if (setupCompleted) + break; + } + + EXPECT_TRUE(setupCompleted); +} + +// 12. MctpReactorDevice creation AFTER mctpEndpointConfigMap population +// (Immediate Setup) +TEST_F(MctpUtilTest, MctpReactorDevice_ImmediateSetup_USB) +{ + std::string endpointPath = + "/au/com/codeconstruct/mctp1/networks/1/endpoints/12"; + std::string emConfigPath = + "/xyz/openbmc_project/inventory/system/board/MockBoard/mctp_device"; + + setupMctpEndpointListener(conn, MctpMessageType::NVME_MI); + + // Trigger Add event FIRST! + std::string addCmd = + "busctl call au.com.codeconstruct.MCTP1 /au/com/codeconstruct/mctp1 com.example.Control TriggerAdd ss \"" + + endpointPath + "\" \"" + emConfigPath + "\""; + runCmd(addCmd); + + // Wait for map to be populated (up to 5 seconds) + bool populated = false; + for (int i = 0; i < 50; i++) + { + io.poll(); + usleep(100000); // 100ms + if (mctpEndpointConfigMap.find(endpointPath) != + mctpEndpointConfigMap.end()) + { + populated = true; + break; + } + } + ASSERT_TRUE(populated); + + // Create BusInfo for target device + BusInfo targetBusInfo; + targetBusInfo["BusType"] = "USB"; + targetBusInfo["Port"] = "1.2.5"; + targetBusInfo["Configuration"] = "1"; + + // Create MctpReactorDevice + auto mctpDev = std::make_shared<MctpReactorDevice>( + conn, emConfigPath, targetBusInfo, std::nullopt, std::nullopt); + + bool setupCompleted = false; + mctpDev->setup([&](const std::error_code& ec, + const std::shared_ptr<MctpEndpoint>& ep) { + setupCompleted = true; + EXPECT_FALSE(ec); + EXPECT_NE(ep, nullptr); + }); + + // Verify it completes IMMEDIATELY + io.poll(); + EXPECT_TRUE(setupCompleted); +} + +// 13. MctpReactorDevice removal notification (Case C) +TEST_F(MctpUtilTest, MctpReactorDevice_RemovalNotification_USB) +{ + std::string endpointPath = + "/au/com/codeconstruct/mctp1/networks/1/endpoints/13"; + std::string emConfigPath = + "/xyz/openbmc_project/inventory/system/board/MockBoard/mctp_device"; + + setupMctpEndpointListener(conn, MctpMessageType::NVME_MI); + + // Trigger Add event FIRST! + std::string addCmd = + "busctl call au.com.codeconstruct.MCTP1 /au/com/codeconstruct/mctp1 com.example.Control TriggerAdd ss \"" + + endpointPath + "\" \"" + emConfigPath + "\""; + runCmd(addCmd); + + // Wait for map to be populated + bool populated = false; + for (int i = 0; i < 50; i++) + { + io.poll(); + usleep(100000); // 100ms + if (mctpEndpointConfigMap.find(endpointPath) != + mctpEndpointConfigMap.end()) + { + populated = true; + break; + } + } + ASSERT_TRUE(populated); + + // Create BusInfo for target device + BusInfo targetBusInfo; + targetBusInfo["BusType"] = "USB"; + targetBusInfo["Port"] = "1.2.5"; + targetBusInfo["Configuration"] = "1"; + + // Create MctpReactorDevice + auto mctpDev = std::make_shared<MctpReactorDevice>( + conn, emConfigPath, targetBusInfo, std::nullopt, std::nullopt); + + bool setupCompleted = false; + std::shared_ptr<MctpEndpoint> endpoint; + mctpDev->setup([&](const std::error_code& ec, + const std::shared_ptr<MctpEndpoint>& ep) { + setupCompleted = true; + endpoint = ep; + EXPECT_FALSE(ec); + EXPECT_NE(ep, nullptr); + }); + + io.poll(); + ASSERT_TRUE(setupCompleted); + ASSERT_NE(endpoint, nullptr); + + bool sensorNotified = false; + endpoint->subscribe( + nullptr, nullptr, + [&](const std::shared_ptr<MctpEndpoint>&) { sensorNotified = true; }); + + // Trigger Remove event! + std::string removeCmd = + "busctl call au.com.codeconstruct.MCTP1 /au/com/codeconstruct/mctp1 com.example.Control TriggerRemove s \"" + + endpointPath + "\""; + runCmd(removeCmd); + + // Wait for removal notification (up to 5 seconds) + for (int i = 0; i < 50; i++) + { + io.poll(); + usleep(100000); // 100ms + if (sensorNotified) + break; + } + + EXPECT_TRUE(sensorNotified); +} + +// 14. Cancellation on Device Removal (Case D) +TEST_F(MctpUtilTest, MctpReactorDevice_Cancellation_USB) +{ + std::string emConfigPath = + "/xyz/openbmc_project/inventory/system/board/MockBoard/mctp_device"; + + setupMctpEndpointListener(conn, MctpMessageType::NVME_MI); + + // Create BusInfo for target device + BusInfo targetBusInfo; + targetBusInfo["BusType"] = "USB"; + targetBusInfo["Port"] = "1.2.5"; + targetBusInfo["Configuration"] = "1"; + + // Create MctpReactorDevice + auto mctpDev = std::make_shared<MctpReactorDevice>( + conn, emConfigPath, targetBusInfo, std::nullopt, std::nullopt); + + bool setupCompleted = false; + std::error_code errorResult; + mctpDev->setup([&](const std::error_code& ec, + const std::shared_ptr<MctpEndpoint>& ep) { + setupCompleted = true; + errorResult = ec; + EXPECT_EQ(ep, nullptr); + }); + + // Verify it didn't complete immediately + io.poll(); + EXPECT_FALSE(setupCompleted); + + // Call remove() to trigger cancellation! + mctpDev->remove(); + + // Verify it completed with operation_canceled! + EXPECT_TRUE(setupCompleted); + EXPECT_EQ(errorResult, std::make_error_code(std::errc::operation_canceled)); +} + int main(int argc, char** argv) { ::testing::InitGoogleTest(&argc, argv);