[PATCH net-next 0/5] net: phylink: phylink_resolve() cleanups

[PATCH net-next 0/5] net: phylink: phylink_resolve() cleanups

[Russell King (Oracle)]

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

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

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

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

Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
---
 drivers/net/phy/phylink.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
index 6ca7ea970f51..65e81ef2225d 100644
--- a/drivers/net/phy/phylink.c
+++ b/drivers/net/phy/phylink.c
@@ -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
-- 
2.30.2

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

[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) <rmk+kernel@armlinux.org.uk>
---
 drivers/net/phy/phylink.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
index 65e81ef2225d..bb20ae5674e5 100644
--- a/drivers/net/phy/phylink.c
+++ b/drivers/net/phy/phylink.c
@@ -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);
 
-- 
2.30.2

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

[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) <rmk+kernel@armlinux.org.uk>
---
 drivers/net/phy/phylink.c | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
index bb20ae5674e5..3af6368a9fbf 100644
--- a/drivers/net/phy/phylink.c
+++ b/drivers/net/phy/phylink.c
@@ -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);
 
-- 
2.30.2

[PATCH net-next 4/5] net: phylink: remove switch() statement in resolve handling

[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) <rmk+kernel@armlinux.org.uk>
---
 drivers/net/phy/phylink.c | 94 +++++++++++++++++++--------------------
 1 file changed, 45 insertions(+), 49 deletions(-)

diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
index 3af6368a9fbf..aaeb8b11e758 100644
--- a/drivers/net/phy/phylink.c
+++ b/drivers/net/phy/phylink.c
@@ -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;
 		}
 	}
 
-- 
2.30.2

[PATCH net-next 5/5] net: phylink: clean up phylink_resolve()

[Russell King (Oracle)]

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

Signed-off-by: Russell King (Oracle) <rmk+kernel@armlinux.org.uk>
---
 drivers/net/phy/phylink.c | 35 ++++++++++++++++-------------------
 1 file changed, 16 insertions(+), 19 deletions(-)

diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
index aaeb8b11e758..b1e828a4286d 100644
--- a/drivers/net/phy/phylink.c
+++ b/drivers/net/phy/phylink.c
@@ -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;
-- 
2.30.2

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

[Andrew Lunn]

On Fri, Nov 08, 2024 at 04:01:44PM +0000, Russell King (Oracle) wrote:
> 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

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

[Russell King (Oracle)]

On Fri, Nov 08, 2024 at 06:34:04PM +0100, Andrew Lunn wrote:
> On Fri, Nov 08, 2024 at 04:01:44PM +0000, Russell King (Oracle) wrote:
> > 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.

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

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

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

Re: [PATCH net-next 0/5] net: phylink: phylink_resolve() cleanups

[patchwork-bot+netdevbpf]

Hello:

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

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!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html