* [PATCH net-next 0/2] net: phy: make PHY_HALTED a transition state to PHY_READY
@ 2018-12-19 6:50 Heiner Kallweit
2018-12-19 6:53 ` [PATCH net-next 1/2] " Heiner Kallweit
` (3 more replies)
0 siblings, 4 replies; 7+ messages in thread
From: Heiner Kallweit @ 2018-12-19 6:50 UTC (permalink / raw)
To: Andrew Lunn, Florian Fainelli, David Miller; +Cc: netdev
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH net-next 1/2] net: phy: make PHY_HALTED a transition state to PHY_READY
2018-12-19 6:50 [PATCH net-next 0/2] net: phy: make PHY_HALTED a transition state to PHY_READY Heiner Kallweit
@ 2018-12-19 6:53 ` Heiner Kallweit
2018-12-19 18:32 ` Florian Fainelli
2018-12-19 6:55 ` [PATCH net-next 2/2] net: phy: remove states PHY_CHANGELINK and PHY_RESUMING Heiner Kallweit
` (2 subsequent siblings)
3 siblings, 1 reply; 7+ messages in thread
From: Heiner Kallweit @ 2018-12-19 6:53 UTC (permalink / raw)
To: Andrew Lunn, Florian Fainelli, David Miller; +Cc: netdev
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
^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH net-next 2/2] net: phy: remove states PHY_CHANGELINK and PHY_RESUMING
2018-12-19 6:50 [PATCH net-next 0/2] net: phy: make PHY_HALTED a transition state to PHY_READY Heiner Kallweit
2018-12-19 6:53 ` [PATCH net-next 1/2] " Heiner Kallweit
@ 2018-12-19 6:55 ` Heiner Kallweit
2018-12-20 0:20 ` [PATCH net-next 0/2] net: phy: make PHY_HALTED a transition state to PHY_READY David Miller
2018-12-21 3:11 ` David Miller
3 siblings, 0 replies; 7+ messages in thread
From: Heiner Kallweit @ 2018-12-19 6:55 UTC (permalink / raw)
To: Andrew Lunn, Florian Fainelli, David Miller; +Cc: netdev
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
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH net-next 1/2] net: phy: make PHY_HALTED a transition state to PHY_READY
2018-12-19 6:53 ` [PATCH net-next 1/2] " Heiner Kallweit
@ 2018-12-19 18:32 ` Florian Fainelli
2018-12-19 20:13 ` Heiner Kallweit
0 siblings, 1 reply; 7+ messages in thread
From: Florian Fainelli @ 2018-12-19 18:32 UTC (permalink / raw)
To: Heiner Kallweit, Andrew Lunn, David Miller; +Cc: netdev
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
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH net-next 1/2] net: phy: make PHY_HALTED a transition state to PHY_READY
2018-12-19 18:32 ` Florian Fainelli
@ 2018-12-19 20:13 ` Heiner Kallweit
0 siblings, 0 replies; 7+ messages in thread
From: Heiner Kallweit @ 2018-12-19 20:13 UTC (permalink / raw)
To: Florian Fainelli, Andrew Lunn, David Miller; +Cc: netdev
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,
>>
>
>
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH net-next 0/2] net: phy: make PHY_HALTED a transition state to PHY_READY
2018-12-19 6:50 [PATCH net-next 0/2] net: phy: make PHY_HALTED a transition state to PHY_READY Heiner Kallweit
2018-12-19 6:53 ` [PATCH net-next 1/2] " Heiner Kallweit
2018-12-19 6:55 ` [PATCH net-next 2/2] net: phy: remove states PHY_CHANGELINK and PHY_RESUMING Heiner Kallweit
@ 2018-12-20 0:20 ` David Miller
2018-12-21 3:11 ` David Miller
3 siblings, 0 replies; 7+ messages in thread
From: David Miller @ 2018-12-20 0:20 UTC (permalink / raw)
To: hkallweit1; +Cc: andrew, f.fainelli, netdev
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.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH net-next 0/2] net: phy: make PHY_HALTED a transition state to PHY_READY
2018-12-19 6:50 [PATCH net-next 0/2] net: phy: make PHY_HALTED a transition state to PHY_READY Heiner Kallweit
` (2 preceding siblings ...)
2018-12-20 0:20 ` [PATCH net-next 0/2] net: phy: make PHY_HALTED a transition state to PHY_READY David Miller
@ 2018-12-21 3:11 ` David Miller
3 siblings, 0 replies; 7+ messages in thread
From: David Miller @ 2018-12-21 3:11 UTC (permalink / raw)
To: hkallweit1; +Cc: andrew, f.fainelli, netdev
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.
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2018-12-21 3:11 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-12-19 6:50 [PATCH net-next 0/2] net: phy: make PHY_HALTED a transition state to PHY_READY Heiner Kallweit
2018-12-19 6:53 ` [PATCH net-next 1/2] " Heiner Kallweit
2018-12-19 18:32 ` Florian Fainelli
2018-12-19 20:13 ` Heiner Kallweit
2018-12-19 6:55 ` [PATCH net-next 2/2] net: phy: remove states PHY_CHANGELINK and PHY_RESUMING Heiner Kallweit
2018-12-20 0:20 ` [PATCH net-next 0/2] net: phy: make PHY_HALTED a transition state to PHY_READY David Miller
2018-12-21 3:11 ` David Miller
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.