net: phy: make PHY_HALTED a transition state to PHY_READY

PATCH In review Heiner Kallweit 3 participants 7 messages Dec 19, 2018 – Dec 21, 2018

Subsystems (3)

Networking Network drivers Ethernet PHY

lore

Share & tools

lore

Heiner Kallweit

author maintainer

2018-12-19 06:50 UTC #0 lore raw

cover net: phy: make PHY_HALTED a transition state to PHY_READY

PHY_HALTED and PHY_READY both are non-started states and quite similar. Major difference is that phy_start() changes from PHY_HALTED to PHY_RESUMING which doesn't reconfigure aneg (what PHY_UP does).

There's no guarantee that PHY registers are completely untouched when waking up from power-down, e.g. after system suspend. Therefore it's safer to reconfigure aneg also when starting from PHY_HALTED. This can be achieved and state machine made simpler by making PHY_HALTED going to PHY_READY after having stopped everything. Then the only way up is over PHY_UP. As part of the change PHY_HALTED is renamed to PHY_HALT to reflect that it is a transition state.

After this change states PHY_RESUMING and PHY_CHANGELINK can be removed.

Heiner Kallweit (2):
net: phy: make PHY_HALTED a transition state to PHY_READY
net: phy: remove states PHY_CHANGELINK and PHY_RESUMING

 drivers/net/phy/phy.c | 45 ++++++++++++++++---------------------------
 include/linux/phy.h   | 24 ++++++-----------------
 2 files changed, 23 insertions(+), 46 deletions(-)

Signature · 1 line

2.20.1

Heiner Kallweit

author maintainer

2018-12-19 06:55 UTC #1 lore raw

PATCH 1/2 net: phy: make PHY_HALTED a transition state to PHY_READY

There's no guarantee that PHY registers are completely untouched when waking up from power-down, e.g. after system suspend. Therefore it's safer to reconfigure aneg also when starting from PHY_HALTED. This can be achieved and state machine made simpler by making PHY_HALTED going to PHY_READY after having stopped everything. Then the only way up is over PHY_UP. Also let's warn in phy_start() if it's called from a state other than PHY_READY.
As part of the change PHY_HALTED is renamed to PHY_HALT to reflect that it is a transition state.

Signed-off-by Heiner Kallweit <[email protected]>

---
 drivers/net/phy/phy.c | 41 +++++++++++++++++------------------------
 include/linux/phy.h   | 16 ++++++++--------
 2 files changed, 25 insertions(+), 32 deletions(-)

drivers/net/phy/phy.c +17 ~~-24~~

index d33e7b3ca..2a69d947e 100644
@@ -47,12 +47,12 @@ static const char *phy_state_to_str(enum phy_state st)
 	switch (st) {
 	PHY_STATE_STR(DOWN)
 	PHY_STATE_STR(READY)
+	PHY_STATE_STR(HALT)
 	PHY_STATE_STR(UP)
 	PHY_STATE_STR(RUNNING)
 	PHY_STATE_STR(NOLINK)
 	PHY_STATE_STR(FORCING)
 	PHY_STATE_STR(CHANGELINK)
-	PHY_STATE_STR(HALTED)
 	PHY_STATE_STR(RESUMING)
 	}

@@ -733,7 +733,7 @@ static void phy_error(struct phy_device *phydev)
 	WARN_ON(1);
 
 	mutex_lock(&phydev->lock);
-	phydev->state = PHY_HALTED;
+	phydev->state = PHY_HALT;
 	mutex_unlock(&phydev->lock);
 
 	phy_trigger_machine(phydev);

@@ -859,16 +859,11 @@ void phy_stop(struct phy_device *phydev)
 	if (phy_interrupt_is_valid(phydev))
 		phy_disable_interrupts(phydev);
 
-	phydev->state = PHY_HALTED;
+	phydev->state = PHY_HALT;
 
 	mutex_unlock(&phydev->lock);
 
 	phy_state_machine(&phydev->state_queue.work);
