Re: [PATCH net v2] net: phy: fix phy_ethtool_set_eee() incorrectly enabling LPI

[Heiner Kallweit <hkallweit1@gmail.com>]

On 22.11.2024 13:22, Russell King (Oracle) wrote:
> ---
>  drivers/net/phy/phy-c45.c |  2 +-
>  drivers/net/phy/phy.c     | 32 ++++++++++++++++++--------------
>  include/linux/phy.h       |  1 +
>  3 files changed, 20 insertions(+), 15 deletions(-)
> 
> diff --git a/drivers/net/phy/phy-c45.c b/drivers/net/phy/phy-c45.c
> index 96d0b3a5a9d3..944ae98ad110 100644
> --- a/drivers/net/phy/phy-c45.c
> +++ b/drivers/net/phy/phy-c45.c
> @@ -1530,7 +1530,7 @@ int genphy_c45_ethtool_get_eee(struct phy_device *phydev,
>  		return ret;
>  
>  	data->eee_enabled = is_enabled;
> -	data->eee_active = ret;
> +	data->eee_active = phydev->eee_active;
>  	linkmode_copy(data->supported, phydev->supported_eee);
>  
>  	return 0;
> diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
> index 4f3e742907cb..e174107b96e2 100644
> --- a/drivers/net/phy/phy.c
> +++ b/drivers/net/phy/phy.c
> @@ -990,14 +990,14 @@ static int phy_check_link_status(struct phy_device *phydev)
>  		phydev->state = PHY_RUNNING;
>  		err = genphy_c45_eee_is_active(phydev,
>  					       NULL, NULL, NULL);
> -		if (err <= 0)
> -			phydev->enable_tx_lpi = false;
> -		else
> -			phydev->enable_tx_lpi = phydev->eee_cfg.tx_lpi_enabled;
> +		phydev->eee_active = err > 0;
> +		phydev->enable_tx_lpi = phydev->eee_cfg.tx_lpi_enabled &&
> +					phydev->eee_active;
>  
>  		phy_link_up(phydev);
>  	} else if (!phydev->link && phydev->state != PHY_NOLINK) {
>  		phydev->state = PHY_NOLINK;
> +		phydev->eee_active = false;
>  		phydev->enable_tx_lpi = false;
>  		phy_link_down(phydev);
>  	}
> @@ -1685,16 +1685,20 @@ EXPORT_SYMBOL(phy_ethtool_get_eee);
>  static void phy_ethtool_set_eee_noneg(struct phy_device *phydev,
>  				      struct ethtool_keee *data)
>  {
> -	if (phydev->eee_cfg.tx_lpi_enabled != data->tx_lpi_enabled ||
> -	    phydev->eee_cfg.tx_lpi_timer != data->tx_lpi_timer) {
> -		eee_to_eeecfg(&phydev->eee_cfg, data);
> -		phydev->enable_tx_lpi = eeecfg_mac_can_tx_lpi(&phydev->eee_cfg);
> -		if (phydev->link) {
> -			phydev->link = false;
> -			phy_link_down(phydev);
> -			phydev->link = true;
> -			phy_link_up(phydev);
> -		}
> +	bool enable_tx_lpi = data->tx_lpi_enabled &&
> +			     phydev->eee_active;
> +
> +	eee_to_eeecfg(&phydev->eee_cfg, data);
> +
> +	if ((phydev->enable_tx_lpi != enable_tx_lpi ||
> +	     phydev->eee_cfg.tx_lpi_timer != data->tx_lpi_timer) &&
> +	    phydev->link) {
> +		phydev->enable_tx_lpi = false;
> +		phydev->link = false;
> +		phy_link_down(phydev);
> +		phydev->enable_tx_lpi = enable_tx_lpi;
> +		phydev->link = true;
> +		phy_link_up(phydev);

This part collides with a pending patch:
https://patchwork.kernel.org/project/netdevbpf/patch/a5efc274-ce58-49f3-ac8a-5384d9b41695@gmail.com/
I think you have to rebase and resubmit once the pending patch has been applied.

>  	}
>  }
>  
> diff --git a/include/linux/phy.h b/include/linux/phy.h
> index 77c6d6451638..6a17cc05f876 100644
> --- a/include/linux/phy.h
> +++ b/include/linux/phy.h
> @@ -723,6 +723,7 @@ struct phy_device {
>  	/* Energy efficient ethernet modes which should be prohibited */
>  	__ETHTOOL_DECLARE_LINK_MODE_MASK(eee_broken_modes);
>  	bool enable_tx_lpi;
> +	bool eee_active;
>  	struct eee_config eee_cfg;
>  
>  	/* Host supported PHY interface types. Should be ignored if empty. */
> -- 2.30.2