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

Jaguar3: fix EFUSE logical-map walk for unordered sections (8822C RFE type)

🐞 Bug fix 📝 Documentation 🕐 20-40 Minutes

AI Description

• Decode the full programmed EFUSE area to handle sections out of logical-base order.
• Remove the misleading upto parameter and update all call sites to the new API.
• Guard physical reads past kPhysMax to avoid 10-bit address wraparound aliasing.

Diagram

graph TD
  A["read_efuse_rfe_type"] --> D["read_efuse_logical_map"] --> E["bounded rd()"] --> F[("Physical EFUSE")]
  B["read_efuse_txpwr_base_8822e"] --> D --> E --> F
  C["probe_efuse_map / cache_efuse_8822e"] --> D --> E --> F

Loading

High-Level Assessment

The following are alternative approaches to this PR:

1. Two-pass scan to enable safe early-exit

• ➕ Could reduce work if callers only need a small logical range
• ➕ Makes early termination explicit (based on complete section inventory)
• ➖ More complex (must first parse headers/lengths, then selectively decode)
• ➖ EFUSE size is small (1KB), so savings are typically negligible
• ➖ Higher risk of reintroducing subtle termination bugs

2. Build and reuse a per-section index/cache

• ➕ Amortizes decode cost across multiple reads
• ➕ Can support selective logical reads once indexed
• ➖ Adds cache invalidation/lifecycle complexity
• ➖ Not needed unless EFUSE decode is a proven hotspot
• ➖ Increases statefulness in a correctness-critical path

Recommendation: Keep the PR’s approach: always decode the whole programmed EFUSE area and remove the misleading upto parameter. Given unordered section bases and the small physical EFUSE size (1KB), the full-walk strategy is the simplest and most robust way to avoid silent under-decoding; the added kPhysMax guard further hardens correctness against address wraparound.

Files changed (2) +40 / -16

Bug fix (1) +34 / -13

HalJaguar3.cppFix EFUSE walk termination; add kPhysMax guard; update call sites +34/-13

Fix EFUSE walk termination; add kPhysMax guard; update call sites

• Removes the 'upto'-based early-break from the 8822C EFUSE packed-map walk so decoding doesn’t assume sections appear in ascending logical-base order. Adds a physical-address bounds guard in the byte reader to prevent 10-bit address masking from wrapping reads past 'kPhysMax' back to 0. Updates all internal callers to use the new 'read_efuse_logical_map(map, len)' signature and relocates/updates the explanatory comment onto the function.

src/jaguar3/HalJaguar3.cpp

Documentation (1) +6 / -3

HalJaguar3.hUpdate EFUSE decode API and document unordered-section invariant +6/-3

Update EFUSE decode API and document unordered-section invariant

• Updates the private 'read_efuse_logical_map' declaration to drop the 'upto' parameter and documents that the EFUSE walk must cover the whole programmed area because section bases are not ordered. Keeps the interface aligned with the corrected implementation in the .cpp.

src/jaguar3/HalJaguar3.h

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

Code Review by Qodo

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

1. Missing lambda capture 🐞 Bug ≡ Correctness

Description

read_efuse_logical_map()’s rd lambda references the function-local kPhysMax but does not
capture it ([this, eu]), which is ill-formed in standard C++ and can break the build. This was
introduced by the new if (a >= kPhysMax) guard inside the lambda.

Code

src/jaguar3/HalJaguar3.cpp[R655-656]

+    if (a >= kPhysMax)
+      return 0xFF;

Evidence

The code defines kPhysMax in function scope and then uses it inside a lambda that does not capture
it; this is a direct, local inconsistency that can prevent compilation of this translation unit.

