RE: [EXT] Re: [PATCH net] net: phylink: fix link state on phy-connect

Hi Russel

On my board I have [Marvell 88E1510] phy working with STATUS-POLLING
I see some inconsistencies  -- first ifconfig-up is different from furthers, no "link is down" reports.
Please refer the behavior example below.
My patch is a "simple solution"  -- always reset/clear Link-state-parameters before going UP.
Possibly, more correct (but much more complicated) solution would be in the   phy state machine   and   phylink resolve modification.
I just found that 
    On ifconfig-down, the phy-state-machine and phylink-resolve
    are stopped before executing before passing over full graceful down/reset state.
    The further ifconfig-up starts with old state parameters.
Special cases not-tested but logic 2 test-cases are:
   remote side changes speed whilst link is Down or Disconnected. But local ifconfig-up starts with old speed.

Best regards
Yan Markman
----------------------------------------------------
EXAMPLE:
buildroot login: root
~# ifconfig eth1 192.169.0.81 up
[   34.072042] mvpp2 f2000000.ethernet eth1: PHY [f212a200.mdio-mii:01] driver [Marvell 88E1510]
[   34.080654] mvpp2 f2000000.ethernet eth1: configuring for phy/rgmii-id link mode
[   37.220506] mvpp2 f2000000.ethernet eth1: Link is Up - 1Gbps/Full - flow control off

~# ifconfig eth1 down
          No print "link is down"

~# ifconfig eth1 up
          "Link is Up" passed twice:
[   60.748041] mvpp2 f2000000.ethernet eth1: PHY [f212a200.mdio-mii:01] driver [Marvell 88E1510]
[   60.756653] mvpp2 f2000000.ethernet eth1: configuring for phy/rgmii-id link mode
[   60.764169] mvpp2 f2000000.ethernet eth1: Link is Up - 1Gbps/Full - flow control off
[   63.908504] mvpp2 f2000000.ethernet eth1: Link is Up - 1Gbps/Full - flow control off

On Link physical disconnect/break 
          No print "link is down"
          But link is in correct state --   ifconfig UP but not-RUNNING
On Link physical re-connect
[   84.388501] mvpp2 f2000000.ethernet eth1: Link is Up - 1Gbps/Full - flow control off
-------------------------------------------------------------------------------------------
YAN's findings:


Best regards
Yan Markman

-----Original Message-----
From: Russell King [mailto:rmk@armlinux.org.uk] 
Sent: Tuesday, November 28, 2017 5:56 PM
To: Antoine Tenart <redacted>
Cc: andrew@lunn.ch; f.fainelli@gmail.com; davem@davemloft.net; Yan Markman <redacted>; gregory.clement@free-electrons.com; thomas.petazzoni@free-electrons.com; miquel.raynal@free-electrons.com; Nadav Haklai <redacted>; mw@semihalf.com; Stefan Chulski <redacted>; netdev@vger.kernel.org; linux-kernel@vger.kernel.org
Subject: [EXT] Re: [PATCH net] net: phylink: fix link state on phy-connect

External Email

----------------------------------------------------------------------
Oh, and lastly, please send patches to linux@armlinux.org.uk or the address I use in the sign-offs - sending them to rmk@armlinux.org.uk is for personal non-Linux mail only, and has resulted in _all_ of these messages ending up in my spam folder.

Thanks.

On Tue, Nov 28, 2017 at 03:53:17PM +0000, Russell King wrote:

On Tue, Nov 28, 2017 at 02:29:32PM +0100, Antoine Tenart wrote:

quoted

From: Yan Markman <redacted>

Hi, thanks for the patch.

quoted

When calling successively _connect, _disconnect and _connect again, 
if the link configuration changed whilst being down from the phylink 
perspective, the last _connect would stay in an incorrect old speed.
Fixes this by setting the link configuration parameters to an 
unknown value when calling phylink_bringup_phy.

Under what circumstances does this occur?

quoted

Fixes: 9525ae83959b ("phylink: add phylink infrastructure")
Signed-off-by: Yan Markman <redacted>
[Antoine: commit message]
Signed-off-by: Antoine Tenart <redacted>
---
 drivers/net/phy/phylink.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c 
index e3bbc70372d3..c2cec3eef67d 100644
--- a/drivers/net/phy/phylink.c
+++ b/drivers/net/phy/phylink.c

@@ -621,6 +621,16 @@ static int phylink_bringup_phy(struct phylink *pl, struct phy_device *phy)
 	if (ret)
 		return ret;
 
+	/* On _disconnect, the phy state machine and phylink resolve
+	 * are stopped before executing full gracefull down/reset state.
+	 * The further _connect starts with incorrect init state. Let's set
+	 * init values here.
+	 */
+	pl->phy_state.link = false;
+	pl->link_config.pause = MLO_PAUSE_AN;
+	pl->link_config.speed = SPEED_UNKNOWN;
+	pl->link_config.duplex = DUPLEX_UNKNOWN;

It would be much better to clean up the phy_state in
phylink_disconnect_phy() and trigger a resolve, rather than doing that 
each time a PHY is connected - the link should be taken down when the 
PHY is removed.

However, I'd like to know under what circumstances this is happening, 
since, if you're hotplugging a PHY you should be doing that via SFP 
which has additional link up/down handling.  What board is this with?

Also note that there's a number of patches in my "phy" branch that I'm 
intending to send as a result of working with Florian over the last 
few weeks.  There's several people working fairly independently in 
this area and having everyone send patches independently of each other 
could get painful to manage.

I'm intending to send patches once I know that net-next is open.

--
Russell King
ARM architecture Linux Kernel maintainer

--
Russell King
ARM architecture Linux Kernel maintainer