[Bug 166724] if_re(4): watchdog timeout

https://bugs.freebsd.org/bugzilla/show_bug.cgi?id=166724

Jérémie Jourdin <[email protected]> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |[email protected]

--- Comment #134 from Jérémie Jourdin <[email protected]> ---
Hi all,

Same problem on 16.0-CURRENT with on-board Realtek RTL8168H NICs (re0: Chip
rev. 0x54000000). Under sustained load these hit the long-standing re0:
watchdog timeout. 

The usual advice is the net/realtek-re-kmod vendor driver, but we wanted to
understand what in-tree re(4) is actually missing and, where we safely could,
fix it in base rather than route around it.

So we did a careful comparison of three drivers on the paths that matter here:
in-tree if_re.c, Linux's r8169, and Realtek's own BSD-licensed vendor driver.
That surfaced a handful of concrete, well-understood gaps. The attached patches
close those, and only those. Every one of them does something that r8169 and/or
the Realtek driver already do - none of it is speculative.

What they address :

1. Lost transmit interrupts. re(4) already carries a 1 Hz "just in case"
reclaim in re_tick(), with a comment admitting Tx-completion interrupts "seem
to be lost on PCIe controllers." We close the two ways that actually happens:
  - A lost transmit doorbell (the "TxPoll requests are lost when packets are
queued too close together" erratum). r8169 and the vendor driver both re-arm
the transmitter from their completion routine; base only did so from the
interrupt handlers. We move the re-arm into re_txeof() so every reclaim path —
interrupt, the 1 Hz tick, and the watchdog — covers it.
  - A swallowed Tx-completion MSI. The INTx path already re-checks interrupt
status before re-enabling interrupts; the MSI path did not. That asymmetry
appears to be exactly why hw.re.msi_disable=1 has been a working workaround for
so long. We make the MSI path re-check and flush, so a completion landing in
the small window between ack and re-enable is serviced at once instead of
waiting on the tick.

2. The reset / recovery path on 8168G and later. Base issues a STOPREQ command
on these parts that neither r8169 nor Realtek's driver use on this family, and
it frees the DMA buffers without first confirming the controller has stopped
touching them. We follow the reference drivers: gate RXDV, drain the on-chip
FIFOs, then reset the controller before the buffers are freed — so it can't DMA
into memory that's about to be released.

3. Watchdog hardening + diagnostics. When the watchdog does fire, we (a) detect
a controller that has dropped off the bus and stop trying to re-init it, (b)
re-assert the ASPM disable — which r8169 flags as a common cause of these
timeouts — and (c) enrich the existing watchdog log line with a one-line
snapshot (interrupt mode, ISR/IMR, Tx-ring position) so a field occurrence
tells us which failure class it was. No new log lines, no debug build needed.

On the "conservative" side :
- Every change lives in a teardown, error/recovery, or interrupt-tail path. The
normal Tx/Rx fast path is untouched — throughput is unchanged (we measured
gigabit line rate before and after).
- They are register reads/writes only: no new locking, no allocations, no
data-structure or KBI changes. Every added loop is bounded.
- All are gated on hwrev / RL_FLAG_PCIE, so unaffected parts are never touched.
- Compiles warning-free under the kernel's -Werror.

We stopped short of porting the per-chip initialisation errata and MCU/PHY
firmware that make up most of the divergence between base re(4) and the vendor
driver. That is the high-risk surface, and it's the part we can't validate the
way we can validate the defensive changes above — a wrong init-register write
leaves the NIC down. 

The diagnostics in (3) exist precisely so that anyone can tell from the field
whether the remaining stalls are the lost-interrupt/recovery class these
patches address, or a genuine hardware-init stall that would justify that
riskier work.

This has been running on our RTL8168H build hosts, so far so good.

The set is five small patches:
- if_rlreg.h: MCU FIFO-status register definitions (8168G+)
- re_stop(): 8168G+ quiesce (drop the spurious STOPREQ, drain FIFOs) +
reset-before-free
- re_txeof(): re-arm the Tx doorbell after reclaim
- re_intr_msi(): recover a swallowed Tx-completion MSI + posted-write flush
- re_watchdog(): dead-controller detection, ASPM re-assert, and the one-line
diagnostics

See the proposed re-pr166724.patch file attached.

Hope this is useful and happy to iterate with reviewers,

Jérémie

-- 
You are receiving this mail because:
You are on the CC list for the bug.
You are the assignee for the bug.