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 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!