test(amdsmi): cover malformed FCLK tables

projects/amdsmi/CHANGELOG.md

@@ -203,6 +203,12 @@ Full documentation for amd_smi_lib is available at [https://rocm.docs.amd.com/pr
 - **Fixed GPU metrics returning no data on integrated GPUs**.  
   - `amdsmi_get_gpu_metrics_info()` and `amdsmi_get_gpu_activity()` failed on APUs such as gfx1103, leaving PLX temperature, GPU utilization and `amd-smi metric` empty. Those devices now report the values they provide with the rest marked N/A, and any GPU without a PLX sensor now reports N/A there instead of a placeholder reading.
 
+- **Preserved errors from empty GPU clock tables**.  
+  - Clock queries retain errors for empty or blank driver data instead of reporting the feature as unsupported.
+
+- **Fixed `amd-smi reset --clocks` leaving a lowered FCLK cap in place**.  
+  - The reset restores the top supported FCLK level and reports restoration failures instead of hiding them.
+
 - **Fixed `amd-smi` printing a Python traceback when an unknown NIC or switch is selected**.  
   - `amd-smi static --nic 999` and `--switch 999` failed while building the "device not found" error, so the command exited `1` with a traceback and no readable message. `--json` and `--csv` produced no parseable output.
   - Both now report `Can not find a device: NIC '999'` (or `SWITCH`) and exit `196`, matching `--gpu`, `--cpu`, and `--core`.

projects/amdsmi/amdsmi_cli/subcommands/reset.py

@@ -14,6 +14,24 @@
 
 
 class ResetCommands:
+    @staticmethod
+    def _restore_fclk_cap(gpu):
+        """Return whether a lowered FCLK cap was raised; AUTO alone leaves it set."""
+        clk_type = amdsmi_interface.AmdSmiClkType.DF
+        try:
+            levels = amdsmi_interface.amdsmi_get_clk_freq(gpu, clk_type)["frequency"]
+            current_max = amdsmi_interface.amdsmi_get_clock_info(gpu, clk_type)["max_clk"]
+        except amdsmi_exception.AmdSmiLibraryException as e:
+            if e.get_error_code() != amdsmi_interface.amdsmi_wrapper.AMDSMI_STATUS_NOT_SUPPORTED:
+                raise
+            logging.debug("FCLK cap is not available | %s", e.get_error_info())
+            return False
+        top_mhz = max((hz for hz in levels if hz > 0), default=0) // 1_000_000
+        if not top_mhz or not isinstance(current_max, int) or current_max >= top_mhz:
+            return False
+        amdsmi_interface.amdsmi_set_gpu_clk_limit(gpu, "fclk", "max", top_mhz)
+        return True
+
     def reset(
         self,
         args,
@@ -229,6 +247,18 @@ def reset(
                     )
                     self.helpers.error_collector.record_library_error(e.get_error_code())
                     # continue to reset clocks and performance level
+                try:
+                    self._restore_fclk_cap(args.gpu)
+                except amdsmi_exception.AmdSmiLibraryException as e:
+                    if e.get_error_code() == amdsmi_interface.amdsmi_wrapper.AMDSMI_STATUS_NO_PERM:
+                        raise PermissionError("Command requires elevation") from e
+                    reset_clocks_results["fclk"] = (
+                        f"[{e.get_error_info(detailed=False)}] Unable to restore FCLK cap"
+                    )
+                    self.helpers.error_collector.record_library_error(e.get_error_code())
+                    logging.debug(
+                        "Failed to restore fclk cap on gpu %s | %s", gpu_id, e.get_error_info()
+                    )
                 try:
                     level_auto = amdsmi_interface.AmdSmiDevPerfLevel.AUTO
                     amdsmi_interface.amdsmi_set_gpu_perf_level(args.gpu, level_auto)

projects/amdsmi/docs/how-to/amdsmi-cli-tool.md

@@ -905,6 +905,11 @@ Command Modifiers:
                                 DEBUG, INFO, WARNING, ERROR, CRITICAL
 ```
 
+`reset --clocks` also restores a lowered FCLK cap to the highest supported DPM
+level. Unsupported FCLK queries skip this step. Clock-query and cap-restoration
+errors are reported in `reset_clocks.fclk` and the exit status while the command
+still attempts to reset the performance level.
+
 (cmd-monitor)=
 ### amd-smi monitor
 

projects/amdsmi/rocm_smi/src/rocm_smi.cc

@@ -96,7 +96,6 @@ static uint64_t get_multiplier_from_char(char units_char) {
       break;
 
     default:
-      assert(false);  // Unexpected units for frequency
       throw amd::smi::rsmi_exception(RSMI_STATUS_UNEXPECTED_DATA, __FUNCTION__);
   }
   return multiplier;

projects/amdsmi/rocm_smi/src/rocm_smi_device.cc

@@ -1281,6 +1281,11 @@ int Device::readDevInfoMultiLineStr(DevInfoTypes type, std::vector<std::string>*
   while (std::getline(fs, line)) {
     retVec->push_back(line);
   }
+  if (fs.bad() || (fs.fail() && !fs.eof())) {
+    ss << "Failed to read devInfoMultiLineStr for DevInfoType (" << get_type_string(type) << ")";
+    LOG_ERROR(ss);
+    return EIO;
+  }
   fs.close();
 
   if (retVec->empty()) {

projects/amdsmi/src/amd_smi/amd_smi_utils.cc

@@ -353,9 +353,15 @@ amdsmi_status_t smi_amdgpu_parse_dpm_ranges(std::istream& dpm_stream,
   unsigned int current_freq = 0;
   char str[10];
   char single_char;
+  bool has_levels = false;
   for (std::string line; getline(dpm_stream, line);) {
     unsigned int dpm_level, freq;
 
+    if (line.find_first_not_of(" \t\r") == std::string::npos) {
+      continue;
+    }
+    has_levels = true;
+
     char firstChar = line[0];
     if (firstChar == 'S') {
       if (sscanf(line.c_str(), "%c: %u%9s", &single_char, &sleep_freq, str) <= 2) {
@@ -388,6 +394,13 @@ amdsmi_status_t smi_amdgpu_parse_dpm_ranges(std::istream& dpm_stream,
       dpm = dpm_level > dpm ? dpm_level : dpm;
     }
   }
+  if (dpm_stream.bad() || (dpm_stream.fail() && !dpm_stream.eof())) {
+    return AMDSMI_STATUS_IO;
+  }
+  // Empty data does not establish whether the clock is supported.
+  if (!has_levels && !od_range.present) {
+    return AMDSMI_STATUS_IO;
+  }
   if (dpm == 0 && current_freq > 0) {
     // if the dpm level is 0, then the current frequency is the min/max frequency
     max = current_freq;

projects/amdsmi/tests/amd_smi_test/unit/gpu/clock_range_test.cc

@@ -10,13 +10,59 @@
 #include <gtest/gtest.h>
 
 #include <climits>
+#include <cstdlib>
+#include <filesystem>
+#include <fstream>
+#include <memory>
 #include <sstream>
+#include <stdexcept>
 
 #include "amd_smi/amdsmi.h"
 #include "amd_smi/impl/amd_smi_clk_testing.h"
+#include "rocm_smi/rocm_smi.h"
+#include "rocm_smi/rocm_smi_device.h"
+#include "rocm_smi/rocm_smi_main.h"
 
 namespace {
 
+class FakeDpmDevice {
+ public:
+  FakeDpmDevice() {
+    auto path = (std::filesystem::temp_directory_path() / "amdsmi_dpm_XXXXXX").string();
+    if (!mkdtemp(path.data())) {
+      throw std::runtime_error("cannot create DPM test directory");
+    }
+    root_ = path;
+    std::filesystem::create_directory(root_ / "device");
+    auto& devices = amd::smi::RocmSMI::getInstance().devices();
+    index_ = static_cast<uint32_t>(devices.size());
+    devices.push_back(std::make_shared<amd::smi::Device>(root_.string(), nullptr));
+  }
+
+  ~FakeDpmDevice() {
+    auto& devices = amd::smi::RocmSMI::getInstance().devices();
+    devices.erase(devices.begin() + index_);
+    std::filesystem::remove_all(root_);
+  }
+
+  FakeDpmDevice(const FakeDpmDevice&) = delete;
+  FakeDpmDevice& operator=(const FakeDpmDevice&) = delete;
+
+  uint32_t index() const { return index_; }
+  std::filesystem::path table() const { return root_ / "device" / "pp_dpm_fclk"; }
+
+  void WriteTable(const char* contents) const {
+    std::ofstream out(table());
+    ASSERT_TRUE(out.is_open());
+    out << contents;
+    ASSERT_TRUE(out.good());
+  }
+
+ private:
+  std::filesystem::path root_;
+  uint32_t index_;
+};
+
 // pp_od_clk_voltage as seen on MI45x: SCLK and MCLK sections plus an OD_RANGE
 // footer, but no OD_FCLK section.
 constexpr char kOdNoFclk[] =
@@ -252,15 +298,98 @@ TEST(GpuUnit, DpmRangesUnavailableSleepSentinelNotRejected) {
   EXPECT_EQ(ranges.sleep_state_freq, -1);
 }
 
-// An empty pp_dpm_* stream with no overdrive range: min keeps its UINT_MAX
-// "unavailable" sentinel and surfaces as -1 rather than failing the call.
-TEST(GpuUnit, DpmRangesEmptyStreamKeepsUnavailableMin) {
-  std::istringstream dpm("");
-  SmiAmdgpuOdClkRange od_range;  // not present -> derive from the (absent) levels
+TEST(GpuUnit, DpmRangesEmptyTablesReportIoError) {
+  for (const char* contents : {"", "\n", " \t\r\n", "\n\n"}) {
+    std::istringstream dpm(contents);
+    SmiAmdgpuOdClkRange od_range;
+    SmiAmdgpuClkRanges ranges{1800, 1300, 2, 400};
+    EXPECT_EQ(smi_amdgpu_parse_dpm_ranges(dpm, od_range, ranges), AMDSMI_STATUS_IO);
+    EXPECT_EQ(ranges.max_freq, 1800);
+    EXPECT_EQ(ranges.min_freq, 1300);
+    EXPECT_EQ(ranges.num_dpm, 2);
+    EXPECT_EQ(ranges.sleep_state_freq, 400);
+  }
+}
+
+// With an overdrive range the domain still has a range to report.
+TEST(GpuUnit, DpmRangesBlankLineKeepsOdRange) {
+  std::istringstream dpm("\n");
+  SmiAmdgpuOdClkRange od_range{true, 1800u, 1300u};
   SmiAmdgpuClkRanges ranges;
   EXPECT_EQ(smi_amdgpu_parse_dpm_ranges(dpm, od_range, ranges), AMDSMI_STATUS_SUCCESS);
-  EXPECT_EQ(ranges.max_freq, 0);
-  EXPECT_EQ(ranges.min_freq, -1);
+  EXPECT_EQ(ranges.max_freq, 1800);
+  EXPECT_EQ(ranges.min_freq, 1300);
+}
+
+TEST(GpuUnit, DpmRangesReadFailureIsNotAnEmptyTable) {
+  for (bool has_od_range : {false, true}) {
+    std::istringstream dpm("0: 1300Mhz\n");
+    dpm.setstate(std::ios::badbit);
+    SmiAmdgpuOdClkRange od_range{has_od_range, 1800u, 1300u};
+    SmiAmdgpuClkRanges ranges;
+    EXPECT_EQ(smi_amdgpu_parse_dpm_ranges(dpm, od_range, ranges), AMDSMI_STATUS_IO);
+  }
+}
+
+TEST(GpuUnit, DpmFrequencyReadFailureIsNotAnEmptyTable) {
+  FakeDpmDevice device;
+  // Offset zero in our own proc mem file fails with EIO without reading memory.
+  std::filesystem::create_symlink("/proc/self/mem", device.table());
+  rsmi_frequencies_t frequencies{};
+  EXPECT_EQ(rsmi_dev_gpu_clk_freq_get(device.index(), RSMI_CLK_TYPE_DF, &frequencies),
+            RSMI_STATUS_UNEXPECTED_SIZE);
+}
+
+TEST(GpuUnit, DpmFrequencyMissingTableNotSupported) {
+  FakeDpmDevice device;
+  rsmi_frequencies_t frequencies{};
+  EXPECT_EQ(rsmi_dev_gpu_clk_freq_get(device.index(), RSMI_CLK_TYPE_DF, &frequencies),
+            RSMI_STATUS_NOT_SUPPORTED);
+}
+
+TEST(GpuUnit, DpmFrequencyEmptyTablesPreserveDataError) {
+  FakeDpmDevice device;
+  for (const char* contents : {"", "\n", " \t\r\n", "\n\n"}) {
+    device.WriteTable("0: 1300Mhz *\n");
+    rsmi_frequencies_t frequencies{};
+    ASSERT_EQ(rsmi_dev_gpu_clk_freq_get(device.index(), RSMI_CLK_TYPE_DF, &frequencies),
+              RSMI_STATUS_SUCCESS);
+    ASSERT_EQ(frequencies.num_supported, 1u);
+
+    device.WriteTable(contents);
+    EXPECT_EQ(rsmi_dev_gpu_clk_freq_get(device.index(), RSMI_CLK_TYPE_DF, &frequencies),
+              RSMI_STATUS_UNEXPECTED_DATA);
+    EXPECT_EQ(frequencies.num_supported, 0u);
+  }
+}
+
+TEST(GpuUnit, DpmFrequencyMalformedTableStillFails) {
+  FakeDpmDevice device;
+  device.WriteTable("0: 1300invalid\n");
+  rsmi_frequencies_t frequencies{};
+  EXPECT_EQ(rsmi_dev_gpu_clk_freq_get(device.index(), RSMI_CLK_TYPE_DF, &frequencies),
+            RSMI_STATUS_UNEXPECTED_DATA);
+}
+
+TEST(GpuUnit, DpmFrequencyValidTableStillReportsLevels) {
+  FakeDpmDevice device;
+  device.WriteTable("0: 1300Mhz\n1: 1550Mhz *\n2: 1800Mhz\n");
+  rsmi_frequencies_t frequencies{};
+  ASSERT_EQ(rsmi_dev_gpu_clk_freq_get(device.index(), RSMI_CLK_TYPE_DF, &frequencies),
+            RSMI_STATUS_SUCCESS);
+  EXPECT_EQ(frequencies.num_supported, 3u);
+  EXPECT_EQ(frequencies.current, 1u);
+  EXPECT_EQ(frequencies.frequency[0], 1300000000u);
+  EXPECT_EQ(frequencies.frequency[2], 1800000000u);
+}
+
+TEST(GpuUnit, DpmRangesWhitespaceDoesNotHideMalformedData) {
+  for (const char* contents : {"\ninvalid\n", " \t\r\ninvalid\n"}) {
+    std::istringstream dpm(contents);
+    SmiAmdgpuOdClkRange od_range;
+    SmiAmdgpuClkRanges ranges;
+    EXPECT_EQ(smi_amdgpu_parse_dpm_ranges(dpm, od_range, ranges), AMDSMI_STATUS_IO);
+  }
 }
 
 // A genuinely out-of-range value (> INT_MAX, not the sentinel) is still

projects/amdsmi/tests/python/README.md

@@ -82,6 +82,23 @@ graph TD
 All three share the same option set and the same GTest-style summary, because
 they all delegate to `common.run_test_dir()`.
 
+The `set` sweep runs each setting with a numeric `--gpu` selector and derives
+clock and power values from that GPU. Each device's exit code is checked
+independently; aggregate errors are not added to the allowed codes.
+Empty DPM data remains an error and is not added to the sweep's allowed statuses.
+
+The sweep explicitly excludes memory-partition changes. A request can remain
+queued until driver reload, and requesting the currently active mode does not
+cancel it. Cleanup therefore does not claim to restore the original memory
+partition. Test NPS transitions only in a separate, controlled driver-reload
+workflow; the generic sweep neither reloads the driver nor clears changes
+queued before the test.
+
+Clock cleanup uses `reset --clocks` rather than replaying captured limits that
+can conflict with the sweep's current bounds. It resets clocks to defaults,
+then reapplies each GPU's saved performance level; custom clock limits are
+not preserved.
+
 ### Invocation
 
 From an installed package (recommended — avoids a shadowing `site-packages` copy):

projects/amdsmi/tests/python/cli/base.py

@@ -121,7 +121,10 @@ class TestCliBase(unittest.TestCase):
     # TODO(amdsmi_team): drop these once the CLI tests supports automated
     #                     input
     SWEEP_EXCLUDED_ARGS = {
-        "set": {"--compute-partition": "waits for confirmation"},
+        "set": {
+            "--compute-partition": "waits for confirmation",
+            "--memory-partition": "cannot cancel a queued change by restoring the active mode",
+        },
         "reset": {
             # --gtt reaches reset.py's prompt_reboot(), which calls `systemctl reboot` on "y"
             "--gtt": "prompts to reboot the host",
@@ -355,8 +358,6 @@ def __init__(self, *args, **kwargs):
         self.memory_partition_modes = ["NPS1", "NPS2", "NPS4", "NPS8"]
         self.power_types = ["ppt0", "ppt1"]
         self.ptl_formats = ["I8", "F16", "BF16", "F32", "F64", "F8", "VECTOR"]
-        self.clk_limits = ["SCLK", "MCLK", "FCLK"]
-        self.limit_types = ["MAX", "MIN"]
         self.clk_levels = ["SCLK", "MCLK", "FCLK", "SOCCLK", "PCIE"]
 
         # When parsing, ignore these entries as they are abnormal
@@ -570,7 +571,7 @@ def FindArgs(self, cmd, match_str):
     def CreateCmds(self, cmd_name, list1_name, list2_name, list3_name, list4_name):
         cmd = f"amd-smi {cmd_name} --help"
         list1_args = self._order_sweep_args(cmd_name, self.FindArgs(cmd, list1_name))
-        list2_args = self.FindArgs(cmd, list2_name)
+        list2_args = ["pass"] if cmd_name == "set" else self.FindArgs(cmd, list2_name)
         list3_args = self.FindArgs(cmd, list3_name)
         list4_args = self.FindArgs(cmd, list4_name)
         if self.Debug:
@@ -624,6 +625,14 @@ def CreateCmds(self, cmd_name, list1_name, list2_name, list3_name, list4_name):
                                 )
                             )
 
+        if cmd_name == "set":
+            # Select the device before resolving its clock and power limits.
+            cmds = [
+                (f"{command} --gpu {gpu_index}", condition)
+                for command, condition in cmds
+                for gpu_index in range(len(self.common.processors))
+            ]
+
         # Calculate and substitute in dependent values
         # Removes cmds that are invalid
         for index, cmd_cond in enumerate(cmds):
@@ -843,7 +852,7 @@ def CreateCmds(self, cmd_name, list1_name, list2_name, list3_name, list4_name):
                     sub_arg = items[2]
                     if sub_arg != found_sub_arg:
                         # Whole word matches only: i.e. remove "--gpu", not "--gpureset"
-                        if "--gpu" in items:
+                        if "--gpu" in items and cmd_name != "set":
                             cmd = ""
 
                 # Remove all file and watch modifiers except for gpu 0