[PATCH] phy state machine: failsafe leave invalid RUNNING state

[PATCH] phy state machine: failsafe leave invalid RUNNING state

[Zefir Kurtisi]

While in RUNNING state, phy_state_machine() checks for link changes by
comparing phydev->link before and after calling phy_read_status().
This works as long as it is guaranteed that phydev->link is never
changed outside the phy_state_machine().

If in some setups this happens, it causes the state machine to miss
a link loss and remain RUNNING despite phydev->link being 0.

This has been observed running a dsa setup with a process continuously
polling the link states over ethtool each second (SNMPD RFC-1213
agent). Disconnecting the link on a phy followed by a ETHTOOL_GSET
causes dsa_slave_get_settings() / dsa_slave_get_link_ksettings() to
call phy_read_status() and with that modify the link status - and
with that bricking the phy state machine.

This patch adds a fail-safe check while in RUNNING, which causes to
move to CHANGELINK when the link is gone and we are still RUNNING.

Signed-off-by: Zefir Kurtisi <zefir.kurtisi@neratec.com>
---
 drivers/net/phy/phy.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index 28548af..0f9a61e 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c
@@ -966,6 +966,15 @@ void phy_state_machine(struct work_struct *work)
 			if (old_link != phydev->link)
 				phydev->state = PHY_CHANGELINK;
 		}
+		/*
+		 * Failsafe: check that nobody set phydev->link=0 between two
+		 * poll cycles, otherwise we won't leave RUNNING state as long
+		 * as link remains down.
+		 */
+		if (!phydev->link && phydev->state == PHY_RUNNING) {
+			phydev->state = PHY_CHANGELINK;
+			dev_warn(&phydev->dev, "no link in PHY_RUNNING\n");
+		}
 		break;
 	case PHY_CHANGELINK:
 		err = phy_read_status(phydev);
-- 
2.7.4

Re: [PATCH] phy state machine: failsafe leave invalid RUNNING state

[Florian Fainelli]

On 01/04/2017 07:04 AM, Zefir Kurtisi wrote:
> While in RUNNING state, phy_state_machine() checks for link changes by
> comparing phydev->link before and after calling phy_read_status().
> This works as long as it is guaranteed that phydev->link is never
> changed outside the phy_state_machine().
> 
> If in some setups this happens, it causes the state machine to miss
> a link loss and remain RUNNING despite phydev->link being 0.
> 
> This has been observed running a dsa setup with a process continuously
> polling the link states over ethtool each second (SNMPD RFC-1213
> agent). Disconnecting the link on a phy followed by a ETHTOOL_GSET
> causes dsa_slave_get_settings() / dsa_slave_get_link_ksettings() to
> call phy_read_status() and with that modify the link status - and
> with that bricking the phy state machine.

That's the interesting part of the analysis, how does this brick the PHY
state machine? Is the PHY driver changing the link status in the
read_status callback that it implements?

> 
> This patch adds a fail-safe check while in RUNNING, which causes to
> move to CHANGELINK when the link is gone and we are still RUNNING.
> 
> Signed-off-by: Zefir Kurtisi <zefir.kurtisi@neratec.com>
> ---
>  drivers/net/phy/phy.c | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
> index 28548af..0f9a61e 100644
> --- a/drivers/net/phy/phy.c
> +++ b/drivers/net/phy/phy.c
> @@ -966,6 +966,15 @@ void phy_state_machine(struct work_struct *work)
>  			if (old_link != phydev->link)
>  				phydev->state = PHY_CHANGELINK;
>  		}
> +		/*
> +		 * Failsafe: check that nobody set phydev->link=0 between two
> +		 * poll cycles, otherwise we won't leave RUNNING state as long
> +		 * as link remains down.
> +		 */
> +		if (!phydev->link && phydev->state == PHY_RUNNING) {
> +			phydev->state = PHY_CHANGELINK;
> +			dev_warn(&phydev->dev, "no link in PHY_RUNNING\n");
> +		}
>  		break;
>  	case PHY_CHANGELINK:
>  		err = phy_read_status(phydev);
> 

-- 
Florian

Re: [PATCH] phy state machine: failsafe leave invalid RUNNING state

[Zefir Kurtisi]

