[PATCH net-next 0/2] net: stmmac: fix and clean up TSO/GSO support

[PATCH net-next 0/2] net: stmmac: fix and clean up TSO/GSO support

[Russell King (Oracle)]

This series:

1. moves setting priv->tso from stmmac_fix_features() to
   stmmac_set_features() as fix_features is not supposed to change
   driver state.

2. simplifies the TSO/GSP test in stmmac_xmit() to simply check if the
   skbuff is a GSO buffer, and whether the gso_type is one that we
   should handle.

A previous version of patch 1 was posted in February:

https://lore.kernel.org/r/E1vuU3X-0000000Ae9G-1Er8@rmk-PC.armlinux.org.uk/

 drivers/net/ethernet/stmicro/stmmac/stmmac.h      |  3 +-
 drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 38 +++++++++++------------
 2 files changed, 21 insertions(+), 20 deletions(-)

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

[PATCH net-next 1/2] net: stmmac: fix .ndo_fix_features()

[Russell King (Oracle)]

netdev features documentation requires that .ndo_fix_features() is
stateless: it shouldn't modify driver state. Yet, stmmac_fix_features()
does exactly that, changing whether GSO frames are processed by the
driver.

Move this code to stmmac_set_features() instead, which is the correct
place for it. We don't need to check whether TSO is supported; this
is already handled via the setup of netdev->hw_features, and we are
guaranteed that if netdev->hw_features indicates that a feature is
not supported, .ndo_set_features() won't be called with it set.

Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
---
 drivers/net/ethernet/stmicro/stmmac/stmmac_main.c | 10 ++--------
 1 file changed, 2 insertions(+), 8 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index 9b6b49331639..8203d6845b00 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -6068,14 +6068,6 @@ static netdev_features_t stmmac_fix_features(struct net_device *dev,
 	if (priv->plat->bugged_jumbo && (dev->mtu > ETH_DATA_LEN))
 		features &= ~NETIF_F_CSUM_MASK;
 
-	/* Disable tso if asked by ethtool */
-	if ((priv->plat->flags & STMMAC_FLAG_TSO_EN) && (priv->dma_cap.tsoen)) {
-		if (features & NETIF_F_TSO)
-			priv->tso = true;
-		else
-			priv->tso = false;
-	}
-
 	return features;
 }
 
@@ -6102,6 +6094,8 @@ static int stmmac_set_features(struct net_device *netdev,
 			stmmac_enable_sph(priv, priv->ioaddr, sph_en, chan);
 	}
 
+	priv->tso = !!(features & NETIF_F_TSO);
+
 	if (features & NETIF_F_HW_VLAN_CTAG_RX)
 		priv->hw->hw_vlan_en = true;
 	else
-- 
2.47.3

[PATCH net-next 2/2] net: stmmac: simplify GSO/TSO test in stmmac_xmit()

[Russell King (Oracle)]

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.

Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
---
 drivers/net/ethernet/stmicro/stmmac/stmmac.h  |  3 +-
 .../net/ethernet/stmicro/stmmac/stmmac_main.c | 30 +++++++++++--------
 2 files changed, 20 insertions(+), 13 deletions(-)

diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac.h b/drivers/net/ethernet/stmicro/stmmac/stmmac.h
index 919a93a52390..8ba8f03e1ce0 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac.h
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac.h
@@ -265,8 +265,9 @@ 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 tso;
 	bool sph_active;
 	bool sph_capable;
 	u32 sarc_type;
diff --git a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
index 8203d6845b00..d3e8d793af52 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -3700,7 +3700,7 @@ static int stmmac_hw_setup(struct net_device *dev)
 	stmmac_set_rings_length(priv);
 
 	/* Enable TSO */
