From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752623AbdK2TeN convert rfc822-to-8bit (ORCPT ); Wed, 29 Nov 2017 14:34:13 -0500 Received: from mx0a-0016f401.pphosted.com ([67.231.148.174]:54154 "EHLO mx0b-0016f401.pphosted.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1751254AbdK2TeL (ORCPT ); Wed, 29 Nov 2017 14:34:11 -0500 From: Yan Markman To: Russell King , Antoine Tenart CC: "andrew@lunn.ch" , "f.fainelli@gmail.com" , "davem@davemloft.net" , "gregory.clement@free-electrons.com" , "thomas.petazzoni@free-electrons.com" , "miquel.raynal@free-electrons.com" , "Nadav Haklai" , "mw@semihalf.com" , "Stefan Chulski" , "netdev@vger.kernel.org" , "linux-kernel@vger.kernel.org" Subject: RE: [EXT] Re: [PATCH net] net: phylink: fix link state on phy-connect Thread-Topic: [EXT] Re: [PATCH net] net: phylink: fix link state on phy-connect Thread-Index: AQHTaE0J1bz47nEl5EqXJeKzEHOhkKMpz+GAgAAA0ICAAeUZ0A== Date: Wed, 29 Nov 2017 19:33:44 +0000 Message-ID: <20448667430e434aad5bb8cd1b082611@IL-EXCH01.marvell.com> References: <20171128132932.27196-1-antoine.tenart@free-electrons.com> <20171128155317.GA7974@flint.armlinux.org.uk> <20171128155611.GA8358@flint.armlinux.org.uk> In-Reply-To: <20171128155611.GA8358@flint.armlinux.org.uk> Accept-Language: en-US Content-Language: en-US X-MS-Has-Attach: X-MS-TNEF-Correlator: x-ms-exchange-transport-fromentityheader: Hosted x-originating-ip: [10.5.102.207] Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 8BIT MIME-Version: 1.0 X-Proofpoint-Virus-Version: vendor=fsecure engine=2.50.10432:,, definitions=2017-11-29_07:,, signatures=0 X-Proofpoint-Details: rule=outbound_notspam policy=outbound score=0 priorityscore=1501 malwarescore=0 suspectscore=0 phishscore=0 bulkscore=0 spamscore=0 clxscore=1011 lowpriorityscore=0 impostorscore=0 adultscore=0 classifier=spam adjust=0 reason=mlx scancount=1 engine=8.0.1-1709140000 definitions=main-1711290252 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 Cc: andrew@lunn.ch; f.fainelli@gmail.com; davem@davemloft.net; Yan Markman ; gregory.clement@free-electrons.com; thomas.petazzoni@free-electrons.com; miquel.raynal@free-electrons.com; Nadav Haklai ; mw@semihalf.com; Stefan Chulski ; 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: > > From: Yan Markman > > 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 > > [Antoine: commit message] > > Signed-off-by: Antoine Tenart > > --- > > 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