dbus-sensors: Remove wrong dbus match rule
Use timer to check if needed service is ready.
This will replace the original dbus match rule given that it has better
performance. (Dbus match will match too many times which drops the
performance)
Tested:
Tested in different situation listed below:
* `smbios-mdrv2` provides CPU/DIMM interfaces which is needed.
1. Start `hwmontempsensor` when `smbios-mdrv2` is up then check
association is completed.
2.
2.1 Start `hwmontempsensor` when `smbios-mdrv2` is down then wait 1
minute to ensure nothing crashes.
2.2 Check association is not completed yet.
2.3 Then restart `smbios-mdrv2` and wait 30 seconds (to ensure timer
triggered)
2.3 Check association is completed.
Google-Bug-Id: 296380894
Change-Id: I0b58016f49098ecec4d39e592ee03668f305a9aa
Signed-off-by: Michael Shen <gpgpgp@google.com>
(cherry picked from commit 9381a251a7737a1c09a943fe1167c9e4c7fe8ca7)
diff --git a/recipes-phosphor/sensors/dbus-sensors/0010-dbus-sensors-Creating-association-between-inventory-.patch b/recipes-phosphor/sensors/dbus-sensors/0010-dbus-sensors-Creating-association-between-inventory-.patch
index 13c3f05..6222230 100644
--- a/recipes-phosphor/sensors/dbus-sensors/0010-dbus-sensors-Creating-association-between-inventory-.patch
+++ b/recipes-phosphor/sensors/dbus-sensors/0010-dbus-sensors-Creating-association-between-inventory-.patch
@@ -1,8 +1,8 @@
-From 3ecf9ff5b0475c27a1414d35453120eba002339a Mon Sep 17 00:00:00 2001
+From 503255dc6a1501ee8378146d0835b096b3109657 Mon Sep 17 00:00:00 2001
From: Michael Shen <gpgpgp@google.com>
Date: Tue, 3 Jan 2023 08:38:40 +0000
-Subject: [PATCH] dbus-sensors: Creating association between inventory and
- sensor
+Subject: [PATCH 06/10] dbus-sensors: Creating association between inventory
+ and sensor
This change is based on the https://gerrit.openbmc.org/c/openbmc/dbus-sensors/+/53789
It sets up additional "inventory" association between hwmon sensor and
@@ -53,55 +53,40 @@
Change-Id: I66d5fc880f79bb504dcceb56aee1c6f3d9df73f5
Signed-off-by: Michael Shen <gpgpgp@google.com>
Signed-off-by: Willy Tu <wltu@google.com>
-
-%% original patch: 0010-dbus-sensors-Creating-association-between-inventory-.patch
+Signed-off-by: Michael Shen <gpgpgp@google.com>
---
- src/HwmonTempMain.cpp | 136 +++++++++++++++++++++++++++++++++++++++-
- src/HwmonTempSensor.cpp | 31 +++++++--
+ src/HwmonTempMain.cpp | 218 +++++++++++++++++++++++++++++++++++++++-
+ src/HwmonTempSensor.cpp | 31 ++++--
src/HwmonTempSensor.hpp | 3 +-
- src/Utils.cpp | 13 ++++
+ src/Utils.cpp | 13 +++
src/Utils.hpp | 2 +
- 5 files changed, 176 insertions(+), 9 deletions(-)
+ 5 files changed, 258 insertions(+), 9 deletions(-)
diff --git a/src/HwmonTempMain.cpp b/src/HwmonTempMain.cpp
-index 9c3e473..2f7ef22 100644
+index 0518071..112b3fd 100644
--- a/src/HwmonTempMain.cpp
+++ b/src/HwmonTempMain.cpp
-@@ -23,6 +23,7 @@
- #include <boost/container/flat_set.hpp>
- #include <sdbusplus/asio/connection.hpp>
- #include <sdbusplus/asio/object_server.hpp>
-+#include <sdbusplus/asio/property.hpp>
- #include <sdbusplus/bus/match.hpp>
-
- #include <array>
-@@ -50,6 +51,7 @@ static constexpr double maxValueTemperature = 127; // DegreesC
- static constexpr double minValueTemperature = -128; // DegreesC
-
- namespace fs = std::filesystem;
-+namespace rules = sdbusplus::bus::match::rules;
-
- static const I2CDeviceTypeMap sensorTypes{
- {"DPS310", I2CDeviceType{"dps310", false}},
-@@ -78,6 +80,14 @@ static const I2CDeviceTypeMap sensorTypes{
+@@ -80,6 +80,16 @@ static const I2CDeviceTypeMap sensorTypes{
{"W83773G", I2CDeviceType{"w83773g", true}},
};
-
-+boost::container::flat_map<std::string, std::string> inventoryMap;
-+
+
+static const boost::container::flat_map<std::string, std::vector<std::string>,
+ std::less<>>
+ sensorTargetMap = {
+ {"SBTSI", {"xyz.openbmc_project.Inventory.Item.Cpu"}},
+};
+
++std::vector<std::string> mergedTargetIfaces;
++std::vector<std::shared_ptr<HwmonTempSensor>> unassociatedSensors;
++boost::container::flat_map<std::string, std::string> inventoryMap;
++
static struct SensorParams
getSensorParameters(const std::filesystem::path& path)
{
-@@ -321,6 +331,109 @@ boost::container::flat_map<std::string,
+@@ -323,6 +333,162 @@ boost::container::flat_map<std::string,
return devices;
}
-
+
+void updateLocation(
+ const std::shared_ptr<sdbusplus::asio::connection>& systemBus,
+ std::string_view service, std::string_view path)
@@ -123,32 +108,10 @@
+ inventoryMap[std::get<std::string>(location)] = path;
+}
+
-+void getInventory(const std::shared_ptr<sdbusplus::asio::connection>& systemBus,
-+ const std::vector<std::string>& interfaces)
++void updateInventory(
++ const std::shared_ptr<sdbusplus::asio::connection>& systemBus,
++ const std::vector<std::string>& interfaces)
+{
-+ static boost::container::flat_map<
-+ std::string, std::unique_ptr<sdbusplus::bus::match::match>>
-+ matchers;
-+
-+ static auto handler =
-+ [&systemBus, &interfaces](sdbusplus::message::message&) {
-+ getInventory(systemBus, interfaces);
-+ };
-+
-+ for (const auto& interface : interfaces)
-+ {
-+ if (matchers.contains(interface))
-+ {
-+ continue;
-+ }
-+
-+ matchers[interface] = std::make_unique<sdbusplus::bus::match::match>(
-+ *systemBus,
-+ rules::interfacesAdded() + rules::interface(interface) +
-+ rules::path_namespace(inventoryPath),
-+ handler);
-+ }
-+
+ sdbusplus::message_t getSubTree = systemBus->new_method_call(
+ mapper::busName, mapper::path, mapper::interface, mapper::subtree);
+ getSubTree.append(inventoryPath);
@@ -185,40 +148,123 @@
+// corresponding `sensorName`. For example if the `LocationCode` of a cpu sensor
+// is `CPU1`. Then "Die_CPU1", "CPU1", "CPU1_T", "this_is_cpu1_sensor" are all
+// valid `sensorName`.
-+std::string findInventoryPath(const std::string& sensorName)
++std::optional<std::string> findInventoryPath(const std::string& sensorName)
+{
-+ auto findInventoryPath =
-+ std::find_if(inventoryMap.begin(), inventoryMap.end(),
-+ [&sensorName](const auto& e) {
++ auto matchedPath = std::find_if(inventoryMap.begin(), inventoryMap.end(),
++ [&sensorName](const auto& e) {
+ // ex: e.first(LocationCode)="CPU1" => search="(CPU1$|CPU1[^0-9].*$)"
+ // This will avoid "CPU11", "CPU12" ... to be matched
+ std::regex locationReg("(" + e.first + "$|" + e.first + "[^0-9].*$)",
+ std::regex::icase);
+ return std::regex_match(sensorName, locationReg);
-+ });
++ });
+
-+ if (findInventoryPath == inventoryMap.end())
++ if (matchedPath == inventoryMap.end())
+ {
-+ std::cerr << "Inventory not found for " << sensorName << "\n";
-+ return "";
++ return std::nullopt;
+ }
-+ return findInventoryPath->second;
++ return matchedPath->second;
++}
++
++void deferAssociationCreation(
++ boost::asio::io_context& io,
++ std::shared_ptr<sdbusplus::asio::connection> systemBus,
++ std::shared_ptr<HwmonTempSensor> hwmonSensor,
++ const std::vector<std::string>& targetIfaces)
++{
++ constexpr uint32_t kIntervalSec = 30;
++ constexpr uint32_t kMaxCheckCount = 60;
++ static uint32_t curCount = 0;
++
++ // Only unassociated sensor will get into this function.
++ unassociatedSensors.emplace_back(hwmonSensor);
++ for (const auto& iface : targetIfaces)
++ {
++ if (std::find(mergedTargetIfaces.begin(), mergedTargetIfaces.end(),
++ iface) == mergedTargetIfaces.end())
++ {
++ // `mergedTargetIfaces` is a vector for storing all the possible
++ // DBus interfaces that can associate with sensors.
++ // Then it will be used in `GetSubTree` to find all the related
++ // object paths and check if the path is correct one by one.
++ mergedTargetIfaces.emplace_back(iface);
++ }
++ }
++
++ static boost::asio::steady_timer timer(io);
++ static std::function<void(const boost::system::error_code&)>
++ updateAssociationFunc = [systemBus, kIntervalSec, kMaxCheckCount](
++ const boost::system::error_code& ec) {
++ if (ec == boost::asio::error::operation_aborted)
++ {
++ /* we were canceled*/
++ return;
++ }
++ if (ec)
++ {
++ std::cerr << "timer error\n";
++ return;
++ }
++
++ updateInventory(systemBus, mergedTargetIfaces);
++ unassociatedSensors.erase(std::remove_if(unassociatedSensors.begin(),
++ unassociatedSensors.end(),
++ [](const auto& sensor) {
++ std::optional<std::string> optPath =
++ findInventoryPath(sensor->name);
++ if (optPath == std::nullopt)
++ {
++ return false;
++ }
++
++ if (sensor->association)
++ {
++ std::vector<Association> associations;
++ associations.emplace_back("inventory", "sensors",
++ optPath.value());
++ associations.emplace_back(
++ "chassis", "all_sensors",
++ fs::path(sensor->configurationPath).parent_path().string());
++ sensor->association->set_property("Associations", associations);
++ }
++ return true;
++ }),
++ unassociatedSensors.end());
++
++ // Restart timer if some sensors still not associated.
++ if (++curCount < kMaxCheckCount && !unassociatedSensors.empty())
++ {
++ timer.expires_after(std::chrono::seconds(kIntervalSec));
++ timer.async_wait(updateAssociationFunc);
++ }
++ };
++
++ timer.expires_after(std::chrono::seconds(kIntervalSec));
++ timer.async_wait(updateAssociationFunc);
+}
+
void createSensors(
boost::asio::io_context& io, sdbusplus::asio::object_server& objectServer,
boost::container::flat_map<std::string, std::shared_ptr<HwmonTempSensor>>&
-@@ -507,11 +620,20 @@ void createSensors(
+@@ -509,12 +675,35 @@ void createSensors(
}
else
{
-+ std::string inventoryPath{};
++ std::optional<std::string> inventoryPath = std::nullopt;
+ const auto& foundIfaces =
+ sensorTargetMap.find(getLastNameFromIface(sensorType));
+ if (foundIfaces != sensorTargetMap.end())
+ {
-+ getInventory(dbusConnection, foundIfaces->second);
++ // Every time `updateInventory` updates `inventoryMap`
++ // as much as possible. So we should check if current
++ // sensor's association already in `inventoryMap`
+ inventoryPath = findInventoryPath(sensorName);
++ if (inventoryPath == std::nullopt)
++ {
++ updateInventory(dbusConnection,
++ foundIfaces->second);
++ inventoryPath = findInventoryPath(sensorName);
++ }
+ }
+
sensor = std::make_shared<HwmonTempSensor>(
@@ -226,21 +272,37 @@
io, sensorName, std::move(sensorThresholds),
thisSensorParameters, pollRate, interfacePath,
- readState, i2cDev);
-+ readState, i2cDev, inventoryPath);
++ readState, i2cDev, inventoryPath.value_or(""));
sensor->setupRead();
++ if (inventoryPath == std::nullopt &&
++ foundIfaces != sensorTargetMap.end())
++ {
++ deferAssociationCreation(io, dbusConnection, sensor,
++ foundIfaces->second);
++ }
}
}
-@@ -566,11 +688,21 @@ void createSensors(
+ hwmonName.erase(
+@@ -568,12 +757,37 @@ void createSensors(
}
else
{
-+ std::string inventoryPath{};
++ std::optional<std::string> inventoryPath = std::nullopt;
+ const auto& foundIfaces = sensorTargetMap.find(
+ getLastNameFromIface(sensorType));
+ if (foundIfaces != sensorTargetMap.end())
+ {
-+ getInventory(dbusConnection, foundIfaces->second);
++ // Every time `updateInventory` updates
++ // `inventoryMap` as much as possible. So we should
++ // check if current sensor's association already in
++ // `inventoryMap`
+ inventoryPath = findInventoryPath(sensorName);
++ if (inventoryPath == std::nullopt)
++ {
++ updateInventory(dbusConnection,
++ foundIfaces->second);
++ inventoryPath = findInventoryPath(sensorName);
++ }
+ }
+
sensor = std::make_shared<HwmonTempSensor>(
@@ -249,17 +311,24 @@
std::move(thresholds), thisSensorParameters,
- pollRate, interfacePath, readState, i2cDev);
+ pollRate, interfacePath, readState, i2cDev,
-+ inventoryPath);
++ inventoryPath.value_or(""));
sensor->setupRead();
++ if (inventoryPath == std::nullopt &&
++ foundIfaces != sensorTargetMap.end())
++ {
++ deferAssociationCreation(io, dbusConnection, sensor,
++ foundIfaces->second);
++ }
}
}
+
diff --git a/src/HwmonTempSensor.cpp b/src/HwmonTempSensor.cpp
index bd442f8..11db939 100644
--- a/src/HwmonTempSensor.cpp
+++ b/src/HwmonTempSensor.cpp
@@ -23,6 +23,7 @@
#include <sdbusplus/asio/object_server.hpp>
-
+
#include <charconv>
+#include <filesystem>
#include <iostream>
@@ -268,7 +337,7 @@
@@ -31,6 +32,8 @@
#include <utility>
#include <vector>
-
+
+namespace fs = std::filesystem;
+
// Temperatures are read in milli degrees Celsius, we need degrees Celsius.
@@ -314,7 +383,7 @@
+ setInventoryAssociation(association, inventoryPath, chassisPath);
+ }
}
-
+
bool HwmonTempSensor::isActive()
diff --git a/src/HwmonTempSensor.hpp b/src/HwmonTempSensor.hpp
index d45dafd..f3937f4 100644
@@ -361,6 +430,6 @@
const std::function<void(sdbusplus::message_t&)>& handler);
+
+std::string getLastNameFromIface(const std::string& iface);
---
-2.41.0.585.gd2178a4bd4-goog
+--
+2.35.1
