From e99c7121e7b3ec70a13ec34963d27c1fb7f58ea4 Mon Sep 17 00:00:00 2001 From: flamingcow Date: Thu, 13 Aug 2026 08:26:22 -0700 Subject: [PATCH] Bridged MDIO reads verify freshness: a late async fetch serves the previous response as the wrong register (observed STATUS 0x0004 as 0.4dB SNR); unchanged-or-zero results re-read bare without re-arming until stable, first read always verified, identify retries against the constant PHY ID --- docs/modules/fs/README.md | 1 + docs/state.md | 2 +- phy.go | 97 +++++++++++++++++++++++++++++---------- 3 files changed, 74 insertions(+), 26 deletions(-) diff --git a/docs/modules/fs/README.md b/docs/modules/fs/README.md index a4c8591..1ee01b0 100644 --- a/docs/modules/fs/README.md +++ b/docs/modules/fs/README.md @@ -14,6 +14,7 @@ PHY at I2C 8-bit **0xAC write / 0xAD read**. | Clause-45 read | I2C write to 0xAC: `[001+DevAD, RegH, RegL]` → **delay >1 ms** (3 ms proven) → I2C read 2 B from 0xAD | - Single-byte reads see the bridge as inert (returns 0) — the DevAD-prefixed frame + delay is mandatory. This is why early probing wrongly declared 0x56 dead. Treat 0x0000 reads as retry-with-longer-delay: 0 is also the bridge's not-ready signature. +- **The fetch is asynchronous and a late one serves the wrong register silently**: the read frame arms a fetch, and if it outruns the delay, reading 0xAD returns the *previous* transaction's response with no error — observed under load even at 3 ms (STATUS 0x0004 served as an SNR DATA read → 0.4 dB). Since the buffer only changes when a fetch lands, a result differing from the previous buffer value is provably fresh; an unchanged (or zero) result must be re-read **bare, without re-arming** — preserving clear-on-read registers — until it stabilizes. cabletest implements this freshness check on every bridged read (`phy.go`). - Recorded for a future multi-byte firmware transport (mlx5 MCIA shape): read = write `[001+devad,RegH,RegL]` offset_size=1, STOP, >1 ms, read 2 B offset_size=0; write = offset_size=2, addr = devad<<8|RegH, data = `[RegL,DataH,DataL]`. ## MDIO command handler diff --git a/docs/state.md b/docs/state.md index 1f1f157..e28f70d 100644 --- a/docs/state.md +++ b/docs/state.md @@ -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): bringup identifies both modules and checks EEE off and jumbo on (forcing with an AN restart only on mismatch); 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, every bridged read staleness-checked — modules/fs/ for the late-fetch hazard): bringup identifies both modules and checks EEE off and jumbo on (forcing with an AN restart only on mismatch); 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 diff --git a/phy.go b/phy.go index 5ea04d1..1948cd8 100644 --- a/phy.go +++ b/phy.go @@ -40,12 +40,14 @@ const ( bcmPHYIDHi = 0x3590 bcmPHYIDLo = 0x5081 - bcmReadDelayUs = 3000 - bcmRetryDelayUs = 10000 + bcmReadDelayUs = 3000 bcmStatusPoll = 100 * time.Millisecond bcmStatusTries = 30 + bcmVerifyGap = 2 * time.Millisecond + bcmVerifyTries = 8 + ecdPoll = 200 * time.Millisecond ecdDeadline = 50 * time.Second @@ -70,6 +72,10 @@ type bcm struct { // Guards multi-op sequences only; single reads are already atomic on the // wire through the compound op. mu sync.Mutex + + rmu sync.Mutex + lastBuf uint16 + haveBuf bool } func openBCM(ifname string) (*bcm, error) { @@ -155,14 +161,50 @@ func (b *bcm) mdioReadDelay(devad, reg uint16, delayUs int) (uint16, error) { return uint16(d[0])<<8 | uint16(d[1]), nil } -// 0x0000 is also the bridge's not-ready signature, so a zero is read again at -// a longer delay before being believed. -func (b *bcm) mdioRead(devad, reg uint16) (uint16, error) { - v, err := b.mdioReadDelay(devad, reg, bcmReadDelayUs) - if err != nil || v != 0 { - return v, err +func (b *bcm) bufRead() (uint16, error) { + resp, err := b.op(fmt.Sprintf("r %02x 2", bcmI2CRead)) + if err != nil { + return 0, err } - return b.mdioReadDelay(devad, reg, bcmRetryDelayUs) + d, err := parseHexBytes(resp, 2) + if err != nil { + return 0, err + } + return uint16(d[0])<<8 | uint16(d[1]), nil +} + +// The bridge fetch is asynchronous: one that outruns the delay leaves the +// previous response in the buffer, served silently as the wrong register's +// data. A changed buffer value proves a fresh fetch; an unchanged or zero one +// is re-read bare — never re-armed, so clear-on-read registers keep their +// data — until it stabilizes. +func (b *bcm) mdioRead(devad, reg uint16) (uint16, error) { + b.rmu.Lock() + defer b.rmu.Unlock() + + v, err := b.mdioReadDelay(devad, reg, bcmReadDelayUs) + if err != nil { + return 0, err + } + if b.haveBuf && v != 0 && v != b.lastBuf { + b.lastBuf = v + return v, nil + } + stable := 0 + for i := 0; i < bcmVerifyTries && stable < 2; i++ { + time.Sleep(bcmVerifyGap) + r, err := b.bufRead() + if err != nil { + return 0, err + } + if r == v { + stable++ + } else { + v, stable = r, 0 + } + } + b.haveBuf, b.lastBuf = true, v + return v, nil } func (b *bcm) mdioWrite(devad, reg, val uint16) error { @@ -229,24 +271,29 @@ func (b *bcm) command(code uint16, params ...uint16) ([5]uint16, error) { return data, nil } +// Retried against the known constant: the first reads after a process start +// can land while the bridge still holds a dead process's pending fetch. func (b *bcm) identify() (string, error) { - hi, err := b.mdioRead(1, 2) - if err != nil { - return "", err + var hi, lo uint16 + var err error + for i := 0; i < 5; i++ { + if hi, err = b.mdioRead(1, 2); err != nil { + return "", err + } + if lo, err = b.mdioRead(1, 3); err != nil { + return "", err + } + if hi == bcmPHYIDHi && lo == bcmPHYIDLo { + sn, err := b.eeprom(68, 16) + if err != nil { + return "", err + } + return "BCM84891L sn " + strings.TrimSpace(string(sn)), nil + } + time.Sleep(200 * time.Millisecond) } - lo, err := b.mdioRead(1, 3) - if err != nil { - return "", err - } - 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 + 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