[PATCH RFC net-next 02/23] net: phy: fix phy_ethtool_set_eee() incorrectly enabling LPI

[Russell King (Oracle)]

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!