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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <20448667430e434aad5bb8cd1b082611@IL-EXCH01.marvell.com>
Date:   Wed, 29 Nov 2017 19:33:44 +0000
From:   Yan Markman <ymarkman@...vell.com>
To:     Russell King <rmk@...linux.org.uk>,
        Antoine Tenart <antoine.tenart@...e-electrons.com>
CC:     "andrew@...n.ch" <andrew@...n.ch>,
        "f.fainelli@...il.com" <f.fainelli@...il.com>,
        "davem@...emloft.net" <davem@...emloft.net>,
        "gregory.clement@...e-electrons.com" 
        <gregory.clement@...e-electrons.com>,
        "thomas.petazzoni@...e-electrons.com" 
        <thomas.petazzoni@...e-electrons.com>,
        "miquel.raynal@...e-electrons.com" <miquel.raynal@...e-electrons.com>,
        "Nadav Haklai" <nadavh@...vell.com>,
        "mw@...ihalf.com" <mw@...ihalf.com>,
        "Stefan Chulski" <stefanc@...vell.com>,
        "netdev@...r.kernel.org" <netdev@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>
Subject: 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@...linux.org.uk] 
Sent: Tuesday, November 28, 2017 5:56 PM
To: Antoine Tenart <antoine.tenart@...e-electrons.com>
Cc: andrew@...n.ch; f.fainelli@...il.com; davem@...emloft.net; Yan Markman <ymarkman@...vell.com>; gregory.clement@...e-electrons.com; thomas.petazzoni@...e-electrons.com; miquel.raynal@...e-electrons.com; Nadav Haklai <nadavh@...vell.com>; mw@...ihalf.com; Stefan Chulski <stefanc@...vell.com>; netdev@...r.kernel.org; linux-kernel@...r.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@...linux.org.uk or the address I use in the sign-offs - sending them to rmk@...linux.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 <ymarkman@...vell.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 <ymarkman@...vell.com>
> > [Antoine: commit message]
> > Signed-off-by: Antoine Tenart <antoine.tenart@...e-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