Skip to contentSkip to conversation

net: phylink: phylink_resolve() cleanups

PATCH Applied Russell King (Oracle) 4 participants 9 messages Nov 8, 2024 – Nov 12, 2024

Subsystems (4)
Networking Network drivers Ethernet PHY SFF/SFP/SFP+ module support
lore
Share & tools
lore

Russell King (Oracle)

author maintainer
#0 lore raw

cover net: phylink: phylink_resolve() cleanups

Hi,

This series does a bit of clean-up in phylink_resolve() to make the code a little easier to follow.

Patch 1 moves the manual flow control setting in two of the switch cases to after the switch().

Patch 2 changes the MLO_AN_FIXED case to be a simple if() statement, reducing its indentation.

Patch 3 changes the MLO_AN_PHY case to also be a simple if() statment, also reducing its indentation.

Patch 4 does the same for the last case.

Patch 5 reformats the code and comments for the reduced indentation, making it easier to read.

 drivers/net/phy/phylink.c | 106 +++++++++++++++++++++-------------------------
 1 file changed, 48 insertions(+), 58 deletions(-)
Signature · 2 lines
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
#1 lore raw

PATCH 1/5 net: phylink: move manual flow control setting

Move the handling of manual flow control configuration to a common location during resolve. We currently evaluate this for all but fixed links.

Signed-off-by Russell King (Oracle) <[email protected]>
---
 drivers/net/phy/phylink.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)
drivers/net/phy/phylink.c +3 -2
index 6ca7ea970f51..65e81ef2225d 100644
@@ -1467,7 +1467,6 @@ static void phylink_resolve(struct work_struct *w)
 		switch (pl->cur_link_an_mode) {
 		case MLO_AN_PHY:
 			link_state = pl->phy_state;
-			phylink_apply_manual_flow(pl, &link_state);
 			mac_config = link_state.link;
 			break;
 

@@ -1528,11 +1527,13 @@ static void phylink_resolve(struct work_struct *w)
 				link_state.pause = pl->phy_state.pause;
 				mac_config = true;
 			}
-			phylink_apply_manual_flow(pl, &link_state);
 			break;
 		}
 	}
 
+	if (pl->cur_link_an_mode != MLO_AN_FIXED)
+		phylink_apply_manual_flow(pl, &link_state);
+
 	if (mac_config) {
 		if (link_state.interface != pl->link_config.interface) {
 			/* The interface has changed, force the link down and
Signature · 1 line
2.30.2
#4 lore raw

PATCH 2/5 net: phylink: move MLO_AN_FIXED resolve handling to if() statement

In reply to Russell King (Oracle)

The switch() statement doesn't sit very well with the preceeding if() statements, and results in excessive indentation that spoils code readability. Begin cleaning this up by converting the MLO_AN_FIXED case to an if() statement.

Signed-off-by Russell King (Oracle) <[email protected]>
---
 drivers/net/phy/phylink.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)
drivers/net/phy/phylink.c +3 -5
index 65e81ef2225d..bb20ae5674e5 100644
@@ -1463,6 +1463,9 @@ static void phylink_resolve(struct work_struct *w)
 	} else if (pl->mac_link_dropped) {
 		link_state.link = false;
 		retrigger = true;
+	} else if (pl->cur_link_an_mode == MLO_AN_FIXED) {
+		phylink_get_fixed_state(pl, &link_state);
+		mac_config = link_state.link;
 	} else {
 		switch (pl->cur_link_an_mode) {
 		case MLO_AN_PHY:

@@ -1470,11 +1473,6 @@ static void phylink_resolve(struct work_struct *w)
 			mac_config = link_state.link;
 			break;
 
-		case MLO_AN_FIXED:
-			phylink_get_fixed_state(pl, &link_state);
-			mac_config = link_state.link;
-			break;
-
 		case MLO_AN_INBAND:
 			phylink_mac_pcs_get_state(pl, &link_state);
Signature · 1 line
2.30.2
#5 lore raw

PATCH 3/5 net: phylink: move MLO_AN_PHY resolve handling to if() statement

In reply to Russell King (Oracle)

The switch() statement doesn't sit very well with the preceeding if() statements, and results in excessive indentation that spoils code readability. Continue cleaning this up by converting the MLO_AN_PHY case to use an if() statmeent.

Signed-off-by Russell King (Oracle) <[email protected]>
---
 drivers/net/phy/phylink.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)
drivers/net/phy/phylink.c +3 -5
index bb20ae5674e5..3af6368a9fbf 100644
@@ -1466,13 +1466,11 @@ static void phylink_resolve(struct work_struct *w)
 	} else if (pl->cur_link_an_mode == MLO_AN_FIXED) {
 		phylink_get_fixed_state(pl, &link_state);
 		mac_config = link_state.link;
+	} else if (pl->cur_link_an_mode == MLO_AN_PHY) {
+		link_state = pl->phy_state;
+		mac_config = link_state.link;
 	} else {
 		switch (pl->cur_link_an_mode) {
-		case MLO_AN_PHY:
-			link_state = pl->phy_state;
-			mac_config = link_state.link;
-			break;
-
 		case MLO_AN_INBAND:
 			phylink_mac_pcs_get_state(pl, &link_state);
Signature · 1 line
2.30.2
#6 lore raw

PATCH 4/5 net: phylink: remove switch() statement in resolve handling

In reply to Russell King (Oracle)

The switch() statement doesn't sit very well with the preceeding if() statements, so let's just convert everything to if()s. As a result of the two preceding commits, there is now only one case in the switch() statement. Remove the switch statement and reduce the code indentation. Code reformatting will be in the following commit.

Signed-off-by Russell King (Oracle) <[email protected]>
---
 drivers/net/phy/phylink.c | 94 +++++++++++++++++++--------------------
 1 file changed, 45 insertions(+), 49 deletions(-)
drivers/net/phy/phylink.c +45 -49
index 3af6368a9fbf..aaeb8b11e758 100644
@@ -1470,60 +1470,56 @@ static void phylink_resolve(struct work_struct *w)
 		link_state = pl->phy_state;
 		mac_config = link_state.link;
 	} else {
-		switch (pl->cur_link_an_mode) {
-		case MLO_AN_INBAND:
-			phylink_mac_pcs_get_state(pl, &link_state);
-
-			/* The PCS may have a latching link-fail indicator.
-			 * If the link was up, bring the link down and
-			 * re-trigger the resolve. Otherwise, re-read the
-			 * PCS state to get the current status of the link.
+		phylink_mac_pcs_get_state(pl, &link_state);
+
+		/* The PCS may have a latching link-fail indicator.
+		 * If the link was up, bring the link down and
+		 * re-trigger the resolve. Otherwise, re-read the
+		 * PCS state to get the current status of the link.
+		 */
+		if (!link_state.link) {
+			if (cur_link_state)
+				retrigger = true;
+			else
+				phylink_mac_pcs_get_state(pl,
+							  &link_state);
+		}
+
+		/* If we have a phy, the "up" state is the union of
+		 * both the PHY and the MAC
+		 */
+		if (pl->phydev)
+			link_state.link &= pl->phy_state.link;
+
+		/* Only update if the PHY link is up */
+		if (pl->phydev && pl->phy_state.link) {
+			/* If the interface has changed, force a
+			 * link down event if the link isn't already
+			 * down, and re-resolve.
 			 */
-			if (!link_state.link) {
-				if (cur_link_state)
-					retrigger = true;
-				else
-					phylink_mac_pcs_get_state(pl,
-								  &link_state);
+			if (link_state.interface !=
+			    pl->phy_state.interface) {
+				retrigger = true;
+				link_state.link = false;
 			}
+			link_state.interface = pl->phy_state.interface;
 
-			/* If we have a phy, the "up" state is the union of
-			 * both the PHY and the MAC
+			/* If we are doing rate matching, then the
+			 * link speed/duplex comes from the PHY
 			 */
-			if (pl->phydev)
-				link_state.link &= pl->phy_state.link;
-
-			/* Only update if the PHY link is up */
-			if (pl->phydev && pl->phy_state.link) {
-				/* If the interface has changed, force a
-				 * link down event if the link isn't already
-				 * down, and re-resolve.
-				 */
-				if (link_state.interface !=
-				    pl->phy_state.interface) {
-					retrigger = true;
-					link_state.link = false;
-				}
-				link_state.interface = pl->phy_state.interface;
-
-				/* If we are doing rate matching, then the
-				 * link speed/duplex comes from the PHY
-				 */
-				if (pl->phy_state.rate_matching) {
-					link_state.rate_matching =
-						pl->phy_state.rate_matching;
-					link_state.speed = pl->phy_state.speed;
-					link_state.duplex =
-						pl->phy_state.duplex;
-				}
-
-				/* If we have a PHY, we need to update with
-				 * the PHY flow control bits.
-				 */
-				link_state.pause = pl->phy_state.pause;
-				mac_config = true;
+			if (pl->phy_state.rate_matching) {
+				link_state.rate_matching =
+					pl->phy_state.rate_matching;
+				link_state.speed = pl->phy_state.speed;
+				link_state.duplex =
+					pl->phy_state.duplex;
 			}
-			break;
+
+			/* If we have a PHY, we need to update with
+			 * the PHY flow control bits.
+			 */
+			link_state.pause = pl->phy_state.pause;
+			mac_config = true;
 		}
 	}
Signature · 1 line
2.30.2
#7 lore raw

PATCH 5/5 net: phylink: clean up phylink_resolve()

In reply to Russell King (Oracle)

Now that we have reduced the indentation level, clean up the code formatting.

Signed-off-by Russell King (Oracle) <[email protected]>
---
 drivers/net/phy/phylink.c | 35 ++++++++++++++++-------------------
 1 file changed, 16 insertions(+), 19 deletions(-)
drivers/net/phy/phylink.c +16 -19
index aaeb8b11e758..b1e828a4286d 100644
@@ -1472,51 +1472,48 @@ static void phylink_resolve(struct work_struct *w)
 	} else {
 		phylink_mac_pcs_get_state(pl, &link_state);
 
-		/* The PCS may have a latching link-fail indicator.
-		 * If the link was up, bring the link down and
-		 * re-trigger the resolve. Otherwise, re-read the
-		 * PCS state to get the current status of the link.
+		/* The PCS may have a latching link-fail indicator. If the link
+		 * was up, bring the link down and re-trigger the resolve.
+		 * Otherwise, re-read the PCS state to get the current status
+		 * of the link.
 		 */
 		if (!link_state.link) {
 			if (cur_link_state)
 				retrigger = true;
 			else
-				phylink_mac_pcs_get_state(pl,
-							  &link_state);
+				phylink_mac_pcs_get_state(pl, &link_state);
 		}
 
-		/* If we have a phy, the "up" state is the union of
-		 * both the PHY and the MAC
+		/* If we have a phy, the "up" state is the union of both the
+		 * PHY and the MAC
 		 */
 		if (pl->phydev)
 			link_state.link &= pl->phy_state.link;
 
 		/* Only update if the PHY link is up */
 		if (pl->phydev && pl->phy_state.link) {
-			/* If the interface has changed, force a
-			 * link down event if the link isn't already
-			 * down, and re-resolve.
+			/* If the interface has changed, force a link down
+			 * event if the link isn't already down, and re-resolve.
 			 */
-			if (link_state.interface !=
-			    pl->phy_state.interface) {
+			if (link_state.interface != pl->phy_state.interface) {
 				retrigger = true;
 				link_state.link = false;
 			}
+
 			link_state.interface = pl->phy_state.interface;
 
-			/* If we are doing rate matching, then the
-			 * link speed/duplex comes from the PHY
+			/* If we are doing rate matching, then the link
+			 * speed/duplex comes from the PHY
 			 */
 			if (pl->phy_state.rate_matching) {
 				link_state.rate_matching =
 					pl->phy_state.rate_matching;
 				link_state.speed = pl->phy_state.speed;
-				link_state.duplex =
-					pl->phy_state.duplex;
+				link_state.duplex = pl->phy_state.duplex;
 			}
 
-			/* If we have a PHY, we need to update with
-			 * the PHY flow control bits.
+			/* If we have a PHY, we need to update with the PHY
+			 * flow control bits.
 			 */
 			link_state.pause = pl->phy_state.pause;
 			mac_config = true;
Signature · 1 line
2.30.2
maintainer
#2 lore raw

Re: [PATCH net-next 1/5] net: phylink: move manual flow control setting

In reply to Russell King (Oracle)
Quoted 3 lines · Russell King (Oracle)

Move the handling of manual flow control configuration to a common location during resolve. We currently evaluate this for all but fixed links.

I could be mis-remembering, but i thought somebody recently mentioned they wanted manual control for flow control for fixed links.

Nothing comes to mind why it should not work. With manual control, it is purely a MAC problem. There is no negotiation involved which would require a PHY, which is missing in the case of fixed links.

Am i missing something? Other than you want to keep the current behaviour.

Andrew

author maintainer
#3 lore raw

Re: [PATCH net-next 1/5] net: phylink: move manual flow control setting

Quoted 7 lines · Andrew Lunn
Quoted 3 lines · Russell King (Oracle)

Move the handling of manual flow control configuration to a common location during resolve. We currently evaluate this for all but fixed links.

I could be mis-remembering, but i thought somebody recently mentioned they wanted manual control for flow control for fixed links.

I haven't seen that request, but I do have a patch prepared in case someone does want it.

Quoted 2 lines

Am i missing something? Other than you want to keep the current behaviour.

Indeed - this is a cleanup series, not a functional change series.

Signature · 2 lines
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 80Mbps down 10Mbps up. Decent connectivity at last!
CI Applied
#8 lore raw
In reply to Russell King (Oracle)
This series was applied to netdev/net-next.git (main) by Jakub Kicinski <[email protected]>:

Hello:

This series was applied to netdev/net-next.git (main) by Jakub Kicinski <[email protected]>:

Quoted 9 lines · On Fri, 8 Nov 2024 16:01:26 +0000 you wrote:

Hi,

This series does a bit of clean-up in phylink_resolve() to make the code a little easier to follow.

Patch 1 moves the manual flow control setting in two of the switch cases to after the switch().

[...]

Here is the summary with links:
- [net-next,1/5] net: phylink: move manual flow control setting

    https://git.kernel.org/netdev/net-next/c/8cc5f4cb94c0
  - [net-next,2/5] net: phylink: move MLO_AN_FIXED resolve handling to if() statement
    https://git.kernel.org/netdev/net-next/c/92abfcb4ced4
  - [net-next,3/5] net: phylink: move MLO_AN_PHY resolve handling to if() statement
    https://git.kernel.org/netdev/net-next/c/f0f46c2a3d8e
  - [net-next,4/5] net: phylink: remove switch() statement in resolve handling
    https://git.kernel.org/netdev/net-next/c/d1a16dbbd84e
  - [net-next,5/5] net: phylink: clean up phylink_resolve()
    https://git.kernel.org/netdev/net-next/c/bc08ce37d99a

You are awesome, thank you!

Signature · 2 lines
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html