jaguar3: EFUSE walk assumed ordered sections and quit early (8822C read rfe_type=0)

[snokvist]

The bug

HalJaguar3::read_efuse_logical_map stopped walking as soon as a section's
logical base passed the byte the caller asked for:

if (base > upto + 8)
  break; /* past the byte we need */

That is only valid if sections appear in ascending base order. They do not.

Physical EFUSE dumped off an RTL8822CU (0bda:c812), decoded by hand:

phys 0x00  hdr=0x00         -> base 0x000   ok
phys 0x09  hdr=0x10         -> base 0x008   ok
phys 0x12  hdr=0x0F ext=48  -> base 0x100   <- early exit fires here
phys 0x2C  hdr=0x4F ext=5D  -> base 0x150   never reached
phys 0xD5  hdr=0x4F ext=4E  -> base 0x110   never reached
phys 0xDA  hdr=0x4F ext=5E  -> base 0x150   never reached

The third section on the chip jumps to base 0x100, so any request below that
— including EEPROM_RFE_OPTION_8822C at logical 0xCA, which is the whole reason
read_efuse_rfe_type() calls this — ended the walk after three sections and
returned a map that was 0xFF almost everywhere.

Why it matters

On the affected adapter read_efuse_rfe_type() returned 0, while the vendor
kernel driver reads 0x03 from the same chip
(/proc/net/rtl88x2cu/<iface>/efuse_map, logical 0xCA). The RFE type gates BB /
RFE configuration, so those units were being brought up against an unprogrammed
default rather than their actual front-end.

It is silent: nothing errors, the map just reads unprogrammed.

The fix

Walk the whole programmed area (the existing 0xFF-header terminator and
kPhysMax bound already stop it). The upto parameter is removed rather than
left unused — a parameter that still looks like it bounds the walk is how this
comes back.

The 8822E branch is untouched: it never used upto, terminating on a long 0xFF
run instead, which is why only the C path was affected.

Hardware verification

adapter	before	after	kernel (efuse_map 0xCA)
RTL8822CU (0bda:c812, C8822C)	rfe_type=0x00	rfe_type=0x03	0x03
RTL8822EU (0bda:a81a, C8822E)	rfe_type=0x15	rfe_type=0x15	0x15

The EU is the regression check — unchanged, and its efuse decoded (0x22=46 0x4c=51 0xca=15) line is identical before and after. The 8822C EFUSE stability
probe also now reports a valid 0x8129 EEPROM ID.

Found while implementing #383 (EFUSE MAC as a per-unit identity), which could not
read the MAC on 8822C for this reason. With this fix that adapter's MAC decodes
correctly — 40:a5:ef:2f:23:08, matching its netdev exactly. The two changes are
independent; this one stands on its own regardless of what happens to #383.

[qodo-free-for-open-source-projects]

PR Summary by Qodo

Fix Jaguar3 EFUSE decode to handle unordered logical sections

🐞 Bug fix 🕐 10-20 Minutes

AI Description

• Decode the full programmed EFUSE area instead of stopping on assumed section order.
• Remove the misleading upto bound from the EFUSE logical-map walker.
• Fix 8822C RFE type reads that previously returned an unprogrammed default.

Diagram

graph TD
  A["EFUSE consumers"] --> B["read_efuse_logical_map()"] --> C["Variant byte reader"] --> D[("Physical EFUSE")]
  B --> E["Logical map / cache"] --> F["RFE type & TXPWR base"]

Loading

High-Level Assessment

Walking to the terminator/kPhysMax is the most robust approach because logical sections are not guaranteed to be ordered. Alternatives like retaining upto with heuristic early-exit conditions would remain correctness-risky (could still miss later sections that backfill lower logical offsets).

Files changed (2) +30 / -14

Bug fix (2) +30 / -14

HalJaguar3.cppRemove unordered-section early exit in EFUSE logical-map walk +26/-12

Remove unordered-section early exit in EFUSE logical-map walk

