Hi Heiner, On Tue, 18 Dec 2018 07:44:33 +0100 <hkallwe...@gmail.com> wrote:
> On 18.12.2018 07:25, Kunihiko Hayashi wrote: > > 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:) > > > Up to you. It's fine with me if you submit the patch, but I can also do it > and mention you in Reported-by and Tested-by. Just let me know. I see. I'll make and submit the patch as a fix for the issue. Thank you, > > > 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 > > > > > > --- Best Regards, Kunihiko Hayashi
