Re: [PATCH V3 1/2] net: phylink: add a function to resume phy alone to fix resume issue with WoL enabled

[Russell King (Oracle)]

On Thu, Feb 23, 2023 at 11:09:04AM +0100, Paolo Abeni wrote:
> On Thu, 2023-02-02 at 16:15 +0800, Clark Wang wrote:
> > Issue we met:
> > On some platforms, mac cannot work after resumed from the suspend with WoL
> > enabled.
> > 
> > The cause of the issue:
> > 1. phylink_resolve() is in a workqueue which will not be executed immediately.
> >    This is the call sequence:
> >        phylink_resolve()->phylink_link_up()->pl->mac_ops->mac_link_up()
> >    For stmmac driver, mac_link_up() will set the correct speed/duplex...
> >    values which are from link_state.
> > 2. In stmmac_resume(), it will call stmmac_hw_setup() after called the
> >    phylink_resume(), because mac need phy rx_clk to do the reset.
> >    stmmac_core_init() is called in function stmmac_hw_setup(), which will
> >    reset the mac and set the speed/duplex... to default value.
> > Conclusion: Because phylink_resolve() cannot determine when it is called, it
> >             cannot be guaranteed to be called after stmmac_core_init().
> > 	    Once stmmac_core_init() is called after phylink_resolve(),
> > 	    the mac will be misconfigured and cannot be used.
> > 
> > In order to avoid this problem, add a function called phylink_phy_resume()
> > to resume phy separately. This eliminates the need to call phylink_resume()
> > before stmmac_hw_setup().
> > 
> > Add another judgement before called phy_start() in phylink_start(). This way
> > phy_start() will not be called multiple times when resumes. At the same time,
> > it may not affect other drivers that do not use phylink_phy_resume().
> > 
> > Signed-off-by: Clark Wang <xiaoning.wang@xxxxxxx>
> > ---
> > V2 change:
> >  - add mac_resume_phy_separately flag to struct phylink to mark if the mac
> >    driver uses the phylink_phy_resume() first.
> > V3 change:
> >  - add brace to avoid ambiguous 'else'
> >    Reported-by: kernel test robot <lkp@xxxxxxxxx>
> > ---
> >  drivers/net/phy/phylink.c | 32 ++++++++++++++++++++++++++++++--
> >  include/linux/phylink.h   |  1 +
> >  2 files changed, 31 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
> > index 319790221d7f..c2fe66f0b78f 100644
> > --- a/drivers/net/phy/phylink.c
> > +++ b/drivers/net/phy/phylink.c
> > @@ -80,6 +80,8 @@ struct phylink {
> >  	DECLARE_PHY_INTERFACE_MASK(sfp_interfaces);
> >  	__ETHTOOL_DECLARE_LINK_MODE_MASK(sfp_support);
> >  	u8 sfp_port;
> > +
> > +	bool mac_resume_phy_separately;
> >  };
> >  
> >  #define phylink_printk(level, pl, fmt, ...) \
> > @@ -1509,6 +1511,7 @@ struct phylink *phylink_create(struct phylink_config *config,
> >  		return ERR_PTR(-EINVAL);
> >  	}
> >  
> > +	pl->mac_resume_phy_separately = false;
> >  	pl->using_mac_select_pcs = using_mac_select_pcs;
> >  	pl->phy_state.interface = iface;
> >  	pl->link_interface = iface;
> > @@ -1943,8 +1946,12 @@ void phylink_start(struct phylink *pl)
> >  	}
> >  	if (poll)
> >  		mod_timer(&pl->link_poll, jiffies + HZ);
> > -	if (pl->phydev)
> > -		phy_start(pl->phydev);
> > +	if (pl->phydev) {
> > +		if (!pl->mac_resume_phy_separately)
> > +			phy_start(pl->phydev);
> > +		else
> > +			pl->mac_resume_phy_separately = false;
> > +	}
> >  	if (pl->sfp_bus)
> >  		sfp_upstream_start(pl->sfp_bus);
> >  }
> > @@ -2024,6 +2031,27 @@ void phylink_suspend(struct phylink *pl, bool mac_wol)
> >  }
> >  EXPORT_SYMBOL_GPL(phylink_suspend);
> >  
> > +/**
> > + * phylink_phy_resume() - resume phy alone
> > + * @pl: a pointer to a &struct phylink returned from phylink_create()
> > + *
> > + * In the MAC driver using phylink, if the MAC needs the clock of the phy
> > + * when it resumes, can call this function to resume the phy separately.
> > + * Then proceed to MAC resume operations.
> > + */
> > +void phylink_phy_resume(struct phylink *pl)
> > +{
> > +	ASSERT_RTNL();
> > +
> > +	if (!test_bit(PHYLINK_DISABLE_MAC_WOL, &pl->phylink_disable_state)
> > +	    && pl->phydev) {
> > +		phy_start(pl->phydev);
> > +		pl->mac_resume_phy_separately = true;
> > +	}
> > +
> 
> Minor nit: the empty line here is not needed.

The author appears to have become non-responsive after sending half of
the two patch series, and hasn't addressed previous feedback.

In any case, someone else was also having similar issues with stmmac,
and proposing different patches, so I've requested that they work
together to solve what looks like one common problem, instead of us
ending up with two patch series potentially addressing that same
issue.

-- 
RMK's Patch system: https://www.armlinux.org.uk/developer/patches/
FTTP is here! 40Mbps down 10Mbps up. Decent connectivity at last!