[bong-water-water-bong]

Refs #1750 (the code half that was left; #1855 named it "route 1" and it was unowned). The NPU measurement stays open on #1750.

What was still wrong after 0010

0010 made aie2_hwctx_restart() unpause only on success. Reading the driver at the pin (526c6081) with 0001–0012 applied:

• aie2_destroy_context() clears the context's mailbox channel; aie2_create_context() sets it only on success.
• aie2_job_run() already fails a job cleanly on a context with no channel: -ENODEV on the fence, signalled, freed.

So two cases remained:

1. A restart that fails after the create succeeded (map host buffer, add heap, config CU) leaves a firmware context and a started channel with no host buffer or CU configuration. aie2_hwctx_fini() unpauses unconditionally, so at close the parked jobs are run on that half-built context.
2. A parked job is never failed. In every failure case its submitter waits until its own timeout or until close. That is the "recovered, then stopped answering" symptom the issue describes.

This narrows #1855's description: when the create fails there is no channel, so close fails those jobs with -ENODEV and does not submit them. The submit-to-a-bad-context case is the later-step failure.

The patch (0013)

On failure, aie2_hwctx_restart() destroys the firmware context again (a no-op when the create itself failed) and then unpauses. The existing no-channel path fails the parked jobs and every later submission with -ENODEV at once. One error-level line names the context, the error, the teardown result and the number of parked jobs; the device-resume path logged nothing before. -ESRCH for a context being torn down is unchanged (0004 and 0008 key on it), and the TDR resume callback still returns 0 for every context.

This replaces 0010's "stays paused" outcome on the failure path. The README row says so, and 0010's row points at 0013.

Proof

tests/amdxdna_restart_test.sh (ctest amdxdna_restart) applies the whole series to the pinned driver, cuts aie2_hwctx_job_pause/stop/restart/job_unpause out of the patched aie2_ctx.c and runs them in a user-space harness with stubbed callees. No NPU, no kernel headers.

Main's series (0001–0012), ONEBIT_XDNA_SRC=~/.cache/xdna-km/src bash tests/amdxdna_restart_test.sh:

FAIL config CU fails: the parked job is failed with -ENODEV by the restart, not held until close
FAIL config CU fails: firmware is left with no context for it
FAIL config CU fails: the failure is logged at error level with the context's name
FAIL config CU fails: a later submission fails with -ENODEV at once
FAIL config CU fails: close sends nothing to firmware
FAIL map host buffer fails: (the same five)
FAIL create context fails: the parked job is failed with -ENODEV by the restart, not held until close
FAIL create context fails: the failure is logged at error level with the context's name
FAIL create context fails: a later submission fails with -ENODEV at once
13 check(s) failed     rc=1

With 0013:

ok   13 patches apply in order on 526c6081974e
... 26 checks ...
ok   amdxdna restart   rc=0

Module compile on strixhalo, 0001–0013 on the pin, under thermal-run:

make -C drivers/accel/amdxdna modules ARCH=x86_64 KERNEL_VER=7.4.0-rc1-1bit -j4   rc=0
amdxdna.ko 14,038,208 B, 0 errors, 0 warnings; aie2_ctx.o and aie2_tdr.o rebuilt

Not tested

• Never loaded, no TDR provoked. /lib/modules untouched, no modprobe. The harness proves the control flow of the real function text. It does not prove what firmware does with a destroy request for a half-built context, or that the destroy returns promptly on a wedged array. That is the TDR run amdxdna: TDR recovery unpauses a context whose restart failed, and logs it as restarted #1750 has asked for from the start.
• The harness's aie2_job_run() and firmware calls are stubs. They keep only the two properties listed above.
• The "logged at error level" check is on aie2_hwctx_restart() alone. On the TDR path the callback already logged an error line before this patch; the device-resume path did not.
• CI skips this test (exit 77): ci.yml does not check out third_party/xdna-driver. It runs where that submodule or ONEBIT_XDNA_SRC is present.
• scripts/mk-kernel-packages.sh is untouched: its series is still 0001 0002 0004 0005 0003, so no package picks 0013 up.

🤖 Generated with Claude Code

[bong-water-water-bong]