-
-	/* Cannot call flush_scheduled_work() here as desired because
-	 * of rtnl_lock(), but PHY_HALTED shall guarantee irq handler
-	 * will not reenable interrupts.
-	 */
 }
 EXPORT_SYMBOL(phy_stop);

@@ -888,29 +883,26 @@ void phy_start(struct phy_device *phydev)
 
 	mutex_lock(&phydev->lock);
 
-	switch (phydev->state) {
-	case PHY_READY:
-		phydev->state = PHY_UP;
-		break;
-	case PHY_HALTED:
+	if (phydev->state == PHY_READY) {
 		/* if phy was suspended, bring the physical link up again */
 		__phy_resume(phydev);
 
 		/* make sure interrupts are re-enabled for the PHY */
 		if (phy_interrupt_is_valid(phydev)) {
 			err = phy_enable_interrupts(phydev);
-			if (err < 0)
-				break;
+			if (err < 0) {
+				WARN_ON(1);
+				goto out;
+			}
 		}
-
-		phydev->state = PHY_RESUMING;
-		break;
-	default:
-		break;
+		phydev->state = PHY_UP;
+		phy_trigger_machine(phydev);
+	} else {
+		WARN(1, "called from state %s\n",
+		     phy_state_to_str(phydev->state));
 	}
+out:
 	mutex_unlock(&phydev->lock);
-
-	phy_trigger_machine(phydev);
 }
 EXPORT_SYMBOL(phy_start);

@@ -962,12 +954,13 @@ void phy_state_machine(struct work_struct *work)
 			phy_link_down(phydev, false);
 		}
 		break;
-	case PHY_HALTED:
+	case PHY_HALT:
 		if (phydev->link) {
 			phydev->link = 0;
 			phy_link_down(phydev, true);
 			do_suspend = true;
 		}
+		phydev->state = PHY_READY;
 		break;
 	}

@@ -990,7 +983,7 @@ void phy_state_machine(struct work_struct *work)
 	 * PHY, if PHY_IGNORE_INTERRUPT is set, then we will be moving
 	 * between states from phy_mac_interrupt().
 	 *
-	 * In state PHY_HALTED the PHY gets suspended, so rescheduling the
+	 * In state PHY_HALT the PHY gets suspended, so rescheduling the
 	 * state machine would be pointless and possibly error prone when
 	 * called from phy_disconnect() synchronously.
 	 */

include/linux/phy.h +8 -8

index da039f211..21e553f51 100644
@@ -288,38 +288,38 @@ struct phy_device *mdiobus_scan(struct mii_bus *bus, int addr);
  *
  * NOLINK: PHY is up, but not currently plugged in.
  * - irq or timer will set RUNNING if link comes back
- * - phy_stop moves to HALTED
+ * - phy_stop moves to HALT
  *
  * FORCING: PHY is being configured with forced settings
  * - if link is up, move to RUNNING
  * - If link is down, we drop to the next highest setting, and
  *   retry (FORCING) after a timeout
- * - phy_stop moves to HALTED
+ * - phy_stop moves to HALT
  *
  * RUNNING: PHY is currently up, running, and possibly sending
  * and/or receiving packets
  * - irq or timer will set NOLINK if link goes down
- * - phy_stop moves to HALTED
+ * - phy_stop moves to HALT
  *
  * CHANGELINK: PHY experienced a change in link state
  * - timer moves to RUNNING if link
  * - timer moves to NOLINK if the link is down
- * - phy_stop moves to HALTED
+ * - phy_stop moves to HALT
  *
- * HALTED: PHY is up, but no polling or interrupts are done. Or
+ * HALT: PHY is up, but no polling or interrupts are done. Or
  * PHY is in an error state.
  *
- * - phy_start moves to RESUMING
+ * - moves to READY
  *
  * RESUMING: PHY was halted, but now wants to run again.
  * - If we are forcing, or aneg is done, timer moves to RUNNING
  * - If aneg is not done, timer moves to AN