On 01/04/2017 04:13 PM, Florian Fainelli wrote:
> 
> 
> On 01/04/2017 07:04 AM, Zefir Kurtisi wrote:
>> While in RUNNING state, phy_state_machine() checks for link changes by
>> comparing phydev->link before and after calling phy_read_status().
>> This works as long as it is guaranteed that phydev->link is never
>> changed outside the phy_state_machine().
>>
>> If in some setups this happens, it causes the state machine to miss
>> a link loss and remain RUNNING despite phydev->link being 0.
>>
>> This has been observed running a dsa setup with a process continuously
>> polling the link states over ethtool each second (SNMPD RFC-1213
>> agent). Disconnecting the link on a phy followed by a ETHTOOL_GSET
>> causes dsa_slave_get_settings() / dsa_slave_get_link_ksettings() to
>> call phy_read_status() and with that modify the link status - and
>> with that bricking the phy state machine.
> 
> That's the interesting part of the analysis, how does this brick the PHY
> state machine? Is the PHY driver changing the link status in the
> read_status callback that it implements?
> 
phydev->read_status points to genphy_read_status(), where the first call goes to
genphy_update_link() which updates the link status.

Thereafter phy_state_machine():RUNNING won't be able to detect the link loss
anymore unless the link state changes again.


I was trying to figure out if there is a rule that forbids changing phydev->link
from outside the state machine, but found several places where it happens (either
directly, or over genphy_read_status() or over genphy_update_link()).

Curious how this did not show up before, since within the dsa setup it is very
easy to trigger:
a) physically disconnect link
b) within one second run ethtool ethX


Cheers,
Zefir

Re: [PATCH] phy state machine: failsafe leave invalid RUNNING state

[Florian Fainelli]

On 01/04/2017 07:27 AM, Zefir Kurtisi wrote:
> On 01/04/2017 04:13 PM, Florian Fainelli wrote:
>>
>>
>> On 01/04/2017 07:04 AM, Zefir Kurtisi wrote:
>>> While in RUNNING state, phy_state_machine() checks for link changes by
>>> comparing phydev->link before and after calling phy_read_status().
>>> This works as long as it is guaranteed that phydev->link is never
>>> changed outside the phy_state_machine().
>>>
>>> If in some setups this happens, it causes the state machine to miss
>>> a link loss and remain RUNNING despite phydev->link being 0.
>>>
>>> This has been observed running a dsa setup with a process continuously
>>> polling the link states over ethtool each second (SNMPD RFC-1213
>>> agent). Disconnecting the link on a phy followed by a ETHTOOL_GSET
>>> causes dsa_slave_get_settings() / dsa_slave_get_link_ksettings() to
>>> call phy_read_status() and with that modify the link status - and
>>> with that bricking the phy state machine.
>>
>> That's the interesting part of the analysis, how does this brick the PHY
>> state machine? Is the PHY driver changing the link status in the
>> read_status callback that it implements?
>>
> phydev->read_status points to genphy_read_status(), where the first call goes to
> genphy_update_link() which updates the link status.
> 
> Thereafter phy_state_machine():RUNNING won't be able to detect the link loss
> anymore unless the link state changes again.
> 
> 
> I was trying to figure out if there is a rule that forbids changing phydev->link
> from outside the state machine, but found several places where it happens (either
> directly, or over genphy_read_status() or over genphy_update_link()).
> 
> Curious how this did not show up before, since within the dsa setup it is very
> easy to trigger:
> a) physically disconnect link
> b) within one second run ethtool ethX

You need to be more specific here about what "the dsa setup" is, drivers
involved, which ports of the switch you are seeing this with (user
facing, CPU port, DSA port?) etc.
-- 
Florian

Re: [PATCH] phy state machine: failsafe leave invalid RUNNING state

[Zefir Kurtisi]

On 01/04/2017 04:30 PM, Florian Fainelli wrote:
> 
> 
> On 01/04/2017 07:27 AM, Zefir Kurtisi wrote:
>> On 01/04/2017 04:13 PM, Florian Fainelli wrote:
>>>
>>>
>>> On 01/04/2017 07:04 AM, Zefir Kurtisi wrote:
>>>> While in RUNNING state, phy_state_machine() checks for link changes by
>>>> comparing phydev->link before and after calling phy_read_status().
>>>> This works as long as it is guaranteed that phydev->link is never
>>>> changed outside the phy_state_machine().
>>>>
>>>> If in some setups this happens, it causes the state machine to miss
>>>> a link loss and remain RUNNING despite phydev->link being 0.
>>>>
>>>> This has been observed running a dsa setup with a process continuously
>>>> polling the link states over ethtool each second (SNMPD RFC-1213
>>>> agent). Disconnecting the link on a phy followed by a ETHTOOL_GSET
>>>> causes dsa_slave_get_settings() / dsa_slave_get_link_ksettings() to
>>>> call phy_read_status() and with that modify the link status - and
>>>> with that bricking the phy state machine.
>>>
>>> That's the interesting part of the analysis, how does this brick the PHY
>>> state machine? Is the PHY driver changing the link status in the
>>> read_status callback that it implements?
>>>
>> phydev->read_status points to genphy_read_status(), where the first call goes to
>> genphy_update_link() which updates the link status.
>>
>> Thereafter phy_state_machine():RUNNING won't be able to detect the link loss
>> anymore unless the link state changes again.
>>
>>
>> I was trying to figure out if there is a rule that forbids changing phydev->link
>> from outside the state machine, but found several places where it happens (either
>> directly, or over genphy_read_status() or over genphy_update_link()).
>>
>> Curious how this did not show up before, since within the dsa setup it is very
>> easy to trigger:
>> a) physically disconnect link
>> b) within one second run ethtool ethX
> 
> You need to be more specific here about what "the dsa setup" is, drivers
> involved, which ports of the switch you are seeing this with (user
> facing, CPU port, DSA port?) etc.
> 
I am working on top of LEDE and with that at kernel 4.4.21 - alas I checked the
related source files and believe the effect should be reproducible with HEAD.

