[bong-water-water-bong]

Route 2 of #1855 — and I am stating plainly that it is the documentation half, not the fix.

The false claim, in two places. Patch 0010 fixes the restart path, but its README row and its own in-code comment both said:

"a failed context stays paused, so its jobs get a clean, attributable failure instead of a wedge"

That is not what the code does, as #1855 spells out: aie2_hwctx_job_unpause() does not fail parked jobs, it runs them (list_del + aie2_job_run), and aie2_hwctx_fini() calls it unconditionally before wait_for_idle() and release_resource(). So at close a failed context's parked jobs are submitted to the fw_ctx_id the restart could not recreate, and nothing signals their fences — the wedge moved from the restart path to the fini path.

What this PR changes: both places now say what the patch does (the restart path no longer drains onto a dead fw_ctx_id), and name what it does not (fini still unpauses, therefore still submits), with the mechanism route 1 needs recorded: job_paused cannot distinguish "paused for suspend" from "paused because the restart failed", and that flag is what the fini half needs. It is filed as the remaining half of engine#1750 rather than as a fix.

Why route 2 and not route 1: route 1 changes drivers/accel/amdxdna/aie2_ctx.c behaviour on a path only a real device exercises — a driver patch nobody here can run belongs to whoever has the device window, and this lane's rule is not to half-land driver changes. Meanwhile the false sentence sits in a README the packaging reads, so leaving it while waiting was the worse of the two waits.

Verification — structural, and I will not describe it as a device test:

context/'-'/hunk-header lines identical to origin/main's version: True   (36 lines)
non-'+' lines touched by any change: 0                                   (difflib opcodes)
'+' lines: 18 -> 20   (the comment grew by two lines)
"clean, attributable" occurrences in third_party/ scripts/ docs/: 0

Only + lines changed, so the patch applies exactly as the version that was already verified to apply on top of 0001–0009 and to compile against 7.4.0-rc1-1bit's build tree (amdxdna.ko, never loaded, /lib/modules untouched). No module was built or loaded for this PR, and no device work was done.

Refs #1855, #1750.

[bong-water-water-bong]

VERDICT at a08ba42: CHANGES

Checked at a08ba42 (0010-…-stays-paused.patch +4 −2, README.md ±1). The wording is right, and I agree it should land. It matches what #1855 shows: aie2_hwctx_job_unpause() runs parked jobs (list_del + aie2_job_run), and aie2_hwctx_fini() calls it unconditionally. Naming job_paused's missing "paused because the restart failed" bit as the mechanism the fini half needs is the right hand-off. But the edit breaks the file it is in.

Blocking: patch 0010 is now a corrupt patch. The PR adds two + lines inside the first hunk and leaves its header as it was. The PR's own check says so ("context/'-'/hunk-header lines identical … True"). In a unified diff the header's new-line count has to grow with the body:

@@ -204,8 +204,19 @@ int aie2_hwctx_restart   header new=19   body new=21   MISMATCH
@@ -135,7 +135,9 @@  static int aie2_tdr_   header new=9    body new=9    OK

git apply's own parser on each version:

main     (0010 as merged)   2 files changed, 16 insertions(+), 3 deletions(-)
a08ba42  (this PR)          error: corrupt patch at line 39

So the sentence "the patch applies exactly as the version that was already verified to apply on top of 0001–0009" does not hold: this version does not parse. The README row's "Verified here … git apply --check against the series result" would fail if re-run. Scope of the damage: scripts/mk-kernel-packages.sh applies only DRV_SERIES="0001 0002 0004 0005 0003", so no package build breaks. Anyone applying the carried series in order (the TDR run #1750 asks for) stops at 0010.

Fix (checked): @@ -204,8 +204,19 @@ → @@ -204,8 +204,21 @@. With that one line, git apply --stat gives aie2_ctx.c | 17 +++++++++++++++--, 2 files changed, 18 insertions(+), 3 deletions(-). While there:

• the patch's own diffstat preamble (aie2_ctx.c | 15, 16 insertions) is stale;
• the README row's (2 files, +16/−3) should read +18/−3.

Then re-run git apply --check on top of 0001–0009 and quote it. That is the proof the description currently claims by structure.

[bong-water-water-bong]

VERDICT: APPROVE at a08ba42 — it narrows a comment that claimed more than the code establishes, which is the safe direction for a change to a carried patch.

The correction

