On 08.04.2019 09:47, Simon Horman wrote:
> On Sun, Apr 07, 2019 at 05:09:28PM +0200, 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@xxxxxxxxx>
>>>
>>> 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;
>>
>
> Thanks, that seems to resolve the problem I observed.
Great, thanks for reporting and testing!
I'll submit this fix then.
Heiner
