|SUCCESS| [v9, 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/171845

_AI Code Review_

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

# DPDK Patch Review

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

### Errors

None.

### Warnings

None.

### Info

**Patch 01 appears clean.**

The changes open up the 10G capability for the AML40 MAC in both the speed mask
and the get_link_capabilities path, and they ensure the user-requested speed
makes it through to the AN base page. The logic is clear and the
`hw->phy.autoneg_advertised` recording in the second patch provides the value
the alarm retry paths need.

---

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

### Errors

None.

### Warnings

None.

### Info

**Patch 02 appears clean.**

Recording `hw->phy.autoneg_advertised` before `setup_link()` and passing it
from the retry paths instead of a hardcoded constant is the right fix for
keeping the AN restart aligned with the original request. The change from
device-id-based base-page advertisement to speed-mask-based is the core of
getting a 10G request to the peer.

---

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

### Errors

None.

### Warnings

None.

### Info

**Patch 03 appears clean.**

Correcting the bit field from `(9, 4)` to `(21, 12)` for the RXS ring equalizer
is a straightforward fix. The two call sites are both changed consistently.

---

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

### Errors

None.

### Warnings

None.

### Info

**Patch 04 appears clean.**

The `hw->link_valid = true` assignment in the success path and the separate
handling of `TXGBE_ERR_PHY_INIT_NOT_DONE` before the timeout split are both
correct and match the 25G path logic.

---

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

### Errors

None.

### Warnings

None.

### Info

**Patch 05 appears clean.**

Deriving `need_restart` from `!link_up || !hw->an_done` so that only incomplete
setups are restarted, and leaving `autoneg_wait_to_complete = false` unchanged
in the alarm handler to avoid reconfiguring an established link, is the correct
approach. The xpcs path returns early when `link_up && hw->an_done` is true,
so the reconfig is correctly skipped there.

---

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

### Errors

None.

### Warnings

None.

### Info

**Patch 06 appears clean.**

Writing a fixed SDA hold time of 100 (0x64) for both RX and TX in
`txgbe_i2c_start()` is a straightforward hardware initialization fix. The
register definitions and the write are both added correctly.

---

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

### Errors

None.

### Warnings

None.

### Info

**Patch 07 appears clean.**

Adding the 10G port status bit recognition to `txgbe_check_mac_link_aml40()`
so that a 10G link is reported as 10G rather than falling through to UNKNOWN
is correct. The bit check is consistent with the 40G path.

---

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

### Errors

None.

### Warnings

None.

### Info

**Patch 08 appears clean.**

Dropping `RTE_MBUF_F_TX_OUTER_UDP_CKSUM` from `TXGBE_TX_OFFLOAD_MASK` aligns
the mask with the already-removed capability in `dev_info->tx_offload_capa`.

---

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

### Errors

None.

### Warnings

None.

### Info

**Patch 09 is substantial but appears correct.**

Key points:
- The `sfp_an_alarm_enabled` flag gates both the SFP detection alarm and the
  AN73 watchdog, and is cleared before cancelling either, so neither can
  re-arm after stop begins.
- The watchdog samples the module-present pin on every tick and drops the
  cached SFP type when the pin reads as absent, so cable removal is detected
  without a GPIO interrupt.
- `txgbe_dev_detect_sfp()` re-arms the AN73 watchdog after identification
  succeeds, with a cancel first to avoid running two instances.
- On the 40G NIC, `txgbe_dev_detect_sfp()` is armed from `txgbe_dev_start()`
  to poll the module-present level every 2 seconds.
- Unknown modules and unsupported identifiers are recorded as
  `txgbe_sfp_type_unknown` so polling resumes only after removal.

The logic is consistent, the ordering of the stop path (flag low, then cancel,
then join link thread, then cancel again) is safe, and the alarm re-arming is
always gated on the flag.

---

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

### Errors

None.

### Warnings

None.

### Info

**Patch 10 appears clean.**

- The 2-second module-present poll for AML40 is armed from `txgbe_dev_start()`
  and re-armed in `txgbe_dev_detect_sfp()` only while the alarm gate is set,
  so it cannot survive stop.
- Unsupported QSFP identifiers and modules with no supported capability bit
  are recorded as `txgbe_sfp_type_unknown` so polling resumes only after
  removal.
- `hw->link_valid` is restored in the xpcs path of `txgbe_setup_phy_link_aml40()`
  so a port started with no module present does not keep forcing the link down
  after AN73 brings it up.

---

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

### Errors

None.

### Warnings

None.

### Info

**Patch 11 appears clean.**

- The 40G PHY takes one FFE byte per lane; replicating each user value over
  the four lanes with `S40G_TX_FFE_4LANE()` is correct.
- The FFE defaults are now picked before parsing devargs and replicated
  afterwards, so both user values and defaults are handled consistently.
- `txgbe_parse_devargs()` is moved after `txgbe_init_shared_code()` so the
  MAC type is known when the 40G defaults are picked.

---

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

### Errors

None.

### Warnings

None.

### Info

**Patch 12 appears clean.**

- The `ffe_pre2` devarg is added for the E56 second pre-cursor tap, and
  `bp_capa` is added for the 40G backplane advertisement selection.
- The defaults are picked per MAC type (Sapphire, AML, AML40) before parsing
  devargs, and the 40G replication is applied afterwards.
- The `bp_capa` range check and the FFE range checks for the 40G PHY are all
  correct.
- Documentation and release notes are updated.

---

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

### Errors

None.

### Warnings

None.

### Info

**Patch 13 appears clean.**

- The `laser_off` devarg gates Tx laser disable on port link-down:
  for DAC cables the PCS is disabled through PMD_CFG0, for QSFP modules the
  SFF-8636 Tx disable byte is written via I2C.
- The I2C write is only attempted after `acquire_swfw_sync()` succeeds, so a
  failed acquire does not release a semaphore that is not held.
- The Tx enable path writes the SFF-8636 Tx disable byte unconditionally (not
  gated on `laser_off`) to clear any stale state left by a previous run.
- On AML40 with `laser_off` enabled, `txgbe_dev_set_link_up()` reruns
  `hw->mac.setup_link()` to restore the configuration disabled on link-down.
- Documentation and release notes are updated.

---

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

### Errors

None.

### Warnings

None.

### Info

**Patch 14 is large but appears correct.**

Key changes:
- The AN FSM completion poll (0x78010, bits 0 and 3) replaces the incorrect
  ephy 0x163c poll. The timeout is 400 ms with a 1 ms step, which is reasonable.
- The page exchange is re-run on every AN next-page interrupt with explicit
  handshakes and a 200 ms total budget. The inline exchange is removed from
  the AN73 flow so every link event re-runs it.
- A CL72 failure no longer aborts the flow; the function returns the status
  but does not block AN from continuing.
- The FFE init mode bits are set in `cfg_40g()`/`cfg_10g()`, and
  `txgbe_e56_set_rxs_ufine_le_max()` is run after the RXS osc init.
- `hw->bp_link_mode` is cleared in `txgbe_set_link_to_sfi()` so a port that
  trained on the backplane does not keep a stale mode after switching to SFI.
- Debug register reads and a leftover log line are dropped.

The logic is consistent, the ordering of init steps is correct, and the
re-running of the exchange on every link event is the right fix for reliability.

---

## PATCH v9 15/16: net/txgbe: align link capabilities and DAC classification

### Errors

None.

### Warnings

None.

### Info

**Patch 15 appears clean.**

Key changes:
- DAC classification is unified on `txgbe_is_dac_cable()`, including the 40G
  FFE selection and the 10G active cable type which was previously treated as
  an optical module.
- The 40G active transceiver identification is added (sfp_type enum and
  identify branch).
- A 10G-only passive DAC is assigned `TXGBE_LINK_SPEED_10GB_FULL` instead of
  preserving a stale speed from the previously installed module.
- A 10G active-limiting DAC is assigned `TXGBE_LINK_SPEED_10GB_FULL`.
- The `bp_auto` devarg is not modified when a 10G-only DAC disables AN, so a
  subsequently hot-plugged AN-capable DAC is re-evaluated and can enable AN73
  again.
- `txgbe_xpcs_an_enabled()` re-evaluates the current link capabilities instead
  of relying on a stale `hw->devarg.auto_neg` value.

---

## PATCH v9 16/16: net/txgbe: respect manual link-down on Amber-Lite

### Errors

None.

### Warnings

None.

### Info

**Patch 16 appears clean.**

- The AN73 watchdog and the setup-link alarm handler both check `hw->dev_start`
  and return early if the port is administratively down, so neither can
  configure the PHY or restart negotiation after `txgbe_dev_set_link_down()`.
- The watchdog stays armed so monitoring resumes after link-up.

---

## Final Check Summary

All patches have been reviewed. No Errors or Warnings identified.
All findings were Info-level observations confirming correctness or noting
pre-existing patterns that the patches do not introduce.

---

Review-Result: CLEAN