Skip to contentSkip to conversation

net phy: Check for aneg completion before setting state to PHY_RUNNING

PATCH Applied Balakumaran Kannan 3 participants 8 messages Apr 24, 2014 – May 2, 2014

Subsystems (3)
Networking Network drivers Ethernet PHY
lore
Share & tools
lore

Balakumaran Kannan

author
#0 lore raw

PATCH 1/1 net phy: Check for aneg completion before setting state to PHY_RUNNING

phy_state_machine should check whether auto-negotiatin is completed before changing phydev->state from PHY_NOLINK to PHY_RUNNING. If auto-negotiation is not completed phydev->state should be set to PHY_AN.

Signed-off-by Balakumaran Kannan <[email protected]>
---
 drivers/net/phy/phy.c | 11 +++++++++++
 1 file changed, 11 insertions(+)
drivers/net/phy/phy.c +11 -0
index 1b6d09a..a972056 100644
@@ -765,6 +765,17 @@ void phy_state_machine(struct work_struct *work)
 			break;
 
 		if (phydev->link) {
+			if (AUTONEG_ENABLE == phydev->autoneg) {
+				err = phy_aneg_done(phydev);
+				if (err < 0)
+					break;
+
+				if (!err) {
+					phydev->state = PHY_AN;
+					phydev->link_timeout = PHY_AN_TIMEOUT;
+					break;
+				}
+			}
 			phydev->state = PHY_RUNNING;
 			netif_carrier_on(phydev->attached_dev);
 			phydev->adjust_link(phydev->attached_dev);
Signature · 1 line
1.8.2.1
#1 lore raw

2014-04-23 19:52 GMT-07:00 Balakumaran Kannan <[email protected]>:

Quoted 4 lines

phy_state_machine should check whether auto-negotiatin is completed before changing phydev->state from PHY_NOLINK to PHY_RUNNING. If auto-negotiation is not completed phydev->state should be set to PHY_AN.

I need some time to review this. Just out of curiosity, is this fixing a bug you have encountered or was this found by state machine inspection?

Quoted 30 lines
Signed-off-by Balakumaran Kannan <[email protected]>
---
 drivers/net/phy/phy.c | 11 +++++++++++
 1 file changed, 11 insertions(+)
drivers/net/phy/phy.c +11 -1
index 1b6d09a..a972056 100644
@@ -765,6 +765,17 @@ void phy_state_machine(struct work_struct *work)
                        break;
 
                if (phydev->link) {
+                       if (AUTONEG_ENABLE == phydev->autoneg) {
+                               err = phy_aneg_done(phydev);
+                               if (err < 0)
+                                       break;
+
+                               if (!err) {
+                                       phydev->state = PHY_AN;
+                                       phydev->link_timeout = PHY_AN_TIMEOUT;
+                                       break;
+                               }
+                       }
                        phydev->state = PHY_RUNNING;
                        netif_carrier_on(phydev->attached_dev);
                        phydev->adjust_link(phydev->attached_dev);
--

1.8.2.1

Signature · 1 line
Florian
author
#2 lore raw
Quoted 9 lines · Florian Fainelli

2014-04-23 19:52 GMT-07:00 Balakumaran Kannan <[email protected]>:

Quoted 4 lines

phy_state_machine should check whether auto-negotiatin is completed before changing phydev->state from PHY_NOLINK to PHY_RUNNING. If auto-negotiation is not completed phydev->state should be set to PHY_AN.

I need some time to review this. Just out of curiosity, is this fixing a bug you have encountered or was this found by state machine inspection?

Actually I encountered a problem that Linux-3.4 sends Neighbor Solicitation (NS) before hardware completes auto-negotiation. The driver being used for my hardware
is smsc911x.

While investigation on this, I came to know that state of phydev follows this sequence
PHY_READY --> PHY_UP (in phy_start)
PHY_UP --> PHY_AN (in phy_start_aneg called by phy_state_machine) PHY_AN --> PHY_NOLINK (in phy_state_machine. Link is down) PHY_NOLINK --> PHY_RUNNING (in phy_state_machine. Link becomes ready)

So I felt that the sequence of PHY_AN --> PHY_RUNNING while link is up and PHY_NOLINK --> PHY_RUNNING are different. In the earlier case aneg completion is taken into account but not in later.

But this patch doesn't fix my problem. I made the smsc driver's open function to poll on phy_aneg_done until auto-negotiation completes.

Here I have a doubt.
* Linux network initialization and phy_state_machine runs in separate threads.
* And driver doesn't bother about state of phydev.
Is it correct?
Doesn't driver need to confirm phydev is ready(PHY_RUNNING) before returning to kernel?

Regards,
K.Balakumaran

maintainer
#3 lore raw

From: Balakumaran Kannan <[email protected]>
Date: Fri, 25 Apr 2014 11:23:03 +0530

Quoted 6 lines

Here I have a doubt.
* Linux network initialization and phy_state_machine runs in separate threads.
* And driver doesn't bother about state of phydev.
Is it correct?
Doesn't driver need to confirm phydev is ready(PHY_RUNNING) before returning to kernel?

The one comment I will make about things here is that the phylib layer really should not call netif_carrier_on() until the PHY really is confirmed to be in a state where it will successfully pass traffic.

If it is doing so before auto-negotiation completes and succeeds, that would be a bug.

maintainer
#5 lore raw
In reply to Balakumaran Kannan

From: Balakumaran Kannan <[email protected]>
Date: Thu, 24 Apr 2014 08:22:47 +0530

Quoted 6 lines

phy_state_machine should check whether auto-negotiatin is completed before changing phydev->state from PHY_NOLINK to PHY_RUNNING. If auto-negotiation is not completed phydev->state should be set to PHY_AN.

Signed-off-by Balakumaran Kannan <[email protected]>

Florian can you please review this at your earliest possible convenience?

Thanks!

#4 lore raw
In reply to Balakumaran Kannan

2014-04-24 22:53 GMT-07:00 Balakumaran Kannan <[email protected]>:

Quoted 23 lines · Florian Fainelli
Quoted 9 lines

2014-04-23 19:52 GMT-07:00 Balakumaran Kannan <[email protected]>:

Quoted 4 lines

phy_state_machine should check whether auto-negotiatin is completed before changing phydev->state from PHY_NOLINK to PHY_RUNNING. If auto-negotiation is not completed phydev->state should be set to PHY_AN.

I need some time to review this. Just out of curiosity, is this fixing a bug you have encountered or was this found by state machine inspection?

Actually I encountered a problem that Linux-3.4 sends Neighbor Solicitation (NS) before hardware completes auto-negotiation. The driver being used for my hardware
is smsc911x.

While investigation on this, I came to know that state of phydev follows this sequence
PHY_READY --> PHY_UP (in phy_start)
PHY_UP --> PHY_AN (in phy_start_aneg called by phy_state_machine) PHY_AN --> PHY_NOLINK (in phy_state_machine. Link is down) PHY_NOLINK --> PHY_RUNNING (in phy_state_machine. Link becomes ready)

Quoted 3 lines

So I felt that the sequence of PHY_AN --> PHY_RUNNING while link is up and PHY_NOLINK --> PHY_RUNNING are different. In the earlier case aneg completion is taken into account but not in later.

I see, we should indeed go back to PHY_AN

Quoted 8 lines

But this patch doesn't fix my problem. I made the smsc driver's open function to poll on phy_aneg_done until auto-negotiation completes.

Here I have a doubt.
* Linux network initialization and phy_state_machine runs in separate threads.
* And driver doesn't bother about state of phydev.
Is it correct?

That is correct, once your driver calls phy_start(), the PHY state machine executes in the background within the context of a separate workqueue.

Quoted 2 lines

Doesn't driver need to confirm phydev is ready(PHY_RUNNING) before returning to kernel?

phy_start() really starts the PHY state machine, but does not block until the PHY is in the PHY_RUNNING state, which is something that might never happen if e.g: the cable is disconnected.

What really tells the network stack whether the link is ready, is when the PHY state machine calls netif_carrier_on().

Signature · 1 line
Florian
Reviewed-by on v1 1/1
#6 lore raw
In reply to Balakumaran Kannan

2014-04-23 19:52 GMT-07:00 Balakumaran Kannan <[email protected]>:

Quoted 6 lines

phy_state_machine should check whether auto-negotiatin is completed before changing phydev->state from PHY_NOLINK to PHY_RUNNING. If auto-negotiation is not completed phydev->state should be set to PHY_AN.

Signed-off-by Balakumaran Kannan <[email protected]>
Reviewed-by Florian Fainelli <[email protected]>
Quoted 28 lines
---
 drivers/net/phy/phy.c | 11 +++++++++++
 1 file changed, 11 insertions(+)
drivers/net/phy/phy.c +11 -1
index 1b6d09a..a972056 100644
@@ -765,6 +765,17 @@ void phy_state_machine(struct work_struct *work)
                        break;
 
                if (phydev->link) {
+                       if (AUTONEG_ENABLE == phydev->autoneg) {
+                               err = phy_aneg_done(phydev);
+                               if (err < 0)
+                                       break;
+
+                               if (!err) {
+                                       phydev->state = PHY_AN;
+                                       phydev->link_timeout = PHY_AN_TIMEOUT;
+                                       break;
+                               }
+                       }
                        phydev->state = PHY_RUNNING;
                        netif_carrier_on(phydev->attached_dev);
                        phydev->adjust_link(phydev->attached_dev);
--

1.8.2.1

Signature · 1 line
Florian