lists.openwall.net   lists  /  announce  owl-users  owl-dev  john-users  john-dev  passwdqc-users  yescrypt  popa3d-users  /  oss-security  kernel-hardening  musl  sabotage  tlsify  passwords  /  crypt-dev  xvendor  /  Bugtraq  Full-Disclosure  linux-kernel  linux-netdev  linux-ext4  linux-hardening  PHC 
Open Source and information security mailing list archives
 
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Date:   Sat, 11 Apr 2020 16:28:24 -0700
From:   Jakub Kicinski <kuba@...nel.org>
To:     Clemens Gruber <clemens.gruber@...ruber.com>
Cc:     netdev@...r.kernel.org, Russell King <linux@...linux.org.uk>,
        Andrew Lunn <andrew@...n.ch>,
        Florian Fainelli <f.fainelli@...il.com>,
        Heiner Kallweit <hkallweit1@...il.com>,
        "David S . Miller" <davem@...emloft.net>,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] net: phy: marvell: Fix pause frame negotiation

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@...ruber.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?

Powered by blists - more mailing lists