netdev - Re: [PATCH net-next 2/7] net: stmmac: move tx_lpi

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <Z3cM-5tShVza0M58@shell.armlinux.org.uk>
Date: Thu, 2 Jan 2025 22:02:35 +0000
From: "Russell King (Oracle)" <linux@...linux.org.uk>
To: Andrew Lunn <andrew@...n.ch>, Heiner Kallweit <hkallweit1@...il.com>,
	Alexandre Torgue <alexandre.torgue@...s.st.com>,
	Andrew Lunn <andrew+netdev@...n.ch>,
	"David S. Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>,
	Jakub Kicinski <kuba@...nel.org>, Jose Abreu <joabreu@...opsys.com>,
	linux-arm-kernel@...ts.infradead.org,
	linux-stm32@...md-mailman.stormreply.com,
	Maxime Coquelin <mcoquelin.stm32@...il.com>, netdev@...r.kernel.org,
	Paolo Abeni <pabeni@...hat.com>
Subject: Re: [PATCH net-next 2/7] net: stmmac: move tx_lpi_timer tracking to
 phylib

On Fri, Dec 13, 2024 at 08:06:22PM +0000, Russell King (Oracle) wrote:
> On Fri, Dec 13, 2024 at 10:59:54AM +0000, Russell King (Oracle) wrote:
> > On Thu, Dec 12, 2024 at 02:46:33PM +0000, Russell King (Oracle) wrote:
> > > @@ -1092,6 +1092,7 @@ static void stmmac_mac_link_up(struct phylink_config *config,
> > >  			phy_init_eee(phy, !(priv->plat->flags &
> > >  				STMMAC_FLAG_RX_CLK_RUNS_IN_LPI)) >= 0;
> > >  		priv->eee_enabled = stmmac_eee_init(priv);
> > > +		priv->tx_lpi_timer = phy->eee_cfg.tx_lpi_timer;
> > >  		priv->tx_lpi_enabled = priv->eee_enabled;
> > >  		stmmac_set_eee_pls(priv, priv->hw, true);
> > >  	}
> > 
> > While looking deeper at stmmac, there's a bug in the above hunk -
> > stmmac_eee_init() makes use of priv->tx_lpi_timer, so this member
> > needs to be set before calling this function. I'll post a v2 shortly.
> 
> I'm going to hold off v2, there's a lot more that can be cleaned up
> here - the EEE code is rather horrid in stmmac, and there's definitely
> one race, and one logical error in it (e.g. why mark software EEE mode
> *enabled* when EEE mode is being disabled - which can lead to the EEE
> timer being added back onto the timer list.)
> 
> There's also weirdness with dwmac4's EEE register fiddling.
> 
> The stmmac driver uses hardware timed LPI entry if the timer is small
> enough to be programmed into hardware, otherwise it uses software mode.
> 
> When software mode wants to enter LPI mode, it sets both:
> 
> 	GMAC4_LPI_CTRL_STATUS_LPIEN (LPI enable)
> 	GMAC4_LPI_CTRL_STATUS_LPITXA (LPI TX Automate)
> 
> When software mode wants to exit LPI mode, it clears both of these
> two bits.
> 
> In hardware mode, when enabling LPI generation, we set the hardware LPI
> entry timer (separate register) to a non-zero value, and then set:
> 
> 	GMAC4_LPI_CTRL_STATUS_LPIEN (LPI enable)
> 	GMAC4_LPI_CTRL_STATUS_LPITXA (LPI TX Automate)
> 	GMAC4_LPI_CTRL_STATUS_LPIATE (LPI Timer enable)
> 
> That seems logical. However, in hardware mode, when we want to then
> disable hardware LPI generation, we set the hardware LPI entry timer to
> zero, the following bits:
> 
> 	GMAC4_LPI_CTRL_STATUS_LPIEN (LPI enable)
> 	GMAC4_LPI_CTRL_STATUS_LPITXA (LPI TX Automate)
> 
> and clear:
> 
> 	GMAC4_LPI_CTRL_STATUS_LPIATE (LPI Timer enable)
> 
> So, hardware mode, disabled, ends up setting the same bits as
> software mode wanting to generate LPI state on the transmit side.
> This makes no sense to me, and looks like another bug in this driver.

I've found a document that describes the GMAC:

https://www.st.com/resource/en/reference_manual/rm0441-stm32mp151-advanced-armbased-32bit-mpus-stmicroelectronics.pdf

Page 3302 gives the definitions for the ETH_MACLCSR, which is this
register.

LPITE (bit 20) - allows the ETH_MACLETR register to define how long to
wait before the TX path enters LPI. Requires LPITXA and LPIEN to both
be set. LPIEN is *not* automatically cleared when the TX path exits
LPI.

LPITXA (bit 19) - if this and LPIEN are set, then LPI is entered once
all outstanding packets have been transmitted, and exits when there's
a packet to be transmitted or the Tx FIFO is flushed. When it exits,
it clears the LPIEN bit (making this a one-shot LPI enter-exit.)

PLS (bit 17) needs to be programmed to reflect the PHY link status,
and is used to hold off LPI state for the LS timer.

LPIEN (bit 16) instructs the MAC to enter (when set) or exit (when
cleared) LPI state. It doesn't say what the behaviour of this bit is
if LPITXA is clear.

So:

LPIEN	LPITXA	LPITE	Effect
0	x	x	No LPI, or LPI exited if active
1	0	0	Undocumented
1	1	0	Tx LPI entered if PLS has been set for the LS
			timer, and once all packets have been sent.
			LPIEN clears when Tx LPI exits.
1	1	1	Tx LPI entered if PLS has been set for the LS
			timer, and transmitter has been idle for
			ETH_MACLETR. Exits do not clear LPIEN, allowing
			for subsequent idle periods to enter LPI.

Therefore, given this description, I believe the code to be wrong.
In the case where we've set LPIEN=1 LPITXA=1 LPITE=1, and we want
to exit/disable LPI, we should not be clearing LPIATE and leaving
LPIEN and LPITXA as they were. According to my reading, this would
cause LPI to remain active until there is a packet to be sent or the
TX FIFO is flushed. At that point, Tx LPI will be exited, causing
LPIEN to be cleared - but the code is wanting to disable Tx LPI
(e.g. because the user configured through ethtool for LPI to be
disabled.)

The question is... does this get fixed? Is there anyone who can test
this (beyond the "patch doesn't seem to cause regressions" but can
actually confirm entry/exit from LPI state?)

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