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, 6 Apr 2019 19:57:32 +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 19:40, Heiner Kallweit wrote:
> 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
>>
> Looks like the PHY doesn't set the "aneg complete" bit. But then, how
> can the link be up? Could you please add a debug output in
> genphy_update_link() printing the value of register BMSR?

There's also an easier way, you can switch on tracing by
echo 1 > /sys/kernel/debug/tracing/events/mdio/mdio_access/enable
and then find the trace in
/sys/kernel/debug/tracing/trace

> I wonder whether your case may be caused by a chip erratum. Item 5 in
> the errata documentation
> (http://ww1.microchip.com/downloads/en/DeviceDoc/80000692D.pdf)
> may be a candidate. Could you please check (as explained in the
> errata document) and report the exact PHY version?
> 
This erratum is taken care of in the phy driver already, so that
doesn't seem to be the reason.

> Could you please also test with another link partner?
> 
>> 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 
> I wondered why the link partner suddenly doesn't report 1Gbps support.
> Seems to be a small flaw in phylib, if locally 1Gbps isn't supported
> (or support is disabled) then we don't check for the link partners
> 1Gbps capability.
> 
>>         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
>>
>>
> 

Powered by blists - more mailing lists