From 0d1a915ebb69d9f2d1da755b5f588ecac706ac7b Mon Sep 17 00:00:00 2001 From: Justin Kim Date: Tue, 24 Sep 2024 12:46:14 -0700 Subject: [PATCH] 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 --- cmake/PlatformPlatformManager.cmake | 1 + fboss/platform/platform_manager/BUCK | 1 + .../platform/platform_manager/I2cExplorer.cpp | 38 ++++++------ fboss/platform/platform_manager/I2cExplorer.h | 1 - .../platform/platform_manager/PciExplorer.cpp | 61 ++++++++----------- fboss/platform/platform_manager/PciExplorer.h | 4 -- fboss/platform/platform_manager/Utils.cpp | 19 ++++++ fboss/platform/platform_manager/Utils.h | 10 +++ .../tests/I2cExplorerTest.cpp | 10 +-- 9 files changed, 79 insertions(+), 66 deletions(-) diff --git a/cmake/PlatformPlatformManager.cmake b/cmake/PlatformPlatformManager.cmake index ff7afe11e83f7..4b9f75399d265 100644 --- 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 index deda923127e2c..82f42f0309cc3 100644 --- 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 index 346475aa44ed4..5539736413206 100644 --- a/fboss/platform/platform_manager/I2cExplorer.cpp +++ b/fboss/platform/platform_manager/I2cExplorer.cpp @@ -1,7 +1,6 @@ // (c) Meta Platforms, Inc. and affiliates. Confidential and proprietary. #include "fboss/platform/platform_manager/I2cExplorer.h" -#include "fboss/lib/i2c/I2cDevIo.h" #include #include @@ -9,12 +8,15 @@ #include #include +#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 index e8d7a36c4255d..b4ce2cc5731ee 100644 --- 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_{}; }; diff --git a/fboss/platform/platform_manager/PciExplorer.cpp b/fboss/platform/platform_manager/PciExplorer.cpp index da0e4f3ce4e52..10a974e1ce8aa 100644 --- 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\\d+).(?P\\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 index 0353a2bdb5abb..ea56b73092d04 100644 --- 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 index 0ca25e8c8118b..6f81ce1bf9d3c 100644 --- 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&& 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 index 52b9e931c2c7f..0a2e0ceb0bab5 100644 --- a/fboss/platform/platform_manager/Utils.h +++ b/fboss/platform/platform_manager/Utils.h @@ -2,6 +2,11 @@ #pragma once +#include +#include +#include +#include + #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&& 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 index 28a5a85b335d0..1d981d1ff9fad 100644 --- 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 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, 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"))