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

[dpdklab at iol.unh.edu]

Test-Label: ai-code-review
Test-Status: SUCCESS
https://dpdk.org/patch/171088

_AI Code Review_

=== Patch Review: series.patch (via Claude) ===
Review date: 2026-10-12

# DPDK Patch Review

## PATCH v8 01/16: net/txgbe: fix failure to configure 10G on dual-speed DAC

### ERRORS

None.

### WARNINGS

None.

### INFO

**Pre-existing: allowed_speeds restriction**  
Line changed: `drivers/net/txgbe/txgbe_ethdev.c:1851`  
The original hardcoded `RTE_ETH_LINK_SPEED_40G` restriction dates from the initial Amber-Lite backplane commit (fb6eb170dfa2). This patch corrects it to allow both 10G and 40G as intended.

**Capability mask opening**  
Lines changed: `drivers/net/txgbe/base/txgbe_aml40.c:101-105`  
The `get_link_capabilities` function now returns the detected module speed from `hw->phy.fiber_suppport_speed` for DAC cables and both 10G|40G for backplane, instead of hardcoding 40G. This aligns the capability advertisement with the physical module.

**Highest speed selection in forced mode**  
Lines added: `drivers/net/txgbe/base/txgbe_aml40.c:180-183`  
When auto-negotiation is off (`speed` is a single value), the code picks the highest available speed. This is correct behavior for forced-speed configuration where the user has explicitly requested one speed.

---

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

### ERRORS

None.

### WARNINGS

None.

### INFO

**Function signature change**  
Line changed: `drivers/net/txgbe/base/txgbe_e56_bp.h:280`  
`txgbe_e56_set_phy_link_mode()` now takes a `u32 speed` bitmask instead of `u8 bp_link_mode`. The implementation uses this to gate which speeds are advertised in the AN base page, allowing explicit speed requests to reach the advertisement.

**Autoneg_advertised recording**  
Lines added: `drivers/net/txgbe/txgbe_ethdev.c:1892-1904`  
The caller speed is recorded in `hw->phy.autoneg_advertised` before `setup_link()` so that the AN restart paths (watchdog, training-restart) can re-apply the same configuration instead of using a hardcoded fallback.

**AN base page advertisement**  
Lines changed: `drivers/net/txgbe/base/txgbe_e56_bp.c:2131-2165`  
The AN base page now advertises the speeds present in the `speed` parameter rather than always advertising all speeds supported by the device. This allows 10G-only requests to reach the base page on a 10G|40G part.

---

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

### ERRORS

None.

### WARNINGS

None.

### INFO

**Bit field correction**  
Line changed: `drivers/net/txgbe/base/txgbe_e56.c:426`  
The RXS ring equalizer/tap configuration was written to the wrong bit field (9:4) instead of the intended (21:12). The fix applies the configured tap values to the correct field so the equalizer setting takes effect.

---

## PATCH v8 04/16: net/txgbe: fix incorrect link state in 10G forced mode

### ERRORS

None.

### WARNINGS

None.

### INFO

**Link_valid restoration**  
Lines added: `drivers/net/txgbe/base/txgbe_aml40.c:205-216`  
The success path of `txgbe_setup_phy_link_aml40()` now restores `hw->link_valid = true`, symmetric to the 40G path. This prevents a stale `link_valid = false` from a previous timeout from keeping the link down after the peer recovers.

**TXGBE_ERR_PHY_INIT_NOT_DONE handling**  
Lines added: `drivers/net/txgbe/base/txgbe_aml40.c:207-210`  
The `TXGBE_ERR_PHY_INIT_NOT_DONE` case is now handled before the timeout split, leaving `link_valid` untouched so the alarm retry can attempt the setup again. This matches the 25G path in `txgbe_setup_phy_link_aml()`.

---

## PATCH v8 05/16: net/txgbe: do not force reconfig on link retry

### ERRORS

None.

### WARNINGS

None.

### INFO

**Need_restart derivation**  
Lines added: `drivers/net/txgbe/base/txgbe_aml.c:250-252` and `drivers/net/txgbe/base/txgbe_aml40.c:175-177`  
The `need_restart` argument to `txgbe_e56_set_phy_link_mode()` is now derived from the current link and AN state (`!link_up || !hw->an_done`) rather than passing the `autoneg_wait_to_complete` flag unchanged. This allows retries to recover an incomplete setup without reconfiguring an established link.

