> -----Original Message-----
> From: Stephen Hemminger <[email protected]>
> Sent: Wednesday, May 27, 2026 11:22 PM
> To: Zaiyu Wang <[email protected]>; Zaiyu Wang
> <[email protected]>
> Cc: [email protected]; [email protected]
> Subject: Re: [PATCH v5 00/21] Wangxun Fixes
>
> On Wed, 27 May 2026 21:02:00 +0800
> Zaiyu Wang <[email protected]> wrote:
>
> > This series fixes several issues found on Wangxun Emerald, Sapphire
> > and Amber-lite NICs, with a focus on link-related problems.
> > ---
> > v5:
> > - Fixed issues identified by AI review
> > ---
> > v4:
> > - Fixed issues identified by devtools scripts
> > ---
> > v3:
> > - Addressed Stephen's comments
> > ---
> > v2:
> > - Fixed compilation error and code style issues
> > ---
> >
> > Zaiyu Wang (21):
> > net/txgbe: remove duplicate xstats counters
> > net/ngbe: remove duplicate xstats counters
> > net/ngbe: add missing CDR config for YT PHY
> > net/ngbe: fix VF promiscuous and allmulticast
> > net/txgbe: fix inaccuracy in Tx rate limiting
> > net/txgbe: fix link status check condition
> > net/txgbe: fix Tx desc free logic
> > net/txgbe: fix link flow control registers for Amber-Lite
> > net/txgbe: fix link flow control config for Sapphire
> > net/txgbe: fix a mass of unknown interrupts
> > net/txgbe: fix traffic class priority configuration
> > net/txgbe: fix link stability for 25G NIC
> > net/txgbe: fix link stability for 40G NIC
> > net/txgbe: fix link stability for Amber-Lite backplane mode
> > net/txgbe: fix FEC mode configuration on 25G NIC
> > net/txgbe: fix SFP module identification
> > net/txgbe: fix get module info operation
> > net/txgbe: fix get EEPROM operation
> > net/txgbe: fix to reset Tx write-back pointer
> > net/txgbe: fix to enable Tx desc check
> > net/txgbe: fix temperature track for AML NIC
> >
> > drivers/net/ngbe/base/ngbe_phy_yt.c | 3 +
> > drivers/net/ngbe/ngbe_ethdev.c | 5 -
> > drivers/net/ngbe/ngbe_ethdev_vf.c | 11 +-
> > drivers/net/txgbe/base/meson.build | 2 +
> > drivers/net/txgbe/base/txgbe.h | 2 +
> > drivers/net/txgbe/base/txgbe_aml.c | 185 +-
> > drivers/net/txgbe/base/txgbe_aml.h | 6 +-
> > drivers/net/txgbe/base/txgbe_aml40.c | 114 +-
> > drivers/net/txgbe/base/txgbe_aml40.h | 6 +-
> > drivers/net/txgbe/base/txgbe_dcb_hw.c | 2 +-
> > drivers/net/txgbe/base/txgbe_e56.c | 3773 +++++++++++++++++++++
> > drivers/net/txgbe/base/txgbe_e56.h | 1753 ++++++++++
> > drivers/net/txgbe/base/txgbe_e56_bp.c | 2597 ++++++++++++++
> > drivers/net/txgbe/base/txgbe_e56_bp.h | 282 ++
> > drivers/net/txgbe/base/txgbe_hw.c | 54 +-
> > drivers/net/txgbe/base/txgbe_hw.h | 4 +-
> > drivers/net/txgbe/base/txgbe_osdep.h | 4 +
> > drivers/net/txgbe/base/txgbe_phy.c | 362 +-
> > drivers/net/txgbe/base/txgbe_phy.h | 45 +-
> > drivers/net/txgbe/base/txgbe_regs.h | 13 +-
> > drivers/net/txgbe/base/txgbe_type.h | 43 +-
> > drivers/net/txgbe/txgbe_ethdev.c | 458 ++-
> > drivers/net/txgbe/txgbe_ethdev.h | 7 +-
> > drivers/net/txgbe/txgbe_rxtx.c | 107 +-
> > drivers/net/txgbe/txgbe_rxtx.h | 36 +
> > drivers/net/txgbe/txgbe_rxtx_vec_common.h | 16 +-
> > 26 files changed, 9445 insertions(+), 445 deletions(-) create mode
> > 100644 drivers/net/txgbe/base/txgbe_e56.c
> > create mode 100644 drivers/net/txgbe/base/txgbe_e56.h
> > create mode 100644 drivers/net/txgbe/base/txgbe_e56_bp.c
> > create mode 100644 drivers/net/txgbe/base/txgbe_e56_bp.h
> >
>
> More AI review feedback summary:
>
>
> 18/21 — get EEPROM (Error): page is declared once before the for loop and
> never reset, so after
> crossing the 256-byte boundary every subsequent iteration walks one page
> further into the
> module than it should. Plus two related issues: data[0x2] reads the output
> buffer not the module
> byte, and the skip branch leaves stale databyte carrying over from the
> previous iteration.
>
> 20/21 — Tx desc check (Error): #ifdef RTE_LIBRTE_SECURITY uses the
> pre-2020 macro name; the modern name is RTE_LIB_SECURITY (and the surrounding
> code in the
> same file uses it correctly). The IPsec guard is therefore always compiled
> out and the wr32m runs
> for every queue, including IPsec ones.
>
> 16/21 & 17/21 — whitespace: \t if (tab-then-space) on two lines. Checkpatch
> will catch it, but
> worth a pass.
>
> 17/21 — TXGBE_SFF_DDM_IMPLEMENTED: value 0x40 is correct as a bit-6 mask of
> byte 0x5C, but
> it is defined next to register-offset macros which makes it read as an offset.
>
> 14/21 — kr_read_poll macro: uses usleep() directly; everything else in the
> txgbe base layer uses
> usec_delay().
>
> 7/21 — type pun: the atomic load casts a volatile uint32_t * to volatile
> uint16_t *. Works on all
> DPDK platforms but is strict-aliasing-iffy; a u32 load with a u16 cast at the
> use site would be
> cleaner.
>
> Longer full review:
>
> Review of [PATCH v5 00/21] net/{txgbe,ngbe} fixes from Zaiyu Wang
>
> This revision addresses the substantive issues raised on v4:
>
> - 07/21: Tx desc free now uses a documented helper
> txgbe_tx_headwb_desc_done() that correctly handles the head==next
> boundary, and switches the headwb_mem read from a plain volatile
> access to rte_atomic_load_explicit(... acquire).
> - 08/21: AML xon/xoff stats no longer use plain assignment. The
> counter now goes through the new TXGBE_UPDATE_COUNTER_32BIT_GENERIC
> macro with offset tracking, and txgbe_clear_hw_cntrs() write-clears
> the AML registers and zeroes hw->last_stats on reset.
> - 11/21: traffic class priority is consistent across all three
> callers. TXGBE_RPUP2TC_UP_SHIFT is bumped to 4, TXGBE_DCBUP2TC_MAP
> is updated to match, txgbe_vmdq_dcb_configure() uses the macros
> instead of a hardcoded *3 shift, and the unused TXGBE_DCBUP2TC_DEC
> is removed. The bonus fix of redirecting
> txgbe_dcb_config_tx_data_arbiter_raptor() to TXGBE_PBTXUP2TC instead
> of TXGBE_PBRXUP2TC is welcome.
> - 12/21: txgbe_setup_phy_link_aml() now sets link_up = false before
> 'goto out' on TXGBE_ERR_TIMEOUT, so the out: block correctly routes
> to *need_reset = true. The generic 'compare' qsort helper has been
> renamed to txgbe_e56_int_cmp().
>
> Remaining findings on v5 below.
>
> ----------------------------------------------------------------
>
> Patch 18/21 (net/txgbe: fix get EEPROM operation)
>
> Error: page accumulation across loop iterations in
> txgbe_get_module_eeprom() will return wrong bytes for any QSFP read that
> crosses the 256-byte
> page boundary.
>
> + uint8_t page = 0;
> ...
> + for (i = info->offset; i < info->offset + info->length; i++) {
> + if (is_sfp) {
> ...
> + } else {
> + offset = i;
> + while (offset >= RTE_ETH_MODULE_SFF_8436_LEN) {
> + offset -= RTE_ETH_MODULE_SFF_8436_LEN / 2;
> + page++;
> + }
> + if (page == 0 || !(data[0x2] & 0x4)) {
> + status = hw->phy.read_i2c_sff8636(hw, page,
> offset,
> + &databyte);
>
> 'page' is declared once before the for loop and never reset, but the while
> loop only increments it.
> For i = 256 the math is correct
> (page=0 entering, page=1 leaving, offset=128). For i = 257 the loop enters
> with page=1 already
> set from the previous iteration and ends with page=2, offset=129 - it should
> still be page=1.
> Every subsequent iteration adds one more page, so the function reads bytes
> from ever-higher
> pages instead of staying on page 1, 2, 3, ...
>
> Reset page (and rebuild offset) at each iteration:
>
> uint16_t addr;
> uint8_t page;
>
> for (i = info->offset; i < info->offset + info->length; i++) {
> ...
> } else {
> page = 0;
> addr = i;
> while (addr >= RTE_ETH_MODULE_SFF_8436_LEN) {
> addr -= RTE_ETH_MODULE_SFF_8436_LEN / 2;
> page++;
> }
> ...
> }
> }
>
> Two related concerns in the same block:
>
> - The flat-memory check '!(data[0x2] & 0x4)' inspects byte 2 of the
> caller's output buffer rather than module byte 2. If info->offset
> > 2 the byte was never read, so the test reads the value left by
> memset (zero) and always evaluates to true. Read module byte 2
> explicitly before the loop and stash it in a local.
>
> - When the skip branch is taken (paged read on flat memory), the
> loop still does 'data[i - info->offset] = databyte', so the byte
> written is whatever databyte held from the previous iteration.
> Set databyte to 0 (or 0xff) before each iteration, or set the
> output byte directly inside the skip branch.
>
> Patch 20/21 (net/txgbe: fix to enable Tx desc check)
>
> Error: the new IPsec guard uses the wrong macro name and is always compiled
> out.
>
> +#ifdef RTE_LIBRTE_SECURITY
> + if (!txq->using_ipsec)
> +#endif
> + wr32m(hw, TXGBE_TDM_DESC_CHK(txq->reg_idx / 32),
> + RTE_BIT32(txq->reg_idx % 32),
> RTE_BIT32(txq->reg_idx % 32));
>
> The DPDK build macro is RTE_LIB_SECURITY; RTE_LIBRTE_SECURITY is the
> pre-2020 name and is no longer defined anywhere in the tree (the rest of this
> same driver uses
> RTE_LIB_SECURITY in the surrounding code, including the deleted block this
> patch replaces).
> Result: the 'if (!txq->using_ipsec)' line is never preprocessed in, the wr32m
> runs for every queue
> unconditionally, and IPsec-enabled queues get the desc-check bit set even
> though the existing
> intent was to skip them.
>
> Replace with RTE_LIB_SECURITY. Matches the existing pattern elsewhere in
> txgbe_rxtx.c
> (txgbe_rxtx.c:64, :444, :882, ...).
>
> Patch 16/21 (net/txgbe: fix SFP module identification)
>
> Warning: stray space in indentation of txgbe_read_i2c_sff8636() body.
>
> + s32 err = hw->phy.write_i2c_byte(hw, TXGBE_SFF_QSFP_PAGE_SELECT,
> + TXGBE_I2C_EEPROM_DEV_ADDR,
> + page);
> + if (err != 0)
> + return err;
>
> The 'if' line is indented with tab+space instead of a single tab.
> checkpatch will flag this.
>
> Info: this patch refactors away phy.read_i2c_byte_unlocked and
> phy.write_i2c_byte_unlocked and
> merges them into the existing phy.read_i2c_byte / phy.write_i2c_byte slots,
> which now no longer
> acquire the swfw semaphore. Patches 17 and 18 add explicit acquire/release
> around their own
> callers, which is correct, but it is worth double-checking that no other
> in-tree caller of
> phy.read_i2c_eeprom / phy.read_i2c_sff8472 / phy.read_i2c_byte runs without
> holding
> TXGBE_MNGSEM_SWPHY after this patch. The lock change and the callsite
> updates would
> arguably read better squashed together or at least kept adjacent in the
> series.
>
> Patch 17/21 (net/txgbe: fix get module info operation)
>
> Warning: stray space in indentation - same checkpatch issue as above.
>
> + if (hw->mac.type == txgbe_mac_aml) {
> + value = rd32(hw, TXGBE_GPIOEXT);
> + if (value & TXGBE_SFP1_MOD_ABS_LS) {
>
> 'if' is tab+space indented; should be two tabs.
>
> Info: TXGBE_SFF_DDM_IMPLEMENTED is added next to the SFP register offset
> definitions, but is
> then used as a bit mask of byte 0x5C:
>
> #define TXGBE_SFF_DDM_IMPLEMENTED 0x40
> #define TXGBE_SFF_SFF_8472_SWAP 0x5C
> ...
> if (sff8472_rev == TXGBE_SFF_SFF_8472_UNSUP || page_swap ||
> !(addr_mode & TXGBE_SFF_DDM_IMPLEMENTED)) {
>
> The value 0x40 is correct as bit 6 of the diagnostic-monitoring-type byte at
> offset 0x5C, but placing
> it alongside register offsets makes the macro look like an offset. Consider
> moving it into a
> bit-mask group or renaming the offset/mask pair to make the intent obvious
> (e.g. _ADDR vs
> _MASK suffix).
>
> Patch 14/21 (net/txgbe: fix link stability for Amber-Lite)
>
> Warning: the new kr_read_poll() macro in txgbe_phy.h uses usleep() directly,
> while the rest of the
> txgbe base layer uses the
> usec_delay() abstraction (which expands to rte_delay_us_block() on DPDK).
> Mixing the two is
> inconsistent and pulls in a POSIX dependency in a base/ file:
>
> +#define kr_read_poll(op, val, cond, sleep_us, \
> + times, args...) \
> +({ \
> ...
> + usleep(__sleep_us);\
> ...
> +})
>
> Switch to usec_delay() to match txgbe_eeprom.c, txgbe_hw.c, and the rest of
> the same file.
>
> Patch 7/21 (net/txgbe: fix Tx desc free logic)
>
> Info: txq->headwb_mem is declared 'volatile uint32_t *', but the new atomic
> read reads it as
> 'volatile uint16_t *':
>
> + const uint16_t head = rte_atomic_load_explicit((volatile uint16_t
> *)txq->headwb_mem,
> +
> rte_memory_order_acquire);
>
> This works on every architecture DPDK supports (lower 16 bits of an aligned
> 32-bit object are
> accessible as a 16-bit atomic and head fits in 16 bits), but it is a type pun
> on a hardware-written
> object and a strict-aliasing violation in pure C. A 32-bit atomic load with
> an explicit cast at use site
> would be cleaner:
>
> uint32_t h = rte_atomic_load_explicit(txq->headwb_mem,
> rte_memory_order_acquire);
> const uint16_t head = (uint16_t)h;
>
> Stephen Hemminger <[email protected]>
>
>
Thanks.
All the comments above have been addressed in v6.
