[Woody-Hu]

What this changes

rocm examine on a Strix Halo APU (gfx1151) reports the machine as a discrete AMD GPU:

"gpus": [{ "name": "Advanced Micro Devices, Inc. [AMD/ATI] Device", "gfx_target": "gfx1151", "is_apu": false }],
"has_apu": false,
"has_discrete_amd": true

Older packaged copies of pci.ids predate 1002:1586, so lspci prints the bare word "Device". classify_amd_marketing_name cannot resolve a target from that name and stamps is_apu: false — the failed lookup, not a verdict about the part. apply_kernel_gpu_membership later fills gfx_target from KFD (only when the field is empty) but never revisits is_apu.

The fix

Four commits, in order:

1. fix(examine): classify from the kernel target. When KFD supplies the target, reclassify with gfx_is_apu_family — the same function the rocminfo path already uses to answer the family question. Kernel-only nodes (no lspci match) are classified from their target instead of being left None. A discrete target (gfx1100, gfx942, …) still classifies as discrete.
2. fix(host_gpu): the 1586 => gfx1151 row requested in examine: lspci cannot name a GPU that pci.ids does not know, though the device id is on the line #507, for hosts that have neither KFD nor rocminfo.
3. fix(examine): review follow-up. The examine-29 When step was "inspects the system", which runs the CLI against the runner itself — world.command_env (the stub lspci/rocminfo and ROCM_CLI_TEST_HOST_ROOT) is applied only by the planted-machine step. Every lane therefore inspected the host instead of the fixture: green on a real Strix Halo whether or not the fix is present, and gpus: [] on a lane without a GPU. It now uses the planted-machine step, which also skips framework probing. The same commit adds:
   • an empty gfx_target is unknown, not discrete. KFD leaves it empty when gfx_target_version does not decode and gfx_is_apu_family("") answers false, so both membership branches now keep None instead of stamping "discrete" from a failed decode — the same mistake this PR fixes for the PCI name, one layer down;
   • unit tests for what the existing ones never reached: the is_apu != Some(true) guard, and an undecoded target staying unknown;
   • a second scenario for the kernel-only branch, which the PCI-matching one cannot reach;
   • the 1586 comments no longer claim pci.ids has no name for the id (upstream carries it; older copies do not), and the row joins pnp_device_id_table_agrees_about_apus.
4. docs(examine): comments only. Which path reads the 1586 row today (Windows/PNP), and that the non-downgrade test pins the guard rather than the fix.

Scenarios

Two new Linux scenarios, both simulated hosts so every Linux lane can drive them:

• @id:examine-classifies-an-apu-the-pci-name-cannot (examine-30): unrecognised lspci name, a KFD gfx1151 node, empty rocminfo.
• @id:examine-classifies-a-kernel-only-apu (examine-31): stub lspci reports an empty bus, so only the kernel topology sees the GPU.

The empty rocminfo stub is deliberate in both: on a runner with ROCm installed the real probe resolves the APU on its own (it already classifies by target) and would mask the defect.

They were examine-29/examine-30 before this branch was rebased onto 79f6e08c; main took examine-29 for the Windows driver-version scenario in the meantime, so both were renumbered.

Verification

Ryzen AI MAX 395 (gfx1151, 1002:1586), same machine, same probe inputs, nightly 1c33248c vs the patched binary:

field	nightly	patched
gpus[0].gfx_target	gfx1151	gfx1151
gpus[0].is_apu	false	true
has_apu	false	true
has_discrete_amd	true	false

Rebased tree (head 6e982fcd, on 79f6e08c), macOS dev machine:

