[PATCH net-next 0/4] net: phy: xpcs: Link errors improvements

[PATCH net-next 0/4] net: phy: xpcs: Link errors improvements

[Jose Abreu]

First set of improvements for XPCS that were developed after exhaustive
tests of Link Down / Link Up transitions.

As a side note, more fixes and improvements may come in the near future
as we are adding XLGMII support at 25G, 40G, 50G and 100G speeds for
both XPCS and stmmac drivers.

Patch 1/4, prevents reading old values from RX/TX fault.

Patch 2/4 and 3/4, signals the check link function for critical errors
that can disturbe link normal operation and that need XPCS reset.

Patch 4/4, resets the XPCS at probe so that we start from a well known
state.

---
Cc: Jose Abreu <Jose.Abreu@synopsys.com>
Cc: Andrew Lunn <andrew@lunn.ch>
Cc: Florian Fainelli <f.fainelli@gmail.com>
Cc: Heiner Kallweit <hkallweit1@gmail.com>
Cc: Russell King <linux@armlinux.org.uk>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: netdev@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
---

Jose Abreu (4):
  net: phy: xpcs: Clear latched value of RX/TX fault
  net: phy: xpcs: Return error upon RX/TX fault
  net: phy: xpcs: Return error when 10GKR link errors are found
  net: phy: xpcs: Reset XPCS upon probe

 drivers/net/phy/mdio-xpcs.c | 15 +++++++++++----
 1 file changed, 11 insertions(+), 4 deletions(-)

-- 
2.7.4

[PATCH net-next 1/4] net: phy: xpcs: Clear latched value of RX/TX fault

[Jose Abreu]

When reading RX/TX fault register we may have latched values from Link
down. Clear the latched value first and then read it again to make sure
no old errors are flagged and that new errors are caught.

Signed-off-by: Jose Abreu <Jose.Abreu@synopsys.com>

---
Cc: Jose Abreu <Jose.Abreu@synopsys.com>
Cc: Andrew Lunn <andrew@lunn.ch>
Cc: Florian Fainelli <f.fainelli@gmail.com>
Cc: Heiner Kallweit <hkallweit1@gmail.com>
Cc: Russell King <linux@armlinux.org.uk>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: netdev@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
---
 drivers/net/phy/mdio-xpcs.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/net/phy/mdio-xpcs.c b/drivers/net/phy/mdio-xpcs.c
index 973f588146f7..a4cbeecc6d42 100644
--- a/drivers/net/phy/mdio-xpcs.c
+++ b/drivers/net/phy/mdio-xpcs.c
@@ -185,6 +185,7 @@ static int xpcs_read_fault(struct mdio_xpcs_args *xpcs,
 		return -EFAULT;
 	}
 
+	xpcs_read(xpcs, MDIO_MMD_PCS, MDIO_STAT2);
 	ret = xpcs_read(xpcs, MDIO_MMD_PCS, MDIO_STAT2);
 	if (ret < 0)
 		return ret;
-- 
2.7.4

[PATCH net-next 2/4] net: phy: xpcs: Return error upon RX/TX fault

[Jose Abreu]

RX/TX fault status results in link errors. Return error upon these cases
so that XPCS can be correctly resumed.

Signed-off-by: Jose Abreu <Jose.Abreu@synopsys.com>

---
Cc: Jose Abreu <Jose.Abreu@synopsys.com>
Cc: Andrew Lunn <andrew@lunn.ch>
Cc: Florian Fainelli <f.fainelli@gmail.com>
Cc: Heiner Kallweit <hkallweit1@gmail.com>
Cc: Russell King <linux@armlinux.org.uk>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: netdev@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
---
 drivers/net/phy/mdio-xpcs.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/drivers/net/phy/mdio-xpcs.c b/drivers/net/phy/mdio-xpcs.c