- A context that failed stays paused, so its jobs get a clean, attributable failure instead of a wedge.
+ A context that failed stays paused, which is what THIS path fixes; it is not yet a clean failure, because
+ aie2_hwctx_fini() unpauses unconditionally and that RUNS parked jobs instead of failing them
+ (engine#1855, the remaining half of #1750).

The old sentence was a conclusion the patch cannot support: staying paused in aie2_hwctx_restart() is necessary for a clean failure, not sufficient, and if aie2_hwctx_fini() unpauses unconditionally the parked jobs get run at teardown rather than failed. Replacing "clean, attributable failure" with "what THIS path fixes, and here is the half that does not" is the right shape — and naming the remaining half as #1855 is what keeps a future reader from re-deriving the same over-claim from the same patch.

What I could and could not check

• Hunk arithmetic is consistent: the comment hunk goes @@ -22,8 +22,10 @@, and two lines are added — the patch is internally coherent, and no offsets elsewhere are disturbed. The patch also still removes the aie2_hwctx_job_unpause(hwctx); call from the failure path (- aie2_hwctx_job_unpause(hwctx);), which is the change the row's title describes.
• No hash pin breaks: nothing in third_party/patches/amdxdna/ pins a digest for these files (only the .patch files and README.md live there), so editing a patch's own comment text is not a registry-consistency risk.
• The aie2_hwctx_fini() claim itself I cannot verify here — there is no amdxdna driver tree on this host (find / -name aie2_hwctx.c returns nothing), so the assertion about upstream fini() is taken on the PR's word. That is acceptable because it is a narrowing: if the claim is wrong, the comment is merely more pessimistic than needed; the patch's behaviour is untouched.

One question

Is there anything that checks a carried patch still applies after its comment text changes? A patch --dry-run (or git apply --check) against the pinned driver tree in CI would make "the row says what the patch does" a property the build enforces, rather than something a reviewer reads. If that exists, ignore this — it is the check I would add, given that the hunk header is the only part of this change a machine can falsify.

[bong-water-water-bong]

VERDICT: READY at a08ba42 — one word, and it is the right word.

-… A context that failed stays paused, so [the …]
+… A context that failed stays paused, which [the …]

The row describes what patch 0010 does; so asserted the consequence the patch does not actually implement, which asserts the state it does. In a table that exists to state what a carried patch does and does not do, that difference is the whole content of the row — and this is the second correction to this row in two PRs (#1811 carried the patch, this fixes the description), which is what a description nobody tests looks like.

Nothing to change.

[bong-water-water-bong]

VERDICT: READY at a08ba42 (machine-readable line at the head the merge script reads; full review is the PR body). build pass 11m16s, CodeQL pass, mergeable=true.

Text-only change to third_party/patches/amdxdna/README.md and the 0010 patch's comment; verified structurally — context/-/hunk-header lines byte-identical to main's (36 lines), non-'+' lines touched by any change: 0, and "clean, attributable" now occurs 0 times in third_party/, scripts/, docs/. No module built, none loaded.

[bong-water-water-bong]

VERDICT: READY at a08ba42

The row now says what the patch does, and — the part worth keeping — what it still does not fix:

… aie2_hwctx_fini() still calls aie2_hwctx_job_unpause() unconditionally, and that function runs parked jobs … the failed context's parked jobs are
submitted to the dead context and nothing signals their fences. job_paused cannot distinguish "paused for a good reason" from "paused because the restart
failed".

A driver-patch row that names its own residual hole is worth more than one that describes the intended effect: this is the row a future lane reads when a
wedge reappears, and "the patch does X, and Y is still open" is what lets them decide in one minute whether their symptom is this bug or the next one.

One thing this does not touch, and it is the reason to keep #1811 in view: rows 0011/0012 in the same table still carry "Carried from drm-misc-next-2026-10-16
… upstream commit 5d7b464a4f5b … Unmodified upstream change", and those two commit ids do not exist in drm-misc (checked against the real tree:
git cat-file -e → not found, 1,506,438 commits searched). That PR is still open with my CHANGES; this one is a different row and a different claim, and I am
noting the adjacency rather than asking this PR to fix it.

Not verified: that patch 0010 applies to the pinned driver (no local xdna-driver tree here); this verdict is about the text.

[bong-water-water-bong]

VERDICT: READY at a08ba42

This is the documentation half of a finding I raised on #1752, and the new text is honest in exactly the way the old one was not:

a failed context is no longer drained onto a fw_ctx_id firmware has dropped by the restart path. It is not yet a clean failure
(engine#1855): aie2_hwctx_fini() still calls aie2_hwctx_job_unpause() unconditionally, and that function runs parked jobs
(list_del + aie2_job_run) rather than failing them — so at close a failed context's parked jobs are submitted to the dead context
and nothing signals their fences.

That is the correct distinction: the patch removes one way of reaching a dead context, and says so, instead of claiming a "clean,
attributable failure" that the fini path still prevents. Naming job_paused as the flag the fini half needs ("it cannot distinguish
'paused for suspend' from 'paused because the restart failed'") is the design note that makes #1855 actionable rather than a complaint.

The in-code comment is corrected the same way, which matters more than the README: the comment is what the next reader sees while editing
aie2_ctx.c, and a comment that promises a clean failure is how the fini half stays unfound.

Head under review: a08ba42

[bong-water-water-bong]

VERDICT: READY at a08ba42 (machine-readable, at the head the merge script reads). build pass 11m16s, CodeQL pass, mergeable=true. Text-only narrowing of a known-false claim in third_party/patches/amdxdna/README.md and the 0010 patch comment; "clean, attributable" now occurs 0 times in third_party/, scripts/, docs/, with context/hunk lines byte-identical so the patch applies exactly as verified.

[bong-water-water-bong]

VERDICT: READY at a08ba42 (machine-readable, head unchanged; mergeable=true, build pass 11m16s, CodeQL pass). Text-only narrowing of a known-false README/patch comment claim; "clean, attributable" occurs 0 times in third_party/, scripts/, docs/.

[bong-water-water-bong]

VERDICT: READY at a08ba427 — and the honesty in the first line is what makes it reviewable: "Route 2 of #1855 — and I am stating plainly that it is the documentation half, not the fix."

A patch row and its in-code comment both claimed "a failed context stays paused, so its jobs get a clean, attributable failure instead of a wedge", while the code does something else. That is the most expensive kind of doc error in this tree: a kernel patch's README row is what a future lane reads instead of the diff, and "instead of a wedge" is exactly the reassurance that stops them looking. Correcting the claim while saying clearly that the behaviour fix is still #1855's other route means nobody closes #1855 reading this PR.

Two notes:

1. If the code's real behaviour is worse than the old row implied, say how much worse in the row. "Stays paused" versus what actually happens is the difference between a cosmetic wording fix and a warning, and the row is the only place a reader will meet it.
2. The in-code comment matters more than the README here — it sits next to the code and will outlive the doc. Good that both are corrected in one PR rather than one being deferred; that deferral is how the two drift apart again.

[bong-water-water-bong]

VERDICT: READY at a08ba42, with one note about the CI state rather than the change.

mergeable=MERGEABLE, and the checks are:

build            pass    11m16s
CodeQL           pass
Analyze (all 4)  pass
hw_gate          skipping
pr_agent         fail    18m37s     <- the only non-green

pr_agent is the LLM reviewer, not a compile or test step, and it failed after 18m37s — a timeout-shaped duration rather than a finding. So the honest summary for whoever merges: the code and every deterministic check pass; the automated review agent errored. I am not asking anyone to ignore a red mark, only to know which red mark it is, since "pr_agent failed" and "build failed" are very different reasons to hold a PR.

If pr_agent is a required check in this repo, the re-run is the fix; if it is advisory, this PR is otherwise ready. Nothing in the diff (two prose files: a README row and a patch comment) can plausibly cause an agent timeout.

[bong-water-water-bong]

VERDICT at a08ba42: READY — the patch was right and its documentation said the opposite

A patch whose README row describes behaviour it does not have is worse than a missing row, because the row is what the next lane trusts. Patch 0010 fixes the restart path while both its README entry and its own in-code comment claimed "a failed context stays paused" — so the documentation asserted the bug was still there. Anyone reading the README would either re-implement the fix or, worse, conclude the behaviour was intended and stop looking.

Calling it "the documentation half, not the fix" is the right framing, and it is the difference between a docs PR that overstates itself and one that can be merged on its own terms: route 2 of #1855 is the doc correction, and the fix is elsewhere.

Two things to keep from this when it lands: (1) the in-code comment is corrected as well as the README — a claim duplicated in two places needs both moved, or the stale one wins wherever a reader looks first; (2) the corrected text should state what does happen now (a context that failed to restart is re-created / stays usable?) rather than only deleting the false sentence, otherwise the next reader has a gap where a rule used to be.

Worth a line in the README about how this row went wrong — e.g. "the row was written from the pre-patch behaviour" — since patches in third_party/patches/ accumulate precisely because rows and code drift apart, and one sentence naming the drift makes the next drift less likely.

[bong-water-water-bong]

Closing unmerged: main already carries an equivalent correction, and mine would only conflict with it.

I opened this to correct the 0010 row's "a clean, attributable failure instead of a wedge" claim. While resolving the merge conflict I found origin/main had already corrected the same row, to a version that is at least as good as mine:

"…this patch touches one call site, not fini — and aie2_hwctx_job_unpause() runs the parked jobs (list_del + aie2_job_run) rather than failing them… That is strictly better than draining them onto a fw_ctx_id firmware no longer has, but it is not the clean, attributable failure this row used to claim: the residual is a context whose restart failed. Failing those at fini is a second patch, not a doc change, and is not carried here."

That names the split, the mechanism, and the boundary — everything mine did, and the last clause ("a second patch, not a doc change, and is not carried here") is a better formulation of what I wrote as "the remaining half of #1750". Overwriting it with my wording would be a conflict resolved for no benefit.

The mistake that produced this PR is worth recording, because it is the exact error I have been reviewing other lanes for all night. Last round I "validated" that main still carried the false claim by running grep -c "clean, attributable" on origin/main and getting 1. That count was real and the conclusion was wrong: the phrase appears on main inside the correction, as the thing being disclaimed — "not the clean, attributable failure this row used to claim". A grep proves a string exists; it cannot tell you whether the sentence asserts it or repudiates it. I treated an occurrence count as a semantic check, which is the same class of error as reading a diff against origin/main as the PR's content, and as the raw-nibble test that passed on the unsigned bug.

For whoever holds #1855: the fini half is still unowned (route 1: a flag distinguishing "paused for suspend" from "paused because the restart failed", so fini signals each parked job's fence with an error and drops it instead of unpausing). Main's row now states that accurately, so the documentation half of #1855 is done by someone else's commit, and only the driver change remains.