[PATCH net] net: phylink: ensure PHY momentary link-fails are handled

[PATCH net] net: phylink: ensure PHY momentary link-fails are handled

[Russell King (Oracle)]

Normally, phylib won't notify changes in quick succession. However, as
a result of commit 3e43b903da04 ("net: phy: Immediately call
adjust_link if only tx_lpi_enabled changes") this is no longer true -
it is now possible that phy_link_down() and phy_link_up() will both
complete before phylink's resolver has run, which means it'll miss that
pl->phy_state.link momentarily became false.

Rename "mac_link_dropped" to be more generic "link_failed" since it will
cover more than the MAC/PCS end of the link failing, and arrange to set
this in phylink_phy_change() if we notice that the PHY reports that the
link is down.

This will ensure that we capture an EEE reconfiguration event.

Fixes: 3e43b903da04 ("net: phy: Immediately call adjust_link if only tx_lpi_enabled changes")
Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
---
 drivers/net/phy/phylink.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
index 4309317de3d1..3e9957b6aa14 100644
--- a/drivers/net/phy/phylink.c
+++ b/drivers/net/phy/phylink.c
@@ -78,7 +78,7 @@ struct phylink {
 	unsigned int pcs_neg_mode;
 	unsigned int pcs_state;
 
-	bool mac_link_dropped;
+	bool link_failed;
 	bool using_mac_select_pcs;
 
 	struct sfp_bus *sfp_bus;
@@ -1475,9 +1475,9 @@ static void phylink_resolve(struct work_struct *w)
 		cur_link_state = pl->old_link_state;
 
 	if (pl->phylink_disable_state) {
-		pl->mac_link_dropped = false;
+		pl->link_failed = false;
 		link_state.link = false;
-	} else if (pl->mac_link_dropped) {
+	} else if (pl->link_failed) {
 		link_state.link = false;
 		retrigger = true;
 	} else {
@@ -1572,7 +1572,7 @@ static void phylink_resolve(struct work_struct *w)
 			phylink_link_up(pl, link_state);
 	}
 	if (!link_state.link && retrigger) {
-		pl->mac_link_dropped = false;
+		pl->link_failed = false;
 		queue_work(system_power_efficient_wq, &pl->resolve);
 	}
 	mutex_unlock(&pl->state_mutex);
@@ -1835,6 +1835,8 @@ static void phylink_phy_change(struct phy_device *phydev, bool up)
 		pl->phy_state.pause |= MLO_PAUSE_RX;
 	pl->phy_state.interface = phydev->interface;
 	pl->phy_state.link = up;
+	if (!up)
+		pl->link_failed = true;
 	mutex_unlock(&pl->state_mutex);
 
 	phylink_run_resolve(pl);
@@ -2158,7 +2160,7 @@ EXPORT_SYMBOL_GPL(phylink_disconnect_phy);
 static void phylink_link_changed(struct phylink *pl, bool up, const char *what)
 {
 	if (!up)
-		pl->mac_link_dropped = true;
+		pl->link_failed = true;
 	phylink_run_resolve(pl);
 	phylink_dbg(pl, "%s link %s\n", what, up ? "up" : "down");
 }
@@ -2792,7 +2794,7 @@ int phylink_ethtool_set_pauseparam(struct phylink *pl,
 	 * link will cycle.
 	 */
 	if (manual_changed) {
-		pl->mac_link_dropped = true;
+		pl->link_failed = true;
 		phylink_run_resolve(pl);
 	}
 
-- 
2.30.2

Re: [PATCH net] net: phylink: ensure PHY momentary link-fails are handled

[Oleksij Rempel]

On Tue, Nov 12, 2024 at 04:20:00PM +0000, Russell King (Oracle) wrote:
> Normally, phylib won't notify changes in quick succession. However, as
> a result of commit 3e43b903da04 ("net: phy: Immediately call
> adjust_link if only tx_lpi_enabled changes") this is no longer true -
> it is now possible that phy_link_down() and phy_link_up() will both
> complete before phylink's resolver has run, which means it'll miss that
> pl->phy_state.link momentarily became false.
> 
> Rename "mac_link_dropped" to be more generic "link_failed" since it will
> cover more than the MAC/PCS end of the link failing, and arrange to set
> this in phylink_phy_change() if we notice that the PHY reports that the
> link is down.
> 
> This will ensure that we capture an EEE reconfiguration event.
> 
> Fixes: 3e43b903da04 ("net: phy: Immediately call adjust_link if only tx_lpi_enabled changes")
> Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>

Reviewed-by: Oleksij Rempel <o.rempel@pengutronix.de>

Thank you!

> ---
>  drivers/net/phy/phylink.c | 14 ++++++++------
>  1 file changed, 8 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
> index 4309317de3d1..3e9957b6aa14 100644
> --- a/drivers/net/phy/phylink.c
> +++ b/drivers/net/phy/phylink.c
> @@ -78,7 +78,7 @@ struct phylink {
>  	unsigned int pcs_neg_mode;
>  	unsigned int pcs_state;
>  
> -	bool mac_link_dropped;
> +	bool link_failed;
>  	bool using_mac_select_pcs;
>  
>  	struct sfp_bus *sfp_bus;
> @@ -1475,9 +1475,9 @@ static void phylink_resolve(struct work_struct *w)
>  		cur_link_state = pl->old_link_state;
>  
>  	if (pl->phylink_disable_state) {
> -		pl->mac_link_dropped = false;
> +		pl->link_failed = false;
>  		link_state.link = false;
> -	} else if (pl->mac_link_dropped) {
> +	} else if (pl->link_failed) {
>  		link_state.link = false;
>  		retrigger = true;
>  	} else {
> @@ -1572,7 +1572,7 @@ static void phylink_resolve(struct work_struct *w)
>  			phylink_link_up(pl, link_state);
>  	}
>  	if (!link_state.link && retrigger) {
> -		pl->mac_link_dropped = false;
> +		pl->link_failed = false;
>  		queue_work(system_power_efficient_wq, &pl->resolve);
>  	}
>  	mutex_unlock(&pl->state_mutex);
> @@ -1835,6 +1835,8 @@ static void phylink_phy_change(struct phy_device *phydev, bool up)
>  		pl->phy_state.pause |= MLO_PAUSE_RX;
>  	pl->phy_state.interface = phydev->interface;
>  	pl->phy_state.link = up;
> +	if (!up)
> +		pl->link_failed = true;
>  	mutex_unlock(&pl->state_mutex);
>  
>  	phylink_run_resolve(pl);
> @@ -2158,7 +2160,7 @@ EXPORT_SYMBOL_GPL(phylink_disconnect_phy);
>  static void phylink_link_changed(struct phylink *pl, bool up, const char *what)
>  {
>  	if (!up)
> -		pl->mac_link_dropped = true;
> +		pl->link_failed = true;
>  	phylink_run_resolve(pl);
>  	phylink_dbg(pl, "%s link %s\n", what, up ? "up" : "down");
>  }
> @@ -2792,7 +2794,7 @@ int phylink_ethtool_set_pauseparam(struct phylink *pl,
>  	 * link will cycle.
>  	 */
>  	if (manual_changed) {
> -		pl->mac_link_dropped = true;
> +		pl->link_failed = true;
>  		phylink_run_resolve(pl);
>  	}
>  
> -- 
> 2.30.2
> 
> 

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

Re: [PATCH net] net: phylink: ensure PHY momentary link-fails are handled

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to netdev/net.git (main)
by Jakub Kicinski <kuba@kernel.org>:

On Tue, 12 Nov 2024 16:20:00 +0000 you wrote:
> Normally, phylib won't notify changes in quick succession. However, as
> a result of commit 3e43b903da04 ("net: phy: Immediately call
> adjust_link if only tx_lpi_enabled changes") this is no longer true -
> it is now possible that phy_link_down() and phy_link_up() will both
> complete before phylink's resolver has run, which means it'll miss that
> pl->phy_state.link momentarily became false.
> 
> [...]

Here is the summary with links:
  - [net] net: phylink: ensure PHY momentary link-fails are handled
    https://git.kernel.org/netdev/net/c/671154f174e0

You are awesome, thank you!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html