[PATCH v5] r8169: migrate Rx path to page_pool, prepare for XDP

[PATCH v5] r8169: migrate Rx path to page_pool, prepare for XDP

[Atharva Potdar]

Replace the alloc_pages() + dma_map_page() + skb_copy() Rx model with
a page_pool-backed napi_build_skb() path for zero-copy delivery.
PP_FLAG_DMA_MAP and PP_FLAG_DMA_SYNC_DEV delegate DMA mapping and
cache sync to the page_pool core for correct cross-architecture
behaviour.

MACs up to RTL_GIGA_MAC_VER_06 are kept on the copy path with no
headroom. Newer MACs reserve XDP_PACKET_HEADROOM (256 B) per buffer
via pp_params.offset and desc->addr, avoiding pskb_expand_head()
reallocations on forwarded traffic and establishing the headroom
contract expected by later XDP patches.

The pool uses order-2 (SZ_16K) pages on all MACs. Old silicon in this
family ignores the descriptor buffer size limit, allowing oversized
packets to corrupt memory (see fdd7b4c3302c, c0cd884af045,
6f0333b8fde4). opts1 and napi_build_skb() both derive their sizes
from tp->rx_buf_sz = SZ_16K - SKB_DATA_ALIGN(sizeof(struct
skb_shared_info)) - rx_headroom, keeping the NIC write window and skb
tailroom consistent and within the allocation.

Rx consumption (cur_rx) is decoupled from refill (dirty_rx).
xdp_rxq_info is registered at pool creation time for the MEM_TYPE_
PAGE_POOL return path required by XDP redirect.

Signed-off-by: Atharva Potdar <atharvapotdar07@gmail.com>
---
Testing on RTL8168h, x86, bare metal:
- Verified napi_build_skb() is taken for all Rx packets via bpftrace.
  No copy-path fallback observed at 1Gbps line rate.
- Confirmed 256B skb headroom on eno1 via bpftrace (403091 packets).
  No pskb_expand_head() calls during routed traffic.
- Cycled ip link down/up under load. Ring repopulates cleanly after
  rtl_reset_work(); no stale descriptor accesses.
- 9000-byte MTU TCP streams at line rate (992 Mbps). nstat showed no
  TcpExtTCPRcvQDrop or OfoPrune regressions.

v5:
 - Introduced tp->rx_buf_sz, derived from SZ_16K -
   SKB_DATA_ALIGN(sizeof(struct skb_shared_info)) - rx_headroom.
   Used for both opts1 and napi_build_skb() frag_size, fixing a
   255-byte DMA overflow past the end of the order-2 page on modern
   MACs and a potential skb_put() panic on large frames.
 - Fixed rtl8169_rx_clear() to iterate all NUM_RX_DESC entries
   unconditionally. Early exit on NULL leaked pool pages on teardown
   after any zero-copy traffic.
 - Fixed rtl_reset_work() to repopulate NULL Rx_databuff slots before
   re-arming descriptors, preventing DMA into stale addresses after
   a reset.
 - Removed spurious #include <linux/if_link.h>.
 - Fixed napi_build_skb() frag_size: must be the total page allocation
   (SZ_16K), not the pre-reduced rx_buf_sz + rx_headroom. The function
   subtracts SKB_DATA_ALIGN(sizeof(struct skb_shared_info)) internally;
   passing an already-reduced value caused a double subtraction, giving
   skb tailroom SBI_ALIGN bytes smaller than opts1 permits and risking
   skb_over_panic on large frames.
v4:
 - Bifurcated headroom by MAC version (0 for legacy, 256 for modern).
   MACs <= RTL_GIGA_MAC_VER_06 retained on copy path as a conservative
   choice given limited test coverage, not a known version-specific
   erratum.
 - Locked pool to order-2 allocations matching R8169_RX_BUF_SIZE.
   Old silicon ignores the descriptor buffer size limit; undersized
   buffers cause memory corruption (fdd7b4c3302c, c0cd884af045,
   6f0333b8fde4).
 - Fixed prefetch offset to align with payload start.
