nvme: decouple NVMeDevice from NVMeSubsystem Decoupled NVMeDevice from NVMeSubsystem. NVMeDevice now takes a name string instead of a shared pointer to NVMeSubsystem. NVMeSensorMain now manages the lifetime and start/stop of both independently. Tests were updated to reflect these changes. Change-Id: Ic3106534742cf55737dc479bc8524a7e9591c4a0 Signed-off-by: Hao Jiang <jianghao@google.com>
diff --git a/src/NVMeDevice.cpp b/src/NVMeDevice.cpp index dcce6de..7801530 100644 --- a/src/NVMeDevice.cpp +++ b/src/NVMeDevice.cpp
@@ -7,8 +7,6 @@ void NVMeDevice::start() { - subsys->start(); - if (intf.getProtocol() == NVMeIntf::Protocol::NVMeMI) { setup(); @@ -29,7 +27,7 @@ void NVMeDevice::stop() { - subsys->stop(); + timer->cancel(); } void NVMeDevice::finalize(const std::error_code& ec, @@ -101,8 +99,8 @@ assert(intf.getProtocol() == NVMeIntf::Protocol::NVMeMI); assert(timer); assert(gracePeriod); - lg2::info("[{SUBSYS}][{ENDPOINT}]: Available", "SUBSYS", subsys->getName(), - "ENDPOINT", ep->describe()); + lg2::info("[{SUBSYS}][{ENDPOINT}]: Available", "SUBSYS", name, "ENDPOINT", + ep->describe()); // The behaviour of MctpEndpoint::subscribe() is to register the provided // callbacks, and then fetch the current state of the endpoint. As a
diff --git a/src/NVMeDevice.hpp b/src/NVMeDevice.hpp index fe1952f..9403fc5 100644 --- a/src/NVMeDevice.hpp +++ b/src/NVMeDevice.hpp
@@ -13,31 +13,27 @@ {}; public: - static std::shared_ptr<NVMeDevice> - create(NVMeIntf intf, const std::shared_ptr<NVMeSubsystem>& subsys) + static std::shared_ptr<NVMeDevice> create(const std::string& name, + NVMeIntf intf) { - return std::make_shared<NVMeDevice>(Private(), std::move(intf), subsys); + return std::make_shared<NVMeDevice>(Private(), name, std::move(intf)); } static std::shared_ptr<NVMeDevice> - create(boost::asio::io_context& io, + create(boost::asio::io_context& io, const std::string& name, const std::shared_ptr<MctpDevice>& dev, NVMeIntf intf, - const std::shared_ptr<NVMeSubsystem>& subsys, std::chrono::seconds gracePeriod = std::chrono::seconds(5)) { - return std::make_shared<NVMeDevice>(Private(), io, dev, std::move(intf), - subsys, gracePeriod); + return std::make_shared<NVMeDevice>(Private(), io, name, dev, + std::move(intf), gracePeriod); } - - NVMeDevice(Private /*unused*/, NVMeIntf intf, - const std::shared_ptr<NVMeSubsystem>& subsys) : - intf(std::move(intf)), subsys(subsys) + NVMeDevice(Private /*unused*/, const std::string& name, NVMeIntf intf) : + name(name), intf(std::move(intf)) {} NVMeDevice(Private /*unused*/, boost::asio::io_context& io, - const std::shared_ptr<MctpDevice>& dev, NVMeIntf intf, - const std::shared_ptr<NVMeSubsystem>& subsys, - std::chrono::seconds gracePeriod) : - dev(dev), intf(std::move(intf)), subsys(subsys), timer(io), + const std::string& name, const std::shared_ptr<MctpDevice>& dev, + NVMeIntf intf, std::chrono::seconds gracePeriod) : + name(name), dev(dev), intf(std::move(intf)), timer(io), gracePeriod{gracePeriod} {} ~NVMeDevice() = default; @@ -53,9 +49,9 @@ void finalize(const std::error_code& ec, const std::shared_ptr<MctpEndpoint>& ep); + std::string name; std::shared_ptr<MctpDevice> dev; NVMeIntf intf; - std::shared_ptr<NVMeSubsystem> subsys; std::optional<boost::asio::steady_timer> timer; std::optional<std::chrono::seconds> gracePeriod; bool recovering{};
diff --git a/src/NVMeSensorMain.cpp b/src/NVMeSensorMain.cpp index c770c7b..23f3b7a 100644 --- a/src/NVMeSensorMain.cpp +++ b/src/NVMeSensorMain.cpp
@@ -38,8 +38,12 @@ #include <unordered_set> // a map with key value of {path, NVMeSubsystem} -using NVMEMap = std::map<std::string, std::shared_ptr<NVMeDevice>>; -static NVMEMap nvmeDevices; +using NVMeSubsystemMap = std::map<std::string, std::shared_ptr<NVMeSubsystem>>; +static NVMeSubsystemMap nvmeSubsystems; + +// a map with key value of {path, NVMeDevice} +using NVMeDeviceMap = std::map<std::string, std::shared_ptr<NVMeDevice>>; +static NVMeDeviceMap nvmeDevices; // A map from root bus number to the Worker // This map means to reuse the same worker for all NVMe EP under the same @@ -285,13 +289,20 @@ auto nvmeSubsys = NVMeSubsystem::create( io, objectServer, dbusConnection, nvmeObjectPath, *sensorName, configData, nvmeIntf, enableFeatureLockdown); - auto nvmeDev = NVMeDevice::create(std::move(nvmeIntf), - nvmeSubsys); - auto [entry, added] = nvmeDevices.try_emplace(nvmeObjectPath, - nvmeDev); - if (added) + auto nvmeDev = NVMeDevice::create(*sensorName, nvmeIntf); + + auto [subsysEntry, subsysAdded] = + nvmeSubsystems.try_emplace(nvmeObjectPath, nvmeSubsys); + if (subsysAdded) { - entry->second->start(); + subsysEntry->second->start(); + } + + auto [devEntry, devAdded] = + nvmeDevices.try_emplace(nvmeObjectPath, nvmeDev); + if (devAdded) + { + devEntry->second->start(); } } catch (std::exception& ex) @@ -360,13 +371,21 @@ io, objectServer, dbusConnection, nvmeObjectPath, *sensorName, configData, nvmeIntf, enableFeatureLockdown); - auto nvmeDev = NVMeDevice::create( - io, mctpDev, std::move(nvmeIntf), nvmeSubsys); - auto [entry, added] = nvmeDevices.try_emplace(nvmeObjectPath, - nvmeDev); - if (added) + auto nvmeDev = NVMeDevice::create(io, *sensorName, mctpDev, + nvmeIntf); + + auto [subsysEntry, subsysAdded] = + nvmeSubsystems.try_emplace(nvmeObjectPath, nvmeSubsys); + if (subsysAdded) { - entry->second->start(); + subsysEntry->second->start(); + } + + auto [devEntry, devAdded] = + nvmeDevices.try_emplace(nvmeObjectPath, nvmeDev); + if (devAdded) + { + devEntry->second->start(); } } catch (std::exception& ex) @@ -390,6 +409,12 @@ } nvmeDevices.clear(); + for (auto& [_, nvmeSubsys] : nvmeSubsystems) + { + nvmeSubsys->stop(); + } + nvmeSubsystems.clear(); + static int count = 0; static ManagedObjectType configs; count += 2; @@ -432,7 +457,7 @@ getter->getConfiguration(std::vector<std::string>{nvme::sensorType}); } -static void interfaceRemoved(sdbusplus::message_t& message, NVMEMap& devices) +static void interfaceRemoved(sdbusplus::message_t& message) { if (message.is_method_error()) { @@ -452,14 +477,19 @@ return; } - auto device = devices.find(path); - if (device == devices.end()) + auto device = nvmeDevices.find(path); + if (device != nvmeDevices.end()) { - return; + device->second->stop(); + nvmeDevices.erase(device); } - device->second->stop(); - devices.erase(device); + auto subsys = nvmeSubsystems.find(path); + if (subsys != nvmeSubsystems.end()) + { + subsys->second->stop(); + nvmeSubsystems.erase(subsys); + } } int main(int argc, char** argv) @@ -552,7 +582,7 @@ static_cast<sdbusplus::bus_t&>(*systemBus), "type='signal',member='InterfacesRemoved',arg0path='" + std::string(inventoryPath) + "/'", - [](sdbusplus::message_t& msg) { interfaceRemoved(msg, nvmeDevices); }); + [](sdbusplus::message_t& msg) { interfaceRemoved(msg); }); setupManufacturingModeMatch(*systemBus);
diff --git a/tests/test_nvme_mi_recovery.cpp b/tests/test_nvme_mi_recovery.cpp index 0065e8e..bccb45c 100644 --- a/tests/test_nvme_mi_recovery.cpp +++ b/tests/test_nvme_mi_recovery.cpp
@@ -1,7 +1,6 @@ #include "NVMeDevice.hpp" #include "NVMeIntf.hpp" #include "NVMeMi.hpp" -#include "NVMeSubsys.hpp" #include "Utils.hpp" #include <boost/asio/steady_timer.hpp> @@ -97,13 +96,9 @@ .WillOnce(testing::InvokeArgument<0>(std::error_code(), mctpEp)); auto systemBus = std::make_shared<sdbusplus::asio::connection>(io); - sdbusplus::asio::object_server objectServer(systemBus, true); auto worker = NVMeMiWorker::create(io); auto intf = NVMeIntf::create<NVMeMi>(io, systemBus, mctpDev, worker); - SensorData sensorData{}; - auto subsys = NVMeSubsystem::create(io, objectServer, systemBus, "/foo", - "bar", sensorData, intf, false); - auto nvmeDev = NVMeDevice::create(io, mctpDev, std::move(intf), subsys, + auto nvmeDev = NVMeDevice::create(io, "bar", mctpDev, intf, std::chrono::seconds(2)); nvmeDev->start(); timer.expires_after(std::chrono::seconds(6)); @@ -157,16 +152,12 @@ .WillOnce(testing::InvokeArgument<0>(std::error_code(), mctpEp)); auto systemBus = std::make_shared<sdbusplus::asio::connection>(io); - sdbusplus::asio::object_server objectServer(systemBus, true); auto worker = NVMeMiWorker::create(io); auto intf = NVMeIntf::create<TestNVMeMi>(io, systemBus, mctpDev, worker); auto testMi = std::dynamic_pointer_cast<TestNVMeMi>( std::get<std::shared_ptr<NVMeMiIntf>>(intf.getInferface())); - SensorData sensorData{}; - auto subsys = NVMeSubsystem::create(io, objectServer, systemBus, "/foo", - "bar", sensorData, intf, false); - auto nvmeDev = NVMeDevice::create(io, mctpDev, std::move(intf), subsys, + auto nvmeDev = NVMeDevice::create(io, "bar", mctpDev, intf, std::chrono::seconds(2)); nvmeDev->start(); @@ -244,16 +235,12 @@ .WillOnce(testing::InvokeArgument<0>(std::error_code(), mctpEp)); auto systemBus = std::make_shared<sdbusplus::asio::connection>(io); - sdbusplus::asio::object_server objectServer(systemBus, true); auto worker = NVMeMiWorker::create(io); auto intf = NVMeIntf::create<TestNVMeMi>(io, systemBus, mctpDev, worker); auto testMi = std::dynamic_pointer_cast<TestNVMeMi>( std::get<std::shared_ptr<NVMeMiIntf>>(intf.getInferface())); - SensorData sensorData{}; - auto subsys = NVMeSubsystem::create(io, objectServer, systemBus, "/foo", - "bar", sensorData, intf, false); - auto nvmeDev = NVMeDevice::create(io, mctpDev, std::move(intf), subsys, + auto nvmeDev = NVMeDevice::create(io, "bar", mctpDev, intf, std::chrono::seconds(2)); nvmeDev->start(); @@ -297,6 +284,7 @@ io.run(); } + // Unused, but required to link successfully std::unordered_map<std::string, void*> pluginLibMap = {};