Skip to content

fix(amdsmi): correct tray output, CPER list input and DRM example crash - #11143

Merged
marifamd merged 13 commits into
developfrom
users/marifamd/amdsmi-verification-fixes
Oct 14, 2026
Merged

marifamd merged 13 commits into
developfrom
users/marifamd/amdsmi-verification-fixes

Conversation

@marifamd

@marifamd marifamd commented Sep 2, 2026 •

Copy link
Copy Markdown
Contributor

Motivation

Verifying develop across MI300A, MI350X, MI210, Strix Halo, Navi 21 + Navi 32 and a 75c1 part turned up these defects:

  • amd-smi node --tray printed an empty section on hardware without UALoE.
  • amdsmi_get_afids_from_cper() raised TypeError for list input.
  • amd_smi_drm_ex segfaulted when run without root, and CI discarded the example exit codes, so nothing caught it.
  • The CLI test generator ran value-taking flags with no value, so those commands failed by construction.

Technical Details

CLI (amdsmi_cli/subcommands/node.py):

  • Drop the non-empty tray_dict guard. Human, --json and --csv now report MAX_ACC_PER_TRAY and TRAY_TYPE as N/A when tray info is unavailable, instead of a bare NODE: and {"node": {}}

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

  • amdsmi_get_afids_from_cper(): check list input with isinstance(..., list). isinstance() with List[Dict[str, Any]] always raises TypeError, so the documented list-of-dicts input never worked and other bad input raised TypeError instead of AmdSmiParameterException. bytes input (the CLI path) was unaffected

Example (example/amd_smi_drm_example.cc):

  • Run the partition profile loop only when amdsmi_get_gpu_accelerator_partition_profile_config() succeeds. Without root it returns AMDSMI_STATUS_NO_PERM before filling its output, so the loop read an uninitialized num_profiles. fix(amdsmi): drm_examples SIGSEGV due to partition var init #11922 zero-initialized the struct on develop; this adds the status check
  • Zero-initialize the remaining output buffers: socket_info, profile, partition_id, vmode, temp_measurements

CLI test generator (tests/python/cli/base.py):

  • Detect value-taking flags from the argparse help layout (one space before a metavar) instead of isupper()
  • Skip a value-taking flag that has no sweep values and report it as Untested

CI (.github/workflows/amdsmi-build.yml):

  • Fail the example step when an example dies from a signal (exit 129-192). Other non-zero exits are the first failing amdsmi_status_t returned from main() and are only logged, since a call can be unsupported on a given ASIC. Before, || echo discarded every exit code

Changelog (CHANGELOG.md):

  • Three 10.2.0 Resolved Issues entries

The --json error-prefix fix from the first revision is dropped: #8624 already removed the exception class name from every output format.

Issue Tracking

N/A, found during manual verification of develop.

Test Plan

Original branch, on the machines above (source build installed to a staging prefix):

  • amd-smi node --tray in human, --json and --csv
  • amd_smi_drm_ex as root and non-root
  • amdsmitst read-only suites, unit_tests.py, test_abi_compat.py
  • cli.test_list, cli.test_node, cli.test_version, cli.test_static

After merging develop:

  • pre-commit on every changed file
  • g++ -fsyntax-only -Wall -Wextra on the DRM example
  • amdsmi_get_afids_from_cper() with bytes, a list of records, and an int, against the installed 27.1.0 library and the mock_cper fixtures
  • FindArgs against stubbed --help output

Test Result

Check Build Before After
amd_smi_drm_ex, non-root (Navi 21 + Navi 32) original Segmentation fault, rc=139 rc=17 (INPUT_OUT_OF_BOUNDS from the clock query, no crash)
amd-smi node --tray original bare NODE: TRAY: with MAX_ACC_PER_TRAY: N/A and TRAY_TYPE: N/A
cli.test_list original failed passed
amdsmitst read-only original 95/95 passed
unit_tests.py original 85 passed, 6 skipped
test_abi_compat.py original 12 passed
cli.test_node, cli.test_version original passed
amdsmi_get_afids_from_cper([record]) merged TypeError same result as the bytes call
amdsmi_get_afids_from_cper(5) merged TypeError AmdSmiParameterException
FindArgs stub merged --nic run with no value skipped and reported as Untested; reset --profile still expands
pre-commit, g++ -fsyntax-only merged clean

cli.test_static failed test_tray and test_asic_revision_id_keys on the original host because tests/python/common/runcmd.py puts /opt/rocm/bin first on PATH, so the CLI tests ran the older installed amd-smi, which has no --tray.

Submission Checklist

@marifamd
marifamd requested a review from a team September 2, 2026 21:11
@marifamd
marifamd requested a review from a team as a code owner September 2, 2026 21:11
@github-actions github-actions Bot added project: amdsmi github actions Pull requests that update GitHub Actions code labels Sep 2, 2026
@therock-pr-bot

therock-pr-bot Bot commented Sep 2, 2026 •

Copy link
Copy Markdown

✅ All Policy Checks Passed

Check Status Details
📝 PR Description ✅ Pass —
⛔ Forbidden Files ✅ Pass —
🧪 Unit Test ⚠️ Warning Error: Source/code files changed without an accompanying unit test.
Expected: add at least one test file named like test_<name>.py / test_<name>.cpp (or <name>_test.*).
Current: code file(s) changed: projects/amdsmi/amdsmi_cli/subcommands/node.py, projects/amdsmi/example/amd_smi_drm_example.cc, projects/amdsmi/py-interface/amdsmi_interface.py; no test file found
🚫 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?

