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

[PATCH net-next] 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 82375d34ad57..a2a0985e8c37 100644
--- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
+++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
@@ -6105,14 +6105,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;
 }
 
@@ -6144,6 +6136,8 @@ static int stmmac_set_features(struct net_device *netdev,
 	else
 		priv->hw->hw_vlan_en = false;
 
+	priv->tso = !!(features & NETIF_F_TSO);
+
 	phylink_rx_clk_stop_block(priv->phylink);
 	stmmac_set_hw_vlan_mode(priv, priv->hw);
 	phylink_rx_clk_stop_unblock(priv->phylink);
-- 
2.47.3

Re: [PATCH net-next] net: stmmac: fix .ndo_fix_features()

[Jakub Kicinski]

On Mon, 23 Feb 2026 11:24:51 +0000 Russell King (Oracle) wrote:
> 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.

No lies detected, but is this enough? The whole TSO enablement 
looks quite wobbly (as you mentioned in another email IIRC). 

Only stmmac_hw_setup() actually calls stmmac_enable_tso(). 
And stmmac_set_features() does not call stmmac_hw_setup().

IDK what the cost of having TSO enabled is for this IP, it's 
entirely possible that there is no cost. So maybe we should set
the TSO feature in features but not in hw_features which will
make it "fixed" to be always enabled? And not bother with handling 
the changes?

> 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 82375d34ad57..a2a0985e8c37 100644
> --- a/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> +++ b/drivers/net/ethernet/stmicro/stmmac/stmmac_main.c
> @@ -6105,14 +6105,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;
>  }
>  
> @@ -6144,6 +6136,8 @@ static int stmmac_set_features(struct net_device *netdev,
>  	else
>  		priv->hw->hw_vlan_en = false;
>  
> +	priv->tso = !!(features & NETIF_F_TSO);

extra nit, I think you're inserting this in the middle of a section of
code handling vlan config.

>  	phylink_rx_clk_stop_block(priv->phylink);
>  	stmmac_set_hw_vlan_mode(priv, priv->hw);
>  	phylink_rx_clk_stop_unblock(priv->phylink);
-- 
pw-bot: cr

Re: [PATCH net-next] net: stmmac: fix .ndo_fix_features()

[Russell King (Oracle)]

On Tue, Feb 24, 2026 at 05:30:37PM -0800, Jakub Kicinski wrote:
> On Mon, 23 Feb 2026 11:24:51 +0000 Russell King (Oracle) wrote:
> > 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.
> 
> No lies detected, but is this enough? The whole TSO enablement 
> looks quite wobbly (as you mentioned in another email IIRC). 
> 
> Only stmmac_hw_setup() actually calls stmmac_enable_tso(). 
> And stmmac_set_features() does not call stmmac_hw_setup().

Looking deeper, dwmac4_enable_tso() has two paths depending on the "en"
flag, both of which are commented as "enable TSO".

Then, looking at the "Enable TSO" block of code in stmmac_hw_setup(),
it looks to me like we can end up with some queues that have TSO
enabled, and others which don't (because TBS has been enabled on the
queue.) As far as I'm aware, the network layer doesn't support
per-queue TSO.

We can't call stmmac_hw_setup() from any path that we care about any
state being preserved - it issues a software reset which causes
everything, including the PTP clock, to be reset and all state then
needs to be reloaded.

Lastly, we don't need to disable the TSE bit (via stmmac_enable_tso()
when we disable TSO, because there's a bit in the descriptors that
also needs to be set for the packet to undergo TSO. However, if the
system undergoes a suspend/resume with TSO disabled by the user, we'll
call stmmac_hw_setup(), which as I note in the previous paragraph will
reset all state, including the TSE bit. stmmac_enable_tso() won't be
called, and thus the TSE bit will remain clear. If the user
subsequently re-enables TSO, then we'll queue packets for TSO but
the hardware won't have that enabled...

That's a problem today. So, while you're right to point this out (I
had missed it) it's a separate problem to the one being addressed
in this patch. In fact, I think there's two new issues that have now
been identified here.

