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:r...@armlinux.org.uk]
Sent: Tuesday, November 28, 2017 5:56 PM
To: Antoine Tenart <antoine.ten...@free-electrons.com>
Cc: and...@lunn.ch; f.faine...@gmail.com; da...@davemloft.net; Yan Markman
<ymark...@marvell.com>; gregory.clem...@free-electrons.com;
thomas.petazz...@free-electrons.com; miquel.ray...@free-electrons.com; Nadav
Haklai <nad...@marvell.com>; m...@semihalf.com; Stefan Chulski
<stef...@marvell.com>; netdev@vger.kernel.org; linux-ker...@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 li...@armlinux.org.uk or the address I
use in the sign-offs - sending them to r...@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:
> > From: Yan Markman <ymark...@marvell.com>
>
> Hi, thanks for the patch.
>
> > 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?
>
> >
> > Fixes: 9525ae83959b ("phylink: add phylink infrastructure")
> > Signed-off-by: Yan Markman <ymark...@marvell.com>
> > [Antoine: commit message]
> > Signed-off-by: Antoine Tenart <antoine.ten...@free-electrons.com>
> > ---
> > 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