@therock-pr-bot

therock-pr-bot Bot commented Sep 2, 2026

Copy link
Copy Markdown

🚫 Please fix the failed policies before requesting reviews.

The following policy checks failed:

  • ❌ PR Description

The Not ready to Review label has been added to this PR.
Once all policies pass, the label will be removed automatically.

Comment thread .github/workflows/amdsmi-build.yml Resolved

@oliveiradan oliveiradan left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comments were addressed. Approved.

Comment thread .github/workflows/amdsmi-build.yml Resolved
Comment thread .github/workflows/amdsmi-build.yml Resolved
Comment thread .github/workflows/amdsmi-build.yml Resolved
@marifamd marifamd changed the title fix(amdsmi): correct tray output, CPER validation, JSON errors and example crash fix(amdsmi): correct tray output, CPER list input and DRM example crash Oct 9, 2026
marifamd and others added 6 commits October 13, 2026 12:32
…ample crash

Six defects found while verifying develop across MI300A, MI350X, MI210,
Strix Halo, Navi and a 75c1 part.

amd-smi node --tray guarded its output on the retrieved dictionary being
non-empty, so the N/A defaults sitting next to it were unreachable and the
section rendered as a bare NODE: header. All three formats now report N/A.

amdsmi_get_afids_from_cper validated its input with isinstance() against a
subscripted generic, which Python rejects for any value, so the function
raised TypeError for every caller.

The CLI printed the Python exception class name ahead of the JSON error
document, so parsing amd-smi --json output failed on every error path.

The DRM example read the accelerator partition configuration into an
uninitialized structure without checking the return value. An unsupported
query left num_profiles indeterminate and the loop ran past the end of the
profiles array, segfaulting for unprivileged callers on every GPU.

The CLI test generator emitted flags that require a value with no value
attached, so any host exposing such a flag failed by construction.

The build workflow discarded example exit codes, which is why the segfault
above went unnoticed. Fatal signals now fail the step; ordinary non-zero
codes are reported but tolerated, since a query can legitimately be
unsupported on a given ASIC.

Signed-off-by: Maisam Arif <Maisam.Arif@amd.com>
…rtiton function and amdsmi-build workflow exit codes
10.1 has branched, so these fixes ship in 10.2.0. Drop the --json error
entry: develop's #8624 already removed the class-name prefix and
documents it.

Signed-off-by: Maisam Arif <Maisam.Arif@amd.com>
State why only a crash fails the step while an error status is logged.

Signed-off-by: Maisam Arif <Maisam.Arif@amd.com>
- Narrow the CPER entry to list input; bytes callers never hit the
  TypeError
- Describe the example crash as the non-root NO_PERM path; the API
  zeroes its output before an unsupported query fails

Signed-off-by: Maisam Arif <Maisam.Arif@amd.com>
@marifamd
marifamd force-pushed the users/marifamd/amdsmi-verification-fixes branch from e145f03 to 978f407 Compare October 13, 2026 17:37
…smi-verification-fixes

Conflicts:
- CHANGELOG.md: keep develop's driver-date entry (#13358), then this
  PR's three 10.2.0 Resolved Issues entries.

Signed-off-by: Maisam Arif <Maisam.Arif@amd.com>
- Restore amdsmi-build.yml and tests/python/cli/base.py to develop
- #12997 changes the same example step and FindArgs, with tests

Signed-off-by: Maisam Arif <Maisam.Arif@amd.com>
- The CLI guide still said the TRAY block and keys are omitted

Signed-off-by: Maisam Arif <Maisam.Arif@amd.com>
- Quote the real --json shape, a list
- Drop the zero-init clause from the DRM entry; #11922 made that change

Signed-off-by: Maisam Arif <Maisam.Arif@amd.com>
- temp_measurements entries are printed even when unsupported

Signed-off-by: Maisam Arif <Maisam.Arif@amd.com>
- Annotate Union[bytes, List[Dict[str, Any]]], valid on Python 3.6
- Describe both input forms in the docstring and the Python API reference
- Report both accepted types in AmdSmiParameterException

Signed-off-by: Maisam Arif <Maisam.Arif@amd.com>
@github-actions github-actions Bot added the documentation Improvements or additions to documentation label Oct 13, 2026
@adam360x

Copy link
Copy Markdown
Contributor

@copilot rebase and fix conflict

…smi-verification-fixes

Co-authored-by: adam360x <61172547+adam360x@users.noreply.github.com>

Copilot AI commented Oct 13, 2026

Copy link
Copy Markdown
Contributor

@copilot rebase and fix conflict

Integrated develop with a clean merge (no conflicts) in fdf54386. I merged rather than rebased because this environment cannot publish rewritten history.

Copilot AI requested a review from adam360x October 13, 2026 22:41
@marifamd
marifamd merged commit b7268e9 into develop Oct 14, 2026
128 of 129 checks passed
@marifamd
marifamd deleted the users/marifamd/amdsmi-verification-fixes branch October 14, 2026 05:19
@systems-assistant

Copy link
Copy Markdown
Contributor

TheRock Submodule Bump Activity

Newest first

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 github actions Pull requests that update GitHub Actions code Not ready to Review organization: ROCm project: amdsmi

Projects

None yet

Development

Successfully merging this pull request may close these issues.

7 participants