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

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

[Russell King (Oracle)]

When phy_ethtool_set_eee_noneg() detects a change in the LPI
parameters, it attempts to update phylib state and trigger the link
to cycle so the MAC sees the updated parameters.

However, in doing so, it sets phydev->enable_tx_lpi depending on
whether the EEE configuration allows the MAC to generate LPI without
taking into account the result of negotiation.

This can be demonstrated with a 1000base-T FD interface by:

 # ethtool --set-eee eno0 advertise 8	# cause EEE to be not negotiated
 # ethtool --set-eee eno0 tx-lpi off
 # ethtool --set-eee eno0 tx-lpi on

This results in being true, despite EEE not having been negotiated and:
 # ethtool --show-eee eno0
	EEE status: enabled - inactive
	Tx LPI: 250 (us)
	Supported EEE link modes:  100baseT/Full
	                           1000baseT/Full
	Advertised EEE link modes:  100baseT/Full
	Link partner advertised EEE link modes:  100baseT/Full
	                                         1000baseT/Full

Fix this by keeping track of whether EEE was negotiated via a new
eee_active member in struct phy_device, and include this state in
the decision whether phydev->enable_tx_lpi should be set.

Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
---
 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);
 	}
 }
 
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

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

[Andrew Lunn]

> 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;

This is missing a kdoc entry.

With that fixed, please add my Reviewed-by:

    Andrew

---
pw-bot: cr

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

[Heiner Kallweit]

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

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

[Heiner Kallweit]

On 22.11.2024 21:04, Heiner Kallweit wrote:
> 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.
> 

Merge of both changes should result in something like this:

static void phy_ethtool_set_eee_noneg(struct phy_device *phydev,
                                      const struct eee_config *old_cfg)
{
        bool enable_tx_lpi;

        if (!phydev->link)
                return;

        enable_tx_lpi = phydev->eee_cfg.tx_lpi_enabled && phydev->eee_active;

        if (enable_tx_lpi != old_cfg->tx_lpi_enabled ||
            phydev->eee_cfg.tx_lpi_timer != old_cfg->tx_lpi_timer) {
                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);
        }
}

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

[Russell King (Oracle)]

On Fri, Nov 22, 2024 at 09:28:04PM +0100, Heiner Kallweit wrote:
> On 22.11.2024 21:04, Heiner Kallweit wrote:
> > 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.
> 
> Merge of both changes should result in something like this:
> 
> static void phy_ethtool_set_eee_noneg(struct phy_device *phydev,
>                                       const struct eee_config *old_cfg)
> {
>         bool enable_tx_lpi;
> 
>         if (!phydev->link)
>                 return;
> 
>         enable_tx_lpi = phydev->eee_cfg.tx_lpi_enabled && phydev->eee_active;
> 
>         if (enable_tx_lpi != old_cfg->tx_lpi_enabled ||
>             phydev->eee_cfg.tx_lpi_timer != old_cfg->tx_lpi_timer) {
>                 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);
>         }
> }

Thanks for pointing this out - I've now merged your patch into my tree
and rebased on top. Your resolution isn't quite right - the if()
statement should be:

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

Since we only need to bounce the link if the LPI timer or the MAC LPI
enable state has changed. I'll send a replacement patch shortly.

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