From 0d0821e4fdad6c18373e9e1b30d6e82afb0bdd74 Mon Sep 17 00:00:00 2001
From: Jimmy Ye <jimmyy@meta.com>
Date: Mon, 16 Sep 2024 15:34:13 -0700
Subject: [PATCH] Fix hwmon+fw_ver handling when publishing firmware versions

Summary: Previously missed a case where fw_ver is under hwmon directory. Simple fix to reorder logic and check for hwmon before checking for fw_ver.

Reviewed By: alandau

Differential Revision: D62774963

fbshipit-source-id: 8023d7308d9360f434ba84341232b07d6baaf689
---
 .../platform_manager/PlatformExplorer.cpp     | 28 +++++++++----------
 .../tests/PlatformExplorerTest.cpp            |  6 ++++
 2 files changed, 20 insertions(+), 14 deletions(-)

diff --git a/fboss/platform/platform_manager/PlatformExplorer.cpp b/fboss/platform/platform_manager/PlatformExplorer.cpp
index d3ff2b74d02f0..ead835997d92b 100644
--- a/fboss/platform/platform_manager/PlatformExplorer.cpp
+++ b/fboss/platform/platform_manager/PlatformExplorer.cpp
@@ -699,26 +699,26 @@ void PlatformExplorer::publishFirmwareVersions() {
     const auto deviceName = linkPathParts.back();
     std::string versionString;
     int odsValue = 0;
+    std::string verDirPath = linkPath;
+    // Check for and handle hwmon case. e.g.
+    // /run/devmap/cplds/FAN0_CPLD/hwmon/hwmon20/
+    auto hwmonSubdirPath = std::filesystem::path(linkPath) / "hwmon";
+    if (platformFsUtils_->exists(hwmonSubdirPath)) {
+      for (const auto& entry : platformFsUtils_->ls(hwmonSubdirPath)) {
+        if (entry.is_directory() &&
+            entry.path().filename().string().starts_with("hwmon")) {
+          verDirPath = hwmonSubdirPath / entry.path().filename();
+          break;
+        }
+      }
+    }
     // New-style fw_ver
-    auto fwVerFilePath = std::filesystem::path(linkPath) / "fw_ver";
+    auto fwVerFilePath = std::filesystem::path(verDirPath) / "fw_ver";
     if (platformFsUtils_->exists(fwVerFilePath)) {
       auto version = readVersionString(fwVerFilePath, platformFsUtils_.get());
       versionString = version.first;
       odsValue = version.second;
     } else {
-      std::string verDirPath = linkPath;
-      // Check for and handle hwmon case. e.g.
-      // /run/devmap/cplds/FAN0_CPLD/hwmon/hwmon20/
-      auto hwmonSubdirPath = std::filesystem::path(linkPath) / "hwmon";
-      if (platformFsUtils_->exists(hwmonSubdirPath)) {
-        for (const auto& entry : platformFsUtils_->ls(hwmonSubdirPath)) {
-          if (entry.is_directory() &&
-              entry.path().filename().string().starts_with("hwmon")) {
-            verDirPath = hwmonSubdirPath / entry.path().filename();
-            break;
-          }
-        }
-      }
       const auto version = readVersionNumber(
           fmt::format("{}/{}_ver", verDirPath, deviceType),
           platformFsUtils_.get());
diff --git a/fboss/platform/platform_manager/tests/PlatformExplorerTest.cpp b/fboss/platform/platform_manager/tests/PlatformExplorerTest.cpp
index 7f75572dd652d..5acd0db0e35b7 100644
--- a/fboss/platform/platform_manager/tests/PlatformExplorerTest.cpp
+++ b/fboss/platform/platform_manager/tests/PlatformExplorerTest.cpp
@@ -72,6 +72,10 @@ TEST(PlatformExplorerTest, PublishFirmwareVersions) {
   std::string cpldBadFwVerPath = "/run/devmap/cplds/TEST_CPLD_BADFWVER";
   EXPECT_TRUE(platformFsUtils->writeStringToFile(
       "123.456.789 // comment", fmt::format("{}/fw_ver", cpldBadFwVerPath)));
+  // Case with fw_ver under hwmon
+  std::string cpldHwmonFwVerPath = "/run/devmap/cplds/FAN0_CPLD_FWVER";
+  EXPECT_TRUE(platformFsUtils->writeStringToFile(
+      "1.2.3", fmt::format("{}/hwmon/hwmon20/fw_ver", cpldHwmonFwVerPath)));
 
   PlatformConfig platformConfig;
   platformConfig.symbolicLinkToDevicePath()[fpgaPath] = "";
@@ -82,6 +86,7 @@ TEST(PlatformExplorerTest, PublishFirmwareVersions) {
   platformConfig.symbolicLinkToDevicePath()[fpgaFwVerPath] = "";
   platformConfig.symbolicLinkToDevicePath()[cpldFwVerPath] = "";
   platformConfig.symbolicLinkToDevicePath()[cpldBadFwVerPath] = "";
+  platformConfig.symbolicLinkToDevicePath()[cpldHwmonFwVerPath] = "";
 
   PlatformExplorer explorer(platformConfig, platformFsUtils);
   explorer.publishFirmwareVersions();
@@ -95,6 +100,7 @@ TEST(PlatformExplorerTest, PublishFirmwareVersions) {
   expectVersions("TEST_FPGA_FWVER", "1.2", 1'002'000);
   expectVersions("TEST_CPLD_FWVER", "123.456.789", 123456789);
   expectVersions("TEST_CPLD_BADFWVER", "ERROR_INVALID_STRING", 0);
+  expectVersions("FAN0_CPLD_FWVER", "1.2.3", 1'002'003);
 }
 
 } // namespace facebook::fboss::platform::platform_manager