index a4cbeecc6d42..23516397b982 100644
--- a/drivers/net/phy/mdio-xpcs.c
+++ b/drivers/net/phy/mdio-xpcs.c
@@ -190,10 +190,14 @@ static int xpcs_read_fault(struct mdio_xpcs_args *xpcs,
 	if (ret < 0)
 		return ret;
 
-	if (ret & MDIO_STAT2_RXFAULT)
+	if (ret & MDIO_STAT2_RXFAULT) {
 		xpcs_warn(xpcs, state, "Receiver fault detected!\n");
-	if (ret & MDIO_STAT2_TXFAULT)
+		return -EFAULT;
+	}
+	if (ret & MDIO_STAT2_TXFAULT) {
 		xpcs_warn(xpcs, state, "Transmitter fault detected!\n");
+		return -EFAULT;
+	}
 
 	ret = xpcs_read_vendor(xpcs, MDIO_MMD_PCS, DW_VR_XS_PCS_DIG_STS);
 	if (ret < 0)
-- 
2.7.4

[PATCH net-next 3/4] net: phy: xpcs: Return error when 10GKR link errors are found

[Jose Abreu]

For 10GKR rate, when link errors are found we need to return fault
status so that XPCS is correctly resumed.

Signed-off-by: Jose Abreu <Jose.Abreu@synopsys.com>

---
Cc: Jose Abreu <Jose.Abreu@synopsys.com>
Cc: Andrew Lunn <andrew@lunn.ch>
Cc: Florian Fainelli <f.fainelli@gmail.com>
Cc: Heiner Kallweit <hkallweit1@gmail.com>
Cc: Russell King <linux@armlinux.org.uk>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: netdev@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
---
 drivers/net/phy/mdio-xpcs.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/net/phy/mdio-xpcs.c b/drivers/net/phy/mdio-xpcs.c
index 23516397b982..83ced7180a44 100644
--- a/drivers/net/phy/mdio-xpcs.c
+++ b/drivers/net/phy/mdio-xpcs.c
@@ -219,8 +219,10 @@ static int xpcs_read_fault(struct mdio_xpcs_args *xpcs,
 	if (ret < 0)
 		return ret;
 
-	if (ret & MDIO_PCS_10GBRT_STAT2_ERR)
+	if (ret & MDIO_PCS_10GBRT_STAT2_ERR) {
 		xpcs_warn(xpcs, state, "Link has errors!\n");
+		return -EFAULT;
+	}
 
 	return 0;
 }
-- 
2.7.4

[PATCH net-next 4/4] net: phy: xpcs: Reset XPCS upon probe

[Jose Abreu]

Reset the XPCS upon probe stage so that we start it from well known
state.

Signed-off-by: Jose Abreu <Jose.Abreu@synopsys.com>

---
Cc: Jose Abreu <Jose.Abreu@synopsys.com>
Cc: Andrew Lunn <andrew@lunn.ch>
Cc: Florian Fainelli <f.fainelli@gmail.com>
Cc: Heiner Kallweit <hkallweit1@gmail.com>
Cc: Russell King <linux@armlinux.org.uk>
Cc: "David S. Miller" <davem@davemloft.net>
Cc: netdev@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
---
 drivers/net/phy/mdio-xpcs.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/net/phy/mdio-xpcs.c b/drivers/net/phy/mdio-xpcs.c
index 83ced7180a44..7960dc52c4bd 100644
--- a/drivers/net/phy/mdio-xpcs.c
+++ b/drivers/net/phy/mdio-xpcs.c
@@ -595,7 +595,7 @@ static int xpcs_probe(struct mdio_xpcs_args *xpcs, phy_interface_t interface)
 			match = entry;
 
 			if (xpcs_check_features(xpcs, match, interface))
-				return 0;
+				return xpcs_soft_reset(xpcs, MDIO_MMD_PCS);
 		}
 	}
 
-- 
2.7.4

Re: [PATCH net-next 1/4] net: phy: xpcs: Clear latched value of RX/TX fault

[Russell King - ARM Linux admin]

On Fri, Mar 13, 2020 at 02:39:40PM +0100, Jose Abreu wrote:
> When reading RX/TX fault register we may have latched values from Link
> down. Clear the latched value first and then read it again to make sure
> no old errors are flagged and that new errors are caught.

The purpose of the latched link down is so that software can respond
to a momentary loss of link with a possible change in the negotiation
results.  That is why IEEE 802.3 wants a link loss to be a latched
event.

Double-reading the status register loses that information, and hides
it from phylink.  A change in negotiation, which can occur very
quickly on fiber links) can go unnoticed if the latching is not
propagated up through phylink.

If the negotiation parameters have changed, and pcs_get_state() does
not report that the link has failed, then mac_link_up() will _not_ be
called with the new link parameters, and the MAC will continue using
the old ones.  Therefore, it is very important that any link-down
event is reported to phylink.

Phylink currently doesn't respond to a link-down event reported via
PCS by re-checking after processing the link loss, but it could do,
which would improve it's behaviour in that scenario.  I would prefer
this resolution, rather than your proposed double-reading of the
status register to "lose" the link-down event.

I do have some patches that make that easier, but they're delayed
behind the mass of patches that I still have outstanding - and trying
to get progress on getting phylink patches merged has been glacial,
and fraught with problems this time around.

> 
> Signed-off-by: Jose Abreu <Jose.Abreu@synopsys.com>
> 
> ---
> Cc: Jose Abreu <Jose.Abreu@synopsys.com>
> Cc: Andrew Lunn <andrew@lunn.ch>
> Cc: Florian Fainelli <f.fainelli@gmail.com>
> Cc: Heiner Kallweit <hkallweit1@gmail.com>
> Cc: Russell King <linux@armlinux.org.uk>
> Cc: "David S. Miller" <davem@davemloft.net>
> Cc: netdev@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org
> ---
>  drivers/net/phy/mdio-xpcs.c | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/drivers/net/phy/mdio-xpcs.c b/drivers/net/phy/mdio-xpcs.c
> index 973f588146f7..a4cbeecc6d42 100644
> --- a/drivers/net/phy/mdio-xpcs.c
> +++ b/drivers/net/phy/mdio-xpcs.c
> @@ -185,6 +185,7 @@ static int xpcs_read_fault(struct mdio_xpcs_args *xpcs,
>  		return -EFAULT;
>  	}
>  
> +	xpcs_read(xpcs, MDIO_MMD_PCS, MDIO_STAT2);
>  	ret = xpcs_read(xpcs, MDIO_MMD_PCS, MDIO_STAT2);
>  	if (ret < 0)
>  		return ret;
> -- 
> 2.7.4
> 
> 

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 10.2Mbps down 587kbps up

