netdev - Re: [PATCH RFC net-next 02/23] net: phy: fix phy_ethtool_set

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: <Z0b-nJ7bt8IlBMpz@shell.armlinux.org.uk>
Date: Wed, 27 Nov 2024 11:12:28 +0000
From: "Russell King (Oracle)" <linux@...linux.org.uk>
To: Andrew Lunn <andrew@...n.ch>, Heiner Kallweit <hkallweit1@...il.com>
Cc: 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,
	Oleksij Rempel <o.rempel@...gutronix.de>,
	Paolo Abeni <pabeni@...hat.com>, UNGLinuxDriver@...rochip.com
Subject: Re: [PATCH RFC net-next 02/23] net: phy: fix phy_ethtool_set_eee()
 incorrectly enabling LPI

On Tue, Nov 26, 2024 at 12:52:21PM +0000, Russell King (Oracle) wrote:
> @@ -1685,15 +1685,21 @@ EXPORT_SYMBOL(phy_ethtool_get_eee);
>  static void phy_ethtool_set_eee_noneg(struct phy_device *phydev,
>  				      const struct eee_config *old_cfg)
>  {
> -	if (phydev->eee_cfg.tx_lpi_enabled != old_cfg->tx_lpi_enabled ||
> +	bool enable_tx_lpi;
> +
> +	if (!phydev->link)
> +		return;
> +
> +	enable_tx_lpi = phydev->eee_cfg.tx_lpi_enabled && phydev->eee_active;
> +
> +	if (phydev->enable_tx_lpi != enable_tx_lpi ||
>  	    phydev->eee_cfg.tx_lpi_timer != old_cfg->tx_lpi_timer) {

I'm wondering whether this should be:

	if (phydev->enable_tx_lpi != enable_tx_lpi ||
	    (phydev->enable_tx_lpi &&
	     phydev->eee_cfg.tx_lpi_timer != old_cfg->tx_lpi_timer)) {

The argument for this change would be to avoid cycling the link when the
LPI timer changes but we're not using LPI.

The argument against this change would be that then we don't program the
hardware, and if the driver reads the initial value from hardware and
is unbound/rebound, we'll lose that update whereas before the phylib
changes, it would have been preserved.

The problem, however, are drivers where the LPI timer is dependent on
the speed.

Any thoughts?

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