mdrv2: Attempt to update D-Bus objects in place
When systemInfoUpdate() is called multiple times, there's a potential
race condition of a user polling for the objects while the array of
D-Bus objects are erased and being repopulated. This results in
incomplete set of memory or CPU counts, which can lead to unforseen
consequences.
Tested: Verified that a corner case that was hitting this case
consistently (when SMBIOS was transferred using ipmi-blob repeatedly)
goes away with this implementation.
Signed-off-by: Brandon Kim <brandonkim@google.com>
Change-Id: I312aa91cd11b1dd06502d04272889922108d39a2
diff --git a/include/cpu.hpp b/include/cpu.hpp
index 6328393..bae8dc0 100644
--- a/include/cpu.hpp
+++ b/include/cpu.hpp
@@ -129,10 +129,11 @@
Item, association>(bus, objPath.c_str()),
cpuNum(cpuId), storage(smbiosTableStorage), motherboardPath(motherboard)
{
- infoUpdate();
+ infoUpdate(smbiosTableStorage, motherboard);
}
- void infoUpdate(void);
+ void infoUpdate(uint8_t* smbiosTableStorage,
+ const std::string& motherboard);
private:
uint8_t cpuNum;
diff --git a/include/dimm.hpp b/include/dimm.hpp
index dbeb053..1ea406a 100644
--- a/include/dimm.hpp
+++ b/include/dimm.hpp
@@ -86,13 +86,13 @@
sdbusplus::server::object_t<sdbusplus::server::xyz::openbmc_project::
state::decorator::OperationalStatus>(
bus, objPath.c_str()),
- dimmNum(dimmId), storage(smbiosTableStorage),
- motherboardPath(motherboard)
+ dimmNum(dimmId)
{
- memoryInfoUpdate();
+ memoryInfoUpdate(smbiosTableStorage, motherboard);
}
- void memoryInfoUpdate(void);
+ void memoryInfoUpdate(uint8_t* smbiosTableStorage,
+ const std::string& motherboard);
uint16_t memoryDataWidth(uint16_t value) override;
size_t memorySizeInKB(size_t value) override;
diff --git a/include/pcieslot.hpp b/include/pcieslot.hpp
index f984650..ccedcb9 100644
--- a/include/pcieslot.hpp
+++ b/include/pcieslot.hpp
@@ -47,13 +47,13 @@
const std::string& motherboard) :
sdbusplus::server::object_t<PCIeSlot, location, embedded, item,
association>(bus, objPath.c_str()),
- pcieNum(pcieId), storage(smbiosTableStorage),
- motherboardPath(motherboard)
+ pcieNum(pcieId)
{
- pcieInfoUpdate();
+ pcieInfoUpdate(smbiosTableStorage, motherboard);
}
- void pcieInfoUpdate();
+ void pcieInfoUpdate(uint8_t* smbiosTableStorage,
+ const std::string& motherboard);
private:
uint8_t pcieNum;
diff --git a/src/cpu.cpp b/src/cpu.cpp
index 7e52bd4..ed4eabb 100644
--- a/src/cpu.cpp
+++ b/src/cpu.cpp
@@ -119,8 +119,12 @@
}
static constexpr uint8_t maxOldVersionCount = 0xff;
-void Cpu::infoUpdate(void)
+void Cpu::infoUpdate(uint8_t* smbiosTableStorage,
+ const std::string& motherboard)
{
+ storage = smbiosTableStorage;
+ motherboardPath = motherboard;
+
uint8_t* dataIn = storage;
dataIn = getSMBIOSTypePtr(dataIn, processorsType);
diff --git a/src/dimm.cpp b/src/dimm.cpp
index d106d47..c84bc51 100644
--- a/src/dimm.cpp
+++ b/src/dimm.cpp
@@ -39,8 +39,12 @@
sdbusplus::server::xyz::openbmc_project::inventory::item::Dimm::Ecc;
static constexpr uint16_t maxOldDimmSize = 0x7fff;
-void Dimm::memoryInfoUpdate(void)
+void Dimm::memoryInfoUpdate(uint8_t* smbiosTableStorage,
+ const std::string& motherboard)
{
+ storage = smbiosTableStorage;
+ motherboardPath = motherboard;
+
uint8_t* dataIn = storage;
dataIn = getSMBIOSTypePtr(dataIn, memoryDeviceType);
diff --git a/src/mdrv2.cpp b/src/mdrv2.cpp
index dd1d8e3..856d817 100644
--- a/src/mdrv2.cpp
+++ b/src/mdrv2.cpp
@@ -445,7 +445,6 @@
phosphor::logging::entry("ERROR=%s", e.what()));
}
- cpus.clear();
int num = getTotalCpuSlot();
if (num == -1)
{
@@ -454,18 +453,30 @@
return;
}
+ // In case the new size is smaller than old, trim the vector
+ if (num < cpus.size())
+ {
+ cpus.erase(cpus.begin() + num, cpus.end());
+ }
+
for (int index = 0; index < num; index++)
{
std::string path = cpuPath + std::to_string(index);
- cpus.emplace_back(std::make_unique<phosphor::smbios::Cpu>(
- bus, path, index, smbiosDir.dir[smbiosDirIndex].dataStorage,
- motherboardPath));
+ if (index + 1 > cpus.size())
+ {
+ cpus.emplace_back(std::make_unique<phosphor::smbios::Cpu>(
+ bus, path, index, smbiosDir.dir[smbiosDirIndex].dataStorage,
+ motherboardPath));
+ }
+ else
+ {
+ cpus[index]->infoUpdate(smbiosDir.dir[smbiosDirIndex].dataStorage,
+ motherboardPath);
+ }
}
#ifdef DIMM_DBUS
- dimms.clear();
-
num = getTotalDimmSlot();
if (num == -1)
{
@@ -474,17 +485,30 @@
return;
}
+ // In case the new size is smaller than old, trim the vector
+ if (num < dimms.size())
+ {
+ dimms.erase(dimms.begin() + num, dimms.end());
+ }
+
for (int index = 0; index < num; index++)
{
std::string path = dimmPath + std::to_string(index);
- dimms.emplace_back(std::make_unique<phosphor::smbios::Dimm>(
- bus, path, index, smbiosDir.dir[smbiosDirIndex].dataStorage,
- motherboardPath));
+ if (index + 1 > dimms.size())
+ {
+ dimms.emplace_back(std::make_unique<phosphor::smbios::Dimm>(
+ bus, path, index, smbiosDir.dir[smbiosDirIndex].dataStorage,
+ motherboardPath));
+ }
+ else
+ {
+ dimms[index]->memoryInfoUpdate(
+ smbiosDir.dir[smbiosDirIndex].dataStorage, motherboardPath);
+ }
}
#endif
- pcies.clear();
num = getTotalPcieSlot();
if (num == -1)
{
@@ -493,12 +517,26 @@
return;
}
+ // In case the new size is smaller than old, trim the vector
+ if (num < pcies.size())
+ {
+ pcies.erase(pcies.begin() + num, pcies.end());
+ }
+
for (int index = 0; index < num; index++)
{
std::string path = pciePath + std::to_string(index);
- pcies.emplace_back(std::make_unique<phosphor::smbios::Pcie>(
- bus, path, index, smbiosDir.dir[smbiosDirIndex].dataStorage,
- motherboardPath));
+ if (index + 1 > pcies.size())
+ {
+ pcies.emplace_back(std::make_unique<phosphor::smbios::Pcie>(
+ bus, path, index, smbiosDir.dir[smbiosDirIndex].dataStorage,
+ motherboardPath));
+ }
+ else
+ {
+ pcies[index]->pcieInfoUpdate(
+ smbiosDir.dir[smbiosDirIndex].dataStorage, motherboardPath);
+ }
}
system.reset();
diff --git a/src/pcieslot.cpp b/src/pcieslot.cpp
index 0ea7a8b..4b109d0 100644
--- a/src/pcieslot.cpp
+++ b/src/pcieslot.cpp
@@ -8,8 +8,12 @@
namespace smbios
{
-void Pcie::pcieInfoUpdate()
+void Pcie::pcieInfoUpdate(uint8_t* smbiosTableStorage,
+ const std::string& motherboard)
{
+ storage = smbiosTableStorage;
+ motherboardPath = motherboard;
+
uint8_t* dataIn = getSMBIOSTypePtr(storage, systemSlots);
if (dataIn == nullptr)