nvmed: probe features on no ONCS bit 4 and skip empty FeatureStore When ONCS bit 4 is not set (e.g. IronFist controllers) and Log Page 12h is not supported, discoverFeatures previously assumed all standard NVMe features were supported and populated featureCollection without verification. Subsequent Redfish queries (like $expand on /Features) called GetFeature on these unsupported features, causing NVMe-MI command failures (Vendor Specific Status SCT 7 SC 0xf3 on IronFist) and HTTP 500 Internal Error responses. This change actively queries asyncAdminGetFeatures with SEL=CURRENT when ONCS bit 4 is not set to verify feature support before adding to featureCollection. Furthermore, when featureCollection is empty (no features supported by the device), FeatureStore::init returns errc::not_supported and skips registering the D-Bus interface, and controller/subsystem/volume classes log info and reset the featureStore pointer. Tested: Added unit tests and verified with gbmc ci docker nvmed. Google-Bug-Id: 558424309 Change-Id: I50c067e5c73987eb59658530788563e1ac867d0d Signed-off-by: Guangzong Chen <guangzong@google.com> TAG=agy CONV=466d579d-564d-4675-89fd-3d720b796c05
diff --git a/src/NVMeController.cpp b/src/NVMeController.cpp index d7fe044..b359c0e 100644 --- a/src/NVMeController.cpp +++ b/src/NVMeController.cpp
@@ -598,8 +598,18 @@ path = this->path](const std::error_code& ec) { if (ec) { - lg2::error("[{PATH}]Failed to initialize feature store: {ERROR}", - "PATH", path, "ERROR", ec.message()); + if (ec == std::errc::no_such_device || + ec == std::errc::not_supported) + { + lg2::info("[{PATH}]Feature store not supported: {ERROR}", + "PATH", path, "ERROR", ec.message()); + } + else + { + lg2::error( + "[{PATH}]Failed to initialize feature store: {ERROR}", + "PATH", path, "ERROR", ec.message()); + } auto self = selfWeak.lock(); if (self) {
diff --git a/src/NVMeFeatureStore.cpp b/src/NVMeFeatureStore.cpp index 1980ada..41ac1c4 100644 --- a/src/NVMeFeatureStore.cpp +++ b/src/NVMeFeatureStore.cpp
@@ -228,6 +228,14 @@ return; } self->discoverFeatures(yield); + if (self->featureCollection.empty()) + { + lg2::info( + "[{PATH}]No NVMe features supported, skipping FeatureStore registration", + "PATH", self->objectPath); + cb(std::make_error_code(std::errc::not_supported)); + return; + } self->registerDbusInterface(); cb({}); }); @@ -347,6 +355,26 @@ if (!oncsBit4) { + NVMeMiIntf::GetFeaturesRequest args{}; + args.fid = meta.fid; + args.sel = NVME_GET_FEATURES_SEL_CURRENT; // 000b + args.nsid = (scope == FeatureScope::Namespace) ? nsid + : NVME_NSID_NONE; + args.cdw11 = 0; + args.data_len = 0; + + auto [ex, cdw0, data] = asyncAdminGetFeatures(io, nvmeIntf, ctrl, + args, yield); + + if (ex) + { + lg2::info( + "[{PATH}]Feature {NAME} (FID {FID}) get feature query failed (unsupported): {ERROR}", + "PATH", path, "NAME", name, "FID", meta.fid, "ERROR", + ex->what()); + continue; + } + featureCollection.emplace_back(name + "_Current", fidStr, meta.protoClass); continue;
diff --git a/src/NVMeSubsys.cpp b/src/NVMeSubsys.cpp index 2f021b6..112f906 100644 --- a/src/NVMeSubsys.cpp +++ b/src/NVMeSubsys.cpp
@@ -198,9 +198,19 @@ path = this->path](const std::error_code& ec) { if (ec) { - lg2::error( - "[{PATH}]Failed to initialize feature store for subsystem: {ERROR}", - "PATH", path, "ERROR", ec.message()); + if (ec == std::errc::no_such_device || + ec == std::errc::not_supported) + { + lg2::info( + "[{PATH}]Feature store not supported for subsystem: {ERROR}", + "PATH", path, "ERROR", ec.message()); + } + else + { + lg2::error( + "[{PATH}]Failed to initialize feature store for subsystem: {ERROR}", + "PATH", path, "ERROR", ec.message()); + } auto self = selfWeak.lock(); if (self) {
diff --git a/src/NVMeVolume.cpp b/src/NVMeVolume.cpp index 0e078dc..b0afbe8 100644 --- a/src/NVMeVolume.cpp +++ b/src/NVMeVolume.cpp
@@ -187,9 +187,19 @@ path = this->path](const std::error_code& ec) { if (ec) { - lg2::error( - "[{PATH}]Failed to initialize feature store for volume: {ERROR}", - "PATH", path, "ERROR", ec.message()); + if (ec == std::errc::no_such_device || + ec == std::errc::not_supported) + { + lg2::info( + "[{PATH}]Feature store not supported for volume: {ERROR}", + "PATH", path, "ERROR", ec.message()); + } + else + { + lg2::error( + "[{PATH}]Failed to initialize feature store for volume: {ERROR}", + "PATH", path, "ERROR", ec.message()); + } auto self = selfWeak.lock(); if (self) {
diff --git a/tests/test_nvme_feature_store.cpp b/tests/test_nvme_feature_store.cpp index 9a16224..2346abc 100644 --- a/tests/test_nvme_feature_store.cpp +++ b/tests/test_nvme_feature_store.cpp
@@ -433,7 +433,7 @@ bool done = false; featureStore->init([&done](const std::error_code& ec) { - EXPECT_FALSE(ec); + EXPECT_EQ(ec, std::errc::not_supported); done = true; }); @@ -778,6 +778,169 @@ featureStore, "TemperatureThreshold_Current")); } +TEST_F(FeatureStoreTest, DiscoverFeaturesNoONCSBit4Supported) +{ + boost::asio::io_context io; + auto conn = std::make_shared<sdbusplus::asio::connection>(io); + sdbusplus::asio::object_server objServer(conn); + std::string objectPath = "/xyz/openbmc_project/nvme/ctrl0"; + + auto mockNvme = std::make_shared<NVMeMiMock>(); + nvme_mi_ctrl_t ctrl = nullptr; + + auto featureStore = std::make_shared<FeatureStore>( + io, objServer, conn, objectPath, mockNvme, ctrl, 0, + FeatureScope::Controller); + + // Mock Identify Controller to return ONCS without bit 4 + EXPECT_CALL(*mockNvme, + adminIdentify(::testing::_, NVME_IDENTIFY_CNS_CTRL, + ::testing::_, ::testing::_, ::testing::_)) + .WillOnce( + [](nvme_mi_ctrl_t, nvme_identify_cns, uint32_t, uint16_t, + std::function<void(nvme_ex_ptr, std::span<uint8_t>)>&& cb) { + nvme_id_ctrl id{}; + id.vwc = 1; + id.oncs = 0; // Bit 4 NOT set + std::vector<uint8_t> data(sizeof(id)); + memcpy(data.data(), &id, sizeof(id)); + cb(nullptr, data); + }); + + // Mock Log Page 12h to fail + EXPECT_CALL(*mockNvme, adminGetLogPage( + ::testing::_, NVME_LOG_LID_FID_SUPPORTED_EFFECTS, + ::testing::_, ::testing::_, ::testing::_, + ::testing::_, ::testing::_)) + .WillOnce( + [](nvme_mi_ctrl_t, nvme_cmd_get_log_lid, uint32_t, uint8_t, + uint16_t, + std::function<void(const std::error_code&, std::span<uint8_t>)>&& + cb, + uint8_t) { + cb(std::make_error_code(std::errc::bad_message), {}); + }); + + // Mock Get Features with SEL=CURRENT (000b) + // Only Arbitration (FID 0x01) succeeds; others fail + EXPECT_CALL( + *mockNvme, + adminGetFeatures(::testing::_, + ::testing::Field(&NVMeMiIntf::GetFeaturesRequest::sel, + NVME_GET_FEATURES_SEL_CURRENT), + ::testing::_)) + .WillRepeatedly( + [](nvme_mi_ctrl_t, const NVMeMiIntf::GetFeaturesRequest& req, + std::function<void(nvme_ex_ptr, uint32_t, std::span<uint8_t>)>&& + cb) { + if (req.fid == 0x01) + { + cb(nullptr, 0, {}); + } + else + { + cb(makeLibNVMeError(0, 0x02, "Feature not supported"), 0, {}); + } + }); + + bool done = false; + featureStore->init([&done](const std::error_code& ec) { + EXPECT_FALSE(ec); + done = true; + }); + + while (!done) + { + io.run_one(); + } + + EXPECT_TRUE(FeatureStoreTest::callIsFeatureSupported( + featureStore, "Arbitration_Current")); + EXPECT_FALSE(FeatureStoreTest::callIsFeatureSupported( + featureStore, "PowerManagement_Current")); + EXPECT_FALSE(FeatureStoreTest::callIsFeatureSupported( + featureStore, "Arbitration_Default")); + EXPECT_FALSE(FeatureStoreTest::callIsFeatureSupported(featureStore, + "Arbitration_Saved")); +} + +TEST_F(FeatureStoreTest, DiscoverFeaturesNoONCSBit4AllFeaturesFail) +{ + boost::asio::io_context io; + auto conn = std::make_shared<sdbusplus::asio::connection>(io); + sdbusplus::asio::object_server objServer(conn); + std::string objectPath = "/xyz/openbmc_project/nvme/ctrl0"; + + auto mockNvme = std::make_shared<NVMeMiMock>(); + nvme_mi_ctrl_t ctrl = nullptr; + + auto featureStore = std::make_shared<FeatureStore>( + io, objServer, conn, objectPath, mockNvme, ctrl, 0, + FeatureScope::Controller); + + // Mock Identify Controller to return ONCS without bit 4 + EXPECT_CALL(*mockNvme, + adminIdentify(::testing::_, NVME_IDENTIFY_CNS_CTRL, + ::testing::_, ::testing::_, ::testing::_)) + .WillOnce( + [](nvme_mi_ctrl_t, nvme_identify_cns, uint32_t, uint16_t, + std::function<void(nvme_ex_ptr, std::span<uint8_t>)>&& cb) { + nvme_id_ctrl id{}; + id.vwc = 1; + id.oncs = 0; // Bit 4 NOT set + std::vector<uint8_t> data(sizeof(id)); + memcpy(data.data(), &id, sizeof(id)); + cb(nullptr, data); + }); + + // Mock Log Page 12h to fail + EXPECT_CALL(*mockNvme, adminGetLogPage( + ::testing::_, NVME_LOG_LID_FID_SUPPORTED_EFFECTS, + ::testing::_, ::testing::_, ::testing::_, + ::testing::_, ::testing::_)) + .WillOnce( + [](nvme_mi_ctrl_t, nvme_cmd_get_log_lid, uint32_t, uint8_t, + uint16_t, + std::function<void(const std::error_code&, std::span<uint8_t>)>&& + cb, + uint8_t) { + cb(std::make_error_code(std::errc::bad_message), {}); + }); + + // Mock Get Features with SEL=CURRENT (000b) to fail for all features (e.g. + // IronFist) + EXPECT_CALL( + *mockNvme, + adminGetFeatures(::testing::_, + ::testing::Field(&NVMeMiIntf::GetFeaturesRequest::sel, + NVME_GET_FEATURES_SEL_CURRENT), + ::testing::_)) + .WillRepeatedly( + [](nvme_mi_ctrl_t, const NVMeMiIntf::GetFeaturesRequest&, + std::function<void(nvme_ex_ptr, uint32_t, std::span<uint8_t>)>&& + cb) { + cb(makeLibNVMeError(0, 0xf3, "Vendor Specific Status (SCT 7 SC 0xf3)"), + 0, {}); + }); + + bool done = false; + std::error_code resultEc; + featureStore->init([&done, &resultEc](const std::error_code& ec) { + resultEc = ec; + done = true; + }); + + while (!done) + { + io.run_one(); + } + + EXPECT_TRUE(done); + EXPECT_EQ(resultEc, std::make_error_code(std::errc::not_supported)); + EXPECT_FALSE(FeatureStoreTest::callIsFeatureSupported( + featureStore, "Arbitration_Current")); +} + int main(int argc, char** argv) { ::testing::InitGoogleTest(&argc, argv);