• Updates the EFUSE logical-map decoder to always walk through the programmed area instead of breaking when a section base exceeds a caller-provided 'upto' bound. Drops the 'upto' parameter and updates all internal call sites (probe, cache, RFE type, TX power base) accordingly, with expanded in-code rationale and a measured 8822CU example.

src/jaguar3/HalJaguar3.cpp

HalJaguar3.hUpdate EFUSE decoder signature and clarify no early-exit guarantee +4/-2

Update EFUSE decoder signature and clarify no early-exit guarantee

• Removes the 'upto' parameter from the private 'read_efuse_logical_map' declaration and documents that EFUSE sections are not ordered by logical base, so an early exit is unsound.

src/jaguar3/HalJaguar3.h

[qodo-free-for-open-source-projects]

Code Review by Qodo

🐞 Bugs (0) 📘 Rule violations (0) 📎 Requirement gaps (0) 🎨 UX issues (0) 🔗 Cross-repo conflicts (0) 📜 Skill insights (0)

1. EFUSE read past kPhysMax ✓ Resolved 🐞 Bug ☼ Reliability

Description

After removing the early-exit, read_efuse_logical_map can now reach the end of the physical EFUSE
and still attempt additional rd(phys++) reads for ext headers / data words without checking `phys
< kPhysMax. Because RtlAdapter::efuse_OneByteRead` masks the address to 10 bits, an out-of-range
read (>=1024) aliases to address 0, silently corrupting the decoded logical map.

Code

src/jaguar3/HalJaguar3.cpp[L707-708]

-    if (base > upto + 8)
-      break; /* past the byte we need */

Evidence

The EFUSE walk reads headers, optional ext headers, and data bytes by incrementing phys without
checking against kPhysMax after the loop-head condition, and efuse_OneByteRead explicitly masks
the address high bits, so reads beyond 1023 alias to low addresses instead of failing safely.

src/jaguar3/HalJaguar3.cpp[695-723]
src/jaguar3/HalJaguar3.cpp[649-655]
src/RtlAdapter.cpp[170-200]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
`HalJaguar3::read_efuse_logical_map` reads physical EFUSE bytes using `rd(phys++)` but does not guard against `phys` advancing beyond `kPhysMax` within a section (ext header + enabled data words). With this PR removing the early-exit, the function is more likely to walk to the physical end and hit this case.
On the non-EU path, `rd()` uses `RtlAdapter::efuse_OneByteRead`, which masks the address down to 10 bits. If `phys` reaches 1024, the effective address becomes 0 and the decode silently starts reading the beginning of EFUSE again.
## Issue Context
The outer loop condition only checks `phys < kPhysMax` at the top of the loop; it does not prevent `rd(phys++)` inside the loop from executing with `phys == kPhysMax`.
## Fix Focus Areas
- src/jaguar3/HalJaguar3.cpp[637-724]
- src/RtlAdapter.cpp[170-201]
### Suggested implementation direction
- Make `rd(a)` explicitly return `0xFF` without touching hardware when `a >= kPhysMax`.
- Additionally (or alternatively), add `if (phys >= kPhysMax) break;` guards before every `rd(phys++)` that occurs after the header read (ext header + each data byte), so a truncated final section can’t trigger an out-of-range read.

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

2. Stale upto documentation ✓ Resolved 🐞 Bug ⚙ Maintainability

Description

The comment above read_efuse_logical_map in HalJaguar3.h still describes an upto parameter even
though the method signature no longer takes it, which is misleading for maintainers and future
callers.

Code

src/jaguar3/HalJaguar3.h[R198-199]

+   * holding) offset `upto`. Backs read_efuse_rfe_type + read_efuse_txpwr_base.
+   * Walks the whole programmed area: sections are NOT ordered by logical base,

Evidence

The declaration has only (uint8_t *map, size_t len) while the comment still says it decodes up to
offset upto.

src/jaguar3/HalJaguar3.h[197-201]

Agent prompt

The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

