Re: [PATCH NET V5 2/2] net: hns: Use phy_driver to setup Phy loopback

[Yunsheng Lin]

Hi, Andrew

On 2017/7/1 23:17, Andrew Lunn wrote:
> On Sat, Jul 01, 2017 at 11:57:32AM +0000, linyunsheng wrote:
>> Hi, Andrew
>>
>> I am agreed wih you on this.
>> But self test is also a feature of our product, and our
>> customer way choose to diagnose a problem using
>> self test, even if self test does not give a clear
>> reason to the problem.
>> we don't want to remove a feature that we don't
>> know when our customer will be using.
> 
> Far enough. So please take a close look at the code and try to fix
> it. The corner cases are your problem, a down'ed interface, WOL, etc.
> It is issues like this which can result in phy_resume() being called
> without there first being a phy_suspend.
> 
I looked into how the phy core deal with down'ed interface, WOL problem,
here is what I found:
1.phydev->state is used to track the state of the phy.
2.phy_start/stop and phy_state_machine work together to make sure the
  phydev->state is consistent with phydev->suspended.

And using phy_start/stop instead of phy_resume/suspend should take care
of down'ed interface problem.
Will using phy_start/stop cause other problems?

As for WOL,
phy_state_machine:
	if (needs_aneg)
		err = phy_start_aneg_priv(phydev, false);
	else if (do_suspend)
		phy_suspend(phydev);

I think the phy core also have the same problem, because above code does
not put the phy into suspending when it is WOL'ed, and it do not check the
return value of phy_suspend.

I hope I am not missing something obvious.
Please let me know if you have any idea about WOL problem, thanks.

Best Regards
Yunsheng Lin