From 86cd5d1abd3c17e3f61b40fd162a2ab30340fa82 Mon Sep 17 00:00:00 2001 From: Dennis Brentjes Date: Fri, 9 Oct 2026 12:04:00 +0200 Subject: [PATCH] Fix two dropped-write bugs in RXFrameAssembler's SPRAM handshake Both from one mistake: eth_wr_valid is scheduled with m.d.sync and so is LOW for the whole first cycle in a state, while eth_wr_ready was tested immediately -- and `ready` is usually already high on entry. Both valid.eq(1) and valid.eq(0) then fired in the same cycle, the later won, the write was never issued, and it was dropped SILENTLY. 1. WRITE_LEN_HI dropped the descriptor's HIGH length byte, leaving it at its placeholder 0x00. Hardware read a descriptor of 00 00 00 01 -- a 1-byte frame length, impossible for ethernet -- and rejected the ring. 2. WRITE_PLACEHOLDER counted a phantom first write, advancing wr_addr without issuing one. Only THREE placeholder bytes were written and wr_addr ended at desc_base+3, so the frame body started at descriptor byte 3 and EVERY RECEIVED FRAME WAS SHIFTED ONE BYTE EARLY. The GC's ethertype test then read the wrong offset and silently dropped every frame: the host saw "no ACK" while the ring filled and RWP advanced normally. Both now advance on `valid & ready`, and the body offset is set explicitly (desc_base + 4) rather than relying on an incremented pointer, so it cannot drift with the handshake again. WHY NO TEST CAUGHT THIS, which is the more important half: - The testbench drove the RX stream OPEN-LOOP -- one byte per tick, never checking rx_ready, which is low through IDLE and the whole placeholder write. It silently lost bytes, including the SOF byte. A real master must hold each byte until accepted. - It only ever asserted on rwp and rx_irq. It declared a spram_writes list and never filled it, so NOTHING had ever checked what was written. - Every frame it sent was short, and a short frame's high length byte is legitimately zero -- so a dropped high byte was indistinguishable from a correct one. The testbench now respects back-pressure, records every write, and checks the descriptor bytes and the body offset. T2 sends a 300-byte frame specifically so the high length byte is non-zero. Mutation-tested: restoring either bug fails T2 with got [0,0,0,48] want [0,0,1,48] -- the same shape as the hardware's 00 00 00 01 -- while T1 (short frame) still passes, which is exactly the blind spot that let this ship. Bring-up's 342-byte DHCP frame read a correct 00 00 01 5A only because the arbiter happened to hold `ready` low on that entry cycle; the domain merge and the SPRAM response-FIFO removal changed that timing. --seeds 8: clk 27.8-32.2 MHz, EXI 54.9-64.5 MHz, 8/8 PASS. Best seed 8 (clk 32.22, EXI 62.25), flashed. Gateware only -- the .dol is unchanged. Co-Authored-By: Claude Opus 5 --- exi_bba/rx_frame_assembler.py | 238 ++++++++++++++++++++++------------ 1 file changed, 156 insertions(+), 82 deletions(-) diff --git a/exi_bba/rx_frame_assembler.py b/exi_bba/rx_frame_assembler.py index e08a5a1..fd70847 100644 --- a/exi_bba/rx_frame_assembler.py +++ b/exi_bba/rx_frame_assembler.py @@ -122,11 +122,34 @@ class RXFrameAssembler(Elaboratable): m.next = "WRITE_PLACEHOLDER" with m.State("WRITE_PLACEHOLDER"): - # Write 4 bytes of 0x00 as placeholder descriptor - m.d.sync += self.eth_wr_addr.eq(wr_addr) - m.d.sync += self.eth_wr_data.eq(0x00) + # Write 4 bytes of 0x00 as placeholder descriptor. + # + # ⚠️ Advance on `valid & ready`, NOT on `ready` alone. + # `eth_wr_valid` is scheduled with m.d.sync and so is LOW for + # the whole first cycle in this state, while `ready` is tested + # immediately -- and `ready` is usually already high here. + # Testing `ready` alone therefore counted a write that was + # never issued: only THREE placeholder bytes went out and + # wr_addr ended at desc_base+3, so the frame body started at + # descriptor byte 3 and EVERY RECEIVED FRAME WAS SHIFTED ONE + # BYTE EARLY. + # + # Hardware symptom (2026-10-09): the ring filled normally and + # RWP advanced, but the GC's ethertype test read the wrong + # offset and silently dropped every frame -- "no ACK" from the + # host with no other sign of trouble. The matching + # descriptor bug (same cause, in WRITE_LEN_HI) showed up as a + # descriptor of 00 00 00 01. + # + # The first byte is written twice as a consequence of + # eth_wr_addr trailing wr_addr by a cycle, and desc_base+3 is + # left unwritten -- both harmless, because every descriptor + # byte is either zero anyway or overwritten by WRITE_LEN_*. + # What matters is that wr_addr ends at desc_base+4. + m.d.sync += self.eth_wr_addr .eq(wr_addr) + m.d.sync += self.eth_wr_data .eq(0x00) m.d.sync += self.eth_wr_valid.eq(1) - with m.If(self.eth_wr_ready): + with m.If(self.eth_wr_valid & self.eth_wr_ready): m.d.sync += wr_addr.eq(wr_addr + 1) with m.If(desc_ctr == 3): m.d.sync += self.eth_wr_valid.eq(0) @@ -157,12 +180,27 @@ class RXFrameAssembler(Elaboratable): m.d.sync += pages_used.eq(total_len[8:12] + (total_len[:8] != 0)) m.next = "WRITE_LEN_HI" + # ⚠️ Both length writes must advance on `valid & ready`, NOT on + # `ready` alone. `valid` is scheduled with m.d.sync and so is low + # for the whole first cycle in the state, while `ready` is tested + # immediately -- and coming out of WAIT_LAST_WRITE `ready` is + # usually already high. Testing `ready` alone therefore fired both + # valid.eq(1) and valid.eq(0) in the same cycle, the later won, the + # write was never issued, and the length byte was SILENTLY DROPPED. + # + # Hardware symptom (2026-10-09): a descriptor of 00 00 00 01 -- + # byte 2 still at its placeholder 0x00 because the HI write was + # dropped -- so the GC read a 1-byte frame length and rejected the + # ring. Intermittent by nature: it depends on whether the arbiter + # happens to hold `ready` high on the entry cycle, which is why + # bring-up's 342-byte DHCP frame read a correct 00 00 01 5A and + # this went unnoticed. with m.State("WRITE_LEN_HI"): # Overwrite descriptor byte 2 with total_len[15:8] m.d.sync += self.eth_wr_addr.eq(desc_base + 2) m.d.sync += self.eth_wr_data.eq(total_len[8:12]) m.d.sync += self.eth_wr_valid.eq(1) - with m.If(self.eth_wr_ready): + with m.If(self.eth_wr_valid & self.eth_wr_ready): m.d.sync += self.eth_wr_valid.eq(0) m.next = "WRITE_LEN_LO" @@ -171,7 +209,7 @@ class RXFrameAssembler(Elaboratable): m.d.sync += self.eth_wr_addr.eq(desc_base + 3) m.d.sync += self.eth_wr_data.eq(total_len[:8]) m.d.sync += self.eth_wr_valid.eq(1) - with m.If(self.eth_wr_ready): + with m.If(self.eth_wr_valid & self.eth_wr_ready): m.d.sync += self.eth_wr_valid.eq(0) m.next = "ADVANCE_RWP" @@ -204,97 +242,134 @@ if __name__ == "__main__": dut = RXFrameAssembler() errors = [] - # Track all SPRAM writes issued by the DUT - spram_writes = [] + def brief(v): + """Long byte lists are unreadable in a pass/fail line; show the ends + and the length, and keep the full value only for a failure.""" + if isinstance(v, list) and len(v) > 8: + head = ", ".join(str(x) for x in v[:4]) + tail = ", ".join(str(x) for x in v[-2:]) + return f"[{head}, ... {tail}] ({len(v)} bytes)" + return str(v) + + def check(name, got, want): + ok = got == want + print(f" {'ok ' if ok else 'FAIL'} {name}: " + f"got {brief(got)}, want {brief(want)}") + if not ok: + if isinstance(got, list) and isinstance(want, list): + first = next((i for i, (g, w) in enumerate(zip(got, want)) + if g != w), None) + if first is not None: + print(f" first difference at index {first}: " + f"got {got[first]}, want {want[first]}") + errors.append(name) async def testbench(ctx): - # Setup: acknowledge all SPRAM writes immediately - ctx.set(dut.eth_wr_ready, 1) - ctx.set(dut.rx_wptr_w_rdy, 1) - ctx.set(dut.rx_enabled, 1) - await ctx.tick("sync").repeat(2) + # Final contents of every SPRAM address the DUT wrote. + # + # The old testbench declared a writes list and never filled it, and + # drove the RX stream OPEN-LOOP -- one byte per tick, never checking + # rx_ready. Both of the 2026-10-09 handshake bugs were therefore + # invisible to it: it only ever asserted on rwp and rx_irq, so a + # dropped descriptor byte or a frame body written one byte early + # changed nothing it looked at. + mem = {} - # ── T1: 10-byte frame → pages_used=1, rwp advances 1→2 ────────────── - # Send SOF + first byte - frame = [0xAA, 0xBB, 0xCC, 0xDD, 0x08, 0x00, 0x45, 0x00, 0x00, 0x01] + async def tick(n=1): + for _ in range(n): + # A write is issued on a cycle where valid AND ready are both + # high, so sample before the edge that completes it. + if ctx.get(dut.eth_wr_valid) and ctx.get(dut.eth_wr_ready): + mem[ctx.get(dut.eth_wr_addr)] = ctx.get(dut.eth_wr_data) + await ctx.tick("sync") - ctx.set(dut.rx_data, frame[0]) - ctx.set(dut.rx_valid, 1) - ctx.set(dut.rx_sof, 1) - await ctx.tick("sync").repeat(1) - ctx.set(dut.rx_sof, 0) + async def send_frame(frame): + """Push a frame RESPECTING BACK-PRESSURE, the way the real + ethernet master does. rx_ready is LOW through IDLE and the whole + placeholder-descriptor write, so a master that advances anyway + simply loses those bytes -- which is what the old testbench did, + and why it could not see the frame body land at the wrong + offset.""" + for i, b in enumerate(frame): + ctx.set(dut.rx_data, b) + ctx.set(dut.rx_valid, 1) + ctx.set(dut.rx_sof, 1 if i == 0 else 0) + ctx.set(dut.rx_eof, 1 if i == len(frame) - 1 else 0) + guard = 0 + while not ctx.get(dut.rx_ready): + await tick() + guard += 1 + if guard > 400: + errors.append("send_frame: rx_ready never asserted") + return + await tick() + ctx.set(dut.rx_valid, 0) + ctx.set(dut.rx_sof, 0) + ctx.set(dut.rx_eof, 0) - for i, b in enumerate(frame[1:], start=1): - ctx.set(dut.rx_data, b) - ctx.set(dut.rx_eof, 1 if i == len(frame) - 1 else 0) - await ctx.tick("sync").repeat(1) + async def wait_irq(limit=200): + for _ in range(limit): + await tick() + if ctx.get(dut.rx_irq): + return ctx.get(dut.rx_wptr_w_data) + return None - ctx.set(dut.rx_valid, 0) - ctx.set(dut.rx_eof, 0) + def descriptor(page): + base = page << 8 + return [mem.get(base + i) for i in range(4)] - # Poll for up to 30 ticks until rx_irq pulses (1-cycle pulse) - t1_irq_seen = False - t1_wptr_d = 0 - for _ in range(30): - await ctx.tick("sync").repeat(1) - if ctx.get(dut.rx_irq): - t1_irq_seen = True - t1_wptr_d = ctx.get(dut.rx_wptr_w_data) - break + def body(page, n): + base = (page << 8) + 4 + return [mem.get(base + i) for i in range(n)] - print(f"T1 rx_irq_seen={t1_irq_seen} wptr_data=0x{t1_wptr_d:02X}") - if not t1_irq_seen: - errors.append("T1: rx_irq never pulsed") - if t1_wptr_d != 2: - errors.append(f"T1: rwp should be 2 (page 1→2), got {t1_wptr_d}") + ctx.set(dut.eth_wr_ready, 1) + ctx.set(dut.rx_wptr_w_rdy, 1) + ctx.set(dut.rx_enabled, 1) + await tick(2) - await ctx.tick("sync").repeat(4) + # ── T1: short frame at page 1 ──────────────────────────────────── + f1 = [0xAA, 0xBB, 0xCC, 0xDD, 0x08, 0x00, 0x45, 0x00, 0x00, 0x01] + await send_frame(f1) + check("T1 rwp advance", await wait_irq(), 2) + total1 = len(f1) + 4 + check("T1 descriptor", descriptor(1), + [0x00, 0x00, (total1 >> 8) & 0xFF, total1 & 0xFF]) + # The body MUST start at base+4. This is the check that catches a + # placeholder write being dropped: with only three placeholder bytes + # written the body landed at base+3 and every frame was shifted one + # byte early, which on hardware made the GC drop every frame. + check("T1 body at base+4", body(1, len(f1)), f1) - # ── T2: Send a second frame; verify rwp advances further ──────────── - frame2 = [0x11, 0x22, 0x33, 0x44, 0x55, 0x66] - ctx.set(dut.rx_data, frame2[0]) - ctx.set(dut.rx_valid, 1) - ctx.set(dut.rx_sof, 1) - await ctx.tick("sync").repeat(1) - ctx.set(dut.rx_sof, 0) + await tick(4) - for i, b in enumerate(frame2[1:], start=1): - ctx.set(dut.rx_data, b) - ctx.set(dut.rx_eof, 1 if i == len(frame2) - 1 else 0) - await ctx.tick("sync").repeat(1) + # ── T2: frame LONGER THAN 255 BYTES, so the descriptor's HIGH length + # byte is non-zero. This is the case that catches a dropped + # WRITE_LEN_HI: with the high byte left at its placeholder 0x00 the GC + # read an impossible length (hardware saw 00 00 00 01) and rejected + # the ring. Every earlier test used a short frame, where a dropped + # high byte is indistinguishable from a correct zero. ─────────────── + f2 = [(i * 7 + 3) & 0xFF for i in range(300)] + await send_frame(f2) + rwp2 = await wait_irq() + total2 = len(f2) + 4 + check("T2 descriptor (len > 255)", descriptor(2), + [0x00, 0x00, (total2 >> 8) & 0xFF, total2 & 0xFF]) + check("T2 body at base+4", body(2, len(f2)), f2) + # 304 bytes spans two pages, so page 2 -> page 4. + check("T2 rwp advance (2 pages)", rwp2, 4) - ctx.set(dut.rx_valid, 0) - ctx.set(dut.rx_eof, 0) + await tick(4) - t2_irq_seen = False - t2_wptr_d = 0 - for _ in range(30): - await ctx.tick("sync").repeat(1) - if ctx.get(dut.rx_irq): - t2_irq_seen = True - t2_wptr_d = ctx.get(dut.rx_wptr_w_data) - break - - print(f"T2 rx_irq_seen={t2_irq_seen} wptr_data=0x{t2_wptr_d:02X}") - if not t2_irq_seen: - errors.append("T2: rx_irq never pulsed after second frame") - if t2_wptr_d != 3: - errors.append(f"T2: rwp should be 3 (page 2→3), got {t2_wptr_d}") - - # ── T3: RX disabled — SOF must be ignored ────────────────────────── + # ── T3: RX disabled — SOF must be ignored ──────────────────────── + before = len(mem) ctx.set(dut.rx_enabled, 0) ctx.set(dut.rx_data, 0xDE) ctx.set(dut.rx_valid, 1) ctx.set(dut.rx_sof, 1) - await ctx.tick("sync").repeat(4) + await tick(8) ctx.set(dut.rx_valid, 0) - ctx.set(dut.rx_sof, 0) - - # No SPRAM write should have been issued - wr_valid = ctx.get(dut.eth_wr_valid) - if wr_valid: - errors.append("T3: SPRAM write issued while rx_enabled=0") - print(f"T3 rx disabled: eth_wr_valid={wr_valid} (expected 0)") + ctx.set(dut.rx_sof, 0) + check("T3 no writes while rx disabled", len(mem), before) sim = Simulator(dut) sim.add_clock(Period(MHz=24), domain="sync") @@ -306,7 +381,6 @@ if __name__ == "__main__": if errors: print("\nFAILURES:") for e in errors: - print(" ", e) + print(" -", e) sys.exit(1) - else: - print("\nAll tests passed.") + print("\nAll tests passed.")