## Issue description
The header comment still references an `upto` parameter that no longer exists in the method signature.
## Issue Context
The implementation and call sites were updated to remove `upto`, but the declaration’s comment was only partially updated.
## Fix Focus Areas
- src/jaguar3/HalJaguar3.h[197-201]
### Suggested implementation direction
- Remove the mention of `upto` from the comment and describe the new behavior (walks the whole programmed area; no early-exit due to unordered sections).

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools

---

_{To customize comments, go to the Qodo configuration screen, or learn more in the docs.}

[snokvist]

Both review findings addressed.

1. Read past kPhysMax — real, and this PR did make it reachable. The loop head checks phys < kPhysMax once per section, but a header + ext header + four enabled data words advance it up to ten bytes further, and efuse_OneByteRead masks the address to 10 bits — so a read at 1024 aliases to 0 and would decode the start of the EFUSE into whatever logical base the truncated section named. Guarded in the shared rd lambda, which covers both the 8822C and 8822E walks in one place:

if (a >= kPhysMax)
  return 0xFF;

0xFF is already what both walks treat as end-of-map/skip, so the truncated section terminates the walk exactly as an unprogrammed area does.

2. Stale upto in the header comment — my error; I appended to the comment instead of rewriting its first sentence. Reworded to describe the actual behaviour.

Re-verified on hardware after both changes, unchanged from the original results:

adapter	rfe_type	kernel efuse_map 0xCA
RTL8822CU (C8822C)	0x03	0x03
RTL8822EU (C8822E)	0x15	0x15

[josephnef]

Reviewed with the branch checked out; decode math verified against the dump, callers audited. The fix is correct and the hardware evidence is exactly the right shape — before/after on the affected 8822CU cross-validated against the vendor kernel efuse_map, with the 8822EU as an explicit no-change regression check. CI fully green.

Correctness — verified:

• The decode math checks out against the dump: hdr=0x0F ext=0x48 → offset 0x20 → base 0x100; hdr=0x4F ext=0x5D → 0x2A → base 0x150. Both match the hand-decode, so the unordered-sections claim is substantiated, not inferred.
• Removing upto instead of leaving it unused is the right call — a parameter that looks like a bound but isn't would reintroduce exactly this bug class.
• Smaller callers stay safe: read_efuse_rfe_type's 0x140-byte stack map vs the section at base 0x150 is handled by the idx < len guard on every data write.
• The kPhysMax guard in rd() is load-bearing, not belt-and-braces: efuse_OneByteRead masks to 10 bits, so a truncated straddling section would alias back to phys 0 and decode the start of the EFUSE into a bogus logical base. Previously the upto break made that unreachable; with the full walk it matters.
• 8822E untouched as claimed: the EU branch returns before the removed break, and neither of its callers depended on upto.

Performance: the 8822C non-cached paths now walk the full programmed area (per-byte USB control reads), but the walk still stops at the first 0xFF header and both callers are one-shot at bring-up/probe time — no meaningful cost.

One factual error in the shipped comment (inline, worth fixing before merge — it's the kind of number a future debugging session will trust) plus two comment-style nits inline.

[josephnef]

Factual slip: per the PR's own hardware table, the kernel value on the affected 8822CU is 0x03 — 0x15 is the 8822EU's (the regression-check adapter). The follow-up commit says "correct the stale doc" but this one survived it. Worth fixing: this exact number is what a future debugging session will trust.

Suggested change

	* the kernel driver read 0x15 from the same chip, i.e. the BB/RFE config was
	* the kernel driver read 0x03 from the same chip, i.e. the BB/RFE config was

[josephnef]

Splice leftover from the old "up to offset upto" text — "up to (and including the block holding) every programmed logical offset" is grammatically broken now. Simpler to just say it decodes the whole programmed area.

Also (pre-existing, but this PR grows the block 4×): this comment documents read_efuse_logical_map yet sits above probe_efuse_map. Consider moving it down to the function it describes while touching it.

[josephnef]

Style nit: the "It used to stop early…" framing is changelog-ish — git carries the history. The measured phys dump is the valuable part (it proves the unordered-sections invariant) and should stay; the framing could be present-tense: "sections are not in ascending base order (measured on an RTL8822CU: …), so the walk must not stop at any requested offset."