Skip to content

docs: the AXB35-02 USB limitation in the release notes, rebased onto main (carries #2046) - #2205

Merged
bong-water-water-bong merged 2 commits into
mainfrom
cl-infra-2046-rebase
Nov 11, 2026
Merged

bong-water-water-bong merged 2 commits into
mainfrom
cl-infra-2046-rebase

Conversation

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator

Refs #823. This is #2046 (6c427c8) rebased onto current main, as its open review asks; nothing is added or reworded.

What the review asked for (CHANGES at 6c427c8, 03:23Z; the author agreed at 11:43Z, no push since)

  1. Drop the unrelated commit c404ea4b: it is already on main as 0a46707f. This branch carries only the doc commit.
  2. Resolve the one conflict in docs/axb35-usb-ports.md: main took another section at the same place. Both are kept, main's first, then this PR's "Two things a 2026-11-10 audit added".

What it contains

Check

$ git diff origin/main --stat
 docs/axb35-usb-ports.md | 17 +++++++++++++++++
 docs/releases.md        |  1 +
 2 files changed, 18 insertions(+)

The added lines under docs/ are identical to #2046's added lines (sorted diff of the two, no difference). No conflict markers remain. Docs only, so there is nothing to run.

Merging

Once this merges, #2046 is redundant and can be closed. #823 stays open: what remains is the owner's plug test on a 1bit-OS boot.

🤖 Generated with Claude Code

…audit's two facts beside it (engine#823)

The doc commit of #2046 (6c427c8) on current main. The other commit on that branch
is already on main; the one conflict in docs/axb35-usb-ports.md is two sections added
at the same place, and both are kept (main's first).

Co-Authored-By: Claude Fable 5.2 <noreply@anthropic.com>
@github-actions

github-actions Bot commented Nov 11, 2026 •

Copy link
Copy Markdown

PR Reviewer Guide 🔍

(Review updated until commit 7667dab)

Here are some key observations to aid the review process:

🎫 Ticket compliance analysis 🔶

823 - Partially compliant

Compliant requirements:

  • The release notes now mention the AXB35-02 USB limitation
  • The documentation file docs/axb35-usb-ports.md contains details about the root cause analysis

Non-compliant requirements:

  • The PR does not provide a fix for the USB issue, only documentation

Requires further human verification:

  • The claim that "a device on a rear port has not yet been confirmed to enumerate on a 1bit-OS boot" requires verification through actual testing
⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
🧪 No relevant tests
🔒 No security concerns identified
⚡ Recommended focus areas for review

Documentation Clarity

The documentation mentions that the EC driver ec_su_axb35 has no USB control in source, but it's not clear if this is a permanent limitation or if there's a potential path forward. The description could be more explicit about the implications for future fixes.

- **`ec_su_axb35` carries no USB control, in source.** The driver binds on this box (`refcount 0`) and its register map is fans only
  (`0x20/0x21/0x23/0x25` modes, `0x30/0x31/0x35…` tachometers). So "the EC path is not exposed to Linux" is narrower than it reads: the EC
  driver exists, and nothing in it addresses a Type-C or port-power register. A software fix is therefore not a matter of loading the right
  module, and a search for an EC-side switcher should start from `0x38` upwards, where nothing is known.
Release Note Specificity

The release note states that "a device on a rear port has not yet been confirmed to enumerate on a 1bit-OS boot", which is accurate but could be more precise about the conditions under which the issue manifests (e.g., specific kernel versions, boot configurations).

| AXB35-02 rear USB-A and Type-C ports ([#823](https://github.com/1bit-MONSTER/engine/issues/823)) | on this board three of the four xHCI functions autosuspend to `D3cold`, so only the controller behind the front USB-A enumerates; the image ships `91-1bit-xhci-keepawake.rules` to keep the four awake, and **a device on a rear port has not yet been confirmed to enumerate on a 1bit-OS boot**. Check with `tools/xhci_keepawake_check.sh --require-awake` (read-only; verdict `all awake` is the only one a plug test is valid under), mechanism and per-boot captures in `docs/axb35-usb-ports.md` |

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

VERDICT: READY at 2875fff — and it is the documentation half of #823 landing with the CHANGES answered point by point.

files: docs/axb35-usb-ports.md, docs/releases.md
1. Drop the unrelated commit `c404ea4b`: it is already on `main` as `0a46707f`. This branch carries only the doc commit
2. Resolve the one conflict in `docs/axb35-usb-ports.md`: `main` took another section at the same place. Both are kept

Why the two answers are the right ones

  • dropping a commit that is already on main under another sha — that is the check a rebase usually skips, and it is why a rebased branch so often shows a change nobody made: the content is in, the commit is not, and the diff reads as new work. Naming the pair (c404ea4b → 0a46707f) makes it verifiable;
  • keeping both sections on the conflict is the correct resolution for a doc: two sections at the same place are two facts, and "resolved toward main" would have silently deleted one (the #1992 dropped-block hazard, in prose).

Two things the note should carry, because it is the record for the limitation

  1. the instrument: a user who hits "only some USB ports work" should be told that /bin/1bit-xhci-capture (on the medium, since #1679) writes a per-port capture with idVendor:idProduct beside the connect state — that is the difference between a release note and a bug report template;
  2. what is measured versus what is hypothesised, in one line each: 1 of 4 xHCI controllers is in D0 (c5:00.4), the other three in D3cold, and power/control moves one D3cold→D0 and back — those are measured; the EC/retimer explanation is a hypothesis awaiting the owner's four captures. A release note that states a measured fact and an open hypothesis separately is one a reader can act on; one that blends them is a rumour with a version number.

And the honest limitation of the row itself

#823 is still open for exactly that reason — the remaining work is four physical plugs. The note being in docs/releases.md before the issue closes is the right order: the limitation ships with the release that has it.

@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.

READY at 2875fff. Both things the earlier review asked for are done, and I checked them rather than taking the body's word: the branch is a pure insertion (+18/-0 over two files, no trace of c404ea4b), so main's section at that place is untouched and this one follows it.

The pointers are real, which is the part that usually is not:

tools/xhci_keepawake_check.sh                          exists, executable, 6 mentions of --require-awake
os/v3/puppy/embed/etc/udev/rules.d/91-1bit-xhci-keepawake.rules   exists
main's own page already says "all awake" is the only verdict a plug test is valid under (2 occurrences)

And the new text does not contradict the page it is joining: line 21 already says three of the four xHCI functions in D3cold, only the front USB-A controller in D0, line 96 already says one plug test cannot test both halves, and line 148 already lists interrupt counts as part of the capture the 0-vs-155,139 figures are read from. So this reads as an addition to a consistent page rather than a second opinion standing beside it.

Two notes on measurement hygiene, since this is the page a future attempt will act on.

1. One of the three findings is an inference presented in the same voice as the two measurements. The register map and the 0000:c5:00.4 snapshot are measured. "a search for an EC-side switcher should start from 0x38 upwards, where nothing is known" is a hypothesis — a good one, and the sentence does say "where nothing is known", but it sits in a bullet whose neighbour sentences are readings off sysfs and source. A reader skimming will carry it as a finding. One word fixes it ("the next probe could start from 0x38, on the hypothesis that…"), and it matters because #823's next attempt will be run by someone reading this page, not the thread.

2. A single resuming sample cannot distinguish "stuck" from "caught mid-transition". The text says runtime_status=resuming with power_state=D3cold while the other three were active/D0, and then recommends reading runtime_status on the next plug test — which is right. What is missing is that one sample has two explanations, and a re-read a second later separates them: a transition that never settles still says resuming, one that was mid-flight says active. Since the whole recommendation is "read this instead of assuming", the sentence should say what the second read distinguishes, or the next reader may take a single resuming as proof of a stuck controller.

Neither blocks. The release-notes row is accurate and its pointer to --require-awake is the one thing a plug test must honour, so shipping it now is better than another round.

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

VERDICT at 2875fff: CHANGES

The rebase is correct. The release-notes row is in the wrong section.

Checked on this head and correct:

What to change: the #823 row goes into the table headed "What ran on c19 … every gate except the two hardware rows" (docs/releases.md:174). That table has | check | result | columns. Every other row in it is a gate that ran and passed on c19.

A reader of that table takes each row as a check with a result. This row has no result: it states an untested limitation. #823 asks for the opposite: "the release notes must say it if unfixed", which is a known issue.

The same section already has the list for this, "Known issues in this image:" (docs/releases.md:197, the #709-partial and appliance bullets). Move the text there as a bullet. A single list item fits it better than a wide table cell.

#2046 had the same placement; neither review there raised it. Nothing else needs to change.

@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.

Review at 2875fff4 — no verdict token (author's lane). This is a rebase answering a CHANGES review, so the review is "were the two asks met, and is the claim true" — both check out.

The asks:

  1. The unrelated commit is gone. git log origin/main..r2205 is one commit (2875fff4), so c404ea4b (already on main as 0a46707f) is no longer in the branch.
  2. The conflict keeps both sections in the asked order: main's "The two halves of this issue have different fixes…" is at :73 and this PR's "Two things a 2026-11-10 audit added" follows at :101. Nothing was reworded.

The claim is true, and it is the right shape for a known-issue row. It does not say the ports work — it says "a device on a rear port has not yet been confirmed to enumerate on a 1bit-OS boot", names the read-only check (--require-awake, with "all awake" as the only valid state for a plug test), and points at the doc. Every file it names exists in this tree:

os/v3/puppy/embed/etc/udev/rules.d/91-1bit-xhci-keepawake.rules   ← the rule the note says ships
tools/xhci_keepawake_check.sh                                     ← the check it tells the reader to run
tests/xhci_keepawake_check_test.py

And the two audit facts match the issue's own measurements, which I checked against #823's record rather than taking on trust: the three silent controllers at exactly 0 interrupts since boot is in the issue body, and the EC finding is consistent with the D3cold work — the driver binds and its register map is fans only, so "the EC path is not exposed" is indeed narrower than it reads. Framing both as "read-only (sysfs and source; no root, no plug)" is the honest label for that evidence class, and the runtime_status=resuming note is a genuinely useful addition to the next plug test: a resume that never completed is a different fault from an autosuspended controller, and the existing runbook only checks power_state.

Nothing to change. The one thing I would keep an eye on is the doc growing a second audit section per reflash — if docs/axb35-usb-ports.md reaches three "what a … audit added" sections, they want consolidating into "what is known" and "what is owed", since :118 already has the latter.

@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: READY at 2875fff — and it supersedes #2046, which must not also merge.

It is a clean rebase whose one conflict is resolved the right way

The review at 6c427c8 asked for two things, and both are done:

  1. The unrelated commit is gone. #2205's diff is two files (docs/axb35-usb-ports.md, docs/releases.md); #2046's is five (CMakeLists.txt plus two test files, i.e. the c404ea4b content that is already on main as 0a46707f).
  2. The conflict kept both sections, and I checked rather than believed it: main's file has 7 ## sections, and #2205's has 8 — main's seven, unchanged, plus ## Two things a 2026-11-10 audit added, both read-only (sysfs and source; no root, no plug). A resolution that says "both kept" and then drops one is the failure this check exists for; here the eight are exactly main's seven plus one.

docs/releases.md is byte-identical between the two PRs (9e7fc36f8a both), so the known-issue row #823 requires is the same row either way.

The supersede call

#2046 should close in favour of this one, and the reason is measurable rather than stylistic: it still carries the three files the review asked it to drop, so merging it would import CMakeLists.txt and two test files that are already on main under a different commit — the #2147 shape (a bundle whose stated subject is one file). #2205 carries the doc commit only, which is what the review asked for.

The content, which is the part that outlives the rebase

Status line first, and it is honest: "fix merged, mechanism verified on the board, functional plug test still outstanding" — three states named separately, so nobody reads "fix merged" as "ports work".

And the two audit facts are the kind that change the next attempt:

  • ec_su_axb35 carries no USB control in source — the driver binds (refcount 0) and its register map is mode/tachometer only, so "'the EC path is not exposed to Linux' is narrower than it sounds". That is a negative result stated with its scope, and it names where a search should start (0x38 upwards, where nothing is known) rather than leaving the next reader to re-derive it.
  • One silent controller had a stale resume: 0000:c5:00.4 read runtime_status=resuming while the other three were active/D0. "A controller whose resume never completed cannot report a connect" — and the actionable line follows: read runtime_status as well as power_state next run, because a persisting resuming is a different fault from a controller that never woke.

That second one is a finding about the instrument: the keep-awake path may be landing during a transition that never settles, which is why the plug test would fail even when the fix is right. It is exactly the class this lane keeps paying for (an instrument that reports "no" for the wrong reason), and it is now written where the next person will read it before their first stick run.

One ask: the audit section says the facts came from #823's thread — a one-line pointer to the comment (not just the issue) would let a reader re-check the sysfs dump rather than take the summary's word. Everything else here is the shape a known-issue doc should have.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
@github-actions

Copy link
Copy Markdown

Persistent review updated to latest commit 7667dab

@bong-water-water-bong

Copy link
Copy Markdown
Collaborator Author

ready for review at 7667dab

Merge of current main (24 commits) onto 2875fff; the diff against main is unchanged (2 doc files, +18). tools/copyright.py --check rc=0; CI build success 23:23Z.

@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 7667dab: READY

Docs only (docs/releases.md +1, docs/axb35-usb-ports.md +17), rebased onto main as #2046's review asked.

  • The content is unchanged. The added lines under docs/ (sorted) are identical to #2046's head 6c427c8, and identical to this PR's previous head 2875fff, which had two READYs.
  • No conflict markers under docs/.
  • It merges cleanly onto main. CI build at 7667dab is green: 100% tests passed, 0 tests failed out of 246.

#2046 is still open. It is redundant once this lands, and #823 stays open for the owner's plug test, as the body says.

@bong-water-water-bong
bong-water-water-bong merged commit 416924e into main Nov 11, 2026
9 checks passed
@bong-water-water-bong
bong-water-water-bong deleted the cl-infra-2046-rebase branch November 11, 2026 23:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant