Re: [PATCH net v2] ibmveth: Disable GSO for packets with small MSS

[mingming cao <mmc@linux.ibm.com>]

On 4/18/26 10:54 AM, Jakub Kicinski wrote:
> This is an AI-generated review of your patch. The human sending this
> email has considered the AI review valid, or at least plausible.

Thanks for the review and the questions!

> ---
> ibmveth: Disable GSO for packets with small MSS
>
> This commit adds an ndo_features_check callback to the ibmveth driver to
> disable segmentation offload when the MSS is less than 224 bytes. This
> prevents physical adapters in SEA configurations from freezing when given
> small MSS packets.
>
>> diff --git a/drivers/net/ethernet/ibm/ibmveth.c b/drivers/net/ethernet/ibm/ibmveth.c
>> --- a/drivers/net/ethernet/ibm/ibmveth.c
>> +++ b/drivers/net/ethernet/ibm/ibmveth.c
> [ ... ]
>
>> @@ -1756,6 +1756,25 @@ static int ibmveth_set_mac_addr(struct net_device *dev, void *p)
>>   	return 0;
>>   }
>>   
>> +static netdev_features_t ibmveth_features_check(struct sk_buff *skb,
>> +						struct net_device *dev,
>> +						netdev_features_t features)
>> +{
>> +	/* Some physical adapters do not support segmentation offload with
>> +	 * MSS < 224. Disable GSO for such packets to avoid adapter freeze.
>> +	 */
>> +	if (skb_is_gso(skb)) {
> The commit message mentions this matches the ibmvnic hardware errata from
> commit f10b09ef687f. That commit disabled GSO for both gso_size < 224 and
> gso_segs == 1.
>
> By omitting the check for skb_shinfo(skb)->gso_segs == 1, could a
> single-segment GSO packet still cause the adapter to freeze?
Good question. Unlike ibmvnic, ibmveth does not need to check for 
single-segment GSO packets (gso_segs == 1).

In ibmvnic, the firmware processes all GSO packets, including 
single-segment ones, through the same LSO code path. This means even 
gso_segs == 1 packets can trigger the hardware errata.

In ibmveth, the PowerVM hypervisor intelligently bypasses the LSO path 
for single-segment packets. When gso_segs == 1, the hypervisor treats 
the packet as a regular (non-GSO) packet and transmits it directly 
without invoking the problematic hardware segmentation logic. Therefore, 
single-segment GSO packets never reach the code path that causes the freeze.

This architectural difference is why ibmvnic needs the gso_segs check 
but ibmveth does not.
>
>> +		if (skb_shinfo(skb)->gso_size < IBMVETH_MIN_LSO_MSS) {
>> +			netdev_warn_once(dev,
>> +					 "MSS %u too small for LSO, disabling GSO\n",
>> +					 skb_shinfo(skb)->gso_size);
>> +			features &= ~NETIF_F_GSO_MASK;
>> +		}
>> +	}
>> +
>> +	return features;
> When implementing a custom ndo_features_check callback, netif_skb_features()
> will bypass the standard dflt_features_check() function. Does this mean
> vlan_features_check() is no longer called for this driver?
>
> If vlan_features_check() is skipped, multi-tagged (QinQ) VLAN packets
> might retain GSO and protocol-specific checksum offloads that the hardware
> parser cannot handle.
>
> Should this return vlan_features_check(skb, features) instead?

You’re correct about the interaction with the default feature filtering. 
With a custom ndo_features_check(), the standard dflt_features_check() 
path is bypassed, so vlan_features_check() must be called explicitly to 
preserve existing VLAN/QinQ and checksum handling.

Thank you for catching this! I'll send v3 shortly with:
1. The vlan_features_check() call added
2. Updated commit message explaining why gso_segs check is not needed 
for ibmveth

Best regards,

Mingming


>> +}