Re: Problem with PHY state machine when using interrupts

[Florian Fainelli <f.fainelli@gmail.com>]

On 07/24/2017 08:01 AM, Mason wrote:
> On 24/07/2017 13:07, Mason wrote:
> 
>> When I set the link down via 'ip link set eth0 down'
>> (as opposed to pulling the Ethernet cable) things don't happen as expected:
>>
>> The driver's adjust_link() callback is never called, and doesn't
>> get a chance make some required changes. And when I set the link
>> up again, there is no network connectivity.
>>
>> I get this problem only if I enable interrupts on my PHY.
>> If I use polling, things work as expected.
>>
>>
>> When I set the link down, devinet_ioctl() eventually calls
>> ndo_set_rx_mode() and ndo_stop()
>>
>> In ndo_stop() the driver calls
>> phy_stop(phydev);
>> which disables interrupts and sets the state to HALTED.
>>
>> In phy_state_machine()
>> the PHY_HALTED case does call the adjust_link() callback:
>>
>> 		if (phydev->link) {
>> 			phydev->link = 0;
>> 			netif_carrier_off(phydev->attached_dev);
>> 			phy_adjust_link(phydev);
>> 			do_suspend = true;
>> 		}
>>
>> But it's not called when I use interrupts...
>>
>> Perhaps because there are no interrupts generated?
>> Or even if there were, they have been turned off by phy_stop?
>>
>> Basically, it seems like when I use interrupts,
>> the phy_state_machine() is not called on link down,
>> which breaks the MAC driver's expectations.
>>
>> Am I barking up the wrong tree?
> 
> FWIW, the patch below solves my issue.
> Basically, we reset the MAC in open(), instead of probe().
> 
> I also had to solve the issue of adjust_link() not being
> called by calling it explicitly in stop() instead of
> relying on phy_stop() to do it indirectly.

Which is of course absolutely not how it is intended to be used.
phy_stop() does the following:

- if the PHY was already HALTED do nothing and exit
- if it was not and an interrupt is valid for this PHY:  disable and
clear these interrupts
- set state to PHY_HALTED

somehow an interrupt should be generated from doing this such that
phy_change(), invoked from phy_interrupt() should have a chance to run
and make the PHY state machine transition properly to PHY_HALTED.

So from there can you check a few things:

- is such an interrupt actually generated?
- if you turn on dynamic debug prints for drivers/net/phy/phy.c where do
we leave the PHY state machine and what state is it in when you call
ifconfig up again?

> 
> With this code, I think it is easy to handle suspend/resume:
> on suspend, I will stop() and on resume, I will start(),
> and everything should work as expected.
> 
> I'd like to hear comments on the patch, so I can turn it
> into a formal submission.



-- 
Florian