Re: [PATCH net-next 01/10] net: stmmac: fix TSO support when some channels have TBS available

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

On Sun, Mar 29, 2026 at 10:42:23AM -0700, Jakub Kicinski wrote:
> On Sun, 29 Mar 2026 10:40:43 +0100 Russell King (Oracle) wrote:
> > On Sat, Mar 28, 2026 at 09:36:41PM +0000, Russell King (Oracle) wrote:
> > > According to the STM32MP25xx manual, which is dwmac v5.3, TBS (time
> > > based scheduling) is not permitted for channels which have hardware
> > > TSO enabled. Intel's commit 5e6038b88a57 ("net: stmmac: fix TSO and
> > > TBS feature enabling during driver open") concurs with this, but it
> > > is incomplete.
> > > 
> > > This commit avoids enabling TSO support on the channels which have
> > > TBS available, which, as far as the hardware is concerned, means we
> > > do not set the TSE bit in the DMA channel's transmit control register.
> > > 
> > > However, the net device's features apply to all queues(channels), which
> > > means these channels may still be handed TSO skbs to transmit, and the
> > > driver will pass them to stmmac_tso_xmit(). This will generate the
> > > descriptors for TSO, even though the channel has the TSE bit clear.
> > > 
> > > Fix this by checking whether the queue(channel) has TBS available,
> > > and if it does, fall back to software GSO support.  
> > 
> > This is sufficient for the immediate issue of fixing the patch below,
> > but I think there's another issue that also needs fixing here.
> > 
> > TSO requires the hardware to support checksum offload, and there is
> > a comment in the driver:
> > 
> >         /* DWMAC IPs can be synthesized to support tx coe only for a few tx
> >          * queues. In that case, checksum offloading for those queues that don't
> >          * support tx coe needs to fallback to software checksum calculation.
> >          *
> >          * Packets that won't trigger the COE e.g. most DSA-tagged packets will
> >          * also have to be checksummed in software.
> >          */
> > 
> > So, it seems at the very least we need to add a check (in a subsequent
> > patch) for priv->plat->tx_queues_cfg[queue].coe_unsupported to
> > stmmac_channel_tso_permitted().
> > 
> > I'm also wondering about the stmmac_has_ip_ethertype() thing, which
> > checks whether the skb can be checksummed by the hardware, and how that
> > interacts with TSO, and whether that's yet another hole that needs
> > plugging.
> 
> If the driver "un-advertises" checksum offload accordingly the core
> should automatically clear TSO feature.

Ah, yes, it's in harmonize_features().

However, I think that stmmac_has_ip_ethertype() is tighter than the
checks that harmonize_features() does.

stmmac_has_ip_ethertype():
        int depth = 0;
        __be16 proto;

        proto = __vlan_get_protocol(skb, eth_header_parse_protocol(skb),
                                    &depth);

        return (depth <= ETH_HLEN) &&
                (proto == htons(ETH_P_IP) || proto == htons(ETH_P_IPV6));

If I'm reading this correctly, then eth_header_parse_protocol(skb)
will return the contents of the ethernet header protocol field.

__vlan_get_protocol() will then return:

 - that protocol if it is not a vlan (0x8100 or 0x88A8)
   with depth set to skb->mac_len, which should be ETH_HLEN here.
 - the protocol below the vlan headers, in which case depth
   will be > ETH_HLEN

Unless I've missed something, that basically means that the 
__vlan_get_protocol() is pointless there, and the entire function
could just be checking that eth_header_parse_protocol(skb) returns
IP or IPv6.

Isn't it the case that skb->protocol should be the same as
eth_header_parse_protocol(skb) for a packet being transmitted at the
point that .ndo_features_check() or .ndo_start_xmit() is called?

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