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

["Russell King (Oracle)" <linux@armlinux.org.uk>]

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!