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

[Stephen Hemminger <stephen@networkplumber.org>]

On Tue,  8 Sep 2026 21:25:45 +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.
> 
> ---
> v2:
>  - add a new commit to handle 10G config on dual-speed DAC
>  - P02: handle TXGBE_ERR_PHY_INIT_NOT_DONE before the timeout split and leave link_valid untouched.
>  - P07: resolve the inner tunnel type in a local variable instead of rewriting mbuf->ol_flags.
>         check the rte_pktmbuf_read() result for short or mis-annotated packets.
>         use RTE_GENEVE_DEFAULT_PORT instead of the literal 6081.
>         add a Fixes: tag pointing to the commit that introduced the tunnel offload handling.
>  - P10: split into a bug-fix commit and a feature commit.
>         document both devargs in the NIC guide and the release notes.
>  - P11: check the acquire_swfw_sync() result and only touch I2C while the semaphore is held.
>         replace the magic numbers with named constants.
>         add the release notes entry.
>  - P12: drop the stray C99 comment on the AN-config flag assignment that checkpatch rejects.
>  - P13: reword the message to state that 40G active cables are handled through the optical path.
>         use txgbe_is_dac_cable() in the 40G FFE selection to match the 25G path.
> ---
> 
> ---
> Not changed in this revision:
> 
>  - P11: enable_tx_laser() is not gated by laser_off. laser_off only
>    controls whether the Tx laser is brought down on port stop, while
>    enable_tx_laser() is unconditionally called from dev_start() and
>    dev_set_link_up() and must always bring the laser up, otherwise the
>    default (laser_off=0) configuration would never bring the link up.
> 
>  - P11: no extra restore of PMD_CFG0 bit 1 on the DAC path. Bits 19:12
>    are restored by txgbe_e56_set_phy_link_mode() and
>    txgbe_set_link_to_amlite(); bit 1 (PMD enable) is also restored by
>    txgbe_e56_set_phy_link_mode(), which the xpcs AN path calls on link
>    up, so no additional write is required.
> 
>  - P12: no total iteration bound was added to txgbe_e56_exchange_page().
>    The loop is bounded per page and exits when the AN FSM reaches 0x8,
>    so it does not hang in a normal Clause 73 flow. As this is hardware
>    related configuration, we leave the current behaviour unchanged for
>    now.
> ---
> 
> Zaiyu Wang (15):
>   net/txgbe: fix failure to configure 10G on dual-speed DAC
>   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: add offload support for tunnel type UDP
>   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 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              |  18 +++
>  doc/guides/rel_notes/release_26_11.rst |  11 ++
>  drivers/net/txgbe/base/txgbe_aml.c     |   3 +-
>  drivers/net/txgbe/base/txgbe_aml40.c   | 100 +++++++++++++--
>  drivers/net/txgbe/base/txgbe_e56.c     |  14 +--
>  drivers/net/txgbe/base/txgbe_e56.h     |   6 +
>  drivers/net/txgbe/base/txgbe_e56_bp.c  | 161 +++++++++++++++----------
>  drivers/net/txgbe/base/txgbe_e56_bp.h  |   2 +
>  drivers/net/txgbe/base/txgbe_hw.c      |  29 +++++
>  drivers/net/txgbe/base/txgbe_phy.c     |  13 ++
>  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       | 120 ++++++++++++++++--
>  drivers/net/txgbe/txgbe_rxtx.c         |  29 ++++-
>  15 files changed, 432 insertions(+), 101 deletions(-)


Looks good, but there are still some leftover items to address.

AI is good at picking out detail but can be wrong. So dont trust it.




Review of "[PATCH v3 00/15] net/txgbe: Amber-Lite link fixes, devargs"

Applied cleanly on d55ccd4 ("pci: remove deprecated catch-all flag").
Reviewed against the post-apply tree.  Build and checkpatch were not
run for this revision.

Closed since the previous revision:

 - The multi-bit speed mask reaching txgbe_set_link_to_amlite() and
   txgbe_e56_tx_ffe_cfg(), which compare with ==.  Patch 01 widens
   allowed_speeds and adds the highest-set-bit reduction ahead of both
   consumers.
 - link_valid set true after TXGBE_ERR_PHY_INIT_NOT_DONE (patch 03).
 - Unchecked rte_pktmbuf_read() and the ol_flags rewrite in
   txgbe_get_tun_len() (patch 08); RTE_GENEVE_DEFAULT_PORT is used and
   the two new TUNNEL_UDP case labels are the right consequence of no
   longer mutating the mbuf.
 - The //aml40-to-do C99 comment.
 - Incomplete txgbe_is_dac_cable() unification (patch 15 converts the
   40G branch of tx_ffe_cfg, rxs_calib_adapt_seq and the aml.c
   capability function as well).
 - Release notes, txgbe.rst entries, named constants for the SFF-8636
   Tx disable byte and PMD_CFG0, and the acquire_swfw_sync return
   checks on the disable path (patches 12 and 13).


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