The setup is as follows:
mv88e6321:
* ports 0+1 connected to fibre-optics transceivers at fixed 100 Mbps
* port 4 is CPU port
* custom phy driver (replacement for marvell.ko) only populated with
  * .config_init to
    * set fixed speed for ports 0+1 (when in FO mode)
    * run genphy_config_init() for all other modes (here: CPU port)
  * .config_aneg=genphy_config_aneg, .read_status=genphy_read_status


To my understanding, the exact setup is irrelevant - to reproduce the issue it is
enough to have a means of running genphy_update_link() (as done in e.g.
mediatek/mtk_eth_soc.c, dsa/slave.c), or genphy_read_status() (as done in e.g.
hisilicon/hns/hns_enet.c) or phy_read_status() (as done in e.g.
ethernet/ti/netcp_ethss.c, ethernet/aeroflex/greth.c, etc.). In the observed
drivers it is mostly implemented in the ETHTOOL_GSET execution path.

Once you get the link state updated outside the phy state machine, it remains in
invalid RUNNING. To prevent that invalid state, to my understanding upper layer
drivers (Ethernet, dsa) must not modify link-states in any case (including calling
the functions noted above), or we need the proposed fail-safe mechanism to prevent
getting stuck.


Thanks,
Zefir

Re: [PATCH] phy state machine: failsafe leave invalid RUNNING state

[Andrew Lunn]

> The setup is as follows:
> mv88e6321:
> * ports 0+1 connected to fibre-optics transceivers at fixed 100 Mbps
> * port 4 is CPU port
> * custom phy driver (replacement for marvell.ko) only populated with
>   * .config_init to
>     * set fixed speed for ports 0+1 (when in FO mode)
>     * run genphy_config_init() for all other modes (here: CPU port)
>   * .config_aneg=genphy_config_aneg, .read_status=genphy_read_status

Kicking off a side discussion:

Why do a custom PHY driver? What cannot you do with the current DSA
code? I've got boards with two FO ports, and using fixed-phy is all i
need to make them work on a 6352.

     Andrew

[SIDE DISCUSSION] Re: [PATCH] phy state machine: failsafe leave invalid RUNNING state

[Matthias May]

On 04/01/17 17:16, Andrew Lunn wrote:
>> The setup is as follows:
>> mv88e6321:
>> * ports 0+1 connected to fibre-optics transceivers at fixed 100 Mbps
>> * port 4 is CPU port
>> * custom phy driver (replacement for marvell.ko) only populated with
>>   * .config_init to
>>     * set fixed speed for ports 0+1 (when in FO mode)
>>     * run genphy_config_init() for all other modes (here: CPU port)
>>   * .config_aneg=genphy_config_aneg, .read_status=genphy_read_status
> 
> Kicking off a side discussion:
> 
> Why do a custom PHY driver? What cannot you do with the current DSA
> code? I've got boards with two FO ports, and using fixed-phy is all i
> need to make them work on a 6352.
> 
>      Andrew
> 

We make two FO boards for 100Mbps and Gbit with different transceivers.
These different transceivers need each their own drive strength.
As Zefir wrote it's basically just a remap to the genphy functions with
some additional hardware specific register writes when the POR values
for FO configuration are detected on a port.

BR
Matthias

Re: [PATCH] phy state machine: failsafe leave invalid RUNNING state

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 1422 bytes --]

Hi Zefir,

