nvmed: do not add unsupported features to featureCollection on capability query error When querying feature capabilities (SEL=011b) during feature discovery, if the drive returns an error (such as Invalid Field in Command 0x02), do not add the feature to featureCollection. Advertising unsupported features causes Redfish and busctl queries to attempt unsupported NVMe commands, resulting in Service Unavailable and 500 Internal Error. Tested: Ran unit tests and verified on target hardware. Google-Bug-Id: 544517012 Change-Id: Ic699209edd0d819190fde0342459c4396a6a6964 Signed-off-by: Guangzong Chen <guangzong@google.com> TAG=agy CONV=322d2541-0ab6-49b1-8c8f-2aeba2f83067
diff --git a/src/NVMeFeatureStore.cpp b/src/NVMeFeatureStore.cpp index 1de3b85..8002348 100644 --- a/src/NVMeFeatureStore.cpp +++ b/src/NVMeFeatureStore.cpp
@@ -355,29 +355,27 @@ if (ex) { - lg2::error("Failed to query capabilities for FID {FID}: {ERROR}", - "FID", meta.fid, "ERROR", ex->what()); - featureCollection.emplace_back(name + "_Current", fidStr, + lg2::info( + "Feature {NAME} (FID {FID}) capabilities query failed (unsupported): {ERROR}", + "NAME", name, "FID", meta.fid, "ERROR", ex->what()); + continue; + } + + featureCaps[meta.fid] = cdw0; + + featureCollection.emplace_back(name + "_Current", fidStr, + meta.protoClass); + + if ((cdw0 & nvmeGetFeaturesCapChangeable) != 0) + { + featureCollection.emplace_back(name + "_Default", fidStr, meta.protoClass); } - else + + if ((cdw0 & nvmeGetFeaturesCapSaveable) != 0) { - featureCaps[meta.fid] = cdw0; - - featureCollection.emplace_back(name + "_Current", fidStr, + featureCollection.emplace_back(name + "_Saved", fidStr, meta.protoClass); - - if ((cdw0 & nvmeGetFeaturesCapChangeable) != 0) - { - featureCollection.emplace_back(name + "_Default", fidStr, - meta.protoClass); - } - - if ((cdw0 & nvmeGetFeaturesCapSaveable) != 0) - { - featureCollection.emplace_back(name + "_Saved", fidStr, - meta.protoClass); - } } } }
diff --git a/tests/test_MctpUtil.cpp b/tests/test_MctpUtil.cpp index 0854d5f..7d1bf53 100644 --- a/tests/test_MctpUtil.cpp +++ b/tests/test_MctpUtil.cpp
@@ -1837,7 +1837,7 @@ MctpReactorBusInfo busInfo(chain); EXPECT_TRUE(busInfo.isValid()); - auto propsList = busInfo.getProperties(); + const auto& propsList = busInfo.getProperties(); ASSERT_EQ(propsList.size(), 2); // Verify order is root-to-leaf (from Test 22)
diff --git a/tests/test_nvme_feature_store.cpp b/tests/test_nvme_feature_store.cpp index 09259dd..f549b08 100644 --- a/tests/test_nvme_feature_store.cpp +++ b/tests/test_nvme_feature_store.cpp
@@ -147,6 +147,12 @@ return fs->featureCaps; } + static bool callIsFeatureSupported(const std::shared_ptr<FeatureStore>& fs, + const std::string& featureName) + { + return fs->isFeatureSupported(featureName); + } + static void addSupportedFeature(const std::shared_ptr<FeatureStore>& fs, const std::string& name, const std::string& fid, @@ -351,6 +357,89 @@ EXPECT_TRUE(cap & 0x4); // Changeable } +TEST_F(FeatureStoreTest, DiscoverFeaturesCapabilityErrorSkipsUnsupportedFeature) +{ + 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/vol1"; + + auto mockNvme = std::make_shared<NVMeMiMock>(); + nvme_mi_ctrl_t ctrl = nullptr; + + auto featureStore = std::make_shared<FeatureStore>( + io, objServer, conn, objectPath, mockNvme, ctrl, 1, + FeatureScope::Namespace); + + // Mock Identify Controller to return ONCS with bit 4 set (Save/Select + // supported) + 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; // VWC supported + id.oncs = htole16(1 << 4); // Save and Select supported + std::vector<uint8_t> data(sizeof(id)); + memcpy(data.data(), &id, sizeof(id)); + cb(nullptr, data); + }); + + // Mock Get Log Page to fail or return empty + 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 SEL=011b (capabilities query) + // ErrorRecovery (FID 0x05) fails with Invalid Field in Command (0x02) + EXPECT_CALL( + *mockNvme, + adminGetFeatures(::testing::_, + ::testing::Field(&NVMeMiIntf::GetFeaturesRequest::sel, + NVME_GET_FEATURES_SEL_SUPPORTED), + ::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 == 0x05) + { + // Simulate drive returning Invalid Field in Command + cb(makeLibNVMeError(0, 0x02, "Invalid Field in Command"), 0, {}); + } + else + { + cb(nullptr, 0x4, {}); // Changeable + } + }); + + bool done = false; + featureStore->init([&done](const std::error_code& ec) { + EXPECT_FALSE(ec); + done = true; + }); + + while (!done) + { + io.run_one(); + } + + // Verify ErrorRecovery_Current is NOT in featureStore + EXPECT_FALSE(FeatureStoreTest::callIsFeatureSupported( + featureStore, "ErrorRecovery_Current")); +} + TEST_F(FeatureStoreTest, GetFeatureInvalidName) { boost::asio::io_context io;