Re: [PATCH net-next 2/4] net: phy: xpcs: Return error upon RX/TX fault

[Russell King - ARM Linux admin]

On Fri, Mar 13, 2020 at 02:39:41PM +0100, Jose Abreu wrote:
> RX/TX fault status results in link errors. Return error upon these cases
> so that XPCS can be correctly resumed.

Are you sure about this?  I'm sure that I read in IEEE 802.3 that
a loss of link results in a receive fault being indicated in
status register 2.

See 49.2.14.1 describing "PCS_status" and the descriptions of MDIO
registers 3.1.2 and 3.8.10.  Basically, the link status (3.1.2) is a
latched-low version of PCS_status, and 3.8.10 is an inverted version
of this, independently latched-high.

Returning -EFAULT seems to mean that we'll soft-reset the PHY, and
reconfigure it every time we attempt to read the status whenever the
link is down.

> 
> Signed-off-by: Jose Abreu <Jose.Abreu@synopsys.com>
> 
> ---
> Cc: Jose Abreu <Jose.Abreu@synopsys.com>
> Cc: Andrew Lunn <andrew@lunn.ch>
> Cc: Florian Fainelli <f.fainelli@gmail.com>
> Cc: Heiner Kallweit <hkallweit1@gmail.com>
> Cc: Russell King <linux@armlinux.org.uk>
> Cc: "David S. Miller" <davem@davemloft.net>
> Cc: netdev@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org
> ---
>  drivers/net/phy/mdio-xpcs.c | 8 ++++++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/net/phy/mdio-xpcs.c b/drivers/net/phy/mdio-xpcs.c
> index a4cbeecc6d42..23516397b982 100644
> --- a/drivers/net/phy/mdio-xpcs.c
> +++ b/drivers/net/phy/mdio-xpcs.c
> @@ -190,10 +190,14 @@ static int xpcs_read_fault(struct mdio_xpcs_args *xpcs,
>  	if (ret < 0)
>  		return ret;
>  
> -	if (ret & MDIO_STAT2_RXFAULT)
> +	if (ret & MDIO_STAT2_RXFAULT) {
>  		xpcs_warn(xpcs, state, "Receiver fault detected!\n");
> -	if (ret & MDIO_STAT2_TXFAULT)
> +		return -EFAULT;
> +	}
> +	if (ret & MDIO_STAT2_TXFAULT) {
>  		xpcs_warn(xpcs, state, "Transmitter fault detected!\n");
> +		return -EFAULT;
> +	}
>  
>  	ret = xpcs_read_vendor(xpcs, MDIO_MMD_PCS, DW_VR_XS_PCS_DIG_STS);
>  	if (ret < 0)
> -- 
> 2.7.4
> 
> 

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line in suburbia: sync at 10.2Mbps down 587kbps up

RE: [PATCH net-next 1/4] net: phy: xpcs: Clear latched value of RX/TX fault

[Jose Abreu]

From: Russell King - ARM Linux admin <linux@armlinux.org.uk>
Date: Mar/13/2020, 14:01:22 (UTC+00:00)

> On Fri, Mar 13, 2020 at 02:39:40PM +0100, Jose Abreu wrote:
> > When reading RX/TX fault register we may have latched values from Link
> > down. Clear the latched value first and then read it again to make sure
> > no old errors are flagged and that new errors are caught.
> 
> The purpose of the latched link down is so that software can respond
> to a momentary loss of link with a possible change in the negotiation
> results.  That is why IEEE 802.3 wants a link loss to be a latched
> event.
> 
> Double-reading the status register loses that information, and hides
> it from phylink.  A change in negotiation, which can occur very
> quickly on fiber links) can go unnoticed if the latching is not
> propagated up through phylink.
> 
> If the negotiation parameters have changed, and pcs_get_state() does
> not report that the link has failed, then mac_link_up() will _not_ be
> called with the new link parameters, and the MAC will continue using
> the old ones.  Therefore, it is very important that any link-down
> event is reported to phylink.
> 
> Phylink currently doesn't respond to a link-down event reported via
> PCS by re-checking after processing the link loss, but it could do,
> which would improve it's behaviour in that scenario.  I would prefer
> this resolution, rather than your proposed double-reading of the
> status register to "lose" the link-down event.
> 
> I do have some patches that make that easier, but they're delayed
> behind the mass of patches that I still have outstanding - and trying
> to get progress on getting phylink patches merged has been glacial,
> and fraught with problems this time around.

This is not link status register. Its TX / RX fault and its latched high. 
Link status is another register and we only read it once because of the 
above reasons you mentioned.

When in 10GKR, this seems to always go up after link transition, hence we 
added the double read.

I just read your reply to patch 2/4 of this series and it looks like the 
two patches are correlated.

---
Thanks,
Jose Miguel Abreu