[PWCI] "net/r8169: support multiq and code refactor" #256
Conversation
8126 and 8127 support 4 RX queues and 2 TX queues. Signed-off-by: Howard Wang <howard_wang@realsil.com.cn> Signed-off-by: 0-day Robot <robot@bytheb.org>
Signed-off-by: Howard Wang <howard_wang@realsil.com.cn> Signed-off-by: 0-day Robot <robot@bytheb.org>
Signed-off-by: Howard Wang <howard_wang@realsil.com.cn> Signed-off-by: 0-day Robot <robot@bytheb.org>
WalkthroughThe r8169 driver is extended to support Receive Side Scaling (RSS) multi-queue operation, featuring multiple RX descriptor format variants (v1/v3/v4), RSS key and indirection table management, and per-queue state tracking for both TX and RX paths. Hardware capability detection and device information reporting now expose RSS and queue capabilities to the network layer. Changes
Sequence DiagramssequenceDiagram
participant App as Application
participant Driver as r8169 Driver
participant HW as Hardware
App->>Driver: Enable RX with multi-queue
Driver->>Driver: rtl_rx_init()
rect rgb(200, 220, 255)
note over Driver: Multi-queue setup
Driver->>Driver: rtl_init_rss(hw, nb_rx_queues)
Driver->>HW: Configure RSS key (rtl8125_store_rss_key)
Driver->>HW: Configure RETA (rtl8125_store_reta)
Driver->>HW: Set RX queue number (rtl8125_set_rx_q_num)
end
loop Per Queue
Driver->>Driver: Allocate RX ring (multi-desc format)
Driver->>HW: Initialize per-queue descriptor pointers
end
Driver->>App: Ready for RX
rect rgb(200, 255, 220)
note over HW: Per-packet processing
HW->>HW: Calculate RSS hash
HW->>HW: Select queue via indirection table
end
HW->>Driver: Packet received on selected queue
Driver->>Driver: Extract RSS hash (rtl_get_rx_desc_hash)
Driver->>Driver: Populate mbuf hash field
Driver->>App: Deliver packet to application
sequenceDiagram
participant Init as Initialization
participant HW as r8169_hw.c
participant Ethdev as r8169_ethdev.c
Init->>Ethdev: Query device capabilities
Ethdev->>Ethdev: rtl_dev_infos_get()
rect rgb(220, 200, 255)
note over Ethdev: RSS capability reporting
Ethdev->>Ethdev: Set hash_key_size = RTL_RSS_KEY_SIZE
Ethdev->>Ethdev: Set reta_size = RTL_MAX_INDIRECTION_TABLE_ENTRIES
Ethdev->>Ethdev: Set flow_type_rss_offloads = RTL_RSS_CTRL_OFFLOAD_ALL
end
Ethdev->>Init: Device info with RSS capabilities
Init->>Ethdev: Configure RSS parameters
Ethdev->>Ethdev: rss_hash_conf_get() / rss_hash_update()
Ethdev->>HW: Invoke rtl8125_config_rss()
rect rgb(220, 255, 200)
note over HW: Hardware configuration
HW->>HW: rtl8125_set_rss_hash_opt()
HW->>HW: rtl8125_store_rss_key()
HW->>HW: rtl8125_store_reta()
end
HW->>Ethdev: Configuration complete
Ethdev->>Init: Ready for operation
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Key areas requiring focused attention:
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Reviewer's GuideThis PR refactors the r8169 PMD to support multiple Tx/Rx queues with Receive Side Scaling (RSS), restructures descriptor handling across hardware versions, improves per-queue doorbell and no-close logic, and collapses chipset-specific switch statements into more maintainable range-based checks. Sequence diagram for RX packet processing with multi-queue and RSSsequenceDiagram
participant HW as "rtl_hw"
participant RXQ as "rtl_rx_queue"
participant DEV as "rte_eth_dev"
participant APP as "Application"
APP->>DEV: Request RX packets
DEV->>RXQ: Call rtl_recv_pkts()
RXQ->>HW: Get RX descriptor type and length
RXQ->>RXQ: For each RX descriptor
RXQ->>HW: Check RSS enabled
RXQ->>RXQ: Extract RSS hash if enabled
RXQ->>DEV: Return mbuf with RSS hash
DEV->>APP: Deliver packets with RSS hash
Sequence diagram for TX queue doorbell with per-queue registerssequenceDiagram
participant HW as "rtl_hw"
participant TXQ as "rtl_tx_queue"
participant DEV as "rte_eth_dev"
participant APP as "Application"
APP->>DEV: Send packet
DEV->>TXQ: Enqueue packet
TXQ->>HW: Update sw_tail_ptr_reg for this queue
HW->>TXQ: Acknowledge doorbell
TXQ->>DEV: Confirm TX completion
DEV->>APP: Notify TX done
Class diagram for updated RX descriptor types and queue structuresclassDiagram
class rtl_rx_desc {
+u32 opts1
+u32 opts2
+u64 addr
}
class rtl_rx_descv3 {
+RxDescDDWord1_rsv1
+RxDescDDWord1_rsv2
+RxDescNormalDDWord2_RSSResult
+RxDescNormalDDWord2_HeaderBufferLen
+RxDescNormalDDWord2_HeaderInfo
+addr_TimeStampLow
+addr_TimeStampHigh
+RxDescNormalDDWord4_opts2
+RxDescNormalDDWord4_opts1
}
class rtl_rx_descv4 {
+addr_RSSInfo
+addr_RSSResult
+RxDescNormalDDWord2_opts2
+RxDescNormalDDWord2_opts1
}
class rtl_hw {
+u8 EnableRss
+u8 HwSuppRxDescType
+u16 RxDescLength
+u8 rss_key[RTL_RSS_KEY_SIZE]
+u8 rss_indir_tbl[RTL_MAX_INDIRECTION_TABLE_ENTRIES]
}
class rtl_rx_queue {
+struct rtl_rx_desc* hw_ring
+struct rtl_rx_entry* sw_ring
+struct rtl_hw* hw
+uint16_t nb_rx_desc
+struct rte_mempool* mb_pool
+uint16_t queue_id
+uint16_t port_id
+uint16_t rx_tail
+struct rte_mbuf* pkt_first_seg
+struct rte_mbuf* pkt_last_seg
}
rtl_hw "1" -- "*" rtl_rx_queue : has
rtl_rx_queue "1" -- "*" rtl_rx_desc : uses
rtl_rx_queue "1" -- "*" rtl_rx_descv3 : uses
rtl_rx_queue "1" -- "*" rtl_rx_descv4 : uses
Class diagram for updated TX queue structure and doorbell logicclassDiagram
class rtl_tx_queue {
+struct rtl_tx_desc* hw_ring
+struct rtl_tx_entry* sw_ring
+struct rtl_hw* hw
+uint16_t nb_tx_desc
+uint16_t queue_id
+uint16_t port_id
+uint16_t tx_free_thresh
+uint16_t tx_free
+uint16_t hw_clo_ptr_reg
+uint16_t sw_tail_ptr_reg
+uint32_t NextHwDesCloPtr
+uint32_t BeginHwDesCloPtr
}
class rtl_hw {
+u8 HwSuppTxNoCloseVer
+u8 EnableTxNoClose
+u32 MaxTxDescPtrMask
}
rtl_hw "1" -- "*" rtl_tx_queue : has
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey there - I've reviewed your changes and they look great!
Prompt for AI Agents
Please address the comments from this code review:
## Individual Comments
### Comment 1
<location> `drivers/net/r8169/r8169_rxtx.c:573-582` </location>
<code_context>
static void
rtl_enable_aspm_clkreq_lock(struct rtl_hw *hw, bool enable)
{
</code_context>
<issue_to_address>
**issue (bug_risk):** Potential for non-uniform RSS key entropy due to memcpy from rte_rand.
If RTL_RSS_KEY_SIZE isn't a multiple of sizeof(u64), the final bytes of rss_key may remain uninitialized. Please ensure all bytes are set, either by handling the tail explicitly or by looping over each byte.
</issue_to_address>
### Comment 2
<location> `drivers/net/r8169/r8169_rxtx.c:481-482` </location>
<code_context>
+ .rss_hash_conf_get = rtl_rss_hash_conf_get,
};
static int
</code_context>
<issue_to_address>
**issue (bug_risk):** Possible memory leak if allocation fails partway through loop.
If allocation fails after some mbufs have been created, the function returns without freeing them. Free any previously allocated mbufs before returning to prevent memory leaks.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 12
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
drivers/net/r8169/r8169_rxtx.c (1)
1649-1652: Fix inconsistent atomic access patterns for opts1 and tx_tail.Both fields are declared atomic (
RTE_ATOMIC(u32) opts1andRTE_ATOMIC(uint32_t) tx_tail) but are accessed inconsistently: opts1 is read viarte_atomic_load_explicit()but written with plain assignment; tx_tail is read atomically but incremented with plain+=. This violates atomic semantics and risks memory ordering violations for descriptor ownership handoff.Use atomic stores and fetch operations with appropriate memory ordering:
txd->opts2 = rte_cpu_to_le_32(opts2); rte_wmb(); -txd->opts1 = rte_cpu_to_le_32(opts1); +rte_atomic_store_explicit(&txd->opts1, rte_cpu_to_le_32(opts1), + rte_memory_order_release);and
-txq->tx_tail += desc_count; +(void)rte_atomic_fetch_add_explicit(&txq->tx_tail, desc_count, + rte_memory_order_release);Alternatively, if the queue is single-threaded per-core, remove
RTE_ATOMICfrom both field declarations in the struct.
🧹 Nitpick comments (13)
doc/guides/nics/features/r8169.ini (1)
16-18: RSS features marked as universally supported; clarify chipset scope.Driver enables RSS only for newer methods (e.g., mcfg ≥ 69). Consider noting in docs that RSS/RETA/Key are available on those chipsets to avoid overstating coverage.
drivers/net/r8169/r8169_compat.h (1)
296-304: Use consistent unsigned bit types for RX status bits.Replace
(1 << n)with(1UL << n)(or reuse BIT_n style) to avoid signed-shift surprises and match existing style.- RxRWT_V3 = (1 << 18), - RxRES_V3 = (1 << 20), - RxRUNT_V3 = (1 << 19), - RxCRC_V3 = (1 << 17), + RxRWT_V3 = (1UL << 18), + RxRES_V3 = (1UL << 20), + RxRUNT_V3 = (1UL << 19), + RxCRC_V3 = (1UL << 17), @@ - RxRES_V4 = (1 << 22), - RxRUNT_V4 = (1 << 21), - RxCRC_V4 = (1 << 20), + RxRES_V4 = (1UL << 22), + RxRUNT_V4 = (1UL << 21), + RxCRC_V4 = (1UL << 20),drivers/net/r8169/r8169_ethdev.h (4)
35-49: Use unsigned/standard bit macros for RSS_CTRL flags.Adopt BIT_x or 1U/1UL shifts consistently to avoid sign issues and match existing style.
- RSS_CTRL_TCP_IPV4_SUPP = (1 << 0), + RSS_CTRL_TCP_IPV4_SUPP = (1UL << 0), @@ - RSS_CTRL_UDP_IPV6_EXT_SUPP = (1 << 13), - RSS_QUAD_CPU_EN = (1 << 16), - RSS_HQ_Q_SUP_R = (1 << 31), + RSS_CTRL_UDP_IPV6_EXT_SUPP = (1UL << 13), + RSS_QUAD_CPU_EN = (1UL << 16), + RSS_HQ_Q_SUP_R = (1UL << 31),
136-142: New RSS fields in struct rtl_hw look fine.Buffers and enable flag are appropriate. Consider zero-initializing rss_indir_tbl to a round‑robin default during init.
158-229: Descriptor structs: consider adding explicit packing/endianness notes.If HW requires exact layout, annotate with comments or static asserts. Current defs are likely fine on little‑endian x86.
301-306: rtl_get_rxdesc should return a byte pointer (void) to avoid type confusion across descriptor formats.*Returning struct rtl_rx_desc* can mislead when HW uses v3/v4 layouts.
-static inline struct rtl_rx_desc* -rtl_get_rxdesc(struct rtl_hw *hw, struct rtl_rx_desc *base, u32 const number) +static inline void* +rtl_get_rxdesc(struct rtl_hw *hw, void *base, u32 const number) { - return (struct rtl_rx_desc *)((u8 *)base + hw->RxDescLength * number); + return (void *)((u8 *)base + hw->RxDescLength * number); }drivers/net/r8169/r8169_hw.c (2)
858-877: Validate nb_rx_queues and clarify bit math in rtl8125_set_rss_hash_opt.
- Guard nb_rx_queues ∈ [1, 4] (HW limit) to avoid undefined settings.
- Using
& (BIT_0|BIT_1|BIT_2)works but& 0x7is clearer.static void rtl8125_set_rss_hash_opt(struct rtl_hw *hw, u16 nb_rx_queues) { - u32 hash_mask_len; + u32 hash_mask_len; u32 rss_ctrl; + /* HW supports up to 4 RX queues */ + if (nb_rx_queues == 0 || nb_rx_queues > 4) + nb_rx_queues = 4; + rss_ctrl = rte_log2_u32(nb_rx_queues); - rss_ctrl &= (BIT_0 | BIT_1 | BIT_2); + rss_ctrl &= 0x7; rss_ctrl <<= RSS_CPU_NUM_OFFSET; @@ hash_mask_len = rte_log2_u32(RTL_MAX_INDIRECTION_TABLE_ENTRIES); - hash_mask_len &= (BIT_0 | BIT_1 | BIT_2); + hash_mask_len &= 0x7; rss_ctrl |= hash_mask_len << RSS_MASK_BITS_OFFSET; RTL_W32(hw, RSS_CTRL_8125, rss_ctrl); }
1834-1838: EnableRss gating based on mcfg is reasonable.Consider documenting the threshold (mcfg ≥ 69) as the condition for RSS capability.
drivers/net/r8169/r8169_ethdev.c (2)
770-795: RETA query reads from cached table only.Consider syncing from HW during init so first query returns actual HW state.
838-880: rss_hash_conf_get: validate key buffer length before copy.Avoid copying into a too-small buffer.
- if (rss_conf->rss_key) { - rss_conf->rss_key_len = RTL_RSS_KEY_SIZE; - memcpy(rss_conf->rss_key, hw->rss_key, RTL_RSS_KEY_SIZE); - } + rss_conf->rss_key_len = RTL_RSS_KEY_SIZE; + if (rss_conf->rss_key) { + if (rss_conf->rss_key_len < RTL_RSS_KEY_SIZE) + return -EINVAL; + memcpy(rss_conf->rss_key, hw->rss_key, RTL_RSS_KEY_SIZE); + }drivers/net/r8169/r8169_rxtx.c (3)
611-618: Potential misprogramming of RX queue number encoding.rte_log2_u32() rounds up to next power-of-two; with nb_rx_queues not a power-of-two, HW may be programmed for more queues than allocated.
- Validate nb_rx_queues is a power-of-two when RSS/multiq is on, or adjust programming to reflect the actual number allocated.
629-634: Avoid fixed-size QUEUE_NUM_LIMIT stack array for RX ring regs.Risk of OOB if nb_rx_queues > 128; compute register per-iteration instead.
- u16 desc_start_addr_lo[QUEUE_NUM_LIMIT]; - desc_start_addr_lo[0] = RxDescAddrLow; - for (i = 1; i < nb_rx_queues; i++) - desc_start_addr_lo[i] = (u16)(RDSAR_Q1_LOW_8125 + (i - 1) * 8); + u16 desc_lo; ... - RTL_W32(hw, desc_start_addr_lo[i], + desc_lo = (i == 0) ? RxDescAddrLow + : (u16)(RDSAR_Q1_LOW_8125 + (i - 1) * 8); + RTL_W32(hw, desc_lo, (u64)rxq->hw_ring_phys_addr & DMA_BIT_MASK(32)); - RTL_W32(hw, desc_start_addr_lo[i] + 4, + RTL_W32(hw, desc_lo + 4, (u64)rxq->hw_ring_phys_addr >> 32);Also applies to: 647-651
1394-1401: Same for TX ring regs: drop QUEUE_NUM_LIMIT stack array.Compute desc register on the fly to avoid OOB and reduce stack.
- u16 desc_start_addr_lo[QUEUE_NUM_LIMIT]; ... - desc_start_addr_lo[0] = TxDescStartAddrLow; - for (i = 1; i < nb_tx_queues; i++) - desc_start_addr_lo[i] = (u16)(TNPDS_Q1_LOW_8125 + (i - 1) * 8); + u16 desc_lo; ... - RTL_W32(hw, desc_start_addr_lo[i], + desc_lo = (i == 0) ? TxDescStartAddrLow + : (u16)(TNPDS_Q1_LOW_8125 + (i - 1) * 8); + RTL_W32(hw, desc_lo, (u64)txq->hw_ring_phys_addr & DMA_BIT_MASK(32)); - RTL_W32(hw, desc_start_addr_lo[i] + 4, + RTL_W32(hw, desc_lo + 4, (u64)txq->hw_ring_phys_addr >> 32);Also applies to: 1424-1433
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (11)
.mailmap(1 hunks)MAINTAINERS(1 hunks)doc/guides/nics/features/r8169.ini(1 hunks)doc/guides/nics/r8169.rst(1 hunks)drivers/net/r8169/r8169_compat.h(2 hunks)drivers/net/r8169/r8169_ethdev.c(7 hunks)drivers/net/r8169/r8169_ethdev.h(5 hunks)drivers/net/r8169/r8169_hw.c(17 hunks)drivers/net/r8169/r8169_hw.h(1 hunks)drivers/net/r8169/r8169_phy.c(8 hunks)drivers/net/r8169/r8169_rxtx.c(28 hunks)
🧰 Additional context used
🧬 Code graph analysis (6)
drivers/net/r8169/r8169_hw.h (1)
drivers/net/r8169/r8169_hw.c (2)
rtl8125_store_rss_key(893-906)rtl8125_config_rss(908-916)
drivers/net/r8169/r8169_phy.c (1)
drivers/net/r8169/r8169_hw.c (3)
rtl_is_8125(2524-2528)rtl_eri_read(81-85)rtl_eri_write(154-159)
drivers/net/r8169/r8169_hw.c (2)
lib/eal/include/rte_bitops.h (1)
rte_log2_u32(1397-1404)drivers/net/r8169/r8169_phy.c (6)
rtl_mdio_real_read_phy_ocp(75-83)rtl_set_phy_mcu_patch_request(308-349)rtl_mdio_real_write_phy_ocp(122-130)rtl_clear_phy_mcu_patch_request(351-392)rtl_mdio_direct_read_phy_ocp(69-73)rtl_mdio_direct_write_phy_ocp(116-120)
drivers/net/r8169/r8169_ethdev.h (1)
drivers/net/r8169/r8169_rxtx.c (1)
rtl_get_rx_port_offloads(312-327)
drivers/net/r8169/r8169_rxtx.c (4)
drivers/net/r8169/r8169_ethdev.h (1)
rtl_get_rxdesc(301-305)lib/eal/include/rte_bitops.h (1)
rte_log2_u32(1397-1404)lib/mbuf/rte_mbuf.h (3)
rte_pktmbuf_data_room_size(919-926)rte_mbuf_raw_alloc(622-636)rte_mbuf_data_iova_default(203-207)drivers/net/r8169/r8169_hw.c (3)
rtl8125_config_rss(908-916)rtl_mac_ocp_read(424-437)rtl_mac_ocp_write(411-422)
drivers/net/r8169/r8169_ethdev.c (2)
drivers/net/r8169/r8169_rxtx.c (2)
rtl_get_rx_port_offloads(312-327)rtl_get_tx_port_offloads(1244-1257)drivers/net/r8169/r8169_hw.c (1)
rtl8125_store_rss_key(893-906)
🔇 Additional comments (27)
.mailmap (1)
685-685: Mailmap entry looks goodSorted correctly and formatted as expected.
MAINTAINERS (1)
1035-1042: Realtek r8169 maintainer addition LGTMEntry is correctly placed and formatted.
drivers/net/r8169/r8169_hw.h (1)
82-84: RSS helper prototypes OKInterfaces align with implementations and driver scope.
Please confirm nb_rx_queues is validated at call sites (>=1 and <= device max).
drivers/net/r8169/r8169_phy.c (4)
512-513: Consider symmetry for CFG_METHOD_91 in PMCH setPower-down avoids clearing PMCH for 91; power-up should likely avoid setting PMCH for 91 too.
Suggested change:
- if (!(hw->mcfg == CFG_METHOD_23 || hw->mcfg == CFG_METHOD_37)) + if (!(hw->mcfg == CFG_METHOD_23 || hw->mcfg == CFG_METHOD_37 || hw->mcfg == CFG_METHOD_91)) RTL_W8(hw, PMCH, RTL_R8(hw, PMCH) | BIT_7 | BIT_6);
473-480: Double-check wait/delay coverage for earlier methodsrtl_wait_phy_ups_resume only waits for 29–36 and 8125; mcfg 21–28 will not wait. The 10 ms delay is applied only for 23/27/28.
Please confirm older methods don’t require wait/settling or expand the ranges accordingly.
Also applies to: 501-503
627-646: CFG_METHOD_50 pre-MCU init pathWatchdog clear and setup logic are scoped and consistent with 8125 staging.
558-565: Verify whether CFG_METHOD_37 should be included in the 21–36 branch or requires separate handling.The gap is real: CFG_METHOD_37 does not fall within either conditional block. However, the codebase shows conflicting patterns. In other functions (e.g., lines 529, 813, 2053, 2076, 2118), CFG_METHOD_37 is grouped with the 21–36 range (e.g.,
CFG_METHOD_27 && hw->mcfg <= CFG_METHOD_37). Yet in the PMCH register handling just above (lines 553–555), CFG_METHOD_37 is explicitly excluded alongside CFG_METHOD_23 and 91.This inconsistency requires clarification: either the 0xD0/0xF2 clearing logic should extend the first branch to include 37, or there is hardware-specific reasoning for the exclusion that needs documentation.
doc/guides/nics/r8169.rst (1)
32-35: Docs: add corresponding features matrix entriesPlease ensure doc/guides/nics/features/r8169.ini reflects RSS (hash/key/RETA) and multi-queue.
drivers/net/r8169/r8169_compat.h (1)
313-316: Cfg9346_EEM_MASK addition looks good.Mask matches EEM1|EEM0 bits and is used safely before toggling unlock. LGTM.
If other places rely on equality with Cfg9346_Unlock, ensure they consistently mask with Cfg9346_EEM_MASK.
drivers/net/r8169/r8169_ethdev.h (1)
51-61: Good separation of RTE flags vs HW register flags.Macros RTL_RSS_OFFLOAD_ALL (RTE flags) vs RTL_RSS_CTRL_OFFLOAD_ALL (HW bits) are clear.
drivers/net/r8169/r8169_hw.c (8)
762-796: Cfg9346 write-gating logic is correct.Unlock/restore sequence and masks are handled safely. LGTM.
813-824: Timer INT clear path refactor is fine.Range checks simplify code without changing behavior for 8125 vs non‑8125.
878-892: RETA programming helper looks correct.Writes 4 entries per dword; matches v2 table layout. LGTM.
893-907: RSS key programming is fine.Sequential 32‑bit writes across 40‑byte key. LGTM.
918-938: RX descriptor type selection is clear.mcfg→v3/v4 mapping and register touches look correct.
1492-1552: Chipset name mapping refactor is OK.Consolidated mapping improves clarity; no behavior concerns.
1872-1885: RX descriptor metadata initialization looks good.Type and length are set coherently per chipset.
908-917: rtl8125_config_rss() is already being invoked during device start—this recommendation is already implemented.The function is called during the RX initialization sequence in
rtl_dev_start()→rtl_rx_init()→rtl8125_config_rss()(r8169_rxtx.c:706). When RSS is enabled (hw->EnableRss), it properly configures all RSS defaults: hash options, RETA, and RSS key via its three helper functions.Likely an incorrect or invalid review comment.
drivers/net/r8169/r8169_ethdev.c (5)
121-125: RSS ops wiring added to eth_dev_ops looks good.Functions are present and signatures match DPDK API.
167-185: Chipset-based autoneg speed selection is a nice cleanup.Mapping aligns with capabilities. LGTM.
418-424: Queue capabilities exposure is fine.4 RX / 2 TX for newer chipsets, 1/1 otherwise. LGTM.
553-563: stats_get signature change (qstats unused) is fine.No issues; style aligns with other PMDs.
298-346: Rewritten Review CommentThe suggested change is redundant.
rtl8125_config_rssis already called withinrtl_rx_initat line 706 ofdrivers/net/r8169/r8169_rxtx.c, after RSS initialization viartl_init_rss. Sincertl_rx_initis invoked inrtl_dev_startbefore any subsequent operations, RSS configuration already occurs at the appropriate point in the device startup sequence. Adding the suggested call inrtl_dev_startwould cause the function to be invoked twice, which is unnecessary.Likely an incorrect or invalid review comment.
drivers/net/r8169/r8169_rxtx.c (4)
703-710: RSS enable/disable and VMQ programming look good.Initialization path cleanly handles both RSS on/off and writes queue count; enabling Rx at the end is correct.
Please confirm RTL8125 expects queue-count programming after cfg9346 is disabled (ordering with other blocks sometimes matters on these chips).
Also applies to: 712-715
409-424: Descriptor helpers are a solid refactor.Unifying v1/v3/v4 via rtl_set_desc_dma_addr/rtl_mark_to_asic/rtl_map_to_asic reduces duplication and is correct with wmb before ownership.
Also applies to: 458-473, 474-480
719-733: RX fast-path logic looks correct overall.Good barriers, error recycling, VLAN/CSUM/RSS handling, and prefetching.
Also applies to: 735-749, 751-770, 772-791, 792-811, 812-828, 829-841, 867-887, 889-899, 919-935, 937-944
1689-1696: TX cleanup paths consistent per family.8125 vs 8168 split is clear; tx_free/head updates are correct with the underflow fix applied.
Also applies to: 1744-1776, 1778-1786
| dev_info->rx_offload_capa = (rtl_get_rx_port_offloads(hw) | | ||
| dev_info->rx_queue_offload_capa); | ||
| dev_info->tx_offload_capa = rtl_get_tx_port_offloads(); | ||
|
|
||
| dev_info->hash_key_size = RTL_RSS_KEY_SIZE; | ||
| dev_info->reta_size = RTL_MAX_INDIRECTION_TABLE_ENTRIES; | ||
| dev_info->flow_type_rss_offloads = RTL_RSS_CTRL_OFFLOAD_ALL; | ||
|
|
There was a problem hiding this comment.
flow_type_rss_offloads uses HW register bits; must use RTE RSS flags. Also avoid ORing with uninitialized queue capa.
- Set
dev_info->rx_offload_capadirectly from driver capabilities. - Use
RTL_RSS_OFFLOAD_ALLforflow_type_rss_offloads. - Only expose hash_key_size/reta_size when RSS is enabled; else 0.
- dev_info->rx_offload_capa = (rtl_get_rx_port_offloads(hw) |
- dev_info->rx_queue_offload_capa);
+ dev_info->rx_offload_capa = rtl_get_rx_port_offloads(hw);
@@
- dev_info->hash_key_size = RTL_RSS_KEY_SIZE;
- dev_info->reta_size = RTL_MAX_INDIRECTION_TABLE_ENTRIES;
- dev_info->flow_type_rss_offloads = RTL_RSS_CTRL_OFFLOAD_ALL;
+ if (hw->EnableRss) {
+ dev_info->hash_key_size = RTL_RSS_KEY_SIZE;
+ dev_info->reta_size = RTL_MAX_INDIRECTION_TABLE_ENTRIES;
+ dev_info->flow_type_rss_offloads = RTL_RSS_OFFLOAD_ALL;
+ } else {
+ dev_info->hash_key_size = 0;
+ dev_info->reta_size = 0;
+ dev_info->flow_type_rss_offloads = 0;
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| dev_info->rx_offload_capa = (rtl_get_rx_port_offloads(hw) | | |
| dev_info->rx_queue_offload_capa); | |
| dev_info->tx_offload_capa = rtl_get_tx_port_offloads(); | |
| dev_info->hash_key_size = RTL_RSS_KEY_SIZE; | |
| dev_info->reta_size = RTL_MAX_INDIRECTION_TABLE_ENTRIES; | |
| dev_info->flow_type_rss_offloads = RTL_RSS_CTRL_OFFLOAD_ALL; | |
| dev_info->rx_offload_capa = rtl_get_rx_port_offloads(hw); | |
| dev_info->tx_offload_capa = rtl_get_tx_port_offloads(); | |
| if (hw->EnableRss) { | |
| dev_info->hash_key_size = RTL_RSS_KEY_SIZE; | |
| dev_info->reta_size = RTL_MAX_INDIRECTION_TABLE_ENTRIES; | |
| dev_info->flow_type_rss_offloads = RTL_RSS_OFFLOAD_ALL; | |
| } else { | |
| dev_info->hash_key_size = 0; | |
| dev_info->reta_size = 0; | |
| dev_info->flow_type_rss_offloads = 0; | |
| } |
🤖 Prompt for AI Agents
In drivers/net/r8169/r8169_ethdev.c around lines 460 to 467, the code currently
ORs rx_offload_capa with an uninitialized rx_queue_offload_capa and assigns
flow_type_rss_offloads from HW register bits; change it to assign
dev_info->rx_offload_capa directly from the driver capability function (do not
OR with dev_info->rx_queue_offload_capa), set dev_info->flow_type_rss_offloads
to RTL_RSS_OFFLOAD_ALL (RTE RSS flags), and only populate
dev_info->hash_key_size and dev_info->reta_size when RSS is enabled (otherwise
set them to 0); leave tx_offload_capa as the driver-provided value.
| static int | ||
| rtl_reta_update(struct rte_eth_dev *dev, | ||
| struct rte_eth_rss_reta_entry64 *reta_conf, uint16_t reta_size) | ||
| { | ||
| struct rtl_adapter *adapter = RTL_DEV_PRIVATE(dev); | ||
| struct rtl_hw *hw = &adapter->hw; | ||
| u32 reta; | ||
| u16 idx, shift; | ||
| u8 mask, rss_indir_tbl; | ||
| int i, j; | ||
|
|
||
| if (reta_size != RTL_MAX_INDIRECTION_TABLE_ENTRIES) { | ||
| PMD_DRV_LOG(ERR, "The size of hash lookup table configured " | ||
| "(%d) doesn't match the number hardware can supported " | ||
| "(%d)", reta_size, RTL_MAX_INDIRECTION_TABLE_ENTRIES); | ||
| return -EINVAL; | ||
| } | ||
|
|
||
| for (i = 0; i < reta_size; i += 4) { | ||
| idx = i / RTE_ETH_RETA_GROUP_SIZE; | ||
| shift = i % RTE_ETH_RETA_GROUP_SIZE; | ||
| mask = (reta_conf[idx].mask >> shift) & 0xf; | ||
|
|
||
| if (!mask) | ||
| continue; | ||
|
|
||
| for (j = 0, reta = 0; j < 4; j++) { | ||
| rss_indir_tbl = (u8)reta_conf[idx].reta[shift + j]; | ||
| reta |= rss_indir_tbl << (j * 8); | ||
|
|
||
| if (!(mask & (1 << j))) | ||
| continue; | ||
|
|
||
| hw->rss_indir_tbl[i + j] = rss_indir_tbl; | ||
| } | ||
|
|
||
| RTL_W32(hw, RSS_INDIRECTION_TBL_8125_V2 + i, reta); | ||
| } | ||
|
|
||
| return 0; | ||
| } |
There was a problem hiding this comment.
Mask handling bug in RETA update; unmasked entries are overwritten. Validate queue IDs.
Build each 32‑bit RETA word from existing values, and only override masked positions. Also check target queue < nb_rx_queues.
static int
rtl_reta_update(struct rte_eth_dev *dev,
struct rte_eth_rss_reta_entry64 *reta_conf, uint16_t reta_size)
{
struct rtl_adapter *adapter = RTL_DEV_PRIVATE(dev);
struct rtl_hw *hw = &adapter->hw;
- u32 reta;
- u16 idx, shift;
- u8 mask, rss_indir_tbl;
- int i, j;
+ u32 reta;
+ u16 idx, shift;
+ u8 mask;
+ int i, j;
if (reta_size != RTL_MAX_INDIRECTION_TABLE_ENTRIES) {
@@
- for (i = 0; i < reta_size; i += 4) {
+ for (i = 0; i < reta_size; i += 4) {
idx = i / RTE_ETH_RETA_GROUP_SIZE;
shift = i % RTE_ETH_RETA_GROUP_SIZE;
- mask = (reta_conf[idx].mask >> shift) & 0xf;
+ mask = (reta_conf[idx].mask >> shift) & 0xF;
if (!mask)
continue;
- for (j = 0, reta = 0; j < 4; j++) {
- rss_indir_tbl = (u8)reta_conf[idx].reta[shift + j];
- reta |= rss_indir_tbl << (j * 8);
-
- if (!(mask & (1 << j)))
- continue;
-
- hw->rss_indir_tbl[i + j] = rss_indir_tbl;
- }
+ reta = 0;
+ for (j = 0; j < 4; j++) {
+ u8 v = hw->rss_indir_tbl[i + j];
+ if (mask & (1U << j)) {
+ v = (u8)reta_conf[idx].reta[shift + j];
+ if (v >= dev->data->nb_rx_queues)
+ return -EINVAL;
+ hw->rss_indir_tbl[i + j] = v;
+ }
+ reta |= ((u32)v) << (j * 8);
+ }
RTL_W32(hw, RSS_INDIRECTION_TBL_8125_V2 + i, reta);
}
return 0;
}| static int | ||
| rtl_rss_hash_update(struct rte_eth_dev *dev, struct rte_eth_rss_conf *rss_conf) | ||
| { | ||
| struct rtl_adapter *adapter = RTL_DEV_PRIVATE(dev); | ||
| struct rtl_hw *hw = &adapter->hw; | ||
| u32 rss_ctrl_8125; | ||
|
|
||
| if (!hw->EnableRss || !(rss_conf->rss_hf & RTL_RSS_OFFLOAD_ALL)) | ||
| return -EINVAL; | ||
|
|
||
| if (rss_conf->rss_key) | ||
| memcpy(hw->rss_key, rss_conf->rss_key, RTL_RSS_KEY_SIZE); | ||
|
|
||
| rtl8125_store_rss_key(hw); | ||
|
|
||
| rss_ctrl_8125 = RTL_R32(hw, RSS_CTRL_8125) & ~RTL_RSS_CTRL_OFFLOAD_ALL; | ||
|
|
||
| if (rss_conf->rss_hf & RTE_ETH_RSS_IPV4) | ||
| rss_ctrl_8125 |= RSS_CTRL_IPV4_SUPP; | ||
| if (rss_conf->rss_hf & RTE_ETH_RSS_NONFRAG_IPV4_TCP) | ||
| rss_ctrl_8125 |= RSS_CTRL_TCP_IPV4_SUPP; | ||
| if (rss_conf->rss_hf & RTE_ETH_RSS_NONFRAG_IPV6_TCP) | ||
| rss_ctrl_8125 |= RSS_CTRL_TCP_IPV6_SUPP; | ||
| if (rss_conf->rss_hf & RTE_ETH_RSS_IPV6) | ||
| rss_ctrl_8125 |= RSS_CTRL_IPV6_SUPP; | ||
| if (rss_conf->rss_hf & RTE_ETH_RSS_IPV6_EX) | ||
| rss_ctrl_8125 |= RSS_CTRL_IPV6_EXT_SUPP; | ||
| if (rss_conf->rss_hf & RTE_ETH_RSS_IPV6_TCP_EX) | ||
| rss_ctrl_8125 |= RSS_CTRL_TCP_IPV6_EXT_SUPP; | ||
| if (rss_conf->rss_hf & RTE_ETH_RSS_NONFRAG_IPV4_UDP) | ||
| rss_ctrl_8125 |= RSS_CTRL_UDP_IPV4_SUPP; | ||
| if (rss_conf->rss_hf & RTE_ETH_RSS_NONFRAG_IPV6_UDP) | ||
| rss_ctrl_8125 |= RSS_CTRL_UDP_IPV6_SUPP; | ||
| if (rss_conf->rss_hf & RTE_ETH_RSS_IPV6_UDP_EX) | ||
| rss_ctrl_8125 |= RSS_CTRL_UDP_IPV6_EXT_SUPP; | ||
|
|
||
| RTL_W32(hw, RSS_CTRL_8125, rss_ctrl_8125); | ||
|
|
||
| return 0; | ||
| } |
There was a problem hiding this comment.
rss_hash_update: accept zero mask (disable), validate key length, and reject unsupported bits instead of requiring any supported bit.
Also return -ENOTSUP when RSS not enabled.
static int
rtl_rss_hash_update(struct rte_eth_dev *dev, struct rte_eth_rss_conf *rss_conf)
{
struct rtl_adapter *adapter = RTL_DEV_PRIVATE(dev);
struct rtl_hw *hw = &adapter->hw;
u32 rss_ctrl_8125;
- if (!hw->EnableRss || !(rss_conf->rss_hf & RTL_RSS_OFFLOAD_ALL))
- return -EINVAL;
+ if (!hw->EnableRss)
+ return -ENOTSUP;
+ /* Only allow supported bits */
+ if (rss_conf->rss_hf & ~RTL_RSS_OFFLOAD_ALL)
+ return -EINVAL;
- if (rss_conf->rss_key)
- memcpy(hw->rss_key, rss_conf->rss_key, RTL_RSS_KEY_SIZE);
+ if (rss_conf->rss_key) {
+ if (rss_conf->rss_key_len != RTL_RSS_KEY_SIZE)
+ return -EINVAL;
+ memcpy(hw->rss_key, rss_conf->rss_key, RTL_RSS_KEY_SIZE);
+ }
rtl8125_store_rss_key(hw);
rss_ctrl_8125 = RTL_R32(hw, RSS_CTRL_8125) & ~RTL_RSS_CTRL_OFFLOAD_ALL;
@@
- RTL_W32(hw, RSS_CTRL_8125, rss_ctrl_8125);
+ /* If rss_hf == 0, this clears all offload bits (disable RSS hashing). */
+ RTL_W32(hw, RSS_CTRL_8125, rss_ctrl_8125);
return 0;
}🤖 Prompt for AI Agents
In drivers/net/r8169/r8169_ethdev.c around lines 797-836, update
rtl_rss_hash_update to: return -ENOTSUP instead of -EINVAL when hw->EnableRss is
false; allow rss_conf->rss_hf == 0 as a valid call to disable RSS (do not treat
zero as error); if rss_conf->rss_key is non-NULL validate its length equals
RTL_RSS_KEY_SIZE and return -EINVAL (or appropriate error) if it does not;
compute a mask of supported RSS bits and reject any bits in rss_conf->rss_hf
that are unsupported by returning -ENOTSUP (rather than requiring at least one
supported bit), and keep existing logic to set hw->rss_key, call
rtl8125_store_rss_key, compute rss_ctrl_8125 based on supported flags, and write
it with RTL_W32.
| if (hw->mcfg >= CFG_METHOD_21 && hw->mcfg <= CFG_METHOD_24) { | ||
| csi_tmp = rtl_eri_read(hw, 0x1AB, 1, ERIAR_ExGMAC); | ||
| csi_tmp &= ~(BIT_2 | BIT_3 | BIT_4 | BIT_5 | BIT_6 | BIT_7); | ||
| rtl_eri_write(hw, 0x1AB, 1, csi_tmp, ERIAR_ExGMAC); | ||
| break; | ||
| case CFG_METHOD_25: | ||
| case CFG_METHOD_27: | ||
| case CFG_METHOD_28: | ||
| case CFG_METHOD_29: | ||
| case CFG_METHOD_30: | ||
| case CFG_METHOD_31: | ||
| case CFG_METHOD_32: | ||
| case CFG_METHOD_33: | ||
| case CFG_METHOD_34: | ||
| case CFG_METHOD_35: | ||
| case CFG_METHOD_36: | ||
| case CFG_METHOD_37: | ||
| } else if (hw->mcfg == CFG_METHOD_25 || | ||
| (hw->mcfg >= CFG_METHOD_27 && hw->mcfg <= CFG_METHOD_37)) { | ||
| csi_tmp = rtl_eri_read(hw, 0x1AB, 1, ERIAR_ExGMAC); | ||
| csi_tmp &= ~(BIT_3 | BIT_6); | ||
| rtl_eri_write(hw, 0x1AB, 1, csi_tmp, ERIAR_ExGMAC); | ||
| break; | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
CFG_METHOD_26 appears unhandled in MCU PME intr masks
Case 26 is not covered by either branch; likely should follow the 25/27–37 path.
If intended to behave like 25/27–37, apply:
-} else if (hw->mcfg == CFG_METHOD_25 ||
- (hw->mcfg >= CFG_METHOD_27 && hw->mcfg <= CFG_METHOD_37)) {
+} else if (hw->mcfg == CFG_METHOD_25 || hw->mcfg == CFG_METHOD_26 ||
+ (hw->mcfg >= CFG_METHOD_27 && hw->mcfg <= CFG_METHOD_37)) {
---
🏁 Script executed:
```shell
#!/bin/bash
# Search for CFG_METHOD_26 in the driver file and nearby context
rg -n 'CFG_METHOD_26' drivers/net/r8169/r8169_phy.c -B5 -A5
Length of output: 617
🏁 Script executed:
#!/bin/bash
# Search for all CFG_METHOD ranges in the file to understand the pattern
rg -n 'CFG_METHOD_(2[1-9]|3[0-9]|4[0-9])' drivers/net/r8169/r8169_phy.c | head -50Length of output: 1948
🏁 Script executed:
#!/bin/bash
# Look at the specific code block in question with broader context
sed -n '510,545p' drivers/net/r8169/r8169_phy.cLength of output: 1076
🏁 Script executed:
#!/bin/bash
# Search for other conditionals handling CFG_METHOD ranges to see the pattern
rg -n 'CFG_METHOD_25|CFG_METHOD_27' drivers/net/r8169/r8169_phy.c -B3 -A3Length of output: 1962
🏁 Script executed:
#!/bin/bash
# Check if CFG_METHOD_26 is defined in header files
fd -e h | xargs grep -l 'CFG_METHOD' | head -5 | xargs grep 'CFG_METHOD_26'Length of output: 129
CFG_METHOD_26 is unhandled in MCU PME intr masks block—include in else-if condition
CFG_METHOD_26 is a valid, defined method (confirmed in r8169_compat.h and r8169_hw.h) but falls into neither branch at lines 524–533. The gap between CFG_METHOD_25 (explicit) and CFG_METHOD_27–37 (range) leaves method 26 without MCU PME interrupt mask configuration. Other code patterns (e.g., line 896: hw->mcfg >= CFG_METHOD_25 && hw->mcfg <= CFG_METHOD_36) indicate 26 should be grouped with 25 and 27–37. Apply the proposed fix:
-} else if (hw->mcfg == CFG_METHOD_25 ||
- (hw->mcfg >= CFG_METHOD_27 && hw->mcfg <= CFG_METHOD_37)) {
+} else if (hw->mcfg == CFG_METHOD_25 || hw->mcfg == CFG_METHOD_26 ||
+ (hw->mcfg >= CFG_METHOD_27 && hw->mcfg <= CFG_METHOD_37)) {🤖 Prompt for AI Agents
In drivers/net/r8169/r8169_phy.c around lines 524–533, CFG_METHOD_26 is not
covered by the MCU PME interrupt mask branches causing it to miss mask
configuration; update the else-if condition so CFG_METHOD_26 is included (e.g.,
change the condition to group CFG_METHOD_25 through CFG_METHOD_37 or explicitly
include CFG_METHOD_26) so the same rtl_eri_read/modify/write path that clears
the appropriate bits runs for method 26 as well.
| rtl_clear_phy_mcu_patch_request(hw); | ||
|
|
||
| rte_delay_ms(2); |
There was a problem hiding this comment.
Preserve result of rtl_clear_phy_mcu_patch_request
Return value is ignored, possibly masking failure in retval.
Apply:
- rtl_clear_phy_mcu_patch_request(hw);
+ if (!rtl_clear_phy_mcu_patch_request(hw))
+ retval = FALSE;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| rtl_clear_phy_mcu_patch_request(hw); | |
| rte_delay_ms(2); | |
| if (!rtl_clear_phy_mcu_patch_request(hw)) | |
| retval = FALSE; | |
| rte_delay_ms(2); |
🤖 Prompt for AI Agents
In drivers/net/r8169/r8169_phy.c around lines 810 to 812, the return value of
rtl_clear_phy_mcu_patch_request is ignored which can hide failures; capture its
return (e.g. retval = rtl_clear_phy_mcu_patch_request(hw)), check for an error
(if retval) and handle it appropriately by returning the error or logging and
jumping to cleanup so failures aren’t silently ignored, then keep the
rte_delay_ms(2) only when the call succeeded if that timing depends on success.
| for (i = 0; i < rxq->nb_rx_desc; i++) | ||
| rxq->hw_ring[i] = zero_rxd; | ||
| memset(rxq->hw_ring, 0, hw->RxDescLength * rxq->nb_rx_desc); | ||
|
|
||
| rtl_mark_as_last_descriptor(hw, rtl_get_rxdesc(hw, rxq->hw_ring, | ||
| rxq->nb_rx_desc - 1)); | ||
|
|
There was a problem hiding this comment.
RX reset zeroing entire ring inside a loop (O(n^2) work).
The loop calls memset over the entire ring on every iteration; wasteful.
Use a single memset:
- int i;
- for (i = 0; i < rxq->nb_rx_desc; i++)
- memset(rxq->hw_ring, 0, hw->RxDescLength * rxq->nb_rx_desc);
+ memset(rxq->hw_ring, 0, hw->RxDescLength * rxq->nb_rx_desc);Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In drivers/net/r8169/r8169_rxtx.c around lines 301 to 306, the code currently
calls memset over the entire RX ring inside a for loop causing O(n^2) work;
replace the loop with a single call to memset once that zeroes the whole ring
(use the existing size expression hw->RxDescLength * rxq->nb_rx_desc) and keep
the subsequent rtl_mark_as_last_descriptor call unchanged so only one full-ring
memset is performed.
| rtl_mark_as_last_descriptor(hw, rtl_get_rxdesc(hw, rxq->hw_ring, i)); | ||
|
|
There was a problem hiding this comment.
OOB access: last RX descriptor marked at index i (== nb_rx_desc).
After the fill loop, i == nb_rx_desc. Using i indexes past the ring end.
Fix:
- rtl_mark_as_last_descriptor(hw, rtl_get_rxdesc(hw, rxq->hw_ring, i));
+ rtl_mark_as_last_descriptor(hw, rtl_get_rxdesc(hw, rxq->hw_ring,
+ rxq->nb_rx_desc - 1));📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| rtl_mark_as_last_descriptor(hw, rtl_get_rxdesc(hw, rxq->hw_ring, i)); | |
| rtl_mark_as_last_descriptor(hw, rtl_get_rxdesc(hw, rxq->hw_ring, | |
| rxq->nb_rx_desc - 1)); |
🤖 Prompt for AI Agents
In drivers/net/r8169/r8169_rxtx.c around lines 506-507, the code calls
rtl_get_rxdesc(hw, rxq->hw_ring, i) after the fill loop where i == nb_rx_desc,
causing an out-of-bounds access; replace the index with the last valid ring slot
by computing the previous index (e.g. last = (i + nb_rx_desc - 1) % nb_rx_desc)
and call rtl_get_rxdesc(..., last) so the last descriptor is marked without
indexing past the ring end.
| for (i = 0; i < RTL_MAX_INDIRECTION_TABLE_ENTRIES; i++) | ||
| hw->rss_indir_tbl[i] = i & (nb_rx_queues - 1); | ||
|
|
||
| for (i = 0; i < RTL_RSS_KEY_SIZE; i += sizeof(u64)) { | ||
| rand = rte_rand(); | ||
| memcpy(&hw->rss_key[i], &rand, sizeof(u64)); | ||
| } | ||
| } |
There was a problem hiding this comment.
RSS indirection uses bitmask; requires power-of-two queues.
Using i & (nb_rx_queues - 1) silently breaks for non-power-of-two.
Either enforce power-of-two or switch to modulo. Modulo keeps behavior general:
- for (i = 0; i < RTL_MAX_INDIRECTION_TABLE_ENTRIES; i++)
- hw->rss_indir_tbl[i] = i & (nb_rx_queues - 1);
+ for (i = 0; i < RTL_MAX_INDIRECTION_TABLE_ENTRIES; i++)
+ hw->rss_indir_tbl[i] = i % nb_rx_queues;Also consider validating queue count when enabling RSS.
🤖 Prompt for AI Agents
In drivers/net/r8169/r8169_rxtx.c around lines 596 to 603 the RSS indirection
table uses a bitmask (i & (nb_rx_queues - 1)), which only works for power-of-two
nb_rx_queues and will produce incorrect mappings for other values; change the
assignment to use modulo (i % nb_rx_queues) to preserve correct distribution for
arbitrary queue counts, and add a validation step where RSS is enabled to either
enforce/require nb_rx_queues be a power-of-two or warn/error if it is not
(preferably allow arbitrary counts by using the modulo change and emit a
debug/warning if nb_rx_queues is not a power-of-two to inform the user).
| first_seg->pkt_len -= RTE_ETHER_CRC_LEN; | ||
| if (data_len <= RTE_ETHER_CRC_LEN) { | ||
| rte_pktmbuf_free_seg(rmb); | ||
| first_seg->nb_segs--; | ||
| last_seg->data_len = last_seg->data_len - | ||
| (RTE_ETHER_CRC_LEN - data_len); | ||
| last_seg->next = NULL; | ||
| } else { | ||
| rmb->data_len = data_len - RTE_ETHER_CRC_LEN; | ||
| } |
There was a problem hiding this comment.
Possible NULL deref of last_seg when trimming CRC on single-frag frames.
If the packet is a single segment and data_len <= 4, last_seg is NULL.
Guard and treat the current mbuf as last_seg:
- if (data_len <= RTE_ETHER_CRC_LEN) {
+ if (data_len <= RTE_ETHER_CRC_LEN) {
+ if (last_seg == NULL)
+ last_seg = rmb;
rte_pktmbuf_free_seg(rmb);
first_seg->nb_segs--;
last_seg->data_len = last_seg->data_len -
(RTE_ETHER_CRC_LEN - data_len);
last_seg->next = NULL;Committable suggestion skipped: line range outside the PR's diff.
🤖 Prompt for AI Agents
In drivers/net/r8169/r8169_rxtx.c around lines 1116 to 1125, the code can
dereference last_seg when handling CRC trimming on frames where data_len <=
RTE_ETHER_CRC_LEN and the packet is single-segment (last_seg is NULL). Fix by
detecting when last_seg == NULL and treating the current mbuf chain head (e.g.,
first_seg) as last_seg before adjusting data_len and next; i.e., set last_seg =
first_seg when last_seg is NULL, then proceed to decrement last_seg->data_len,
set last_seg->next = NULL and update nb_segs after freeing the tail segment.
| if (enable_tx_no_close) { | ||
| next_hw_desc_clo_ptr0 = rtl_get_hw_clo_ptr(hw); | ||
| hw->NextHwDesCloPtr0 = next_hw_desc_clo_ptr0; | ||
| tx_desc_closed = rtl_fast_mod_mask(next_hw_desc_clo_ptr0 - | ||
| hw->BeginHwDesCloPtr0, hw->MaxTxDescPtrMask); | ||
| txq->NextHwDesCloPtr = rtl_get_hw_clo_ptr(txq); | ||
| tx_desc_closed = rtl_fast_mod_mask(txq->NextHwDesCloPtr - | ||
| txq->BeginHwDesCloPtr, | ||
| hw->MaxTxDescPtrMask); | ||
| tx_left = RTE_MIN(((rte_atomic_load_explicit(&txq->tx_tail, | ||
| rte_memory_order_relaxed) % nb_tx_desc) - head), | ||
| tx_desc_closed); | ||
| hw->BeginHwDesCloPtr0 += tx_left; | ||
| txq->BeginHwDesCloPtr += tx_left; | ||
| } else { |
There was a problem hiding this comment.
tx_left underflow when EnableTxNoClose is false (8125 paths).
Computing (tail % nb) - head can wrap negative to a huge uint, causing over-free or hangs.
Apply in both functions:
- } else {
- tx_left = (rte_atomic_load_explicit(&txq->tx_tail,
- rte_memory_order_relaxed) % nb_tx_desc) - head;
- }
+ } else {
+ uint16_t tail = (uint16_t)(rte_atomic_load_explicit(&txq->tx_tail,
+ rte_memory_order_relaxed) % nb_tx_desc);
+ tx_left = (tail >= head) ? (tail - head) : (nb_tx_desc - (head - tail));
+ }Also applies to: 1813-1822
Auto-submission for "http://patchwork.dpdk.org/project/dpdk/list/?series=36484"
Summary by Sourcery
Enable multi-queue operation and RSS on the r8169 driver by adding configurable RX/TX queue numbers, descriptor format abstraction, and RSS key/RETA management APIs, while refactoring and consolidating descriptor and hardware-specific logic.
New Features:
Enhancements:
Documentation:
Summary by CodeRabbit
Release Notes
New Features
Documentation