src/jaguar3/HalJaguar3.cpp[635-661]

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()` defines `kPhysMax` as a function-local `constexpr` and then uses it inside the `rd` lambda (`if (a >= kPhysMax)`), but the lambda capture list is `[this, eu]`. In standard C++, referencing an automatic local variable inside a lambda requires capturing it, so this can fail compilation.
### Issue Context
This guard was added to prevent `efuse_OneByteRead` address-masking aliasing when reads go past the physical EFUSE bound.
### Fix Focus Areas
- src/jaguar3/HalJaguar3.cpp[635-661]
### Suggested fix
Update the lambda capture list to include `kPhysMax` (e.g. `auto rd = [this, eu, kPhysMax](uint16_t a) -> uint8_t { ... }`), or move `kPhysMax` to a namespace/class-scope constant so it’s not a function-local capture concern.

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

---

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

---

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

[snokvist]

Thanks for the review — all three comments are addressed in 38790b1 (comment-only; build clean, 49/49 ctest):

• the 0x15 slip → now 0x03, the affected 8822CU's value. You were right that it is the kind of number a later session trusts; 0x15 was the 8822EU regression-check value in the wrong sentence. The same slip was in the PR body of Expose the EFUSE MAC as a per-unit adapter identity #383 and is fixed there too.
• the splice + placement → the block no longer tries to say "up to … every offset"; it just says it decodes the whole programmed area, and it moved down onto read_efuse_logical_map. probe_efuse_map keeps its own 8822C-only note.
• present tense → the invariant with the measured dump as evidence; the "it used to" framing is gone.

Independent cross-check against the vendor parser

Since the argument for removing upto is "sections are not ordered", it is worth having the vendor's own walk on record. eeprom_parser_88xx() (rtl88x2cu 20230728, hal/halmac/halmac_88xx/halmac_efuse_88xx.c:1139) is a do { … } while (1) with no logical-offset bound at all — it terminates only on hdr == 0xff (or hdr2 == 0xff) and on physical-size guards. So the post-fix walk is the vendor's, and the pre-fix upto exit was devourer-local.

Two things fall out of that:

• The kPhysMax guard is the right shape — the vendor bounds-checks efuse_idx after every advance (header, ext, and each of the 8 data bytes), and is stricter still: it fails the whole parse rather than continuing. Returning 0xFF from rd() is the gentler equivalent.
• The header decode matches bit-for-bit: vendor blk_idx = ((hdr2 & 0xF0) >> 1) | ((hdr >> 5) & 0x07) vs devourer ((ext & 0xF0) >> 1) | ((hdr & 0xE0) >> 5), so the hand-decode in the comment (0x0F/0x48 → 0x100, 0x4F/0x5D → 0x150) is the vendor's arithmetic, not a re-derivation.

It also answers the "previously-read bytes moving" half of #385: the vendor writes log_map[eeprom_idx] = value8 unconditionally as it walks, so a later section overriding an earlier one is the designed re-burn semantics — and the measured unit does carry two sections at base 0x150 (phys 0x2C and 0xDA).

#385's validation asks

"check that read_efuse_txpwr_base output changes are the intended ones" — there is nothing to check, and that is verifiable rather than hopeful. read_efuse_txpwr_base_8822e() returns early unless _variant == C8822E, and on 8822E the walk takes the EU branch, which returns before the removed exit ever ran. So that reader's output is bit-identical on both variants. The complete consumer set of the changed (non-EU) walk is read_efuse_rfe_type, probe_efuse_map, and #383's MAC read. probe_efuse_map's compare surface does grow — it now sees the whole map instead of the first three sections — which strictly improves what the stability probe is checking.

The RFE blast radius, measured offline. I walked all five 8822C tables twice through PhyTableLoaderJaguar3::Load with rfe_type = 0 and = 3 and diffed the register-write streams:

table	writes rfe0 / rfe3	distinct addrs	changed	added/removed
phy_reg	1289 / 1289	923 / 923	0	0
agc_tab	450 / 450	2 / 2	0	0
radioa	789 / 789	55 / 55	5	0
radiob	697 / 697	37 / 37	2	0
cal_init	2464 / 2464	44 / 44	0	0

Identical counts and identical address sets everywhere — no row appears or disappears. The entire delta is 7 RF register values:

radioa  0x52: 0x000942ca -> 0x000902ca
        0x63: 0x00000c02 -> 0x00000002
        0xb3: 0x0007c760 -> 0x000fc760
        0xb6: 0x000187f8 -> 0x000387f8
        0xdd: 0x00000500 -> 0x00000540
radiob  0x52: 0x000942c0 -> 0x000902ca
        0x63: 0x00000c02 -> 0x00000002

Cut-independent (identical for cut_version 0–3). BB, AGC and RFK cal-init are untouched, so the on-air surface is RF front-end path config only — which is exactly what an RFE type is supposed to select. Happy to land the harness as a ctest if you want it as a standing check; it needs no hardware.

On-air A/B — I don't have an SDR here, so I can't produce the per-rate measurement you asked for. What I can run on the affected 8822CU is a link-level A/B: fixed TX power and channel, N frames per MCS, before/after, comparing per-MCS PDR and receiver RSSI. Say the word and I'll post it. Worth noting the direction of travel independently of any measurement: 0x03 is what the vendor driver programs on that same unit, so the fix moves devourer's RF config onto the kernel's rather than away from it — today those units run the rfe_type=0 column by accident.

Interaction with #383

They conflict, in both directions — GitHub reports each as mergeable against master because it only tests them pairwise with the base. #383 calls read_efuse_logical_map(map, sizeof(map), kMacLogicalOff + 6) in two places, and this PR removes that parameter, so whichever lands second needs the arg dropped at both call sites (cache_efuse_8822e and perm_mac) — a 2-token change, nothing semantic: with upto gone the 8822C on-demand walk simply becomes the full walk it wanted.

I merged both onto master locally and resolved it that way: build clean, 49/49. Merge them in whatever order suits you and I'll push the fixup to the trailing one immediately — or say the word and I'll rebase #383 onto this branch so it lands conflict-free.

[josephnef]

Approving with the on-air arm #385 asked for, run on this rig's affected RTL8822CU (the second of the 2/2 units that reproduce the truncated walk).

On-air TX A/B (master vs this fix, txdemo MCS7/20 flood, SDR duty)

band	master (rfe_type=0)	fixed (rfe_type=0x03)
ch36 (5 GHz)	49.2% duty (~32.0 Mbps on-air)	48.7% (~31.7 Mbps) — tie
ch6 (2.4 GHz)	78.2–81.8% over 4 interleaved reps	92.5–94.2%

The 2.4 GHz gap is reproducible (interleaved A/B ×3, per-arm sd < 1 point, far above this rig's ±3-point single-probe floor): the correct front-end rows are worth ~+12 points of duty at MCS7/ch6 on this unit — master was flying the unprogrammed-default tables. 5 GHz is unchanged.

RX + identity regression checks

• RX smoke A/B on the same unit, ch6 ambient: 345 vs 408 clean frames / 8 s, 0 corrupt both arms, HEALTHY verdicts — the table change does not deafen RX.
• 8822CU efuse: stability probe id 0x8129 valid, rfe_type=0x03 (kernel agrees), and the #383 MAC decodes to a8:b5:8e:6a:94:ea — the exact value the #385 worktree experiment predicted on this unit.
• 8822EU (98:03:cf:cf:a4:49), 8814AU (20:0d:b0:c7:e4:b3): unchanged, stable across runs.

The kPhysMax guard against the 10-bit address aliasing is a good catch — a truncated tail section would otherwise silently decode the start of the EFUSE into a wrong logical base.

CI note: the pushes landed during today's GitHub Actions incident and the pull_request events were dropped (no runs existed for either head SHA); both PRs were close/reopened to re-trigger. Merge order: this one first, then #383 rides clean.

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

Code review by qodo was updated up to the latest commit 38790b1