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

[Grygorii Strashko]

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?

-- 
regards,
-grygorii