[auto build test ERROR on net-next/master]
[also build test ERROR on v4.10-rc2 next-20170104]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Zefir-Kurtisi/phy-state-machine-failsafe-leave-invalid-RUNNING-state/20170105-033018
config: x86_64-kexec (attached as .config)
compiler: gcc-6 (Debian 6.2.0-3) 6.2.0 20160901
reproduce:
        # save the attached .config to linux build tree
        make ARCH=x86_64 

All errors (new ones prefixed by >>):

   drivers/net/phy/phy.c: In function 'phy_state_machine':
>> drivers/net/phy/phy.c:1075:20: error: 'struct phy_device' has no member named 'dev'; did you mean 'drv'?
       dev_warn(&phydev->dev, "no link in PHY_RUNNING\n");
                       ^~

vim +1075 drivers/net/phy/phy.c

  1069			 * Failsafe: check that nobody set phydev->link=0 between two
  1070			 * poll cycles, otherwise we won't leave RUNNING state as long
  1071			 * as link remains down.
  1072			 */
  1073			if (!phydev->link && phydev->state == PHY_RUNNING) {
  1074				phydev->state = PHY_CHANGELINK;
> 1075				dev_warn(&phydev->dev, "no link in PHY_RUNNING\n");
  1076			}
  1077			break;
  1078		case PHY_CHANGELINK:

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 24448 bytes --]

Re: [PATCH] phy state machine: failsafe leave invalid RUNNING state

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 1404 bytes --]

Hi Zefir,

[auto build test ERROR on net-next/master]
[also build test ERROR on v4.10-rc2 next-20170104]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Zefir-Kurtisi/phy-state-machine-failsafe-leave-invalid-RUNNING-state/20170105-033018
config: i386-randconfig-i1-201701 (attached as .config)
compiler: gcc-4.8 (Debian 4.8.4-1) 4.8.4
reproduce:
        # save the attached .config to linux build tree
        make ARCH=i386 

All errors (new ones prefixed by >>):

   drivers/net/phy/phy.c: In function 'phy_state_machine':
>> drivers/net/phy/phy.c:1075:20: error: 'struct phy_device' has no member named 'dev'
       dev_warn(&phydev->dev, "no link in PHY_RUNNING\n");
                       ^

vim +1075 drivers/net/phy/phy.c

  1069			 * Failsafe: check that nobody set phydev->link=0 between two
  1070			 * poll cycles, otherwise we won't leave RUNNING state as long
  1071			 * as link remains down.
  1072			 */
  1073			if (!phydev->link && phydev->state == PHY_RUNNING) {
  1074				phydev->state = PHY_CHANGELINK;
> 1075				dev_warn(&phydev->dev, "no link in PHY_RUNNING\n");
  1076			}
  1077			break;
  1078		case PHY_CHANGELINK:

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 29567 bytes --]

Re: [PATCH] phy state machine: failsafe leave invalid RUNNING state

[Florian Fainelli]

