[PATCH net-next 0/2] net: phy: make PHY_HALTED a transition state to PHY_READY

[PATCH net-next 0/2] net: phy: make PHY_HALTED a transition state to PHY_READY

[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).

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(-)

-- 
2.20.1

[PATCH net-next 1/2] net: phy: make PHY_HALTED a transition state to PHY_READY

[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).

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 <hkallweit1@gmail.com>
---
 drivers/net/phy/phy.c | 41 +++++++++++++++++------------------------
 include/linux/phy.h   | 16 ++++++++--------
 2 files changed, 25 insertions(+), 32 deletions(-)

diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index d33e7b3ca..2a69d947e 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c
@@ -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.
 	 */
diff --git a/include/linux/phy.h b/include/linux/phy.h
index da039f211..21e553f51 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -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,
-- 
2.20.1

Re: [PATCH net-next 1/2] net: phy: make PHY_HALTED a transition state to PHY_READY

[Florian Fainelli]

On 12/18/18 10:53 PM, Heiner Kallweit wrote:
> 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. 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.

> 
> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
> ---
>  drivers/net/phy/phy.c | 41 +++++++++++++++++------------------------
>  include/linux/phy.h   | 16 ++++++++--------
>  2 files changed, 25 insertions(+), 32 deletions(-)
> 
> diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
> index d33e7b3ca..2a69d947e 100644
> --- a/drivers/net/phy/phy.c
> +++ b/drivers/net/phy/phy.c
> @@ -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.
>  	 */
> diff --git a/include/linux/phy.h b/include/linux/phy.h
> index da039f211..21e553f51 100644
> --- a/include/linux/phy.h
> +++ b/include/linux/phy.h
> @@ -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,
> 


-- 
Florian

Re: [PATCH net-next 1/2] net: phy: make PHY_HALTED a transition state to PHY_READY

[Heiner Kallweit]

On 19.12.2018 19:32, Florian Fainelli wrote:
> On 12/18/18 10:53 PM, Heiner Kallweit wrote:
>> 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. 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.
> 
Right, maybe PHY_HALTING would be better? Because in this state we
bring the link down and then change to PHY_READY.

> 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.

>>
>> Signed-off-by: Heiner Kallweit <hkallweit1@gmail.com>
>> ---
>>  drivers/net/phy/phy.c | 41 +++++++++++++++++------------------------
>>  include/linux/phy.h   | 16 ++++++++--------
>>  2 files changed, 25 insertions(+), 32 deletions(-)
>>
>> diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
>> index d33e7b3ca..2a69d947e 100644
>> --- a/drivers/net/phy/phy.c
>> +++ b/drivers/net/phy/phy.c
>> @@ -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.
>>  	 */
>> diff --git a/include/linux/phy.h b/include/linux/phy.h
>> index da039f211..21e553f51 100644
>> --- a/include/linux/phy.h
>> +++ b/include/linux/phy.h
>> @@ -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,
>>
> 
> 

[PATCH net-next 2/2] net: phy: remove states PHY_CHANGELINK and PHY_RESUMING

[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 <hkallweit1@gmail.com>
---
 drivers/net/phy/phy.c |  4 ----
 include/linux/phy.h   | 12 ------------
 2 files changed, 16 deletions(-)

diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index 2a69d947e..b34158420 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c
@@ -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:
diff --git a/include/linux/phy.h b/include/linux/phy.h
index 21e553f51..68fac589d 100644
--- a/include/linux/phy.h
+++ b/include/linux/phy.h
@@ -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
 };
 
 /**
-- 
2.20.1

Re: [PATCH net-next 0/2] net: phy: make PHY_HALTED a transition state to PHY_READY

[David Miller]

From: Heiner Kallweit <hkallweit1@gmail.com>
Date: Wed, 19 Dec 2018 07:50:43 +0100

> 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.

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.

Re: [PATCH net-next 0/2] net: phy: make PHY_HALTED a transition state to PHY_READY

[David Miller]

From: Heiner Kallweit <hkallweit1@gmail.com>
Date: Wed, 19 Dec 2018 07:50:43 +0100

> 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.

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.