Strip canaries, double-reads, retries and floors from the module protocol; every bcm method holds the per-module lock for its whole operation; facts stay in docs with internal-client contention marked unresolved

2026-08-13 10:32:24 -07:00
parent 9d1f15045f
commit f2891886f3
3 changed files with 47 additions and 106 deletions
@@ -23,9 +23,9 @@ Protocol and full verified catalog: [bcm84891l-mdio-commands.md](bcm84891l-mdio-
 - **SET commands execute stale DATA** — the handler never clears DATA registers; write every parameter register explicitly before any SET.
 - **GETs must be invoked bare** — pre-writing *any* DATA register, not just the documented DATA1 display flag, leaves the handler executing as a no-op with results never written (sentinel pre-fills survived GET_SNR untouched, proven on hardware).
 - **STATUS must never be written** — any user value (0x0000 and 0x0008 both tried) closes the mailbox: the next command is silently ignored until firmware restores it.
- **PASS cannot prove completion.** The previous command's PASS stays latched while a slow firmware still executes, and until then the DATA registers are firmware scratch — the die temperature turns up in them (0x004E–0x0051 observed as "SNR" and "EEE mode"; the campaign's "anomalous 0x0047" was this same race). cabletest reads results twice and requires agreement (DATA1 excluded — temperature lands there autonomously), and discards SNR samples below 15 dB absolute, far under anything a trained link produces and exactly where every garbage signature sits.
+- **PASS cannot prove completion.** The previous command's PASS stays latched while a slow firmware still executes, and until then the DATA registers are firmware scratch — the die temperature turns up in them (0x004E–0x0051 observed as "SNR" and "EEE mode"; the campaign's "anomalous 0x0047" was this same race).
 - **Handler writes during firmware-busy windows wedge the µC permanently** — DATA/CMD writes landing while firmware does post-AN provisioning or training work (a window stretching ~10 s past relink) killed the SMI service four times; reads alone never once. Every handler write — SETs, GET commands, the ECD trigger — runs only in a quiet window: carrier up and stable ≥ 10 s (`phy.go`).
- **Bridge reads carry no fetch identity** — a fetch that outruns the delay leaves the previous transaction's data at 0xAD with no error. cabletest brackets read batches with a known-answer canary (PHY ID 1.2 = 0x3590) and discards the batch on mismatch (`phy.go`).
+- **Bridge reads carry no fetch identity** — a fetch that outruns the delay leaves the previous transaction's data at 0xAD with no error.
 - Poll STATUS ~100 ms; frozen up to 2 s during 10GBASE-T training — only run after link-up.

 ## Proven diagnostics
