| Message ID | 99ed7726-1e57-b3af-c48a-7b8da4c09c47@gmail.com |
|---|---|
| State | Deferred, archived |
| Delegated to: | David Miller |
| Headers | show |
| Series | net: phy: make PHY_HALTED a transition state to PHY_READY | expand |
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, >
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, >> > >
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,
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(-)