1. The lack of setting TSE on resume if TSO is supported but but has
   been disabled by the user.
2. The interaction of TBS and TSO feature where some channels can
   support TSO and others not, which I don't think can be supported.
   So, I think if we have _any_ channel with TBS enabled, we have to
   disable TSO on the entire netdev.

I think these need to be separate patches, and this one is still
valid on its own.

-- 
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] net: stmmac: fix .ndo_fix_features()

[Russell King (Oracle)]

On Wed, Feb 25, 2026 at 08:24:10AM +0000, Russell King (Oracle) wrote:
> On Tue, Feb 24, 2026 at 05:30:37PM -0800, Jakub Kicinski wrote:
> > On Mon, 23 Feb 2026 11:24:51 +0000 Russell King (Oracle) wrote:
> > > 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.
> > 
> > No lies detected, but is this enough? The whole TSO enablement 
> > looks quite wobbly (as you mentioned in another email IIRC). 
> > 
> > Only stmmac_hw_setup() actually calls stmmac_enable_tso(). 
> > And stmmac_set_features() does not call stmmac_hw_setup().
> 
> Looking deeper, dwmac4_enable_tso() has two paths depending on the "en"
> flag, both of which are commented as "enable TSO".
> 
> Then, looking at the "Enable TSO" block of code in stmmac_hw_setup(),
> it looks to me like we can end up with some queues that have TSO
> enabled, and others which don't (because TBS has been enabled on the
> queue.) As far as I'm aware, the network layer doesn't support
> per-queue TSO.
> 
> We can't call stmmac_hw_setup() from any path that we care about any
> state being preserved - it issues a software reset which causes
> everything, including the PTP clock, to be reset and all state then
> needs to be reloaded.
> 
> Lastly, we don't need to disable the TSE bit (via stmmac_enable_tso()
> when we disable TSO, because there's a bit in the descriptors that
> also needs to be set for the packet to undergo TSO. However, if the
> system undergoes a suspend/resume with TSO disabled by the user, we'll
> call stmmac_hw_setup(), which as I note in the previous paragraph will
> reset all state, including the TSE bit. stmmac_enable_tso() won't be
> called, and thus the TSE bit will remain clear. If the user
> subsequently re-enables TSO, then we'll queue packets for TSO but
> the hardware won't have that enabled...
> 
> That's a problem today. So, while you're right to point this out (I
> had missed it) it's a separate problem to the one being addressed
> in this patch. In fact, I think there's two new issues that have now
> been identified here.
> 
> 1. The lack of setting TSE on resume if TSO is supported but but has
>    been disabled by the user.
> 2. The interaction of TBS and TSO feature where some channels can
>    support TSO and others not, which I don't think can be supported.
>    So, I think if we have _any_ channel with TBS enabled, we have to
>    disable TSO on the entire netdev.

With the assumption that TBS + TSO is not supported, then the following
drivers have a problem:

dwmac-intel.c: intel_mgbe_common_data() sets TBS for tx queues other
 than the first, but also sets STMMAC_FLAG_TSO_EN.

dwmac-qcom-ethqos.c: same as dwmac-intel for TBS, but TSO depends on a
 DT property.

dwmac-socfpga.c: always sets STMMAC_FLAG_TSO_EN, but configures queues
 6 and 7 for TBS.

stmmac_pci.c: snps_gmac5_default_data() sets STMMAC_FLAG_TSO_EN and
 enables TBS for tx queues other than the first.

In each of these cases, we end up with some transmit queues that can
support TSO, and others that can't (because TBS is enabled.)

What a nice can of worms...

The options as I see it are:
1. scan the transmit queue configuration, if any have TBS enabled,
   disable TSO support for the entire interface.

or

2. rip out TSO support, making the code simpler, and thereby removing
   the need to try and fix the problems here, and making this patch
   unnecessary.

-- 
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] net: stmmac: fix .ndo_fix_features()

[Jakub Kicinski]

