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

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

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!