Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Fri, 13 Mar 2020 14:01:22 +0000
From:   Russell King - ARM Linux admin <linux@armlinux.org.uk>
To:     Jose Abreu <Jose.Abreu@synopsys.com>
Cc:     netdev@vger.kernel.org, Joao Pinto <Joao.Pinto@synopsys.com>,
        Andrew Lunn <andrew@lunn.ch>,
        Florian Fainelli <f.fainelli@gmail.com>,
        Heiner Kallweit <hkallweit1@gmail.com>,
        "David S. Miller" <davem@davemloft.net>,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH net-next 1/4] net: phy: xpcs: Clear latched value of
 RX/TX fault
Message-ID: <20200313140122.GC25745@shell.armlinux.org.uk>
References: <cover.1584106347.git.Jose.Abreu@synopsys.com>
 <50f3dd2ab58fecfea1156aaf8dbfa99d0c7b36be.1584106347.git.Jose.Abreu@synopsys.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <50f3dd2ab58fecfea1156aaf8dbfa99d0c7b36be.1584106347.git.Jose.Abreu@synopsys.com>
User-Agent: Mutt/1.10.1 (2018-07-13)
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

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