Hi Heiner, On Mon, 17 Dec 2018 19:43:31 +0100 <hkallwe...@gmail.com> wrote:
> On 17.12.2018 19:41, Heiner Kallweit wrote: > > On 17.12.2018 07:41, Kunihiko Hayashi wrote: > >> Hi, > >> > >> Gentle ping... > >> Are there any comments about changes since v2? > >> > >> v2: https://www.spinics.net/lists/netdev/msg536926.html > >> > >> Thank you, > >> > >> On Mon, 3 Dec 2018 17:22:29 +0900 <hayashi.kunih...@socionext.com> wrote: > >> > >>> Even though the link is down before entering hibernation, > >>> there is an issue that the network interface always links up after > >>> resuming > >>> from hibernation. > >>> > >>> The phydev->state is PHY_READY before enabling the network interface, so > >>> the link is down. After resuming from hibernation, the phydev->state is > >>> forcibly set to PHY_UP in mdio_bus_phy_restore(), and the link becomes up. > >>> > >>> This patch adds a new convenient function to check whether the PHY is in > >>> a started state, and expects to solve the issue by changing phydev->state > >>> to PHY_UP and calling phy_start_machine() only when the PHY is started. > >>> > > This convenience function and the related change to phy_stop() are part of > > the following already and don't need to be part of your patch. > > https://patchwork.ozlabs.org/patch/1014171/ I see. I'll follow your patch when necessary. > >>> Suggested-by: Heiner Kallweit <hkallwe...@gmail.com> > >>> Signed-off-by: Kunihiko Hayashi <hayashi.kunih...@socionext.com> > >>> --- > >>> > >>> Changes since v2: > >>> - add mutex lock/unlock for changing phydev->state > >>> - check whether the mutex is locked in phy_is_started() > >>> > >>> Changes since v1: > >>> - introduce a new helper function phy_is_started() and use it instead of > >>> checking link status > >>> - replace checking phydev->state with phy_is_started() in > >>> phy_stop_machine() > >>> > >>> drivers/net/phy/phy.c | 2 +- > >>> drivers/net/phy/phy_device.c | 12 +++++++++--- > >>> include/linux/phy.h | 13 +++++++++++++ > >>> 3 files changed, 23 insertions(+), 4 deletions(-) > >>> > >>> diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c > >>> index 1d73ac3..f484d03 100644 > >>> --- a/drivers/net/phy/phy.c > >>> +++ b/drivers/net/phy/phy.c > >>> @@ -670,7 +670,7 @@ void phy_stop_machine(struct phy_device *phydev) > >>> cancel_delayed_work_sync(&phydev->state_queue); > >>> > >>> mutex_lock(&phydev->lock); > >>> - if (phydev->state > PHY_UP && phydev->state != PHY_HALTED) > >>> + if (phy_is_started(phydev)) > >>> phydev->state = PHY_UP; > > > > I'm wondering whether we need to do this. If the PHY is attached, > > then mdio_bus_phy_suspend() calls phy_stop_machine() which does > > exactly the same. If the PHY is not attached, then we don't have > > to do anything. Therefore I think we just have to do the same as > > in mdio_bus_phy_resume(): > > > > if (phydev->attached_dev && phydev->adjust_link) > > phy_start_machine(phydev); Agreed. Although the original code changed phydev->state, it seems that it's only enough to - call phy_stop_machine() in mdio_bus_phy_suspend() - call phy_start_machine() in mdio_bus_phy_resume() and mdio_bus_phy_restore() if the PHY is attached. > > Can you test this? I tested your code instead of applying my entire patch, and I confirmed that the code solved the issue in my environment. Do you make new patch instead of my patch? (and I can add Reported-by: for the issue and Tested-by:) Thank you, > > > Sorry for the confusion, this comment is related to the next part > of your patch. > > >>> mutex_unlock(&phydev->lock); > >>> } > >>> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c > >>> index ab33d17..4897d24 100644 > >>> --- a/drivers/net/phy/phy_device.c > >>> +++ b/drivers/net/phy/phy_device.c > >>> @@ -309,10 +309,16 @@ static int mdio_bus_phy_restore(struct device *dev) > >>> return ret; > >>> > >>> /* The PHY needs to renegotiate. */ > >>> - phydev->link = 0; > >>> - phydev->state = PHY_UP; > >>> + mutex_lock(&phydev->lock); > >>> + if (phy_is_started(phydev)) { > >>> + phydev->state = PHY_UP; > >>> + mutex_unlock(&phydev->lock); > >>> + phydev->link = 0; > >>> + phy_start_machine(phydev); > >>> + } else { > >>> + mutex_unlock(&phydev->lock); > >>> + } > >>> > >>> - phy_start_machine(phydev); > >>> > >>> return 0; > >>> } > >>> diff --git a/include/linux/phy.h b/include/linux/phy.h > >>> index 3ea87f7..dd21537 100644 > >>> --- a/include/linux/phy.h > >>> +++ b/include/linux/phy.h > >>> @@ -898,6 +898,19 @@ static inline bool phy_is_pseudo_fixed_link(struct > >>> phy_device *phydev) > >>> } > >>> > >>> /** > >>> + * phy_is_started - Convenience function for testing whether a PHY is in > >>> + * a started state > >>> + * @phydev: the phy_device struct > >>> + * > >>> + * The caller must have taken the phy_device mutex lock. > >>> + */ > >>> +static inline bool phy_is_started(struct phy_device *phydev) > >>> +{ > >>> + WARN_ON(!mutex_is_locked(&phydev->lock)); > >>> + return phydev->state >= PHY_UP && phydev->state != PHY_HALTED; > >>> +} > >>> + > >>> +/** > >>> * phy_write_mmd - Convenience function for writing a register > >>> * on an MMD on a given PHY. > >>> * @phydev: The phy_device struct > >>> -- > >>> 2.7.4 > >> > >> --- > >> Best Regards, > >> Kunihiko Hayashi > >> > >> > >> > > --- Best Regards, Kunihiko Hayashi
