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

[Stephen Hemminger <stephen@networkplumber.org>]

On Mon, 26 Oct 2026 20:21:46 +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).
> 
> ---
> v11:
>  - P12: fall back to the default value when an FFE tap does not fit the
>         register field, instead of only logging it.
>  - P13: change the patch from a new feature to a fix:
>         take the transmitter down on AML40 link down. Clearing GPIO bit 1,
>         which holds the module in reset, does not stop the transmitter for
>         every DAC cable and QSFP module. The laser_off devarg and its
>         documentation are dropped; link setup runs again on link-up to
>         restore what link-down disabled.
>  - P17: fix warnings in check-git-log, and add the Cc: stable tag that
>         belongs with the Fixes line.

Better but still has issues.

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

Series applies to main and every commit builds with -Dwerror=true.
Dropping laser_off and the FFE range fallback address the v9 comments.
The 13/16 locking problem from v9 is still there.

Errors
------

Patch 13/17 (net/txgbe: fix Tx laser left on at AML40 link down)

enable_tx_laser() still takes the SWFW semaphore and does an I2C write
on AML40:

		    hw->mac.acquire_swfw_sync(hw, TXGBE_MNGSEM_SWPHY) == 0) {
			hw->phy.write_i2c_eeprom(hw,
				TXGBE_SFF_8636_TX_DISABLE, 0x0);

and txgbe_set_link_to_amlite() still calls enable_tx_laser() on the
TXGBE_ERR_PHY_INIT_NOT_DONE path, under the phy_lock spinlock taken in
txgbe_setup_phy_link_aml40().

It is worse than a long hold. txgbe_dev_setup_link_alarm_handler_aml()
acquires hw->phy.phy_semaphore_mask before calling setup_link(), and
txgbe_read_phy_if() sets that mask to TXGBE_MNGSEM_SWPHY. The nested
acquire therefore cannot succeed. It spins 200 x 5 ms holding
phy_lock, then the timeout path in txgbe_acquire_swfw_sync() clears
the SW bit the outer caller still holds, so the rest of that handler
runs without the semaphore and its final release drops a lock it no
longer owns.

Keep enable/disable_tx_laser() GPIO and PMD_CFG0 only, and do the
SFF-8636 write from the ethdev ops, which hold neither lock:

static void
txgbe_qsfp_tx_disable(struct txgbe_hw *hw, bool off)
{
	if (hw->mac.type != txgbe_mac_aml40 || txgbe_is_dac_cable(hw) ||
	    hw->phy.sfp_type == txgbe_sfp_type_unknown ||
	    hw->phy.sfp_type == txgbe_sfp_type_not_present)
		return;

	if (hw->mac.acquire_swfw_sync(hw, TXGBE_MNGSEM_SWPHY) != 0)
		return;
	hw->phy.write_i2c_eeprom(hw, TXGBE_SFF_8636_TX_DISABLE,
			off ? TXGBE_SFF_8636_TX_DISABLE_ALL_LANES : 0);
	hw->mac.release_swfw_sync(hw, TXGBE_MNGSEM_SWPHY);
}

Call it from txgbe_dev_start/stop() and txgbe_dev_set_link_up/down().

Warnings
--------

Patch 16/17 (net/txgbe: respect manual link-down on Amber-Lite)

Same as v9. hw->dev_start is checked once at the top of
txgbe_dev_setup_link_alarm_handler_aml() and the AN73 watchdog, then
setup_link() runs for seconds on the alarm or txgbe-link thread.
txgbe_dev_set_link_down() does not cancel the alarms or wait for the
link thread, so a link-down that lands in that window is undone when
setup_link() returns.

Do what txgbe_dev_stop() does, before touching the laser:

	hw->dev_start = false;
	if (hw->mac.type == txgbe_mac_aml ||
	    hw->mac.type == txgbe_mac_aml40) {
		rte_eal_alarm_cancel(txgbe_dev_e56_check_bp_event, dev);
		rte_eal_alarm_cancel(txgbe_dev_setup_link_alarm_handler_aml, hw);
		txgbe_dev_wait_setup_link_complete(dev, 0);
		rte_eal_alarm_cancel(txgbe_dev_setup_link_alarm_handler_aml, hw);
	}
	hw->mac.disable_tx_laser(hw);

and re-arm txgbe_dev_e56_check_bp_event in txgbe_dev_set_link_up().

Info
----

Patch 5/17: need_restart is still always true past the early return.
Pass true and drop the variable.

Patch 12/17: the *_dflt variables are initialized to 0 and then
unconditionally overwritten. Declare them where they are assigned.

Patch 14/17: still busy-waits up to 200 ms (page exchange) and 400 ms
(CL72) with usec_delay() on the EAL alarm thread.

Patch 15/17: the txgbe_is_40g_fiber_qsfp() branch is still identical
to the final else. Drop the helper.

Patch 17/17: a field rename is not a fix, and 234ce0d1fa9d is not in
25.11, so there is nothing to backport. Drop Fixes: and Cc: stable.

Review-Result: ERROR