Re: [PATCH net-next v14 5/7] r8169: add support and enable rss

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 0 · Medium: 5 · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Medium] The patch silently bundles a fix for a pre-existing integer
  underflow in the chip-independent RX path into a feature commit.
- [Medium] The one-line commit message does not disclose the new IRQ
  allocation policy for RTL8127, which is all-or-nothing and consumes…
- [Medium] For the new RSS (v4) RX descriptor the patch re-declares the
  MAC error status bits shifted one position up relative to the legacy…
- [Medium] Packets received on the newly enabled RSS RX rings are handed
  to GRO without recording the receiving queue, so per-queue RPS/RFS…
- [Medium] The new minimum-length guard in rtl_rx() is too weak: it only
  rejects `pkt_size < ETH_FCS_LEN`, so a descriptor reporting exactly 4…

--

r8169: add support and enable rss

This patch adds RSS support for RTL8127 (RTL_GIGA_MAC_VER_80): a second RX
descriptor layout, an RSS key and indirection table, per-ring RX error
accounting and a new MSI-X vector allocation policy.  RSS is turned on by
default when enough MSI-X vectors are granted.  A few questions below.

> diff --git a/drivers/net/ethernet/realtek/r8169_main.c b/drivers/net/ethernet/realtek/r8169_main.c
> index ec3643892fe2..ad360f145076 100644
> --- a/drivers/net/ethernet/realtek/r8169_main.c
> +++ b/drivers/net/ethernet/realtek/r8169_main.c
> @@ -82,6 +82,19 @@
>  #define R8169_MAX_RX_QUEUES	8
>  #define R8169_DEFAULT_RX_QUEUES	1
>  #define R8169_MAX_TX_QUEUES	1
> +#define R8127_MAX_NUM_IRQVEC	32
> +#define R8127_MIN_NUM_IRQVEC	30
> +#define R8169_IRQ_DEFAULT	1
> +#define RTL_RSS_KEY_SIZE	40
> +#define RSS_CPU_NUM_MASK	GENMASK(18, 16)
> +#define RSS_HASH_MASK		GENMASK(10, 8)
> +#define RTL_MAX_INDIRECTION_TABLE_ENTRIES 128
> +#define RXS_RSS_UDP		BIT(27)
> +#define RXS_RSS_IPV4		BIT(28)
> +#define RXS_RSS_IPV6		BIT(29)
> +#define RXS_RSS_TCP		BIT(30)
> +#define RXS_RSS_L3_TYPE_MASK	(RXS_RSS_IPV4 | RXS_RSS_IPV6)
> +#define RXS_RSS_L4_TYPE_MASK	(RXS_RSS_TCP | RXS_RSS_UDP)
>  
>  #define OCP_STD_PHY_BASE	0xa400
>  
> @@ -506,6 +519,9 @@ enum rtl_register_content {
>  	RxRUNT	= (1 << 20),
>  	RxCRC	= (1 << 19),
>  
> +	RXRUNT_RSS	= (1 << 21),
> +	RXCRC_RSS	= (1 << 20),
> +
>  	/* ChipCmdBits */
>  	StopReq		= 0x80,
>  	CmdReset	= 0x10,

[ ... ]

> @@ -611,6 +627,20 @@ enum rtl_register_content {
>  #define	ISRIMR_LINKCHG	BIT(29)
>  #define	ISRIMR_TOK_Q0	BIT(8)
>  #define	ISRIMR_ROK_Q0	BIT(0)
> +#define RTL_DESC_TYPE_CTRL		0xd8
> +#define RSS_KEY_REG			0x4600
> +#define RSS_INDIRECTION_TBL_REG		0x4700
> +#define RSS_CTRL_TCP_IPV4_SUPP		BIT(0)
> +#define RTL_DESC_TYPE_RSS		BIT(1)
> +#define RSS_CTRL_IPV4_SUPP		BIT(1)
> +#define RSS_CTRL_TCP_IPV6_SUPP		BIT(2)
> +#define RSS_CTRL_IPV6_SUPP		BIT(3)
> +#define RSS_CTRL_IPV6_EXT_SUPP		BIT(4)
> +#define RSS_CTRL_TCP_IPV6_EXT_SUPP	BIT(5)
> +#define RSS_CTRL_UDP_IPV4_SUPP		BIT(11)
> +#define RSS_CTRL_UDP_IPV6_SUPP		BIT(12)
> +#define	RX_RES_RSS			BIT(22)
> +#define RTL_RX_Q_NUM_MASK		GENMASK(4, 2)
>  };

[ ... ]

> @@ -5093,28 +5299,91 @@ static inline int rtl8169_fragmented_frame(u32 status)

[ ... ]

>  static bool rtl8169_check_rx_desc_error(struct rtl8169_rx_ring *ring,
> +					struct rtl8169_private *tp,
>  					u32 status)
>  {
> -	if (unlikely(status & RxRES)) {
> -		u64_stats_update_begin(&ring->stats.syncp);
> -		if (status & (RxRWT | RxRUNT))
> -			ring->stats.rx_length_errors++;
> -		if (status & RxCRC)
> -			ring->stats.rx_crc_errors++;
> -		u64_stats_update_end(&ring->stats.syncp);
> -		return true;
> +	switch (tp->init_rx_desc_type) {
> +	case RX_DESC_TYPE_RSS:
> +		if (unlikely(status & RX_RES_RSS)) {
> +			u64_stats_update_begin(&ring->stats.syncp);
> +			if (status & RXRUNT_RSS)
> +				ring->stats.rx_length_errors++;
> +			if (status & RXCRC_RSS)
> +				ring->stats.rx_crc_errors++;
> +			u64_stats_update_end(&ring->stats.syncp);
> +			return true;
> +		}
> +		break;

[Severity: Medium]

The three v4 error bits are the legacy bits shifted up by one:

	RX_RES_RSS   = BIT(22)  vs  RxRES   = BIT(21)
	RXRUNT_RSS   = BIT(21)  vs  RxRUNT  = BIT(20)
	RXCRC_RSS    = BIT(20)  vs  RxCRC   = BIT(19)

Is there also a shifted counterpart of RxRWT (receive watchdog /
over-length, legacy BIT(22), so BIT(23) here)?  It is neither defined nor
tested anywhere in the patch, which leads to two differences on the RSS
path.

First, the RSS branch above never bumps ring->stats.rx_length_errors for a
watchdog/over-length error, while the default branch counts
status & (RxRWT | RxRUNT) as a length error, so rx_length_errors is fed by
two different bit maps depending on descriptor type.

Second, the rx-all bypass in rtl_rx() below drops an RWT frame
unconditionally on the legacy layout but not on the RSS layout:

	if (tp->init_rx_desc_type == RX_DESC_TYPE_DEFAULT) {
		if (status & RxRWT || !(status & (RxRUNT | RxCRC)))
			goto release_descriptor;
	} else {
		if (!(status & (RXRUNT_RSS | RXCRC_RSS)))
			goto release_descriptor;
	}

So a descriptor carrying the over-length bit together with a CRC or RUNT
error is delivered to the stack on RTL8127 but discarded on every other
chip.  Is that difference intended, or is the RWT equivalent simply missing
from the new bit definitions?

> @@ -5154,11 +5423,21 @@ static int rtl_rx(struct net_device *dev, struct rtl8169_private *tp,
>  
>  			if (!(dev->features & NETIF_F_RXALL))
>  				goto release_descriptor;
> -			else if (status & RxRWT || !(status & (RxRUNT | RxCRC)))
> -				goto release_descriptor;
> +
> +			if (tp->init_rx_desc_type == RX_DESC_TYPE_DEFAULT) {
> +				if (status & RxRWT || !(status & (RxRUNT | RxCRC)))
> +					goto release_descriptor;
> +			} else {
> +				if (!(status & (RXRUNT_RSS | RXCRC_RSS)))
> +					goto release_descriptor;
> +			}
>  		}
>  
>  		pkt_size = status & GENMASK(13, 0);
> +
> +		if (unlikely(pkt_size < ETH_FCS_LEN))
> +			goto release_descriptor;
> +
>  		if (likely(!(dev->features & NETIF_F_RXFCS)))
>  			pkt_size -= ETH_FCS_LEN;

[Severity: Medium]

This guard is placed after the shared pkt_size = status & GENMASK(13, 0)
assignment, so it runs for RX_DESC_TYPE_DEFAULT as well.  At the baseline
the code was:

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

with no lower bound, so the pkt_size underflow was reachable on every
r8169/r8168/r8125 chip with rx-all enabled, not only on the new RTL8127 RSS
path.

The recorded commit message is only "This patch adds support and enable rss
for RTL8127." — the "fix an integer underflow in rtl_rx()" note lives below
the scissors line and is stripped from git history, and there is no Fixes:
tag or Cc: stable.

Would it make sense to split this hunk into its own patch with a Fixes: tag
so it can be found and backported on its own?

[Severity: Medium]

Is ETH_FCS_LEN a large enough lower bound here?  A descriptor reporting
exactly 4 gives a zero-length skb, and any reported length in 4..17 (or
4..13 with rx-fcs on) gives an skb shorter than ETH_HLEN, which is then
handed to eth_type_trans():

	skb_copy_to_linear_data(skb, rx_buf, pkt_size);
	skb->tail += pkt_size;
	skb->len = pkt_size;
	...
	skb->protocol = eth_type_trans(skb, dev);

eth_type_trans() uses eth_skb_pull_mac(), which ignores the NULL return of
skb_pull_inline() when ETH_HLEN > skb->len:

include/linux/etherdevice.h:
	static inline struct ethhdr *eth_skb_pull_mac(struct sk_buff *skb)
	{
		struct ethhdr *eth = (struct ethhdr *)skb->data;

		skb_pull_inline(skb, ETH_HLEN);
		return eth;
	}

so eth_skb_pkt_type() reads eth->h_dest and eth_type_trans() reads
eth->h_proto at offsets 0..13, while only pkt_size bytes were copied into a
head that napi_alloc_skb() does not zero.  Can skb->protocol, skb->pkt_type
and the ring->stats.multicast accounting then be derived from stale heap
bytes?

For an AF_PACKET listener those bytes also become visible, since
eth_header_parse() copies six source-MAC bytes with no length check:

net/ethernet/eth.c:
	const struct ethhdr *eth = eth_hdr(skb);

	memcpy(haddr, eth->h_source, ETH_ALEN);

and packet_recvmsg() returns them in sockaddr_ll.sll_addr.

Would testing for a complete Ethernet header be more appropriate here, for
example ETH_HLEN + ETH_FCS_LEN before the FCS adjustment?  Also, when this
goto fires no rx_dropped or rx_length_errors counter is updated, so the
frames disappear without accounting — is that intentional?

> @@ -5191,10 +5470,12 @@ static int rtl_rx(struct net_device *dev, struct rtl8169_private *tp,
>  		skb->len = pkt_size;
>  		dma_sync_single_for_device(d, addr, pkt_size, DMA_FROM_DEVICE);
>  
> -		rtl8169_rx_csum(skb, status);
> +		if (tp->num_rx_rings > 1)
> +			rtl8169_rx_hash(tp, desc, skb);
> +		rtl8169_rx_csum(tp, skb, status);
>  		skb->protocol = eth_type_trans(skb, dev);

[Severity: Medium]

Should rtl_rx() also call skb_record_rx_queue() here?  The receiving ring is
known to the caller, rtl8169_poll_msix_rx() picks it from the MSI-X message
id and passes it to rtl_rx(), but no driver code records it before
napi_gro_receive().  git grep skb_record_rx_queue finds no hit in
r8169_main.c at the baseline, at this commit, or at the end of the series.

rtl_init_one() calls netif_set_real_num_queues(tp->dev, 1,
tp->num_rx_rings), so sysfs exposes rps_cpus and rps_flow_cnt for every
ring, but get_rps_cpu() only advances past rx-0 when the queue is recorded:

net/core/dev.c:get_rps_cpu() {
	struct netdev_rx_queue *rxqueue = dev->_rx;
	...
	if (skb_rx_queue_recorded(skb)) {
		u16 index = skb_get_rx_queue(skb);
	...
}

Does this mean per-queue rps_cpus settings on rx-1..rx-7 have no effect, and
the rx-0 setting is applied to traffic from all rings?  Setting the RSS hash
does not set skb->queue_mapping, so it doesn't substitute for the missing
call.

> @@ -5823,6 +6106,32 @@ static void rtl_set_irq_mask(struct rtl8169_private *tp)
>  	}
>  }
>  
> +static int get_max_irq_nvecs(struct rtl8169_private *tp)
> +{
> +	if (tp->mac_version == RTL_GIGA_MAC_VER_80)
> +		return R8127_MAX_NUM_IRQVEC;
> +	return R8169_IRQ_DEFAULT;
> +}
> +
> +static int get_min_irq_nvecs(struct rtl8169_private *tp)
> +{
> +	if (tp->mac_version == RTL_GIGA_MAC_VER_80)
> +		return R8127_MIN_NUM_IRQVEC;
> +	return R8169_IRQ_DEFAULT;
> +}
> +
> +static void rtl8169_set_rx_ring_num(struct rtl8169_private *tp)
> +{
> +	if (tp->irq_nvecs >= get_min_irq_nvecs(tp)) {
> +		unsigned int rss_queue_num = netif_get_num_default_rss_queues();
> +
> +		tp->num_rx_rings = rounddown_pow_of_two(min(rss_queue_num,
> +							    tp->hw_supp_num_rx_queues));
> +		if (tp->num_rx_rings >= 2)
> +			tp->init_rx_desc_type = RX_DESC_TYPE_RSS;
> +	}
> +}
> +
>  static int rtl_alloc_irq(struct rtl8169_private *tp)
>  {
>  	struct pci_dev *pdev = tp->pci_dev;
> @@ -5843,7 +6152,11 @@ static int rtl_alloc_irq(struct rtl8169_private *tp)
>  		break;
>  	}
>  
> -	nvecs = pci_alloc_irq_vectors(pdev, 1, 1, flags);
> +	nvecs = pci_alloc_irq_vectors(pdev, get_min_irq_nvecs(tp),
> +				      get_max_irq_nvecs(tp), flags);
> +
> +	if (nvecs < 0)
> +		nvecs = pci_alloc_irq_vectors(pdev, 1, 1, flags);
>  
>  	if (nvecs < 0)
>  		return nvecs;

[Severity: Medium]

This makes the RTL8127 allocation all-or-nothing: 30 to 32 vectors, or a
single vector.  Are the extra reserved vectors used anywhere?
rtl8169_request_irq() only installs handlers for vectors up to
R8169_MAX_RX_QUEUES and for MSIX_ID_VEC_MAP_LINKCHG:

	for (i = 0; i < tp->irq_nvecs; i++) {
		...
			if (i <= R8169_MAX_RX_QUEUES)
				rc = request_irq(irq, rtl8169_interrupt_msix, ...);
			else if (i == MSIX_ID_VEC_MAP_LINKCHG)
				rc = request_irq(irq, rtl8169_interrupt_other, ...);
			else
				continue;

so 21 to 23 of the reserved vectors end up with no handler, and the
kcalloc() of napi structures is sized by tp->irq_nvecs as well.

Both downgrades are also silent.  If the platform cannot supply 30 vectors
the fallback to (1, 1) leaves num_rx_rings at 1 and init_rx_desc_type at
RX_DESC_TYPE_DEFAULT with no message.  On a 2 or 3 CPU machine
rounddown_pow_of_two(min(netif_get_num_default_rss_queues(),
hw_supp_num_rx_queues)) can be 1, so RSS stays off while 30 to 32 vectors
remain reserved.

Would a dev_info() on those paths help, and should the requested count match
the number of vectors the driver actually hooks up?

The one-line commit message does not describe this new allocation policy at
all — could it be mentioned there?

[ ... ]

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