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

From: Russell King <rmk@armlinux.org.uk>
To: Antoine Tenart <antoine.tenart@free-electrons.com>
Cc: andrew@lunn.ch, f.fainelli@gmail.com, davem@davemloft.net,
	Yan Markman <ymarkman@marvell.com>,
	gregory.clement@free-electrons.com,
	thomas.petazzoni@free-electrons.com,
	miquel.raynal@free-electrons.com, nadavh@marvell.com,
	mw@semihalf.com, stefanc@marvell.com, netdev@vger.kernel.org,
	linux-kernel@vger.kernel.org
Subject: Re: [PATCH net] net: phylink: fix link state on phy-connect
Date: Tue, 28 Nov 2017 15:53:17 +0000	[thread overview]
Message-ID: <20171128155317.GA7974@flint.armlinux.org.uk> (raw)
In-Reply-To: <20171128132932.27196-1-antoine.tenart@free-electrons.com>

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