Shared device readiness granular checking

Summary: Before when PM waits for device creation, it just waited for full 5 seconds. This is going to be inefficient with more device creation waitings. So moving to granular checking => check every x ms until 5 seconds. So if PM has an opportunity to exit before 5s. This'll help with device creation success cases. Since I2CExplorer and PciExplorer both relies on this, I implemented in Utils.cpp Reviewed By: somasun Differential Revision: D62465134 fbshipit-source-id: 6bcd1da9714deddd6f69e471a9e5f9309ce1d6a9
facebook · Sep 24, 2024 · 0d1a915 · 0d1a915
1 parent 158a52f
commit 0d1a915
Show file tree

Hide file tree

Showing 9 changed files with 79 additions and 66 deletions.
diff --git a/cmake/PlatformPlatformManager.cmake b/cmake/PlatformPlatformManager.cmake
@@ -49,6 +49,7 @@ add_library(platform_manager_i2c_explorer
 target_link_libraries(platform_manager_i2c_explorer
   fmt::fmt
   platform_manager_config_cpp2
+  platform_manager_utils
   i2c_ctrl
   platform_utils
   Folly::folly

diff --git a/fboss/platform/platform_manager/BUCK b/fboss/platform/platform_manager/BUCK
@@ -69,6 +69,7 @@ cpp_library(
         "fbsource//third-party/fmt:fmt",
         ":i2c_misc",
         ":platform_manager_config-cpp2-types",
+        ":utils",
     ],
     exported_external_deps = [
         "re2",

diff --git a/fboss/platform/platform_manager/I2cExplorer.cpp b/fboss/platform/platform_manager/I2cExplorer.cpp
@@ -1,20 +1,22 @@
 // (c) Meta Platforms, Inc. and affiliates. Confidential and proprietary.
 
 #include "fboss/platform/platform_manager/I2cExplorer.h"
-#include "fboss/lib/i2c/I2cDevIo.h"
 
 #include <folly/FileUtil.h>
 #include <folly/String.h>
 #include <folly/logging/xlog.h>
 #include <filesystem>
 #include <stdexcept>
 
+#include "fboss/lib/i2c/I2cDevIo.h"
+#include "fboss/platform/platform_manager/Utils.h"
+
 namespace fs = std::filesystem;
 
 namespace {
 
 const re2::RE2 kI2cMuxChannelRegex{"channel-\\d+"};
-constexpr auto kI2cDevCreationWaitSecs = 5;
+constexpr auto kI2cDevCreationWaitSecs = std::chrono::seconds(5);
 constexpr auto kCpuI2cBusNumsWaitSecs = 1;
 
 std::string getI2cAdapterName(const fs::path& busPath) {
@@ -162,7 +164,7 @@ void I2cExplorer::createI2cDevice(
       busNum);
   auto [exitStatus, standardOut] = platformUtils_->execCommand(cmd);
   XLOG_IF(INFO, !standardOut.empty()) << standardOut;
-  if (exitStatus != 0 || !isI2cDeviceCreated(busNum, addr)) {
+  if (exitStatus != 0) {
     throw std::runtime_error(fmt::format(
         "Failed to create i2c device for {} ({}) at bus: {}, addr: {} with exit status {}",
         pmUnitScopedName,
@@ -171,6 +173,21 @@ void I2cExplorer::createI2cDevice(
         addr.hex2Str(),
         exitStatus));
   }
+  if (!Utils().checkDeviceReadiness(
+          [&]() -> bool { return isI2cDevicePresent(busNum, addr); },
+          fmt::format(
+              "I2cDevice at busNum: {} and addr: {} is not yet initialized. Waiting for at most {}s",
+              busNum,
+              addr.hex4Str(),
+              kI2cDevCreationWaitSecs.count()),
+          kI2cDevCreationWaitSecs)) {
+    throw std::runtime_error(fmt::format(
+        "Failed to initialize i2c device for {} ({}) at bus: {}, addr: {}",
+        pmUnitScopedName,
+        deviceName,
+        busNum,
+        addr.hex2Str()));
+  }
   XLOG(INFO) << fmt::format(
       "Created i2c device {} ({}) at {}",
       pmUnitScopedName,
@@ -218,19 +235,4 @@ std::string I2cExplorer::getDeviceI2cPath(
 std::string I2cExplorer::getI2cBusCharDevPath(uint16_t busNum) {
   return fmt::format("/dev/i2c-{}", busNum);
 }
-
-bool I2cExplorer::isI2cDeviceCreated(uint16_t busNum, const I2cAddr& addr)
-    const {
-  if (isI2cDevicePresent(busNum, addr)) {
-    return true;
-  }
-  XLOG(INFO) << fmt::format(
-      "I2cDevice at busNum: {} and addr: {} is not yet created. Waiting for {}s",
-      busNum,
-      addr.hex4Str(),
-      kI2cDevCreationWaitSecs);
-  sleep(kI2cDevCreationWaitSecs);
-  return isI2cDevicePresent(busNum, addr);
-}
-
 } // namespace facebook::fboss::platform::platform_manager
diff --git a/fboss/platform/platform_manager/I2cExplorer.h b/fboss/platform/platform_manager/I2cExplorer.h
@@ -78,7 +78,6 @@ class I2cExplorer {
   static std::string getI2cBusCharDevPath(uint16_t busNum);
 
  private:
-  virtual bool isI2cDeviceCreated(uint16_t busNum, const I2cAddr& addr) const;
   std::shared_ptr<PlatformUtils> platformUtils_{};
 };
 

diff --git a/fboss/platform/platform_manager/PciExplorer.cpp b/fboss/platform/platform_manager/PciExplorer.cpp
@@ -19,8 +19,7 @@ using namespace facebook::fboss::platform::platform_manager;
 namespace {
 const re2::RE2 kSpiBusRe{"spi\\d+"};
 const re2::RE2 kSpiDevIdRe{"spi(?P<BusNum>\\d+).(?P<ChipSelect>\\d+)"};
-// TODO (T181346009) for more granular interval retries.
-constexpr auto kPciWaitSecs = 5;
+constexpr auto kPciWaitSecs = std::chrono::seconds(5);
 
 bool hasEnding(std::string const& input, std::string const& ending) {
   if (input.length() >= ending.length()) {
@@ -64,15 +63,15 @@ PciDevice::PciDevice(
       std::string(subSystemVendorId, 2, 4),
       std::string(subSystemDeviceId, 2, 4));
 
-  if (!fs::exists(charDevPath_)) {
-    XLOG(INFO) << fmt::format(
-        "No character device found at {} for {}. Waiting for {}s",
-        charDevPath_,
-        name,
-        kPciWaitSecs);
-    sleep(kPciWaitSecs);
-  }
-  if (!fs::exists(charDevPath_)) {
+  if (!Utils().checkDeviceReadiness(
+          [&charDevPath_ = charDevPath_]() -> bool {
+            return fs::exists(charDevPath_);
+          },
+          fmt::format(
+              "No character device found at {} for {}. Waiting for at most {}s",
+              charDevPath_,
+              name,
+              kPciWaitSecs.count()))) {
     throw std::runtime_error(fmt::format(
         "No character device found at {} for {}. This could either mean the "
         "FPGA does not show up as PCI device (see lspci output), or the kmods "
@@ -278,7 +277,7 @@ void PciExplorer::create(
   // PciSubDevice creation failure cases.
   // 1. ioctl() failures (ret < 0).
   // More details: https://man7.org/linux/man-pages/man2/ioctl.2.html#ERRORS
-  // 2. PciSubDevice driver binding failure (checkPciSubDeviceReadiness =
+  // 2. PciSubDevice driver binding failure (checkDeviceReadiness =
   // false).
   if (ret < 0) {
     throw std::runtime_error(fmt::format(
@@ -293,8 +292,20 @@ void PciExplorer::create(
         auxData.csr_offset,
         auxData.iobuf_offset,
         folly::errnoStr(savedErrno)));
-  } else if (!checkPciSubDeviceReadiness(
-                 pciDevice, fpgaIpBlockConfig, auxData.id.id)) {
+  }
+  if (!Utils().checkDeviceReadiness(
+          [&]() -> bool {
+            return isPciSubDeviceReady(
+                pciDevice, fpgaIpBlockConfig, auxData.id.id);
+          },
+          fmt::format(
+              "PciSubDevice {} with deviceName {} and instId {} is not yet initialized "
+              "at {}. Waiting for at most {}",
+              *fpgaIpBlockConfig.pmUnitScopedName(),
+              *fpgaIpBlockConfig.deviceName(),
+              auxData.id.id,
+              pciDevice.sysfsPath(),
+              kPciWaitSecs.count()))) {
     throw std::runtime_error(fmt::format(
         "Failed to initialize device {} in {} using {}. "
         "Args - deviceName: {} instanceId: {}, "
@@ -557,28 +568,6 @@ std::string PciExplorer::getXcvrCtrlSysfsPath(
       pciDevice.sysfsPath()));
 }
 
-bool PciExplorer::checkPciSubDeviceReadiness(
-    const PciDevice& pciDevice,
-    const FpgaIpBlockConfig& fpgaIpBlockConfig,
-    uint32_t instanceId) {
-  if (isPciSubDeviceReady(pciDevice, fpgaIpBlockConfig, instanceId)) {
-    return true;
-  }
-  XLOG(INFO) << fmt::format(
-      "PciSubDevice {} with deviceName {} and instId {} is not yet created "
-      "at {}. Waiting for {}s",
-      *fpgaIpBlockConfig.pmUnitScopedName(),
-      *fpgaIpBlockConfig.deviceName(),
-      instanceId,
-      pciDevice.sysfsPath(),
-      kPciWaitSecs);
-  sleep(kPciWaitSecs);
-  if (isPciSubDeviceReady(pciDevice, fpgaIpBlockConfig, instanceId)) {
-    return true;
-  }
-  return false;
-}
-
 bool PciExplorer::isPciSubDeviceReady(
     const PciDevice& pciDevice,
     const FpgaIpBlockConfig& fpgaIpBlockConfig,

diff --git a/fboss/platform/platform_manager/PciExplorer.h b/fboss/platform/platform_manager/PciExplorer.h
@@ -145,10 +145,6 @@ class PciExplorer {
       const PciDevice& pciDevice,
       const FpgaIpBlockConfig& fpgaIpBlockConfig,
       uint32_t instanceId);
-  bool checkPciSubDeviceReadiness(
-      const PciDevice& pciDevice,
-      const FpgaIpBlockConfig& fpgaIpBlockConfig,
-      uint32_t instanceId);
   bool isPciSubDeviceReady(
       const PciDevice& pciDevice,
       const FpgaIpBlockConfig& fpgaIpBlockConfig,

diff --git a/fboss/platform/platform_manager/Utils.cpp b/fboss/platform/platform_manager/Utils.cpp
@@ -170,6 +170,25 @@ std::string Utils::resolveWatchdogCharDevPath(const std::string& sysfsPath) {
   return charDevPath;
 }
 
+bool Utils::checkDeviceReadiness(
+    std::function<bool()>&& isDeviceReadyFunc,
+    const std::string& onWaitMsg,
+    std::chrono::seconds maxWaitSecs) {
+  if (isDeviceReadyFunc()) {
+    return true;
+  }
+  XLOG(INFO) << onWaitMsg;
+  std::chrono::milliseconds waitIntervalMs = std::chrono::milliseconds(50);
+  auto start = std::chrono::steady_clock::now();
+  do {
+    std::this_thread::sleep_for(waitIntervalMs);
+    if (isDeviceReadyFunc()) {
+      return true;
+    }
+  } while (std::chrono::steady_clock::now() <= start + maxWaitSecs);
+  return false;
+}
+
 int Utils::getGpioLineValue(const std::string& charDevPath, int lineIndex)
     const {
   struct gpiod_chip* chip = gpiod_chip_open(charDevPath.c_str());

diff --git a/fboss/platform/platform_manager/Utils.h b/fboss/platform/platform_manager/Utils.h
@@ -2,6 +2,11 @@
 
 #pragma once
 
+#include <chrono>
+#include <functional>
+#include <optional>
+#include <string>
+
 #include "fboss/platform/platform_manager/gen-cpp2/platform_manager_config_types.h"
 
 namespace facebook::fboss::platform::platform_manager {
@@ -32,6 +37,11 @@ class Utils {
   // Throws an exception when it fails to resolve CharDevicePath
   std::string resolveWatchdogCharDevPath(const std::string& sysfsPath);
 
+  bool checkDeviceReadiness(
+      std::function<bool()>&& isDeviceReadyFunc,
+      const std::string& onWaitMsg,
+      std::chrono::seconds maxWaitSecs = std::chrono::seconds(1));
+
   virtual int getGpioLineValue(const std::string& charDevPath, int lineIndex)
       const;
 };

diff --git a/fboss/platform/platform_manager/tests/I2cExplorerTest.cpp b/fboss/platform/platform_manager/tests/I2cExplorerTest.cpp
@@ -17,7 +17,6 @@ class MockI2cExplorer : public I2cExplorer {
   explicit MockI2cExplorer(std::shared_ptr<MockPlatformUtils> platformUtils)
       : I2cExplorer(platformUtils) {}
   MOCK_METHOD(bool, isI2cDevicePresent, (uint16_t, const I2cAddr&), (const));
-  MOCK_METHOD(bool, isI2cDeviceCreated, (uint16_t, const I2cAddr&), (const));
   MOCK_METHOD(
       std::optional<std::string>,
       getI2cDeviceName,
@@ -32,8 +31,7 @@ TEST(I2cExplorerTest, createI2cDeviceSuccess) {
 
   // CASE-1: No device present; creation succeeds.
   EXPECT_CALL(i2cExplorer, isI2cDevicePresent(4, I2cAddr(15)))
-      .WillOnce(Return(false));
-  EXPECT_CALL(i2cExplorer, isI2cDeviceCreated(4, I2cAddr(15)))
+      .WillOnce(Return(false))
       .WillOnce(Return(true));
   EXPECT_CALL(
       *platformUtils,
@@ -56,7 +54,7 @@ TEST(I2cExplorerTest, createI2cDeviceFailure) {
   auto i2cExplorer = MockI2cExplorer(platformUtils);
   // CASE-1: echoing lm75 15 into the new_device file fails.
   EXPECT_CALL(i2cExplorer, isI2cDevicePresent(4, I2cAddr(15)))
-      .WillOnce(Return(false));
+      .WillRepeatedly(Return(false));
   EXPECT_CALL(
       *platformUtils,
       execCommand("echo lm73 0x0f > /sys/bus/i2c/devices/i2c-4/new_device"))
@@ -67,9 +65,7 @@ TEST(I2cExplorerTest, createI2cDeviceFailure) {
 
   // CASE-2: echoing succeeded but I2C device not created.
   EXPECT_CALL(i2cExplorer, isI2cDevicePresent(3, I2cAddr(16)))
-      .WillOnce(Return(false));
-  EXPECT_CALL(i2cExplorer, isI2cDeviceCreated(3, I2cAddr(16)))
-      .WillOnce(Return(false));
+      .WillRepeatedly(Return(false));
   EXPECT_CALL(
       *platformUtils,
       execCommand("echo pmbus 0x10 > /sys/bus/i2c/devices/i2c-3/new_device"))