Re: [PATCH v6 00/15] Wangxun fixes and new features

[Stephen Hemminger <stephen@networkplumber.org>]

On Tue, 29 Sep 2026 21:04:31 +0800
Zaiyu Wang <zaiyuwang@trustnetic.com> wrote:

> This series addresses link-related issues on Wangxun Amber-lite 25G/40G NICs
> (CR/KR training, hot-plug, 10G link state).
> 
> ---
> v6:
>  - fix the apply failure.
> ---
> v5:
>  - drop the UDP tunnel patch: the generic UDP tunnel flag covers any UDP
>    tunnel, so the tunnel type cannot be resolved from the destination port.
>  - P02: advertise 10G, 25G, and 40G from the requested speed mask instead
>         of the device ID, so the 25G and 40G parts advertise 10G by
>         default as well.
>  - P07: keep the patch limited to decoding the 10G link speed from PORTSTAT;
>         the AML40 default 10G|40G advertisement change is now in P02.
>  - P09: gate the SFP detection alarm and the AN73 watchdog on a new per-port
>         flag that dev_stop clears before the first cancel.
>         cancel SFP detection before the watchdog.
>  - P10: re-arm the 40G module poll only while the SFP/AN73 alarm flag is set.
>  - P12: treat the ffe_pre2 and bp_capa devargs as a feature rather than a fix;
>         drop the Fixes tags and stable Cc, and document the 25G/40G Amber-Lite
>         FFE defaults.
>  - P14: bound the AN page exchange, propagate its timeout to the watchdog h
>         andler, and avoid register reads used only by disabled BP debug logs.
>  - P15: stop clearing the auto_neg devarg when a 10G-only DAC disables AN;
>         derive the effective AN73 state from the current module capabilities
>         instead. Classify 40G active cables through the optical path, unify
>         DAC checks on txgbe_is_dac_cable().
> 
> Not handled in this revision:
> 
>  - P12: bp_capa is not range-checked. Devarg validation will be added in a
>         separate change so all txgbe devargs can be handled consistently.
>  - P14: the AN page exchange still busy-waits on the alarm thread and can
>         delay handling for other ports. A later change will decouple negotiation
>         from the alarm callback and run per-port negotiations in parallel.
> ---
> 
> 
> Zaiyu Wang (15):
>   net/txgbe: fix failure to configure 10G on dual-speed DAC
>   net/txgbe: use the requested speed in E56 AN setup
>   net/txgbe: fix e56 PHY configuration error
>   net/txgbe: fix incorrect link state in 10G forced mode
>   net/txgbe: do not force reconfig on link retry
>   net/txgbe: set i2c sda hold time
>   net/txgbe: fix link speed display info for 10G mode
>   net/txgbe: remove stale outer UDP checksum offload flag
>   net/txgbe: fix SFP hot-plug when auto-negotiation is on
>   net/txgbe: fix DAC hot-plug on 40G NIC with auto-negotiation
>   net/txgbe: fix 40G FFE tuning applied to first lane only
>   net/txgbe: add pre2 FFE tap and backplane capability devargs
>   net/txgbe: add devarg to turn off Tx laser for 40G NIC
>   net/txgbe: fix CR/KR link training and recovery
>   net/txgbe: align link capabilities and DAC classification
> 
>  doc/guides/nics/txgbe.rst              |  25 +++-
>  doc/guides/rel_notes/release_26_11.rst |  11 ++
>  drivers/net/txgbe/base/txgbe_aml.c     |   4 +-
>  drivers/net/txgbe/base/txgbe_aml40.c   |  92 ++++++++++--
>  drivers/net/txgbe/base/txgbe_e56.c     |  14 +-
>  drivers/net/txgbe/base/txgbe_e56.h     |   6 +
>  drivers/net/txgbe/base/txgbe_e56_bp.c  | 196 +++++++++++++++----------
>  drivers/net/txgbe/base/txgbe_e56_bp.h  |   4 +-
>  drivers/net/txgbe/base/txgbe_hw.c      |  33 +++++
>  drivers/net/txgbe/base/txgbe_osdep.h   |  12 +-
>  drivers/net/txgbe/base/txgbe_phy.c     |  25 +++-
>  drivers/net/txgbe/base/txgbe_phy.h     |   7 +
>  drivers/net/txgbe/base/txgbe_regs.h    |   3 +
>  drivers/net/txgbe/base/txgbe_type.h    |  17 ++-
>  drivers/net/txgbe/txgbe_ethdev.c       | 178 ++++++++++++++++++++--
>  drivers/net/txgbe/txgbe_ethdev.h       |   2 +
>  drivers/net/txgbe/txgbe_rxtx.c         |   1 -
>  17 files changed, 502 insertions(+), 128 deletions(-)