-	if (priv->tso) {
+	if (priv->gso_enabled_types) {
 		for (chan = 0; chan < tx_cnt; chan++) {
 			struct stmmac_tx_queue *tx_q = &priv->dma_conf.tx_queue[chan];
 
@@ -4675,7 +4675,6 @@ static netdev_tx_t stmmac_xmit(struct sk_buff *skb, struct net_device *dev)
 	u32 queue = skb_get_queue_mapping(skb);
 	int nfrags = skb_shinfo(skb)->nr_frags;
 	unsigned int first_entry, tx_packets;
-	int gso = skb_shinfo(skb)->gso_type;
 	struct stmmac_txq_stats *txq_stats;
 	struct dma_desc *desc, *first_desc;
 	struct stmmac_tx_queue *tx_q;
@@ -4687,14 +4686,9 @@ 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);
 
-	/* Manage oversized TCP frames for GMAC4 device */
-	if (skb_is_gso(skb) && priv->tso) {
-		if (gso & (SKB_GSO_TCPV4 | SKB_GSO_TCPV6))
-			return stmmac_tso_xmit(skb, dev);
-		if (priv->plat->core_type == DWMAC_CORE_GMAC4 &&
-		    (gso & SKB_GSO_UDP_L4))
-			return stmmac_tso_xmit(skb, dev);
-	}
+	if (skb_is_gso(skb) &&
+	    skb_shinfo(skb)->gso_type & priv->gso_enabled_types)
+		return stmmac_tso_xmit(skb, dev);
 
 	if (priv->est && priv->est->enable &&
 	    priv->est->max_sdu[queue]) {
@@ -6049,6 +6043,18 @@ static int stmmac_change_mtu(struct net_device *dev, int new_mtu)
 	return 0;
 }
 
+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 netdev_features_t stmmac_fix_features(struct net_device *dev,
 					     netdev_features_t features)
 {
@@ -6094,7 +6100,7 @@ static int stmmac_set_features(struct net_device *netdev,
 			stmmac_enable_sph(priv, priv->ioaddr, sph_en, chan);
 	}
 
-	priv->tso = !!(features & NETIF_F_TSO);
+	stmmac_set_gso_types(priv, features & NETIF_F_TSO);
 
 	if (features & NETIF_F_HW_VLAN_CTAG_RX)
 		priv->hw->hw_vlan_en = true;
@@ -7828,7 +7834,7 @@ static int __stmmac_dvr_probe(struct device *device,
 		ndev->hw_features |= NETIF_F_TSO | NETIF_F_TSO6;
 		if (priv->plat->core_type == DWMAC_CORE_GMAC4)
 			ndev->hw_features |= NETIF_F_GSO_UDP_L4;
-		priv->tso = true;
+		stmmac_set_gso_types(priv, true);
 		dev_info(priv->device, "TSO feature enabled\n");
 	}
 
-- 
2.47.3

Re: [PATCH net-next 2/2] net: stmmac: simplify GSO/TSO test in stmmac_xmit()

[Russell King (Oracle)]

On Fri, Mar 27, 2026 at 09:40:09AM +0000, Russell King (Oracle) wrote:
> 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.
> 
> Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>

AI review of this patch regurgitates Jakub's point that was discussed.

> @@ -3700,7 +3700,7 @@ static int stmmac_hw_setup(struct net_device *dev)
>  	stmmac_set_rings_length(priv);
>  
>  	/* Enable TSO */
> -	if (priv->tso) {
> +	if (priv->gso_enabled_types) {
>  		for (chan = 0; chan < tx_cnt; chan++) {
>  			struct stmmac_tx_queue *tx_q = &priv->dma_conf.tx_queue[chan];
>  

...

> @@ -7828,7 +7834,7 @@ static int __stmmac_dvr_probe(struct device *device,
>  		ndev->hw_features |= NETIF_F_TSO | NETIF_F_TSO6;
>  		if (priv->plat->core_type == DWMAC_CORE_GMAC4)
>  			ndev->hw_features |= NETIF_F_GSO_UDP_L4;
> -		priv->tso = true;
> +		stmmac_set_gso_types(priv, true);

Clearly, the issue it is regurgitating has been there for a long time
and isn't a new issue introduced by this patch.

AI needs to stop doing this, because it is encouraging multiple changes
in a single patch, which is against the normal kernel process.

As already pointed out, there are multiple issues with stmmac TSO
support, particularly with glue drivers that enable TSO on some
queues/channels and not others, since netdev core TSO support is
global across all channels.

So, won't the AI response in this patch - it's just another pre-
existing issue that needs fixing in a separate patch.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH net-next 2/2] net: stmmac: simplify GSO/TSO test in stmmac_xmit()

[Russell King (Oracle)]

On Sat, Mar 28, 2026 at 08:24:37AM +0000, Russell King (Oracle) wrote:
> On Fri, Mar 27, 2026 at 09:40:09AM +0000, Russell King (Oracle) wrote:
> > 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.
> > 
> > Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
> 
> AI review of this patch regurgitates Jakub's point that was discussed.
> 
> > @@ -3700,7 +3700,7 @@ static int stmmac_hw_setup(struct net_device *dev)
> >  	stmmac_set_rings_length(priv);
> >  
> >  	/* Enable TSO */
> > -	if (priv->tso) {
> > +	if (priv->gso_enabled_types) {
> >  		for (chan = 0; chan < tx_cnt; chan++) {
> >  			struct stmmac_tx_queue *tx_q = &priv->dma_conf.tx_queue[chan];
> >  
> 
> ...
> 
> > @@ -7828,7 +7834,7 @@ static int __stmmac_dvr_probe(struct device *device,
> >  		ndev->hw_features |= NETIF_F_TSO | NETIF_F_TSO6;
> >  		if (priv->plat->core_type == DWMAC_CORE_GMAC4)
> >  			ndev->hw_features |= NETIF_F_GSO_UDP_L4;
> > -		priv->tso = true;
> > +		stmmac_set_gso_types(priv, true);
> 
> Clearly, the issue it is regurgitating has been there for a long time
> and isn't a new issue introduced by this patch.
> 
> AI needs to stop doing this, because it is encouraging multiple changes
> in a single patch, which is against the normal kernel process.
> 
> As already pointed out, there are multiple issues with stmmac TSO
> support, particularly with glue drivers that enable TSO on some
> queues/channels and not others, since netdev core TSO support is
> global across all channels.
> 
> So, won't the AI response in this patch - it's just another pre-
> existing issue that needs fixing in a separate patch.

Looking at the TSO vs TBS issue (which precludes the use of TSO on a
channel in stmmac) I can't find an obvious reason for this in the
available documentation. However, unfortunately, iMX8MP doesn't support
TSO, so the TSO bits are elided there, but does support TBS (needing
enhanced descriptors to be enabled). STM32MP151 on the other hand
supports TSO but not TBS, and thus fails to mention anything about
enhanced descriptors or TBS.

When stmmac_enable_tbs() enables TBS, it isn't actually enabling a
feature specific bit, but switching the channel to use enhanced
descriptor format. This format extends the basic descriptors by
placing four extra 32-bit words before the basic descriptor.

Looking at the enhanced normal descriptor format for TDES3, it
indicates that the format includes bit 18 in the control field, which
is the TSE bit (TCP segmentation enable for this packet.) So, it seems
it's not a limitation of the descriptor format.

So, either "TSO and TBS cannot co-exist" is incorrect, or there is a
hardware limitation that isn't documented between these two manuals.

One other interesting point is that stmmac_tso_xmit() seems to
handle the case where TSO and TBS are enabled on the channel:

                if (tx_q->tbs & STMMAC_TBS_AVAIL)
                        mss_desc = &tx_q->dma_entx[tx_q->cur_tx].basic;
                else
                        mss_desc = &tx_q->dma_tx[tx_q->cur_tx];

                stmmac_set_mss(priv, mss_desc, mss);
...
        if (tx_q->tbs & STMMAC_TBS_AVAIL)
                desc = &tx_q->dma_entx[first_entry].basic;
        else
                desc = &tx_q->dma_tx[first_entry];
        first = desc;

etc.

Avoiding enabling TSO for a TBS channel was added by this commit:

commit 5e6038b88a5718910dd74b949946d9d9cee9a041
Author: Ong Boon Leong <boon.leong.ong@intel.com>
Date:   Wed Apr 21 17:11:49 2021 +0800

    net: stmmac: fix TSO and TBS feature enabling during driver open

    TSO and TBS cannot co-exist and current implementation requires two
    fixes:

     1) stmmac_open() does not need to call stmmac_enable_tbs() because
        the MAC is reset in stmmac_init_dma_engine() anyway.
     2) Inside stmmac_hw_setup(), we should call stmmac_enable_tso() for
        TX Q that is _not_ configured for TBS.

    Fixes: 579a25a854d4 ("net: stmmac: Initial support for TBS")
    Signed-off-by: Ong Boon Leong <boon.leong.ong@intel.com>
    Signed-off-by: David S. Miller <davem@davemloft.net>

which doesn't really explain the background, and leaves all the TBS
cruft in the TSO transmit path (nothing like properly updating the
driver, eh? No wonder stmmac is such a mess!)

Maybe Ong Boon Leong can indicate where this restriction comes from?
Note: as mentioned previously, disabling TSO only on some channels is
actually wrong - the netdev core doesn't know which channels support
TSO and which don't, so the driver is likely to still get TSO skbuffs
for channels that the above commit has disabled TSO support. So, if
TBS is enabled and it is incompatible with TSO, then either we need
to use software TSO support _or_ disable TSO for the entire interface.

Incidentally, while looking at this, I found a few more pre-conditions
for TSO:

- TxPBL must be >= 4
- MSS[13:0] must be more than the configured data width in bytes, up to
  a maximum of 1023 bytes.

I'm fairly certain that the driver does nothing to ensure that this
in the case for either of these two points.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH net-next 2/2] net: stmmac: simplify GSO/TSO test in stmmac_xmit()

[Russell King (Oracle)]

On Sat, Mar 28, 2026 at 09:31:46AM +0000, Russell King (Oracle) wrote:
> On Sat, Mar 28, 2026 at 08:24:37AM +0000, Russell King (Oracle) wrote:
> > On Fri, Mar 27, 2026 at 09:40:09AM +0000, Russell King (Oracle) wrote:
> > > 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.
> > > 
> > > Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
> > 
> > AI review of this patch regurgitates Jakub's point that was discussed.
> > 
> > > @@ -3700,7 +3700,7 @@ static int stmmac_hw_setup(struct net_device *dev)
> > >  	stmmac_set_rings_length(priv);
> > >  
> > >  	/* Enable TSO */
> > > -	if (priv->tso) {
> > > +	if (priv->gso_enabled_types) {
> > >  		for (chan = 0; chan < tx_cnt; chan++) {
> > >  			struct stmmac_tx_queue *tx_q = &priv->dma_conf.tx_queue[chan];
> > >  
> > 
> > ...
> > 
> > > @@ -7828,7 +7834,7 @@ static int __stmmac_dvr_probe(struct device *device,
> > >  		ndev->hw_features |= NETIF_F_TSO | NETIF_F_TSO6;
> > >  		if (priv->plat->core_type == DWMAC_CORE_GMAC4)
> > >  			ndev->hw_features |= NETIF_F_GSO_UDP_L4;
> > > -		priv->tso = true;
> > > +		stmmac_set_gso_types(priv, true);
> > 
> > Clearly, the issue it is regurgitating has been there for a long time
> > and isn't a new issue introduced by this patch.
> > 
> > AI needs to stop doing this, because it is encouraging multiple changes
> > in a single patch, which is against the normal kernel process.
> > 
> > As already pointed out, there are multiple issues with stmmac TSO
> > support, particularly with glue drivers that enable TSO on some
> > queues/channels and not others, since netdev core TSO support is
> > global across all channels.
> > 
> > So, won't the AI response in this patch - it's just another pre-
> > existing issue that needs fixing in a separate patch.
> 
> Looking at the TSO vs TBS issue (which precludes the use of TSO on a
> channel in stmmac) I can't find an obvious reason for this in the
> available documentation. However, unfortunately, iMX8MP doesn't support
> TSO, so the TSO bits are elided there, but does support TBS (needing
> enhanced descriptors to be enabled). STM32MP151 on the other hand
> supports TSO but not TBS, and thus fails to mention anything about
> enhanced descriptors or TBS.
> 
> When stmmac_enable_tbs() enables TBS, it isn't actually enabling a
> feature specific bit, but switching the channel to use enhanced
> descriptor format. This format extends the basic descriptors by
> placing four extra 32-bit words before the basic descriptor.
> 
> Looking at the enhanced normal descriptor format for TDES3, it
> indicates that the format includes bit 18 in the control field, which
> is the TSE bit (TCP segmentation enable for this packet.) So, it seems
> it's not a limitation of the descriptor format.
> 
> So, either "TSO and TBS cannot co-exist" is incorrect, or there is a
> hardware limitation that isn't documented between these two manuals.
> 
> One other interesting point is that stmmac_tso_xmit() seems to
> handle the case where TSO and TBS are enabled on the channel:
> 
>                 if (tx_q->tbs & STMMAC_TBS_AVAIL)
>                         mss_desc = &tx_q->dma_entx[tx_q->cur_tx].basic;
>                 else
>                         mss_desc = &tx_q->dma_tx[tx_q->cur_tx];
> 
>                 stmmac_set_mss(priv, mss_desc, mss);
> ...
>         if (tx_q->tbs & STMMAC_TBS_AVAIL)
>                 desc = &tx_q->dma_entx[first_entry].basic;
>         else
>                 desc = &tx_q->dma_tx[first_entry];
>         first = desc;
> 
> etc.
> 
> Avoiding enabling TSO for a TBS channel was added by this commit:
> 
> commit 5e6038b88a5718910dd74b949946d9d9cee9a041
> Author: Ong Boon Leong <boon.leong.ong@intel.com>
> Date:   Wed Apr 21 17:11:49 2021 +0800
> 
>     net: stmmac: fix TSO and TBS feature enabling during driver open
> 
>     TSO and TBS cannot co-exist and current implementation requires two
>     fixes:
> 
>      1) stmmac_open() does not need to call stmmac_enable_tbs() because
>         the MAC is reset in stmmac_init_dma_engine() anyway.
>      2) Inside stmmac_hw_setup(), we should call stmmac_enable_tso() for
>         TX Q that is _not_ configured for TBS.
> 
>     Fixes: 579a25a854d4 ("net: stmmac: Initial support for TBS")
>     Signed-off-by: Ong Boon Leong <boon.leong.ong@intel.com>
>     Signed-off-by: David S. Miller <davem@davemloft.net>
> 
> which doesn't really explain the background, and leaves all the TBS
> cruft in the TSO transmit path (nothing like properly updating the
> driver, eh? No wonder stmmac is such a mess!)

The more I look at this, the more I'm convinced this commit is
incorrect, even if it is the case that the hardware doesn't support
TSO and TBS together.

When TSO is enabled (NETIF_F_TSO set in the netif's features) then
the core net layer can submit skbuffs that need to be processed using
TSO.

If such a skbuff hits a channel that has TSO disabled (because the
above commit caused:

	stmmac_enable_tso(priv, priv->ioaddr, 1, chan);

not to be called) then the TSE bit in the transmit control register
will not be set, thereby disabling TSO on this particular channel.

However, stmmac_xmit() will still call through to stmmac_tso_xmit()
which will dutifully populate the transmit ring with descriptors that
assume TSE has been set in the transmit control register.

It seems to me _that_ is even more broken than "the hardware doesn't
support TSO and TBS together" - I have no idea what the stmmac hardware
does if it encounters descriptors with TSE set but TSE is disabled in
the transmit control register. My guess would be it would ignore the
TSE bit in the descriptor and assume that it's one very large packet
to be sent - and either error out because it's longer than the
maximum the hardware can support or it will just try to transmit it
anyway.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH net-next 2/2] net: stmmac: simplify GSO/TSO test in stmmac_xmit()

[Russell King (Oracle)]

On Sat, Mar 28, 2026 at 05:25:58PM +0000, Russell King (Oracle) wrote:
> On Sat, Mar 28, 2026 at 09:31:46AM +0000, Russell King (Oracle) wrote:
> > On Sat, Mar 28, 2026 at 08:24:37AM +0000, Russell King (Oracle) wrote:
> > > On Fri, Mar 27, 2026 at 09:40:09AM +0000, Russell King (Oracle) wrote:
> > > > 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.
> > > > 
> > > > Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
> > > 
> > > AI review of this patch regurgitates Jakub's point that was discussed.
> > > 
> > > > @@ -3700,7 +3700,7 @@ static int stmmac_hw_setup(struct net_device *dev)
> > > >  	stmmac_set_rings_length(priv);
> > > >  
> > > >  	/* Enable TSO */
> > > > -	if (priv->tso) {
> > > > +	if (priv->gso_enabled_types) {
> > > >  		for (chan = 0; chan < tx_cnt; chan++) {
> > > >  			struct stmmac_tx_queue *tx_q = &priv->dma_conf.tx_queue[chan];
> > > >  
> > > 
> > > ...
> > > 
> > > > @@ -7828,7 +7834,7 @@ static int __stmmac_dvr_probe(struct device *device,
> > > >  		ndev->hw_features |= NETIF_F_TSO | NETIF_F_TSO6;
> > > >  		if (priv->plat->core_type == DWMAC_CORE_GMAC4)
> > > >  			ndev->hw_features |= NETIF_F_GSO_UDP_L4;
> > > > -		priv->tso = true;
> > > > +		stmmac_set_gso_types(priv, true);
> > > 
> > > Clearly, the issue it is regurgitating has been there for a long time
> > > and isn't a new issue introduced by this patch.
> > > 
> > > AI needs to stop doing this, because it is encouraging multiple changes
> > > in a single patch, which is against the normal kernel process.
> > > 
> > > As already pointed out, there are multiple issues with stmmac TSO
> > > support, particularly with glue drivers that enable TSO on some
> > > queues/channels and not others, since netdev core TSO support is
> > > global across all channels.
> > > 
> > > So, won't the AI response in this patch - it's just another pre-
> > > existing issue that needs fixing in a separate patch.
> > 
> > Looking at the TSO vs TBS issue (which precludes the use of TSO on a
> > channel in stmmac) I can't find an obvious reason for this in the
> > available documentation. However, unfortunately, iMX8MP doesn't support
> > TSO, so the TSO bits are elided there, but does support TBS (needing
> > enhanced descriptors to be enabled). STM32MP151 on the other hand
> > supports TSO but not TBS, and thus fails to mention anything about
> > enhanced descriptors or TBS.
> > 
> > When stmmac_enable_tbs() enables TBS, it isn't actually enabling a
> > feature specific bit, but switching the channel to use enhanced
> > descriptor format. This format extends the basic descriptors by
> > placing four extra 32-bit words before the basic descriptor.
> > 
> > Looking at the enhanced normal descriptor format for TDES3, it
> > indicates that the format includes bit 18 in the control field, which
> > is the TSE bit (TCP segmentation enable for this packet.) So, it seems
> > it's not a limitation of the descriptor format.
> > 
> > So, either "TSO and TBS cannot co-exist" is incorrect, or there is a
> > hardware limitation that isn't documented between these two manuals.
> > 
> > One other interesting point is that stmmac_tso_xmit() seems to
> > handle the case where TSO and TBS are enabled on the channel:
> > 
> >                 if (tx_q->tbs & STMMAC_TBS_AVAIL)
> >                         mss_desc = &tx_q->dma_entx[tx_q->cur_tx].basic;
> >                 else
> >                         mss_desc = &tx_q->dma_tx[tx_q->cur_tx];
> > 
> >                 stmmac_set_mss(priv, mss_desc, mss);
> > ...
> >         if (tx_q->tbs & STMMAC_TBS_AVAIL)
> >                 desc = &tx_q->dma_entx[first_entry].basic;
> >         else
> >                 desc = &tx_q->dma_tx[first_entry];
> >         first = desc;
> > 
> > etc.
> > 
> > Avoiding enabling TSO for a TBS channel was added by this commit:
> > 
> > commit 5e6038b88a5718910dd74b949946d9d9cee9a041
> > Author: Ong Boon Leong <boon.leong.ong@intel.com>
> > Date:   Wed Apr 21 17:11:49 2021 +0800
> > 
> >     net: stmmac: fix TSO and TBS feature enabling during driver open
> > 
> >     TSO and TBS cannot co-exist and current implementation requires two
> >     fixes:
> > 
> >      1) stmmac_open() does not need to call stmmac_enable_tbs() because
> >         the MAC is reset in stmmac_init_dma_engine() anyway.
> >      2) Inside stmmac_hw_setup(), we should call stmmac_enable_tso() for
> >         TX Q that is _not_ configured for TBS.
> > 
> >     Fixes: 579a25a854d4 ("net: stmmac: Initial support for TBS")
> >     Signed-off-by: Ong Boon Leong <boon.leong.ong@intel.com>
> >     Signed-off-by: David S. Miller <davem@davemloft.net>
> > 
> > which doesn't really explain the background, and leaves all the TBS
> > cruft in the TSO transmit path (nothing like properly updating the
> > driver, eh? No wonder stmmac is such a mess!)
> 
> The more I look at this, the more I'm convinced this commit is
> incorrect, even if it is the case that the hardware doesn't support
> TSO and TBS together.
> 
> When TSO is enabled (NETIF_F_TSO set in the netif's features) then
> the core net layer can submit skbuffs that need to be processed using
> TSO.
> 
> If such a skbuff hits a channel that has TSO disabled (because the
> above commit caused:
> 
> 	stmmac_enable_tso(priv, priv->ioaddr, 1, chan);
> 
> not to be called) then the TSE bit in the transmit control register
> will not be set, thereby disabling TSO on this particular channel.
> 
> However, stmmac_xmit() will still call through to stmmac_tso_xmit()
> which will dutifully populate the transmit ring with descriptors that
> assume TSE has been set in the transmit control register.
> 
> It seems to me _that_ is even more broken than "the hardware doesn't
> support TSO and TBS together" - I have no idea what the stmmac hardware
> does if it encounters descriptors with TSE set but TSE is disabled in
> the transmit control register. My guess would be it would ignore the
> TSE bit in the descriptor and assume that it's one very large packet
> to be sent - and either error out because it's longer than the
> maximum the hardware can support or it will just try to transmit it
> anyway.

Okay, I've found a statement in the stm32mp25xx documentation which
backs up Intel's commit, but that commit is still wrong because it
only half does the job.

However, I think I now have a solution - implementing
.ndo_features_check() which will mask out the NETIF_F_GSO_MASK
features if either the header length is greater than 1023 (the
hardware maximum) or the queue (as returned by
skb_get_queue_mapping(skb)) is for a queue which has TBS available,
and thus has TSO disabled.

I'm surprised we haven't had reports of brokenness in this area.

I think it's time to re-shuffle these patches (plus I think there's
a bit more scope to clean up some of the TSO code which would mean
placing stmmac_set_gso_types() in a different location to keep
all the TSO-related code together. I'll mark this series as
superseded once I have its replacement ready.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!

Re: [PATCH net-next 2/2] net: stmmac: simplify GSO/TSO test in stmmac_xmit()

[Andrew Lunn]

> I'm surprised we haven't had reports of brokenness in this area.

Could a self test be written for this case?

      Andrew