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

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

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

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