On Wed, 25 Feb 2026 10:27:09 +0000 Russell King (Oracle) wrote:
> The options as I see it are:
> 1. scan the transmit queue configuration, if any have TBS enabled,
>    disable TSO support for the entire interface.
> 
> or
> 
> 2. rip out TSO support, making the code simpler, and thereby removing
>    the need to try and fix the problems here, and making this patch
>    unnecessary.

normally corner cases like this TSO + TBS thing are handled in
.ndo_features_check. The driver can selectively clear the TSO
feature for a single packet it sees heading down the stack
towards its TBS queue. The stack will then run GSO and feed 
it segments one by one.

FWIW Andrew's suggestion to do the GSO in the driver is very much
legit, but I agree that its orthogonal if you're trying to simply
fix brokenness.

Re: [PATCH net-next] net: stmmac: fix .ndo_fix_features()

[Andrew Lunn]

> Then, looking at the "Enable TSO" block of code in stmmac_hw_setup(),
> it looks to me like we can end up with some queues that have TSO
> enabled, and others which don't (because TBS has been enabled on the
> queue.) As far as I'm aware, the network layer doesn't support
> per-queue TSO.

There is a software implementation of TSO. See for example:

commit 3ae8f4e0b98b640aadf410c21185ccb6b5b02351
Author: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
Date:   Mon May 19 14:00:00 2014 -0300

    net: mv643xx_eth: Implement software TSO

You might be able to use this to fill in the gaps.

    Andrew

Re: [PATCH net-next] net: stmmac: fix .ndo_fix_features()

[Russell King (Oracle)]

On Wed, Feb 25, 2026 at 02:26:42PM +0100, Andrew Lunn wrote:
> > Then, looking at the "Enable TSO" block of code in stmmac_hw_setup(),
> > it looks to me like we can end up with some queues that have TSO
> > enabled, and others which don't (because TBS has been enabled on the
> > queue.) As far as I'm aware, the network layer doesn't support
> > per-queue TSO.
> 
> There is a software implementation of TSO. See for example:
> 
> commit 3ae8f4e0b98b640aadf410c21185ccb6b5b02351
> Author: Ezequiel Garcia <ezequiel.garcia@free-electrons.com>
> Date:   Mon May 19 14:00:00 2014 -0300
> 
>     net: mv643xx_eth: Implement software TSO
> 
> You might be able to use this to fill in the gaps.

That'll likely need some major rework to the descriptor handling, which
is also buggy, certainly in the non-TSO path.

stmmac_xmit() does this:

        if (unlikely(stmmac_tx_avail(priv, queue) < nfrags + 1)) {
                if (!netif_tx_queue_stopped(netdev_get_tx_queue(dev, queue))) {
                        netif_tx_stop_queue(netdev_get_tx_queue(priv->dev,
                                                                queue));
                        /* This is a hard error, log it. */
                        netdev_err(priv->dev,
                                   "%s: Tx Ring full when queue awake\n",
                                   __func__);
                }
                return NETDEV_TX_BUSY;
        }

Then it sets the descriptors, which are consumed as per:

- 1 descriptor if priv->dma_cap.vlins && skb has a vlan
- for non-jumbo:
  - 1 descriptor for skb head (with sarc & tbs)
- for jumbo:
  - if headlen > 8KiB:
    - 1 descriptor for first bmax (2 or 8 KiB) of skb head
        (sets buffer2 of descriptor to data + 4KiB)
    - 1 descriptor for remainder of skb head
        (sets buffer2 of descriptor to data + 4KiB)
  - otherwise:
    - 1 descriptor for skb head
        (sets buffer2 of descriptor to data + 4KiB)
- 1 descriptor per skb fragment

So, the number of descriptors used is 1 + nfrags + overhead, where
overhead is:

overhead += 1 if jumbo > 8KiB is supported
overhead += 1 if priv->dma_cap.vlins

To put it another way:
- if we have a non-jumbo non-vlan skb, then 1 + nfrags descriptors will
  be used.
- if we have a jumbo vlan skb, then 3 + nfrags descriptors will be
  used.