**Alarm handler change**  
Line changed: `drivers/net/txgbe/txgbe_ethdev.c:3240`  
The link alarm retry calls `mac.setup_link()` with `autoneg_wait_to_complete = false` instead of `true`. Passing `true` would reset an established AN73 link, which then drops and may not recover after DAC hot-plug.

---

## PATCH v8 06/16: net/txgbe: set i2c sda hold time

### ERRORS

None.

### WARNINGS

None.

### INFO

**I2C timing fix**  
Lines added: `drivers/net/txgbe/base/txgbe_phy.c:1337-1338`  
The I2C SDA hold time is set to 100 (0x64) I2C clock periods for both RX and TX in `txgbe_i2c_start()`. The measured data_in_hold_time did not meet the hardware requirement, which could corrupt SFP EEPROM reads over I2C.

---

## PATCH v8 07/16: net/txgbe: fix link speed display info for 10G mode

### ERRORS

None.

### WARNINGS

None.

### INFO

**10G speed recognition**  
Lines added: `drivers/net/txgbe/base/txgbe_aml40.c:88-90`  
The 10G port status bit is now recognized in `txgbe_check_mac_link_aml40()`. Previously, any non-40G speed fell through to the default case (UNKNOWN) which was then reported as 100M. A 10G link is now reported as 10G.

---

## PATCH v8 08/16: net/txgbe: remove stale outer UDP checksum offload flag

### ERRORS

None.

### WARNINGS

None.

### INFO

**Offload mask consistency**  
Line removed: `drivers/net/txgbe/txgbe_rxtx.c:64`  
`RTE_MBUF_F_TX_OUTER_UDP_CKSUM` is dropped from `TXGBE_TX_OFFLOAD_MASK` because it is no longer advertised in `dev_info->tx_offload_capa`. The two are now consistent: the advertised capability and the accepted offload flags match.

---

## PATCH v8 09/16: net/txgbe: fix SFP hot-plug when auto-negotiation is on

### ERRORS

None.

### WARNINGS

None.

### INFO

**Hot-plug detection on 40G NIC**  
Lines added: `drivers/net/txgbe/txgbe_ethdev.c:3085-3102`  
The 40G NIC never delivers the GPIO interrupt for module insertion or removal, so `txgbe_dev_detect_sfp()` is not called automatically. The patch polls the module-present level every 2 seconds instead. Identification is attempted only when the cached module state is not present, and the poll re-arms itself only while the SFP/AN73 alarm gate is set.

**Unsupported module handling**  
Lines added: `drivers/net/txgbe/base/txgbe_phy.c:1128-1129` and `drivers/net/txgbe/base/txgbe_phy.c:1163-1167`  
An unsupported QSFP identifier or a QSFP module with no supported capability bit now records the cached state as `txgbe_sfp_type_unknown` instead of leaving it as `not_present`. This prevents indefinite polling and I2C reads.

**AN73 watchdog re-arm**  
Lines added: `drivers/net/txgbe/txgbe_ethdev.c:3115-3123`  
The AN73 watchdog is re-armed from `txgbe_dev_detect_sfp()` once a module has been identified, and cancelled on the removal path. This allows monitoring to resume after a module is inserted.

**Alarm gate**  
Lines added: `drivers/net/txgbe/txgbe_ethdev.c:1974-1975` and `drivers/net/txgbe/txgbe_ethdev.c:2033-2050`  
A per-port `sfp_an_alarm_enabled` flag gates both the SFP detection alarm and the AN73 watchdog. `txgbe_dev_start()` sets it before arming the alarms; `txgbe_dev_stop()` clears it before cancelling them. This prevents a handler already running on the alarm thread from re-arming itself or arming the other alarm on a port that is being stopped.

---

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

### ERRORS

None.

### WARNINGS

None.

### INFO

**Module polling**  
Lines added: `drivers/net/txgbe/txgbe_ethdev.c:1987-1990` and `drivers/net/txgbe/txgbe_ethdev.c:3136-3141`  
The 40G NIC does not deliver a GPIO interrupt on module insertion or removal, so the module-present level is polled every 2 seconds. Re-identification is only attempted from the not-present state, and the poll re-arms itself only while the SFP/AN73 alarm gate is set.

**Unknown module recording**  
Lines added: `drivers/net/txgbe/base/txgbe_phy.c:1128-1129` and `drivers/net/txgbe/base/txgbe_phy.c:1163-1167`  
An unsupported QSFP identifier or a QSFP module with no supported capability bit is now recorded as `txgbe_sfp_type_unknown` instead of being left as `not_present`. This prevents indefinite polling.

