RE: [PATCH v5 00/21] Wangxun Fixes

["Zaiyu Wang" <zaiyuwang@trustnetic.com>]

> -----Original Message-----
> From: Stephen Hemminger <stephen@networkplumber.org>
> Sent: Wednesday, May 27, 2026 11:22 PM
> To: Zaiyu Wang <zaiyuwang@trustnetic.com>; Zaiyu Wang <zaiyuwang@trustnetic.com>
> Cc: dev@dpdk.org; dev@dpdk.org
> Subject: Re: [PATCH v5 00/21] Wangxun Fixes
> 
> On Wed, 27 May 2026 21:02:00 +0800
> Zaiyu Wang <zaiyuwang@trustnetic.com> 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 <stephen@networkplumber.org>
> 
> 
Thanks.
All the comments above have been addressed in v6.