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

[Stephen Hemminger <stephen@networkplumber.org>]

On Mon, 12 Oct 2026 18:56:17 +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).
> 
> ---
> v8:
>  - P05: keep a retry from reconfiguring an established AN73 link, while
>         still restarting PHY configuration when the link is down or AN is
>         incomplete.
>  - P10: on AML40, mark unsupported and unclassifiable QSFP modules as
>         unknown so the module poll does not repeatedly identify them or
>         rerun module and link setup.
>  - P12: place the txgbe release-note entries between the Solarflare and
>         ZTE driver entries.
>  - P13: rerun complete AML40 link setup on set_link_up() with laser_off
>         enabled, restoring the DAC configuration without adding a separate
>         state flag.
>  - P14: correct the AN FSM condition described in the commit message
>         and remove the stray line after the Signed-off-by trailer.
>  - P15: assign the detected 10G speed for passive and active-limiting
>         DACs, and remove the unreachable AML40 10G SFP capability branch.
>  - P16: add a new patch to keep Amber-Lite ports administratively down
>         by preventing the AN73 watchdog and setup-link retry paths from
>         configuring PHY while hw->dev_start is cleared.
> ---

Still lots of issues shown from running Claude AI review from web.

Subject: Re: [PATCH v8 00/16] net/txgbe: Amber-Lite link fixes

Series applies cleanly to main and every commit builds with
-Dwerror=true. All Fixes: hashes resolve.

Two errors: patch 16 can leave the link permanently down after a
set_link_down/set_link_up cycle, and patch 13 adds a driver-specific
devarg that changes set_link_down semantics.


Errors
------

[PATCH v8 16/16] net/txgbe: respect manual link-down on Amber-Lite

The early return in txgbe_dev_setup_link_alarm_handler_aml() skips
clearing TXGBE_FLAG_NEED_LINK_CONFIG.

On 25G SFP (media_type fiber) with LSC enabled, set_link_down()
calls txgbe_dev_link_update(); link is down, so link_update_share()
sets NEED_LINK_CONFIG and spawns the link thread. The thread calls
the AML handler, which now returns because dev_start is false, and
the flag stays set. After that, every txgbe_dev_link_update_share()
returns link down at the NEED_LINK_CONFIG check before calling
check_link or starting a new thread, including the call from
set_link_up(). Nothing else clears the flag, so the port reports
link down until the application restarts the process.

Clear the flag before returning:

	struct rte_eth_dev *dev = hw->dev_back;
	struct txgbe_interrupt *intr = TXGBE_DEV_INTR(dev);
	...
	if (!hw->dev_start) {
		intr->flags &= ~TXGBE_FLAG_NEED_LINK_CONFIG;
		return;
	}


[PATCH v8 13/16] net/txgbe: add devarg to turn off Tx laser for 40G NIC

NAK on the devarg. rte_eth_dev_set_link_down() is supposed to take
the link down, and the peer seeing loss of signal is the point of
it. Whether that happens should not depend on a per-driver knob.
Make it the unconditional behaviour on AML40 when link is set down,
and run link setup unconditionally in set_link_up().

The commit message already shows the cost of the knob: the enable
path has to stay ungated because a previous run with laser_off=1
can leave the module transmitter disabled. Without the devarg that
case goes away.


Warnings
--------

[PATCH v8 03/16] net/txgbe: fix e56 PHY configuration error

Commit message says the wrong field degrades both 40G and 10G, but
only txgbe_e56_cfg_40g() is changed. txgbe_e56_cfg_10g() still has
the same write:

	addr = E56PHY_RXS_RINGO_0_ADDR;
	rdata = rd32_ephy(hw, addr);
	set_fields_e56(&rdata, 9, 4, 0x366);

cfg_25g and the e56_bp.c copies already use (21, 12). Either fix
cfg_10g in this patch or reword the message to say 40G only.


[PATCH v8 12/16] net/txgbe: add pre2 FFE tap and backplane capability devargs

bp_capa is not range checked. Any value other than 0..2 leaves the
40G block in txgbe_e56_set_phy_link_mode() advertising nothing for
40G. Validate it in txgbe_parse_devargs():

	if (bp_capa > 2) {
		PMD_INIT_LOG(WARNING, "invalid bp_capa %u, using 0",
			     bp_capa);
		bp_capa = 0;
	}

Also, why is KR4 vs CR4 a user setting at all? This is a property
of the board; derive it from the subsystem id if possible.


Info
----

[PATCH v8 02/16] net/txgbe: use the requested speed in E56 AN setup

The three ifs building autoneg_advertised reduce to:

	hw->phy.autoneg_advertised = speed &
		(TXGBE_LINK_SPEED_10GB_FULL |
		 TXGBE_LINK_SPEED_25GB_FULL |
		 TXGBE_LINK_SPEED_40GB_FULL);

Drop "as the base layer of the out-of-tree driver does" from the
commit message; the change stands on its own.


[PATCH v8 10/16] net/txgbe: fix DAC hot-plug on 40G NIC with auto-negotiation

With no module plugged in, every 2 s poll goes to the NOT_PRESENT
branch, logs "SFP not present." and cancels the watchdog again. Act
only on the transition:

	} else if (err == TXGBE_ERR_SFP_NOT_PRESENT) {
		if (hw->phy.sfp_type != txgbe_sfp_type_not_present) {
			PMD_DRV_LOG(INFO, "SFP not present.");
			hw->phy.sfp_type = txgbe_sfp_type_not_present;
			rte_eal_alarm_cancel(txgbe_dev_e56_check_bp_event,
					     dev);
		}
	}


[PATCH v8 11/16] net/txgbe: fix 40G FFE tuning applied to first lane only

On AML40 a user ffe_* value above 0xFF is silently truncated by
S40G_TX_FFE_4LANE(). Reject or warn instead.


[PATCH v8 12/16] and [PATCH v8 13/16]

12 adds a stray blank line in release_26_11.rst and 13 removes it.
Drop the hunk from 12.


[PATCH v8 15/16] net/txgbe: align link capabilities and DAC classification

The comment "mirroring the temporary workaround in the previous
version" describes patch history, not the code. Suggest:

	/* Unknown module: report 40G so setup_mac_link() does not
	 * fail with TXGBE_ERR_LINK_SETUP.
	 */

The DEBUGOUT added at the top of txgbe_get_link_capabilities_aml40()
now runs on every watchdog tick, since txgbe_xpcs_an_enabled() calls
get_link_capabilities(). Remove it.

The bullet "Remove the AML40 10G SFP capability branch" describes a
branch that is not in the tree this patch applies to. Drop it.