On 01/04/2017 08:10 AM, Zefir Kurtisi wrote:
> On 01/04/2017 04:30 PM, Florian Fainelli wrote:
>>
>>
>> On 01/04/2017 07:27 AM, Zefir Kurtisi wrote:
>>> On 01/04/2017 04:13 PM, Florian Fainelli wrote:
>>>>
>>>>
>>>> On 01/04/2017 07:04 AM, Zefir Kurtisi wrote:
>>>>> While in RUNNING state, phy_state_machine() checks for link changes by
>>>>> comparing phydev->link before and after calling phy_read_status().
>>>>> This works as long as it is guaranteed that phydev->link is never
>>>>> changed outside the phy_state_machine().
>>>>>
>>>>> If in some setups this happens, it causes the state machine to miss
>>>>> a link loss and remain RUNNING despite phydev->link being 0.
>>>>>
>>>>> This has been observed running a dsa setup with a process continuously
>>>>> polling the link states over ethtool each second (SNMPD RFC-1213
>>>>> agent). Disconnecting the link on a phy followed by a ETHTOOL_GSET
>>>>> causes dsa_slave_get_settings() / dsa_slave_get_link_ksettings() to
>>>>> call phy_read_status() and with that modify the link status - and
>>>>> with that bricking the phy state machine.
>>>>
>>>> That's the interesting part of the analysis, how does this brick the PHY
>>>> state machine? Is the PHY driver changing the link status in the
>>>> read_status callback that it implements?
>>>>
>>> phydev->read_status points to genphy_read_status(), where the first call goes to
>>> genphy_update_link() which updates the link status.
>>>
>>> Thereafter phy_state_machine():RUNNING won't be able to detect the link loss
>>> anymore unless the link state changes again.
>>>
>>>
>>> I was trying to figure out if there is a rule that forbids changing phydev->link
>>> from outside the state machine, but found several places where it happens (either
>>> directly, or over genphy_read_status() or over genphy_update_link()).
>>>
>>> Curious how this did not show up before, since within the dsa setup it is very
>>> easy to trigger:
>>> a) physically disconnect link
>>> b) within one second run ethtool ethX
>>
>> You need to be more specific here about what "the dsa setup" is, drivers
>> involved, which ports of the switch you are seeing this with (user
>> facing, CPU port, DSA port?) etc.
>>
> I am working on top of LEDE and with that at kernel 4.4.21 - alas I checked the
> related source files and believe the effect should be reproducible with HEAD.
> 
> The setup is as follows:
> mv88e6321:
> * ports 0+1 connected to fibre-optics transceivers at fixed 100 Mbps
> * port 4 is CPU port
> * custom phy driver (replacement for marvell.ko) only populated with
>   * .config_init to
>     * set fixed speed for ports 0+1 (when in FO mode)
>     * run genphy_config_init() for all other modes (here: CPU port)
>   * .config_aneg=genphy_config_aneg, .read_status=genphy_read_status
> 
> 
> To my understanding, the exact setup is irrelevant - to reproduce the issue it is
> enough to have a means of running genphy_update_link() (as done in e.g.
> mediatek/mtk_eth_soc.c, dsa/slave.c), or genphy_read_status() (as done in e.g.
> hisilicon/hns/hns_enet.c) or phy_read_status() (as done in e.g.
> ethernet/ti/netcp_ethss.c, ethernet/aeroflex/greth.c, etc.). In the observed
> drivers it is mostly implemented in the ETHTOOL_GSET execution path.
> 
> Once you get the link state updated outside the phy state machine, it remains in
> invalid RUNNING. To prevent that invalid state, to my understanding upper layer
> drivers (Ethernet, dsa) must not modify link-states in any case (including calling
> the functions noted above), or we need the proposed fail-safe mechanism to prevent
> getting stuck.

OK, I see the code path involved now, sorry -ENOCOFFEE when I initially
responded. Yes, clearly, we should not be mangling the PHY device's link
by calling genphy_read_status(). At first glance, none of the users
below should be doing what they are doing, but let's kick a separate
patch series to collect feedback from the driver writes.

Thanks!
-- 
Florian

Re: [PATCH] phy state machine: failsafe leave invalid RUNNING state

[Zefir Kurtisi]

