Re: [PATCH net v3] net: stmmac: drop gso_enabled_types and rely on netdev features

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

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

> The gso_enabled_types field is used by stmmac_xmit() to decide whether a
> GSO skb should be passed to stmmac_tso_xmit(). It is updated in
> stmmac_set_features() based solely on NETIF_F_TSO, so disabling IPv4
> TSO while keeping IPv6 TSO (NETIF_F_TSO6) enabled zeroes the mask. As a
> result IPv6 GSO frames, which the networking stack still generates since
> NETIF_F_TSO6 is enabled, fall through to the non-TSO xmit path where
> they are not handled.
> 
> The networking stack already manages the GSO logic: a GSO skb is only
> delivered to the driver when the matching offload feature (NETIF_F_TSO,
> NETIF_F_TSO6 or NETIF_F_GSO_UDP_L4) is enabled, otherwise the frame is
> segmented in software before reaching ndo_start_xmit().
> stmmac_features_check() also validates each GSO frame against the TSO
> hardware constraints and falls back to software GSO when they are not met.
> 
> Drop the gso_enabled_types field and rely on skb_is_gso() in
> stmmac_xmit() instead, which correctly routes IPv6 GSO frames to the TSO
> path when NETIF_F_TSO is disabled. This also removes the data race
> between stmmac_set_gso_types(), called from the feature-set path, and
> the lockless read of gso_enabled_types in stmmac_xmit().
> 
> Fixes: 2e4082e4b739 ("net: stmmac: simplify GSO/TSO test in stmmac_xmit()")

Commenting on sashiko's report:
https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260824-stmmac-fix-tso6-features-v3-1-c73a7a4a0ec7%40oss.qualcomm.com

- Is 2e4082e4b739 really the commit that introduced this behaviour?
  - Commit 2e4082e4b739 is introducing the gso_enabled_types field. Using a
    cached value for gso capabilities introduces a race when the user modifies
    them via ethtool.
    The other issues this patch is resolving are quite old:
    - commit 9edfa7dab8112a012b349b7937f5444fdc21e8f9
      Author: Niklas Cassel <niklas.cassel@axis.com>
      Date:   Mon Jun 19 18:36:44 2017 +0200
    - commit b776620651a1182976b51643bb2c5b08d535fb2e
      Author: Jose Abreu <Jose.Abreu@synopsys.com>
      Date:   Mon Nov 11 15:42:39 2019 +0100
    We can add these two more Fixes tag to the patch but I guess it will be
    hard to backport till that version.

Regards,
Lorenzo

> Signed-off-by: Lorenzo Bianconi <lorenzo.bianconi@oss.qualcomm.com>
> ---
> Changes in v3:
> - Drop gso_enabled_types field and rely on the network stack to manage
>   TSO/GSO logic.
> - Link to v2: https://lore.kernel.org/r/20260812-stmmac-fix-tso6-features-v2-1-72c3b06eb8d4@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.h      |  2 --
>  drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 19 +------------------
>  2 files changed, 1 insertion(+), 20 deletions(-)
> 
> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac.h b/drivers/net/ethernet/stmicro/stmmac/stmmac.h
> index 8ba8f03e1ce0..7582fca63741 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac.h
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac.h
> @@ -265,8 +265,6 @@ struct stmmac_priv {
>  	u32 rx_coal_frames[MTL_MAX_RX_QUEUES];
>  
>  	int hwts_tx_en;
> -	/* skb_shinfo(skb)->gso_type types that we handle */
> -	unsigned int gso_enabled_types;
>  	bool tx_path_in_lpi_mode;
>  	bool sph_active;
>  	bool sph_capable;
> diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> index b2b7d0242dd3..3a956c8b611b 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> @@ -4375,18 +4375,6 @@ 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)
> -{
> -	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;
> -	}
> -}
> -
>  static void stmmac_set_gso_features(struct net_device *ndev)
>  {
>  	struct stmmac_priv *priv = netdev_priv(ndev);
> @@ -4420,8 +4408,6 @@ 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);
> -
>  	dev_info(priv->device, "TSO feature enabled\n");
>  }
>  
> @@ -4771,8 +4757,7 @@ static netdev_tx_t stmmac_xmit(struct sk_buff *skb, struct net_device *dev)
>  	if (priv->tx_path_in_lpi_mode && priv->eee_sw_timer_en)
>  		stmmac_stop_sw_lpi(priv);
>  
> -	if (skb_is_gso(skb) &&
> -	    skb_shinfo(skb)->gso_type & priv->gso_enabled_types)
> +	if (skb_is_gso(skb))
>  		return stmmac_tso_xmit(skb, dev);
>  
>  	if (priv->est && priv->est->enable &&
> @@ -6206,8 +6191,6 @@ 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);
> -
>  	if (features & NETIF_F_HW_VLAN_CTAG_RX)
>  		priv->hw->hw_vlan_en = true;
>  	else
> 
> ---
> base-commit: 7cbfb180945ce529608e4d4e24a6d483699fab1e
> change-id: 20260808-stmmac-fix-tso6-features-7fdc5e9448e2
> 
> Best regards,
> -- 
> Lorenzo Bianconi <lorenzo.bianconi@oss.qualcomm.com>
> 

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