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

[Stephen Hemminger <stephen@networkplumber.org>]

On Fri, 23 Oct 2026 19:03:38 +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).
> 
> ---
> v10:
>  - P12: check the FFE taps against the register field width of the MAC in
>         use, instead of only for the 40G one. On the Sapphire the main and
>         pre taps are six-bit fields and the post tap is seven-bit, so a
>         larger value spills into the neighbouring field rather than being
>         truncated.
>  - P12/P13: say "Amber-Lite 40G NIC" for bp_capa and laser_off, the only
>         MAC that implements either.
>  - P17: add a new patch to rename fiber_suppport_speed to fiber_support_speed.
>         The field was added with the misspelling in 234ce0d1fa, copied from the
>         out-of-tree base driver.

Most of it is fine, but you still are keeping a driver specific behavior around PHY
control which was identified as not acceptable in an earlier review.

I object to cases where driver writers treat their PMD as a unique and special case
because that would end up tying the user application to a specific PMD
and it makes it harder for users to manage and write code.

If there is a missing feature in the ethdev management layer, then control operations
and offloads should be added there and drivers should all work against that.

The current AI review reflects that.

Subject: Re: [PATCH v10 00/17] net/txgbe: Amber-Lite link fixes

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

The v8 comments on 02, 03, 10, 15 and 16 are addressed. The
NEED_LINK_CONFIG fix in 16 is correct.

Patches 01-11 and 14-17 look good. 12 and 13 still need work.


Errors
------

[PATCH v10 13/17] net/txgbe: add devarg to turn off Tx laser for AML40

NAK on the devarg still stands.

The new paragraph says it is opt-in because link setup resets the
PHY and restarts AN73 on the next link-up. set_link_up() is a
control path operation, and the application asked for the link to
come back up. Renegotiation is the expected cost of that.

Leaving the transmitter on is the bug. With the laser on,
rte_eth_dev_set_link_down() does not take the link down, and the
peer cannot see the loss of signal. Drivers should not each pick
their own meaning for set_link_down.

Remove TXGBE_DEVARG_LASER_OFF and hw->devarg.laser_off. Disable the
transmitter unconditionally on AML40 link down, and in
txgbe_dev_set_link_up() run:

	if (hw->mac.type == txgbe_mac_aml40) {
		speed = hw->phy.autoneg_advertised;
		...
		err = hw->mac.setup_link(hw, speed, true);
		...
	}

Drop the doc and release note entries with it.


Info
----

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

The new FFE range checks log at ERR level but still store the value.
On Sapphire an ffe_pre above 0x3F still runs into the main field
through "ffe_main << 8 | ffe_pre". Fall back to the default, as is
already done for bp_capa:

	if (ffe_pre > 0x3F) {
		PMD_INIT_LOG(ERR, "devarg '%s' value %u exceeds 0x3F",
			     TXGBE_DEVARG_FFE_PRE, ffe_pre);
		ffe_pre = 8;
	}

The bp_capa explanation in the commit message is fine.