However, the decision to stop the queue is:

        if (unlikely(stmmac_tx_avail(priv, queue) <= (MAX_SKB_FRAGS + 1))) {
                netif_dbg(priv, hw, priv->dev, "%s: stop transmitted packets\n",
                          __func__);
                netif_tx_stop_queue(netdev_get_tx_queue(priv->dev, queue));
        }

So it doesn't account for the extra descriptor for vlan insertion, nor
does it account for the extra descriptor for a jumbo frame.

In the existing TSO path, the same problem exists - its the same check
for the last test, although the first test is different:

        /* Desc availability based on threshold should be enough safe */
        if (unlikely(stmmac_tx_avail(priv, queue) <
                (((skb->len - proto_hdr_len) / TSO_MAX_BUFF_SIZE + 1)))) {

This doesn't deal with vlan insertion by the hardware, nor I think
jumbo frames (I may be wrong on that) but it does use an extra
descriptor each time the MSS (as the driver calls it, actually
skb_shinfo(skb)->gso_size) changes on this specific queue. So, it
can be one extra descriptor on top of that +1 above.

The implications of this is that if the descriptors are almost full,
there is the possibility to overwrite the tail descriptor (that the
hardware has yet to process) as, when setting up the descriptors,
the insertion index is merely incremented, and not individually
checked to see whether it's caught up with the tail.

I think the only thing which saves the driver from this is that the
transmit descriptor ring is large (512 entries by default) and
probably never fills up in a way that we hit the boundary condition
that allows descriptor overwrite to occur. However, there is this
comment:

/* TX and RX Descriptor Length, these need to be power of two.
 * TX descriptor length less than 64 may cause transmit queue timed out error.
 * RX descriptor length less than 64 may cause inconsistent Rx chain error.
 */
in drivers/net/ethernet/stmicro/stmmac/common.h, which hints at the
problem here: if we do overflow the tail, the hardware may have already
processed that descriptor, but we haven't yet reaped it. The driver
will be expecting to write to the following descriptor. However, when
we real the tail, the driver will clear it. This results in following
packets being added after a descriptor with the hardware OWN bit clear,
so transmission will stop when the hardware gets to that descriptor,
eventually causing a transmit timeout.

With this driver, it's really a case of "which of its many problems
do we address first" because the more I look at it, the more problems
I'm finding.

The next problem which can be seen in my analysis of the descriptor
usage is - take a close look at the jumbo processing. In some
configurations, the length fields are 11 bits. The jumbo code seems
to know this because it calculates bmax accordingly. However, the
rest of the code just assumes that - even though we can only describe
up to 2KiB (actually 4KiB-1) bytes in the length field and it limits
the length of the first descriptor to that, it then promptly maps the
bytes from 4KiB to the total length to the second buffer in the
descriptor. What happens to the 2KiB..4KiB part of the data in this
case?!?!

I have a huge pile of cleanups to this driver that have helped to
uncover these problems by making the code more readable than it
currently is - and one of the problems is, I'm generating cleanups
and finding problems faster than I can get the changes merged.

During the last cycle, I tried to avoid adding to my set of patches
for this driver during the -rc phase. That hasn't helped. It also
meant that I didn't get any chance to post much else, such as the
Marvell PTP changes.

I was thinking that, when I get stmmac's phylink mess sorted out,
(essentially a few more patches beyond the part 3 RFC I posted today)
I'll stop hacking on stmmac, but stmmac just seems to be far too
broken. It needs someone to be a proper maintainer, someone who is
prepared to say "no" to patches that increase the messy complexity in
the driver that I think has led to a lot of these problems... to say
no to patches that introduce e.g. TBS without first thinking about
its interaction with TSO and whether the upper networking layers can
cope with some but not all queues being TSO capable... etc.

I do feel that, having got involved with stmmac to try and sort out
its bloody abuse of phylink, it's now become a huge mill stone, and
I don't have time to look at phylink / sfp related stuff (which is
why I've hardly been able to look at Maxime's patches.)

Can't we just delete stmmac to put it out of its misery? :D

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