• cargo test -p rocm-core --lib: 794 passed, 3 failed, 3 ignored. The three failures are pre-existing and macOS-only (proc_lifecycle's two signal tests and fix::tests::print_only_fix_returns_zero); they fail identically at the base commit.
• The five tests this PR adds or changes pass: a_kernel_target_classifies_an_unnamed_apu, a_kernel_only_apu_node_is_classified_from_its_target, a_resolved_apu_verdict_is_not_downgraded_by_a_kernel_target, an_unparseable_kernel_target_leaves_the_family_unknown, plus a_kernel_visible_gpu_the_pci_scan_missed_is_still_listed.
• cargo fmt --all -- --check clean; cargo clippy -p rocm-core --all-targets -- -D warnings clean on 1.96.

Not run here: the cucumber e2e scenarios. They are Linux-only and this machine is macOS, so examine-30/examine-31 need the CI Linux lane — which is still awaiting workflow approval on this fork PR, so no Rust, e2e or clippy check has executed against the head yet (only the docs build).

Scope and follow-ups

• Narrow fix: is_apu classification only. GfxPackaging is untouched.
• Fixes the classification symptom in examine: Strix Halo APU (gfx1151, Ryzen AI Max+ 395) misclassified as discrete GPU (has_apu=false) #449. Resolving a target from the marketing name, and reporting UMA/carveout, stay out of scope.
• The 1586 row only affects the Windows/PNP path for now; on Linux, reading device ids from lspci is the other half of examine: lspci cannot name a GPU that pci.ids does not know, though the device id is on the line #507 and is not part of this PR.
• Follow-up, not fixed here: probe_gpus_sysfs_fallback still leaves is_apu: None and its comment still argues that claiming either verdict would invent a fact. It is the last pass in the sequence (KFD → rocminfo → sysfs), so it only runs when neither KFD nor lspci is available, and it faces the same target-only evidence as the kernel-only branch. Changing it belongs with the classification rework rather than this fix; the comment there is now the only place the old reasoning stands.
• fix(examine): identify AMD GPUs by device id and one name table, and never downgrade an APU verdict (ROCMAI-534) #521 reworks this same classification around one device-id table and a gfx_apu_verdict. It does not touch apply_kernel_gpu_membership and does not add a 1586 row, so on a host with no rocminfo and an old pci.ids it does not fix examine: Strix Halo APU (gfx1151, Ryzen AI Max+ 395) misclassified as discrete GPU (has_apu=false) #449 on its own. The two are complementary; whichever lands second needs the other's row or reclassification removed.

Checklist

• Searched tests/e2e-cucumber/expectations.toml for examine: Strix Halo APU (gfx1151, Ryzen AI Max+ 395) misclassified as discrete GPU (has_apu=false) #449 and examine: lspci cannot name a GPU that pci.ids does not know, though the device id is on the line #507 — there are no rows to narrow.
• No new subcommand or subsystem, so the module-organization rule does not apply; the change stays in the existing examine.rs/host_gpu.rs modules.
• The changed user-facing state (has_apu, is_apu, has_discrete_amd) is asserted as the resulting examine --json document in examine-30 and examine-31, not as message wording.

AI assistance

This change was developed with an AI coding agent; every measurement, unit test and e2e run quoted above was executed on the hardware/environment described.

[tomastola]

Thanks for this, @Woody-Hu, and for the clear write-up with before/after numbers from real hardware. The diagnosis is right: the failed name lookup was being treated as a verdict about the part, and classifying from the kernel target is the correct fix. A few things need attention before it can land. The first one is the important one, and it's a one-line change.

Review: needs work

This is a review of the whole change at 0b542d58 against main at 1c33248c.
It fixes the is_apu classification symptom from #449 and adds the 1586 row that #507 asks for. The description says it doesn't try to close the rest of either issue.

Blocking

• examine-29 never uses the host it plants, so it fails on lanes without a real APU and passes on a real one whether or not the fix is present. tests/e2e-cucumber/features/examine.feature:447, tests/e2e-cucumber/tests/e2e/examine_steps.rs:523, :1510-1541

  • The Given step puts the fake lspci/rocminfo PATH and ROCM_CLI_TEST_HOST_ROOT into world.command_env.
  • The When step it uses, "the user inspects the system in machine-readable form", calls crate::run_rocm, and that function never applies command_env. Only run_rocm_with_scenario_env does, and that is used by "the user inspects that machine in machine-readable form" (examine_steps.rs:1543).
  • What I saw:
    • At this head on a Linux host with no GPU, the scenario failed with "lspci not found; cannot enumerate PCI GPUs", "gpus": [] and has_apu: false. The planted env was never used.
    • With the step text switched to "inspects that machine", it passes at head and fails with the examine.rs change reverted. So the fixture itself is good; only the step wiring is off.
    • On a real Strix Halo with ROCm installed, the host's own lspci, KFD and rocminfo answer instead, so the scenario passes whether or not the fix is present.
  • Suggested fix: use the "inspects that machine in machine-readable form" step. It also skips framework probing, which this scenario doesn't need. The feature comment ("every Linux lane can drive it") and the description's "3/3 steps pass" will then be accurate.

• Nothing tests the new if gpu.is_apu != Some(true) guard. crates/rocm-core/src/examine.rs:1725

  • I replaced the guard with if true. All 756 rocm-core lib tests still passed, and so did examine-29 with its When step corrected.
  • Every test that reaches the empty-target branch starts from is_apu: Some(false).
  • The guard only changes the result in one case: the name matched an APU keyword but resolved no target, and KFD then reports a target that gfx_is_apu_family doesn't list. The PCI scan never leaves an AMD entry at None, so in practice the guard means == Some(false).
  • Suggested fix: add a unit test that starts from Some(true), an empty target and a KFD target outside the family list, and asserts the verdict stays Some(true). Or, if "never downgrade" isn't intended here, drop the guard.

• The kernel-only path has user-visible behaviour but no scenario. crates/rocm-core/src/examine.rs:1707

  • AGENTS.md asks for a scenario as well as a unit test when user-observable behaviour changes: "User-observable behavior needs a scenario, not only a unit test."
  • On a host without lspci, this change sets has_apu and has_discrete_amd through the kernel-only branch. Only a_kernel_only_apu_node_is_classified_from_its_target covers that.
  • Even with the When step corrected, examine-29 always has a PCI match, so it never reaches this branch. When I broke the branch, the scenario stayed green.
  • Suggested fix: add a variant of examine-29 whose fake lspci prints nothing, as plant_gpu_fixtures already does. Alternatively, state the gap in the PR text, which AGENTS.md allows.

Non-blocking

• The sysfs fallback still leaves is_apu unset, and its comments now contradict the reasoning here. examine.rs:1477, :1500-1508, :5627-5632

  • The comment this PR removes tied the two paths together ("for the same reason probe_gpus_sysfs_fallback leaves it unset").
  • probe_gpus_sysfs_fallback still says "Claiming either would be inventing a fact". The test comment at :5629 still says "the topology says which target, not whether it is integrated". As a result, a gfx1151 found only through DRM ip-discovery still reports has_apu: false.
  • Your reasoning should be the one that stands, because the rocminfo path (:1447) and the hipInfo path (:3189) already classify from the target. Either classify in the sysfs fallback too and update both comments, or note it as a follow-up.

• A kernel-only node with an empty or unlisted target now reads as discrete. examine.rs:1707

  • KfdGpuNode.gfx_target is empty when the version doesn't decode (host_gpu.rs:1003), and gfx_is_apu_family("") is false. That node used to be None and is now Some(false), so has_discrete_amd: true gets set with no evidence behind it.
  • APU targets missing from the list (Raven, gfx902) are affected the same way.
  • The rocminfo path already behaves like this. Still, keeping None when gfx_target is empty would be a cheap guard.

• The comments say pci.ids has no name for 1002:1586, but current upstream pci.ids does. host_gpu.rs:717-718, examine.rs:5101, examine.feature:432, and the commit message of 0b542d5

  • Upstream now has: 1586 Strix Halo [Radeon Graphics / Radeon 8050S Graphics / Radeon 8060S Graphics]. The bug is still real on hosts with an older packaged copy, so "older distro copies of pci.ids" would be accurate.
  • That upstream entry would also be a good source to cite for the new row, as the gfx90c rows cite theirs.
  • The row's comment justifies it with Linux evidence (KFD, pci.ids), but this table is only read on the Windows paths.
  • 1586 isn't in pnp_device_id_table_agrees_about_apus (examine.rs:5893) yet.

Decisions for the author

• This PR bundles #449 and #507. AGENTS.md asks for one logical change per PR. The 1586 row only matters on Windows and doesn't affect the #449 scenario. Both are about Strix Halo and the description is upfront about the split, so keeping them together is defensible. Splitting them would let the table row land on its own.
• It overlaps with #521. The description says the two don't overlap because GfxPackaging is untouched. However, #521 ("classify integrated AMD GPUs by target, and never downgrade a verdict") reworks the same classification in examine.rs and already includes a ("Device", "1586", "gfx1151", true) case for #449. Whichever lands second will conflict or duplicate. It may be worth agreeing an order with the #521 author.

[nowycondro]

Automated review (recall-biased, high effort): found one likely correctness regression and one altitude/consistency gap. Details inline / below.

[nowycondro]

Unresolved KFD target is stamped as a confident "discrete" verdict instead of "unknown".

node.gfx_target can legitimately be an empty string: kfd_gpu_nodes_in builds it via parse_linux_kfd_gfx_target(version.trim()).unwrap_or_default(), and parse_linux_kfd_gfx_target deliberately returns None (not a guess) for a 5/6-digit gfx_target_version it cannot map through gfx_target_from_gc_version — the comment there even says the intent is to "report the target as unknown" for exactly this case (a future/unrecognized chip).

gfx_is_apu_family("") returns false (see the existing test assert!(!gfx_is_apu_family(""))), so this line turns that "unknown" signal into is_apu: Some(false), i.e. a confident discrete verdict. Before this PR, this same kernel-only branch always set is_apu: None, which summarise_gpu_categories correctly treats as "don't count it either way."

Concretely: a future AMD APU whose gfx_target_version isn't yet in gfx_target_from_gc_version, on a host with no lspci match and no rocminfo (the exact "ROCm not installed yet" scenario rocm examine exists for), now reports has_discrete_amd: true / has_apu: false for an actual APU — the very symptom class this PR fixes for the PCI-name path, reintroduced one layer down for the KFD-target-decode path.

Suggest guarding on node.gfx_target.is_empty() the same way the matched branch guards on a resolved name, e.g. is_apu: (!node.gfx_target.is_empty()).then(|| gfx_is_apu_family(&node.gfx_target)).

[Woody-Hu]

Fixed in f5f435e. gfx_target empty now stays None in both membership branches -- the kernel-only one through (!node.gfx_target.is_empty()).then(...), and the matched one by only reclassifying when the node actually named a target. Added an_unparseable_kernel_target_leaves_the_family_unknown, which asserts is_apu: None and that summarise_gpu_categories leaves has_discrete_amd false.

[nowycondro]

Finding (could not anchor inline — on an unchanged line outside this diff's hunks): crates/rocm-core/src/examine.rs, probe_gpus_sysfs_fallback (around line 1500-1509).

The sysfs-fallback path has the identical bug this PR fixes, and is left unfixed.

probe_gpus_sysfs_fallback is in the exact same situation as apply_kernel_gpu_membership's kernel-only branch this PR changes: it only has a kernel-derived gfx_target (no marketing name, no packaging info), yet the comment there says "is_apu is left unset rather than guessed... Claiming either would be inventing a fact" and sets is_apu: None unconditionally.

That directly contradicts this PR's own stated principle a few hundred lines away ("the target is enough to answer the family question... has_apu only needs the family"). On a host where both lspci and rocminfo are unreachable — which is exactly the case this fallback exists for — a Strix Halo / gfx115x APU detected only through the DRM ip-discovery route still reports has_apu: false, reproducing issue #449's symptom on this adjacent path.

Since this PR is already establishing "gfx_is_apu_family(target) is good enough evidence for the family question," the same one-line fix (is_apu: Some(gfx_is_apu_family(&gfx_target))) should apply here too, rather than leaving this path as a special case that disagrees with the new rule.

[nowycondro]

🤖 Automated review (Claude Code) · 0b542d5

Reviewed by reading the PR diff and the full contents of crates/rocm-core/src/examine.rs and host_gpu.rs at head commit 0b542d58, tracing apply_kernel_gpu_membership, gfx_is_apu_family, parse_linux_kfd_gfx_target, gfx_target_from_gc_version, and the downstream consumers (summarise_gpu_categories, the iGPU/dGPU collision check in diagnose.rs) against existing tests. No Rust toolchain was available in this sandbox, so nothing was built or executed — findings below are from static reading of the diff plus surrounding code, not verified compilation/test runs. I also glanced at tomastola's existing CHANGES_REQUESTED review for context but formed these independently; they overlap with two of that review's non-blocking points but are reported here as primary findings since they recur independently in my own trace.

Findings (posted individually as inline/PR comments above):

1. [Blocking, high confidence] crates/rocm-core/src/examine.rs:1707 — In apply_kernel_gpu_membership's kernel-only branch, an unresolved KFD gfx target (empty string, produced when parse_linux_kfd_gfx_target can't map a gfx_target_version and intentionally returns None/"unknown") is fed into gfx_is_apu_family, which returns false for "" (per the existing test assert!(!gfx_is_apu_family(""))). This turns a genuine "unknown" signal into a confident is_apu: Some(false), flipping has_discrete_amd to true for what could be a real, not-yet-mapped APU. Before this PR the same branch left is_apu: None, which summarise_gpu_categories correctly treats as "don't count either way." This reintroduces, on the KFD-decode path, the exact symptom class (examine: Strix Halo APU (gfx1151, Ryzen AI Max+ 395) misclassified as discrete GPU (has_apu=false) #449) the PR fixes on the PCI-name path.

2. [Non-blocking, medium confidence] crates/rocm-core/src/examine.rs:1500 (probe_gpus_sysfs_fallback) — This adjacent path has the identical "target alone answers the family question" situation this PR fixes elsewhere, but still deliberately sets is_apu: None, contradicting the PR's own new reasoning. On a host where both lspci and rocminfo are unreachable, an APU (e.g. Strix Halo/gfx115x) detected only via the DRM ip-discovery sysfs fallback still reports has_apu: false, reproducing issue examine: Strix Halo APU (gfx1151, Ryzen AI Max+ 395) misclassified as discrete GPU (has_apu=false) #449's symptom on this untouched path. (tomastola's review flags this same gap as non-blocking.)

No other blocking issues found beyond what's noted above; reuse/simplification/efficiency angles and cross-file caller checks did not surface anything that survived verification.

🤖 Generated with Claude Code

[Woody-Hu]

Pushed f5f435e for the review. What changed, item by item:

Blocking 1 -- examine-29 never used the host it plants. Fixed by switching the When to "the user inspects that machine in machine-readable form". You were right about the mechanism: run_rocm does not apply world.command_env, so the stub PATH and ROCM_CLI_TEST_HOST_ROOT were dropped and the scenario asked the runner itself. The comment on the scenario now says why that step is load-bearing instead of only claiming every Linux lane can drive it.

Blocking 2 -- nothing tested the is_apu != Some(true) guard. Added a_resolved_apu_verdict_is_not_downgraded_by_a_kernel_target: is_apu: Some(true), empty target, node target gfx1200 (outside the APU list). It asserts the verdict stays Some(true) and that the empty target field is still filled in.

Blocking 3 -- the kernel-only path had no scenario. Added examine-30 (examine-classifies-a-kernel-only-apu): a planted KFD node at gfx1151, a stub lspci that exits 0 with an empty bus, and a stub rocminfo. That reaches the matched == None branch, which examine-29 cannot because it always has a PCI match. The Then asserts has_apu: true, has_discrete_amd: false, is_apu: true and gfx_target: gfx1151.

Non-blocking, taken:

• An undecoded gfx_target is now unknown rather than discrete, in both branches (also raised inline by the automated review). KFD leaves the field empty when gfx_target_version does not decode, and gfx_is_apu_family("") is false, so Some(false) there was a confident verdict from a failed decode. Covered by an_unparseable_kernel_target_leaves_the_family_unknown.
• The 1586 comments now say older packaged pci.ids copies predate the entry rather than that upstream has no name -- upstream does have it, and the row cites it the way the gfx90c rows cite theirs. 1586 is now in pnp_device_id_table_agrees_about_apus.

Non-blocking, deferred: the sysfs fallback still leaves is_apu unset. That is a different probe path (DRM ip-discovery hosts), and classifying there changes behaviour for hosts this PR never reproduced on, so I would rather land it as its own change with its own evidence than widen this one. Your other point stands, though: the comment that says the topology "not whether it is integrated" no longer matches the rocminfo and hipInfo paths, and I will fix that wording in that follow-up.

Decisions: keeping #449 and #507 in one PR as you allowed -- say the word if you would rather have the 1586 row split out, it is two files and one test row. On #521: it reworks the same classification and already carries a 1586 row, so whichever lands second should drop its duplicate rather than merge both. I have no preference on the order; happy to rebase on top of it, and I will drop both the row and the reclassification here if #521 lands first.

Local verification: cargo test -p rocm-core --lib gives 734 passed with 3 failures (proc_lifecycle x2, fix::print_only_fix_returns_zero) that reproduce on 0b542d5 without this commit, so they are not from these changes. cargo check -p e2e-cucumber --tests is clean. On macOS the examine scenarios are @requires-os:linux, so the CI lane is the first real run of examine-30.

[Woody-Hu]

Note for whoever has write access: the workflows for f5f435e are sitting in action_required (fork PR, approval needed), so nothing has run yet. Approving them is the only way to see whether examine-30 and the fixed examine-29 pass.

[nowycondro]

🔴 Automated review · pr-review-watcher · f5f435e

Review — needs work

Full review of the whole change (examine.rs, host_gpu.rs, examine.feature, examine_steps.rs) between base efb9bc37 and head f5f435ed.
Implements the is_apu classification fix for the Strix Halo symptom in issue #449 (explicitly scoped to the classification question only — the issue's other two asks, resolving a target from the marketing name and reporting UMA/VRAM-carveout, are stated out of scope), and adds the host_gpu.rs 1586 → gfx1151 PCI-id table row requested in #507 (the description itself notes this only exercises the Windows/PNP path for now; the Linux lspci device-id reader #507 also asks for is still unbuilt). Neither issue is claimed closed.

Blocking

• The PR description is stale relative to the current head
  The live description (and its "Verification on hardware" / checklist sections) documents only the first two commits (72e4848a, 0b542d58) and scenario examine-29. It says nothing about the third commit (f5f435ed), which: rewires examine-29's When step to actually drive the planted host (the scenario previously ran against the real test runner and passed regardless of the fix), adds a new scenario examine-30 for the kernel-only-node branch, changes an undecoded/empty kernel target from "discrete" to "unknown" (None), and corrects the pci.ids claim in the comments. A reader relying on the description to decide what's covered would not know any of this happened.
  Confidence 85 · architectural · Fix: update the PR description to cover the latest commit (new scenario, the When-step correction, the unknown-vs-discrete semantics, the comment correction), or at minimum link to the commit's own message, which already explains it well.

Non-blocking

• A sibling function's rationale now contradicts this change's own reasoning — crates/rocm-core/src/examine.rs:1493-1508 (probe_gpus_sysfs_fallback) vs. crates/rocm-core/src/examine.rs:1671-1737 (apply_kernel_gpu_membership)
  probe_gpus_sysfs_fallback still leaves is_apu: None with the comment "is_apu is left unset rather than guessed... Claiming either would be inventing a fact," even when it has a known gfx_target. This change's new code, given the exact same shape of evidence (a bare gfx_target, no marketing name), now argues the opposite and classifies family from it via gfx_is_apu_family. A gfx1151 part found only through DRM ip-discovery (sysfs fallback's documented use case) still reports has_apu: false after this PR.
  Confidence 85 · architectural · Fix: apply the same target-derived classification in probe_gpus_sysfs_fallback, or add a comment there explaining why it is deliberately excluded.

• One new test only discriminates a guard mutation, not a full revert — crates/rocm-core/src/examine.rs:5203 (a_resolved_apu_verdict_is_not_downgraded_by_a_kernel_target)
  If the whole diff were reverted (the if gpu.is_apu != Some(true) && ... block removed outright, not just weakened), is_apu would also trivially stay Some(true) here, since pre-PR code never touched is_apu in this branch at all — this test only fails if the guard condition itself is mutated (e.g. to if true), which it does correctly catch. Full-revert coverage for the fix is actually carried by the sibling test at examine.rs:5110 (a_kernel_target_classifies_an_unnamed_apu).
  Confidence 75 · logic · Fix: none required; optionally note in the test's comment that it is a guard-regression test rather than a feature-presence test.

• No cross-reference between the two unrelated "1586" fixes — crates/rocm-core/src/host_gpu.rs:722, tests/e2e-cucumber/tests/e2e/examine_steps.rs:1541,1591, tests/e2e-cucumber/features/examine.feature:450,462
  The new "1586" => Some("gfx1151") row in host_gpu.rs is reached only through the Windows/PNP path (gfx_target_from_pnp_device_id and its three call sites, all under WindowsDisplayAdapter/parse_windows_display_gfx_target) — the Linux lspci-name path (probe_gpus_lspci → classify_amd_marketing_name) never consults it, and the device-id reader #507 asks for to make that possible (extract_lspci_device_id or similar) does not exist in this codebase yet. The two new Linux e2e scenarios correctly exercise the real Linux fix (apply_kernel_gpu_membership's target-based reclassification), not this table row, but neither the feature file's scenario comments nor the fixture doc comments say so — a reader skimming both diffs together could assume the e2e scenarios validate the new table row.
  Confidence 70 · architectural · Fix: a one-line note in either file disambiguating the two "1586" fixes (Windows PNP table vs. Linux kernel-target reclassification).

Decisions for the author

None

Positive signals

• The non-downgrade guard (gpu.is_apu != Some(true) at examine.rs:1735) is minimal and strictly additive — it only ever strengthens an unresolved or failed verdict, never overwrites one the PCI name already resolved.
• The four new unit tests (examine.rs:5110,5146,5171,5203) each isolate a different branch of the fix (unnamed-but-matched, kernel-only, undecoded target, already-resolved) — together they close exactly the mutation gaps an earlier review round identified as untested.
• The two new e2e scenarios (examine-29/examine-30) correctly use the scenario-environment When step and were verified end-to-end against the KFD location_id/gfx_target_version decode arithmetic (0xc500 → 0000:c5:00.0; 110501 → gfx1151).
• Code comments throughout are unusually precise about why, and that reasoning is actually encoded in the guard logic rather than only asserted in prose (e.g., the .then(...) guard genuinely is what keeps an empty target from becoming Some(false)).
• Commit messages are thorough and well-structured: each of the three commits explains what changed and why, cites concrete before/after JSON, and is signed off.

Deployment notes

This is a cross-repository (fork) pull request, and the only status check that has run against the reviewed commit is a docs build (docs/readthedocs.com) — no Rust test, clippy, formatting, or commit-signature check has executed yet, likely pending workflow approval for a fork contribution. The author reports (unverified here) cargo test -p rocm-core --lib: 756 passed, clean clippy/fmt. This repo's sandbox had no cargo toolchain, so these claims were not independently re-run.

What this covered

All four changed files in full (crates/rocm-core/src/examine.rs, crates/rocm-core/src/host_gpu.rs, tests/e2e-cucumber/features/examine.feature, tests/e2e-cucumber/tests/e2e/examine_steps.rs) between efb9bc37672e9ca57966be7e30cd064061909348 and f5f435ed80128d4b707f8bb5527267e2ad5e5d07, plus the full surrounding context: gfx_is_apu_family, classify_amd_marketing_name, probe_gpus_sysfs_fallback, summarise_gpu_categories, apply_rocminfo_gpu_agents, the Windows PNP-id path, and diagnose.rs's consumers of is_apu. Reviewed via two independent fan-out passes (production code; e2e tests) plus driver-level verification and cross-checks. History pass covered all three commit messages. Agent-instruction pass checked against this repo's AGENTS.md.

Did not run: cargo test / cargo clippy (no Rust toolchain available in this review sandbox) — verification was by static tracing instead. No CI build/test/lint check has executed against this commit (only a docs build), so that class of verification is not handled by anything external either.

This PR's own review/discussion state was visible during the mandatory live-state check that precedes this review (to decide whether a re-review was warranted); none of that prior discussion's wording or conclusions were used to produce the findings above — each was independently re-derived and verified directly against the code, and several (the e2e wiring fix, the four new unit tests, the pci.ids comment correction) were confirmed to already be resolved on the current head. Not otherwise reconciled against any discussion on this PR.

[Woody-Hu]

Rebased onto 79f6e08c (head 6e982fcd) and rewrote the description to match the tree. Since the last review:

• Description now covers all four commits, including the third one: the When-step correction, the new kernel-only scenario, an empty gfx_target staying None, and the pci.ids comment correction. It also states what was and was not run.
• Rebase: the only conflict was examine.feature. main took examine-29 for the Windows driver scenario, so the two scenarios here are now examine-30 (@id:examine-classifies-an-apu-the-pci-name-cannot) and examine-31 (@id:examine-classifies-a-kernel-only-apu).
• Comments only, from the latest review: the 1586 row now says that only the Windows/PNP path reads it today and that the Linux lspci device-id reader is the other half of examine: lspci cannot name a GPU that pci.ids does not know, though the device id is on the line #507, so the Linux scenarios are not taken as coverage for that row; the non-downgrade test now says it pins the guard rather than the fix, and names the sibling test that carries revert coverage.
• probe_gpus_sysfs_fallback is left unchanged and listed as a follow-up in the description, with the reason: it is the last pass in the sequence (KFD → rocminfo → sysfs) and faces the same target-only evidence, but the classification itself is being reworked in fix(examine): identify AMD GPUs by device id and one name table, and never downgrade an APU verdict (ROCMAI-534) #521.

Local gates on the rebased tree: cargo test -p rocm-core --lib → 794 passed, 3 failed (the same three macOS-only failures that fail at the base commit), cargo fmt --all -- --check clean, cargo clippy -p rocm-core --all-targets -- -D warnings clean on 1.96.

Not run: the cucumber scenarios. They are @requires-os:linux and this machine is macOS, so examine-30/examine-31 need the CI Linux lane — CI, CodeQL and E2E self-hosted are still action_required on this fork PR and need a maintainer to approve the workflow run.

@tomastola the three blocking items from your review are addressed in the third commit; could you take another look when you have a moment?

[nowycondro]

🔴 Automated review · pr-review-watcher · 6e982fc

Review — needs work

Full review of the whole change (4 files, merge-base 79f6e08c42b9…head 6e982fcd8b72).
Fixes the classification symptom reported in #449 (not the Linux-lspci-device-id half of
#507, which the description says is deliberately out of scope and is instead addressed by
the separate, still-open PR #521).

Blocking

• Branch is not mergeable against current main — a real conflict, not a stale-UI
  artifact — tests/e2e-cucumber/features/examine.feature:465 (this PR's new
  examine-30), tests/e2e-cucumber/features/examine.feature:464 (main's existing,
  colliding examine-30)
  gh pr view 726 --json mergeable,mergeStateStatus reports "mergeable":"CONFLICTING",
  "mergeStateStatus":"DIRTY". Reproduced locally with
  git merge-tree origin/main pr726head, which reports
  CONFLICT (content): Merge conflict in tests/e2e-cucumber/features/examine.feature.
  Root cause: this branch's merge-base (79f6e08c) predates a main commit (ea86403c,
  "Remove amdgpu-install as an install method") that appended its own new scenario —
  also self-titled Scenario: examine-30 - System inspection reports a current install-method name... — to the same feature file, at essentially the same anchor
  point (right after examine-29) that this PR's two new scenarios
  (examine-30/examine-31) are appended to. A naive "accept both sides" conflict
  resolution leaves two different scenarios both named examine-30, which violates this
  repo's own uniqueness/sequential-numbering invariants enforced by
  tests/e2e-cucumber/tests/feature_naming.rs (scenario_indexes_are_unique_across_the_suite,
  scenario_names_are_indexed_sequentially_per_feature) — a drift guard written for exactly
  this class of mistake.
  Confidence 100 · architectural · Fix: rebase onto current main, renumber this PR's two
  new scenarios past whatever is now the last examine-NN, and update the stale
  cross-reference in the next finding to match.

Non-blocking

• Stale scenario cross-reference in a doc comment —
  tests/e2e-cucumber/tests/e2e/examine_steps.rs:1588
plant_kernel_only_apu_fixtures's doc comment says "The rocminfo stub is load-bearing
  for the same reason as in examine-29" — but examine-29 is the unrelated Windows
  driver-version/age scenario, which has no rocminfo stub at all. The matching reasoning
  actually lives in the scenario immediately above it, examine-30 ("A GPU whose PCI name
  is unrecognised..."). Almost certainly a leftover from the renumbering this PR's third
  commit performed (29/30 → 30/31 per its own commit message), where this one
  cross-reference was missed.
  Confidence 100 · mechanical · Fix: change "examine-29" to "examine-30" (and again to
  whatever it becomes after the rebase above).

Decisions for the author

• probe_gpus_sysfs_fallback still returns is_apu: None for the same
  target-only-evidence situation this PR just resolved for the kernel-membership and
  PCI-name-lookup paths — answered-by-intent
  PR description: "Follow-up, not fixed here: probe_gpus_sysfs_fallback still leaves
  is_apu: None and its comment still argues that claiming either verdict would invent a
  fact. It is the last pass in the sequence (KFD → rocminfo → sysfs), so it only runs when
  neither KFD nor lspci is available... Changing it belongs with the classification rework
  rather than this fix." Would otherwise read as an inconsistency this PR introduces (two
  code paths reasoning about the same "target is the only evidence" situation now disagree
  on what to do with it). The description owns this as a deliberate, scoped-out follow-up —
  confirm that's still the intended boundary before merging.

Positive signals

• The new host_gpu.rs "1586" table-row comment precisely scopes which platform path reads
  it today (Windows/PNP) versus the still-open Linux half of #507, so a reader can't mistake
  this row alone for closing the Linux case.
• a_resolved_apu_verdict_is_not_downgraded_by_a_kernel_target's comment explicitly flags
  itself as a guard-regression test rather than revert coverage, and names the sibling test
  that actually carries the revert coverage — more precise test-intent documentation than
  this kind of change usually gets.
• The two new e2e fixtures' planted KFD location_id/gfx_target_version values are
  arithmetically correct for the PCI address and gfx target they claim to encode
  (50432 == 0xc500, matching pci_id "0000:c5:00.0"; 110501 decodes via this repo's own
  major/minor/revision packing to gfx1151) — verified by hand, not just plausible-looking
  magic numbers.

Deployment notes

No Rust/clippy/e2e CI has executed against this head commit — the only check that has run
is the docs build (pass). The description attributes this to the fork's workflow run
awaiting maintainer approval. Combined with the confirmed merge conflict above, this PR is
not yet in a CI-verified, mergeable state; approve the workflow run and get a green
Rust/clippy/e2e lane — which, post-rebase, will also need to pass
tests/e2e-cucumber/tests/feature_naming.rs — before merging.

What this covered

DEGRADED REVIEW. Fan-out was attempted (two parallel code-review workers, one for
examine.rs/host_gpu.rs, one for the .feature/step-definition pair) but every spawn
failed on a concurrency limit shared across this host (20 subagents already running,
none of them mine) and the harness directed not to retry; the review proceeded
single-context instead. Recommend re-running with fan-out available, per this skill's own
guidance that a single-context pass catches a minority of what an independent-worker pass
finds.

What was actually read: all 4 changed files in full
(crates/rocm-core/src/examine.rs, crates/rocm-core/src/host_gpu.rs,
tests/e2e-cucumber/features/examine.feature,
tests/e2e-cucumber/tests/e2e/examine_steps.rs) between merge-base 79f6e08c42b9 and head
6e982fcd8b72, plus surrounding call paths: classify_amd_marketing_name,
probe_gpus_lspci, gfx_target_from_pnp_device_id/amd_pci_device_id_from_pnp_id, the KFD
gfx_target_version decode chain (parse_linux_kfd_gfx_target,
gfx_target_from_gc_version), gfx_target_has_no_rocm_build, and
tests/e2e-cucumber/tests/feature_naming.rs's scenario-numbering guard. Also read, at
Step 9: this PR's own description, and issues #449, #507, and PR #521 by identifier via
gh issue view/gh pr view.

Did not run: cargo test/cargo check/cargo clippy — no Rust toolchain is available in
this review environment, so the code-logic reasoning above is argued from tracing the
source, not from execution, and is scored accordingly. The merge-conflict finding above
was verified by actually running git merge-tree and querying GitHub's own mergeable
field. History and prior-changes passes: no prior review of this PR was consulted (by
design); git log over the touched files shows no relevant history beyond the linked
issues. Not reconciled against any discussion thread on this PR.