Re: [PATCH net v2] net: stmmac: keep gso_enabled_types in sync with netdev features

[Lorenzo Bianconi <lorenzo.bianconi@oss.qualcomm.com>]

[-- Attachment #1: Type: text/plain, Size: 5952 bytes --]

> On Wed, 12 Aug 2026 06:38:13 +0200 Lorenzo Bianconi wrote:
> > stmmac_set_gso_types() derives priv->gso_enabled_types all-or-nothing
> > from NETIF_F_TSO, but stmmac_set_gso_features() advertises NETIF_F_TSO,
> > NETIF_F_TSO6 and (on GMAC4) NETIF_F_GSO_UDP_L4 as independently
> > toggleable features. Since netdev_fix_features() only ties TSO6 to the
> > checksum features, disabling TSO (ethtool -K ethX tx-tcp-segmentation
> > off) leaves TSO6 enabled in dev->features while gso_enabled_types
> > becomes 0.
> > 
> > For a TCPv6 GSO skb the stack then keeps the frame unsegmented
> > (NETIF_F_TSO6 is still set), stmmac_features_check() does not clear the
> > GSO mask, and the stmmac_xmit() gate (gso_type & gso_enabled_types) is
> > false, so the multi-MSS skb is transmitted through the ordinary
> > descriptor path as a single oversized frame.
> > 
> > Derive each GSO type from its own feature bit instead, so the mask stays
> > in sync with dev->features and TCPv6 (or UDP L4 on GMAC4) segmentation
> > keeps working when only TSO is disabled.
> > 
> > Fixes: 9edfa7dab811 ("net: stmmac: enable TSO for IPv6")
> > Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@oss.qualcomm.com>
> > ---
> > Changes in v2:
> > - Fix possible race between stmmac_set_gso_types() and stmmac_xmit().
> > - Fix fixes tag.
> > - Link to v1: https://lore.kernel.org/r/20260808-stmmac-fix-tso6-features-v1-1-f82b17595052@oss.qualcomm.com
> > ---
> >  drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 28 +++++++++++++----------
> >  1 file changed, 16 insertions(+), 12 deletions(-)
> > 
> > diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > index a71f0df26378..df3549f55741 100644
> > --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> > @@ -4371,16 +4371,20 @@ static void stmmac_flush_tx_descriptors(struct stmmac_priv *priv, int queue)
> >  	stmmac_set_queue_tx_tail_ptr(priv, tx_q, queue, tx_q->cur_tx);
> >  }
> >  
> > -static void stmmac_set_gso_types(struct stmmac_priv *priv, bool tso)
> > +static void stmmac_set_gso_types(struct stmmac_priv *priv,
> > +				 netdev_features_t features)
> >  {
> > -	if (!tso) {
> > -		priv->gso_enabled_types = 0;
> > -	} else {
> > -		/* Manage oversized TCP frames for GMAC4 device */
> > -		priv->gso_enabled_types = SKB_GSO_TCPV4 | SKB_GSO_TCPV6;
> > -		if (priv->plat->core_type == DWMAC_CORE_GMAC4)
> > -			priv->gso_enabled_types |= SKB_GSO_UDP_L4;
> > -	}
> > +	unsigned int gso_types = 0;
> > +
> > +	if (features & NETIF_F_TSO)
> > +		gso_types |= SKB_GSO_TCPV4;
> > +	if (features & NETIF_F_TSO6)
> > +		gso_types |= SKB_GSO_TCPV6;
> > +	/* Manage oversized UDP frames for GMAC4 devices */
> > +	if (features & NETIF_F_GSO_UDP_L4)
> > +		gso_types |= SKB_GSO_UDP_L4;
> > +
> > +	WRITE_ONCE(priv->gso_enabled_types, gso_types);
> >  }
> >  
> >  static void stmmac_set_gso_features(struct net_device *ndev)
> > @@ -4416,7 +4420,7 @@ static void stmmac_set_gso_features(struct net_device *ndev)
> >  	if (priv->plat->core_type == DWMAC_CORE_GMAC4)
> >  		ndev->hw_features |= NETIF_F_GSO_UDP_L4;
> >  
> > -	stmmac_set_gso_types(priv, true);
> > +	stmmac_set_gso_types(priv, ndev->hw_features);
> >  
> >  	dev_info(priv->device, "TSO feature enabled\n");
> >  }
> > @@ -4766,7 +4770,7 @@ static netdev_tx_t stmmac_xmit(struct sk_buff *skb, struct net_device *dev)
> >  		stmmac_stop_sw_lpi(priv);
> >  
> >  	if (skb_is_gso(skb) &&
> > -	    skb_shinfo(skb)->gso_type & priv->gso_enabled_types)
> > +	    (skb_shinfo(skb)->gso_type & READ_ONCE(priv->gso_enabled_types)))
> 
> Maybe I'm missing something but I don't see any device configuration
> based on gso_enabled_types. Why does this mask exist in the first place?

gso_enabled_types field has been intrdocued in the following commit:

commit 2e4082e4b739191d71e03fe6c55cec68d36f67fe
Author: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
Date:   Wed Apr 1 08:21:55 2026 +0100

    net: stmmac: simplify GSO/TSO test in stmmac_xmit()
    
    The test in stmmac_xmit() to see whether we should pass the skbuff to
    stmmac_tso_xmit() is more complex than it needs to be. This test can
    be simplified by storing the mask of GSO types that we will pass, and
    setting it according to the enabled features.
    
    Note that "tso" is a mis-nomer since commit b776620651a1 ("net:
    stmmac: Implement UDP Segmentation Offload"). Also note that this
    commit controls both via the TSO feature. We preserve this behaviour
    in this commit.
    
    Also, this commit unconditionally accessed skb_shinfo(skb)->gso_type
    for all frames, even when skb_is_gso() was false. This access is
    eliminated.

> 
> Right now AFAICT packets which miss the mask check get chucked on the
> wire as one giant frame, without segmentation.
> 
> Can we just delete gso_enabled_types completely? Stack should obey the
> features for future frames, and avoiding races here may be hard.

I agree we can just remove gso_enabled_types field since if the user disables
TSO or UDP GSO, the driver will receive segmented packets for particular
traffic (the segmentation logic is managed by the stack). I think it is enough
to gate packets for stmmac_tso_xmit() using skb_is_gso() check.
I will fix it in v3.

Regards,
Lorenzo

> 
> >  		return stmmac_tso_xmit(skb, dev);
> >  
> >  	if (priv->est && priv->est->enable &&
> > @@ -6198,7 +6202,7 @@ static int stmmac_set_features(struct net_device *netdev,
> >  			stmmac_enable_sph(priv, priv->ioaddr, sph_en, chan);
> >  	}
> >  
> > -	stmmac_set_gso_types(priv, features & NETIF_F_TSO);
> > +	stmmac_set_gso_types(priv, features);
> >  
> >  	if (features & NETIF_F_HW_VLAN_CTAG_RX)
> >  		priv->hw->hw_vlan_en = true;
> -- 
> pw-bot: cr

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]