On 01/04/2017 10:44 PM, Florian Fainelli wrote:
> On 01/04/2017 08:10 AM, Zefir Kurtisi wrote:
>> On 01/04/2017 04:30 PM, Florian Fainelli wrote:
>>>
>>>
>>> On 01/04/2017 07:27 AM, Zefir Kurtisi wrote:
>>>> On 01/04/2017 04:13 PM, Florian Fainelli wrote:
>>>>>
>>>>>
>>>>> On 01/04/2017 07:04 AM, Zefir Kurtisi wrote:
>>>>>> While in RUNNING state, phy_state_machine() checks for link changes by
>>>>>> comparing phydev->link before and after calling phy_read_status().
>>>>>> This works as long as it is guaranteed that phydev->link is never
>>>>>> changed outside the phy_state_machine().
>>>>>>
>>>>>> If in some setups this happens, it causes the state machine to miss
>>>>>> a link loss and remain RUNNING despite phydev->link being 0.
>>>>>>
>>>>>> This has been observed running a dsa setup with a process continuously
>>>>>> polling the link states over ethtool each second (SNMPD RFC-1213
>>>>>> agent). Disconnecting the link on a phy followed by a ETHTOOL_GSET
>>>>>> causes dsa_slave_get_settings() / dsa_slave_get_link_ksettings() to
>>>>>> call phy_read_status() and with that modify the link status - and
>>>>>> with that bricking the phy state machine.
>>>>>
>>>>> That's the interesting part of the analysis, how does this brick the PHY
>>>>> state machine? Is the PHY driver changing the link status in the
>>>>> read_status callback that it implements?
>>>>>
>>>> phydev->read_status points to genphy_read_status(), where the first call goes to
>>>> genphy_update_link() which updates the link status.
>>>>
>>>> Thereafter phy_state_machine():RUNNING won't be able to detect the link loss
>>>> anymore unless the link state changes again.
>>>>
>>>>
>>>> I was trying to figure out if there is a rule that forbids changing phydev->link
>>>> from outside the state machine, but found several places where it happens (either
>>>> directly, or over genphy_read_status() or over genphy_update_link()).
>>>>
>>>> Curious how this did not show up before, since within the dsa setup it is very
>>>> easy to trigger:
>>>> a) physically disconnect link
>>>> b) within one second run ethtool ethX
>>>
>>> You need to be more specific here about what "the dsa setup" is, drivers
>>> involved, which ports of the switch you are seeing this with (user
>>> facing, CPU port, DSA port?) etc.
>>>
>> I am working on top of LEDE and with that at kernel 4.4.21 - alas I checked the
>> related source files and believe the effect should be reproducible with HEAD.
>>
>> The setup is as follows:
>> mv88e6321:
>> * ports 0+1 connected to fibre-optics transceivers at fixed 100 Mbps
>> * port 4 is CPU port
>> * custom phy driver (replacement for marvell.ko) only populated with
>>   * .config_init to
>>     * set fixed speed for ports 0+1 (when in FO mode)
>>     * run genphy_config_init() for all other modes (here: CPU port)
>>   * .config_aneg=genphy_config_aneg, .read_status=genphy_read_status
>>
>>
>> To my understanding, the exact setup is irrelevant - to reproduce the issue it is
>> enough to have a means of running genphy_update_link() (as done in e.g.
>> mediatek/mtk_eth_soc.c, dsa/slave.c), or genphy_read_status() (as done in e.g.
>> hisilicon/hns/hns_enet.c) or phy_read_status() (as done in e.g.
>> ethernet/ti/netcp_ethss.c, ethernet/aeroflex/greth.c, etc.). In the observed
>> drivers it is mostly implemented in the ETHTOOL_GSET execution path.
>>
>> Once you get the link state updated outside the phy state machine, it remains in
>> invalid RUNNING. To prevent that invalid state, to my understanding upper layer
>> drivers (Ethernet, dsa) must not modify link-states in any case (including calling
>> the functions noted above), or we need the proposed fail-safe mechanism to prevent
>> getting stuck.
> 
> OK, I see the code path involved now, sorry -ENOCOFFEE when I initially
> responded. Yes, clearly, we should not be mangling the PHY device's link
> by calling genphy_read_status(). At first glance, none of the users
> below should be doing what they are doing, but let's kick a separate
> patch series to collect feedback from the driver writes.
> 
> Thanks!
> 
Ok, thanks for taking time.

The kbuild test robot error is due to 'struct device dev' been removed from
phy_device struct since 4.4.21. Does it make sense to provide a v2 fixing that, or
do you expect that this fail-safe mechanism is not needed once all Ethernet/dsa
drivers are fixed?

I think it won't hurt to add the check simply to ensure that it got fixed and the
issue is not popping up thereafter.


Cheers,
Zefir

Re: [PATCH] phy state machine: failsafe leave invalid RUNNING state

[Florian Fainelli]

