* RE: [PATCH v2 1/2] bus/fslmc: fix ignored return value in fslmc_bus_unplug
From: Hemant Agrawal @ 2026-05-18 7:19 UTC (permalink / raw)
To: Md Shofiqul Islam, dev@dpdk.org; +Cc: stable@dpdk.org
In-Reply-To: <20260516110828.35701-2-shofiqtest@gmail.com>
Acked-by: Hemant Agrawal <hemant.agrawal@nxp.com>
^ permalink raw reply
* RE: [PATCH v2 2/2] dma/dpaa2: fix dpaa2_qdma_remove always returning success
From: Hemant Agrawal @ 2026-05-18 7:19 UTC (permalink / raw)
To: Md Shofiqul Islam, dev@dpdk.org; +Cc: stable@dpdk.org
In-Reply-To: <20260516110828.35701-3-shofiqtest@gmail.com>
Acked-by: Hemant Agrawal <hemant.agrawal@nxp.com>
^ permalink raw reply
* Re: [PATCH V2 00/15] power: unify and improve lcore ID verification
From: lihuisong (C) @ 2026-05-18 7:02 UTC (permalink / raw)
To: anatoly.burakov, sivaprasad.tummala, stephen
Cc: dev, thomas, fengchengwen, yangxingui, zhanjie9, lihuisong
In-Reply-To: <20260507024230.1198111-1-lihuisong@huawei.com>
Kindly ping for reivew.
/Huisong
On 5/7/2026 10:42 AM, Huisong Li wrote:
> This patch series reworks the lcore ID verification logic within the
> power library to ensure consistency and improve maintainability.
>
> Currently, various cpufreq drivers implement their own lcore ID checks,
> which are limited to simple range validation and involve significant
> code duplication. Moreover, these checks do not account for whether the
> core is actually managed by the application.
>
> For the verification in cpufreq-related APIs and power QoS APIs, although
> service cores do not typically invoke these APIs, they may operate in
> polling modes where power management is required. To maintain compatibility
> with applications using service cores, the validation logic now explicitly
> allows both ROLE_RTE and ROLE_SERVICE.
>
> But the lcore ID in the pmd_mgmt library must be ROLE_RTE because it is
> mainly used together with the data plane of ethdev PMD. So use
> rte_lcore_is_enabled to verify.
>
> Key Changes:
> 1. Add lcore role verification to individual cpufreq drivers.
> 2. Introduces a unified macro in the power library to standardize lcore ID
> checks.
> 3. Moves verification logic from individualdrivers to the framework level.
> This reduces code duplication.
> 4. Allow the service cores to configure power QoS.
> 5. Use rte_lcore_is_enabled to verfify the lcore ID in pmd_mgmt.
>
> ---
> v2:
> - Allow the service cores to set power API.
>
> ---
>
> Huisong Li (15):
> eal: add interface to check if lcore is EAL managed
> power/kvm_vm: validate lcore role in cpufreq API
> power/acpi_cpufreq: validate lcore role in cpufreq API
> power/amd_pstate: validate lcore role in cpufreq API
> power/cppc_cpufreq: validate lcore role in cpufreq API
> power/intel_pstate: validate lcore role in cpufreq API
> power: add a common macro to verify lcore ID
> power/cpufreq: add the lcore ID verification to framework
> power/acpi_cpufreq: remove the verification of lcore ID
> power/amd_pstate: remove the verification of lcore ID
> power/cppc_cpufreq: remove the verification of lcore ID
> power/intel_pstate: remove the verification of lcore ID
> power/kvm_vm: remove the verification of lcore ID
> power: allow the service core to config power QoS
> power: add lcore ID check for PMD mgmt
>
> drivers/power/acpi/acpi_cpufreq.c | 65 -------------------
> drivers/power/amd_pstate/amd_pstate_cpufreq.c | 65 -------------------
> drivers/power/cppc/cppc_cpufreq.c | 65 -------------------
> .../power/intel_pstate/intel_pstate_cpufreq.c | 65 -------------------
> drivers/power/kvm_vm/kvm_vm.c | 10 ---
> lib/eal/common/eal_common_lcore.c | 11 ++++
> lib/eal/include/rte_lcore.h | 11 ++++
> lib/power/power_common.h | 7 ++
> lib/power/rte_power_cpufreq.c | 14 +++-
> lib/power/rte_power_pmd_mgmt.c | 21 +++---
> lib/power/rte_power_qos.c | 10 +--
> 11 files changed, 55 insertions(+), 289 deletions(-)
>
^ permalink raw reply
* [PATCH] net/iavf: fix VF reset race and stale ARQ message handling
From: Talluri Chaitanyababu @ 2026-05-18 5:42 UTC (permalink / raw)
To: dev, bruce.richardson, aman.deep.singh
Cc: shaiq.wani, Talluri Chaitanyababu, stable
During VF reset, multiple issues can lead to initialization instability.
The first issue is a race condition in iavf_check_vf_reset_done(),
where VFR state VFACTIVE is treated as both "reset not started" and
"reset completed". This can allow the VF to proceed before the PF
has acknowledged the reset, resulting in inconsistent initialization.
The second issue is the presence of stale messages in the Admin
Receive Queue (ARQ) after VF reset. These may include opcode 0
(VIRTCHNL_OP_UNKNOWN) or responses to commands issued before reset,
which can interfere with API negotiation and cause command mismatch
errors.
Additionally, opcode 0 messages generate excessive warning logs,
causing unnecessary noise during initialization.
The solution involves:
1. Introducing a two-phase reset wait mechanism in
iavf_check_vf_reset_done():
- Wait for RSTAT to leave VFACTIVE to confirm reset start.
- Wait for RSTAT to return to VFACTIVE or COMPLETED to confirm
reset completion.
2. Draining stale ARQ messages after admin queue initialization
and before issuing commands in iavf_init_vf().
3. Downgrading opcode 0 message logging to DEBUG level while
preserving mismatch detection for other opcodes.
4. Refactoring reset-start detection and ARQ drain logic into
helper functions (iavf_wait_for_reset_start() and
iavf_drain_arq()) to improve readability and reuse.
This ensures reliable VF reset handling and stable initialization.
Fixes: 28a1a72eac26 ("net/iavf: add VF initiated reset")
Cc: stable@dpdk.org
Signed-off-by: Talluri Chaitanyababu <chaitanyababux.talluri@intel.com>
---
drivers/net/intel/iavf/iavf_ethdev.c | 54 ++++++++++++++++++++++++++++
drivers/net/intel/iavf/iavf_vchnl.c | 16 +++++++--
2 files changed, 67 insertions(+), 3 deletions(-)
diff --git a/drivers/net/intel/iavf/iavf_ethdev.c b/drivers/net/intel/iavf/iavf_ethdev.c
index 1eca20bc9a..692e4455dc 100644
--- a/drivers/net/intel/iavf/iavf_ethdev.c
+++ b/drivers/net/intel/iavf/iavf_ethdev.c
@@ -2002,11 +2002,36 @@ iavf_dev_rx_queue_intr_disable(struct rte_eth_dev *dev, uint16_t queue_id)
return 0;
}
+/* Wait until PF acknowledges VF reset (RSTAT leaves VFACTIVE) */
+static int
+iavf_wait_for_reset_start(struct iavf_hw *hw)
+{
+ int i;
+ uint32_t rstat;
+
+ for (i = 0; i < 100; i++) {
+ rte_delay_ms(10);
+
+ rstat = IAVF_READ_REG(hw, IAVF_VFGEN_RSTAT);
+ rstat &= IAVF_VFGEN_RSTAT_VFR_STATE_MASK;
+ rstat >>= IAVF_VFGEN_RSTAT_VFR_STATE_SHIFT;
+
+ if (rstat != VIRTCHNL_VFR_VFACTIVE)
+ return 0;
+ }
+
+ return -1;
+}
+
static int
iavf_check_vf_reset_done(struct iavf_hw *hw)
{
int i, reset;
+ /* Phase 1: wait for reset to start (leave VFACTIVE) */
+ if (iavf_wait_for_reset_start(hw) != 0)
+ PMD_DRV_LOG(DEBUG, "VF reset did not start within timeout");
+
for (i = 0; i < IAVF_RESET_WAIT_CNT; i++) {
reset = IAVF_READ_REG(hw, IAVF_VFGEN_RSTAT) &
IAVF_VFGEN_RSTAT_VFR_STATE_MASK;
@@ -2517,6 +2542,31 @@ iavf_init_proto_xtr(struct rte_eth_dev *dev)
}
}
+/* Drain stale Admin Receive Queue messages after reset */
+static void
+iavf_drain_arq(struct iavf_hw *hw, struct iavf_info *vf)
+{
+ struct iavf_arq_event_info event;
+ int drain_count = 0;
+
+ memset(&event, 0, sizeof(event));
+ event.msg_buf = vf->aq_resp;
+
+ while (drain_count < IAVF_AQ_LEN) {
+ event.buf_len = IAVF_AQ_BUF_SZ;
+
+ if (iavf_clean_arq_element(hw, &event, NULL) != IAVF_SUCCESS)
+ break;
+
+ drain_count++;
+ }
+
+ if (drain_count > 0)
+ PMD_INIT_LOG(DEBUG,
+ "Drained %d stale ARQ messages",
+ drain_count);
+}
+
static int
iavf_init_vf(struct rte_eth_dev *dev)
{
@@ -2558,6 +2608,10 @@ iavf_init_vf(struct rte_eth_dev *dev)
PMD_INIT_LOG(ERR, "unable to allocate vf_aq_resp memory");
goto err_aq;
}
+
+ /* Drain stale ARQ messages after VF reset */
+ iavf_drain_arq(hw, vf);
+
if (iavf_check_api_version(adapter) != 0) {
PMD_INIT_LOG(ERR, "check_api version failed");
goto err_api;
diff --git a/drivers/net/intel/iavf/iavf_vchnl.c b/drivers/net/intel/iavf/iavf_vchnl.c
index 08dd6f2d7f..a014be8b57 100644
--- a/drivers/net/intel/iavf/iavf_vchnl.c
+++ b/drivers/net/intel/iavf/iavf_vchnl.c
@@ -296,11 +296,21 @@ iavf_read_msg_from_pf(struct iavf_adapter *adapter, uint16_t buf_len,
__func__, vpe->event);
}
} else {
- /* async reply msg on command issued by vf previously */
+ /* Async reply for previously issued VF command.
+ * Stale messages from before reset are ignored, and polling
+ * continues until the expected response is received.
+ */
result = IAVF_MSG_CMD;
if (opcode != vf->pend_cmd) {
- PMD_DRV_LOG(WARNING, "command mismatch, expect %u, get %u",
- vf->pend_cmd, opcode);
+ if (opcode == VIRTCHNL_OP_UNKNOWN)
+ PMD_DRV_LOG(DEBUG,
+ "Ignoring stale msg (opcode 0), pending cmd %u",
+ vf->pend_cmd);
+ else
+ PMD_DRV_LOG(WARNING,
+ "command mismatch, expect %u, get %u",
+ vf->pend_cmd, opcode);
+
result = IAVF_MSG_ERR;
}
}
--
2.43.0
^ permalink raw reply related
* Re: [PATCH v2 1/1] net/mana: add device reset support
From: Stephen Hemminger @ 2026-05-18 3:48 UTC (permalink / raw)
To: Wei Hu; +Cc: dev, longli, weh
In-Reply-To: <20260509060113.61686-2-weh@linux.microsoft.com>
On Fri, 8 May 2026 23:01:13 -0700
Wei Hu <weh@linux.microsoft.com> wrote:
> From: Wei Hu <weh@microsoft.com>
>
> Add support for handling hardware reset events in the MANA driver.
> When the MANA kernel driver receives a hardware service event, it
> initiates a device reset and notifies userspace via
> IBV_EVENT_DEVICE_FATAL. The DPDK driver handles this by performing
> an automatic teardown and recovery sequence.
>
> The reset flow has two phases. In the enter phase, running on the
> EAL interrupt thread, the driver transitions the device state,
> waits for data path threads to reach a quiescent state using RCU,
> stops queues, tears down IB resources, and frees per-queue MR
> caches. A control thread is then spawned to handle the exit phase:
> it waits for the hardware to recover, unregisters the interrupt
> handler, re-probes the PCI device, reinitializes MR caches, and
> restarts queues.
>
> A per-device spinlock serializes the reset path with ethdev
> operations. Operations that cannot wait (configure, queue setup)
> return -EBUSY during reset, while dev_stop and dev_close join the
> reset thread and use a blocking lock to ensure proper sequencing.
>
> Multi-process support is included: secondary processes unmap and
> remap doorbell pages via IPC during the reset enter and exit
> phases. Data path functions in both primary and secondary
> processes check the device state atomically and return early when
> the device is not active.
>
> The driver uses ethdev recovery events to notify upper layers
> (e.g. netvsc) of the reset lifecycle: RTE_ETH_EVENT_ERR_RECOVERING
> on entry, RTE_ETH_EVENT_RECOVERY_SUCCESS or
> RTE_ETH_EVENT_RECOVERY_FAILED on completion. A PCI device removal
> event callback distinguishes hot-remove from service reset.
>
> Signed-off-by: Wei Hu <weh@microsoft.com>
> ---
Lots of issues found during AI review..
Subject: Re: [PATCH v2 1/1] net/mana: add device reset support
To: Wei Hu <weh@linux.microsoft.com>
Cc: dev@dpdk.org, longli@microsoft.com, weh@microsoft.com
In-Reply-To: <20260509060113.61686-2-weh@linux.microsoft.com>
References: <20260509060113.61686-1-weh@linux.microsoft.com>
<20260509060113.61686-2-weh@linux.microsoft.com>
On Fri, 8 May 2026 23:01:13 -0700, Wei Hu wrote:
> Add support for handling hardware reset events in the MANA driver.
Several issues need attention before this can be merged.
Errors
------
1. Memory leak: priv->dev_state_qsv is never freed on close.
In mana_probe_port (non-reset path) the qsbr buffer is allocated:
priv->dev_state_qsv = rte_zmalloc_socket("mana_rcu", sz, ...);
It is only freed in the probe-failure goto label. mana_dev_close
has no corresponding rte_free, so every successful close leaks
the allocation.
2. Joinable reset thread is leaked when reset completes on its own.
Both mana_reset_thread (skip path) and mana_reset_exit_delay
(normal completion) do:
priv->reset_thread_active = false;
rte_spinlock_unlock(&priv->reset_ops_lock);
return 0;
The pthread is still joinable at this point. If mana_dev_close
runs afterward it sees active==false and skips rte_thread_join,
leaving the TCB/stack unreaped. The flag must only be cleared by
whoever actually joins the thread; otherwise the reset thread
should be detached (and then dev_close cannot wait for it, which
is its own problem). Either way the current pattern is wrong.
3. Spinlock held across blocking operations.
priv->reset_ops_lock is rte_spinlock_t and is held across:
- mana_reset_enter: mana_dev_stop, mana_dev_close,
ibv_close_device,
mana_mp_req_on_rxtx (5s IPC timeout)
- mana_reset_exit_delay: ibv_close_device, mana_pci_probe,
mana_mp_req_on_rxtx, mana_dev_start
IPC with a 5-second timeout and device probe under a spinlock is
not acceptable. Use a sleeping mutex (pthread_mutex_t initialized
with PTHREAD_PROCESS_SHARED, since priv is in shared memory), or
split the lock so the long operations run outside it.
4. pthread_mutex_init / pthread_cond_init without PTHREAD_PROCESS_SHARED.
priv is allocated with rte_zmalloc_socket, so reset_cond_mutex and
reset_cond live in hugepage memory mapped into every secondary.
The patch initializes them with NULL attributes:
pthread_mutex_init(&priv->reset_cond_mutex, NULL);
pthread_cond_init(&priv->reset_cond, NULL);
Even if only the primary uses them today, placement in shared
memory requires the process-shared attribute. Otherwise behavior
is undefined and the choice will silently break the day a
secondary starts touching these fields.
Warnings
--------
5. Secondary data path has no synchronization with the doorbell unmap.
MANA_MP_REQ_RESET_ENTER causes the secondary MP handler to munmap
proc_priv->db_page and set it to NULL. The secondary's rx_burst /
tx_burst protect themselves only with an atomic state check:
rte_rcu_qsbr_thread_online(dstate_qsv, tid);
if (state != MANA_DEV_ACTIVE || !db_page) { ... return 0; }
But the qsbr only has primary threads registered (registration
happens in mana_dev_configure, which never runs in secondary), so
thread_online/offline in secondary do not block the primary's
qsbr_check. The MP handler in secondary therefore unmaps db_page
while a peer secondary lcore can still be inside rx/tx_burst with
a stale pointer. Result: SIGSEGV on the next doorbell write.
The secondary needs its own quiescence mechanism (a per-process
qsbr or a reader-side rwlock around the data path that the MP
handler acquires before unmap).
6. Return value of rte_dev_event_callback_unregister is discarded.
In mana_intr_uninstall:
if (dev->device)
rte_dev_event_callback_unregister(dev->device->name,
mana_pci_remove_event_cb,
priv);
The API returns -EAGAIN if the callback is currently executing.
When that happens the unregister silently fails, the callback
stays on the list, and any later free of priv produces a
use-after-free the next time the EAL device-event thread
dispatches.
7. PCI-remove callback depends on a monitor nobody starts.
rte_dev_event_callback_register only wires the callback into the
dispatch table; events are only delivered when something calls
rte_dev_event_monitor_start(). The patch never calls it. Whether
this works in practice depends on the application (testpmd,
netvsc) calling it first. At minimum document the requirement;
better, have the driver start the monitor on first probe and stop
it on last remove.
8. No documentation or release-note update.
This is a substantial new feature. doc/guides/rel_notes/ and
doc/guides/nics/mana.rst should both be updated to describe the
reset lifecycle, the RTE_ETH_EVENT_ERR_RECOVERING /
RECOVERY_SUCCESS / RECOVERY_FAILED behavior, and the dependency
on rte_dev_event_monitor_start.
9. priv->reset_thread_active is bool, accessed without atomic ops.
It is read in mana_dev_stop_lock and mana_dev_close_lock before
the spinlock is acquired (intentionally, to avoid deadlock with
the reset thread), and written from at least three threads (intr
handler, reset thread, dev_close). It should be RTE_ATOMIC(bool)
with explicit ordering. This compounds issue #2.
10. mana_intr_handler races with mana_pci_remove_event_cb on dev_state.
intr_handler does:
if (state == MANA_DEV_ACTIVE) {
rte_spinlock_lock(&priv->reset_ops_lock);
mana_reset_enter(priv); /* stores RESET_ENTER */
pci_remove_event_cb stores RESET_FAILED before taking the lock.
If intr_handler reads ACTIVE, pci_remove fires (sets
RESET_FAILED) while intr_handler is waiting on the lock,
intr_handler then acquires it and overwrites RESET_FAILED with
RESET_ENTER, and the driver attempts a reset on a device that
has already been physically removed. The state-set in
pci_remove_event_cb must occur under the lock, or the
intr_handler must re-check state after taking the lock.
Info
----
11. (void *)0 is used in mp.c (proc_priv->db_page = (void *)0; and
if (proc_priv->db_page != 0)). Use NULL.
12. The hand-off of reset_ops_lock from mana_reset_enter to
mana_reset_thread to mana_reset_exit_delay (released in the
latter) is hard to follow. A short comment block at
mana_reset_enter listing which functions take the lock and which
release it would help future maintainers.
13. The widened TOCTOU window in mana_tx_burst (db_page is now read
at the top of the function rather than just before the doorbell)
is not itself a bug, but it makes #5 worse. Once #5 is fixed
via a proper reader-side primitive, consider keeping the db_page
read inside the critical section.
^ permalink raw reply
* Re: [PATCH v2] net/hns3: fix L4 checksum incorrect for tunnel packets
From: Stephen Hemminger @ 2026-05-18 3:30 UTC (permalink / raw)
To: Xingui Yang
Cc: dev, david.marchand, fengchengwen, lihuisong, liuyonglong,
kangfenglong
In-Reply-To: <20260509064753.3292074-1-yangxingui@huawei.com>
On Sat, 9 May 2026 14:47:53 +0800
Xingui Yang <yangxingui@huawei.com> wrote:
> In HIP09 simple BD mode, the driver incorrectly calculates L4_START
> position for tunnel packets. The current implementation only uses
> l2_len + l3_len without considering outer header lengths (outer_l2_len
> and outer_l3_len), causing hardware to calculate checksum from wrong
> offset.
>
> For a typical NvGRE tunnel packet with structure:
> Outer Eth(14) + Outer IPv6(40) + NvGRE(8) + Inner Eth(14) +
> Inner IPv6(40) + TCP(20)
>
> Expected L4_START: 116 bytes (from packet start to inner TCP header)
> Actual calculation: 62 bytes (missing outer headers)
>
> This results in incorrect TCP/UDP checksum for all tunnel packets when
> using simple BD mode, including NvGRE, VxLAN, GRE, GENEVE tunnels.
>
> Fix by adding outer_l2_len and outer_l3_len to L4_START calculation
> when RTE_MBUF_F_TX_TUNNEL_MASK flag is set.
>
> Fixes: 6393fc0b823c ("net/hns3: simplify hardware checksum offloading")
> Cc: stable@dpdk.org
>
> Signed-off-by: Xingui Yang <yangxingui@huawei.com>
> ---
> drivers/net/hns3/hns3_rxtx.c | 7 ++++++-
> 1 file changed, 6 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/net/hns3/hns3_rxtx.c b/drivers/net/hns3/hns3_rxtx.c
> index 573604b0cd..4d48cbdc11 100644
> --- a/drivers/net/hns3/hns3_rxtx.c
> +++ b/drivers/net/hns3/hns3_rxtx.c
> @@ -3982,6 +3982,7 @@ hns3_handle_simple_bd(struct hns3_tx_queue *txq, struct hns3_desc *desc,
> {
> #define HNS3_TCP_CSUM_OFFSET 16
> #define HNS3_UDP_CSUM_OFFSET 6
> + uint32_t l4_start;
>
> /*
> * In HIP09, NIC HW support Tx simple BD mode that the HW will
> @@ -4003,9 +4004,13 @@ hns3_handle_simple_bd(struct hns3_tx_queue *txq, struct hns3_desc *desc,
> ((m->ol_flags & RTE_MBUF_F_TX_L4_MASK) == RTE_MBUF_F_TX_TCP_CKSUM ||
> (m->ol_flags & RTE_MBUF_F_TX_L4_MASK) == RTE_MBUF_F_TX_UDP_CKSUM)) {
> /* set checksum start and offset, defined in 2 Bytes */
> + l4_start = m->l2_len + m->l3_len;
Applied to next-net; moved declaration of l4_start into that basic block.
^ permalink raw reply
* Re: [PATCH dpdk v4] net/tap: use offsets provided by rte_net_get_ptype
From: Stephen Hemminger @ 2026-05-18 3:14 UTC (permalink / raw)
To: Robin Jarry; +Cc: dev
In-Reply-To: <20260512151611.186577-2-rjarry@redhat.com>
On Tue, 12 May 2026 17:16:12 +0200
Robin Jarry <rjarry@redhat.com> wrote:
> Instead of guessing what are the proper header lengths, pass
> a rte_net_hdr_lens struct to rte_net_get_ptype and use it to get the
> proper header lengths/offsets in tap_verify_csum.
>
> This allows supporting stacked VLAN/QinQ tags and IPv6 extensions.
>
> Signed-off-by: Robin Jarry <rjarry@redhat.com>
> ---
AI patch review
Short summary suitable for replying to the patch:
The v4 patch claims to add IPv6 extension header support, but the L4
checksum path is broken for that case. The patch matches both IPV6
and IPV6_EXT in the L3 block and then falls through to
rte_ipv6_udptcp_cksum_verify(), which is documented as not supporting
extension headers. Concretely, that helper uses ipv6_hdr->proto for
the pseudo-header (which is the first extension-header type, not the
L4 protocol) and uses ipv6_hdr->payload_len as the raw-cksum length
(which over-reads past the L4 data by the size of the extensions).
Result: valid TCP/UDP over IPv6+ext packets get tagged
RX_L4_CKSUM_BAD, contradicting the commit message.
Suggested fix: keep IPV6_EXT in the L3 sanity-check block, but skip
L4 verification when l3 == RTE_PTYPE_L3_IPV6_EXT until a helper that
handles extension headers exists.
^ permalink raw reply
* Re: [PATCH v3 0/7] net/netvsc: fix VF hotplug and service reset handling
From: Stephen Hemminger @ 2026-05-18 2:59 UTC (permalink / raw)
To: Long Li; +Cc: dev, Wei Hu
In-Reply-To: <20260515192843.552762-1-longli@microsoft.com>
On Fri, 15 May 2026 12:28:34 -0700
Long Li <longli@microsoft.com> wrote:
> This series fixes several issues in the netvsc PMD's VF hot-plug retry
> logic and adds support for MANA service reset (suspend/resume) recovery.
>
> Patches 1-5 fix the VF hot-add retry path to handle Azure-specific
> timing issues: slow MANA driver probe (>100s), udev interface renames,
> asynchronous mana_ib registration, and multi-NIC staggered VF
> appearance.
>
> Patch 6 fixes per-queue stats forwarding from VF to netvsc.
>
> Patch 7 adds recovery event handling for MANA service resets, where
> the kernel suspends/resumes the VF without PCI remove.
>
> v3:
> - Patch 1: wrapped rte_eal_alarm_set lines to fix checkpatch
> line-length warning
> - Patch 4: changed "retry loop exiting" log from NOTICE to DEBUG
> to avoid noise on every successful VF re-attach
> - Patch 6: removed dead -ENOTSUP fallback to rte_eth_stats_get,
> replaced with direct -ENOTSUP return; documented caller contract
> for zeroed buffers
> - Patch 7: deferred all recovery callbacks via rte_eal_alarm_set
> consistent with INTR_RMV pattern; dropped unlocked vf_attached
> guard in recovery_failed; cancel new alarms in hn_vf_close
>
> v2:
> - Patch 1: added comment explaining why indefinite retry is safe
> - Patch 2: changed SIOCGIFHWADDR retry log to DEBUG
> - Patch 3: restored ERR log for non-ENODEV/EEXIST failures
> - Patch 4: changed per-iteration logs from NOTICE to DEBUG;
> used RTE_ETHER_ADDR_PRT_FMT macros
> - Patch 5: fixed commit message (limit 120 not 30); changed
> mac_retry log to DEBUG; explicit NULL comparisons
> - Patch 6: added comment for direct dev_ops call; added -ENOTSUP
> fallback
> - Patch 7: added dev_started check in recovery_success; added
> vf_attached guard in recovery_failed
>
> Long Li (7):
> net/netvsc: retry VF hotplug indefinitely until PCI device disappears
> net/netvsc: retry on SIOCGIFHWADDR failure during VF hotplug
> net/netvsc: retry full probe when IB device not ready during hotplug
> net/netvsc: add debug logging for VF hotplug retry
> net/netvsc: retry when no matching MAC found in net directory
> net/netvsc: forward per-queue stats from VF device
> net/netvsc: handle VF recovery events for service reset
>
> drivers/net/netvsc/hn_ethdev.c | 142 +++++++++++++++++++++----
> drivers/net/netvsc/hn_var.h | 1 +
> drivers/net/netvsc/hn_vf.c | 182 ++++++++++++++++++++++++++++++++-
> 3 files changed, 302 insertions(+), 23 deletions(-)
>
Applied to next-net
^ permalink raw reply
* Re: [PATCH] ethdev: fix pointer check in GENEVE and RAW flow copy
From: Stephen Hemminger @ 2026-05-18 2:45 UTC (permalink / raw)
To: Denis Lyulin
Cc: Ori Kam, Thomas Monjalon, Andrew Rybchenko, Ferruh Yigit,
Michael Baum, dev, stable, adrien.mazarguil
In-Reply-To: <20260507112012.119140-1-lyulin.2003@mail.ru>
On Thu, 7 May 2026 14:20:11 +0300
Denis Lyulin <lyulin.2003@mail.ru> wrote:
> When rte_flow_conv_item_spec() is called from rte_flow_conv_pattern(),
> the spec, last and mask pointers are checked separately. If the API
> is used incorrectly, the spec pointer may be NULL while last and mask
> may be valid pointers.
>
> In rte_flow_conv_item_spec() for GENVE_OPT and RAW item types the spec
> pointer is used even if the function is called to copy last or mask.
> It may cause a NULL pointer (spec) dereference.
>
> This commit adds extra check of item->spec and if it is NULL, does not
> copy further data relying on it
>
> Fixes: 841a0445442d ("ethdev: fix GENEVE option item conversion")
> Cc: michaelba@nvidia.com
> Cc: adrien.mazarguil@6wind.com
> Cc: stable@dpdk.org
>
> Signed-off-by: Denis Lyulin <lyulin.2003@mail.ru>
> ---
AI spotted a couple issues here, please address.
Warnings
========
* lib/ethdev/rte_flow.c: RAW case guard is overly broad and regresses
the LAST-without-spec path.
The new wrapper `if (spec.raw && last.raw) { ... } else { tmp = 0; }`
is too coarse. The original code only dereferences spec.raw on the
SPEC and MASK paths:
if (type == RTE_FLOW_CONV_ITEM_SPEC || // uses spec, mask
(type == RTE_FLOW_CONV_ITEM_MASK && // uses spec, last, mask
((spec.raw->length & mask.raw->length) >=
(last.raw->length & mask.raw->length))))
tmp = spec.raw->length & mask.raw->length;
else
tmp = last.raw->length & mask.raw->length; // uses last, mask only
For type == RTE_FLOW_CONV_ITEM_LAST the else branch is taken and
spec.raw is never touched. With the patch, when an item has
item->last set but item->spec == NULL, the wrapper short-circuits
to tmp = 0 and the `last` pattern is silently dropped (dst.raw->
pattern stays NULL after the struct-initializer memcpy). The
original code copied it correctly.
rte_flow_conv_pattern() calls this function with type=LAST whenever
src->last is non-NULL, regardless of src->spec, so this path is
reachable through the public conv API.
Note also that mask.raw is guaranteed non-NULL by the
`item->mask ? item->mask : &rte_flow_item_raw_mask` fallback two
lines above, so it does not need to participate in the guard.
A more surgical guard preserves the LAST behaviour, e.g.:
if (type == RTE_FLOW_CONV_ITEM_SPEC && spec.raw)
tmp = spec.raw->length & mask.raw->length;
else if (type == RTE_FLOW_CONV_ITEM_MASK && spec.raw && last.raw &&
((spec.raw->length & mask.raw->length) >=
(last.raw->length & mask.raw->length)))
tmp = spec.raw->length & mask.raw->length;
else if (last.raw)
tmp = last.raw->length & mask.raw->length;
else
tmp = 0;
Info
====
* The commit message attributes the bug only to calls from
rte_flow_conv_pattern(), but the more directly reachable path is
rte_flow_conv() with RTE_FLOW_CONV_OP_ITEM_MASK (rte_flow.c around
line 1144), which validates only item->mask and leaves item->spec
potentially NULL before invoking rte_flow_conv_item_spec() with
type=MASK. Worth mentioning so reviewers can locate the trigger.
* The GENEVE_OPT hunk uses spec.geneve_opt->option_len to size the
copy of src.geneve_opt->data, even when src is the mask (type=MASK)
or the last (type=LAST). This pre-existing assumption (that
spec.option_len describes the layout for all three) is preserved by
the patch and is out of scope here, but worth a sanity check that
the spec.option_len fallback to 0 (no copy) is the desired
behaviour when only a mask is supplied via OP_ITEM_MASK.
^ permalink raw reply
* Re: [PATCH v3 1/3] net/zxdh: optimize queue structure to improve performance
From: Stephen Hemminger @ 2026-05-18 2:20 UTC (permalink / raw)
To: Junlong Wang; +Cc: dev
In-Reply-To: <20260509062930.3254766-2-wang.junlong1@zte.com.cn>
On Sat, 9 May 2026 14:29:27 +0800
Junlong Wang <wang.junlong1@zte.com.cn> wrote:
> Reorganize structure fields for better cache locality.
> Remove RX software ring (sw_ring) to reduce memory allocation and
> copy.
>
> Signed-off-by: Junlong Wang <wang.junlong1@zte.com.cn>
> ---
Looks good.
Some AI comments:
[PATCH v3 1/3] net/zxdh: optimize queue structure to improve performance
Warning: silent bug fix
This patch quietly fixes a real bug in zxdh_queue_enable_intr(). Upstream has:
static inline void
zxdh_queue_enable_intr(struct zxdh_virtqueue *vq)
{
if (vq->vq_packed.event_flags_shadow == ZXDH_RING_EVENT_FLAGS_DISABLE) {
vq->vq_packed.event_flags_shadow = ZXDH_RING_EVENT_FLAGS_DISABLE;
vq->vq_packed.ring.driver->desc_event_flags = vq->vq_packed.event_flags_shadow;
}
}
The function checks for DISABLE and then sets to DISABLE — interrupts are never enabled. The patch corrects both occurrences to ENABLE. That fix is not mentioned in the commit message, and it has no Fixes: tag or Cc: stable@dpdk.org. Please split it into its own patch ahead of the structure reorganization so it can be backported.
The commit message also omits other non-trivial changes: removal of zxdh_mb(), and the inlining of zxdh_queue_notify() so it no longer dispatches through ZXDH_VTPCI_OPS()->notify_queue. Worth a sentence each.
^ permalink raw reply
* Re: [PATCH v3 3/3] net/zxdh: optimize Tx xmit pkts performance
From: Stephen Hemminger @ 2026-05-18 2:22 UTC (permalink / raw)
To: Junlong Wang; +Cc: dev
In-Reply-To: <20260509062930.3254766-4-wang.junlong1@zte.com.cn>
On Sat, 9 May 2026 14:29:29 +0800
Junlong Wang <wang.junlong1@zte.com.cn> wrote:
> Add simple Tx xmit functions (zxdh_xmit_pkts_simple)
> for single-segment packet xmit.
>
> Signed-off-by: Junlong Wang <wang.junlong1@zte.com.cn>
> ---
Some AI review feedback.
PATCH v3 3/3] net/zxdh: optimize Tx xmit pkts performance
Error: NULL pointer dereference in pkt_padding()
hdr = (struct zxdh_net_hdr_dl *)rte_pktmbuf_prepend(cookie, hdr_len);
rte_memcpy(hdr, net_hdr_dl, hdr_len);
rte_pktmbuf_prepend() returns NULL when headroom is insufficient. The existing zxdh_xmit_pkts_packed() path guards its push fast-path with txm->data_off >= ZXDH_DL_NET_HDR_SIZE and falls back to the indirect path when that fails. The simple Tx path has no such guard; any mbuf submitted with headroom < hw->dl_net_hdr_len will crash here.
Add a NULL check, or screen mbufs with insufficient headroom in zxdh_xmit_pkts_simple() before calling submit_to_backend_simple().
Error: out-of-bounds read in zxdh_xmit_pkts_simple() stats loop
if ((vq->vq_avail_idx + nb_pkts) >= vq->vq_nentries) {
nb_tx = vq->vq_nentries - vq->vq_avail_idx;
nb_tx_left = nb_pkts - nb_tx;
submit_to_backend_simple(vq, tx_pkts, nb_tx);
...
tx_pkts += nb_tx;
}
if (nb_tx_left) {
submit_to_backend_simple(vq, tx_pkts, nb_tx_left);
...
}
zxdh_queue_notify(vq);
txvq->stats.packets += nb_pkts;
for (nb_tx = 0; nb_tx < nb_pkts; nb_tx++)
zxdh_update_packet_stats(&txvq->stats, tx_pkts[nb_tx]);
When the ring wraps within a burst, tx_pkts is advanced past the first chunk. The stats loop then walks nb_pkts entries from the advanced pointer, reading past the end of the caller's mbuf array. The first chunk's per-packet stats are also skipped.
Use a separate cursor for the submit calls and iterate the stats loop over the original tx_pkts, or accumulate per-packet stats inside each submit step.
^ permalink raw reply
* Re: [PATCH v15 00/11] net/sxe2: fix logic errors and address feedback
From: Stephen Hemminger @ 2026-05-18 0:02 UTC (permalink / raw)
To: liujie5; +Cc: dev
In-Reply-To: <20260516074618.2343883-1-liujie5@linkdatatechnology.com>
On Sat, 16 May 2026 15:46:07 +0800
liujie5@linkdatatechnology.com wrote:
> From: Jie Liu <liujie5@linkdatatechnology.com>
>
> This patch set addresses the feedback received on the v10 submission
> for the sxe2 PMD. The primary focus is on fixing vector path selection,
> ensuring memory safety during mbuf initialization, and cleaning up
> redundant logic in the configuration functions.
Driver is in much better shape now.
AI is still finding a few things.
Review of [PATCH v15 00/11] sxe2 PMD
====================================
Overall comments
----------------
Most of the issues called out in the v13 review have been
addressed in v15:
- Inverted "if (!ret)" error checks in sxe2_dev_pci_map_init()
are now "if (ret)".
- Non-ASCII characters in sxe2_net_map_addr_info_pf[] removed.
- rte_ticketlock_t around blocking ioctl() replaced with
pthread_mutex_t.
- Driver-private u8/s8/u16/... typedefs, __le*/__be* aliases,
sxe2_errno.h, STATIC macro, and the reinvented
LIST_FOR_EACH_ENTRY / COMPILER_BARRIER / sxe2_*_lock /
udelay/mdelay/msleep / __iomem / IS_ERR helpers are gone.
- "MTU update = Y" claim in the features matrix dropped;
"Free Tx mbuf on demand = Y" is now backed by the new
.tx_done_cleanup callback added in patch 11.
- Redundant queue_idx bounds checks at the top of
rx_queue_setup / tx_queue_setup removed.
- "reseted" log-message typo fixed.
What remains:
Patch 11/11: net/sxe2: implement Tx done cleanup
------------------------------------------------
Error: NULL pointer dereference of tx_queue parameter.
sxe2_tx_done_cleanup() dereferences txq before checking it for
NULL:
int32_t sxe2_tx_done_cleanup(void *tx_queue, uint32_t free_cnt)
{
struct sxe2_tx_queue *txq = (struct sxe2_tx_queue *)tx_queue;
struct sxe2_adapter *adapter = txq->vsi->adapter; /* segfault if txq == NULL */
int32_t ret;
if (txq == NULL) { /* too late */
ret = 0;
goto l_end;
}
...
The NULL check must run before any field access:
struct sxe2_tx_queue *txq = tx_queue;
struct sxe2_adapter *adapter;
if (txq == NULL)
return 0;
adapter = txq->vsi->adapter;
...
Patch 03/10 -> 09/10: vector mode probe gated on RTE_PROC_PRIMARY
-----------------------------------------------------------------
Warning: sxe2_rx_mode_func_set() runs the vector capability probe
only in the primary process:
if (rte_eal_process_type() == RTE_PROC_PRIMARY) {
ret = sxe2_rx_vec_support_check(dev, &vec_flags);
...
}
but sxe2_dev_init() calls sxe2_rx_mode_func_set(dev) and
sxe2_tx_mode_func_set(dev) from the secondary-process branch as
well. In a secondary, the vector probe is skipped, rx_mode_flags
stays 0, and dev->rx_pkt_burst lands on
sxe2_rx_pkts_scattered{,_split}. Since dev->rx_pkt_burst is
per-rte_eth_dev and re-written by whichever process attaches
last, the effective Rx burst mode depends on attach ordering.
The same pattern was flagged in v13 and is unchanged in v15.
Either run the same probe in secondaries, or share the primary's
decision through rte_eth_dev_data so secondaries inherit it.
Patch 03/10: common/sxe2: add sxe2 basic structures
---------------------------------------------------
Warning: macro identifier BITS_TO_uint32_t in sxe2_osal.h. This
is the visible result of a wholesale "u32 -> uint32_t" rename
applied to identifier text without manual review. The macro is
unused in the rest of the driver (the only mentions are the
definition itself and a later rename of the right-hand side),
so the cleanest fix is to delete it. If a "bits to 32-bit-word
count" macro is wanted later, name it BITS_TO_U32 or use
RTE_DIM/RTE_ALIGN_MUL_CEIL idioms.
Patch 09/11: drivers: add data path for Rx and Tx
-------------------------------------------------
Info: in sxe2_tx_pkts() the exit chain remains
l_exit_logic:
if (tx_num == 0)
goto l_end;
goto l_end_of_tx;
l_end_of_tx:
SXE2_PCI_REG_WRITE_WC(...);
The unconditional "goto l_end_of_tx;" immediately before the
"l_end_of_tx:" label is dead — fall through. The same pattern
appears in sxe2_rx_mode_func_set() at the end:
dev->rx_pkt_burst = sxe2_rx_pkts_scattered;
goto l_end;
l_end:
PMD_LOG_DEBUG(...);
Both can simply be deleted.
Series hygiene
--------------
Info: several identifiers are introduced with typos in one patch
and corrected in a later patch within the same series:
- sxe2_commoin_inited (patch 03) -> sxe2_common_inited (patch 05)
- sxe2_drv_dev_handshark (patch 03) -> sxe2_drv_dev_handshke
(patch 04). The renamed form is still a typo (missing 'a'
between 'h' and 'k'). Log strings around it were corrected
to "handshake" in patch 05; the function name was not.
- Dead "if (ret != 0)" after two void-returning
sxe2_{rx,tx}_mode_func_set() calls is added in patch 08 and
removed again in patch 09.
Each commit needs to be independently correct so that "git
bisect" lands on meaningful states. Renaming an exported
internal symbol (RTE_EXPORT_INTERNAL_SYMBOL(sxe2_drv_dev_handshke))
mid-series also churns the auto-generated version map. Please
collapse these into the patches that introduce the names, and
fix sxe2_drv_dev_handshke -> sxe2_drv_dev_handshake.
^ permalink raw reply
* Re: [PATCH v4 05/20] net/txgbe: fix inaccuracy in Tx rate limiting
From: Stephen Hemminger @ 2026-05-17 23:40 UTC (permalink / raw)
To: Zaiyu Wang; +Cc: dev, stable, Jiawen Wu
In-Reply-To: <20260511103604.19724-6-zaiyuwang@trustnetic.com>
On Mon, 11 May 2026 18:35:47 +0800
Zaiyu Wang <zaiyuwang@trustnetic.com> wrote:
> Amber-lite NIC's TX rate limiting has large deviations for small
> packets. To address this, the following changes are made:
>
> 1. Set TDM_RL_ADJ (0x1820c) to 21B (includes 7B Ethernet preamble,
> 1B SFD, 1B EFD, and 12B IPG).
> 2) Remove the rate offset in the driver (e.g., 105 / 100, a rough
> compensation value from Linux kernel driver tests).
>
> After these changes, accuracy deviation for 64B packets is within
> ~5%, while large packets show lower deviation.
>
> Fixes: a309ab43acf3 ("net/txgbe: support Tx queue rate limiting for Amber-Lite")
> Cc: stable@dpdk.org
>
> Signed-off-by: Zaiyu Wang <zaiyuwang@trustnetic.com>
> ---
AI feedback on this one. Looks ok as is but worth looking at.
# DPDK Patch Review: net/txgbe: fix inaccuracy in Tx rate limiting
## Summary
This patch improves TX rate limiting accuracy for Amber-lite NIC by:
1. Setting TDM_RL_ADJ register to 21B (Ethernet framing overhead)
2. Removing the 105/100 compensation factor
## Review Findings
### **Errors**
None found.
### **Warnings**
**1. TDM_RL_ADJ write placement causes per-queue reconfiguration**
The `TXGBE_TDM_RL_ADJ` register write is inside the per-queue rate limit configuration loop, causing the same global register to be written every time any queue's rate limit is set. This register appears to be a global hardware adjustment parameter (affects all queues), not a per-queue setting.
**Suggested fix:** Move the register write outside the queue configuration context, ideally to device initialization or as a one-time setup when rate limiting is first enabled:
```c
/* Set once during device init or first rate limit config */
static bool rl_adj_initialized = false;
if (!rl_adj_initialized) {
wr32(hw, TXGBE_TDM_RL_ADJ, 21);
rl_adj_initialized = true;
}
wr32(hw, TXGBE_TDM_RL_QUEUE_IDX, queue_idx);
```
Or better yet, move it to a device initialization function if this is a hardware parameter that should be set once at device start.
**2. Magic number 21 lacks explanation**
The value 21 is documented in the commit message but not in the code. Future maintainers will not see the breakdown (7B preamble + 1B SFD + 1B EFD + 12B IPG).
**Suggested fix:** Add a comment or define:
```c
/* Ethernet framing overhead: 7B preamble + 1B SFD + 1B EFD + 12B IPG */
#define TXGBE_TDM_RL_ADJ_OVERHEAD 21
wr32(hw, TXGBE_TDM_RL_ADJ, TXGBE_TDM_RL_ADJ_OVERHEAD);
```
**3. Missing release notes update**
This is a fix that changes hardware behavior and improves accuracy. Users may observe different TX rates after upgrading. The change should be documented in release notes to inform users of the accuracy improvement.
**Suggested fix:** Add an entry to `doc/guides/rel_notes/release_26_11.rst` (or current release):
```rst
* **net/txgbe: Improved TX rate limiting accuracy for Amber-lite NIC.**
Fixed TX rate limiting accuracy for 64-byte packets from ~20% deviation
to within ~5% by correcting hardware framing overhead configuration.
```
### **Info**
**1. Consider validating tx_rate is non-zero**
The code computes `factor_int = link_speed / tx_rate`. If `tx_rate` is somehow zero (due to caller error or invalid input), this would cause division by zero. The existing code may have validation elsewhere, but defensive programming would check this.
```c
if (tx_rate == 0) {
PMD_DRV_LOG(ERR, "TX rate cannot be zero");
return -EINVAL;
}
```
**2. Variable scope: factor_fra could be declared at initialization**
The variable `factor_fra` is declared but not initialized, then assigned in both branches of the conditional. C99 style would allow:
```c
u32 factor_fra = (frac * 16384) / 10000;
if (factor_fra >= 8192)
factor_fra = 0;
```
This is stylistic only; the existing code is correct.
^ permalink raw reply
* Re: [PATCH v4 17/20] net/txgbe: fix get module info operation
From: Stephen Hemminger @ 2026-05-17 23:53 UTC (permalink / raw)
To: Zaiyu Wang; +Cc: dev, stable, Jiawen Wu
In-Reply-To: <20260511103604.19724-18-zaiyuwang@trustnetic.com>
On Mon, 11 May 2026 18:35:59 +0800
Zaiyu Wang <zaiyuwang@trustnetic.com> wrote:
> The original I2C access flow in the module information retrieval
> process was flawed. Correct the implementation to properly fetch
> module info.
>
> Fixes: abf042d32b39 ("net/txgbe: add Amber-Lite 25G/40G NICs")
> Cc: stable@dpdk.org
>
> Signed-off-by: Zaiyu Wang <zaiyuwang@trustnetic.com>
> ---
More errors spotted in AI review
=== Patch 17/20 ===
# DPDK Patch Review: net/txgbe: fix get module info operation
## Summary
This patch refactors the module information retrieval code for the txgbe driver. The changes add proper identifier detection for different SFP/QSFP module types and improve the I2C access flow.
---
## ERRORS
### 1. Resource leak on error path after lock acquisition
**File:** `drivers/net/txgbe/txgbe_ethdev.c`
**Location:** Lines 5368-5382
The code acquires a software semaphore (`TXGBE_MNGSEM_SWPHY`) at line 5368 but returns directly with `-EIO` at line 5382 without releasing the lock.
```c
status = hw->mac.acquire_swfw_sync(hw, TXGBE_MNGSEM_SWPHY);
if (status)
return -EBUSY;
if (hw->mac.type == txgbe_mac_aml40) {
status = hw->phy.read_i2c_sff8636(hw, 0,
TXGBE_SFF_IDENTIFIER,
&identifier);
} else {
status = hw->phy.read_i2c_eeprom(hw,
TXGBE_SFF_IDENTIFIER,
&identifier);
}
if (status != 0)
goto ERROR_IO; /* GOOD - releases lock */
```
However, the early returns at lines 5358-5365 before the lock is acquired would bypass the lock entirely if the pre-checks fail. While these don't leak the lock, they create an inconsistent state-checking flow.
**Suggested fix:** Move the presence checks after the lock acquisition to ensure consistent locking semantics:
```c
status = hw->mac.acquire_swfw_sync(hw, TXGBE_MNGSEM_SWPHY);
if (status)
return -EBUSY;
/* Check module presence after acquiring lock */
if (hw->mac.type == txgbe_mac_aml40) {
value = rd32(hw, TXGBE_GPIOEXT);
if (value & TXGBE_SFP1_MOD_PRST_LS) {
status = -EIO;
goto ERROR_IO;
}
}
if (hw->mac.type == txgbe_mac_aml) {
value = rd32(hw, TXGBE_GPIOEXT);
if (value & TXGBE_SFP1_MOD_ABS_LS) {
status = -EIO;
goto ERROR_IO;
}
}
```
---
## WARNINGS
### 1. Implicit comparison with zero
**File:** `drivers/net/txgbe/txgbe_ethdev.c`
**Location:** Multiple occurrences (lines 5383, 5389, 5399, 5424)
The code uses `if (status != 0)` consistently, which is correct. However, there is one instance that needs verification:
```c
if (status)
return -EBUSY;
```
**Suggested fix:** For consistency with the rest of the function:
```c
if (status != 0)
return -EBUSY;
```
### 2. Mixed type usage: `u8` vs `uint8_t`
**File:** `drivers/net/txgbe/txgbe_ethdev.c`
**Location:** Lines 5350-5351, others use `uint8_t`
```c
u8 identifier = 0;
u8 sff8636_rev = 0;
```
While `u32 value;` appears at line 5353, the rest of the function uses `uint8_t` and `uint32_t`. DPDK coding standards prefer explicit-width types with the `_t` suffix.
**Suggested fix:**
```c
uint8_t identifier = 0;
uint8_t sff8636_rev = 0;
uint32_t value;
```
### 3. Unnecessary variable initialization
**File:** `drivers/net/txgbe/txgbe_ethdev.c`
**Location:** Lines 5350-5351
```c
u8 identifier = 0;
u8 sff8636_rev = 0;
```
Both variables are assigned before use via the `read_i2c_*` functions. Initializing to 0 defeats uninitialized-variable warnings.
**Suggested fix:**
```c
uint8_t identifier;
uint8_t sff8636_rev;
```
### 4. Release notes required
This patch fixes a bug (I2C access flow) and should be documented in the current release notes under "Fixed Issues" for the txgbe driver, per the contribution guidelines for bug fixes.
---
## INFO
### 1. Code organization improvement
The new code structure with a switch statement based on `identifier` is clearer than the original logic. This is a good refactoring that makes the code easier to maintain.
### 2. Error message clarity
The error messages "I2C IO ERROR" and "SFF Module Type not recognized" are informative. Consider whether these should include additional context (e.g., the identifier value for unrecognized types) to aid debugging.
---
## POSITIVE OBSERVATIONS
1. **Proper lock cleanup:** The `ERROR_IO` label correctly releases the semaphore before returning, which is the right pattern for error handling with acquired resources.
2. **Consistent error propagation:** The function properly returns `-EIO`, `-EINVAL`, and `-EBUSY` based on the failure type.
3. **Cc: stable@dpdk.org included:** Appropriate for a bug fix that should be backported.
---
## SUMMARY OF REQUIRED CHANGES
**Must fix (Errors):**
1. Move module presence checks after lock acquisition to ensure lock is held during all hardware access and properly released on all error paths
**Should fix (Warnings):**
1. Use explicit comparison `status != 0` instead of implicit `if (status)`
2. Use `uint8_t` and `uint32_t` instead of `u8` and `u32`
3. Remove unnecessary initialization of `identifier` and `sff8636_rev`
4. Add release notes entry for this bug fix
============================================================
^ permalink raw reply
* Re: [PATCH v4 18/20] net/txgbe: fix get EEPROM operation
From: Stephen Hemminger @ 2026-05-17 23:54 UTC (permalink / raw)
To: Zaiyu Wang; +Cc: dev, stable, Jiawen Wu
In-Reply-To: <20260511103604.19724-19-zaiyuwang@trustnetic.com>
On Mon, 11 May 2026 18:36:00 +0800
Zaiyu Wang <zaiyuwang@trustnetic.com> wrote:
> The original I2C access flow in the module information retrieval
> process was flawed. Correct the implementation to properly fetch
> module info.
>
> Fixes: abf042d32b39 ("net/txgbe: add Amber-Lite 25G/40G NICs")
> Cc: stable@dpdk.org
>
> Signed-off-by: Zaiyu Wang <zaiyuwang@trustnetic.com>
> ---
I agree with AI that u8, u16 should not be used except in base/ code.
# Review of Patch 18/20: net/txgbe: fix get EEPROM operation
## Errors
### 1. Wrong integer types used for kernel/hardware types
The code uses kernel-style type names (`u32`, `u8`, `u16`) instead of standard C types or DPDK types. This is inconsistent with DPDK coding standards which require standard C types.
**Location:**
```c
u32 value;
u8 identifier = 0;
u16 offset;
u8 page = 0;
```
**Fix:** Use standard C types:
```c
uint32_t value;
uint8_t identifier = 0;
uint16_t offset;
uint8_t page = 0;
```
### 2. Resource leak on error path
The function acquires a semaphore with `hw->mac.acquire_swfw_sync()` but fails to release it if `info->length == 0` check fails.
**Location:**
```c
if (info->length == 0)
return -EINVAL;
status = hw->mac.acquire_swfw_sync(hw, TXGBE_MNGSEM_SWPHY);
```
**Fix:** The `info->length == 0` check should be moved before the semaphore acquisition, OR the error path should release the semaphore:
```c
if (info->length == 0)
return -EINVAL;
status = hw->mac.acquire_swfw_sync(hw, TXGBE_MNGSEM_SWPHY);
if (status)
return -EBUSY;
```
This is actually correct as-is since the check happens before acquisition. **NOT AN ERROR.**
### 3. Implicit comparison for boolean
```c
if (identifier == TXGBE_SFF_IDENTIFIER_SFP)
is_sfp = true;
```
This uses an integer comparison to set a boolean, which is fine. However, the boolean `is_sfp` is used directly in conditionals later, which is acceptable for `bool` types. **NOT AN ERROR.**
## Warnings
### 1. Missing bounds check on page calculation
The `page` variable is incremented in a loop without any upper bound check. While the loop is bounded by `info->length`, there's no verification that `page` doesn't exceed valid page numbers.
**Location:**
```c
while (offset >= RTE_ETH_MODULE_SFF_8436_LEN) {
offset -= RTE_ETH_MODULE_SFF_8436_LEN / 2;
page++;
}
```
**Suggestion:** Add bounds check on page number before use, or verify that `info->length` cannot cause page overflow:
```c
while (offset >= RTE_ETH_MODULE_SFF_8436_LEN) {
offset -= RTE_ETH_MODULE_SFF_8436_LEN / 2;
page++;
}
if (page > MAX_VALID_PAGE) {
status = -EINVAL;
goto ERROR_IO;
}
```
### 2. Buffer initialization redundancy
`memset(data, 0, info->length)` clears the buffer, but then every byte is written in the loop. The memset may be needed for partial reads on error, but the error path returns without partial data, making this initialization unnecessary overhead.
**Location:**
```c
memset(data, 0, info->length);
for (i = info->offset; i < info->offset + info->length; i++) {
...
data[i - info->offset] = databyte;
}
```
**Consideration:** If partial reads are acceptable (keeping what was read before error), the memset is appropriate. Otherwise, it's unnecessary.
### 3. Inconsistent coding style with bool keyword
The patch uses C99 `bool` type (`bool is_sfp = false`), which is good practice per guidelines. However, verify this is consistently available (requires `<stdbool.h>` which should be included).
### 4. Error path could leak partial data
On error, the function returns without clearing the potentially partially-filled buffer. If the caller expects zeroed data on error, this could be an information leak (though likely harmless in this context).
## Info
### 1. Magic number in condition
```c
if (page == 0 || !(data[0x2] & 0x4))
```
The `0x2` offset and `0x4` bitmask are magic numbers that should be defined as named constants for clarity.
**Suggestion:**
```c
#define SFF8636_STATUS_OFFSET 0x2
#define SFF8636_FLAT_MEM_BIT 0x4
if (page == 0 || !(data[SFF8636_STATUS_OFFSET] & SFF8636_FLAT_MEM_BIT))
```
### 2. Potential integer overflow in loop bound
```c
for (i = info->offset; i < info->offset + info->length; i++)
```
If `info->offset + info->length` overflows (though unlikely with typical EEPROM sizes), the loop condition becomes incorrect. Consider bounds validation before the loop.
---
## Summary
**Critical issues:** 1 (wrong type names - must use C standard types)
**Important warnings:** 2 (missing page bounds check, potential partial data exposure)
**Style/clarity issues:** 2 (magic numbers, redundant memset)
The most important fix is changing the type declarations from `u8`/`u16`/`u32` to `uint8_t`/`uint16_t`/`uint32_t`.
^ permalink raw reply
* Re: [PATCH v4 07/20] net/txgbe: fix Tx desc free logic
From: Stephen Hemminger @ 2026-05-17 23:44 UTC (permalink / raw)
To: Zaiyu Wang; +Cc: dev, stable, Jiawen Wu
In-Reply-To: <20260511103604.19724-8-zaiyuwang@trustnetic.com>
On Mon, 11 May 2026 18:35:49 +0800
Zaiyu Wang <zaiyuwang@trustnetic.com> wrote:
> On some server environments, this driver caused TDM non-fatal errors
> or PCIe request errors during Tx operation
>
> In Amber-Lite NIC's Tx head write-back mode, the hardware periodically
> writes back a head index pointing to the next descriptor it is adout
> to process in Tx ring. All descriptors before the head are considered
> processed by hardware and can be safely freed by the driver.
>
> The root cause is that the driver can safely free a batch of descriptors
> only when the hardware's write-back head pointer has advanced beyond all
> descriptors in that batch, meaning they have all been processed by the
> hardware. If the driver frees a descriptor before the hardware has
> finished processing it, invalid memory access may occur, leading to the
> observed bug.
>
> To fix the issue, correct the boundary check in all three Tx cleanup
> functions, each of which was missing the proper condition to prevent
> freeing unprocessed descriptors.
>
> Fixes: 8ada71d0bb7f ("net/txgbe: add Tx head write-back mode for Amber-Lite")
> Cc: stable@dpdk.org
>
> Signed-off-by: Zaiyu Wang <zaiyuwang@trustnetic.com>
> ---
Lots of AI feedback on this one.
# Review of DPDK Patch: net/txgbe: fix Tx desc free logic
## Errors
### 1. Shared variable access without atomics (3 locations)
**Lines affected:**
- `drivers/net/txgbe/txgbe_rxtx.c:101` (txgbe_tx_free_bufs)
- `drivers/net/txgbe/txgbe_rxtx.c:655` (txgbe_xmit_cleanup)
- `drivers/net/txgbe/txgbe_rxtx_vec_common.h:98` (txgbe_tx_free_bufs)
```c
volatile uint16_t head = (uint16_t)*txq->headwb_mem;
```
**Problem:** The code reads a hardware-updated value (`*txq->headwb_mem`) using a `volatile` qualifier. The `volatile` keyword does NOT provide atomicity or memory ordering guarantees between the hardware and CPU. On some architectures, reading a `uint16_t` from DMA-accessible memory without proper barriers or atomic operations can result in torn reads, stale cached values, or reordering issues.
**Fix:** Use atomic load with appropriate memory ordering:
```c
uint16_t head = rte_atomic_load_explicit(
(uint16_t *)txq->headwb_mem,
rte_memory_order_acquire);
```
The `acquire` ordering ensures that all descriptor writes by the hardware are visible to the CPU after reading the head pointer. This is critical for correctness in hardware write-back scenarios.
Note: The `headwb_mem` field type may need adjustment to be an `rte_atomic` type or cast appropriately.
---
### 2. Whitespace/indentation error in vec_common.h
**Line:** `drivers/net/txgbe/txgbe_rxtx_vec_common.h:97-98`
```c
tx_last_dd -= txq->nb_tx_desc;
volatile uint16_t head = (uint16_t)*txq->headwb_mem;
```
**Problem:** Inconsistent indentation. The `volatile uint16_t head` declaration is indented with tabs-only (correct), but it visually appears to be part of a removed block based on the diff context showing the previous line had different indentation.
**Fix:** Verify the indentation matches the surrounding code block. Based on the diff, this appears correct after the change, but confirm the tabs align with the function's indentation level.
---
## Warnings
### 1. Logic change correctness verification needed
The patch adds three new early-return conditions checking if `head == tx_next_dd`. While this appears correct (hardware hasn't advanced past the point we want to free to), the logic for wrap-around cases should be verified:
```c
if (txq->tx_next_dd == head)
return 0;
else if (txq->tx_next_dd > head && head > tx_last_dd)
return 0;
else if (tx_last_dd > txq->tx_next_dd &&
(head > tx_last_dd || head < txq->tx_next_dd))
return 0;
```
**Concern:** The wrap-around logic assumes the ring wraps at `nb_tx_desc`. The condition `tx_last_dd > txq->tx_next_dd` handles the wrap case, but verify that:
- `tx_last_dd` calculation correctly wraps (it does: `tx_last_dd -= txq->nb_tx_desc`)
- The inequality checks are strict `>` not `>=` consistently
The existing code appears correct, but this is complex boundary logic that warrants testing, especially the second `else if` condition in wrap-around scenarios.
---
### 2. Missing const qualifier on read-only atomic load result
**All three locations:** The `head` variable is read from hardware but never modified:
```c
volatile uint16_t head = (uint16_t)*txq->headwb_mem;
```
**Suggestion:** Once converted to atomic operations, declare as `const`:
```c
const uint16_t head = rte_atomic_load_explicit(...);
```
This documents that `head` is never modified after the initial read and prevents accidental writes.
---
## Info
### 1. Consider helper function for repeated logic
The same boundary-check logic appears in three separate functions with only minor variations. Consider extracting it to a static inline helper:
```c
static inline bool
txgbe_hw_head_in_range(uint16_t hw_head, uint16_t start,
uint16_t end, uint16_t ring_size)
{
if (start == hw_head)
return false;
if (start > hw_head)
return hw_head <= end;
else /* wrap case */
return hw_head > end || hw_head < start;
}
```
This would reduce code duplication and make the logic easier to test and maintain.
---
### 2. Comment explaining write-back head semantics
The commit message mentions "Tx head write-back mode" but the code lacks comments explaining the hardware behavior. Consider adding:
```c
/*
* In Amber-Lite NIC Tx head write-back mode, hardware periodically
* updates *headwb_mem with the index of the next descriptor to process.
* Descriptors before this head pointer are safe to free.
*/
const uint16_t head = rte_atomic_load_explicit(...);
```
---
## Summary
**Must fix (Errors):**
1. Replace `volatile` read with `rte_atomic_load_explicit()` with `rte_memory_order_acquire` ordering in all three locations
**Should fix (Warnings):**
1. Add `const` qualifier to the loaded `head` value after converting to atomics
2. Verify wrap-around boundary logic with testing (appears correct but is complex)
**Consider (Info):**
1. Extract repeated boundary-check logic to a helper function
2. Add comments documenting hardware write-back semantics
The core fix (adding the `head == tx_next_dd` check) appears correct and addresses the described bug. The primary issue is the use of `volatile` instead of proper atomic operations for accessing hardware-updated shared memory.
^ permalink raw reply
* Re: [PATCH v4 09/20] net/txgbe: fix link flow control config for Sapphire
From: Stephen Hemminger @ 2026-05-17 23:46 UTC (permalink / raw)
To: Zaiyu Wang; +Cc: dev, stable, Jiawen Wu, Ferruh Yigit
In-Reply-To: <20260511103604.19724-10-zaiyuwang@trustnetic.com>
On Mon, 11 May 2026 18:35:51 +0800
Zaiyu Wang <zaiyuwang@trustnetic.com> wrote:
> SP chips have a hardware bug preventing XON flow control support,
> so the driver disables it.
>
> Fixes: 69ce8c8a4ce3 ("net/txgbe: support flow control")
> Cc: stable@dpdk.org
>
> Signed-off-by: Zaiyu Wang <zaiyuwang@trustnetic.com>
> ---
Comment vs code mismatch
# Review of Patch 9/20: net/txgbe: fix link flow control config for Sapphire
## Errors
None.
## Warnings
### W1: Conditional logic inverted (line 1042-1043)
The comment states "SP doesn't support xon" but the condition enables XON for `txgbe_mac_aml` and `txgbe_mac_aml40`, which are **not** SP chips based on typical wangxun naming. The logic appears inverted.
**Why this matters**: If SP chips are `txgbe_mac_sp` or similar (not aml/aml40), then XON is being enabled on the wrong hardware, causing the bug the patch claims to fix.
**Suggested fix**: Verify the hardware type constants. If SP chips are a different enum value, the condition should be:
```c
/* SP doesn't support xon */
if (hw->mac.type != txgbe_mac_sp && hw->mac.type != txgbe_mac_sp40)
fcrtl |= TXGBE_FCWTRLO_XON;
```
Or if the current condition is correct, the comment should read "Only AML supports xon" to match the code.
### W2: Inconsistent capitalization in comment (line 1042)
Comment uses lowercase "xon" while the code uses uppercase `XON` in the macro name `TXGBE_FCWTRLO_XON`.
**Suggested fix**:
```c
/* SP doesn't support XON */
```
## Info
None.
---
**Summary**: The primary concern is the apparent mismatch between the comment and the conditional logic. The patch claims to disable XON for SP chips, but the condition enables it for aml/aml40. This requires verification that either the comment is wrong or the condition should be negated/use different constants.
^ permalink raw reply
* Re: [PATCH v4 12/20] net/txgbe: fix link stability for 25G NIC
From: Stephen Hemminger @ 2026-05-17 23:49 UTC (permalink / raw)
To: Zaiyu Wang; +Cc: dev, stable, Jiawen Wu
In-Reply-To: <20260511103604.19724-13-zaiyuwang@trustnetic.com>
On Mon, 11 May 2026 18:35:54 +0800
Zaiyu Wang <zaiyuwang@trustnetic.com> wrote:
> The link was previously configured via firmware, but this approach
> resulted in unstable link behavior. To resolve the issue, re-add the
> PHY configuration flow directly into the driver.
>
> Fixes: ead3616f630d ("net/txgbe: support PHY configuration via SW-FW mailbox")
> Cc: stable@dpdk.org
>
> Signed-off-by: Zaiyu Wang <zaiyuwang@trustnetic.com>
> ---
Lots of AI review feedback here:
# Review of Patch 12/20: net/txgbe: fix link stability for 25G NIC
## Errors
### 1. Use-after-free potential in txgbe_e56_rxs_calib_adapt_seq (CORRECTNESS)
**File:** `drivers/net/txgbe/base/txgbe_e56.c`
**Location:** Lines ~1600-1650
The function calls `txgbe_e56_rxs_osc_init_for_temp_track_range(hw, speed)` which can return `TXGBE_ERR_TIMEOUT`. When the timeout path at line ~1115 is taken, it writes to `E56PHY_PMD_CFG_0_ADDR` to disable RX and then returns `-1`. However, the caller `txgbe_e56_rxs_calib_adapt_seq` continues execution and accesses PHY registers without verifying the hardware state is valid. If the initialization timed out, subsequent register accesses may be unreliable.
**Suggested fix:**
```c
status = txgbe_e56_rxs_osc_init_for_temp_track_range(hw, speed);
if (status != 0)
return status; /* Propagate timeout or error immediately */
```
### 2. Resource leak: lock not released on error path (CORRECTNESS)
**File:** `drivers/net/txgbe/base/txgbe_aml.c`
**Location:** Lines 241-247
In `txgbe_setup_phy_link_aml`, `rte_spinlock_lock(&hw->phy_lock)` is acquired at line 241. If `txgbe_set_link_to_amlite` returns `TXGBE_ERR_PHY_INIT_NOT_DONE` at line 246, the code jumps to `out:` at line 264 without releasing the spinlock. The lock is only released on the success path or `TXGBE_ERR_TIMEOUT`.
**Suggested fix:**
```c
rte_spinlock_lock(&hw->phy_lock);
ret_status = txgbe_set_link_to_amlite(hw, speed);
rte_spinlock_unlock(&hw->phy_lock); /* Always release before checking status */
if (ret_status == TXGBE_ERR_PHY_INIT_NOT_DONE)
goto out;
```
### 3. Missing error propagation to caller (CORRECTNESS)
**File:** `drivers/net/txgbe/base/txgbe_aml.c`
**Location:** Lines 280-283
In `txgbe_setup_phy_link_aml`, the function returns `status` which is initialized to 0 and never assigned a failure value, even when `txgbe_set_link_to_amlite` fails or link is not established. The function always returns success regardless of actual outcome.
**Suggested fix:**
```c
if (!link_up) {
*need_reset = true;
return TXGBE_ERR_TIMEOUT; /* Return error when link fails */
}
```
### 4. Double error return value overwrite (CORRECTNESS)
**File:** `drivers/net/txgbe/base/txgbe_e56.c`
**Location:** Lines 1090-1095, 1157-1162, 1197-1202, 1242-1247
In `txgbe_e56_rxs_osc_init_for_temp_track_range`, the pattern:
```c
if (timer++ > PHYINIT_TIMEOUT) {
DEBUGOUT("ERROR: ...");
break;
return -1; /* Unreachable: break exits the loop */
}
```
The `return -1` is unreachable. After `break`, execution continues to the next loop iteration check and then proceeds past the loop. The error is not propagated.
**Suggested fix:**
```c
if (timer++ > PHYINIT_TIMEOUT) {
DEBUGOUT("ERROR: Wait timeout");
return TXGBE_ERR_TIMEOUT; /* Remove break, return immediately */
}
```
### 5. Error code dropped without propagation (CORRECTNESS)
**File:** `drivers/net/txgbe/base/txgbe_e56.c`
**Location:** Lines 1828-1830
In `txgbe_temp_track_seq`:
```c
status = txgbe_e56_get_temp(hw, &temperature);
if (status)
return 0; /* Returns success when get_temp failed */
```
Temperature reading failure is silently converted to success. The caller cannot distinguish between a successful temperature track and a failed temperature read.
**Suggested fix:**
```c
status = txgbe_e56_get_temp(hw, &temperature);
if (status)
return status; /* Propagate the error */
```
### 6. Integer overflow in bit shift (CORRECTNESS)
**File:** `drivers/net/txgbe/base/txgbe_e56.c`
**Location:** Lines 19-31 in `set_fields_e56()`
The function performs `(1 << bit_low)` where `bit_low` can be up to 31 (since bit fields in the code span 0-31). The literal `1` is `int` (signed 32-bit). When `bit_low >= 31`, `1 << 31` invokes undefined behavior (signed overflow).
**Suggested fix:**
```c
if (bit_high == bit_low) {
if (set_value == 0)
*src_data &= ~(1U << bit_low); /* Use 1U for unsigned */
else
*src_data |= (1U << bit_low);
} else {
for (i = bit_low; i <= bit_high; i++)
*src_data &= ~(1U << i);
*src_data |= (set_value << bit_low);
}
```
### 7. Missing bounds check on array size calculation (CORRECTNESS)
**File:** `drivers/net/txgbe/base/txgbe_e56.c`
**Location:** Line 1498 in `txgbe_e56_rx_rd_second_code()`
The code calls `qsort(RXS_BBCDR_SECOND_ORDER_ST, array_size, sizeof(int), ...)` where `array_size = ARRAY_SIZE(RXS_BBCDR_SECOND_ORDER_ST)`. The array has 5 elements, but the calculation `array_size = ARRAY_SIZE(...)` trusts the macro. If N changes in the loop initialization (line 1489: `N = 5`), the array could overflow.
**Suggested fix:**
```c
/* Ensure loop count matches array size */
BUILD_ASSERT(N <= ARRAY_SIZE(RXS_BBCDR_SECOND_ORDER_ST));
```
## Warnings
### 1. Variables declared but may be read before initialization (STYLE)
**File:** `drivers/net/txgbe/base/txgbe_aml.c`
**Location:** Lines 325-327, 355-357
Variables `need_reset` are declared immediately before use, which is acceptable C99 style. However, they are declared in the middle of a conditional block scope, which can reduce clarity when the same variable name is reused in different branches.
**Suggested improvement:**
Declare `need_reset` once at function scope for better readability, though the current usage is technically correct.
### 2. Missing documentation for complex algorithm (STYLE)
**File:** `drivers/net/txgbe/base/txgbe_e56.c`
**Location:** Lines 902-1264 (txgbe_e56_rxs_osc_init_for_temp_track_range)
This 360-line function implements a complex PHY oscillator calibration sequence with numbered steps in comments (1, 2, 3...). The algorithm is critical for 25G link stability but lacks a high-level overview comment explaining the purpose, temperature dependency, and why specific register sequences are needed.
**Suggested improvement:**
Add a function-level Doxygen comment explaining the calibration procedure and referencing the hardware specification section.
### 3. Magic numbers without named constants (STYLE)
**File:** `drivers/net/txgbe/base/txgbe_e56.c`
**Location:** Multiple locations (e.g., lines 505, 732, 1000)
Numerous magic numbers appear in register writes (e.g., `0x68c1`, `0x3321`, `0x973e`, `0xccde` at lines 2281-2284) without explanation. These appear to be RS-FEC configuration magic values but are not documented.
**Suggested improvement:**
Define named constants or add inline comments explaining what these values configure:
```c
#define RS_FEC_CONFIG_VAL1 0x68c1 /* RS-FEC configuration register value */
```
## Info
### 1. Deep nesting in calibration sequence
**File:** `drivers/net/txgbe/base/txgbe_e56.c`
**Location:** Lines 1550-1750 (txgbe_e56_rxs_calib_adapt_seq)
The ADC calibration loop (16 iterations) has 5 levels of nesting. Consider extracting the calibration step into a helper function to improve readability.
### 2. Possible candidate for helper function
**File:** `drivers/net/txgbe/base/txgbe_e56.c`
**Location:** Lines 1950-2050 (repeated CTLE bypass pattern)
The code to override CTLE bypass for multiple lanes (RXS1, RXS2, RXS3) at lines 1999-2030 is repetitive. This pattern could be extracted into a helper function.
---
## Summary
**Critical Issues:** 7 Errors identified, primarily correctness bugs affecting resource management and error handling.
**Style Issues:** 3 Warnings - documentation and code organization improvements.
**Observations:** 2 Info items - code structure suggestions.
The most serious issues are:
1. Lock leak on error path (Error #2)
2. Missing error propagation causing silent failures (Errors #3, #5)
3. Undefined behavior in bit operations (Error #6)
4. Unreachable error returns masking timeouts (Error #4)
============================================================
^ permalink raw reply
* Re: [PATCH v4 03/20] net/ngbe: add missing CDR config for YT PHY
From: Stephen Hemminger @ 2026-05-17 23:37 UTC (permalink / raw)
To: Zaiyu Wang; +Cc: dev, stable, Jiawen Wu
In-Reply-To: <20260511103604.19724-4-zaiyuwang@trustnetic.com>
On Mon, 11 May 2026 18:35:45 +0800
Zaiyu Wang <zaiyuwang@trustnetic.com> wrote:
> According to the PHY vendor, when YT8531S operates in UTP-to-Fiber or
> RGMII-to-Fiber mode with auto-negotiation disabled (Force mode),
> additional CDR (Clock Data Recovery) configuration is required to
> improve link connectivity. Without this config, link may be unstable
> or fail to establish.
>
> Fixes: f1268369403d ("net/ngbe: support autoneg on/off for external PHY SFI mode")
> Cc: stable@dpdk.org
>
> Signed-off-by: Zaiyu Wang <zaiyuwang@trustnetic.com>
> ---
Semi-automated AI review feedback.
# DPDK Patch Review - net/ngbe: add missing CDR config for YT PHY
## Summary
This patch adds Clock Data Recovery (CDR) configuration for YT8531S PHY in Force mode (auto-negotiation disabled). No errors found.
---
## CORRECTNESS BUGS
None identified.
---
## WARNINGS
### 1. Missing error checks on PHY register writes
**File:** `drivers/net/ngbe/base/ngbe_phy_yt.c`
**Lines:** 267-268
The two `ngbe_write_phy_reg_sds_ext_yt()` calls do not check return values. If these PHY register writes fail, the link may be unstable (the exact problem this patch aims to fix), but the function will silently continue.
**Current code:**
```c
/* force mode need to config cdr */
ngbe_write_phy_reg_sds_ext_yt(hw, 0x3, 0, 0x1434);
ngbe_write_phy_reg_sds_ext_yt(hw, 0xe, 0, 0x163);
value = YT_BCR_RESET | YT_BCR_DUPLEX;
```
**Suggested fix:**
```c
/* force mode need to config cdr */
s32 ret;
ret = ngbe_write_phy_reg_sds_ext_yt(hw, 0x3, 0, 0x1434);
if (ret != 0)
return ret;
ret = ngbe_write_phy_reg_sds_ext_yt(hw, 0xe, 0, 0x163);
if (ret != 0)
return ret;
value = YT_BCR_RESET | YT_BCR_DUPLEX;
```
Note: This assumes `ngbe_write_phy_reg_sds_ext_yt()` returns 0 on success, negative on failure (standard DPDK convention). The existing code pattern in the function does not check errors on PHY writes in the auto-negotiation path either (lines 264-265), so this may be intentional design. However, for a fix addressing link stability, propagating write failures would be more robust.
---
## INFO
### 1. Magic numbers in PHY register writes
**File:** `drivers/net/ngbe/base/ngbe_phy_yt.c`
**Lines:** 267-268
The CDR configuration uses literal hex values `0x1434` and `0x163` without named constants or comments explaining their meaning. This is vendor-specific configuration. Consider adding a comment referencing the PHY vendor documentation or datasheet section, or defining named constants if these values are used elsewhere.
Example:
```c
/* CDR config per YT8531S datasheet section X.Y for Force mode */
#define YT_CDR_CONFIG_REG3 0x1434
#define YT_CDR_CONFIG_REGE 0x163
ngbe_write_phy_reg_sds_ext_yt(hw, 0x3, 0, YT_CDR_CONFIG_REG3);
ngbe_write_phy_reg_sds_ext_yt(hw, 0xe, 0, YT_CDR_CONFIG_REGE);
```
This is a style suggestion, not a requirement, and may be deferred if the vendor documentation is not publicly available or these values are unique to this use case.
^ permalink raw reply
* Re: [PATCH v4 14/20] net/txgbe: fix link stability for Amber-Lite backplane mode
From: Stephen Hemminger @ 2026-05-17 23:50 UTC (permalink / raw)
To: Zaiyu Wang; +Cc: dev, stable, Jiawen Wu
In-Reply-To: <20260511103604.19724-15-zaiyuwang@trustnetic.com>
On Mon, 11 May 2026 18:35:56 +0800
Zaiyu Wang <zaiyuwang@trustnetic.com> wrote:
> The link was previously configured via firmware, but this approach
> resulted in unstable link behavior. To resolve the issue, re-add the
> PHY configuration flow directly into the driver.
>
> Fixes: ead3616f630d ("net/txgbe: support PHY configuration via SW-FW mailbox")
> Cc: stable@dpdk.org
>
> Signed-off-by: Zaiyu Wang <zaiyuwang@trustnetic.com>
> ---
Lots of AI feedback here:
# Review of DPDK Patch: net/txgbe: fix link stability for 40G NIC
## Summary
This patch re-adds direct PHY configuration flow to the txgbe driver to fix unstable link behavior on 40G NICs. The previous firmware-based configuration approach was causing link stability issues.
## ERRORS
### 1. Use-after-free risk in `txgbe_setup_phy_link_aml40`
**File:** `drivers/net/txgbe/base/txgbe_aml40.c:157-159`
```c
ret_status = txgbe_set_link_to_amlite(hw, speed);
rte_spinlock_unlock(&hw->phy_lock);
if (ret_status == TXGBE_ERR_TIMEOUT)
hw->link_valid = false;
```
The spinlock is unlocked before checking `ret_status`, but if another thread modifies `hw->link_valid` between lines 158 and 160, the state becomes inconsistent. The assignment to `hw->link_valid` should occur before unlocking:
```c
ret_status = txgbe_set_link_to_amlite(hw, speed);
if (ret_status == TXGBE_ERR_TIMEOUT)
hw->link_valid = false;
rte_spinlock_unlock(&hw->phy_lock);
```
### 2. Missing error propagation in `txgbe_e56_rx_rd_second_code_40g`
**File:** `drivers/net/txgbe/base/txgbe_e56.c:1816`
The function declares `status = 0` and returns `status`, but never assigns a failure value even when qsort is called on potentially invalid data. If the timeout in the preceding while loop is reached (line 1825), the SECOND_CODE array may contain incomplete data, but the function still returns success.
### 3. Missing bounds check before array access
**File:** `drivers/net/txgbe/base/txgbe_e56.c:1831`
```c
median = ((N + 1) / 2) - 1;
*SECOND_CODE = RXS_BBCDR_SECOND_ORDER_ST[median];
```
If `N=5`, `median=2` which is valid. However, this code pattern is repeated multiple times (lines 244, 1831, etc.) with `N` as a constant, so it's safe. Nevertheless, adding `RTE_VERIFY(median < ARRAY_SIZE(RXS_BBCDR_SECOND_ORDER_ST))` would make intent explicit.
**Not flagging this as an error** since `N=5` is a fixed constant throughout.
### 4. Timeout return without cleanup in `txgbe_e56_rxs_calib_adapt_seq_40G`
**File:** `drivers/net/txgbe/base/txgbe_e56.c:2475-2481`
```c
if (timer++ > PHYINIT_TIMEOUT) {
rdata = 0;
addr = E56PHY_PMD_CFG_0_ADDR;
rdata = rd32_ephy(hw, addr);
set_fields_e56(&rdata, E56PHY_PMD_CFG_0_RX_EN_CFG, 0x0);
wr32_ephy(hw, addr, rdata);
return TXGBE_ERR_TIMEOUT;
}
```
The function has already configured many registers in the loop `for (i = 0; i < 4; i++)` (starting line 2393). When a timeout occurs on lane 0-2, the function returns immediately without restoring registers on the lanes that were successfully configured. This leaves the hardware in a partially configured state. The cleanup should disable all lanes, not just the one that timed out.
## WARNINGS
### 1. Hardcoded timeout in multiple locations
**File:** `drivers/net/txgbe/base/txgbe_e56.c` (multiple locations)
The `PHYINIT_TIMEOUT` constant is used consistently, but the delays vary (100µs, 500µs, 1000µs, 10ms). For the 500µs delay case (e.g., line 2478), `PHYINIT_TIMEOUT` iterations result in `PHYINIT_TIMEOUT * 500µs` total wait time. If `PHYINIT_TIMEOUT` is intended to be milliseconds, the timeout duration becomes inconsistent across different polling loops. Consider documenting what the timeout value represents (iterations? milliseconds?) and using consistent delay granularity.
### 2. Potentially unreachable code after loop
**File:** `drivers/net/txgbe/base/txgbe_e56.c:2656`
```c
for (j = 0; j < 16; j++) {
// ... ADC adaptation loop
}
/* g. Repeat #a to #f total 16 times */
```
The comment `/* g. Repeat #a to #f total 16 times */` appears *after* the loop that already runs 16 times. This is documentation only, but could be confusing. The comment should be before the loop or removed.
### 3. Inconsistent use of `msleep` vs `usec_delay`
**File:** `drivers/net/txgbe/base/txgbe_e56.c`
The patch uses `msleep()` for delays >= 10ms (lines 181, 3029) and `usec_delay()` for shorter delays (line 1826). However, line 3029 uses `msleep(10)` for 10ms, while line 2707 uses no delay after setting a register. Consider documenting the rationale for sleep vs busy-wait or using a consistent threshold.
### 4. Variable `bypass_ctle` hardcoded but declared as variable
**File:** `drivers/net/txgbe/base/txgbe_e56.c:2396`
```c
u32 bypass_ctle = true;
```
The variable `bypass_ctle` is declared as `u32` but assigned a boolean value, and it's never modified. Either:
- Change to `const bool bypass_ctle = true;` (preferred)
- Or document why it's a runtime variable despite being hardcoded
### 5. Missing validation of speed parameter in initialization functions
**File:** `drivers/net/txgbe/base/txgbe_e56.c:2206`
```c
if (speed == TXGBE_LINK_SPEED_10GB_FULL || speed == TXGBE_LINK_SPEED_40GB_FULL) {
CMVAR_SEC_LOW_TH = S10G_CMVAR_SEC_LOW_TH;
// ...
} else if (speed == TXGBE_LINK_SPEED_25GB_FULL) {
// ...
} else {
DEBUGOUT("Error Speed\n");
return 0; // Returns success despite error
}
```
The function returns 0 (success) when an invalid speed is passed, but logs "Error Speed". This should return an error code like `-EINVAL` or `TXGBE_ERR_PARAM`.
## INFORMATIONAL
### 1. Large function complexity
The function `txgbe_e56_rxs_calib_adapt_seq_40G` spans ~280 lines with deeply nested loops (3-level nesting). Consider refactoring into smaller helper functions for each calibration stage (ADC offset, ADC gain, interleaver adaptation) to improve readability and maintainability.
### 2. Magic numbers without symbolic constants
**File:** `drivers/net/txgbe/base/txgbe_e56.c:2470`
```c
while (EPHY_XFLD(E56G__PMD_CTRL_FSM_RX_STAT_0, ctrl_fsm_rx0_st) != 0x21 ||
```
The value `0x21` (POWERDN_ST) appears in multiple locations (lines 2470, 3145). This is already defined as `E56PHY_RX_POWERDN_ST` elsewhere. Use the symbolic constant consistently.
### 3. Duplicated initialization sequences
The 40G initialization in `txgbe_e56_cfg_40g` (lines 176-566) and the existing 10G/25G code share many similar register sequences. Consider extracting common configuration patterns into helper functions to reduce code duplication and maintenance burden.
### 4. Temperature check frequency
**File:** `drivers/net/txgbe/base/txgbe_e56.c:2253-2256`
The temperature tracking sequence comment states "must be run before the temperature drifts by >5degC" and recommends running every 100ms. However, the patch doesn't add timer-based periodic execution—it only runs during link setup. If temperature tracking is critical for stability, consider documenting that the caller must invoke this periodically.
---
## Positive Observations
1. The patch correctly adds `hw->link_valid` checks in `txgbe_check_mac_link_aml40` to prevent reporting link up when PHY configuration fails (lines 57-60, 80-81).
2. Error paths in timeout scenarios attempt cleanup by disabling RX (e.g., line 2477).
3. The use of median filtering for SECOND_CODE (lines 1829-1831) reduces noise from asynchronous hardware updates—good defensive programming.
^ permalink raw reply
* Re: [PATCH v4 04/20] net/ngbe: fix VF promiscuous and allmulticast
From: Stephen Hemminger @ 2026-05-17 23:39 UTC (permalink / raw)
To: Zaiyu Wang; +Cc: dev, stable, Jiawen Wu
In-Reply-To: <20260511103604.19724-5-zaiyuwang@trustnetic.com>
On Mon, 11 May 2026 18:35:46 +0800
Zaiyu Wang <zaiyuwang@trustnetic.com> wrote:
> The configuration of allmulti and promiscuous modes conflicts
> together. For instance, if we enable promiscuous mode, then enable and
> disable allmulti, then the promiscuous mode is wrongly disabled.
>
> To resolve this, the following changes are made:
> - do nothing when we set/unset allmulti if promiscuous mode is on
> - restore the proper mode (none or allmulti) when we disable
> promiscuous mode
>
> Fixes: 7744e90805b5 ("net/ngbe: add promiscuous and allmulticast ops for VF device")
> Cc: stable@dpdk.org
>
> Signed-off-by: Zaiyu Wang <zaiyuwang@trustnetic.com>
> ---
Worth mentioning, but IMHO this is case of AI being overly pedantic.
Patch is OK as is.
# Review of DPDK Patch: net/ngbe: fix VF promiscuous and allmulticast
## Summary
This patch fixes a mode conflict issue in the ngbe VF driver between promiscuous and allmulticast modes. The overall approach is correct: it prevents mode conflicts by making allmulticast operations no-ops when promiscuous mode is active, and restores the appropriate mode when disabling promiscuous.
## Errors
None identified.
## Warnings
### 1. Implicit comparison in existing code (line 1226)
**Issue:** The existing code uses `if (dev->data->promiscuous == 1)` which mixes explicit and implicit styles. The patch changes this to `if (dev->data->promiscuous)` which is implicit.
**Why it matters:** DPDK coding standards require explicit comparisons for integers (`!= 0` or `== 0`).
**Suggested fix:**
```c
/* Line 1226 - be consistent with explicit comparison */
if (dev->data->promiscuous != 0)
return 0;
/* Line 1250 - same issue */
if (dev->data->promiscuous != 0)
return 0;
```
Note: While the patch changes `== 1` to implicit, the proper fix is to use `!= 0` since `promiscuous` is an integer field, not a boolean type.
### 2. Error propagation pattern inconsistency
**Issue:** The function `ngbevf_dev_promiscuous_disable()` returns 0 early when allmulticast check causes mode selection, but this early return bypasses the switch statement's error handling for the actual mode update call.
**Context:** This is not a bug in the patch itself—the early returns in `ngbevf_dev_allmulticast_enable()` and `ngbevf_dev_allmulticast_disable()` when promiscuous mode is active are correct (those are intentional no-ops). The mode variable change in `ngbevf_dev_promiscuous_disable()` correctly passes through to the existing switch statement for error handling.
No change needed—this comment is for clarity only.
## Info
### Code structure observation
The logic correctly handles the mode transitions:
- When disabling promiscuous mode, it checks `all_multicast` flag and sets the appropriate target mode (ALLMULTI if enabled, NONE otherwise)
- When enabling/disabling allmulticast while promiscuous is active, it returns early as a no-op (correct, since promiscuous overrides allmulticast)
This is a clean fix for the mode conflict issue described in the commit message.
---
## Final Recommendation
Address the implicit comparison style issue (Warning #1) to comply with DPDK coding standards. Change `if (dev->data->promiscuous)` to `if (dev->data->promiscuous != 0)` in both locations (lines 1226 and 1250).
^ permalink raw reply
* Re: [PATCH v4 16/20] net/txgbe: fix SFP module identification
From: Stephen Hemminger @ 2026-05-17 23:52 UTC (permalink / raw)
To: Zaiyu Wang; +Cc: dev, stable, Jiawen Wu
In-Reply-To: <20260511103604.19724-17-zaiyuwang@trustnetic.com>
On Mon, 11 May 2026 18:35:58 +0800
Zaiyu Wang <zaiyuwang@trustnetic.com> wrote:
> Some optical modules were not correctly recognized due to ambiguous
> classification in the original detection flow. Rework the module
> identification logic to cover all module types. Also narrow the
> I2C lock scope to avoid potential race conditions during module
> access.
>
> Fixes: ab191e6d9189 ("net/txgbe: support new SFP/QSFP modules")
> Cc: stable@dpdk.org
>
> Signed-off-by: Zaiyu Wang <zaiyuwang@trustnetic.com>
> ---
Lots of AI feedback here.
Note: not all this is required; like the request for extra release notes is AI being picky.
# Review of Patch 16/20: net/txgbe: fix SFP module identification
## Summary
This patch refactors SFP/QSFP module identification logic and adjusts I2C locking scope. The changes address module type detection and claim to fix race conditions.
---
## Errors
### 1. Resource leak in `txgbe_identify_sfp_module()` - multiple error paths missing unlock
**Error paths that acquire lock but fail to release on error:**
At line 836, `hw->mac.acquire_swfw_sync()` acquires `TXGBE_MNGSEM_SWPHY`. Several error paths after this point fail to release the lock:
```c
err = hw->phy.read_i2c_eeprom(hw, TXGBE_SFF_IDENTIFIER, &identifier);
if (err != 0) {
/* ... */
hw->mac.release_swfw_sync(hw, TXGBE_MNGSEM_SWPHY); // OK - releases
return TXGBE_ERR_SFP_NOT_PRESENT;
}
if (identifier != TXGBE_SFF_IDENTIFIER_SFP) {
hw->phy.type = txgbe_phy_sfp_unsupported;
hw->mac.release_swfw_sync(hw, TXGBE_MNGSEM_SWPHY); // OK - releases
return TXGBE_ERR_SFP_NOT_SUPPORTED;
}
err = hw->phy.read_i2c_eeprom(hw, ...);
/* Multiple read_i2c_eeprom calls follow */
```
**After line 894**, there are at least **4 read_i2c_eeprom() calls** (lines ~895-930 in the context) that can fail with `goto out;` - but `goto out;` is removed in this patch. If any of these reads fail now, the function returns immediately **without releasing the lock acquired at line 836**.
Example at line 1000:
```c
err = hw->phy.read_i2c_eeprom(hw, TXGBE_SFF_CABLE_SPEC_COMP, &cable_spec);
if (err != 0)
goto out; /* This label is REMOVED - now falls through or returns */
```
The old code had `out:` label that might have handled cleanup. The new code removes this pattern but doesn't add `hw->mac.release_swfw_sync()` calls before each early return.
**Impact:** Lock held indefinitely on I2C read failure, blocking other threads.
**Fix:** Add lock release before every early return after line 836:
```c
err = hw->phy.read_i2c_eeprom(hw, TXGBE_SFF_CABLE_SPEC_COMP, &cable_spec);
if (err != 0) {
hw->mac.release_swfw_sync(hw, TXGBE_MNGSEM_SWPHY);
goto out;
}
```
---
### 2. `txgbe_write_i2c_byte()` removes STOP condition - probable hardware protocol violation
**Line 1299 (old) vs 1299 (new):**
Old code:
```c
wr32(hw, TXGBE_I2CDATA, byte_offset | TXGBE_I2CDATA_STOP);
```
New code:
```c
wr32(hw, TXGBE_I2CDATA, byte_offset);
```
The `TXGBE_I2CDATA_STOP` flag is removed from the byte_offset write but remains only on the data write. This changes the I2C transaction sequence:
- Old: Write byte_offset with STOP, write data with STOP
- New: Write byte_offset (no STOP), write data with WRITE+STOP (via `txgbe_i2c_stop()`)
**Analysis:** I2C writes to EEPROMs typically follow: START → device_addr+W → ACK → byte_offset → ACK → data → ACK → STOP. Removing STOP from the byte_offset phase may be correct if the hardware requires a combined write transaction, **but this is a functional change to I2C timing that is not explained in the commit message** and could break module writes.
**Impact:** Could cause I2C write failures or data corruption on SFP/QSFP EEPROMs.
**Fix:** Either:
1. Verify via hardware datasheet that single STOP at end is correct, document in commit message
2. Restore `TXGBE_I2CDATA_STOP` on byte_offset write
---
### 3. Removed validation logic without replacement - SFP vendor checking deleted
**Lines 1025-1070 (old code, removed):** The old `txgbe_identify_qsfp_module()` read vendor OUI bytes, checked for Intel vendor, and enforced "allow_any_sfp" policy. This entire block is deleted with no equivalent in the new code.
**Impact:**
- Unsupported vendor modules are now silently accepted (security/quality risk)
- `hw->allow_unsupported_sfp` flag is no longer checked (user configuration ignored)
- Warning messages for untested modules removed (users not informed of risks)
**This is a functional regression unless the validation moved elsewhere** (not evident in this patch).
**Fix:** Restore vendor OUI check or document why it's safe to remove.
---
## Warnings
### 1. `txgbe_identify_qsfp_module()` GPIO config runs unconditionally
**Lines 1098-1099:**
```c
/* config GPIO before read i2c */
wr32(hw, TXGBE_GPIODATA, TXGBE_GPIOBIT_1);
```
This GPIO write now runs **even when media_type is not fiber_qsfp** (the media check is after the GPIO write at lines 1108-1111). The old code had this GPIO write inside the `if (hw->mac.type == txgbe_mac_aml40)` block, after the media check.
**Impact:** Unnecessary hardware access, potential side effects on non-QSFP ports.
**Fix:** Move GPIO config inside the `if (hw->mac.type == txgbe_mac_aml40)` block or add media_type check first.
---
### 2. Missing error propagation - `txgbe_read_i2c_sff8636()` page select failure ignored
**Line 1215:**
```c
hw->phy.write_i2c_byte(hw, TXGBE_SFF_QSFP_PAGE_SELECT,
TXGBE_I2C_EEPROM_DEV_ADDR,
page);
```
The return value from `write_i2c_byte()` is discarded. If page select fails, the subsequent read at line 1218 will read from the **wrong page**, producing incorrect module data.
**Fix:**
```c
s32 err = hw->phy.write_i2c_byte(hw, TXGBE_SFF_QSFP_PAGE_SELECT,
TXGBE_I2C_EEPROM_DEV_ADDR, page);
if (err != 0)
return err;
```
---
### 3. Implicit comparison on pointer
**Line 1123 (in `txgbe_identify_qsfp_module`):**
```c
if (err)
return -EBUSY;
```
Should be explicit:
```c
if (err != 0)
return -EBUSY;
```
---
### 4. Release notes likely required
This patch changes module detection logic and affects supported module types. API behavior changes (vendor validation removed, new module type support) typically require release notes under "Fixed Issues" or "Known Issues" if compatibility changes exist.
---
### 5. Function pointer assignments removed but functions still defined
**Lines 2912-2913 (txgbe_hw.c):**
```c
phy->read_i2c_byte_unlocked = txgbe_read_i2c_byte_unlocked;
phy->write_i2c_byte_unlocked = txgbe_write_i2c_byte_unlocked;
```
These assignments are removed, but the commit message doesn't indicate whether the `*_unlocked` functions are deleted or just unused. If the functions remain defined but unreferenced, they are dead code. If other drivers reference `phy->read_i2c_byte_unlocked` as a function pointer, this breaks them.
---
## Info
### 1. Simplified I2C locking approach is clearer
The removal of separate `*_unlocked` variants and the move to caller-managed locking (acquire once, multiple reads, release) is a good pattern that reduces lock churn.
---
### 2. New SFP type handling for 25G/40G modules
The patch adds proper detection for:
- 25G AOC modules (`txgbe_sfp_type_25g_aoc_core0/1`)
- 40G QSFP types with Ethernet compliance byte checks
This appears to address the "ambiguous classification" mentioned in the commit message.
---
### 3. Consistent use of `read_i2c_sff8636()` helper
The new `txgbe_read_i2c_sff8636()` helper centralizes page-select logic for QSFP modules, which is good for maintainability (despite the error handling issue noted above).
---
## Final Check Summary
**Errors (must fix):**
1. Lock leaks in `txgbe_identify_sfp_module()` on I2C read errors after line 836
2. `TXGBE_I2CDATA_STOP` removal in `txgbe_write_i2c_byte()` changes I2C protocol, not explained/verified
3. Vendor OUI validation removed without replacement - security/quality regression
**Warnings (should fix):**
1. GPIO config runs before media_type check
2. Page select error ignored in `txgbe_read_i2c_sff8636()`
3. Implicit comparison `if (err)`
4. Release notes likely needed for behavior changes
5. Unused function pointer assignments removed - check for dead code
^ permalink raw reply
* Re: [PATCH v4 20/20] net/txgbe: fix to enable Tx desc check
From: Stephen Hemminger @ 2026-05-17 23:55 UTC (permalink / raw)
To: Zaiyu Wang; +Cc: dev, stable, Jiawen Wu
In-Reply-To: <20260511103604.19724-21-zaiyuwang@trustnetic.com>
On Mon, 11 May 2026 18:36:02 +0800
Zaiyu Wang <zaiyuwang@trustnetic.com> wrote:
> Now lib security is enabled by default, and cannot be disabled if the
> driver is intended to be used. So Tdm_desc_chk is always unable to enable.
> Remove this restriction, and just enable the corresponding queue check.
>
> Fixes: 0eabdfcd4af4 ("net/txgbe: enable Tx descriptor error interrupt")
> Cc: stable@dpdk.org
>
> Signed-off-by: Zaiyu Wang <zaiyuwang@trustnetic.com>
> ---
Minor AI stuff here:
# Review of DPDK Patch: net/txgbe: fix to enable Tx desc check
## Summary
This patch modifies the Tx descriptor check logic in the txgbe driver to work correctly when security features are enabled. The change moves descriptor check setup from a global per-device operation to a per-queue operation.
## ERRORS
None found.
## WARNINGS
### 1. Bit manipulation should use RTE_BIT32() macro
**File:** drivers/net/txgbe/txgbe_rxtx.c
**Lines:** 4774, 4775
The code uses `BIT()` macro for bitmask generation. DPDK coding standards prefer `RTE_BIT32()` or `RTE_BIT64()` for clarity and portability.
**Current code:**
```c
wr32m(hw, TXGBE_TDM_DESC_CHK(txq->reg_idx / 32),
BIT(txq->reg_idx % 32), BIT(txq->reg_idx % 32));
```
**Suggested fix:**
```c
wr32m(hw, TXGBE_TDM_DESC_CHK(txq->reg_idx / 32),
RTE_BIT32(txq->reg_idx % 32), RTE_BIT32(txq->reg_idx % 32));
```
Verify that `BIT()` is defined in the driver's headers. If it's a legacy macro, consider using the DPDK standard `RTE_BIT32()` for consistency.
### 2. Conditional compilation without corresponding documentation update
**Context:** The patch changes the logic for when descriptor checks are enabled but there's no indication of release notes or documentation updates.
The original code disabled all descriptor checks when `RTE_LIB_SECURITY` was not defined. The new code enables descriptor checks per-queue unless the queue is using IPsec. This is a functional behavior change that should be documented.
**Suggested action:**
- Update release notes to describe the change in Tx descriptor check behavior
- Document that descriptor checks are now enabled per-queue based on IPsec usage rather than globally based on library compilation
## INFO
### 1. Potential logic optimization
The same bitmask value `BIT(txq->reg_idx % 32)` is used twice in the `wr32m()` call. Consider extracting it to a local variable for clarity:
```c
uint32_t queue_bit = BIT(txq->reg_idx % 32);
wr32m(hw, TXGBE_TDM_DESC_CHK(txq->reg_idx / 32), queue_bit, queue_bit);
```
This makes the intent clearer (setting the bit to 1) and slightly improves readability.
### 2. Parentheses around `using_ipsec` unnecessary
**Line:** 4772
```c
if (!(txq->using_ipsec))
```
Could be written as:
```c
if (!txq->using_ipsec)
```
The extra parentheses around `using_ipsec` are unnecessary since `!` has higher precedence than most operators.
---
## OVERALL ASSESSMENT
The patch correctly addresses the issue where descriptor checks could not be enabled when the security library was compiled in. The logic change from global enable/disable to per-queue configuration based on IPsec usage is sound.
**Key improvements made:**
- Moves descriptor check setup into the per-queue loop
- Only disables checks for queues actually using IPsec
- Removes the blanket disable when security library is present
**Correctness:** No bugs detected. The resource management and control flow are correct.
**Style compliance:** Minor issues with macro usage; otherwise compliant.
^ permalink raw reply
* Re: [PATCH v2 0/6] net/gve: add hardware timestamping support
From: Stephen Hemminger @ 2026-05-17 23:15 UTC (permalink / raw)
To: Mark Blasko; +Cc: dev
In-Reply-To: <20260515231936.3296603-1-blasko@google.com>
On Fri, 15 May 2026 23:19:29 +0000
Mark Blasko <blasko@google.com> wrote:
> This patch series introduces support for GVE hardware timestamping on DQO
> queues. To support concurrent access, a mutex lock is introduced to protect
> admin queue operations. A mechanism is then added to periodically synchronize
> the NIC clock via AdminQ, and support is introduced for the read_clock ethdev
> operation. Finally, the RX datapath is updated to reconstruct full 64-bit
> timestamps from the 32-bit values in DQO descriptors.
>
> ---
> v2:
> - Patch 1: Dropped ROBUST mutex attribute.
> - Patch 3: Added adminq timestamp counter reset to gve_adminq_alloc.
> - Patch 4:
> - Removed redundant void* casts.
> - Handled alarm reschedule failures by marking timestamp stale.
> - Added transient error logging on memzone allocation failure.
> - Patch 5: Scoped read_clock ethdev operation strictly to DQO queues.
> - Patch 6:
> - Scoped timestamp offload capability advertisement strictly to
> DQO queues.
> - Predicated capability advertisement directly on memzone
> allocation.
> - Initialized mbuf_timestamp_offset to -1.
> - Added blank line separating release notes.
> ---
>
> Mark Blasko (6):
> net/gve: add thread safety to admin queue
> net/gve: add device option support for HW timestamps
> net/gve: add AdminQ command for NIC timestamps
> net/gve: add periodic NIC clock synchronization
> net/gve: support read clock ethdev op
> net/gve: reconstruct HW timestamps from DQO
>
> .mailmap | 1 +
> doc/guides/nics/features/gve.ini | 1 +
> doc/guides/nics/gve.rst | 20 ++++
> doc/guides/rel_notes/release_26_07.rst | 4 +
> drivers/net/gve/base/gve_adminq.c | 128 +++++++++++++++++----
> drivers/net/gve/base/gve_adminq.h | 29 +++++
> drivers/net/gve/base/gve_desc_dqo.h | 8 +-
> drivers/net/gve/gve_ethdev.c | 149 ++++++++++++++++++++++++-
> drivers/net/gve/gve_ethdev.h | 39 +++++++
> drivers/net/gve/gve_rx_dqo.c | 26 +++++
> 10 files changed, 383 insertions(+), 22 deletions(-)
>
Looks good, once again AI followup still found some things you need to address.
Patch 5: net/gve: support read clock ethdev op
Error: gve_read_clock is defined as static but never assigned to
any eth_dev_ops table. In v1 the patch added
".read_clock = gve_read_clock" to both gve_eth_dev_ops and
gve_eth_dev_ops_dqo. The v2 changelog notes "Scoped read_clock
ethdev operation strictly to DQO queues," which should have left
the assignment in gve_eth_dev_ops_dqo and removed it from
gve_eth_dev_ops. Instead both assignments were dropped, leaving
the function unreferenced. DPDK CI builds with -Dwerror=true, so
-Wunused-function will fail the build, and the read_clock feature
is unreachable at runtime in any case.
Restore the assignment in gve_eth_dev_ops_dqo:
static const struct eth_dev_ops gve_eth_dev_ops_dqo = {
...
.reta_query = gve_rss_reta_query,
.read_clock = gve_read_clock,
};
Warning: (unchanged from v1) gve_read_clock and the periodic
gve_read_nic_clock alarm callback both issue
GVE_ADMINQ_REPORT_NIC_TIMESTAMP into the single shared DMA buffer
priv->nic_ts_report, then read it after gve_adminq_execute_cmd
has released adminq_lock. If gve_read_clock is preempted between
gve_adminq_report_nic_timestamp returning and the be64_to_cpu
read, the alarm callback can memset() and reissue its own
command, so the user thread will read either zero or another
command's response. The simplest fix is for gve_read_clock to
return the cached priv->last_read_nic_timestamp instead of
issuing a fresh adminq command - the 250ms periodic sync keeps
it fresh enough for .read_clock semantics. Once the dev_op
registration is restored this race becomes reachable.
^ permalink raw reply
* Re: [PATCH] devtools: fix SPDX tag check
From: Thomas Monjalon @ 2026-05-17 21:34 UTC (permalink / raw)
To: David Marchand; +Cc: dev, stable, Richardson, Bruce, Hemant Agrawal
In-Reply-To: <CAJFAV8yAP34iRGiGKx0+Ls6_FpSa0EubP3KiDrUEU0fY-tp0FQ@mail.gmail.com>
>> If a file has no SPDX tag and is not filtered out by no_license_list,
>> there will be an error when using its path containing a slash
>> in the sed command delimited with slashes.
>>
>> It is fixed by using the pipe character as sed command delimiter.
>>
>> Fixes: b99a3b8aa989 ("license: standardize SPDX tag")
>> Cc: stable@dpdk.org
>>
>> Reported-by: David Marchand <david.marchand@redhat.com>
>> Signed-off-by: Thomas Monjalon <thomas@monjalon.net>
>
> Acked-by: David Marchand <david.marchand@redhat.com>
Applied
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox