Repository navigation
amdxdna 0010: the README row stops claiming a clean failure it does not deliver (fixes #1855) - #1924
Conversation
…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
left a comment
There was a problem hiding this comment.
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 callsaie2_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.
|
VERDICT: READY at The diff is
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. |
|
VERDICT: CLOSE at Measured, not eyeballed: So the correction is real but has already landed — the PR differs from the base it branched from and is identical to and 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 The correction itself is the right one and it is on main: staying paused is necessary for a clean failure, not sufficient, because |
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
0010movesaie2_hwctx_job_unpause()off the sharedout:label so a context whose restart failed is no longer unpaused and drained onto afw_ctx_idfirmware no longer has. That is correct and worth carrying. What the row claimed on top of it — "a failed context stays paused, soits jobs get a clean, attributable failure instead of a wedge" — is not what happens:
grep -nE "aie2_hwctx_fini|job_unpause" third_party/patches/amdxdna/0010-*.patchshows theremoved line and its moved replacement, and
aie2_hwctx_finiis never named);aie2_hwctx_fini()still callsaie2_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
finito fail instead of run is driver behaviour and needs the kernel tree — not this lane's call, and notsomething a README edit can honestly imply.
What this does not touch
No patch bytes, no code, no series order: the
0010diff is unchanged, so it still applies on top of0001–0009and still conflicts on thebare 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.