v3:
 - Reserved XDP_PACKET_HEADROOM per buffer to prevent pskb_expand_head()
   on forwarded traffic.
 - Decoupled Rx consumption (cur_rx) from buffer refill (dirty_rx).
 - Registered xdp_rxq_info mem model at pool creation time.
v2:
 - Reverted buffer size to SZ_16K to prevent MTU regression and restore
   protection against the chipset buffer-limit erratum.
v1:
 - Initial page_pool migration.

 drivers/net/ethernet/realtek/r8169_main.c | 173 ++++++++++++++++++----
 1 file changed, 141 insertions(+), 32 deletions(-)

diff --git a/drivers/net/ethernet/realtek/r8169_main.c b/drivers/net/ethernet/realtek/r8169_main.c
index ec4fc21fa..c548e7e8d 100644
--- a/drivers/net/ethernet/realtek/r8169_main.c
+++ b/drivers/net/ethernet/realtek/r8169_main.c
@@ -31,7 +31,9 @@
 #include <linux/unaligned.h>
 #include <net/ip6_checksum.h>
 #include <net/netdev_queues.h>
+#include <net/page_pool/helpers.h>
 #include <net/phy/realtek_phy.h>
+#include <net/xdp.h>
 
 #include "r8169.h"
 #include "r8169_firmware.h"
