PEL: Handle multiple inv paths per loc code
DataInterface::getInventoryFromLocCode() was only returning a single
inventory path from GetFRUsByExpandedLocationCode() even though multiple
paths may have been returned.
Mostly that was fine, except when a processor on a DCM was called out.
That would lead to only one processor on the DCM being set to not
functional by service_indicators.cpp, so on the web UI the actual CPU
called out may not have been marked as unhealthy (health status critical
in Redfish).
This commit changes getInventoryFromLocCode() to return all the paths
that GetFRUsByExpandedLocationCode() returns, and then makes the
corresponding changes in service_indicators.cpp to be able to handle
multiple inventory paths per location code when setting them to not
functional and creating a critical association.
The other code that was calling this function can just use the first
path returned, since in those cases it's just needed to get the VPD
information for the PEL, and all the paths would return the same info
anyway since they had the same location code.
Signed-off-by: Matt Spinler <spinler@us.ibm.com>
Change-Id: Ia16f50881e4a4f84c171ae20b7a99eddcc98ad4f
diff --git a/extensions/openpower-pels/data_interface.cpp b/extensions/openpower-pels/data_interface.cpp
index 0e31b0e..901fea3 100644
--- a/extensions/openpower-pels/data_interface.cpp
+++ b/extensions/openpower-pels/data_interface.cpp
@@ -500,7 +500,7 @@
return expandedLocationCode;
}
-std::string
+std::vector<std::string>
DataInterface::getInventoryFromLocCode(const std::string& locationCode,
uint16_t node, bool expanded) const
{
@@ -531,21 +531,13 @@
std::vector<sdbusplus::message::object_path> entries;
reply.read(entries);
- // Get the shortest entry from the paths received, as this
- // would be the path furthest up the inventory hierarchy so
- // would be the parent FRU. There is guaranteed to at least
- // be one entry if the call didn't fail.
- std::string shortest{entries[0]};
+ std::vector<std::string> paths;
+ // Note: The D-Bus method will fail if nothing found.
std::for_each(entries.begin(), entries.end(),
- [&shortest](const auto& path) {
- if (path.str.size() < shortest.size())
- {
- shortest = path;
- }
- });
+ [&paths](const auto& path) { paths.push_back(path); });
- return shortest;
+ return paths;
}
void DataInterface::assertLEDGroup(const std::string& ledGroup,
diff --git a/extensions/openpower-pels/data_interface.hpp b/extensions/openpower-pels/data_interface.hpp
index dbc4874..396fc68 100644
--- a/extensions/openpower-pels/data_interface.hpp
+++ b/extensions/openpower-pels/data_interface.hpp
@@ -352,7 +352,7 @@
uint16_t node) const = 0;
/**
- * @brief Returns the inventory path for the FRU that the location
+ * @brief Returns the inventory paths for the FRU that the location
* code represents.
*
* @param[in] locationCode - If an expanded location code, then the
@@ -367,11 +367,11 @@
* @param[in] expanded - If the location code already has the relevent
* VPD fields embedded in it.
*
- * @return std::string - The inventory D-Bus object
+ * @return std::vector<std::string> - The inventory D-Bus objects
*/
- virtual std::string getInventoryFromLocCode(const std::string& LocationCode,
- uint16_t node,
- bool expanded) const = 0;
+ virtual std::vector<std::string>
+ getInventoryFromLocCode(const std::string& LocationCode, uint16_t node,
+ bool expanded) const = 0;
/**
* @brief Sets the Asserted property on the LED group passed in.
@@ -695,7 +695,7 @@
uint16_t node) const override;
/**
- * @brief Returns the inventory path for the FRU that the location
+ * @brief Returns the inventory paths for the FRU that the location
* code represents.
*
* @param[in] locationCode - If an expanded location code, then the
@@ -710,11 +710,11 @@
* @param[in] expanded - If the location code already has the relevent
* VPD fields embedded in it.
*
- * @return std::string - The inventory D-Bus object
+ * @return std::vector<std::string> - The inventory D-Bus objects
*/
- std::string getInventoryFromLocCode(const std::string& locationCode,
- uint16_t node,
- bool expanded) const override;
+ std::vector<std::string>
+ getInventoryFromLocCode(const std::string& locationCode, uint16_t node,
+ bool expanded) const override;
/**
* @brief Sets the Asserted property on the LED group passed in.
diff --git a/extensions/openpower-pels/service_indicators.cpp b/extensions/openpower-pels/service_indicators.cpp
index 5327129..9572f25 100644
--- a/extensions/openpower-pels/service_indicators.cpp
+++ b/extensions/openpower-pels/service_indicators.cpp
@@ -203,9 +203,15 @@
{
try
{
- auto inventoryPath = _dataIface.getInventoryFromLocCode(locCode, 0,
- true);
- paths.push_back(std::move(inventoryPath));
+ auto inventoryPaths = _dataIface.getInventoryFromLocCode(locCode, 0,
+ true);
+ for (const auto& path : inventoryPaths)
+ {
+ if (std::find(paths.begin(), paths.end(), path) == paths.end())
+ {
+ paths.push_back(path);
+ }
+ }
}
catch (const std::exception& e)
{
diff --git a/extensions/openpower-pels/src.cpp b/extensions/openpower-pels/src.cpp
index 0b4de6c..e195fa7 100644
--- a/extensions/openpower-pels/src.cpp
+++ b/extensions/openpower-pels/src.cpp
@@ -1058,12 +1058,12 @@
else
{
// A hardware callout
- std::string inventoryPath;
+ std::vector<std::string> inventoryPaths;
try
{
// Get the inventory item from the unexpanded location code
- inventoryPath =
+ inventoryPaths =
dataIface.getInventoryFromLocCode(regCallout.locCode, 0, false);
}
catch (const std::exception& e)
@@ -1075,7 +1075,8 @@
return;
}
- addInventoryCallout(inventoryPath, priority, locCode, dataIface);
+ // Just use first path returned since they all point to the same FRU.
+ addInventoryCallout(inventoryPaths[0], priority, locCode, dataIface);
}
if (callout)
@@ -1187,10 +1188,13 @@
try
{
- auto inventoryPath = dataIface.getInventoryFromLocCode(
+ auto inventoryPaths = dataIface.getInventoryFromLocCode(
callout.locationCode, 0, false);
- addInventoryCallout(inventoryPath, priority, locCode, dataIface);
+ // Just use first path returned since they all
+ // point to the same FRU.
+ addInventoryCallout(inventoryPaths[0], priority, locCode,
+ dataIface);
}
catch (const std::exception& e)
{
@@ -1328,8 +1332,11 @@
try
{
- inventoryPath = dataIface.getInventoryFromLocCode(
+ auto inventoryPaths = dataIface.getInventoryFromLocCode(
unexpandedLocCode, 0, false);
+ // Just use first path returned since they all
+ // point to the same FRU.
+ inventoryPath = inventoryPaths[0];
}
catch (const std::exception& e)
{
diff --git a/test/openpower-pels/mocks.hpp b/test/openpower-pels/mocks.hpp
index 5c44727..df85066 100644
--- a/test/openpower-pels/mocks.hpp
+++ b/test/openpower-pels/mocks.hpp
@@ -37,7 +37,7 @@
MOCK_METHOD(std::vector<std::string>, getSystemNames, (), (const override));
MOCK_METHOD(std::string, expandLocationCode, (const std::string&, uint16_t),
(const override));
- MOCK_METHOD(std::string, getInventoryFromLocCode,
+ MOCK_METHOD(std::vector<std::string>, getInventoryFromLocCode,
(const std::string&, uint16_t, bool), (const override));
MOCK_METHOD(void, assertLEDGroup, (const std::string&, bool),
(const override));
diff --git a/test/openpower-pels/pel_manager_test.cpp b/test/openpower-pels/pel_manager_test.cpp
index 012e42c..36b1d14 100644
--- a/test/openpower-pels/pel_manager_test.cpp
+++ b/test/openpower-pels/pel_manager_test.cpp
@@ -925,7 +925,8 @@
// Add a PEL with a callout as if hostboot added it
{
EXPECT_CALL(*mockIface, getInventoryFromLocCode("U42", 0, true))
- .WillOnce(Return("/system/chassis/processor"));
+ .WillOnce(
+ Return(std::vector<std::string>{"/system/chassis/processor"}));
EXPECT_CALL(*mockIface,
setFunctional("/system/chassis/processor", false))
@@ -962,11 +963,13 @@
// First call to this is when building the Callout section
EXPECT_CALL(*mockIface, getInventoryFromLocCode("P42-C23", 0, false))
- .WillOnce(Return("/system/chassis/processor"));
+ .WillOnce(
+ Return(std::vector<std::string>{"/system/chassis/processor"}));
// Second call to this is finding the associated LED group
EXPECT_CALL(*mockIface, getInventoryFromLocCode("U42-P42-C23", 0, true))
- .WillOnce(Return("/system/chassis/processor"));
+ .WillOnce(
+ Return(std::vector<std::string>{"/system/chassis/processor"}));
EXPECT_CALL(*mockIface,
setFunctional("/system/chassis/processor", false))
diff --git a/test/openpower-pels/pel_test.cpp b/test/openpower-pels/pel_test.cpp
index b31704b..cc951ec 100644
--- a/test/openpower-pels/pel_test.cpp
+++ b/test/openpower-pels/pel_test.cpp
@@ -860,8 +860,8 @@
.WillOnce(Return("UXXX-P1"));
EXPECT_CALL(dataIface, getInventoryFromLocCode("P1", 0, false))
- .WillOnce(
- Return("/xyz/openbmc_project/inventory/chassis/motherboard/cpu0"));
+ .WillOnce(Return(std::vector<std::string>{
+ "/xyz/openbmc_project/inventory/chassis/motherboard/cpu0"}));
EXPECT_CALL(
dataIface,
@@ -1007,7 +1007,8 @@
.WillOnce(Return("UXXX-P0-C1"));
EXPECT_CALL(dataIface, getInventoryFromLocCode("P0-C1", 0, false))
.Times(1)
- .WillOnce(Return("/inv/system/chassis/motherboard/bmc"));
+ .WillOnce(Return(
+ std::vector<std::string>{"/inv/system/chassis/motherboard/bmc"}));
EXPECT_CALL(dataIface, getHWCalloutFields(
"/inv/system/chassis/motherboard/bmc", _, _, _))
.Times(1)
diff --git a/test/openpower-pels/service_indicators_test.cpp b/test/openpower-pels/service_indicators_test.cpp
index 2c53fd9..f443c64 100644
--- a/test/openpower-pels/service_indicators_test.cpp
+++ b/test/openpower-pels/service_indicators_test.cpp
@@ -268,7 +268,8 @@
service_indicators::LightPath lightPath{dataIface};
EXPECT_CALL(dataIface, getInventoryFromLocCode("U42", 0, true))
- .WillOnce(Return("/system/chassis/processor"));
+ .WillOnce(
+ Return(std::vector<std::string>{"/system/chassis/processor"}));
EXPECT_CALL(dataIface,
setFunctional("/system/chassis/processor", false))
@@ -284,6 +285,32 @@
lightPath.activate(pel);
}
+ // With the same U42 callout, have it be associated with two
+ // inventory paths
+ {
+ MockDataInterface dataIface;
+ service_indicators::LightPath lightPath{dataIface};
+
+ EXPECT_CALL(dataIface, getInventoryFromLocCode("U42", 0, true))
+ .WillOnce(Return(std::vector<std::string>{"/system/chassis/cpu0",
+ "/system/chassis/cpu1"}));
+
+ EXPECT_CALL(dataIface, setFunctional("/system/chassis/cpu0", false))
+ .Times(1);
+ EXPECT_CALL(dataIface, setFunctional("/system/chassis/cpu1", false))
+ .Times(1);
+
+ EXPECT_CALL(dataIface, setCriticalAssociation("/system/chassis/cpu0"))
+ .Times(1);
+ EXPECT_CALL(dataIface, setCriticalAssociation("/system/chassis/cpu1"))
+ .Times(1);
+
+ auto data = pelFactory(1, 'O', 0x20, 0xA400, 500);
+ PEL pel{data};
+
+ lightPath.activate(pel);
+ }
+
// A non-info BMC PEL with no callouts will set the platform SAI LED.
{
MockDataInterface dataIface;
@@ -329,7 +356,8 @@
service_indicators::LightPath lightPath{dataIface};
EXPECT_CALL(dataIface, getInventoryFromLocCode("U42", 0, true))
- .WillOnce(Return("/system/chassis/processor"));
+ .WillOnce(
+ Return(std::vector<std::string>{"/system/chassis/processor"}));
EXPECT_CALL(dataIface,
setFunctional("/system/chassis/processor", false))
@@ -347,7 +375,8 @@
service_indicators::LightPath lightPath{dataIface};
EXPECT_CALL(dataIface, getInventoryFromLocCode("U42", 0, true))
- .WillOnce(Return("/system/chassis/processor"));
+ .WillOnce(
+ Return(std::vector<std::string>{"/system/chassis/processor"}));
EXPECT_CALL(dataIface,
setCriticalAssociation("/system/chassis/processor"))
diff --git a/test/openpower-pels/src_test.cpp b/test/openpower-pels/src_test.cpp
index f072508..f2ade68 100644
--- a/test/openpower-pels/src_test.cpp
+++ b/test/openpower-pels/src_test.cpp
@@ -644,12 +644,12 @@
.WillOnce(Return("UXXX-P0-C9"));
EXPECT_CALL(dataIface, getInventoryFromLocCode("P0-C8", 0, false))
- .WillOnce(Return(
- "/xyz/openbmc_project/inventory/chassis/motherboard/cpu0"));
+ .WillOnce(Return(std::vector<std::string>{
+ "/xyz/openbmc_project/inventory/chassis/motherboard/cpu0"}));
EXPECT_CALL(dataIface, getInventoryFromLocCode("P0-C9", 0, false))
- .WillOnce(Return(
- "/xyz/openbmc_project/inventory/chassis/motherboard/cpu1"));
+ .WillOnce(Return(std::vector<std::string>{
+ "/xyz/openbmc_project/inventory/chassis/motherboard/cpu1"}));
EXPECT_CALL(
dataIface,
@@ -866,18 +866,18 @@
EXPECT_CALL(dataIface, getInventoryFromLocCode("P1-C40", 0, false))
.Times(3)
- .WillRepeatedly(
- Return("/xyz/openbmc_project/inventory/chassis/motherboard/cpu0"));
+ .WillRepeatedly(Return(std::vector<std::string>{
+ "/xyz/openbmc_project/inventory/chassis/motherboard/cpu0"}));
EXPECT_CALL(dataIface, getInventoryFromLocCode("P1", 0, false))
.Times(3)
- .WillRepeatedly(
- Return("/xyz/openbmc_project/inventory/chassis/motherboard"));
+ .WillRepeatedly(Return(std::vector<std::string>{
+ "/xyz/openbmc_project/inventory/chassis/motherboard"}));
EXPECT_CALL(dataIface, getInventoryFromLocCode("P1-C15", 0, false))
.Times(3)
- .WillRepeatedly(
- Return("/xyz/openbmc_project/inventory/chassis/motherboard/bmc"));
+ .WillRepeatedly(Return(std::vector<std::string>{
+ "/xyz/openbmc_project/inventory/chassis/motherboard/bmc"}));
EXPECT_CALL(dataIface, expandLocationCode("P1-C40", 0))
.Times(3)
@@ -1089,7 +1089,8 @@
.WillOnce(Return("UXXX-P0-C1"));
EXPECT_CALL(dataIface, getInventoryFromLocCode("P0-C1", 0, false))
.Times(1)
- .WillOnce(Return("/inv/system/chassis/motherboard/bmc"));
+ .WillOnce(Return(std::vector<std::string>{
+ "/inv/system/chassis/motherboard/bmc"}));
EXPECT_CALL(
dataIface,
getHWCalloutFields("/inv/system/chassis/motherboard/bmc", _, _, _))
@@ -1281,7 +1282,8 @@
EXPECT_CALL(dataIface, getInventoryFromLocCode("P0-C1", 0, false))
.Times(1)
- .WillOnce(Return("/inv/system/chassis/motherboard/bmc"));
+ .WillOnce(Return(std::vector<std::string>{
+ "/inv/system/chassis/motherboard/bmc"}));
EXPECT_CALL(
dataIface,
getHWCalloutFields("/inv/system/chassis/motherboard/bmc", _, _, _))