netdev - Re: [PATCH RFC net-next 00/23] net: phylink managed EEE support

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 for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <Z0XZZszZFVbVl_kN@pengutronix.de>
Date: Tue, 26 Nov 2024 15:21:26 +0100
From: Oleksij Rempel <o.rempel@...gutronix.de>
To: "Russell King (Oracle)" <linux@...linux.org.uk>
Cc: Andrew Lunn <andrew@...n.ch>, Heiner Kallweit <hkallweit1@...il.com>,
	Alexandre Torgue <alexandre.torgue@...s.st.com>,
	Andrew Lunn <andrew+netdev@...n.ch>,
	Bryan Whitehead <bryan.whitehead@...rochip.com>,
	"David S. Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>,
	Florian Fainelli <florian.fainelli@...adcom.com>,
	Jakub Kicinski <kuba@...nel.org>, Jose Abreu <joabreu@...opsys.com>,
	linux-arm-kernel@...ts.infradead.org,
	linux-stm32@...md-mailman.stormreply.com,
	Marcin Wojtas <marcin.s.wojtas@...il.com>,
	Maxime Coquelin <mcoquelin.stm32@...il.com>, netdev@...r.kernel.org,
	Paolo Abeni <pabeni@...hat.com>, UNGLinuxDriver@...rochip.com
Subject: Re: [PATCH RFC net-next 00/23] net: phylink managed EEE support

Hi Russell,

On Tue, Nov 26, 2024 at 01:01:11PM +0000, Russell King (Oracle) wrote:
> On Tue, Nov 26, 2024 at 12:51:36PM +0000, Russell King (Oracle) wrote:
> > Patch 11 adds phylink managed EEE support. Two new MAC APIs are added,
> > to enable and disable LPI. The enable method is passed the LPI timer
> > setting which it is expected to program into the hardware, and also a
> > flag ehther the transmit clock should be stopped.
> > 
> >  *** There are open questions here. Eagle eyed reviewers will notice
> >    pl->config->lpi_interfaces. There are MACs out there which only
> >    support LPI signalling on a subset of their interface types. Phylib
> >    doesn't understand this. I'm handling this at the moment by simply
> >    not activating LPI at the MAC, but that leads to ethtool --show-eee
> >    suggesting that EEE is active when it isn't.
> >  *** Should we pass the phy_interface_t to these functions?
> >  *** Should mac_enable_tx_lpi() be allowed to fail if the MAC doesn't
> >    support the interface mode?
> 
> There is another point to raise here - should we have a "validate_eee"
> method in struct phylink_mac_ops so that MAC drivers can validate
> settings such as the tx_lpi_timer value can be programmed into the
> hardware?
> 
> We do have the situation on Marvell platforms where the programmed
> value depends on the MAC speed, and is only 8 bit, which makes
> validating its value rather difficult - at 1G speeds, it's a
> resolution of 1us so we can support up to 255us. At 100M speeds,
> it's 10us, supporting up to 2.55ms. This makes it awkward to be able
> to validate the set_eee() settings are sane for the hardware. Should
> Marvell platforms instead implement a hrtimer above this? That sounds
> a bit problematical to manage sanely.

I agree that tx_lpi_timer can be a problem, and this is not just a
Marvell issue.  For example, I think the FEC MAC on i.MX8MP might also
be affected.  But I can't confirm this because I don't have an i.MX8MP
board with a PHY that supports MAC-controlled EEE mode. The Realtek PHY
I have uses PHY-controlled EEE (SmartEEE).

Except for this, I think there should be sane default values for
tx_lpi_timer.  The IEEE 802.3-2022 standard (Section 78.2) describes EEE
timing, but it doesn’t give a clear recommendation for tx_lpi_timer.
IMO, the best value for tx_lpi_timer should be the sum of the time
needed to put the full chain (MAC -> PHY -> remote PHY) into sleep mode
and the time needed to wake the chain. These times are link-speed
specific, so defaults should consider PHY timings for each link speed.

Except for tx_lpi_timer, some MACs also allow configuration of the Twake
timer.  For example, the FEC driver uses the lpi_timer value to
configure the Twake timer, and the LAN78xx driver also provides a
configurable Twake timer register.

If the Twake timer is not configured properly, or if the system has
quirks causing the actual Twake time to be longer than expected, it can
result in frame corruption. 

Regards,
Oleksij
-- 
Pengutronix e.K.                           |                             |
Steuerwalder Str. 21                       | http://www.pengutronix.de/  |
31137 Hildesheim, Germany                  | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |