[net 0/3] gianfar: Tx flow control fix (adjust_link)

[net 0/3] gianfar: Tx flow control fix (adjust_link)

[Claudiu Manoil]

Fix a small blunder in the Tx pause frame settings, that
went unnoticed in the tangled code of adjust_link().
I followed up with a couple of simple refactoring patches,
aiming to make adjust_link() more manageable.

(The last 2 patches may be postponed if they are too much
for the current stage of net.)

Claudiu Manoil (3):
  gianfar: Fix Tx flow control deactivation
  gianfar: Refactor link speed update (adjust_link)
  gianfar: Refactor link flow control update (adjust_link)

 drivers/net/ethernet/freescale/gianfar.c | 134 ++++++++++++++++---------------
 1 file changed, 70 insertions(+), 64 deletions(-)

-- 
2.7.4

[net 1/3] gianfar: Fix Tx flow control deactivation

[Claudiu Manoil]

Indeed, the wrong register is checked for the Tx flow control bit,
it should have been maccfg1 not maccfg2.
This went unnoticed for so long probably because the impact is
hardly visible, not to mention the tangled code from adjust_link().
First, link flow control (i.e. handling of Rx/Tx link level pause frames)
is disabled by default (needs to be enabled via 'ethtool -A').
Secondly, maccfg2 always returns 0 for tx_flow_oldval (except for a few
old boards), which results in Tx flow control remaining always on
once activated.

Fixes: 45b679c9a3ccd9e34f28e6ec677b812a860eb8eb -
	"gianfar: Implement PAUSE frame generation support"

Cc: stable@kernel.org

Signed-off-by: Claudiu Manoil <claudiu.manoil@nxp.com>
---
 drivers/net/ethernet/freescale/gianfar.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/ethernet/freescale/gianfar.c b/drivers/net/ethernet/freescale/gianfar.c
index c4b4b0a..5be52d8 100644
--- a/drivers/net/ethernet/freescale/gianfar.c
+++ b/drivers/net/ethernet/freescale/gianfar.c
@@ -3687,7 +3687,7 @@ static noinline void gfar_update_link_state(struct gfar_private *priv)
 		u32 tempval1 = gfar_read(&regs->maccfg1);
 		u32 tempval = gfar_read(&regs->maccfg2);
 		u32 ecntrl = gfar_read(&regs->ecntrl);
-		u32 tx_flow_oldval = (tempval & MACCFG1_TX_FLOW);
+		u32 tx_flow_oldval = (tempval1 & MACCFG1_TX_FLOW);
 
 		if (phydev->duplex != priv->oldduplex) {
 			if (!(phydev->duplex))
-- 
2.7.4

[net 2/3] gianfar: Refactor link speed update (adjust_link)

[Claudiu Manoil]

Encapsulate link speed update logic. These settings affect only
the maccfg2 and ecntrl regs.

Signed-off-by: Claudiu Manoil <claudiu.manoil@nxp.com>
---
 drivers/net/ethernet/freescale/gianfar.c | 92 +++++++++++++++++---------------
 1 file changed, 49 insertions(+), 43 deletions(-)

diff --git a/drivers/net/ethernet/freescale/gianfar.c b/drivers/net/ethernet/freescale/gianfar.c
index 5be52d8..46880a9 100644
--- a/drivers/net/ethernet/freescale/gianfar.c
+++ b/drivers/net/ethernet/freescale/gianfar.c
@@ -3672,6 +3672,54 @@ static u32 gfar_get_flowctrl_cfg(struct gfar_private *priv)
 	return val;
 }
 
+static void gfar_update_link_speed(struct gfar_private *priv)
+{
+	struct gfar __iomem *regs = priv->gfargrp[0].regs;
+	struct phy_device *phydev = priv->ndev->phydev;
+	u32 maccfg2, ecntrl;
+
+	maccfg2 = gfar_read(&regs->maccfg2);
+	ecntrl = gfar_read(&regs->ecntrl);
+
+	if (phydev->duplex != priv->oldduplex) {
+		if (!(phydev->duplex))
+			maccfg2 &= ~(MACCFG2_FULL_DUPLEX);
+		else
+			maccfg2 |= MACCFG2_FULL_DUPLEX;
+
+		priv->oldduplex = phydev->duplex;
+	}
+
+	if (phydev->speed != priv->oldspeed) {
+		switch (phydev->speed) {
+		case 1000:
+			maccfg2 = ((maccfg2 & ~(MACCFG2_IF)) | MACCFG2_GMII);
+
+			ecntrl &= ~(ECNTRL_R100);
+			break;
+		case 100:
+		case 10:
+			maccfg2 = ((maccfg2 & ~(MACCFG2_IF)) | MACCFG2_MII);
+
+			/* Reduced mode distinguishes between 10 and 100 */
+			if (phydev->speed == SPEED_100)
+				ecntrl |= ECNTRL_R100;
+			else
+				ecntrl &= ~(ECNTRL_R100);
+			break;
+		default:
+			netif_warn(priv, link, priv->ndev,
+				   "Invalid link speed!\n");
+			break;
+		}
+
+		priv->oldspeed = phydev->speed;
+	}
+
+	gfar_write(&regs->maccfg2, maccfg2);
+	gfar_write(&regs->ecntrl, ecntrl);
+}
+
 static noinline void gfar_update_link_state(struct gfar_private *priv)
 {
 	struct gfar __iomem *regs = priv->gfargrp[0].regs;
@@ -3685,49 +3733,9 @@ static noinline void gfar_update_link_state(struct gfar_private *priv)
 
 	if (phydev->link) {
 		u32 tempval1 = gfar_read(&regs->maccfg1);
-		u32 tempval = gfar_read(&regs->maccfg2);
-		u32 ecntrl = gfar_read(&regs->ecntrl);
 		u32 tx_flow_oldval = (tempval1 & MACCFG1_TX_FLOW);
 
-		if (phydev->duplex != priv->oldduplex) {
-			if (!(phydev->duplex))
-				tempval &= ~(MACCFG2_FULL_DUPLEX);
-			else
-				tempval |= MACCFG2_FULL_DUPLEX;
-
-			priv->oldduplex = phydev->duplex;
-		}
-
-		if (phydev->speed != priv->oldspeed) {
-			switch (phydev->speed) {
-			case 1000:
-				tempval =
-				    ((tempval & ~(MACCFG2_IF)) | MACCFG2_GMII);
-
-				ecntrl &= ~(ECNTRL_R100);
-				break;
-			case 100:
-			case 10:
-				tempval =
-				    ((tempval & ~(MACCFG2_IF)) | MACCFG2_MII);
-
-				/* Reduced mode distinguishes
-				 * between 10 and 100
-				 */
-				if (phydev->speed == SPEED_100)
-					ecntrl |= ECNTRL_R100;
-				else
-					ecntrl &= ~(ECNTRL_R100);
-				break;
-			default:
-				netif_warn(priv, link, priv->ndev,
-					   "Ack!  Speed (%d) is not 10/100/1000!\n",
-					   phydev->speed);
-				break;
-			}
-
-			priv->oldspeed = phydev->speed;
-		}
+		gfar_update_link_speed(priv);
 
 		tempval1 &= ~(MACCFG1_TX_FLOW | MACCFG1_RX_FLOW);
 		tempval1 |= gfar_get_flowctrl_cfg(priv);
@@ -3749,8 +3757,6 @@ static noinline void gfar_update_link_state(struct gfar_private *priv)
 			priv->tx_actual_en = 0;
 
 		gfar_write(&regs->maccfg1, tempval1);
-		gfar_write(&regs->maccfg2, tempval);
-		gfar_write(&regs->ecntrl, ecntrl);
 
 		if (!priv->oldlink)
 			priv->oldlink = 1;
-- 
2.7.4

[net 3/3] gianfar: Refactor link flow control update (adjust_link)

[Claudiu Manoil]

Encapsulate link layer flow control logic. These settings
are touching maccfg1 reg exclusively.

Signed-off-by: Claudiu Manoil <claudiu.manoil@nxp.com>
---
 drivers/net/ethernet/freescale/gianfar.c | 68 ++++++++++++++++----------------
 1 file changed, 34 insertions(+), 34 deletions(-)

diff --git a/drivers/net/ethernet/freescale/gianfar.c b/drivers/net/ethernet/freescale/gianfar.c
index 46880a9..1648173 100644
--- a/drivers/net/ethernet/freescale/gianfar.c
+++ b/drivers/net/ethernet/freescale/gianfar.c
@@ -3672,6 +3672,36 @@ static u32 gfar_get_flowctrl_cfg(struct gfar_private *priv)
 	return val;
 }
 
+static void gfar_update_link_flowctrl(struct gfar_private *priv)
+{
+	struct gfar __iomem *regs = priv->gfargrp[0].regs;
+	u32 maccfg1, tx_flow_oldval;
+	int i;
+
+	maccfg1 = gfar_read(&regs->maccfg1);
+	tx_flow_oldval = (maccfg1 & MACCFG1_TX_FLOW);
+
+	maccfg1 &= ~(MACCFG1_TX_FLOW | MACCFG1_RX_FLOW);
+	maccfg1 |= gfar_get_flowctrl_cfg(priv);
+
+	/* Turn last free buffer recording on */
+	if ((maccfg1 & MACCFG1_TX_FLOW) && !tx_flow_oldval) {
+		for (i = 0; i < priv->num_rx_queues; i++) {
+			u32 bdp_dma;
+
+			bdp_dma = gfar_rxbd_dma_lastfree(priv->rx_queue[i]);
+			gfar_write(priv->rx_queue[i]->rfbptr, bdp_dma);
+		}
+
+		priv->tx_actual_en = 1;
+	}
+
+	if (unlikely(!(maccfg1 & MACCFG1_TX_FLOW) && tx_flow_oldval))
+		priv->tx_actual_en = 0;
+
+	gfar_write(&regs->maccfg1, maccfg1);
+}
+
 static void gfar_update_link_speed(struct gfar_private *priv)
 {
 	struct gfar __iomem *regs = priv->gfargrp[0].regs;
@@ -3722,44 +3752,14 @@ static void gfar_update_link_speed(struct gfar_private *priv)
 
 static noinline void gfar_update_link_state(struct gfar_private *priv)
 {
-	struct gfar __iomem *regs = priv->gfargrp[0].regs;
-	struct net_device *ndev = priv->ndev;
-	struct phy_device *phydev = ndev->phydev;
-	struct gfar_priv_rx_q *rx_queue = NULL;
-	int i;
-
 	if (unlikely(test_bit(GFAR_RESETTING, &priv->state)))
 		return;
 
-	if (phydev->link) {
-		u32 tempval1 = gfar_read(&regs->maccfg1);
-		u32 tx_flow_oldval = (tempval1 & MACCFG1_TX_FLOW);
-
+	if (priv->ndev->phydev->link) {
+		gfar_update_link_flowctrl(priv);
 		gfar_update_link_speed(priv);
 
-		tempval1 &= ~(MACCFG1_TX_FLOW | MACCFG1_RX_FLOW);
-		tempval1 |= gfar_get_flowctrl_cfg(priv);
-
-		/* Turn last free buffer recording on */
-		if ((tempval1 & MACCFG1_TX_FLOW) && !tx_flow_oldval) {
-			for (i = 0; i < priv->num_rx_queues; i++) {
-				u32 bdp_dma;
-
-				rx_queue = priv->rx_queue[i];
-				bdp_dma = gfar_rxbd_dma_lastfree(rx_queue);
-				gfar_write(rx_queue->rfbptr, bdp_dma);
-			}
-
-			priv->tx_actual_en = 1;
-		}
-
-		if (unlikely(!(tempval1 & MACCFG1_TX_FLOW) && tx_flow_oldval))
-			priv->tx_actual_en = 0;
-
-		gfar_write(&regs->maccfg1, tempval1);
-
-		if (!priv->oldlink)
-			priv->oldlink = 1;
+		priv->oldlink = 1;
 
 	} else if (priv->oldlink) {
 		priv->oldlink = 0;
@@ -3768,7 +3768,7 @@ static noinline void gfar_update_link_state(struct gfar_private *priv)
 	}
 
 	if (netif_msg_link(priv))
-		phy_print_status(phydev);
+		phy_print_status(priv->ndev->phydev);
 }
 
 static const struct of_device_id gfar_match[] =
-- 
2.7.4

Re: [net 0/3] gianfar: Tx flow control fix (adjust_link)

[David Miller]

From: Claudiu Manoil <claudiu.manoil@nxp.com>
Date: Fri, 1 Sep 2017 12:41:00 +0300

> Fix a small blunder in the Tx pause frame settings, that
> went unnoticed in the tangled code of adjust_link().
> I followed up with a couple of simple refactoring patches,
> aiming to make adjust_link() more manageable.
> 
> (The last 2 patches may be postponed if they are too much
> for the current stage of net.)

You need to fix some things up here.

First, do not mix bug fixes with cleanups.  Refactoring is a cleanup.

Submit the bug fix for 'net' and then later you can submit the
cleanups for 'net-next'.

Second, do not CC: stable for networking bug fixes, instead explicitly
ask me to queue up the fix for -stable.

Third, you need to fix how you specify your Fixes tag, it must
be exactly:

Fixes: $(SHA1_ID) ("Commit header text.")

And no matter how long the line is, do not break it up.

Thank you.