Error: the enable path is still not guarded by the devarg.  This was
raised on the previous revision and is unchanged:

  void txgbe_enable_tx_laser_multispeed_fiber(struct txgbe_hw *hw)
  {
      ...
      if (hw->mac.type == txgbe_mac_aml40) {
          wr32m(hw, TXGBE_GPIODIR, TXGBE_GPIOBIT_1, TXGBE_GPIOBIT_1);
          esdp_reg |= TXGBE_GPIOBIT_1;
          if (hw->mac.acquire_swfw_sync(hw, TXGBE_MNGSEM_SWPHY) == 0) {
              hw->phy.write_i2c_eeprom(hw,
                      TXGBE_SFF_8636_TX_DISABLE, 0x0);

laser_off defaults to 0 and is documented as opt-in, but this I2C
EEPROM write runs on every enable_tx_laser() for every AML40 port
whether or not the user asked for it.

The disable path deliberately branches before touching I2C:

          if (hw->devarg.laser_off) {
                  if (txgbe_is_dac_cable(hw) ||
                      hw->phy.sfp_type == txgbe_sfp_type_unknown) {
                          ... clear PMD_CFG0 19:16, 15:12, bit 1 ...
                  } else if (hw->mac.acquire_swfw_sync(...) == 0) {
                          ... SFF-8636 Tx disable ...

so with laser_off=1 on a DAC cable, down clears those PMD_CFG0 fields
and up writes I2C to a cable that has no EEPROM Tx-disable byte to
write.  Please gate the enable path on hw->devarg.laser_off and mirror
the DAC/QSFP branch.

Warning: the disable path's "no module" case tests

          hw->phy.sfp_type == txgbe_sfp_type_unknown

but patches 09 and 10 in this same series set

          hw->phy.sfp_type = txgbe_sfp_type_not_present;

on module removal.  Those are different enumerators (0 versus 0xFFFE),
so after a pull the disable path falls into the else and issues an I2C
write to an absent module instead of taking the PCS branch.  Test both,
or use one enumerator consistently for "no module".


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

Error: txgbe_e56_exchange_page() can still loop indefinitely.  count2
was added and is incremented on every iteration, but it is only used
in a log message:

          BP_LOG("AN exchange page done in %d ms\n", count2);

The loop bound is still reset from inside the loop body:

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

so a link partner that keeps asserting next-page at 0x70019 keeps
resetting count and the loop never terminates.  This revision also
moves the call out of txgbe_handle_e56_bkp_an73_flow() and into
txgbe_dev_e56_check_bp_event(), so it now runs directly in the
interrupt/alarm thread and a wedge there blocks every other alarm in
the process.

count2 is the budget that is needed; it just needs to be enforced:

          if (count2 >= MAX_AN_PAGE_ITERATIONS)
                  return -ETIMEDOUT;

at the top of the loop body would cap the total while still allowing
count to restart per page.

Info (pre-existing, not introduced by this patch): three results in
txgbe_e56_cl72_training() are stored and discarded.  All three lines
are diff context here, so they predate the patch, but the function is
being reworked so they are cheap to fix in passing:

  status = txgbe_set_phy_link_mode(hw, bylinkmode);  /* never read */
  ...
  status = txgbe_e56_get_temp(hw, &temp_data);       /* overwritten */
  ...
  status = txgbe_e56_rxs_adc_adapt_seq(hw, bypass_ctle);

The last one is neither ORed into ret nor returned, so an adc
adaptation failure is silently dropped.

The removal of the early return after txgbe_e56_cl72_training() in
txgbe_handle_e56_bkp_an73_flow() is fine: status is not reassigned
before the return, so a training failure is still propagated and the
idle-detect thresholds are now programmed either way.


Patch 10/15: net/txgbe: fix DAC hot-plug on 40G NIC with autoneg

Warning: the new rearm re-arms without cancelling first, so the poll
chain can be duplicated:

  rearm:
      if (hw->mac.type == txgbe_mac_aml40 && !txgbe_is_backplane(hw) &&
          dev->data->dev_started)
          rte_eal_alarm_set(2000 * 1000, txgbe_dev_detect_sfp, dev);

txgbe_dev_detect_sfp() is also scheduled independently by
txgbe_dev_sfp_event() -- unconditionally on TXGBE_GPIOBIT_2, and again
on TXGBE_GPIOBIT_4 for aml40.  That invocation falls through to rearm
as well and starts a second self-perpetuating 2-second chain, and each
subsequent GPIO event adds another.  Patch 09 handles exactly this for
the other alarm, with the comment "so that only one instance of it is
running at a time":

          rte_eal_alarm_cancel(txgbe_dev_e56_check_bp_event, dev);
          rte_eal_alarm_set(hw->bp_event_interval,
                            txgbe_dev_e56_check_bp_event, dev);

The same cancel is needed before the rearm set.

(The dev_stop path is fine -- rte_eal_alarm_cancel() re-walks the list
in a do/while until nothing matching is executing, so a re-arm issued
by the executing callback is picked up on the next pass.)


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

Warning: the *autoneg the patch computes is never read.  All six call
sites of mac.get_link_capabilities pass a local that is not examined
afterwards: txgbe_setup_phy_link_aml40(), txgbe_dev_start(), the two
link alarm handlers in txgbe_ethdev.c, and both FEC ops.  So the new
DAC branch has no effect:

          if (hw->phy.fiber_suppport_speed ==
              TXGBE_LINK_SPEED_10GB_FULL &&
              hw->devarg.auto_neg == 0) {
                  *autoneg = false;
          } else {
                  *autoneg = hw->devarg.auto_neg;
          }

It is also a no-op on its own terms: the guard already requires
hw->devarg.auto_neg == 0, so the else arm yields false anyway and the
whole construct reduces to *autoneg = hw->devarg.auto_neg.

The 25G sibling that this is modelled on writes the field that is
actually consulted:

          if (hw->phy.fiber_suppport_speed ==
              TXGBE_LINK_SPEED_10GB_FULL) {
                  hw->devarg.auto_neg = false;
                  *autoneg = false;

hw->devarg.auto_neg is what txgbe_xpcs_an_enabled() reads.  If the
intent is to force AN off for 10G-only DAC cables on AML40, the aml40
version needs that assignment too; otherwise the comment about the
legacy AUTO=0/1 default does not describe what the code does.

Info: the multispeed_fiber arm is unreachable on this MAC type.
hw->phy.multispeed_fiber is only assigned in
txgbe_identify_sfp_module(), and only under txgbe_mac_aml or the SP
else-branch; txgbe_identify_qsfp_module() never touches it.  So

          } else if (hw->phy.multispeed_fiber) {

never runs on aml40 and the "multispeed fiber must come before
single-sfp/qsfp fiber" ordering comment has nothing to order.


Patch 01/15: net/txgbe: fix failure to configure 10G on dual-speed DAC

Info: the third item in the commit message does not hold.  The message
lists as a blocker that setup_phy_link_aml40() "passed a hardcoded 40
to txgbe_e56_set_phy_link_mode(), ignoring the caller speed", but that
function discards the argument:

  int txgbe_e56_set_phy_link_mode(struct txgbe_hw *hw,
                               u8 bp_link_mode, u32 need_restart)
  {
          ...
          UNREFERENCED_PARAMETER(bp_link_mode);

so replacing 40 with speed changes nothing.  Worth also noting that
the new argument is a bitmask -- TXGBE_LINK_SPEED_40GB_FULL is 0x0040
-- passed into a u8 whose other users in txgbe_e56_bp.c compare it
against 10, 25 and 40.  If the parameter is ever wired up, the value
handed in here will be wrong.  Either drop the hunk and the third
bullet, or convert the speed to a link mode at the call site.


Patch 09/15: net/txgbe: fix SFP hot-plug when auto-negotiation is on

Info: the new module-present sampling is not gated on
!txgbe_is_backplane(hw):

  else if (hw->mac.type == txgbe_mac_aml40)
      value = rd32(hw, TXGBE_GPIOEXT) & TXGBE_SFP1_MOD_PRST_LS;

  if (value != 0 && hw->phy.sfp_type != txgbe_sfp_type_not_present) {
      PMD_DRV_LOG(INFO, "SFP module removed, stop AN73 watchdog.");

On a backplane AML40 port there is no module, so this reports a removal
that never happened and clears sfp_type.  It is harmless for the AN
gate -- txgbe_xpcs_an_enabled() returns true for backplane regardless
of sfp_type -- but the log is misleading.  Patch 10 adds exactly the
!txgbe_is_backplane(hw) guard for its own alarm; the same guard fits
here.


No Reviewed-by on this revision -- the two Errors above are both
carried over unchanged from the previous revision.

Review-Result: ERROR