Re: [net-next,v12,2/7] r8169: refactor RX path to prepare for multi-queue

[netdev-bot+sashiko@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential
issue(s) to consider:

Critical: 0 · High: 0 · Medium: 3 · Low: 0

- [Medium] Undisclosed functional change folded into a patch whose
  changelog promises a pure refactor: `R8169_RX_RING_BYTES` is changed…
- [Medium] State introduced by this patch that is never consumed, and a
  netdev queue count advertised to the core that never matches reality.
- [Medium] IRQF_NO_THREAD is silently dropped for MSI/MSI-X vectors.

--

r8169: refactor RX path to prepare for multi-queue

This patch introduces struct rtl8169_rx_ring and moves the RX ring state
out of rtl8169_private into a per-queue array, and switches the netdev
allocation to devm_alloc_etherdev_mqs() with up to 8 RX queues while
num_rx_rings stays at 1.

> diff --git a/drivers/net/ethernet/realtek/r8169_main.c b/drivers/net/ethernet/realtek/r8169_main.c
> index 87eb10616a0c..9311a0cab4eb 100644
> --- a/drivers/net/ethernet/realtek/r8169_main.c
> +++ b/drivers/net/ethernet/realtek/r8169_main.c
> @@ -74,9 +74,19 @@
>  #define NUM_TX_DESC	256	/* Number of Tx descriptor registers */
>  #define NUM_RX_DESC	256	/* Number of Rx descriptor registers */
>  #define R8169_TX_RING_BYTES	(NUM_TX_DESC * sizeof(struct TxDesc))
> -#define R8169_RX_RING_BYTES	(NUM_RX_DESC * sizeof(struct RxDesc))
> +
> +/*
> + * Workaround for the hardware DMA prefetcher. The H/W might aggressively
> + * fetch one more descriptor even after hitting the RingEnd mark. We
> + * allocate this extra dummy space as padding to prevent out-of-bounds
> + * access and potential IOMMU faults.
> + */
> +#define R8169_RX_RING_BYTES	((NUM_RX_DESC + 1) * sizeof(struct RxDesc))

[Severity: Medium]
The changelog describes this patch as "a preparatory refactoring of the RX
path", but this macro changes behaviour for every supported chip.  Two
questions about it.

First, if the hardware really does "fetch one more descriptor even after
hitting the RingEnd mark", then the driver before this patch lets the device
DMA past the end of the ring on all chip generations.  Would that make this
an independent fix that wants its own patch with a Fixes: tag, so that stable
trees pick it up?  As submitted the change is invisible to backporters.

Second, sizeof(struct RxDesc) is 16 and NUM_RX_DESC is 256, so the old
request was exactly 4096 bytes and the new one is 4112 bytes:

	rtl8169_alloc_rx_desc()
	  dma_alloc_coherent(&pdev->dev, R8169_RX_RING_BYTES, ...)

Since dma_alloc_coherent() rounds up to a power-of-two page order, does this
turn the per-ring coherent allocation from one page into two, doubling the
coherent DMA footprint per RX ring (and eight times that once the rest of the
series enables multiple rings)?  Would allocating one page and reserving the
last descriptor as the pad, or documenting the extra page, be preferable?

>  #define R8169_TX_STOP_THRS	(MAX_SKB_FRAGS + 1)
>  #define R8169_TX_START_THRS	(2 * R8169_TX_STOP_THRS)
> +#define R8169_MAX_RX_QUEUES	8
> +#define R8169_DEFAULT_RX_QUEUES	1
> +#define R8169_MAX_TX_QUEUES	1
>  
>  #define OCP_STD_PHY_BASE	0xa400
>  

[ ... ]

> @@ -728,6 +739,15 @@ enum rtl_dash_type {
>  	RTL_DASH_25_BP,
>  };
>  
> +struct rtl8169_rx_ring {
> +	u32 cur_rx;
> +	u32 dirty_rx;

[Severity: Medium]
Is dirty_rx used anywhere?  Grepping the tree at the end of this series
returns only this declaration and the single store in
rtl8169_init_ring_indexes() below - there is no comparison against cur_rx and
no refill or doorbell logic reading it.  Should the field be dropped until the
patch that actually needs it?

> +	struct RxDesc *rx_desc_array;
> +	dma_addr_t rx_desc_phy_addr[NUM_RX_DESC];
> +	dma_addr_t rx_phy_addr;
> +	struct page *rx_databuff[NUM_RX_DESC];
> +};
> +

[ ... ]

> @@ -2620,9 +2638,26 @@ static void rtl_init_rxcfg(struct rtl8169_private *tp)
>  	}
>  }
>  
> +static void rtl8169_rx_desc_init(struct rtl8169_private *tp)
> +{
> +	for (int i = 0; i < tp->num_rx_rings; i++) {
> +		struct rtl8169_rx_ring *ring = &tp->rx_ring[i];
> +
> +		memset(ring->rx_desc_array, 0x0, R8169_RX_RING_BYTES);
> +	}
> +}
> +
>  static void rtl8169_init_ring_indexes(struct rtl8169_private *tp)
>  {
> -	tp->dirty_tx = tp->cur_tx = tp->cur_rx = 0;
> +	tp->dirty_tx = 0;
> +	tp->cur_tx = 0;
> +
> +	for (int i = 0; i < tp->num_rx_rings; i++) {
> +		struct rtl8169_rx_ring *ring = &tp->rx_ring[i];
> +
> +		ring->dirty_rx = 0;
> +		ring->cur_rx = 0;
> +	}
>  }
>  

