Skip to content

fix-5-amdgpu-load: needs_reboot disagrees between fix.rs catalog and diagnose.rs #418

Description

@jussielo-amd

Summary

fix-5-amdgpu-load's needs_reboot value disagrees between the two places that define it, the same defect class fixed for fix-9-igpu-dgpu's auto_applicable in #413.

  • crates/rocm-core/src/fix.rs (FixRecipe catalog, line 179): needs_reboot: true — unconditional.
  • crates/rocm-core/src/diagnose.rs (check_5_amdgpu_not_loaded, line 730): needs_reboot: !blacklisted.is_empty() — only true when a modprobe blacklist entry was found; false otherwise (e.g. amdgpu simply isn't loaded yet, no blacklist involved).

Both Fix values are independently constructed for the same fix-5-amdgpu-load id, so rocm diagnose and rocm fix fix-5-amdgpu-load can report different needs_reboot for the same underlying condition, and rocm diagnose --json/its rendered flags: line reflects whichever branch diagnose.rs took.

Which is correct?

Unclear without more investigation — possibly the diagnose.rs conditional is actually more accurate (a reboot may genuinely not be needed if there was no blacklist to clear, since sudo modprobe amdgpu alone doesn't require one), in which case fix.rs's unconditional true is the one that should be relaxed. This needs a decision, not just picking one side to match the other.

Suggested next step

  • Decide which value is semantically correct for each case (blacklisted vs. not).
  • Either make fix.rs's FixRecipe.needs_reboot conditional to match, or simplify diagnose.rs's construction to the unconditional true, so both surfaces agree.
  • Add a regression test analogous to fix_9_igpu_dgpu_is_auto_applicable_on_linux in diagnose.rs (added in fix(diagnose): unify remediation flag wording with rocm fix #413) pinning the expected value for both the blacklisted and non-blacklisted cases, to prevent future drift.

Context

Found during the follow-up review of #413, which fixed the same drift class for fix-9-igpu-dgpu's auto_applicable field. Left out of that PR's scope since it's a different fix-id and needs its own correctness decision rather than a straightforward alignment.

Activity

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

Metadata

Metadata

Assignees

No one assigned

    Labels

    No labels
    No labels

    Type

    No type

    Projects

    No projects

      Milestone

      No milestone

      Relationships

      None yet

      Development

      No branches or pull requests

      Issue actions