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

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:

quoted

On Thu, Nov 30, 2017 at 08:51:21AM +0000, Yan Markman wrote:

quoted

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?

-- 
regards,
-grygorii