On 01/05/2017 01:23 AM, Zefir Kurtisi wrote:
> On 01/04/2017 10:44 PM, Florian Fainelli wrote:
>> On 01/04/2017 08:10 AM, Zefir Kurtisi wrote:
>>> On 01/04/2017 04:30 PM, Florian Fainelli wrote:
>>>>
>>>>
>>>> On 01/04/2017 07:27 AM, Zefir Kurtisi wrote:
>>>>> On 01/04/2017 04:13 PM, Florian Fainelli wrote:
>>>>>>
>>>>>>
>>>>>> On 01/04/2017 07:04 AM, Zefir Kurtisi wrote:
>>>>>>> While in RUNNING state, phy_state_machine() checks for link changes by
>>>>>>> comparing phydev->link before and after calling phy_read_status().
>>>>>>> This works as long as it is guaranteed that phydev->link is never
>>>>>>> changed outside the phy_state_machine().
>>>>>>>
>>>>>>> If in some setups this happens, it causes the state machine to miss
>>>>>>> a link loss and remain RUNNING despite phydev->link being 0.
>>>>>>>
>>>>>>> This has been observed running a dsa setup with a process continuously
>>>>>>> polling the link states over ethtool each second (SNMPD RFC-1213
>>>>>>> agent). Disconnecting the link on a phy followed by a ETHTOOL_GSET
>>>>>>> causes dsa_slave_get_settings() / dsa_slave_get_link_ksettings() to
>>>>>>> call phy_read_status() and with that modify the link status - and
>>>>>>> with that bricking the phy state machine.
>>>>>>
>>>>>> That's the interesting part of the analysis, how does this brick the PHY
>>>>>> state machine? Is the PHY driver changing the link status in the
>>>>>> read_status callback that it implements?
>>>>>>
>>>>> phydev->read_status points to genphy_read_status(), where the first call goes to
>>>>> genphy_update_link() which updates the link status.
>>>>>
>>>>> Thereafter phy_state_machine():RUNNING won't be able to detect the link loss
>>>>> anymore unless the link state changes again.
>>>>>
>>>>>
>>>>> I was trying to figure out if there is a rule that forbids changing phydev->link
>>>>> from outside the state machine, but found several places where it happens (either
>>>>> directly, or over genphy_read_status() or over genphy_update_link()).
>>>>>
>>>>> Curious how this did not show up before, since within the dsa setup it is very
>>>>> easy to trigger:
>>>>> a) physically disconnect link
>>>>> b) within one second run ethtool ethX
>>>>
>>>> You need to be more specific here about what "the dsa setup" is, drivers
>>>> involved, which ports of the switch you are seeing this with (user
>>>> facing, CPU port, DSA port?) etc.
>>>>
>>> I am working on top of LEDE and with that at kernel 4.4.21 - alas I checked the
>>> related source files and believe the effect should be reproducible with HEAD.
>>>
>>> The setup is as follows:
>>> mv88e6321:
>>> * ports 0+1 connected to fibre-optics transceivers at fixed 100 Mbps
>>> * port 4 is CPU port
>>> * custom phy driver (replacement for marvell.ko) only populated with
>>>   * .config_init to
>>>     * set fixed speed for ports 0+1 (when in FO mode)
>>>     * run genphy_config_init() for all other modes (here: CPU port)
>>>   * .config_aneg=genphy_config_aneg, .read_status=genphy_read_status
>>>
>>>
>>> To my understanding, the exact setup is irrelevant - to reproduce the issue it is
>>> enough to have a means of running genphy_update_link() (as done in e.g.
>>> mediatek/mtk_eth_soc.c, dsa/slave.c), or genphy_read_status() (as done in e.g.
>>> hisilicon/hns/hns_enet.c) or phy_read_status() (as done in e.g.
>>> ethernet/ti/netcp_ethss.c, ethernet/aeroflex/greth.c, etc.). In the observed
>>> drivers it is mostly implemented in the ETHTOOL_GSET execution path.
>>>
>>> Once you get the link state updated outside the phy state machine, it remains in
>>> invalid RUNNING. To prevent that invalid state, to my understanding upper layer
>>> drivers (Ethernet, dsa) must not modify link-states in any case (including calling
>>> the functions noted above), or we need the proposed fail-safe mechanism to prevent
>>> getting stuck.
>>
>> OK, I see the code path involved now, sorry -ENOCOFFEE when I initially
>> responded. Yes, clearly, we should not be mangling the PHY device's link
>> by calling genphy_read_status(). At first glance, none of the users
>> below should be doing what they are doing, but let's kick a separate
>> patch series to collect feedback from the driver writes.
>>
>> Thanks!
>>
> Ok, thanks for taking time.
> 
> The kbuild test robot error is due to 'struct device dev' been removed from
> phy_device struct since 4.4.21. Does it make sense to provide a v2 fixing that, or
> do you expect that this fail-safe mechanism is not needed once all Ethernet/dsa
> drivers are fixed?

I think there is value in identifying wrong behaving drivers while we
fix them one after the other.

> 
> I think it won't hurt to add the check simply to ensure that it got fixed and the
> issue is not popping up thereafter.

Agreed, can you resubmit against the latest net-next/master tree?

Thanks!
-- 
Florian

[PATCH v2] phy state machine: failsafe leave invalid RUNNING state

[Zefir Kurtisi]

While in RUNNING state, phy_state_machine() checks for link changes by
comparing phydev->link before and after calling phy_read_status().
This works as long as it is guaranteed that phydev->link is never
changed outside the phy_state_machine().

If in some setups this happens, it causes the state machine to miss
a link loss and remain RUNNING despite phydev->link being 0.

This has been observed running a dsa setup with a process continuously
polling the link states over ethtool each second (SNMPD RFC-1213
agent). Disconnecting the link on a phy followed by a ETHTOOL_GSET
causes dsa_slave_get_settings() / dsa_slave_get_link_ksettings() to
call phy_read_status() and with that modify the link status - and
with that bricking the phy state machine.

This patch adds a fail-safe check while in RUNNING, which causes to
move to CHANGELINK when the link is gone and we are still RUNNING.

Signed-off-by: Zefir Kurtisi <zefir.kurtisi@neratec.com>
---
Changes to v1:
* fix kbuild test robot error: use phydev_err instead of dev_warn
  (adapt to changed struct phy_device after 4.4.21)
---
 drivers/net/phy/phy.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index 25f93a9..48da6e9 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c