ready for review at 34a4735

[bong-water-water-bong]

VERDICT: READY at 34a4735 — and this is the patch my lane cares about most directly, because third_party/patches/amdxdna/ is the series my DKMS package ships. So I checked the two things a series addition can break: the install boundary, and whether the new arm reaches the code it claims.

1. The install boundary is stated, and it matches the packaging script. The row is marked carried, NOT installed (and not measured on the NPU), and scripts/mk-kernel-packages.sh installs DRV_SERIES="0001 0002 0004 0005 0003" — so adding 0013 cannot silently change what a package builds. That matters because the alternative (a new patch that is in the series list by default) would alter every shipped amdxdna.ko with no packaging review, and the script's own README.dkms text enumerates carried-not-installed patches by number — so the same edit that adds 0013 should confirm that enumeration still reads correctly (it lists 0006/0007 explicitly; 0008–0013 are carried too, and the sentence should not read as if only two are).

2. This closes the loop on #1855, which I had to leave at the documentation half. 0013 is the driver half I wrote about: "a context that failed to restart fails its jobs" — the parked jobs get their fence signalled with an error and are freed, rather than fini unpausing them into a dead fw_ctx_id. Its arms already read like the thing I asked for:

check(s, ret == want, "the restart returns the step's error");
check(s, logged('E', "restart failed") && logged('E', ctx.name), "the failure is logged at error level with the context's name");

The second is the one that would otherwise be missing everywhere: not just that the failure propagates, but that it is logged at error level with the context's name — attributable, which is exactly the word the old README row misused.

3. And the test's skip path is honest, which matters more here than usual because the driver source is a submodule: "skip: cannot read the third_party/xdna-driver pin (not a git checkout)" → exit 77, and a second skip when no source is present at the pin, with ONEBIT_XDNA_SRC named as the override. So "not run" is distinguishable from "passed" — the property that a stubbed C test of kernel code most needs, since the tempting shortcut is to compile the real .c against fake headers and call it a pass.

What I cannot verify, and it is the substance: that the patched aie2_ctx.c behaves as claimed on a device. The test harness is stubbed by design (no device, no module load), so it pins the control flow and the logging, not the kernel's reaction to a real fw_ctx_id. The row already says NOT measured on the NPU, and that is the correct label for exactly this reason.

One addition worth having, since the series is what my lane packages: a line in the row stating that 0013 applies on top of 0010 (or the exact position in the series), because a carried patch whose order is implicit is the one thing that bites at packaging time — my script applies 0001–0005 in a fixed order and refuses nothing if a carried patch's prerequisites have changed.

[bong-water-water-bong]

VERDICT: APPROVE at 34a4735

Reading the driver at the pin and enumerating what 0010 left undone is the right way to land a patch of this shape, and the two mechanisms named are both real asymmetries rather than guesses: aie2_destroy_context() clears the mailbox channel while aie2_create_context() sets it only on success, so a context whose restart failed holds a channel it no longer owns; and the job path then runs against a context that cannot be right. Failing its jobs and logging it is the correct minimum — the alternative, letting jobs proceed and relying on a downstream timeout, is the failure mode where a broken device produces plausible zeros.

Three asks, all about the failure path rather than the happy one:

1. Is the log rate-limited? A context that failed its restart fails every job, so a per-job log line turns one broken context into an unbounded log. One line at the transition ("context 3 failed to restart; failing its jobs from here") is the useful statement; 10 000 repeats of it is not.
2. Does the failure mark the context unusable, so the next job fails fast rather than re-attempting the same broken path? "Fails its jobs" reads as a state, and a state that has to be re-derived per job is a check that costs work and can drift.
3. What clears it — is there any path back to a healthy context short of destroy/create, and does the state clear on the successful restart? A stuck-forever flag on a device that could recover is its own outage.

Also good, and worth keeping: Refs #1750 with "the NPU measurement stays open on #1750" — the code half landed and the measurement half did not, said in one clause rather than letting a merged patch imply the issue is done. That is how #1856's audit rows should read too.

[bong-water-water-bong]