Looks OK to me, AI did find a couple typos that you should fix:


Reviewed the whole v6 series applied on top of main.  All 15 patches apply
cleanly, net/txgbe builds with no new warnings, and I found no correctness
bugs.  Three Info-level nits below, all in 14/15.

14/15 net/txgbe: fix CR/KR link training and recovery

A typo regression in a log string the patch rewrites:

	BP_LOG("KR TRAINNING CHECK = %x. pmd_ctrl:%lx-%lx-%lx-%lx\n",

"TRAINNING" should be "TRAINING".  The line it replaces spelled it
correctly ("KR TRAINING CHECK = %x, %s. ..."), so this loses the old string
for anyone grepping logs.  checkpatch flags it as TYPO_SPELLING.

Declarations after a statement in the new txgbe_e56_get_txffe():

	if (!rte_log_can_log(RTE_LOGTYPE_TXGBE_BP, RTE_LOG_DEBUG))
		return;

	/* 21. read txffe to check kr training status */
	u32 rdata = 0, pmd_ctrl = 0, lane_idx = 0, lane_num = 0, txffe = 0;

This compiles, but the rest of txgbe_e56_bp.c declares at the top of the
block.  Hoisting the declaration above the early return would match the
file.

Stale comment on the page-exchange loop:

	/* 50ms timeout */
	for (count = 0; count < 50; count++) {

count is reset to 0 each time a next page arrives, so the loop is really
bounded by count2 against AN_PAGE_EXCHANGE_TIMEOUT_MS (200).  The comment
describes the per-round budget, not the loop bound.

Review-Result: CLEAN

---
Notes on things that looked suspicious but check out, recorded so they do
not get re-reviewed:

- 02/15: the "readers fall back to get_link_capabilities() when the field
  is zero" claim in the commit message is accurate -- both readers do
  "speed = hw->phy.autoneg_advertised; if (!speed) ...get_link_capabilities()".

- 06/15: wr32m(hw, TXGBE_I2C_SDA_HOLD, ..., 0x640064) against masks
  0xff0000 / 0xffff places 0x64 correctly in both the RX and TX fields.

- 08/15: no remaining RTE_MBUF_F_TX_OUTER_UDP_CKSUM reference anywhere in
  the driver, so dropping it from TXGBE_TX_OFFLOAD_MASK is complete.

- 11/15: moving txgbe_parse_devargs() after txgbe_init_shared_code() is
  safe -- the latter only sets the MAC type and the ops tables and reads no
  devarg.  The four-lane FFE value reaches the register whole through
  wr32_ephy(), so widening the fields to u32 is what makes the replication
  work.

- 14/15: removing the "if (status) return status;" after
  txgbe_e56_cl72_training() does not swallow the error -- status is not
  reassigned through the idle-detect writes and is still returned, and the
  caller checks ret.  The commit message says the non-abort is deliberate.

- 15/15: txgbe_xpcs_an_enabled() calling get_link_capabilities() does not
  recurse (no capability function calls back into it), and the
  !hw->devarg.auto_neg early return keeps the 25G path's unconditional
  *autoneg = true from re-enabling AN against the devarg.

- 09/15 and 14/15 are the two patches checkpatch rejects, both on false
  positives: "no space after cast" on RTE_ATOMIC(uint32_t), and
  COMPLEX_MACRO on "#define ...CFG_0 1, 0", which is the bitfield-pair
  convention used throughout the e56 headers.

One design point worth a second opinion rather than a fix: in 10/15,
txgbe_dev_detect_sfp() re-arms itself at the "rearm:" label, which the two
sfp_an_alarm_enabled == 0 early returns deliberately skip.  That is how the
poll stops on dev_stop, and it is correct, but it does make that gate the
only thing terminating the 2-second poll that 40G hot-plug detection now
depends on.