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

[Yan Markman]

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.

After calling mac_config() the phylink checks  if (!link  &&  !netif_carrier_ok()) and decides to abort further down since all-done...

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

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;

Thank you
Best regards
Yan Markman

-----Original Message-----
From: Russell King - ARM Linux [mailto:linux@xxxxxxxxxxxxxxx] 
Sent: Friday, December 01, 2017 7:48 PM
To: Florian Fainelli <f.fainelli@xxxxxxxxx>
Cc: Grygorii Strashko <grygorii.strashko@xxxxxx>; Yan Markman <ymarkman@xxxxxxxxxxx>; Antoine Tenart <antoine.tenart@xxxxxxxxxxxxxxxxxx>; andrew@xxxxxxx; davem@xxxxxxxxxxxxx; gregory.clement@xxxxxxxxxxxxxxxxxx; thomas.petazzoni@xxxxxxxxxxxxxxxxxx; miquel.raynal@xxxxxxxxxxxxxxxxxx; Nadav Haklai <nadavh@xxxxxxxxxxx>; mw@xxxxxxxxxxxx; Stefan Chulski <stefanc@xxxxxxxxxxx>; netdev@xxxxxxxxxxxxxxx; linux-kernel@xxxxxxxxxxxxxxx
Subject: Re: [EXT] Re: [PATCH net] net: phylink: fix link state on phy-connect

On Fri, Dec 01, 2017 at 09:36:42AM -0800, Florian Fainelli wrote:
> On 12/01/2017 09:24 AM, Russell King - ARM Linux wrote:
> > On Fri, Dec 01, 2017 at 11:07:22AM -0600, Grygorii Strashko wrote:
> >> Hi Russell,
> >>
> >> On 11/30/2017 07:28 AM, Russell King - ARM Linux wrote:
> >>> On Thu, Nov 30, 2017 at 10:10:18AM +0000, Russell King - ARM Linux wrote:
> >>>> On Thu, Nov 30, 2017 at 08:51:21AM +0000, Yan Markman wrote:
> >>>>> The phylink_stop is called before phylink_disconnect_phy You 
> >>>>> could see in mvpp2.c:
> >>>>>
> >>>>> mvpp2_stop_dev() {
> >>>>> 	phylink_stop(port->phylink);
> >>>>> }
> >>>>>
> >>>>> mvpp2_stop()       {
> >>>>> 	mvpp2_stop_dev(port);
> >>>>> 	phylink_disconnect_phy(port->phylink);
> >>>>> }
> >>>>>
> >>>>> .ndo_stop = mvpp2_stop,
> >>>>
> >>>> Sorry, I don't have this in mvpp2.c, so I have no visibility of 
> >>>> what you're working with.
> >>>>
> >>>> What you have above looks correct, and I see no reason why the 
> >>>> p21 patch would not have resolved your issue.  The p21 patch 
> >>>> ensures that phylink_resolve() gets called and completes before 
> >>>> phylink_stop() returns.  In that case, phylink_resolve() will 
> >>>> call the mac_link_down() method if the link is not already down.  
> >>>> It will also print the "Link is Down" message.
> >>>>
> >>>> Florian has already tested this patch after encountering a 
> >>>> similar issue, and has reported that it solves the problem for 
> >>>> him.  I've also tested it with mvneta, and the original mvpp2x driver on Macchiatobin.
> >>>>
> >>>> Maybe there's something different about mvpp2, but as I have no 
> >>>> visibility of that driver and the modifications therein, I can't 
> >>>> comment further other than stating that it works for three 
> >>>> different implementations.
> >>>>
> >>>> Maybe you could try and work out what's going on with the p21 
> >>>> patch in your case?
> >>>
> >>> I think I now realise what's probably going on.
> >>>
> >>> If you call netif_carrier_off() before phylink_stop(), then 
> >>> phylink will believe that the link is already down, and so it 
> >>> won't bother calling
> >>> mac_link_down() - it will believe that the link is already down.
> >>>
> >>> I'll update the documentation for phylink_stop() to spell out this 
> >>> aspect.
> >>>
> >>
> >> There are pretty high number of net drivers which do call
> >> 	netif_carrier_off(dev);
> >> before
> >> 	phy_stop(dev->phydev);
> >> in .ndo_stop() callback.
> >>
> >> As per you comment this seems to be incorrect, so should such calls 
> >> be removed?
> > 
> > Well, I think the question that needs to be asked is this:
> > 
> >   Is calling netif_carrier_off() before phy_stop() safe?
> > 
> > Well, reading the phylib code, this is the answer I've come to:
> > 
> >   Between phy_start() and phy_stop(), phylib is free to manage the
> >   carrier state itself through the phylib state machine.
> > 
> >   This means if you call netif_carrier_off() prior to phy_stop(),
> >   there is nothing preventing the phylib state machine from running,
> >   and a co-incident poll of the PHY could notice that the link has
> >   come up, and re-enable the carrier while your ndo_stop() method
> >   is still running.
> > 
> > So, my conclusion is that this practice is provably racy, though 
> > it's probably not that easy to trigger the race (which is probably 
> > why no one has reported the problem.)
> > 
> > Given that it's racy, it's not something that I think phylink should 
> > care about, and should "softly" discourage it.  So, I'm happy with 
> > what phylink is doing here, and I suggest fixing the drivers for 
> > this race.
> > 
> > In any case, it should result in less code in the drivers - since 
> > the work you need to do when the link goes down is a subset of the 
> > work you need to do when the network interface is taken down.
> > 
> 
> While I agree with all of what written before, in practice, calling
> netif_carrier_off() when using PHYLIB can cause inconsistent carrier 
> states at most, but it would not be messing the state machine itself 
> because PHYLIB does not make uses of netif_carrier_ok() to make any 
> decisions as whether the link has dropped or not, it bases its 
> information solely on phydev->link.

Indeed, but the point I'm making is that this sequence is very possible with drivers that mess about by fiddling with stuff before they call phy_stop():

	CPU0					CPU1
	netif_carrier_off()
	mvpp2_egress_disable()
						phy_state_machine()
						 (phydev->state = PHY_AN)
						phy_link_up()
						phy_link_change()
						netif_carrier_on()
						mvpp2_link_event()
						mvpp2_egress_enable()
						mvpp2_ingress_enable()
	mvpp2_port_disable()
	phy_stop(ndev->phydev)

At this point, egress has not been disabled as mvpp2_stop_dev() wants, because the phylib state machine got in before it was stopped, called the adjust link function which then had the effect of re-enabling the egress.

If that doesn't matter, then what's the point of the
mvpp2_egress_disable() call in the mvpp2_stop_dev() path... either it matters and the mvpp2_stop_dev() sequence is broken, or it doesn't matter and some the work that mvpp2_stop_dev() is doing is unnecessary.

--
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