Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751894AbdLBO71 (ORCPT ); Sat, 2 Dec 2017 09:59:27 -0500 Received: from pandora.armlinux.org.uk ([78.32.30.218]:35042 "EHLO pandora.armlinux.org.uk" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751745AbdLBO70 (ORCPT ); Sat, 2 Dec 2017 09:59:26 -0500 Date: Sat, 2 Dec 2017 14:58:57 +0000 From: Russell King - ARM Linux To: Yan Markman Cc: Florian Fainelli , Grygorii Strashko , Antoine Tenart , "andrew@lunn.ch" , "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 Message-ID: <20171202145857.GO10595@n2100.armlinux.org.uk> References: <21ec97be76d54a6c8a80fd5b56d35678@IL-EXCH01.marvell.com> <20171129212032.GI8356@n2100.armlinux.org.uk> <20171130101018.GA10595@n2100.armlinux.org.uk> <20171130132830.GA5529@n2100.armlinux.org.uk> <51496048-7e11-e0aa-7c47-3c04eee70e3a@ti.com> <20171201172440.GK10595@n2100.armlinux.org.uk> <221420a4-9f56-373e-f5cd-0d2fcb02e5fb@gmail.com> <20171201174730.GM10595@n2100.armlinux.org.uk> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: User-Agent: Mutt/1.5.23 (2014-03-12) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 6061 Lines: 146 On Sat, Dec 02, 2017 at 11:08:45AM +0000, Yan Markman wrote: > Hi Russel > > The Grygorii has raised one Additional point (about netif_carrier_off) > I just didn't want to start before finishing the previous one. > > On ifconfig-down the mac_config() called but with LINK=0. > The config has no any knowledge what is intention -- up or down and > should be done under disabled ingress/egress, and so the mac_config > one of its action is netif_carrier_off. With the "p21" patch applied, which is now queued for 4.15-rc by davem, the behaviour of phylink when phylink_stop() is called becomes entirely predictable. When phylink_stop() has been called, provided the carrier state is left alone, it is guaranteed that mac_link_down() will be called if the link was originally up, and this will complete prior to phylink_stop() returning. After that call has been made, and provided no further calls from the MAC driver to phylink are made, phylink will make no further calls to the MAC driver via mac_config(), mac_link_up() or mac_link_down(). It will only resume making these calls once phylink_start() is called. phylink_start() will cause mac_config() to be called for the current link mode. A resolve of the current state is then triggered, which may trigger further mac_config() calls to be made. If the link is then deemed to be up, a call to mac_link_up() will be made. > After calling mac_config() the phylink checks > if (!link && !netif_carrier_ok()) > and decides to abort further down since all-done... phylink does not contain any such if () statement, so I'm not sure what code you are referring to. > REMOVE netif_carrier_off looks like correct BUT has cases where de driver stops to works properly (sorry, I can't remember now what exactly). > So finally I have placed there the CONDITIONAL carrier-off depending upon link: > > static void mvpp2_mac_config(){ > if (state->link) --- occasionally is TRUE on UP but FALSE on down > netif_carrier_off(port->dev);//YANM You should not be changing the carrier state in your mac_config() function, because, again, just like having netif_carrier_off() before phylink_stop(), it will mess phylink's tracking of the current state and will cause the mac_link_*() functions to be called erratically. > BTW: It's seems your below patch should be present anyway. > +++ b/drivers/net/phy/phylink.c > @@ -798,6 +798,7 @@ void phylink_disconnect_phy(struct phylink *pl) > + pl->phy_state.link = false; Here's an example without the above on Macchiatobin of a up -> down -> up sequence on the gigabit wired ethernet port on this board (which I have bound to a Linux bridge device). The exact command used for this was: # ifconfig eth2 down; sleep 2; ifconfig eth2 up [66926.127009] mvpp2x f4000000.ppv22 eth2: Link is Down [66926.131557] br0: port 1(eth2) entered disabled state [66928.144845] mvpp2x f4000000.ppv22 eth2: configuring for inband/sgmii link mode [66928.144853] mvpp2x f4000000.ppv22 eth2: reconfig: pm 4->4 cm 201->201 f 2->2 [66928.154937] IPv6: ADDRCONF(NETDEV_UP): eth2: link is not ready [66929.783866] IPv6: ADDRCONF(NETDEV_UP): br0: link is not ready [66929.979499] IPv6: ADDRCONF(NETDEV_UP): br0: link is not ready [66931.213407] mvpp2x f4000000.ppv22 eth2: reconfig: pm 4->4 cm 201->201 f a->a [66931.213424] mvpp2x f4000000.ppv22 eth2: Link is Up - 1Gbps/Full - flow control off [66931.213433] IPv6: ADDRCONF(NETDEV_CHANGE): eth2: link becomes ready [66931.213682] br0: port 1(eth2) entered blocking state [66931.213685] br0: port 1(eth2) entered forwarding state [66931.213920] IPv6: ADDRCONF(NETDEV_CHANGE): br0: link becomes ready This is with the "p21" patch applied, and mvpp2x_the netif_carrier_off() before phylink_stop() in mvpp2x removed. Basically: void mv_pp2x_stop_dev(struct mv_pp2x_port *port) { struct gop_hw *gop = &port->priv->hw.gop; struct mv_mac_data *mac = &port->mac_data; if (port->mac_data.phylink) { phylink_stop(port->mac_data.phylink); /* Disable interrupts on all CPUs */ mv_pp2x_port_interrupts_disable(port); mv_pp2x_port_napi_disable(port); netif_tx_stop_all_queues(port->dev); } else { /* Stop new packets from arriving to RXQs */ mv_pp2x_ingress_disable(port); mdelay(10); /* Disable interrupts on all CPUs */ mv_pp2x_port_interrupts_disable(port); mv_pp2x_port_napi_disable(port); netif_carrier_off(port->dev); netif_tx_stop_all_queues(port->dev); mv_pp2x_egress_disable(port); } if (port->comphy) phy_power_off(port->comphy); if (port->priv->pp2_version == PPV21) { mv_pp21_port_disable(port); } else { mv_gop110_port_events_mask(gop, mac); mv_gop110_port_disable(gop, mac); port->mac_data.flags &= ~MV_EMAC_F_LINK_UP; port->mac_data.flags &= ~MV_EMAC_F_PORT_UP; } if (!port->mac_data.phylink) { if (port->mac_data.phy_dev) phy_stop(port->mac_data.phy_dev); else tasklet_kill(&port->link_change_tasklet); } } with the mac_link_down() being: static void mv_pp22_mac_link_down(struct net_device *dev, unsigned int mode) { struct mv_pp2x_port *port = netdev_priv(dev); port->mac_data.link = 0; mv_pp2x_ingress_disable(port); mv_pp2x_egress_disable(port); port->mac_data.flags &= ~MV_EMAC_F_LINK_UP; } The phylink case is the same, but with the ingress/egress disable in a slightly different position - as it would be if the interface were taken down without the link being up. -- RMK's Patch system: http://www.armlinux.org.uk/developer/patches/ FTTC broadband for 0.8mile line in suburbia: sync at 8.8Mbps down 630kbps up According to speedtest.net: 8.21Mbps down 510kbps up