@@ -729,6 +731,10 @@ enum rtl_dash_type {
 };
 
 struct rtl8169_private {
+	struct page_pool *rx_pool;
+	struct xdp_rxq_info xdp_rxq;
+	u32 rx_headroom;
+	u32 rx_buf_sz;
 	void __iomem *mmio_addr;	/* memory map physical address */
 	struct pci_dev *pci_dev;
 	struct net_device *dev;
@@ -739,6 +745,7 @@ struct rtl8169_private {
 	u32 cur_rx; /* Index into the Rx descriptor buffer of next Rx pkt. */
 	u32 cur_tx; /* Index into the Tx descriptor buffer of next Rx pkt. */
 	u32 dirty_tx;
+	u32 dirty_rx;
 	struct TxDesc *TxDescArray;	/* 256-aligned Tx descriptor ring */
 	struct RxDesc *RxDescArray;	/* 256-aligned Rx descriptor ring */
 	dma_addr_t TxPhyAddr;
@@ -2622,6 +2629,7 @@ static void rtl_init_rxcfg(struct rtl8169_private *tp)
 static void rtl8169_init_ring_indexes(struct rtl8169_private *tp)
 {
 	tp->dirty_tx = tp->cur_tx = tp->cur_rx = 0;
+	tp->dirty_rx = 0;
 }
 
 static void rtl_jumbo_config(struct rtl8169_private *tp)
@@ -4148,37 +4156,30 @@ static int rtl8169_change_mtu(struct net_device *dev, int new_mtu)
 	return 0;
 }
 
-static void rtl8169_mark_to_asic(struct RxDesc *desc)
+static void rtl8169_mark_to_asic(struct RxDesc *desc, u32 rx_buf_sz)
 {
 	u32 eor = le32_to_cpu(desc->opts1) & RingEnd;
 
 	desc->opts2 = 0;
 	/* Force memory writes to complete before releasing descriptor */
 	dma_wmb();
-	WRITE_ONCE(desc->opts1, cpu_to_le32(DescOwn | eor | R8169_RX_BUF_SIZE));
+	WRITE_ONCE(desc->opts1, cpu_to_le32(DescOwn | eor | rx_buf_sz));
 }
 
 static struct page *rtl8169_alloc_rx_data(struct rtl8169_private *tp,
 					  struct RxDesc *desc)
 {
-	struct device *d = tp_to_dev(tp);
-	int node = dev_to_node(d);
 	dma_addr_t mapping;
 	struct page *data;
 
-	data = alloc_pages_node(node, GFP_KERNEL, get_order(R8169_RX_BUF_SIZE));
+	data = page_pool_dev_alloc_pages(tp->rx_pool);
 	if (!data)
 		return NULL;
 
-	mapping = dma_map_page(d, data, 0, R8169_RX_BUF_SIZE, DMA_FROM_DEVICE);
-	if (unlikely(dma_mapping_error(d, mapping))) {
-		netdev_err(tp->dev, "Failed to map RX DMA!\n");
-		__free_pages(data, get_order(R8169_RX_BUF_SIZE));
-		return NULL;
-	}
+	mapping = page_pool_get_dma_addr(data) + tp->rx_headroom;
 
 	desc->addr = cpu_to_le64(mapping);
-	rtl8169_mark_to_asic(desc);
+	rtl8169_mark_to_asic(desc, tp->rx_buf_sz);
 
 	return data;
 }
@@ -4187,15 +4188,21 @@ static void rtl8169_rx_clear(struct rtl8169_private *tp)
 {
 	int i;
 
-	for (i = 0; i < NUM_RX_DESC && tp->Rx_databuff[i]; i++) {
-		dma_unmap_page(tp_to_dev(tp),
-			       le64_to_cpu(tp->RxDescArray[i].addr),
-			       R8169_RX_BUF_SIZE, DMA_FROM_DEVICE);
-		__free_pages(tp->Rx_databuff[i], get_order(R8169_RX_BUF_SIZE));
+	for (i = 0; i < NUM_RX_DESC; i++) {
+		if (!tp->Rx_databuff[i])
+			continue;
+		page_pool_put_full_page(tp->rx_pool, tp->Rx_databuff[i], false);
 		tp->Rx_databuff[i] = NULL;
 		tp->RxDescArray[i].addr = 0;
 		tp->RxDescArray[i].opts1 = 0;
 	}
+
+	if (tp->rx_pool) {
+		if (xdp_rxq_info_is_reg(&tp->xdp_rxq))
+			xdp_rxq_info_unreg(&tp->xdp_rxq);
+		page_pool_destroy(tp->rx_pool);
+		tp->rx_pool = NULL;
+	}
 }
 
 static int rtl8169_rx_fill(struct rtl8169_private *tp)
@@ -4221,12 +4228,57 @@ static int rtl8169_rx_fill(struct rtl8169_private *tp)
 
 static int rtl8169_init_ring(struct rtl8169_private *tp)
 {
+	struct page_pool_params params = {0};
+	int err;
+
 	rtl8169_init_ring_indexes(tp);
 
+	if (tp->mac_version <= RTL_GIGA_MAC_VER_06) {
+		tp->rx_headroom = 0;
+		tp->rx_buf_sz = R8169_RX_BUF_SIZE;
+	} else {
+		tp->rx_headroom = XDP_PACKET_HEADROOM;
+		tp->rx_buf_sz = SZ_16K -
+						SKB_DATA_ALIGN(sizeof(struct skb_shared_info)) -
+						XDP_PACKET_HEADROOM;
+	}
+
+	params.order = get_order(R8169_RX_BUF_SIZE);
+	params.flags = PP_FLAG_DMA_MAP | PP_FLAG_DMA_SYNC_DEV;
+	params.pool_size = NUM_RX_DESC;
+	params.nid = dev_to_node(tp_to_dev(tp));
+	params.dev = tp_to_dev(tp);
+	params.napi = &tp->napi;
+	params.dma_dir = DMA_FROM_DEVICE;
+	params.offset = tp->rx_headroom;
+	params.max_len = tp->rx_buf_sz;
+
+	tp->rx_pool = page_pool_create(&params);
+	if (IS_ERR(tp->rx_pool)) {
+		err = PTR_ERR(tp->rx_pool);
+		tp->rx_pool = NULL;
+		return err;
+	}
+
+	err = xdp_rxq_info_reg(&tp->xdp_rxq, tp->dev, 0, tp->napi.napi_id);
+	if (err)
+		goto err_free_pool;
+
+	err = xdp_rxq_info_reg_mem_model(&tp->xdp_rxq, MEM_TYPE_PAGE_POOL, tp->rx_pool);
+	if (err)
+		goto err_unreg_rxq;
+
 	memset(tp->tx_skb, 0, sizeof(tp->tx_skb));
 	memset(tp->Rx_databuff, 0, sizeof(tp->Rx_databuff));
 
 	return rtl8169_rx_fill(tp);
+
+err_unreg_rxq:
+	xdp_rxq_info_unreg(&tp->xdp_rxq);
+err_free_pool:
+	page_pool_destroy(tp->rx_pool);
+	tp->rx_pool = NULL;
+	return err;
 }
 
 static void rtl8169_unmap_tx_skb(struct rtl8169_private *tp, unsigned int entry)
@@ -4311,8 +4363,20 @@ static void rtl_reset_work(struct rtl8169_private *tp)
 
 	rtl8169_cleanup(tp);
 
-	for (i = 0; i < NUM_RX_DESC; i++)
-		rtl8169_mark_to_asic(tp->RxDescArray + i);
+	for (i = 0; i < NUM_RX_DESC; i++) {
+		if (tp->Rx_databuff[i])
+			continue;
+		tp->Rx_databuff[i] =
+			rtl8169_alloc_rx_data(tp, tp->RxDescArray + i);
+		if (!tp->Rx_databuff[i])
+			break;
+	}
+
+	for (i = 0; i < NUM_RX_DESC; i++) {
+		if (!tp->Rx_databuff[i])
+			continue;
+		rtl8169_mark_to_asic(tp->RxDescArray + i, tp->rx_buf_sz);
+	}
 
 	napi_enable(&tp->napi);
 	rtl_hw_start(tp);
@@ -4768,16 +4832,39 @@ static inline void rtl8169_rx_csum(struct sk_buff *skb, u32 opts1)
 		skb_checksum_none_assert(skb);
 }
 
+static void rtl8169_rx_refill(struct rtl8169_private *tp)
+{
+	while (tp->dirty_rx != tp->cur_rx) {
+		unsigned int entry = tp->dirty_rx % NUM_RX_DESC;
+		struct RxDesc *desc = tp->RxDescArray + entry;
+
+		if (!tp->Rx_databuff[entry]) {
+			struct page *new_page = page_pool_dev_alloc_pages(tp->rx_pool);
+
+			if (unlikely(!new_page))
+				break;
+
+			tp->Rx_databuff[entry] = new_page;
+
+			desc->addr = cpu_to_le64(page_pool_get_dma_addr(new_page) +
+				tp->rx_headroom);
+		}
+		rtl8169_mark_to_asic(desc, tp->rx_buf_sz);
+
+		tp->dirty_rx++;
+	}
+}
+
 static int rtl_rx(struct net_device *dev, struct rtl8169_private *tp, int budget)
 {
 	struct device *d = tp_to_dev(tp);
 	int count;
 
-	for (count = 0; count < budget; count++, tp->cur_rx++) {
+	for (count = 0; count < budget;) {
 		unsigned int pkt_size, entry = tp->cur_rx % NUM_RX_DESC;
 		struct RxDesc *desc = tp->RxDescArray + entry;
 		struct sk_buff *skb;
-		const void *rx_buf;
+		void *rx_buf;
 		dma_addr_t addr;
 		u32 status;
 
@@ -4820,21 +4907,40 @@ static int rtl_rx(struct net_device *dev, struct rtl8169_private *tp, int budget
 			goto release_descriptor;
 		}
 
-		skb = napi_alloc_skb(&tp->napi, pkt_size);
-		if (unlikely(!skb)) {
-			dev->stats.rx_dropped++;
-			goto release_descriptor;
-		}
+		if (unlikely(!tp->Rx_databuff[entry]))
+			break;
 
 		addr = le64_to_cpu(desc->addr);
 		rx_buf = page_address(tp->Rx_databuff[entry]);
 
 		dma_sync_single_for_cpu(d, addr, pkt_size, DMA_FROM_DEVICE);
-		prefetch(rx_buf);
-		skb_copy_to_linear_data(skb, rx_buf, pkt_size);
-		skb->tail += pkt_size;
-		skb->len = pkt_size;
-		dma_sync_single_for_device(d, addr, pkt_size, DMA_FROM_DEVICE);
+		prefetch(rx_buf + tp->rx_headroom);
+
+		if (unlikely(tp->rx_headroom == 0)) {
+			skb = napi_alloc_skb(&tp->napi, pkt_size);
+			if (likely(skb)) {
+				skb_copy_to_linear_data(skb, rx_buf, pkt_size);
+				skb_put(skb, pkt_size);
+			}
+			dma_sync_single_for_device(d, addr, pkt_size, DMA_FROM_DEVICE);
+		} else {
+			skb = napi_build_skb(rx_buf, tp->rx_buf_sz + tp->rx_headroom
+				+ SKB_DATA_ALIGN(sizeof(struct skb_shared_info)));
+			if (likely(skb)) {
+				skb_reserve(skb, tp->rx_headroom);
+				skb_put(skb, pkt_size);
+				skb_mark_for_recycle(skb);
+
+				tp->Rx_databuff[entry] = NULL;
+			} else {
+				dma_sync_single_for_device(d, addr, pkt_size, DMA_FROM_DEVICE);
+			}
+		}
+
+		if (unlikely(!skb)) {
+			dev->stats.rx_dropped++;
+			goto release_descriptor;
+		}
 
 		rtl8169_rx_csum(skb, status);
 		skb->protocol = eth_type_trans(skb, dev);
@@ -4848,9 +4954,12 @@ static int rtl_rx(struct net_device *dev, struct rtl8169_private *tp, int budget
 
 		dev_sw_netstats_rx_add(dev, pkt_size);
 release_descriptor:
-		rtl8169_mark_to_asic(desc);
+		tp->cur_rx++;
+		count++;
 	}
 
+	rtl8169_rx_refill(tp);
+
 	return count;
 }
 
-- 
2.55.0

Re: [PATCH v5] r8169: migrate Rx path to page_pool, prepare for XDP

[Simon Horman]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260713035049.25310-1-atharvapotdar07@gmail.com?part=1
---
r8169: migrate Rx path to page_pool, prepare for XDP

This commit replaces the alloc_pages() + dma_map_page() + skb_copy() Rx
model with a page_pool-backed napi_build_skb() path. It keeps MACs up to
RTL_GIGA_MAC_VER_06 on the copy path, and reserves XDP_PACKET_HEADROOM
on newer MACs. It also decouples cur_rx from dirty_rx and registers an
xdp_rxq for MEM_TYPE_PAGE_POOL.

A few questions on the new code below.

> diff --git a/drivers/net/ethernet/realtek/r8169_main.c b/drivers/net/ethernet/realtek/r8169_main.c
> index ec4fc21fa21f..c548e7e8db87 100644
> --- a/drivers/net/ethernet/realtek/r8169_main.c
> +++ b/drivers/net/ethernet/realtek/r8169_main.c

[ ... ]

> @@ -4221,12 +4228,57 @@ static int rtl8169_rx_fill(struct rtl8169_private *tp)
>  
>  static int rtl8169_init_ring(struct rtl8169_private *tp)
>  {
> +	struct page_pool_params params = {0};
> +	int err;
> +
>  	rtl8169_init_ring_indexes(tp);
>  
> +	if (tp->mac_version <= RTL_GIGA_MAC_VER_06) {
> +		tp->rx_headroom = 0;
> +		tp->rx_buf_sz = R8169_RX_BUF_SIZE;
> +	} else {
> +		tp->rx_headroom = XDP_PACKET_HEADROOM;
> +		tp->rx_buf_sz = SZ_16K -
> +						SKB_DATA_ALIGN(sizeof(struct skb_shared_info)) -
> +						XDP_PACKET_HEADROOM;
> +	}

[High]

Can this silently break jumbo Rx on RTL8125/RTL8126? For those MACs
rtl_jumbo_max() returns JUMBO_16K (16362 bytes) and dev->max_mtu is
left unchanged by this patch, but tp->rx_buf_sz here becomes:

    SZ_16K - SKB_DATA_ALIGN(sizeof(struct skb_shared_info))
           - XDP_PACKET_HEADROOM

which is roughly 15808 bytes. This value is written into every
descriptor by rtl8169_mark_to_asic():

    WRITE_ONCE(desc->opts1, cpu_to_le32(DescOwn | eor | rx_buf_sz));

while rtl_set_rx_max_size() still programs RxMaxSize based on
R8169_RX_BUF_SIZE + 1 = 16384. When a frame larger than ~15808 bytes
arrives at an MTU near 16362, the hardware will split it across
descriptors, and rtl8169_fragmented_frame() then drops it and bumps
rx_length_errors:

    if (unlikely(rtl8169_fragmented_frame(status))) {
        dev->stats.rx_dropped++;
        dev->stats.rx_length_errors++;
        goto release_descriptor;
    }

Should dev->max_mtu (or rtl_jumbo_max()) be adjusted for the newer
MACs so this reduction in the per-descriptor buffer size does not
silently drop max-size jumbo frames?

Relatedly, rtl8169_change_mtu() does not recompute rx_buf_sz or
repopulate the ring, so a change from a smaller MTU up towards
JUMBO_16K will not re-evaluate this either. Is that intentional?

Since the commit message describes this only as XDP preparation, could
the MTU-visible behaviour change be called out there as well?

[ ... ]

> +	tp->rx_pool = page_pool_create(&params);
> +	if (IS_ERR(tp->rx_pool)) {
> +		err = PTR_ERR(tp->rx_pool);
> +		tp->rx_pool = NULL;
> +		return err;
> +	}
> +
> +	err = xdp_rxq_info_reg(&tp->xdp_rxq, tp->dev, 0, tp->napi.napi_id);
> +	if (err)
> +		goto err_free_pool;

[Medium]

At this point, is tp->napi.napi_id actually assigned yet? rtl_open()
calls rtl8169_init_ring() before rtl8169_up() -> napi_enable(), and
napi_hash_add() (which populates napi_id) runs from
napi_enable_locked(). netif_napi_add() at probe time does not assign a
napi_id either.

If so, xdp_rxq_info_reg() is called with napi_id == 0 and the xdp_rxq
loses the association intended for busy-poll, netlink queue
introspection and XDP redirect wakeups.

Would deferring xdp_rxq_info_reg() until after napi_enable(), or using
a persistent napi_config-based id, keep the association the commit
message is establishing?

[ ... ]

> @@ -4311,8 +4363,20 @@ static void rtl_reset_work(struct rtl8169_private *tp)
>  
>  	rtl8169_cleanup(tp);
>  
> -	for (i = 0; i < NUM_RX_DESC; i++)
> -		rtl8169_mark_to_asic(tp->RxDescArray + i);
> +	for (i = 0; i < NUM_RX_DESC; i++) {
> +		if (tp->Rx_databuff[i])
> +			continue;
> +		tp->Rx_databuff[i] =
> +			rtl8169_alloc_rx_data(tp, tp->RxDescArray + i);
> +		if (!tp->Rx_databuff[i])
> +			break;
> +	}
> +
> +	for (i = 0; i < NUM_RX_DESC; i++) {
> +		if (!tp->Rx_databuff[i])
> +			continue;
> +		rtl8169_mark_to_asic(tp->RxDescArray + i, tp->rx_buf_sz);
> +	}
>  
>  	napi_enable(&tp->napi);
>  	rtl_hw_start(tp);

[High]

Can this leave the Rx ring with permanent gaps under memory pressure?

rtl8169_cleanup() does not clear Rx_databuff[], so on entry this loop
tries to refill slots consumed by the zero-copy path. On the first
allocation failure it breaks:

    if (!tp->Rx_databuff[i])
        break;

This skips every later NULL slot, even those where a subsequent
allocation might have succeeded. The second loop only marks entries
that already have a databuff, so the gap entries keep DescOwn=0.

rtl_reset_work() is void and rtl_hw_start() runs unconditionally, so
hardware then walks the ring sequentially and stalls at the first
gap. Since the normal refill (rtl8169_rx_refill()) only iterates
between dirty_rx and cur_rx, would slots at higher indices that were
never allocated here ever be filled again?

Would it be preferable to keep retrying across all slots (rather than
break on the first failure), or to fail the reset out to a scheduled
retry when allocation fails?

[ ... ]

> @@ -4768,16 +4832,39 @@ static inline void rtl8169_rx_csum(struct sk_buff *skb, u32 opts1)
>  		skb_checksum_none_assert(skb);
>  }
>  
> +static void rtl8169_rx_refill(struct rtl8169_private *tp)
> +{
> +	while (tp->dirty_rx != tp->cur_rx) {
> +		unsigned int entry = tp->dirty_rx % NUM_RX_DESC;
> +		struct RxDesc *desc = tp->RxDescArray + entry;
> +
> +		if (!tp->Rx_databuff[entry]) {
> +			struct page *new_page = page_pool_dev_alloc_pages(tp->rx_pool);
> +
> +			if (unlikely(!new_page))
> +				break;
> +
> +			tp->Rx_databuff[entry] = new_page;
> +
> +			desc->addr = cpu_to_le64(page_pool_get_dma_addr(new_page) +
> +				tp->rx_headroom);
> +		}
> +		rtl8169_mark_to_asic(desc, tp->rx_buf_sz);
> +
> +		tp->dirty_rx++;
> +	}
> +}
> +
>  static int rtl_rx(struct net_device *dev, struct rtl8169_private *tp, int budget)
>  {
>  	struct device *d = tp_to_dev(tp);
>  	int count;
>  
> -	for (count = 0; count < budget; count++, tp->cur_rx++) {
> +	for (count = 0; count < budget;) {
>  		unsigned int pkt_size, entry = tp->cur_rx % NUM_RX_DESC;
>  		struct RxDesc *desc = tp->RxDescArray + entry;
>  		struct sk_buff *skb;
> -		const void *rx_buf;
> +		void *rx_buf;
>  		dma_addr_t addr;
>  		u32 status;
>  
> @@ -4820,21 +4907,40 @@ static int rtl_rx(struct net_device *dev, struct rtl8169_private *tp, int budget
>  			goto release_descriptor;
>  		}
>  
> -		skb = napi_alloc_skb(&tp->napi, pkt_size);
> -		if (unlikely(!skb)) {
> -			dev->stats.rx_dropped++;
> -			goto release_descriptor;
> -		}
> +		if (unlikely(!tp->Rx_databuff[entry]))
> +			break;

[High]

Can this pair of paths leave the ring wedged with hardware waiting on a
DescOwn=0 slot that software has already given up on?

If rtl8169_rx_refill() runs after some pages were consumed by the
zero-copy path and page_pool_dev_alloc_pages() fails, the loop breaks
without calling rtl8169_mark_to_asic() and without advancing dirty_rx:

    if (unlikely(!new_page))
        break;
    ...
    rtl8169_mark_to_asic(desc, tp->rx_buf_sz);
    tp->dirty_rx++;

So the descriptor stays with DescOwn=0 and Rx_databuff[entry] == NULL.

On the next NAPI iteration, when rtl_rx() reaches that entry the new
early exit:

    if (unlikely(!tp->Rx_databuff[entry]))
        break;

leaves rtl_rx() without advancing tp->cur_rx and without re-marking
the descriptor. Hardware is also stuck at this same slot, so if there
is no other IRQ source (e.g. TX completions) to schedule NAPI again,
does anything actually recover the Rx queue?

The pre-patch code always called rtl8169_mark_to_asic(desc) on the
drop path in rtl_rx(), so this stalled state was not structurally
reachable. Should the alloc-failure path here retain ownership with
the hardware (mark_to_asic with a placeholder / retry later) rather
than leaving DescOwn=0?

[ ... ]

Re: [PATCH v5] r8169: migrate Rx path to page_pool, prepare for XDP

[Simon Horman]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
https://sashiko.dev/#/patchset/20260713035049.25310-1-atharvapotdar07@gmail.com?part=1
---
--- Patch [1]: [PATCH v5] r8169: migrate Rx path to page_pool, prepare for XDP ---
commit db5a368e94d087883d45d81ba30f59c05de881cf
Author: Atharva Potdar <atharvapotdar07@gmail.com>

r8169: migrate Rx path to page_pool, prepare for XDP

Replace the alloc_pages() + dma_map_page() + skb_copy() Rx model with
a page_pool-backed napi_build_skb() path for zero-copy delivery.

> diff --git a/drivers/net/ethernet/realtek/r8169_main.c b/drivers/net/ethernet/realtek/r8169_main.c
> index ec4fc21fa21fb..c548e7e8db874 100644
> --- a/drivers/net/ethernet/realtek/r8169_main.c
> +++ b/drivers/net/ethernet/realtek/r8169_main.c
> @@ -4311,8 +4363,20 @@ static void rtl_reset_work(struct rtl8169_private *tp)
>  
>  	rtl8169_cleanup(tp);
>  
> -	for (i = 0; i < NUM_RX_DESC; i++)
> -		rtl8169_mark_to_asic(tp->RxDescArray + i);
> +	for (i = 0; i < NUM_RX_DESC; i++) {
> +		if (tp->Rx_databuff[i])
> +			continue;
> +		tp->Rx_databuff[i] =
> +			rtl8169_alloc_rx_data(tp, tp->RxDescArray + i);
> +		if (!tp->Rx_databuff[i])
> +			break;
> +	}

[Severity: High]
If memory allocation fails under pressure during an interface reset,
this loop breaks and leaves tp->Rx_databuff[i] as NULL. When NAPI runs,
could this lead to a permanent deadlock in rtl_rx()?

If a descriptor has a NULL buffer, rtl_rx() currently does:

    if (unlikely(!tp->Rx_databuff[entry]))
        break;

This breaks out of the Rx loop without advancing tp->cur_rx. Because
cur_rx didn't advance, the subsequent call to rtl8169_rx_refill() sees
tp->dirty_rx == tp->cur_rx and will not attempt to allocate memory.
Would it be better to advance cur_rx and drop the packet, or handle
the allocation failure without stalling the entire ring?

> @@ -4820,21 +4907,40 @@ static int rtl_rx(struct net_device *dev, struct rtl8169_private *tp, int budget
>  			goto release_descriptor;
>  		}
>  
> -		skb = napi_alloc_skb(&tp->napi, pkt_size);
> -		if (unlikely(!skb)) {
> -			dev->stats.rx_dropped++;
> -			goto release_descriptor;
> -		}
> +		if (unlikely(!tp->Rx_databuff[entry]))
> +			break;
>  
>  		addr = le64_to_cpu(desc->addr);
>  		rx_buf = page_address(tp->Rx_databuff[entry]);
>  
>  		dma_sync_single_for_cpu(d, addr, pkt_size, DMA_FROM_DEVICE);
> -		prefetch(rx_buf);
> -		skb_copy_to_linear_data(skb, rx_buf, pkt_size);
> -		skb->tail += pkt_size;
> -		skb->len = pkt_size;
> -		dma_sync_single_for_device(d, addr, pkt_size, DMA_FROM_DEVICE);
> +		prefetch(rx_buf + tp->rx_headroom);
> +
> +		if (unlikely(tp->rx_headroom == 0)) {
> +			skb = napi_alloc_skb(&tp->napi, pkt_size);
> +			if (likely(skb)) {
> +				skb_copy_to_linear_data(skb, rx_buf, pkt_size);
> +				skb_put(skb, pkt_size);

[Severity: Critical]
This is a pre-existing issue, but earlier in rtl_rx() there is an
integer underflow on runt frames:

    pkt_size = status & GENMASK(13, 0);
    if (likely(!(dev->features & NETIF_F_RXFCS)))
        pkt_size -= ETH_FCS_LEN;

If NETIF_F_RXALL is enabled, the hardware can pass runt frames smaller
than 4 bytes. In this case, pkt_size underflows to a massive unsigned
value. 

Since this code passes pkt_size directly into dma_sync_single_for_device()
and skb_put(), could this massive value trigger an skb_over_panic() and
crash the kernel when processing runts?