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

[Stephen Hemminger <stephen@networkplumber.org>]

On Thu, 27 Aug 2026 19:41:52 +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.


Using Claude AI review (versus the simpler CI AI review) shows several issues
that need addressing before this can be merged.

Here is my edit of the wordy AI feedbac.

Patch 02: net/txgbe: fix incorrect link state in 10G forced mode

Warning: the new else branch sets link_valid on a PHY init failure.

  ret_status = txgbe_set_link_to_amlite(hw, speed);
  if (ret_status == TXGBE_ERR_TIMEOUT)
          hw->link_valid = false;
  else
          hw->link_valid = true;

txgbe_set_link_to_amlite() can also return TXGBE_ERR_PHY_INIT_NOT_DONE
(VR_PCS_DIG_CTRL1 reset never cleared), which now falls into the else
and marks the link valid after the PHY failed to come out of reset.

The 25G sibling in txgbe_aml.c handles that code separately, before
the timeout/else split, precisely so link_valid is left alone:

  if (ret_status == TXGBE_ERR_PHY_INIT_NOT_DONE)
          goto out;
  if (ret_status == TXGBE_ERR_TIMEOUT) {
          hw->link_valid = false;
          ...

Suggest matching the aml.c structure here.


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

Warning: allowed_speeds is not extended to match.  txgbe_dev_start()
still has

  if (hw->mac.type == txgbe_mac_aml40)
          allowed_speeds = RTE_ETH_LINK_SPEED_40G;

so an application that explicitly requests RTE_ETH_LINK_SPEED_10G on an
AML40 port is still rejected with "Invalid link setting", even though
the driver now configures and reports 10G links.  Either add
RTE_ETH_LINK_SPEED_10G to allowed_speeds or explain why 10G is
autoneg-only.

See also the Error under patch 13 — the 40G|10G mask added here becomes
harmful once patch 13 widens the capability mask.


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

Error: unchecked rte_pktmbuf_read() return, then dereferenced.

  uh = rte_pktmbuf_read(mbuf, mbuf->outer_l2_len + mbuf->outer_l3_len,
                        sizeof(udphdr), &udphdr);
  if (uh->dest == rte_cpu_to_be_16(6081))

rte_pktmbuf_read() returns NULL when off + len > pkt_len.  outer_l2_len
and outer_l3_len come from the application and are not validated by the
driver, so a short or mis-annotated packet gives a NULL dereference in
the Tx fast path.  Please add a NULL check.

Note the neighbouring GRE and GENEVE cases have the same unchecked
pattern, so this is a pre-existing habit in the function rather than
something the patch invents, but the new code should not extend it.

Warning: txgbe_get_tun_len() rewrites the application's mbuf.

  mbuf->ol_flags &= ~RTE_MBUF_F_TX_TUNNEL_UDP;
  ...
  mbuf->ol_flags |= RTE_MBUF_F_TX_TUNNEL_GENEVE;   /* or VXLAN */

The Tx burst should not mutate ol_flags of a buffer it does not own.
The application still owns the mbuf and may inspect or re-transmit it
(cloned/multicast paths transmit the same mbuf more than once), and a
second pass then sees VXLAN/GENEVE instead of UDP.

Only the local switch below needs the resolved type, so a local
variable would do:

  uint64_t tun_type = mbuf->ol_flags & RTE_MBUF_F_TX_TUNNEL_MASK;

  if (tun_type == RTE_MBUF_F_TX_TUNNEL_UDP) {
          ...
          tun_type = (uh->dest == rte_cpu_to_be_16(RTE_GENEVE_DEFAULT_PORT))
                     ? RTE_MBUF_F_TX_TUNNEL_GENEVE
                     : RTE_MBUF_F_TX_TUNNEL_VXLAN;
  }
  switch (tun_type) {

This also removes an ordering subtlety: txgbe_xmit_pkts() snapshots
ol_flags into tx_ol_req before calling txgbe_get_tun_len(), so
tx_desc_ol_flags_to_ptid() and txgbe_set_xmit_ctx() see the original
TUNNEL_UDP while the mbuf itself has been rewritten.  The two new case
labels are correct today only because of that ordering.

Warning: Cc: stable@dpdk.org with no Fixes: tag.  The commit body
describes a defect (packets sent with a wrong descriptor or dropped),
so a Fixes: tag looks appropriate; otherwise drop the stable Cc.

Info: use RTE_GENEVE_DEFAULT_PORT from <rte_geneve.h> rather than the
literal 6081.


Patch 10: net/txgbe: add backplane FFE and capability devargs

The move of txgbe_parse_devargs() to after txgbe_init_shared_code() is
safe -- init_shared_code() only sets the MAC type, the ops tables,
max_link_up_time and the LAN id, and reads no devarg.  Wiring bp_capa
up is a real fix too: txgbe_e56_bp.c already branched on
hw->phy.bp_capa but nothing ever set it, so it was permanently 0.

Warning: the patch adds two new devargs (ffe_pre2, bp_capa) under a
Fixes: tag with Cc: stable@dpdk.org.  New configuration options are not
backport material.  Suggest splitting the ffe_pre2/bp_capa plumbing out
as a plain feature patch without the stable Cc, and keeping the stable
Cc only for the 40G per-lane FFE replication, which is the actual bug.

Warning: no release notes entry for the new devargs.


Patch 11: net/txgbe: add devarg to turn off Tx laser for 40G NIC

Error: the enable path is not guarded by the devarg.

  void txgbe_enable_tx_laser_multispeed_fiber(struct txgbe_hw *hw)
  {
          if (hw->mac.type == txgbe_mac_aml40) {
                  ...
                  txgbe_acquire_swfw_sync(hw, 1);
                  hw->phy.write_i2c_eeprom(hw, 86, 0x0);
                  txgbe_release_swfw_sync(hw, 1);

laser_off defaults to 0 and is documented as opt-in, but this I2C write
runs for every AML40 port on every enable_tx_laser(), whether or not the
user asked for it.  The disable path deliberately branches on
txgbe_is_dac_cable() / sfp_type before touching I2C; the enable path
does neither, so it also issues an I2C EEPROM write when a DAC cable or
no module is present.

Please guard it with hw->devarg.laser_off and mirror the DAC/QSFP
branch from the disable path.

Warning: with laser_off=1 on a DAC cable, disable clears PMD_CFG0 bits
19:16, 15:12 and bit 1.  Bits 19:12 are restored by
txgbe_e56_set_phy_link_mode() and by txgbe_set_link_to_amlite(), but I
could not find anything that restores bit 1 on the xpcs AN path --
only txgbe_set_link_to_amlite() sets it, and that is not called when
txgbe_xpcs_an_enabled() is true (DAC plus auto_neg=1, which is exactly
the configuration this devarg targets).  Could you confirm bit 1 is
restored somewhere on the AN path, or clear only what gets restored?

Warning: unchecked return values on the new calls.
txgbe_acquire_swfw_sync() returns TXGBE_ERR_SWFW_SYNC on a semaphore
timeout; on failure the code still performs the I2C write and then
releases a semaphore it does not hold.  write_i2c_eeprom() also returns
s32.  The convention elsewhere in the driver is

  err = hw->mac.acquire_swfw_sync(hw, TXGBE_MNGSEM_SWPHY);
  if (err != 0)
          return;

Info: several magic numbers.  0x1400 has a define, PMD_CFG0, in the
txgbe_e56.h that this patch now includes; 19:16 has one too,
E56PHY_PMD_CFG_0_RX_EN_CFG.  The semaphore mask 1 should be
TXGBE_MNGSEM_SWPHY (same value, clearer).  Byte 86 / 0xf is the
SFF-8636 Tx disable register and its all-lanes value -- both deserve
names.

Info: no release notes entry for the new devarg.


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

Error: leftover TODO and a C99 comment (checkpatch ERROR),
txgbe_ethdev.c:3806:

  intr->flags |= TXGBE_FLAG_NEED_AN_CONFIG;//aml40-to-do

Error: txgbe_e56_exchange_page() can loop indefinitely.  The loop bound
is reset from inside the loop body:

  for (count = 0; count < 50; count++) {
          ...
          if (rdata & BIT(14)) {
                  if (rdata & BIT(15)) {
                          wr32_epcs(hw, 0x70016, 0x2001);
                          next_page = 1;
                          count = 0; /* reset count to wait next page */
                  }
          }
          ...
          usec_delay(1000);
  }

The reset is not gated on base_page or on any independent budget, so as
long as the link partner keeps asserting next-page at 0x70019 the loop
never terminates.  This runs from txgbe_dev_e56_check_bp_event() in the
interrupt/alarm thread, so a peer that keeps requesting next pages wedges
that thread and blocks every other alarm on the process.

count2 already counts total iterations -- it just is not used as a
bound.  Something like

  if (count2 >= MAX_AN_PAGE_ITERATIONS)
          return -ETIMEDOUT;

would cap the total while still allowing count to restart per page.

Info: the "50ms timeout" comment no longer matches the code once count
can be reset; the effective wait is 50 ms per page, unbounded overall.

Info: falling out of the loop into the check_ability label and then
distinguishing the two paths by testing count == 50 is hard to follow.
An explicit "goto timeout" would read better.


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

Error: the widened capability mask lets a multi-bit speed reach code
that tests it with ==, so backplane and multispeed-fiber ports get
configured for 10G instead of 40G.

After this patch, get_link_capabilities_aml40() returns
10GB_FULL | 40GB_FULL for backplane and for multispeed fiber.  Patch 05
made txgbe_dev_start() request 40GB_FULL | 10GB_FULL on autoneg, so
"speed &= link_capabilities" in txgbe_setup_phy_link_aml40() now leaves
both bits set, where before patch 13 the mask reduced it to 40G alone.

Both downstream consumers test for equality:

  /* txgbe_set_link_to_amlite() */
  if (speed == TXGBE_LINK_SPEED_40GB_FULL) {
          ... 40G config ...
  } else {
          ... 10G config ...     /* taken with both bits set */
  }

  /* txgbe_e56_tx_ffe_cfg() */
  if (speed == TXGBE_LINK_SPEED_10GB_FULL) ...
  else if (speed == TXGBE_LINK_SPEED_25GB_FULL) ...
  else if (speed == TXGBE_LINK_SPEED_40GB_FULL) ...
  /* no branch matches; ffe_main/pre1/pre2/post stay 0 and are
   * written to the PHY as zeros */

Reachable whenever txgbe_xpcs_an_enabled() is false, which is the case
for backplane with auto_neg=0 and for multispeed fiber always (it
requires DAC or backplane).  For backplane the FFE path is entered
unconditionally, so both symptoms occur together.

Either resolve the mask to a single speed before calling setup_link, or
have txgbe_set_link_to_amlite()/txgbe_e56_tx_ffe_cfg() select the
highest set bit rather than comparing for equality.

Warning: the commit message and the code disagree about active DACs.
The message says

  "Unify DAC classification on txgbe_is_dac_cable(), so active DACs are
   no longer treated as optical modules."

but txgbe_qsfp_type_40g_active_core0/1 is added to
txgbe_is_40g_fiber_qsfp() and not to txgbe_is_dac_cable().  An active
40G cable therefore still takes the optical path: no DAC FFE values in
txgbe_e56_tx_ffe_cfg(), bypass_ctle left true in
txgbe_e56_rxs_calib_adapt_seq(), tx_ffe_cfg not called at all from
txgbe_setup_phy_link_aml40(), and AN off.

For comparison, the 10G equivalent txgbe_sfp_type_da_act_lmt_core0/1 is
in txgbe_is_dac_cable().  SFF-8636 byte 131 bit 0 is "40G Active Cable
(XLPPI)", which covers active copper as well as AOC.

Please either add the 40g_active types to txgbe_is_dac_cable() or
reword the commit message to say they are handled as optical.

Info: the unification is incomplete.  The 25G branch of
txgbe_e56_tx_ffe_cfg() was converted to txgbe_is_dac_cable(), but the
40G branch a few lines above still open-codes

  if (hw->phy.sfp_type == txgbe_qsfp_type_40g_cu_core0 ||
      hw->phy.sfp_type == txgbe_qsfp_type_40g_cu_core1 ||
      txgbe_is_backplane(hw))

which will keep missing any type added to txgbe_is_dac_cable() later.