Skip to content

fix(amdsmi): report NPM power limit failures correctly - #12723

Draft
marifamd wants to merge 1 commit into
developfrom
users/marifamd/npm-power-limit-followup
Draft

marifamd wants to merge 1 commit into
developfrom
users/marifamd/npm-power-limit-followup

Conversation

@marifamd

@marifamd marifamd commented Oct 2, 2026 •

Copy link
Copy Markdown
Contributor

Motivation

Follow-up to #11471. It fixes the failure reporting, test-hook, and doc issues found in review after that PR merged.

Technical Details

Library (rocm_smi/src/rocm_smi_npm.cc, rocm_smi/src/rocm_smi.cc, src/amd_smi/amd_smi.cc):

  • A write the driver rejects with EPERM (non-permitted guest) now returns AMDSMI_STATUS_NO_PERM. Before, WriteSysfsStr() reported it as NOT_SUPPORTED
  • amdsmi_set_npm_limit() treats a UINT64_MAX platform max as unavailable (AMDSMI_STATUS_UNEXPECTED_DATA) instead of an unlimited bound
  • Restored the previous number parsing for the existing NPM readers (a leading space parses again)
  • Moved the setter's status and range checks into validate_npm_board_limit() so unit tests cover them without root
  • Node handles are now validated against the handle registry itself. The BUILD_TESTS-only pointer hook and its CMake defines are replaced by amdsmi_test_get_node_handle(), which works like amdsmi_test_sleep()
  • Added static_assert(sizeof(amdsmi_npm_info_t) == 64)

Header / docs (include/amd_smi/amdsmi.h, rocm_smi/include/rocm_smi/rocm_smi.h):

  • Corrected the amdsmi_get_npm_info() / amdsmi_set_npm_limit() return codes; shortened the doxygen
  • Fixed the rsmi_npm_info_t.reserved comment (it was reserved[5], and two fields were added)

CLI (set_value.py, amdsmi_helpers.py, amdsmi_parser.py, node.py):

  • A failed amd-smi set -n now exits non-zero (records NOT_SUPPORTED and library errors)
  • -n with -g raises AmdSmiInvalidParameterException instead of sys.exit(2)
  • -n help text is static. The parser no longer probes hardware to print the max
  • The node CSV column is emitted only for amd-smi node -p

Python API (py-interface/amdsmi_interface.py):

  • amdsmi_set_npm_limit() rejects bool limits; fixed the node_handle type hints and docstrings

Tests:

  • New root-free validate_npm_board_limit() tests; temp board directories use mkdtemp
  • Moved the amdsmi_get_power_info() null check into the functional power read test
  • Removed from __future__ import annotations (Python 3.6) and stale comments from the NPM tests

Docs: CHANGELOG.md, docs/reference/amdsmi-py-api.md, docs/how-to/amdsmi-cli-tool.md

Not changed here (need the spec owner):

  • Guest (1VF) platform tags for the NPM APIs
  • A minimum node power limit (the driver only exposes a max)

Issue Tracking

JIRA ID: ROCM-31980

Test Plan

  • Built with -DBUILD_TESTS=ON (Ubuntu 22.04, non-NPM GPU, non-root)
  • amdsmitst --gtest_filter='GpuUnit*:*TestPowerRead*'
  • python3 -m unittest discover -s unit from tests/python, run against the build's Python package
  • clang-format, ruff format / ruff check, gersemi, check_test_conventions.py, license headers

Test Result

  • GTest: 152 run, 141 passed, 11 skipped (root-only SetNpmLimitRoot*, NPM hardware, read-write power test), 0 failed
  • Python unit: 220 passed, including test_runner_hygiene
  • Style checks clean. Root-only setter tests and MI450 hardware were not run

Submission Checklist

@github-actions github-actions Bot added documentation Improvements or additions to documentation project: amdsmi labels Oct 2, 2026
@therock-pr-bot

therock-pr-bot Bot commented Oct 2, 2026

Copy link
Copy Markdown

✅ All Policy Checks Passed

Check Status Details
📝 PR Description ✅ Pass —
⛔ Forbidden Files ✅ Pass —
🧪 Unit Test ✅ Pass —
🚫 Draft PR 🔜 To Be Enabled —
🚩 Feature Flag 🔜 To Be Enabled —
📊 Code Coverage 🔜 To Be Enabled —

🎉 All policy checks passed!

📖 Need help? See the Policy FAQ for details on every check and how to fix failures.

🙋 Wish to Override Policy?

Library (rocm_smi, amd_smi.cc, amdsmi.h):
- Report a driver-rejected write (EPERM) as AMDSMI_STATUS_NO_PERM
  instead of NOT_SUPPORTED; WriteSysfsStr() dropped the errno
- Treat a UINT64_MAX platform max as unavailable in the setter and
  restore the previous number parsing for existing NPM readers
- Move the setter checks into validate_npm_board_limit() so they run
  in unit tests without root
- Validate node handles against the handle registry itself and
  replace the BUILD_TESTS-only pointer hook with an unguarded
  amdsmi_test_get_node_handle(), like amdsmi_test_sleep()
- Pin sizeof(amdsmi_npm_info_t) to 64 and correct the doxygen return
  codes for amdsmi_get_npm_info() and amdsmi_set_npm_limit()

CLI (set_value.py, amdsmi_helpers.py, amdsmi_parser.py, node.py):
- Record NOT_SUPPORTED and library errors so a failed set -n exits
  non-zero
- Raise AmdSmiInvalidParameterException for -n with -g instead of
  calling sys.exit(2)
- Use static -n help text and drop the hardware probe at parser build
- Emit the CSV node column only for node -p

Python API (amdsmi_interface.py):
- Reject bool limits and fix the node_handle type hints and docstrings

Tests and docs:
- Add root-free validate_npm_board_limit() tests and use mkdtemp for
  temp board directories
- Move the amdsmi_get_power_info() null check to the power read test
- Remove __future__ imports (Python 3.6) and stale test comments
- Update CHANGELOG, Python API reference, and CLI docs

Signed-off-by: Maisam Arif <Maisam.Arif@amd.com>
@marifamd
marifamd force-pushed the users/marifamd/npm-power-limit-followup branch from a2455d2 to 4b873b7 Compare October 13, 2026 17:33

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

documentation Improvements or additions to documentation organization: ROCm project: amdsmi

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants