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:   Sun, 7 Apr 2019 17:09:28 +0200
From:   Heiner Kallweit <hkallweit1@...il.com>
To:     Simon Horman <horms@...ge.net.au>
Cc:     Andrew Lunn <andrew@...n.ch>,
        Florian Fainelli <f.fainelli@...il.com>,
        David Miller <davem@...emloft.net>, netdev@...r.kernel.org,
        linux-renesas-soc@...r.kernel.org
Subject: Re: [PATCH net-next] net: phy: fix autoneg mismatch case in
 genphy_read_status

On 05.04.2019 18:16, Simon Horman wrote:
> On Tue, Apr 02, 2019 at 08:43:30PM +0200, Heiner Kallweit wrote:
>> The original patch didn't consider the case that autoneg process
>> finishes successfully but both link partners have no mode in common.
>> In this case there's no link, nevertheless we may be interested in
>> what the link partner advertised.
>>
>> Like phydev->link we set phydev->autoneg_complete in
>> genphy_update_link() and use the stored value in genphy_read_status().
>> This way we don't have to read register BMSR again.
>>
>> Fixes: b6163f194c69 ("net: phy: improve genphy_read_status")
>> Signed-off-by: Heiner Kallweit <hkallweit1@...il.com>
> 
> Hi,
> 
> I have observed a regression with this patch as present as
> 4950c2ba49cc ("net: phy: fix autoneg mismatch case in genphy_read_status")
> in net-next.
> 
> The platform is the Renesas R-Car E3 (r8a77990) based Ebisu-4D board.
> 
> 
> Without this patch (commit before 4950c2ba49cc) the link is negotiated
> as follows:
> 
> [    3.257699] libphy: ravb_mii: probed
> [    3.266498] ravb e6800000.ethernet eth0: Base address at 0xe6800000, 2e:09:0a:03:85:38, IRQ 104.
> [    3.537082] Micrel KSZ9031 Gigabit PHY e6800000.ethernet-ffffffff:00: attached PHY driver [Micrel KSZ9031 Gigabit PHY] (mii_bus:phy_addr=e6800000.ethernet-ffffffff:00, irq=165)
> [    9.667161] ravb e6800000.ethernet eth0: Link is Up - 1Gbps/Full - flow control off
> 
> # ethtool --version
> ethtool version 3.16
> # ethtool eth0
> Settings for eth0:
>         Supported ports: [ TP MII ]
>         Supported link modes:   100baseT/Full 
>                                 1000baseT/Full 
>         Supported pause frame use: No
>         Supports auto-negotiation: Yes
>         Advertised link modes:  100baseT/Full 
>                                 1000baseT/Full 
>         Advertised pause frame use: No
>         Advertised auto-negotiation: Yes
>         Link partner advertised link modes:  10baseT/Half 10baseT/Full 
>                                              100baseT/Half 100baseT/Full 
>                                              1000baseT/Full 
>         Link partner advertised pause frame use: No
>         Link partner advertised auto-negotiation: Yes
>         Speed: 1000Mb/s
>         Duplex: Full
>         Port: MII
>         PHYAD: 0
>         Transceiver: external
>         Auto-negotiation: on
>         Supports Wake-on: g
>         Wake-on: d
>         Current message level: 0x000000cc (204)
>                                link timer rx_err tx_err
>         Link detected: yes
> 
> 
> With this patch the link is negotiated as follows, note the "Unknown":
> 
> [    3.268609] libphy: ravb_mii: probed
> [    3.277402] ravb e6800000.ethernet eth0: Base address at 0xe6800000, 2e:09:0a:03:85:38, IRQ 104.
> [    3.565057] Micrel KSZ9031 Gigabit PHY e6800000.ethernet-ffffffff:00: attached PHY driver [Micrel KSZ9031 Gigabit PHY] (mii_bus:phy_addr=e6800000.ethernet-ffffffff:00, irq=165)
> [    9.804423] ravb e6800000.ethernet eth0: Link is Up - Unknown/Unknown - flow control off
> 
> And ethtool reports:
> 
> # ethtool eth0
> Settings for eth0:
>         Supported ports: [ TP MII ]
>         Supported link modes:   100baseT/Full 
>                                 1000baseT/Full 
>         Supported pause frame use: No
>         Supports auto-negotiation: Yes
>         Advertised link modes:  100baseT/Full 
>                                 1000baseT/Full 
>         Advertised pause frame use: No
>         Advertised auto-negotiation: Yes
>         Speed: Unknown!
>         Duplex: Unknown! (255)
>         Port: MII
>         PHYAD: 0
>         Transceiver: external
>         Auto-negotiation: on
>         Supports Wake-on: g
>         Wake-on: d
>         Current message level: 0x000000cc (204)
>                                link timer rx_err tx_err
>         Link detected: yes
> 
> 
> 
> I noticed this when preparing a patch limit the maximum speed the device to
> 100Mbit/s as follows:
> 
> --- a/arch/arm64/boot/dts/renesas/r8a77990-ebisu.dts
> +++ b/arch/arm64/boot/dts/renesas/r8a77990-ebisu.dts
> @@ -272,6 +272,7 @@
>                 interrupt-parent = <&gpio2>;
>                 interrupts = <21 IRQ_TYPE_LEVEL_LOW>;
>                 reset-gpios = <&gpio1 20 GPIO_ACTIVE_LOW>;
> +               max-speed = <100>;
>         };
>  };
>  
> While the 100Mbit/s limit applied on top of the commit before 4950c2ba49cc
> things work as expected:
> 
> [    3.258347] libphy: ravb_mii: probed
> [    3.266739] ravb e6800000.ethernet eth0: Base address at 0xe6800000, 2e:09:0a:03:85:38, IRQ 104.
> [    3.553642] Micrel KSZ9031 Gigabit PHY e6800000.ethernet-ffffffff:00: attached PHY driver [Micrel KSZ9031 Gigabit PHY] (mii_bus:phy_addr=e6800000.ethernet-ffffffff:00, irq=165)
> [    5.585027] ravb e6800000.ethernet eth0: Link is Up - 100Mbps/Full - flow control off
> 
> # ethtool eth0
> Settings for eth0:
>         Supported ports: [ TP MII ]
>         Supported link modes:   100baseT/Full 
>         Supported pause frame use: No
>         Supports auto-negotiation: Yes
>         Advertised link modes:  100baseT/Full 
>         Advertised pause frame use: No
>         Advertised auto-negotiation: Yes
>         Link partner advertised link modes:  10baseT/Half 10baseT/Full 
>                                              100baseT/Half 100baseT/Full 
>         Link partner advertised pause frame use: No
>         Link partner advertised auto-negotiation: Yes
>         Speed: 100Mb/s
>         Duplex: Full
>         Port: MII
>         PHYAD: 0
>         Transceiver: external
>         Auto-negotiation: on
>         Supports Wake-on: g
>         Wake-on: d
>         Current message level: 0x000000cc (204)
>                                link timer rx_err tx_err
>         Link detected: yes
> 
> 
> However, with the 100Mbit/s limit applied on top of 4950c2ba49cc the boot hangs
> while (or just after?) negotiating the link:
> 
> [    3.541549] Micrel KSZ9031 Gigabit PHY e6800000.ethernet-ffffffff:00: attached PHY driver [Micrel KSZ9031 Gigabit PHY] (mii_bus:phy_addr=e6800000.ethernet-ffffffff:00, irq=165)
> [    5.536389] ravb e6800000.ethernet eth0: Link is Up - Unknown/Unknown - flow control off
> 
> 

There's one path where phydev->autoneg_complete isn't set.
Could you please test whether the following fixes the issue?


diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index a6f3ad971..2df8f7737 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -1710,19 +1710,17 @@ int genphy_update_link(struct phy_device *phydev)
 	 */
 	if (!phy_polling_mode(phydev)) {
 		status = phy_read(phydev, MII_BMSR);
-		if (status < 0) {
+		if (status < 0)
 			return status;
-		} else if (status & BMSR_LSTATUS) {
-			phydev->link = 1;
-			return 0;
-		}
+		else if (status & BMSR_LSTATUS)
+			goto done;
 	}
 
 	/* Read link and autonegotiation status */
 	status = phy_read(phydev, MII_BMSR);
 	if (status < 0)
 		return status;
-
+done:
 	phydev->link = status & BMSR_LSTATUS ? 1 : 0;
 	phydev->autoneg_complete = status & BMSR_ANEGCOMPLETE ? 1 : 0;
 
-- 
2.21.0


Powered by blists - more mailing lists