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

[Jakub Kicinski <kuba@kernel.org>]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
---
r8169: add support and enable rss

This patch adds support and enables Receive Side Scaling (RSS) for RTL8127.
It configures RSS keys, redirection tables, and introduces support for
parsing RSS-specific RX descriptors.

> diff --git a/drivers/net/ethernet/realtek/r8169_main.c b/drivers/net/ethernet/realtek/r8169_main.c
> --- a/drivers/net/ethernet/realtek/r8169_main.c
> +++ b/drivers/net/ethernet/realtek/r8169_main.c
[ ... ]
> @@ -4920,27 +5123,87 @@ static inline int rtl8169_fragmented_frame(u32 status)
>  	return (status & (FirstFrag | LastFrag)) != (FirstFrag | LastFrag);
>  }
>  
> -static inline void rtl8169_rx_csum(struct sk_buff *skb,
> +static inline void rtl8169_rx_hash(struct rtl8169_private *tp,
> +				   struct RxDesc *desc,
> +				   struct sk_buff *skb)
[ ... ]
> +static inline void rtl8169_rx_csum(struct rtl8169_private *tp,
> +				   struct sk_buff *skb,
>  				   u32 opts1)
>  {
> -	u32 status = opts1 & (RxProtoMask | RxCSFailMask);
> +	bool csum_ok = false;
> +
> +	switch (tp->init_rx_desc_type) {
> +	case RX_DESC_TYPE_RSS:
> +		if (((opts1 & RX_TCPT_DESC_RSS) && !(opts1 & RX_TCPF_DESC_RSS)) ||
> +		    ((opts1 & RX_UDPT_DESC_RSS) && !(opts1 & RX_UDPF_DESC_RSS)))
> +			csum_ok = true;
> +		break;
> +	default:
> +		u32 status = opts1 & (RxProtoMask | RxCSFailMask);

[Severity: Low]
Will this compile under standard C? Declaring a variable immediately
following a label (default:) is a syntax error in C unless it is wrapped in
a block.

>  
> -	if (status == RxProtoTCP || status == RxProtoUDP)
> +		if (status == RxProtoTCP || status == RxProtoUDP)
> +			csum_ok = true;
> +		break;
> +	}
> +
> +	if (csum_ok)
>  		skb->ip_summed = CHECKSUM_UNNECESSARY;
>  	else
>  		skb_checksum_none_assert(skb);
>  }
[ ... ]
>  static bool rtl8169_check_rx_desc_error(struct net_device *dev,
>  					struct rtl8169_private *tp,
>  					u32 status)
>  {
> -	if (unlikely(status & RxRES)) {
> -		if (status & (RxRWT | RxRUNT))
> -			dev->stats.rx_length_errors++;
> -		if (status & RxCRC)
> -			dev->stats.rx_crc_errors++;
> -		return true;
> +	switch (tp->init_rx_desc_type) {
> +	case RX_DESC_TYPE_RSS:
> +		if (unlikely(status & RX_RES_RSS)) {
> +			if (status & RX_RUNT_RSS)
> +				dev->stats.rx_length_errors++;
> +			if (status & RX_CRC_RSS)
> +				dev->stats.rx_crc_errors++;

[Severity: Medium]
Can these direct increments to dev->stats cause lost updates?

Now that RSS is enabled, multiple RX queues can trigger NAPI polling
concurrently on different CPUs. If multiple queues receive errored packets
simultaneously, they will race to read-modify-write these global stat
counters.

> +			return true;
> +		}
> +		break;
> +	default:
> +		if (unlikely(status & RxRES)) {
> +			if (status & (RxRWT | RxRUNT))
> +				dev->stats.rx_length_errors++;
> +			if (status & RxCRC)
> +				dev->stats.rx_crc_errors++;
> +			return true;
> +		}
> +		break;
>  	}
>  	return false;
>  }
[ ... ]
> @@ -4977,7 +5240,8 @@ 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 ((status & RxRWT || !(status & (RxRUNT | RxCRC))) &&
> +			    tp->init_rx_desc_type == RX_DESC_TYPE_DEFAULT)
> +				goto release_descriptor;
>  		}

[Severity: High]
Does this bypass the drop logic for fatal errors when RSS and RXALL are
both enabled?

If the hardware receives a fatally errored packet while RSS is enabled,
the descriptor type check (tp->init_rx_desc_type == RX_DESC_TYPE_DEFAULT)
prevents the code from taking the goto release_descriptor path. This could
cause the driver to process frames with corrupted length fields or 
uninitialized memory.

>  
> @@ -5010,10 +5274,12 @@ static int rtl_rx(struct net_device *dev, struct rtl8169_private *tp,