Re: [PATCH net] net: phy: Fix the issue that netif always links up after resuming

[Kunihiko Hayashi]

Hi Heiner,

On Tue, 18 Dec 2018 19:15:39 +0100 <hkallweit1@xxxxxxxxx> wrote:

> On 18.12.2018 08:57, Kunihiko Hayashi 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.
> > 
> > If the link is still down before enabling the network interface,
> > and after resuming from hibernation, the phydev->state is forcibly set
> > to PHY_UP in mdio_bus_phy_restore(), and the link becomes up.
> > 
> > In suspend sequence, only if the PHY is attached, mdio_bus_phy_suspend()
> > calls phy_stop_machine(), and mdio_bus_phy_resume() calls
> > phy_start_machine().
> > In resume sequence, it's enough to do the same as mdio_bus_phy_resume()
> > because the state has been preserved.
> > 
> > This patch fixes the issue by calling phy_start_machine() in
> > mdio_bus_phy_restore() in the same way as mdio_bus_phy_resume().
> > 
> The patch itself is fine and also that you tagged it as "net".
> What's missing is a "Fixes:" line (before the Suggested-by:).
> I think it should be:
> 
> Fixes: bc87922ff59d ("phy: Move PHY PM operations into phy_device")
> 
> This commit didn't actually introduce the issue, but moved it to today's
> place.

Thanks for pointing out.
I was concerned about that and which commit would be fixed if added.

Thank you,

> 
> > Suggested-by: Heiner Kallweit <hkallweit1@xxxxxxxxx>
> > Signed-off-by: Kunihiko Hayashi <hayashi.kunihiko@xxxxxxxxxxxxx>
> > ---
> >  drivers/net/phy/phy_device.c | 7 ++-----
> >  1 file changed, 2 insertions(+), 5 deletions(-)
> > 
> > This patch is based on the RFC patch discussion [1].
> > [1] https://www.spinics.net/lists/netdev/msg537326.html
> > 
> > diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
> > index 7d5d698..3685be4 100644
> > --- a/drivers/net/phy/phy_device.c
> > +++ b/drivers/net/phy/phy_device.c
> > @@ -315,11 +315,8 @@ static int mdio_bus_phy_restore(struct device *dev)
> >  	if (ret < 0)
> >  		return ret;
> >  
> > -	/* The PHY needs to renegotiate. */
> > -	phydev->link = 0;
> > -	phydev->state = PHY_UP;
> > -
> > -	phy_start_machine(phydev);
> > +	if (phydev->attached_dev && phydev->adjust_link)
> > +		phy_start_machine(phydev);
> >  
> >  	return 0;
> >  }
> > 

---
Best Regards,
Kunihiko Hayashi