[ ... ]

> @@ -4184,55 +4240,107 @@ static void rtl8169_rx_clear(struct rtl8169_private *tp)

[ ... ]

> +static int rtl8169_alloc_rx_desc(struct rtl8169_private *tp)
> +{
> +	struct pci_dev *pdev = tp->pci_dev;
> +	struct rtl8169_rx_ring *ring;
> +
> +	for (int i = 0; i < tp->num_rx_rings; i++) {
> +		ring = &tp->rx_ring[i];
> +		ring->rx_desc_array = dma_alloc_coherent(&pdev->dev,
> +							 R8169_RX_RING_BYTES,
> +							 &ring->rx_phy_addr,
> +							 GFP_KERNEL);
> +		if (!ring->rx_desc_array)
> +			return -ENOMEM;
> +	}
> +	return 0;
> +}
> +

[ ... ]

> @@ -5688,7 +5812,10 @@ static int rtl_init_one(struct pci_dev *pdev, const struct pci_device_id *ent)
>  	u32 txconfig;
>  	u32 xid;
>  
> -	dev = devm_alloc_etherdev(&pdev->dev, sizeof (*tp));
> +	dev = devm_alloc_etherdev_mqs(&pdev->dev, sizeof(*tp),
> +				      R8169_MAX_TX_QUEUES,
> +				      R8169_MAX_RX_QUEUES);
> +

[Severity: Medium]
This asks the core for 8 RX queues on every chip, and alloc_netdev_mqs() sets
dev->real_num_rx_queues = rxqs:

net/core/dev.c:alloc_netdev_mqs() {
	...
	dev->real_num_rx_queues = rxqs;
	...
}

rtl_setup_rx_params() keeps tp->num_rx_rings at 1, and there is no
netif_set_real_num_rx_queues() call in the driver, either here or at the end
of the series.  Does that leave every r8169 device advertising 8 RX queues to
userspace - /sys/class/net/<dev>/queues/rx-0 through rx-7 with their RPS/RFS
knobs, plus the netdev-genl queue enumeration - while only ring 0 ever
receives packets?

Should rtl_init_one() call netif_set_real_num_rx_queues(dev,
tp->num_rx_rings) once the ring count is known?

>  	if (!dev)
>  		return -ENOMEM;
>  

[ ... ]

One more observation about the series rather than this hunk.  Before the
series, rtl_open() picked the IRQ flags explicitly:

	irqflags = pci_dev_msi_enabled(pdev) ? IRQF_NO_THREAD : IRQF_SHARED;
	retval = request_irq(tp->irq, rtl8169_interrupt, irqflags, dev->name, tp);

[Severity: Medium]
rtl8169_request_irq() now uses pci_request_irq() for all vectors, and that
helper hard-codes the flags:

drivers/pci/irq.c:pci_request_irq() {
	unsigned long irqflags = IRQF_SHARED;

	if (!handler)
		irqflags |= IRQF_ONESHOT;
	...
}

There is no way for the caller to pass IRQF_NO_THREAD, so does this lose the
behaviour that commit 870f531e178e ("r8169: set IRQF_NO_THREAD if MSI(X) is
enabled") added, namely keeping the handler unthreaded when the interrupt
cannot be shared?

On kernels booted with threadirqs, or on PREEMPT_RT, the small handler that
does rtl_irq_disable(), napi_schedule() and rtl_ack_events() would then run
from an IRQ thread even for MSI/MSI-X vectors.  Was that intended, and could
it be mentioned in the changelog if so?

Keeping request_irq() with IRQF_NO_THREAD for MSI/MSI-X and IRQF_SHARED for
INTx, using pci_irq_vector(pdev, i) for the vector lookup, would preserve the
previous behaviour.

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260831053940.620-1-javen_xu%40realsil.com.cn