Re: [PATCH v4 00/16] Wangxun fixes and new features

[Stephen Hemminger <stephen@networkplumber.org>]

On Tue, 22 Sep 2026 20:37:33 +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), and additionally refines UDP
> offload handling.
> 
> ---

These AI comments look like the need addressing

Review: [PATCH v4 00/16] net/txgbe: Amber-Lite link and offload fixes
Author: Zaiyu Wang <zaiyuwang@trustnetic.com>

Series applies to main; every commit builds with -Dwerror=true.
Fixes: hashes resolve except the one noted in 13/16.


Patch 10/16: net/txgbe: fix SFP hot-plug when auto-negotiation is on

Warning: AN73 watchdog can be re-armed after dev_stop

  txgbe_dev_detect_sfp() now calls rte_eal_alarm_set() for
  txgbe_dev_e56_check_bp_event with no started check. dev_stop
  cancels check_bp_event first (txgbe_ethdev.c ~2121) and
  detect_sfp second (~2126). A detect_sfp instance running in the
  alarm thread between the two cancels, or one armed by a GPIO
  interrupt before txgbe_disable_intr(), re-arms the watchdog on a
  stopped port. check_bp_event re-arms itself unconditionally at
  "out:", so it keeps running, and after dev_close it dereferences
  freed port data.

  Patch 11 gates its own re-arm on dev->data->dev_started, but that
  flag is only cleared after dev_stop returns, so it does not close
  this window either. Cancel txgbe_dev_detect_sfp before
  txgbe_dev_e56_check_bp_event in dev_stop, and skip the re-arm in
  detect_sfp when hw->adapter_stopped is set.


Patch 09/16: net/txgbe: add offload support for tunnel type UDP

Warning: RTE_MBUF_F_TX_TUNNEL_UDP resolved by destination port

  The generic UDP tunnel flag is defined for tunnels that are not
  VXLAN or GENEVE. txgbe_get_tun_len() maps port 6081 to GENEVE and
  everything else to VXLAN, i.e. a fixed 16 byte UDP+VXLAN header.
  Any other UDP encapsulation gets a wrong tunnel length, so the
  inner checksum and TSO offsets are wrong, and GENEVE on a
  non-default port loses its options length. Since the driver
  advertises RTE_ETH_TX_OFFLOAD_UDP_TNL_TSO, either reject tunnels
  it cannot size in txgbe_prep_pkts() or stop advertising the
  capability.

Info: the new rte_pktmbuf_read() NULL check is right, but the GRE
  and GENEVE cases in the same switch still dereference grh and gh
  without one. Same fix, same function.


Patch 13/16: net/txgbe: fix unset pre2 FFE tap and backplane capability

Warning: Fixes: 6104fd11089d does not exist. The 25G commit is
  6104fd11086e, as used in 05/16 and 12/16.

Warning: this is a feature, not a fix

  The recommended pre2 values are S25G_TX_FFE_CFG_DAC_PRE2 = 0x0 and
  S40G_TX_FFE_CFG_PRE2 = 0x0, so programming ffe_pre2 = 0 was never
  wrong. bp_capa = 0 is the existing KR4+CR4 behaviour. What remains
  is two new devargs, which do not belong in stable. Drop the Fixes
  tags and Cc: stable.

  The patch also changes the ffe_main/pre/post defaults on the 25G
  MAC from 27/8/44 to 0x2a/0x03/0x11. Neither the commit message nor
  txgbe.rst mentions this; the guide still documents 27, 8 and 44.

Info: bp_capa is not range checked. Any value above 2 advertises no
  40G ability at all on the 40G backplane.


Patch 16/16: net/txgbe: align link capabilities and DAC classification

Warning: get_link_capabilities_aml40() clears hw->devarg.auto_neg

  For a DAC whose fiber_suppport_speed is 10G only, the new DAC
  branch writes hw->devarg.auto_neg = false. Nothing sets it back.
  With the hot-plug support in 10/16 and 11/16, replacing that
  module with a 40G QSFP DAC leaves txgbe_xpcs_an_enabled() false,
  so AN73 never runs until the port is re-probed. The aml function
  has the same pattern, but hot-plug now makes it reachable on
  aml40. Return *autoneg = false without touching the devarg.

Info: PMD_DRV_LOG in base/txgbe_aml40.c; base code uses DEBUGOUT.
  txgbe_is_40g_fiber_qsfp() and txgbe_is_10g_fiber_sfp() return int
  with true/false; make them bool.


Patch 02/16: net/txgbe: use the requested speed in E56 AN setup

Info: default AN advertisement changes. With dev_start passing
  10G|25G (aml) or 10G|40G (aml40 after 07/16), the backplane
  branch now advertises 10GBASE-KR on the 25G and 40G parts, where
  previously it advertised only 25G or only 40G. Say so in the
  commit message.


Patch 07/16: net/txgbe: fix link speed display info for 10G mode

Info: adding 10G to the aml40 autoneg speed mask is not needed for
  the reported speed; that comes from the new PORTSTAT check. It
  changes what is advertised (see 02/16) and belongs with 01/16.


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

Info: the CL72 poll (400 x 1 ms) and the page exchange (up to
  200 x 1 ms) busy-wait inside txgbe_dev_e56_check_bp_event(), which
  runs on the EAL interrupt thread. This stalls interrupt and alarm
  handling for all ports.

Info: BP_LOG expands to RTE_LOG, so its arguments are evaluated even
  when the log type is disabled. txgbe_e56_get_txffe() and the new
  "an_int" arguments do a dozen PHY reads per call only for logging.
  The BP_LOG("%s %d\n", __func__, __LINE__) in check_bp_event is a
  leftover debug line.