Re: [PATCH net-next 2/7] net: stmmac: move tx_lpi_timer tracking to phylib

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

On Thu, Jan 02, 2025 at 10:02:35PM +0000, Russell King (Oracle) wrote:
> 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?)

Okay, given the lack of engagement on this patch set, I'm going to
submit it in its latest form anyway (with a few more patches of the
same ilk) so we can at least move forward cleaning up the code.

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