Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Sat, 11 Apr 2020 16:28:24 -0700
From:   Jakub Kicinski <kuba@kernel.org>
To:     Clemens Gruber <clemens.gruber@pqgruber.com>
Cc:     netdev@vger.kernel.org, Russell King <linux@armlinux.org.uk>,
        Andrew Lunn <andrew@lunn.ch>,
        Florian Fainelli <f.fainelli@gmail.com>,
        Heiner Kallweit <hkallweit1@gmail.com>,
        "David S . Miller" <davem@davemloft.net>,
        linux-kernel@vger.kernel.org
Subject: Re: [PATCH v2] net: phy: marvell: Fix pause frame negotiation
Message-ID: <20200411162824.59791b84@kicinski-fedora-pc1c0hjn.dhcp.thefacebook.com>
In-Reply-To: <20200411165125.1091-1-clemens.gruber@pqgruber.com>
References: <20200411165125.1091-1-clemens.gruber@pqgruber.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Sat, 11 Apr 2020 18:51:25 +0200 Clemens Gruber wrote:
> The negotiation of flow control / pause frame modes was broken since
> commit fcf1f59afc67 ("net: phy: marvell: rearrange to use
> genphy_read_lpa()") moved the setting of phydev->duplex below the
> phy_resolve_aneg_pause call. Due to a check of DUPLEX_FULL in that
> function, phydev->pause was no longer set.
> 
> Fix it by moving the parsing of the status variable before the blocks
> dealing with the pause frames.
> 
> As the Marvell 88E1510 datasheet does not specify the timing between the
> link status and the "Speed and Duplex Resolved" bit, we have to force
> the link down as long as the resolved bit is not set, to avoid reporting
> link up before we even have valid Speed/Duplex.
> 
> Tested with a Marvell 88E1510 (RGMII to Copper/1000Base-T)
> 
> Fixes: fcf1f59afc67 ("net: phy: marvell: rearrange to use genphy_read_lpa()")
> Signed-off-by: Clemens Gruber <clemens.gruber@pqgruber.com>
> ---
> Changes since v1:
> - Force link to 0 if resolved bit is not set as suggested by Russell King
> 
>  drivers/net/phy/marvell.c | 46 ++++++++++++++++++++-------------------
>  1 file changed, 24 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/net/phy/marvell.c b/drivers/net/phy/marvell.c
> index 9a8badafea8a..561df5e33f65 100644
> --- a/drivers/net/phy/marvell.c
> +++ b/drivers/net/phy/marvell.c
> @@ -1278,6 +1278,30 @@ static int marvell_read_status_page_an(struct phy_device *phydev,
>  	int lpa;
>  	int err;
>  
> +	if (!(status & MII_M1011_PHY_STATUS_RESOLVED)) {
> +		phydev->link = 0;
> +		return 0;
> +	}

This doesn't address my comment, so was I wrong? What I was trying to
say is that the function updates the established link info as well as
autoneg advertising info. If the link is not resolved we can't read the
link info, but we should still report the advertising modes. No?