**Link_valid restoration on xpcs path**  
Lines added: `drivers/net/txgbe/base/txgbe_aml40.c:183-190`  
The xpcs path of `txgbe_setup_phy_link_aml40()` restores `hw->link_valid = true`, as the non-xpcs path already does. An earlier `txgbe_set_link_to_amlite()` timeout left it false, which kept the check_phy_link and check_mac_link gates forcing the link down even after AN73 brought it up.

---

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

### ERRORS

None.

### WARNINGS

None.

### INFO

**Per-lane FFE replication**  
Lines changed: `drivers/net/txgbe/base/txgbe_type.h:742-745`  
The FFE fields grow to 32 bits to hold the replicated tap value. Each tap value is replicated over the four lanes using the `S40G_TX_FFE_4LANE()` macro.

**E56 PHY defaults**  
Lines added: `drivers/net/txgbe/txgbe_ethdev.c:498-507`  
The E56 PHY needs its own FFE defaults, as the ones previously defined only applied to the Sapphire PHY. The defaults are picked based on the MAC type after the shared code init so the MAC type is known.

**Devargs parsing order**  
Lines moved: `drivers/net/txgbe/txgbe_ethdev.c:670`  
Parsing the devargs is moved after `txgbe_init_shared_code()` so the MAC type is known when the 40G defaults are picked.

---

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

### ERRORS

None.

### WARNINGS

None.

### INFO

**New devargs**  
Lines added: `doc/guides/nics/txgbe.rst:135-137` and `doc/guides/nics/txgbe.rst:186-188`  
The `ffe_pre2` devarg is added for the E56 second pre-cursor tap, and the `bp_capa` devarg is added for the 40G backplane advertisement selection. Both are documented in the driver guide and the release notes.

**FFE pre2 handling**  
Lines added: `drivers/net/txgbe/txgbe_ethdev.c:533-534` and `drivers/net/txgbe/txgbe_ethdev.c:561`  
The `ffe_pre2` devarg is parsed and stored in `hw->phy.ffe_pre2`. For the 40G PHY, it is replicated over the four lanes using `S40G_TX_FFE_4LANE()`.

---

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

### ERRORS

None.

### WARNINGS

None.

### INFO

**Laser control on link-down**  
Lines added: `drivers/net/txgbe/base/txgbe_hw.c:3263-3285`  
When `laser_off` is enabled, the Tx laser is brought down on port link-down: for DAC cables the PCS is disabled, for QSFP modules the Tx disable bit is written via I2C. The SFF-8636 Tx disable register is only touched while the management semaphore is held, and the acquire result is checked.

**Laser restore on link-up**  
Lines added: `drivers/net/txgbe/txgbe_ethdev.c:2174-2186`  
The Tx enable state is restored when the link comes up. For AML40, normal link setup is rerun when link is brought up with `laser_off` enabled, as re-enabling the transmitter alone does not restore the configuration disabled for DAC on link-down.

**Ungated restore**  
Lines added: `drivers/net/txgbe/base/txgbe_hw.c:3316-3323`  
Restoring the Tx enable state is deliberately not gated on `laser_off`. The SFF-8636 Tx disable byte keeps its value while the module stays powered, so a module left with Tx disabled by a run that exited with `laser_off=1` is still disabled when the driver next starts with `laser_off=0`. Nothing else clears that byte when the link comes up, so gating this write would leave the laser off for good.

---

## PATCH v8 14/16: net/txgbe: fix CR/KR link training and recovery

### ERRORS

None.

### WARNINGS

None.

### INFO

**AN FSM polling**  
Lines changed: `drivers/net/txgbe/base/txgbe_e56_bp.c:2548-2557`  
The AN FSM (0x78010) is now polled for CL72 completion until bits 0 and 3 are set (`(rdata & 0x9) == 0x9`), with a 1 ms step and a 400 ms budget. This replaces the ephy 0x163c mask-0xe poll that tested the wrong condition.

**Page exchange re-run**  
Lines changed: `drivers/net/txgbe/base/txgbe_e56_bp.c:2355-2421`  
The page exchange is now re-run on every AN next-page interrupt (1 ms step, 50 ms per exchange round, 200 ms total, explicit next-page handshakewarning: review has no Review-Result line, severity inferred from prose (response may be truncated)