From mboxrd@z Thu Jan 1 00:00:00 1970 From: Heiner Kallweit Subject: Re: [PATCH net-next 1/2] net: phy: make PHY_HALTED a transition state to PHY_READY Date: Wed, 19 Dec 2018 21:13:59 +0100 Message-ID: <859946d8-2d44-5c01-65b8-851e6e88397d@gmail.com> References: <99ed7726-1e57-b3af-c48a-7b8da4c09c47@gmail.com> Mime-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 7bit Cc: "netdev@vger.kernel.org" To: Florian Fainelli , Andrew Lunn , David Miller Return-path: Received: from mail-wm1-f66.google.com ([209.85.128.66]:51473 "EHLO mail-wm1-f66.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1728435AbeLSUOJ (ORCPT ); Wed, 19 Dec 2018 15:14:09 -0500 Received: by mail-wm1-f66.google.com with SMTP id b11so7269315wmj.1 for ; Wed, 19 Dec 2018 12:14:07 -0800 (PST) In-Reply-To: Content-Language: en-US Sender: netdev-owner@vger.kernel.org List-ID: 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 >> --- >> 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, >> > >