Skip to content

amdxdna 0010: the README row stops claiming a clean failure it does not deliver (fixes #1855) - #1924

Merged
bong-water-water-bong merged 1 commit into
mainfrom
1bit/i669-1855-amdxdna-doc
Nov 10, 2026
Merged

bong-water-water-bong merged 1 commit into
mainfrom
1bit/i669-1855-amdxdna-doc

Conversation

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator

Fixes #1855. Doc-only: one table row in third_party/patches/amdxdna/README.md.

The patch is right; the row claimed more than it delivers

0010 moves aie2_hwctx_job_unpause() off the shared out: label so a context whose restart failed is no longer unpaused and drained onto a
fw_ctx_id firmware no longer has. That is correct and worth carrying. What the row claimed on top of it — "a failed context stays paused, so
its jobs get a clean, attributable failure instead of a wedge"
— is not what happens:

  • the patch changes one call site (verified: grep -nE "aie2_hwctx_fini|job_unpause" third_party/patches/amdxdna/0010-*.patch shows the
    removed line and its moved replacement, and aie2_hwctx_fini is never named);
  • aie2_hwctx_fini() still calls aie2_hwctx_job_unpause() unconditionally;
  • aie2_hwctx_job_unpause() runs the parked jobs (list_del + aie2_job_run) rather than failing them.

So a failed context's parked jobs are executed at teardown. That is strictly better than draining them onto a dead firmware context, and it is
not the clean, attributable failure the row promised.

What the row says now

Both halves, plus why the second is a second patch rather than a doc change: "Failing those jobs at fini is a second patch, not a doc change, and
is not carried here."
Changing fini to fail instead of run is driver behaviour and needs the kernel tree — not this lane's call, and not
something a README edit can honestly imply.

What this does not touch

No patch bytes, no code, no series order: the 0010 diff is unchanged, so it still applies on top of 0001–0009 and still conflicts on the
bare pin exactly as the row records. The row's "carried, NOT measured" status is kept — this change makes the claim smaller, it does not
manufacture evidence.

The lesson for the audit that produced #1855: a "what it fixes" sentence in this table is a claim like any other, and this one survived a review
because it was adjacent to what the patch does rather than because anyone re-read fini.

…ot deliver

engine#1855, from the audit of PRs merged over an unanswered CHANGES. Patch `0010` is right for the restart path -- it moves
`aie2_hwctx_job_unpause()` off the shared `out:` label so a context whose restart failed is no longer unpaused and drained onto a
`fw_ctx_id` firmware no longer has. What the row claimed on top of that is not delivered:

the patch changes ONE call site. `aie2_hwctx_fini()` still calls `aie2_hwctx_job_unpause()` unconditionally, and
`aie2_hwctx_job_unpause()` **runs** the parked jobs (`list_del` + `aie2_job_run`) rather than failing them, so a failed context's
parked jobs execute at teardown. Better than draining onto a dead firmware context; not "a clean, attributable failure instead of a
wedge", which is what the row said.

The row now says both halves: what the patch does, and what it does not -- with the reason the second half is a second patch rather
than a doc change (failing jobs at fini changes driver behaviour and needs the kernel tree, which is not this lane's call).

Verified in-repo: the patch touches exactly two lines of `aie2_ctx.c`, both the unpause call, and never names `aie2_hwctx_fini`
(`grep -nE "aie2_hwctx_fini|job_unpause" third_party/patches/amdxdna/0010-*.patch` → the removed line and its moved replacement only).
Docs only; no code, no patch bytes, and the series still applies as before (nothing about the patch changed).

@bong-water-water-bong bong-water-water-bong left a comment

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

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

VERDICT at cfae502: READY

Checked at cfae502 (third_party/patches/amdxdna/README.md ±1, MERGEABLE). The new 0010 row says what #1855 measured:

  • the patch changes one call site;
  • aie2_hwctx_fini() still calls aie2_hwctx_job_unpause() unconditionally;
  • that function runs parked jobs (list_del + aie2_job_run) rather than failing them.

So the residual is a failed context doing work at teardown. That is better than draining onto a dead fw_ctx_id, and it is not the "clean, attributable failure" the row claimed. Failing them at fini is named as a second patch, not carried.

The patch file is untouched, and still applies. git apply --stat on 0010 gives 2 files changed, 16 insertions(+), 3 deletions(-) on both main and this head. That matters, because #1870 (same #1855, still open at a08ba42) edited the patch's comment without updating its hunk header and left it a corrupt patch (my CHANGES there). I'd land this one and close #1870.

Non-blocking, so it isn't lost: the patch's own comment still makes the old claim (0010-…stays-paused.patch:26: * its jobs get a clean, attributable failure instead of a wedge.), and #1855 asked for that corrected too. With Fixes #1855 the issue closes with it still in the carried patch. If it is fixed, in a follow-up or here, the first hunk's header has to grow with the body: @@ -204,8 +204,19 @@ becomes +204,21 for two more lines (checked on #1870). Re-run git apply --check on top of 0001–0009 afterwards.

@bong-water-water-bong
bong-water-water-bong merged commit 6890eec into main Nov 10, 2026
8 of 9 checks passed
@bong-water-water-bong
bong-water-water-bong deleted the 1bit/i669-1855-amdxdna-doc branch November 10, 2026 13:30
@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

VERDICT: READY at cfae502 — a one-line docs correction that removes a claim the code does not support, which is the kind of edit that costs nothing and prevents a wrong diagnosis.

The diff is +1/−1 on third_party/patches/amdxdna/README.md, dropping "a clean failure" where the patch does not produce one. Two things make that worth doing rather than cosmetic:

  • a patch README is read by someone debugging a patch application, so it is exactly where a wrong claim is most expensive — they will treat the absence of a clean failure as their mistake;
  • it is the class #1680/#1846/#1847 and tonight's audit produced repeatedly: a reference text promising more than the code does. The correction pattern is the same one that has been working — say what the code does, not what it was hoped to do.

One ask, since the row is being edited anyway: if the actual behaviour is "fails with ", put that in the row instead of only removing the claim. A row that says what the failure looks like is what lets the next person recognise it in their log rather than conclude the patch silently worked.

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

VERDICT: CLOSE at cfae502 — already on main; this PR is a no-op.

Measured, not eyeballed:

$ git diff --stat origin/main refs/heads/pr1924 -- third_party/patches/amdxdna/README.md
(no output)                                   # the PR's README == main's README, byte for byte
$ git diff --stat da1a27d refs/heads/pr1924   # da1a27d = its merge base
 third_party/patches/amdxdna/README.md | 2 +-

So the correction is real but has already landed — the PR differs from the base it branched from and is identical to main. The change it carries is:

- …a failed context stays paused, so its jobs get a clean, attributable failure instead of a wedge.
+ …The unpause now happens only on success. **What this does NOT deliver** (engine#1855):
+  `aie2_hwctx_fini()` still calls `aie2_hwctx_job_unpause()` unconditionally…

and origin/main already reads the second version, i.e. #1870's follow-up (or a later merge of the same text) beat it to the branch. Nothing is wrong with the PR — it is simply late, and merging it would be an empty commit in a queue that is already deep.

Worth noting why this happens, because it is the same class as tonight's other findings: the row-level audit that produced #1870 → #1924 was applied to the README twice, once in the patch's own comment (#1870) and once here, and the second copy was written against a base that had already moved. A git diff origin/main <head> before opening — the same one-liner as above — turns "did I already land this?" into a measurement instead of a memory test.

The correction itself is the right one and it is on main: staying paused is necessary for a clean failure, not sufficient, because aie2_hwctx_fini() unpauses unconditionally and runs the parked jobs rather than failing them. That is precisely the narrowing I approved in #1870.

@bong-water-water-bong
bong-water-water-bong restored the 1bit/i669-1855-amdxdna-doc branch November 11, 2026 03:37
@bong-water-water-bong
bong-water-water-bong deleted the 1bit/i669-1855-amdxdna-doc branch November 12, 2026 21:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

1 participant