@@ -54,7 +54,7 @@ Protocol and full verified catalog: [bcm84891l-mdio-commands.md](bcm84891l-mdio-

 ## Firmware-reliability notes

-**The firmware is its own mailbox client — the handler is a shared, unarbitrated resource.** Watched read-only on an idle module: CMD sits at 0x0031 (`CMD_GET_CURRENT_TEMP`, bit 15 consumed) and DATA1 tracks the live die temperature (0x4F–0x52 ≈ 79–82 °C under load), refreshed every ~3.5 s — the firmware issues its own temperature command through the same CMD/STATUS/DATA registers the host uses, with no arbitration. Every mystery this explains: temperature appearing in DATA1 (it is that command's output — the campaign's "anomalous 0x0047" included), PASS satisfying a host poll when it belongs to the internal command, and the µC wedges (two writers colliding on CMD/DATA, likeliest when internal management activity spikes after AN events). Host defenses in `phy.go`: quiet-window writes, double-read results (DATA1 excluded), a CMD readback proving the command was not interleaved, the PHY-ID canary, and the SNR floor. Any GET whose answer lives in DATA1 is unusable; corroborate through IEEE registers or the wire. The hard rule that stands: write every DATA register explicitly before any SET — the handler executes stale DATA.
+**The firmware is its own mailbox client — the handler is a shared, unarbitrated resource.** Watched read-only on an idle module: CMD sits at 0x0031 (`CMD_GET_CURRENT_TEMP`, bit 15 consumed) and DATA1 tracks the live die temperature (0x4F–0x52 ≈ 79–82 °C under load), refreshed every ~3.5 s — the firmware issues its own temperature command through the same CMD/STATUS/DATA registers the host uses, with no arbitration. Every mystery this explains: temperature appearing in DATA1 (it is that command's output — the campaign's "anomalous 0x0047" included), PASS satisfying a host poll when it belongs to the internal command, and the µC wedges (two writers colliding on CMD/DATA, likeliest when internal management activity spikes after AN events). The host side keeps exactly one conversation open at a time (one per-module lock around every whole operation, `phy.go`) and confines writes to quiet windows; **contention with the internal client is otherwise unresolved** — a host GET can still be preempted (~10% per command at the temp cadence) and its results replaced. Any GET whose answer lives in DATA1 is unusable; corroborate through IEEE registers or the wire. The hard rule that stands: write every DATA register explicitly before any SET — the handler executes stale DATA.

 ## ECD — recovered from the OpenBCM SDK, proven on hardware

@@ -2,7 +2,7 @@

 ## Committed tree

-AF_PACKET raw sockets everywhere (`sock.go`); flow-director steering; per-packet-MAC-rx-stamped rate buckets (`SO_TIMESTAMPING` cmsg, `rx_filter=ALL` as a hard host check — nics/README.md for what that demands of the NIC; **temporarily bypassed** in `ts.go` so BCM work can run on the X520, which cannot stamp — the check reports yellow and the panel rates read zero there; restore to fatal for the product NIC); read-time-stamped NIC-counter rates; test interfaces pinned to MTU 9000 with a 9018-byte jumbo in the size mix (the modules' jumbo path is exercised, not assumed); BCM module diagnostics (`phy.go`, over the patched-ixgbe `sff_i2c` debugfs, compound-op framing; handler writes confined to quiet windows — the µC wedges otherwise — with results double-read, canary-validated, and floor-checked — modules/fs/ for the full trap list): bringup identifies both modules and forces EEE off and jumbo on every boot — no trustworthy readback exists and no cable is guaranteed to probe through; the ECD — per-pair verdicts, lengths and pair maps are the length/wiring path — runs through one async path at startup and on every reset, never blocking the UI, with counters re-baselining only after the diag's own link blip so it is never charged to the run; a 1 Hz poller feeds per-pair SNR margin (vs the ≈26.5 dB operating point; green ≥ 3 dB, amber ≥ 1 dB — provisional until the graded-noise run) and the corrected-error set (PCS 3.33 errored blocks/BER, PMA 1.147 fast-retrain count) to the panel and console; framebuffer UI; harness.
+AF_PACKET raw sockets everywhere (`sock.go`); flow-director steering; per-packet-MAC-rx-stamped rate buckets (`SO_TIMESTAMPING` cmsg, `rx_filter=ALL` as a hard host check — nics/README.md for what that demands of the NIC; **temporarily bypassed** in `ts.go` so BCM work can run on the X520, which cannot stamp — the check reports yellow and the panel rates read zero there; restore to fatal for the product NIC); read-time-stamped NIC-counter rates; test interfaces pinned to MTU 9000 with a 9018-byte jumbo in the size mix (the modules' jumbo path is exercised, not assumed); BCM module diagnostics (`phy.go`, over the patched-ixgbe `sff_i2c` debugfs, compound-op framing; one per-module lock around every whole operation, handler writes confined to quiet windows — the µC wedges otherwise; the firmware's own mailbox use still contends, unresolved — modules/fs/ for the full trap list): bringup identifies both modules and forces EEE off and jumbo on every boot — no trustworthy readback exists and no cable is guaranteed to probe through; the ECD — per-pair verdicts, lengths and pair maps are the length/wiring path — runs through one async path at startup and on every reset, never blocking the UI, with counters re-baselining only after the diag's own link blip so it is never charged to the run; a 1 Hz poller feeds per-pair SNR margin (vs the ≈26.5 dB operating point; green ≥ 3 dB, amber ≥ 1 dB — provisional until the graded-noise run) and the corrected-error set (PCS 3.33 errored blocks/BER, PMA 1.147 fast-retrain count) to the panel and console; framebuffer UI; harness.

 ## Stashes

@@ -67,8 +67,8 @@ type bcm struct {
 	ifname string
 	path   string

-	// Guards multi-op sequences only; single reads are already atomic on the
-	// wire through the compound op.
+	// Every method holds it for its whole logical operation: exactly one
+	// host-side conversation with the module at a time, by construction.
 	mu sync.Mutex
 }

@@ -191,27 +191,9 @@ func (b *bcm) waitStatus(want func(uint16) bool) (uint16, error) {
 	return 0, fmt.Errorf("%s: command handler stuck, status %#04x", b.ifname, st)
 }

-// The bridge gives no signal tying a response to its fetch, so a known-answer
-// read validates each batch: 0x3590 back from PHY ID 1.2 proves the bridge
-// was serving timely through the window; anything else discards the batch.
-func (b *bcm) canary() error {
-	v, err := b.mdioRead(1, 2)
-	if err != nil {
-		return err
-	}
-	if v != bcmPHYIDHi {
-		return fmt.Errorf("%s: bridge served %#04x for the PHY ID canary", b.ifname, v)
-	}
-	return nil
-}
-
-// GETs must be invoked bare: pre-writing any DATA register — not just the
-// documented DATA1 flag — leaves the handler executing as a no-op. SETs must
-// pass their full parameter set (the handler executes stale DATA) and get
-// settle time in place of unprovable completion. Results are read twice and
-// must agree — a stale PASS otherwise serves the handler's in-flight scratch
-// — with DATA1 excluded from the comparison (firmware writes temperature
-// there autonomously).
+// GETs must be invoked bare (pre-writing any DATA register leaves the handler
+// executing as a no-op); SETs must pass their full parameter set (the handler
+// executes stale DATA) and get settle time in place of unprovable completion.
 func (b *bcm) command(code uint16, params ...uint16) ([5]uint16, error) {
 	b.mu.Lock()
 	defer b.mu.Unlock()
@@ -242,69 +224,44 @@ func (b *bcm) command(code uint16, params ...uint16) ([5]uint16, error) {
 	if st == bcmStError {
 		return data, fmt.Errorf("%s: command %#04x returned ERROR", b.ifname, code)
 	}
-	// A SET's DATA registers are its parameters, scribbled over by firmware
-	// afterwards; there is nothing to read back.
 	if len(params) > 0 {
-		return data, b.canary()
+		return data, nil
 	}
 	for i := range data {
 		if data[i], err = b.mdioRead(bcmMMDVendor, bcmRegData1+uint16(i)); err != nil {
 			return data, err
 		}
 	}
-	for i := 1; i < len(data); i++ {
-		again, err := b.mdioRead(bcmMMDVendor, bcmRegData1+uint16(i))
-		if err != nil {
-			return data, err
-		}
-		if again != data[i] {
-			return data, fmt.Errorf("%s: command %#04x results unstable", b.ifname, code)
-		}
-	}
-	// The firmware is its own mailbox client (an internal GET_CURRENT_TEMP
-	// every few seconds); CMD still holding our code proves a GET's results
-	// are ours. Bit 15 is consumed on acceptance.
-	cmdv, err := b.mdioRead(bcmMMDVendor, bcmRegCmd)
-	if err != nil {
-		return data, err
-	}
-	if cmdv != code&^0x8000 {
-		return data, fmt.Errorf("%s: command %#04x preempted, CMD reads %#04x", b.ifname, code, cmdv)
-	}
-	if err := b.canary(); err != nil {
-		return data, err
-	}
 	return data, nil
 }

-// Retried against the known constant, long enough to outlast a µC left busy
-// by a dead process's in-flight diag or a link mid-training.
 func (b *bcm) identify() (string, error) {
-	var hi, lo uint16
-	var err error
-	for i := 0; i < 30; i++ {
-		if hi, err = b.mdioRead(1, 2); err != nil {
+	b.mu.Lock()
+	defer b.mu.Unlock()
+	hi, err := b.mdioRead(1, 2)
+	if err != nil {
 		return "", err
 	}
-		if lo, err = b.mdioRead(1, 3); err != nil {
+	lo, err := b.mdioRead(1, 3)
+	if err != nil {
 		return "", err
 	}
-		if hi == bcmPHYIDHi && lo == bcmPHYIDLo {
+	if hi != bcmPHYIDHi || lo != bcmPHYIDLo {
+		return "", fmt.Errorf("%s: PHY ID %#04x:%#04x, want %#04x:%#04x",
+			b.ifname, hi, lo, bcmPHYIDHi, bcmPHYIDLo)
+	}
 	sn, err := b.eeprom(68, 16)
 	if err != nil {
 		return "", err
 	}
 	return "BCM84891L sn " + strings.TrimSpace(string(sn)), nil
-		}
-		time.Sleep(500 * time.Millisecond)
-	}
-	return "", fmt.Errorf("%s: PHY ID %#04x:%#04x, want %#04x:%#04x",
-		b.ifname, hi, lo, bcmPHYIDHi, bcmPHYIDLo)
 }

 // PMA 1.1 latches low, so the first read reports any drop since it was last
 // read and the second reports the wire as it is now.
 func (b *bcm) linkUp() (bool, error) {
+	b.mu.Lock()
+	defer b.mu.Unlock()
 	if _, err := b.mdioRead(1, 1); err != nil {
 		return false, err
 	}
@@ -326,6 +283,8 @@ func (b *bcm) forceJumbo() error {
 }

 func (b *bcm) restartAN() error {
+	b.mu.Lock()
+	defer b.mu.Unlock()
 	v, err := b.mdioRead(7, 0)
 	if err != nil {
 		return err
@@ -334,6 +293,8 @@ func (b *bcm) restartAN() error {
 }

 func (b *bcm) eeeAdvert() (uint16, error) {
+	b.mu.Lock()
+	defer b.mu.Unlock()
 	return b.mdioRead(7, 60)
 }

@@ -358,6 +319,8 @@ func (b *bcm) snr() ([4]float64, error) {
 }

 func (b *bcm) pcsLatch() (blocks, ber uint64, err error) {
+	b.mu.Lock()
+	defer b.mu.Unlock()
 	v, err := b.mdioRead(3, 33)
 	if err != nil {
 		return 0, 0, err
@@ -366,6 +329,8 @@ func (b *bcm) pcsLatch() (blocks, ber uint64, err error) {
 }

 func (b *bcm) fastRetrainCount() (uint16, error) {
+	b.mu.Lock()
+	defer b.mu.Unlock()
 	v, err := b.mdioRead(1, 147)
 	if err != nil {
 		return 0, err
@@ -404,13 +369,6 @@ func (b *bcm) cableDiag() (ecdResult, error) {
 		}
 		time.Sleep(ecdPoll)
 	}
-	// The run's own blip leaves the µC busy training, so a failed canary here
-	// means try the latched results again shortly, not give up.
-	var canaryErr error
-	for try := 0; try < 20; try++ {
-		if try > 0 {
-			time.Sleep(500 * time.Millisecond)
-		}
 	v, err := b.mdioRead(1, bcmRegECDResult)
 	if err != nil {
 		return res, err
@@ -423,11 +381,7 @@ func (b *bcm) cableDiag() (ecdResult, error) {
 		}
 		res.metres[i] = int(m)
 	}
-		if canaryErr = b.canary(); canaryErr == nil {
 	return res, nil
-		}
-	}
-	return res, canaryErr
 }

 const (
@@ -477,9 +431,6 @@ func (m *phyModule) poll() error {
 	}
 	quiet := !m.upSince.IsZero() && time.Since(m.upSince) >= bcmQuiet

-	if err := m.bcm.canary(); err != nil {
-		return err
-	}
 	link, err := m.bcm.linkUp()
 	if err != nil {
 		return err
@@ -490,13 +441,6 @@ func (m *phyModule) poll() error {
 		if snr, err = m.bcm.snr(); err != nil {
 			return err
 		}
-		// The proven garbage signatures — die temperature, handler status —
-		// all sit far below any SNR a trained link can have.
-		for _, s := range snr {
-			if s < 15 {
-				return fmt.Errorf("%s: implausible SNR %.1f discarded", m.bcm.ifname, s)
-			}
-		}
 	}
 	blocks, ber, err := m.bcm.pcsLatch()
 	if err != nil {
@@ -506,9 +450,6 @@ func (m *phyModule) poll() error {
 	if err != nil {
 		return err
 	}
-	if err := m.bcm.canary(); err != nil {
-		return err
-	}

 	m.mu.Lock()
 	m.sampled = true