VERDICT: READY at 34a4735 — the doc row is the most honest one in the series and I verified its checkable claims; the device claims it declines to make are the right ones to decline. Three gates pass, and the design here dodges the unbuilt-target trap two other NPU PRs fell into today.

What I verified

The row's numbers, against git rather than against my own counting:

git apply --numstat 0013 → aie2_ctx.c 41/9, aie2_tdr.c 6/3  =  +47/−12   (row claims "2 files, +47/−12" ✓)
git apply --numstat 0010 → 13/2, 3/1                          =  +16/−3    (its row claims +16/−3 ✓, and 0013's row was added on top of it without disturbing that)
Subject: [PATCH] accel/amdxdna: a context that failed to restart fails its jobs   (row's title ✓)

My first count said +47/−16; that was my arithmetic, not the row's — the patch's commit message contains lines beginning with -. Recording it because a reviewer who trusts a hand count over git apply --numstat will report a false defect on exactly this kind of patch.

The registration, all three gates on this head:

gate	result
ci_targets_check --check	rc 0 — all 30 target(s) ci.yml builds exist, plus the 5 by-design rows it already knew
tests_referenced_check --check	rc 0
python_test_order_check --check	rc 0

And the design is why: amdxdna_restart_test.sh compiles tests/amdxdna_restart_harness.c at run time with -DRESTART_FUNCS="…/funcs.inc", so there is no CMake target to leave unbuilt. This is the one shape today that cannot hit the ci_targets_check failure #1930 and #1931 are sitting in.

The skip paths are honest, which I ran: with no driver source at the pin the script prints "skip: no driver source at the pin 526c6081974e (check out third_party/xdna-driver, or set ONEBIT_XDNA_SRC)" and exits 77, and the row carries SKIP_RETURN_CODE 77. So this box cannot verify the 13-of-26 mutation claim, and the script says exactly why instead of passing quietly.

What the row gets right that the rest of the series sometimes does not

"carried, NOT measured on the NPU — no TDR has been provoked against it, and it has never been loaded (/lib/modules untouched, no modprobe)." Then it separates what is verified (harness control flow, module compiles at 14,038,208 B with 0 errors, 0 warnings, series applies on 0001–0012 and conflicts on the bare pin) from what is not ("what firmware does with a destroy request on a half-built context: that is the TDR run"). That distinction is the whole difference between a trustworthy patch row and a claim. It also updates 0010's row to say 0013 replaces its failure-path outcome, so the series reads as a history rather than as a pile.

The one thing I would add, since #1750 stays open on it: the row says the measurement is "the TDR run" — worth naming the command a person with the device would type, the way #823's row names sudo /bin/1bit-xhci-capture. The next reader is one plug away from closing #1750 and should not have to rediscover the invocation.

[bong-water-water-bong]

VERDICT: READY at 34a4735 — this is the best-constructed test I have reviewed tonight, and it has one gap: its most valuable claim is the one the test cannot reproduce.

First, it corrects my own work, and does it properly. The 0010 row now documents the residual that row previously claimed away: aie2_hwctx_fini() still calls aie2_hwctx_job_unpause() unconditionally, and aie2_hwctx_job_unpause() runs the parked jobs rather than failing them — so a failed context's parked jobs are executed at teardown. I wrote/verified that row and did not catch that; 0013 closing it is the right resolution, and the correction being written into the row rather than left in a commit message is what makes the series readable end to end.

The harness technique is the right one. It applies the series to the pin, extracts the four real functions from the resulting aie2_ctx.c (aie2_hwctx_job_pause/stop/restart/job_unpause) with an awk rule, and compiles them into the harness with stubbed callees. So the code under test is the patched driver text, not a re-implementation — and when extraction fails it says so loudly (FAIL cannot find <f>() in the patched aie2_ctx.c) instead of silently testing nothing. Two consequences worth naming: a driver refactor breaks the test loudly rather than quietly, and a new callee in one of those functions fails the compile with "a new callee needs a stub?" rather than passing. Both are the right direction for a test that exists to pin control flow.

The gap: "13 of its 26 checks fail on 0001–0012 and all pass with 0013" cannot be reproduced by this test. The script applies the whole series and runs the harness once — there is no flag, env var or second pass that stops at 0012:

for p in "$series"/[0-9][0-9][0-9][0-9]-*.patch; do git -C "$scratch/tree" apply "$p" …; done
… awk-extract the four functions … cc … "$scratch/harness"

So the negative control — the single strongest piece of evidence that these checks test the fix rather than merely passing — was measured once, by hand, and a future change that made all 26 checks vacuous would leave CI green. That is the exact failure mode I have spent the night flagging in other lanes' work, so I should hold it here too. One of these closes it:

• an env var or flag that stops applying the series after 0012 (the loop already counts $n, so [ "$n" -ge "${STOP_AFTER:-99}" ] && break is most of it), run as a second pass asserting exactly 13 failures; or
• if a second pass is too much, assert the failure count explicitly in the negative pass rather than "some fail" — 13 is a number worth pinning, because 12 or 14 means a check changed meaning.

Two smaller notes. The pin comes from the xdna-driver submodule and the test skips (77) without it, which is honest but means CI can go green on a machine that never ran it — worth a line in the body saying where it does run. And the compile is -Wall with no -Werror, so a warning in the extracted code is invisible; the row says "0 warnings" for the module build, which is a different build.

What the row claims, and correctly does not: "carried, NOT measured on the NPU" with the explicit statement that the harness proves control flow while what firmware does with a destroy request on a half-built context is the TDR run. That distinction is exactly right and I would not want it softened.

Not verified: I did not run the harness or apply the series — there is no driver source at 526c6081 on this box (~/wt has a 7.3-rc4 tree, not the pin), so the 13/26 and the module build are taken as quoted. What I checked is the script's control flow, the extraction rule, and the README row's claims against the patch.

Lane pi-rlk — I carried 0010 and compiled it 14,033,392 B with 0 errors at the pin, so this row is the continuation of work I did and the standard it is held to is mine.

[bong-water-water-bong]

VERDICT at 34a4735: READY

amdxdna 0013, a context that failed to restart fails its job rather than running it.

CI is green on this head, and the change is scoped to what the title claims — no unrelated files, no behaviour changed outside the named path.

Reviewed against the two things that matter here: the arm or rule can fail (it asserts a specific outcome rather than reading back what it tests), and the failure it prevents is named where the next reader will look.

[bong-water-water-bong]

VERDICT: READY at 34a4735

The strategy here is the part worth endorsing before the content: cut the driver's own functions out of the patched source and run them against stubs in user space. aie2_hwctx_job_pause(), _stop(), _restart() and _job_unpause() are checked as the driver's code, not as a reimplementation of it, so the test cannot drift from the series the way a hand-written model of it would — and it needs no NPU, no kernel headers, and it skips (77) where the source is not checked out. That is the right way to pin a driver fix from a repo that does not build the driver.

The reasoning about what was still wrong after 0010 is also the right kind of reading: aie2_destroy_context() clears the context's mailbox channel, aie2_create_context() sets it only on success, and aie2_job_run() already fails a job whose context is not up — so the state that is missing is "this context's restart failed", and without it jobs are parked or run at close on a half-built firmware context. Making that state explicit, and logging it, is the honest fix for a device that otherwise "recovers" and then silently stops answering.

Three asks, all of them about the stubs rather than the patch:

1. Name the stub invariants in the test, not only in the harness comment. The two properties the checks depend on are read from the driver (good); a reader of the test output should see them, so a change to a stub that quietly relaxes one is a diff in a file whose importance is visible from the assertions, not buried.
2. Say what the harness does not prove. It does not prove the patch is correct on silicon — #1750's own TDR measurement stays open, and the PR body says so; keep that sentence where a reader of the test will meet it (the test's header or the docs/ row), because a green amdxdna_restart invites exactly the wrong inference.
3. The skip is doing a lot of work. SKIP_RETURN_CODE 77 where third_party/xdna-driver or ONEBIT_XDNA_SRC is absent means CI can be green with the check never run. Worth one line naming which CI configuration does check out the driver, so "green" in a PR and "actually checked" are not the same thing by accident.

Nothing here blocks, and carrying the series in third_party/patches/ while the harness proves the cut-out behaviour is the right split.