@@ -1065,6 +1065,15 @@ void phy_state_machine(struct work_struct *work)
 			if (old_link != phydev->link)
 				phydev->state = PHY_CHANGELINK;
 		}
+		/*
+		 * Failsafe: check that nobody set phydev->link=0 between two
+		 * poll cycles, otherwise we won't leave RUNNING state as long
+		 * as link remains down.
+		 */
+		if (!phydev->link && phydev->state == PHY_RUNNING) {
+			phydev->state = PHY_CHANGELINK;
+			phydev_err(phydev, "no link in PHY_RUNNING\n");
+		}
 		break;
 	case PHY_CHANGELINK:
 		err = phy_read_status(phydev);
-- 
2.7.4

Re: [PATCH v2] phy state machine: failsafe leave invalid RUNNING state

[David Miller]

From: Zefir Kurtisi <zefir.kurtisi@neratec.com>
Date: Fri,  6 Jan 2017 12:14:48 +0100

> While in RUNNING state, phy_state_machine() checks for link changes by
> comparing phydev->link before and after calling phy_read_status().
> This works as long as it is guaranteed that phydev->link is never
> changed outside the phy_state_machine().
> 
> If in some setups this happens, it causes the state machine to miss
> a link loss and remain RUNNING despite phydev->link being 0.
> 
> This has been observed running a dsa setup with a process continuously
> polling the link states over ethtool each second (SNMPD RFC-1213
> agent). Disconnecting the link on a phy followed by a ETHTOOL_GSET
> causes dsa_slave_get_settings() / dsa_slave_get_link_ksettings() to
> call phy_read_status() and with that modify the link status - and
> with that bricking the phy state machine.
> 
> This patch adds a fail-safe check while in RUNNING, which causes to
> move to CHANGELINK when the link is gone and we are still RUNNING.
> 
> Signed-off-by: Zefir Kurtisi <zefir.kurtisi@neratec.com>
> ---
> Changes to v1:
> * fix kbuild test robot error: use phydev_err instead of dev_warn
>   (adapt to changed struct phy_device after 4.4.21)

Florian and Andrew, please provide some feedback on this.

Thank you.

Re: [PATCH v2] phy state machine: failsafe leave invalid RUNNING state

[Florian Fainelli]

On 01/06/2017 03:14 AM, Zefir Kurtisi wrote:
> While in RUNNING state, phy_state_machine() checks for link changes by
> comparing phydev->link before and after calling phy_read_status().
> This works as long as it is guaranteed that phydev->link is never
> changed outside the phy_state_machine().
> 
> If in some setups this happens, it causes the state machine to miss
> a link loss and remain RUNNING despite phydev->link being 0.
> 
> This has been observed running a dsa setup with a process continuously
> polling the link states over ethtool each second (SNMPD RFC-1213
> agent). Disconnecting the link on a phy followed by a ETHTOOL_GSET
> causes dsa_slave_get_settings() / dsa_slave_get_link_ksettings() to
> call phy_read_status() and with that modify the link status - and
> with that bricking the phy state machine.
> 
> This patch adds a fail-safe check while in RUNNING, which causes to
> move to CHANGELINK when the link is gone and we are still RUNNING.
> 
> Signed-off-by: Zefir Kurtisi <zefir.kurtisi@neratec.com>

Reviewed-by: Florian Fainelli <f.fainelli@gmail.com>
-- 
Florian

Re: [PATCH v2] phy state machine: failsafe leave invalid RUNNING state

[David Miller]

From: Zefir Kurtisi <zefir.kurtisi@neratec.com>
Date: Fri,  6 Jan 2017 12:14:48 +0100

> While in RUNNING state, phy_state_machine() checks for link changes by
> comparing phydev->link before and after calling phy_read_status().
> This works as long as it is guaranteed that phydev->link is never
> changed outside the phy_state_machine().
> 
> If in some setups this happens, it causes the state machine to miss
> a link loss and remain RUNNING despite phydev->link being 0.
> 
> This has been observed running a dsa setup with a process continuously
> polling the link states over ethtool each second (SNMPD RFC-1213
> agent). Disconnecting the link on a phy followed by a ETHTOOL_GSET
> causes dsa_slave_get_settings() / dsa_slave_get_link_ksettings() to
> call phy_read_status() and with that modify the link status - and
> with that bricking the phy state machine.
> 
> This patch adds a fail-safe check while in RUNNING, which causes to
> move to CHANGELINK when the link is gone and we are still RUNNING.
> 
> Signed-off-by: Zefir Kurtisi <zefir.kurtisi@neratec.com>

Patch applied, thanks.