- * - phy_stop moves to HALTED
+ * - phy_stop moves to HALT
  */
 enum phy_state {
 	PHY_DOWN = 0,
 	PHY_READY,
-	PHY_HALTED,
+	PHY_HALT,
 	PHY_UP,
 	PHY_RUNNING,
 	PHY_NOLINK,

Signature · 1 line

2.20.1

Florian Fainelli

2018-12-19 18:33 UTC #2 lore raw

Quoted 4 lines · Heiner Kallweit

PHY_HALTED and PHY_READY both are non-started states and quite similar. Major difference is that phy_start() changes from PHY_HALTED to PHY_RESUMING which doesn't reconfigure aneg (what PHY_UP does).

Quoted 9 lines

There's no guarantee that PHY registers are completely untouched when waking up from power-down, e.g. after system suspend. Therefore it's safer to reconfigure aneg also when starting from PHY_HALTED. This can be achieved and state machine made simpler by making PHY_HALTED going to PHY_READY after having stopped everything. Then the only way up is over PHY_UP. Also let's warn in phy_start() if it's called from a state other than PHY_READY.
As part of the change PHY_HALTED is renamed to PHY_HALT to reflect that it is a transition state.

Sorry for being uber nitpicky here, but a state machine is supposed to contain.. state names, PHY_HALT is more of an action.

The PHY library is not particularly optimized at the moment to avoid disrupting the link when there is not a need to, so if somehow the register contents are lost because of a low power mode that the PHY has entered, the PHY driver's resume function is responsible for bringing things back online.

Quoted 169 lines

Signed-off-by Heiner Kallweit <[email protected]>

---
 drivers/net/phy/phy.c | 41 +++++++++++++++++------------------------
 include/linux/phy.h   | 16 ++++++++--------
 2 files changed, 25 insertions(+), 32 deletions(-)

drivers/net/phy/phy.c +17 ~~-24~~

index d33e7b3ca..2a69d947e 100644
@@ -47,12 +47,12 @@ static const char *phy_state_to_str(enum phy_state st)
 	switch (st) {
 	PHY_STATE_STR(DOWN)
 	PHY_STATE_STR(READY)
+	PHY_STATE_STR(HALT)
 	PHY_STATE_STR(UP)
 	PHY_STATE_STR(RUNNING)
 	PHY_STATE_STR(NOLINK)
 	PHY_STATE_STR(FORCING)
 	PHY_STATE_STR(CHANGELINK)
-	PHY_STATE_STR(HALTED)
 	PHY_STATE_STR(RESUMING)
 	}

@@ -733,7 +733,7 @@ static void phy_error(struct phy_device *phydev)
 	WARN_ON(1);
 
 	mutex_lock(&phydev->lock);
-	phydev->state = PHY_HALTED;
+	phydev->state = PHY_HALT;
 	mutex_unlock(&phydev->lock);
 
 	phy_trigger_machine(phydev);

@@ -859,16 +859,11 @@ void phy_stop(struct phy_device *phydev)
 	if (phy_interrupt_is_valid(phydev))
 		phy_disable_interrupts(phydev);
 
-	phydev->state = PHY_HALTED;
+	phydev->state = PHY_HALT;
 
 	mutex_unlock(&phydev->lock);
 
 	phy_state_machine(&phydev->state_queue.work);
-
-	/* Cannot call flush_scheduled_work() here as desired because
-	 * of rtnl_lock(), but PHY_HALTED shall guarantee irq handler
-	 * will not reenable interrupts.
-	 */
 }
 EXPORT_SYMBOL(phy_stop);

@@ -888,29 +883,26 @@ void phy_start(struct phy_device *phydev)
 
 	mutex_lock(&phydev->lock);
 
-	switch (phydev->state) {
-	case PHY_READY:
-		phydev->state = PHY_UP;
-		break;
-	case PHY_HALTED:
+	if (phydev->state == PHY_READY) {
 		/* if phy was suspended, bring the physical link up again */
 		__phy_resume(phydev);
 
 		/* make sure interrupts are re-enabled for the PHY */
 		if (phy_interrupt_is_valid(phydev)) {
 			err = phy_enable_interrupts(phydev);
-			if (err < 0)
-				break;
+			if (err < 0) {
+				WARN_ON(1);
+				goto out;
+			}
 		}
-
-		phydev->state = PHY_RESUMING;
-		break;
-	default:
-		break;
+		phydev->state = PHY_UP;
+		phy_trigger_machine(phydev);
+	} else {
+		WARN(1, "called from state %s\n",
+		     phy_state_to_str(phydev->state));
 	}
+out:
 	mutex_unlock(&phydev->lock);
-
-	phy_trigger_machine(phydev);
 }
 EXPORT_SYMBOL(phy_start);

@@ -962,12 +954,13 @@ void phy_state_machine(struct work_struct *work)
 			phy_link_down(phydev, false);
 		}
 		break;
-	case PHY_HALTED:
+	case PHY_HALT:
 		if (phydev->link) {
 			phydev->link = 0;
 			phy_link_down(phydev, true);
 			do_suspend = true;
 		}
+		phydev->state = PHY_READY;
 		break;
 	}

@@ -990,7 +983,7 @@ void phy_state_machine(struct work_struct *work)
 	 * PHY, if PHY_IGNORE_INTERRUPT is set, then we will be moving
 	 * between states from phy_mac_interrupt().
 	 *
-	 * In state PHY_HALTED the PHY gets suspended, so rescheduling the
+	 * In state PHY_HALT the PHY gets suspended, so rescheduling the
 	 * state machine would be pointless and possibly error prone when
 	 * called from phy_disconnect() synchronously.
 	 */

include/linux/phy.h +8 -8

index da039f211..21e553f51 100644
@@ -288,38 +288,38 @@ struct phy_device *mdiobus_scan(struct mii_bus *bus, int addr);
  *
  * NOLINK: PHY is up, but not currently plugged in.
  * - irq or timer will set RUNNING if link comes back
- * - phy_stop moves to HALTED
+ * - phy_stop moves to HALT
  *
  * FORCING: PHY is being configured with forced settings
  * - if link is up, move to RUNNING
  * - If link is down, we drop to the next highest setting, and
  *   retry (FORCING) after a timeout
- * - phy_stop moves to HALTED
+ * - phy_stop moves to HALT
  *
  * RUNNING: PHY is currently up, running, and possibly sending
  * and/or receiving packets
  * - irq or timer will set NOLINK if link goes down
- * - phy_stop moves to HALTED
+ * - phy_stop moves to HALT
  *
  * CHANGELINK: PHY experienced a change in link state
  * - timer moves to RUNNING if link
  * - timer moves to NOLINK if the link is down
- * - phy_stop moves to HALTED
+ * - phy_stop moves to HALT
  *
- * HALTED: PHY is up, but no polling or interrupts are done. Or
+ * HALT: PHY is up, but no polling or interrupts are done. Or
  * PHY is in an error state.
  *
- * - phy_start moves to RESUMING
+ * - moves to READY
  *
  * RESUMING: PHY was halted, but now wants to run again.
  * - If we are forcing, or aneg is done, timer moves to RUNNING
  * - If aneg is not done, timer moves to AN
- * - phy_stop moves to HALTED
+ * - phy_stop moves to HALT
  */
 enum phy_state {
 	PHY_DOWN = 0,
 	PHY_READY,
-	PHY_HALTED,
+	PHY_HALT,
 	PHY_UP,
 	PHY_RUNNING,
 	PHY_NOLINK,

Signature · 1 line

Florian

Heiner Kallweit

author maintainer

2018-12-19 20:14 UTC #3 lore raw

Quoted 15 lines · Florian Fainelli

Quoted 4 lines · Heiner Kallweit
PHY_HALTED and PHY_READY both are non-started states and quite similar. Major difference is that phy_start() changes from PHY_HALTED to PHY_RESUMING which doesn't reconfigure aneg (what PHY_UP does).
Quoted 9 lines
There's no guarantee that PHY registers are completely untouched when waking up from power-down, e.g. after system suspend. Therefore it's safer to reconfigure aneg also when starting from PHY_HALTED. This can be achieved and state machine made simpler by making PHY_HALTED going to PHY_READY after having stopped everything. Then the only way up is over PHY_UP. Also let's warn in phy_start() if it's called from a state other than PHY_READY.
As part of the change PHY_HALTED is renamed to PHY_HALT to reflect that it is a transition state.

Quoted 3 lines

Sorry for being uber nitpicky here, but a state machine is supposed to contain.. state names, PHY_HALT is more of an action.

Right, maybe PHY_HALTING would be better? Because in this state we bring the link down and then change to PHY_READY.

Quoted 6 lines

The PHY library is not particularly optimized at the moment to avoid disrupting the link when there is not a need to, so if somehow the register contents are lost because of a low power mode that the PHY has entered, the PHY driver's resume function is responsible for bringing things back online.

Yes, we could do things like reconfiguring aneg in the resume callback. However to me this seems to be more complex and not as easy as just going the path PHY_READY -> PHY_UP -> reconfig -> PHY_RUNNING. Why should we treat a phy_start() after a phy_stop() different than the first phy_start() after connecting the PHY?
In my perspective there's no good justification for having separate states PHY_READY and PHY_HALTED.

Only small overhead of my proposal is that a phy_start after a phy_stop() checks the PHY aneg registers whether everything is as expected. So far a phy_start() after a phy_stop() doesn't care and just checks whether the link is up.

I know, I removed half of the state machine already and one may wonder whether all this code was actually redundant. But well, it still seems to work.

Quoted 171 lines

Quoted 169 lines

Signed-off-by Heiner Kallweit <[email protected]>

---
 drivers/net/phy/phy.c | 41 +++++++++++++++++------------------------
 include/linux/phy.h   | 16 ++++++++--------
 2 files changed, 25 insertions(+), 32 deletions(-)

drivers/net/phy/phy.c +17 ~~-24~~

index d33e7b3ca..2a69d947e 100644
@@ -47,12 +47,12 @@ static const char *phy_state_to_str(enum phy_state st)
 	switch (st) {
 	PHY_STATE_STR(DOWN)
 	PHY_STATE_STR(READY)
+	PHY_STATE_STR(HALT)
 	PHY_STATE_STR(UP)
 	PHY_STATE_STR(RUNNING)
 	PHY_STATE_STR(NOLINK)
 	PHY_STATE_STR(FORCING)
 	PHY_STATE_STR(CHANGELINK)
-	PHY_STATE_STR(HALTED)
 	PHY_STATE_STR(RESUMING)
 	}

@@ -733,7 +733,7 @@ static void phy_error(struct phy_device *phydev)
 	WARN_ON(1);
 
 	mutex_lock(&phydev->lock);
-	phydev->state = PHY_HALTED;
+	phydev->state = PHY_HALT;
 	mutex_unlock(&phydev->lock);
 
 	phy_trigger_machine(phydev);

@@ -859,16 +859,11 @@ void phy_stop(struct phy_device *phydev)
 	if (phy_interrupt_is_valid(phydev))
 		phy_disable_interrupts(phydev);
 
-	phydev->state = PHY_HALTED;
+	phydev->state = PHY_HALT;
 
 	mutex_unlock(&phydev->lock);
 
 	phy_state_machine(&phydev->state_queue.work);
-
-	/* Cannot call flush_scheduled_work() here as desired because
-	 * of rtnl_lock(), but PHY_HALTED shall guarantee irq handler
-	 * will not reenable interrupts.
-	 */
 }
 EXPORT_SYMBOL(phy_stop);

@@ -888,29 +883,26 @@ void phy_start(struct phy_device *phydev)
 
 	mutex_lock(&phydev->lock);
 
-	switch (phydev->state) {
-	case PHY_READY:
-		phydev->state = PHY_UP;
-		break;
-	case PHY_HALTED:
+	if (phydev->state == PHY_READY) {
 		/* if phy was suspended, bring the physical link up again */
 		__phy_resume(phydev);
 
 		/* make sure interrupts are re-enabled for the PHY */
 		if (phy_interrupt_is_valid(phydev)) {
 			err = phy_enable_interrupts(phydev);
-			if (err < 0)
-				break;
+			if (err < 0) {
+				WARN_ON(1);
+				goto out;
+			}
 		}
-
-		phydev->state = PHY_RESUMING;
-		break;
-	default:
-		break;
+		phydev->state = PHY_UP;
+		phy_trigger_machine(phydev);
+	} else {
+		WARN(1, "called from state %s\n",
+		     phy_state_to_str(phydev->state));
 	}
+out:
 	mutex_unlock(&phydev->lock);
-
-	phy_trigger_machine(phydev);
 }
 EXPORT_SYMBOL(phy_start);

@@ -962,12 +954,13 @@ void phy_state_machine(struct work_struct *work)
 			phy_link_down(phydev, false);
 		}
 		break;
-	case PHY_HALTED:
+	case PHY_HALT:
 		if (phydev->link) {
 			phydev->link = 0;
 			phy_link_down(phydev, true);
 			do_suspend = true;
 		}
+		phydev->state = PHY_READY;
 		break;
 	}

@@ -990,7 +983,7 @@ void phy_state_machine(struct work_struct *work)
 	 * PHY, if PHY_IGNORE_INTERRUPT is set, then we will be moving
 	 * between states from phy_mac_interrupt().
 	 *
-	 * In state PHY_HALTED the PHY gets suspended, so rescheduling the
+	 * In state PHY_HALT the PHY gets suspended, so rescheduling the
 	 * state machine would be pointless and possibly error prone when
 	 * called from phy_disconnect() synchronously.
 	 */

include/linux/phy.h +8 -8

index da039f211..21e553f51 100644
@@ -288,38 +288,38 @@ struct phy_device *mdiobus_scan(struct mii_bus *bus, int addr);
  *
  * NOLINK: PHY is up, but not currently plugged in.
  * - irq or timer will set RUNNING if link comes back
- * - phy_stop moves to HALTED
+ * - phy_stop moves to HALT
  *
  * FORCING: PHY is being configured with forced settings
  * - if link is up, move to RUNNING
  * - If link is down, we drop to the next highest setting, and
  *   retry (FORCING) after a timeout
- * - phy_stop moves to HALTED
+ * - phy_stop moves to HALT
  *
  * RUNNING: PHY is currently up, running, and possibly sending
  * and/or receiving packets
  * - irq or timer will set NOLINK if link goes down
- * - phy_stop moves to HALTED
+ * - phy_stop moves to HALT
  *
  * CHANGELINK: PHY experienced a change in link state
  * - timer moves to RUNNING if link
  * - timer moves to NOLINK if the link is down
- * - phy_stop moves to HALTED
+ * - phy_stop moves to HALT
  *
- * HALTED: PHY is up, but no polling or interrupts are done. Or
+ * HALT: PHY is up, but no polling or interrupts are done. Or
  * PHY is in an error state.
  *
- * - phy_start moves to RESUMING
+ * - moves to READY
  *
  * RESUMING: PHY was halted, but now wants to run again.
  * - If we are forcing, or aneg is done, timer moves to RUNNING
  * - If aneg is not done, timer moves to AN
- * - phy_stop moves to HALTED
+ * - phy_stop moves to HALT
  */
 enum phy_state {
 	PHY_DOWN = 0,
 	PHY_READY,
-	PHY_HALTED,
+	PHY_HALT,
 	PHY_UP,
 	PHY_RUNNING,
 	PHY_NOLINK,

Heiner Kallweit

author maintainer

2018-12-19 06:55 UTC #4 lore raw

PATCH 2/2 net: phy: remove states PHY_CHANGELINK and PHY_RESUMING

In reply to Heiner Kallweit

After having made PHY_HALTED a transition state to PHY_READY and having removed direct use of PHY_CHANGELINK in drivers we can remove states PHY_RESUMING and PHY_CHANGELINK.

Signed-off-by Heiner Kallweit <[email protected]>

---
 drivers/net/phy/phy.c |  4 ----
 include/linux/phy.h   | 12 ------------
 2 files changed, 16 deletions(-)

drivers/net/phy/phy.c +0 -4

index 2a69d947e..b34158420 100644
@@ -52,8 +52,6 @@ static const char *phy_state_to_str(enum phy_state st)
 	PHY_STATE_STR(RUNNING)
 	PHY_STATE_STR(NOLINK)
 	PHY_STATE_STR(FORCING)
-	PHY_STATE_STR(CHANGELINK)
-	PHY_STATE_STR(RESUMING)
 	}
 
 	return NULL;

@@ -936,8 +934,6 @@ void phy_state_machine(struct work_struct *work)
 		break;
 	case PHY_NOLINK:
 	case PHY_RUNNING:
-	case PHY_CHANGELINK:
-	case PHY_RESUMING:
 		err = phy_check_link_status(phydev);
 		break;
 	case PHY_FORCING:

include/linux/phy.h +0 ~~-12~~

index 21e553f51..68fac589d 100644
@@ -301,20 +301,10 @@ struct phy_device *mdiobus_scan(struct mii_bus *bus, int addr);
  * - irq or timer will set NOLINK if link goes down
  * - phy_stop moves to HALT
  *
- * CHANGELINK: PHY experienced a change in link state
- * - timer moves to RUNNING if link
- * - timer moves to NOLINK if the link is down
- * - phy_stop moves to HALT
- *
  * HALT: PHY is up, but no polling or interrupts are done. Or
  * PHY is in an error state.
  *
  * - moves to READY
- *
- * RESUMING: PHY was halted, but now wants to run again.
- * - If we are forcing, or aneg is done, timer moves to RUNNING
- * - If aneg is not done, timer moves to AN
- * - phy_stop moves to HALT
  */
 enum phy_state {
 	PHY_DOWN = 0,

@@ -324,8 +314,6 @@ enum phy_state {
 	PHY_RUNNING,
 	PHY_NOLINK,
 	PHY_FORCING,
-	PHY_CHANGELINK,
-	PHY_RESUMING
 };
 
 /**

Signature · 1 line

2.20.1

David Miller

maintainer

2018-12-20 00:20 UTC #5 lore raw

In reply to Heiner Kallweit

From: Heiner Kallweit <[email protected]>
Date: Wed, 19 Dec 2018 07:50:43 +0100

Quoted 12 lines

PHY_HALTED and PHY_READY both are non-started states and quite similar. Major difference is that phy_start() changes from PHY_HALTED to PHY_RESUMING which doesn't reconfigure aneg (what PHY_UP does).
There's no guarantee that PHY registers are completely untouched when waking up from power-down, e.g. after system suspend. Therefore it's safer to reconfigure aneg also when starting from PHY_HALTED. This can be achieved and state machine made simpler by making PHY_HALTED going to PHY_READY after having stopped everything. Then the only way up is over PHY_UP. As part of the change PHY_HALTED is renamed to PHY_HALT to reflect that it is a transition state.

Quoted 2 lines

After this change states PHY_RESUMING and PHY_CHANGELINK can be removed.

It looks like this series is still under discussion, so I'll hold on this until the discussion meets some kind of agreement or conclusion.

Thanks.

David Miller

maintainer

2018-12-21 03:11 UTC #6 lore raw

In reply to Heiner Kallweit

From: Heiner Kallweit <[email protected]>
Date: Wed, 19 Dec 2018 07:50:43 +0100

Quoted 12 lines

PHY_HALTED and PHY_READY both are non-started states and quite similar. Major difference is that phy_start() changes from PHY_HALTED to PHY_RESUMING which doesn't reconfigure aneg (what PHY_UP does).
There's no guarantee that PHY registers are completely untouched when waking up from power-down, e.g. after system suspend. Therefore it's safer to reconfigure aneg also when starting from PHY_HALTED. This can be achieved and state machine made simpler by making PHY_HALTED going to PHY_READY after having stopped everything. Then the only way up is over PHY_UP. As part of the change PHY_HALTED is renamed to PHY_HALT to reflect that it is a transition state.

Quoted 2 lines

After this change states PHY_RESUMING and PHY_CHANGELINK can be removed.

Patch #1 is under discussion still with any clear resolution, and the net-next tree is closing tonight, so I'm marking this as deferred.