Re: [PATCH net] net: phylink: fix link state on phy-connect

[Russell King <rmk@armlinux.org.uk>]

Oh, and lastly, please send patches to linux@armlinux.org.uk or the
address I use in the sign-offs - sending them to rmk@armlinux.org.uk
is for personal non-Linux mail only, and has resulted in _all_ of
these messages ending up in my spam folder.

Thanks.

On Tue, Nov 28, 2017 at 03:53:17PM +0000, Russell King wrote:
> On Tue, Nov 28, 2017 at 02:29:32PM +0100, Antoine Tenart wrote:
> > From: Yan Markman <ymarkman@marvell.com>
> 
> Hi, thanks for the patch.
> 
> > When calling successively _connect, _disconnect and _connect again, if
> > the link configuration changed whilst being down from the phylink
> > perspective, the last _connect would stay in an incorrect old speed.
> > Fixes this by setting the link configuration parameters to an unknown
> > value when calling phylink_bringup_phy.
> 
> Under what circumstances does this occur?
> 
> > 
> > Fixes: 9525ae83959b ("phylink: add phylink infrastructure")
> > Signed-off-by: Yan Markman <ymarkman@marvell.com>
> > [Antoine: commit message]
> > Signed-off-by: Antoine Tenart <antoine.tenart@free-electrons.com>
> > ---
> >  drivers/net/phy/phylink.c | 10 ++++++++++
> >  1 file changed, 10 insertions(+)
> > 
> > diff --git a/drivers/net/phy/phylink.c b/drivers/net/phy/phylink.c
> > index e3bbc70372d3..c2cec3eef67d 100644
> > --- a/drivers/net/phy/phylink.c
> > +++ b/drivers/net/phy/phylink.c
> > @@ -621,6 +621,16 @@ static int phylink_bringup_phy(struct phylink *pl, struct phy_device *phy)
> >  	if (ret)
> >  		return ret;
> >  
> > +	/* On _disconnect, the phy state machine and phylink resolve
> > +	 * are stopped before executing full gracefull down/reset state.
> > +	 * The further _connect starts with incorrect init state. Let's set
> > +	 * init values here.
> > +	 */
> > +	pl->phy_state.link = false;
> > +	pl->link_config.pause = MLO_PAUSE_AN;
> > +	pl->link_config.speed = SPEED_UNKNOWN;
> > +	pl->link_config.duplex = DUPLEX_UNKNOWN;
> 
> It would be much better to clean up the phy_state in
> phylink_disconnect_phy() and trigger a resolve, rather than doing that
> each time a PHY is connected - the link should be taken down when the
> PHY is removed.
> 
> However, I'd like to know under what circumstances this is happening,
> since, if you're hotplugging a PHY you should be doing that via SFP
> which has additional link up/down handling.  What board is this with?
> 
> Also note that there's a number of patches in my "phy" branch that
> I'm intending to send as a result of working with Florian over the
> last few weeks.  There's several people working fairly independently
> in this area and having everyone send patches independently of each
> other could get painful to manage.
> 
> I'm intending to send patches once I know that net-next is open.
> 
> -- 
> Russell King
> ARM architecture Linux Kernel maintainer

-- 
Russell King
ARM architecture Linux Kernel maintainer