[PATCH v2 net-next] net: dsa: microchip: ksz9477: split half-duplex monitoring function

[PATCH v2 net-next] net: dsa: microchip: ksz9477: split half-duplex monitoring function

[Enguerrand de Ribaucourt]

In order to respect the 80 columns limit, split the half-duplex
monitoring function in two.

This is just a styling change, no functional change.

Signed-off-by: Enguerrand de Ribaucourt <enguerrand.de-ribaucourt@savoirfairelinux.com>

---
v2:
 - added line breaks after return statements
 - removed Fixed-by: tag
v1: https://lore.kernel.org/netdev/20240708084934.131175-1-enguerrand.de-ribaucourt@savoirfairelinux.com/
---
 drivers/net/dsa/microchip/ksz9477.c | 91 +++++++++++++++++------------
 1 file changed, 54 insertions(+), 37 deletions(-)

diff --git a/drivers/net/dsa/microchip/ksz9477.c b/drivers/net/dsa/microchip/ksz9477.c
index 425e20daf1e9..1e2293aa00dc 100644
--- a/drivers/net/dsa/microchip/ksz9477.c
+++ b/drivers/net/dsa/microchip/ksz9477.c
@@ -427,54 +427,71 @@ void ksz9477_freeze_mib(struct ksz_device *dev, int port, bool freeze)
 	mutex_unlock(&p->mib.cnt_mutex);
 }
 
-int ksz9477_errata_monitor(struct ksz_device *dev, int port,
-			   u64 tx_late_col)
+static int ksz9477_half_duplex_monitor(struct ksz_device *dev, int port,
+				       u64 tx_late_col)
 {
+	u8 lue_ctrl;
 	u32 pmavbc;
-	u8 status;
 	u16 pqm;
 	int ret;
 
-	ret = ksz_pread8(dev, port, REG_PORT_STATUS_0, &status);
+	/* Errata DS80000754 recommends monitoring potential faults in
+	 * half-duplex mode. The switch might not be able to communicate anymore
+	 * in these states. If you see this message, please read the
+	 * errata-sheet for more information:
+	 * https://ww1.microchip.com/downloads/aemDocuments/documents
+	 * /UNG/ProductDocuments/Errata/KSZ9477S-Errata-DS80000754.pdf
+	 * To workaround this issue, half-duplex mode should be avoided.
+	 * A software reset could be implemented to recover from this state.
+	 */
+	dev_warn_once(dev->dev,
+		      "Half-duplex detected on port %d, transmission halt may occur\n",
+		      port);
+	if (tx_late_col != 0) {
+		/* Transmission halt with late collisions */
+		dev_crit_once(dev->dev,
+			      "TX late collisions detected, transmission may be halted on port %d\n",
+			      port);
+	}
+	ret = ksz_read8(dev, REG_SW_LUE_CTRL_0, &lue_ctrl);
 	if (ret)
 		return ret;
-	if (!(FIELD_GET(PORT_INTF_SPEED_MASK, status) == PORT_INTF_SPEED_NONE) &&
-	    !(status & PORT_INTF_FULL_DUPLEX)) {
-		/* Errata DS80000754 recommends monitoring potential faults in
-		 * half-duplex mode. The switch might not be able to communicate anymore
-		 * in these states.
-		 * If you see this message, please read the errata-sheet for more information:
-		 * https://ww1.microchip.com/downloads/aemDocuments/documents/UNG/ProductDocuments/Errata/KSZ9477S-Errata-DS80000754.pdf
-		 * To workaround this issue, half-duplex mode should be avoided.
-		 * A software reset could be implemented to recover from this state.
-		 */
-		dev_warn_once(dev->dev,
-			      "Half-duplex detected on port %d, transmission halt may occur\n",
-			      port);
-		if (tx_late_col != 0) {
-			/* Transmission halt with late collisions */
-			dev_crit_once(dev->dev,
-				      "TX late collisions detected, transmission may be halted on port %d\n",
-				      port);
-		}
-		ret = ksz_read8(dev, REG_SW_LUE_CTRL_0, &status);
+	if (lue_ctrl & SW_VLAN_ENABLE) {
+		ret = ksz_pread16(dev, port, REG_PORT_QM_TX_CNT_0__4, &pqm);
 		if (ret)
 			return ret;
-		if (status & SW_VLAN_ENABLE) {
-			ret = ksz_pread16(dev, port, REG_PORT_QM_TX_CNT_0__4, &pqm);
-			if (ret)
-				return ret;
-			ret = ksz_read32(dev, REG_PMAVBC, &pmavbc);
-			if (ret)
-				return ret;
-			if ((FIELD_GET(PMAVBC_MASK, pmavbc) <= PMAVBC_MIN) ||
-			    (FIELD_GET(PORT_QM_TX_CNT_M, pqm) >= PORT_QM_TX_CNT_MAX)) {
-				/* Transmission halt with Half-Duplex and VLAN */
-				dev_crit_once(dev->dev,
-					      "resources out of limits, transmission may be halted\n");
-			}
+
+		ret = ksz_read32(dev, REG_PMAVBC, &pmavbc);
+		if (ret)
+			return ret;
+
+		if ((FIELD_GET(PMAVBC_MASK, pmavbc) <= PMAVBC_MIN) ||
+		    (FIELD_GET(PORT_QM_TX_CNT_M, pqm) >= PORT_QM_TX_CNT_MAX)) {
+			/* Transmission halt with Half-Duplex and VLAN */
+			dev_crit_once(dev->dev,
+				      "resources out of limits, transmission may be halted\n");
 		}
 	}
+
+	return ret;
+}
+
+int ksz9477_errata_monitor(struct ksz_device *dev, int port,
+			   u64 tx_late_col)
+{
+	u8 status;
+	int ret;
+
+	ret = ksz_pread8(dev, port, REG_PORT_STATUS_0, &status);
+	if (ret)
+		return ret;
+
+	if (!(FIELD_GET(PORT_INTF_SPEED_MASK, status)
+	      == PORT_INTF_SPEED_NONE) &&
+	    !(status & PORT_INTF_FULL_DUPLEX)) {
+		ret = ksz9477_half_duplex_monitor(dev, port, tx_late_col);
+	}
+
 	return ret;
 }
 
-- 
2.34.1

Re: [PATCH v2 net-next] net: dsa: microchip: ksz9477: split half-duplex monitoring function

[Arun.Ramadoss]

On Thu, 2024-08-08 at 15:14 +0000, Enguerrand de Ribaucourt wrote:
> EXTERNAL EMAIL: Do not click links or open attachments unless you
> know the content is safe
> 
> In order to respect the 80 columns limit, split the half-duplex
> monitoring function in two.
> 
> This is just a styling change, no functional change.
> 
> Signed-off-by: Enguerrand de Ribaucourt <
> enguerrand.de-ribaucourt@savoirfairelinux.com>

Acked-by: Arun Ramadoss <arun.ramadoss@microchip.com>

Re: [PATCH v2 net-next] net: dsa: microchip: ksz9477: split half-duplex monitoring function

[patchwork-bot+netdevbpf]

Hello:

This patch was applied to netdev/net-next.git (main)
by David S. Miller <davem@davemloft.net>:

On Thu,  8 Aug 2024 15:14:21 +0000 you wrote:
> In order to respect the 80 columns limit, split the half-duplex
> monitoring function in two.
> 
> This is just a styling change, no functional change.
> 
> Signed-off-by: Enguerrand de Ribaucourt <enguerrand.de-ribaucourt@savoirfairelinux.com>
> 
> [...]

Here is the summary with links:
  - [v2,net-next] net: dsa: microchip: ksz9477: split half-duplex monitoring function
    https://git.kernel.org/netdev/net-next/c/c4e82c025b3f

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

Re: [PATCH v2 net-next] net: dsa: microchip: ksz9477: split half-duplex monitoring function

[Andrew Lunn]

> -	ret = ksz_pread8(dev, port, REG_PORT_STATUS_0, &status);
> +	/* Errata DS80000754 recommends monitoring potential faults in
> +	 * half-duplex mode. The switch might not be able to communicate anymore
> +	 * in these states. If you see this message, please read the
> +	 * errata-sheet for more information:
> +	 * https://ww1.microchip.com/downloads/aemDocuments/documents
> +	 * /UNG/ProductDocuments/Errata/KSZ9477S-Errata-DS80000754.pdf

The 80 character rule is not sacrosanct. We allow strings which will
appear in the log to be longer than 80 because they are hard to grep
for otherwise. The 80 character limit is really about keeping the
indentation level low, encouraging lots of small functions which do
one thing, and does it well. This is exactly what you are doing here,
taking a big function and splitting it up into smaller functions with
less indentation. I see a URL as something which should not be split
like this, it should be allowed to be longer than 80 characters,
because i want it to be easy to copy/paste into a web browser.

With that fixed, please add a Reviewed-by: Andrew Lunn <andrew@lunn.ch>

    Andrew