| Message ID | ad288d85-ea57-82bc-5711-1a0fbaad2aa8@gmail.com |
|---|---|
| State | Accepted |
| Delegated to: | David Miller |
| Headers | show |
| Series | [net-next] net: phy: fix autoneg mismatch case in genphy_read_status | expand |
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@gmail.com> Reviewed-by: Andrew Lunn <andrew@lunn.ch> Andrew
From: Heiner Kallweit <hkallweit1@gmail.com> Date: Tue, 2 Apr 2019 20:43:30 +0200 > 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@gmail.com> Applied.
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@gmail.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
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@gmail.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? 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? 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 > >
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@gmail.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 >> >> >
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@gmail.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;
On Fri, Apr 05, 2019 at 07:40:51PM +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@gmail.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? I added the following debug patch: diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c index 72fc714c9427..88c6cf79c438 100644 --- a/drivers/net/phy/phy_device.c +++ b/drivers/net/phy/phy_device.c @@ -1710,6 +1710,7 @@ int genphy_update_link(struct phy_device *phydev) */ if (!phy_polling_mode(phydev)) { status = phy_read(phydev, MII_BMSR); + pr_err("1: MII_BMSR=0x%08x\n", status); if (status < 0) { return status; } else if (status & BMSR_LSTATUS) { @@ -1720,6 +1721,7 @@ int genphy_update_link(struct phy_device *phydev) /* Read link and autonegotiation status */ status = phy_read(phydev, MII_BMSR); + pr_err("2: MII_BMSR=0x%08x\n", status); if (status < 0) return status; And the out put is as follows: # dmesg | egrep '(ravb|libphy|Mic)' [ 2.028963] libphy: Fixed MDIO Bus: probed [ 3.244122] ravb e6800000.ethernet: ignoring dependency for device, assuming no driver [ 3.256398] libphy: ravb_mii: probed [ 3.264867] ravb e6800000.ethernet eth0: Base address at 0xe6800000, 2e:09:0a:03:85:38, IRQ 104. [ 3.551385] 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) [ 3.569904] libphy: 1: MII_BMSR=0x00007949 [ 3.574397] libphy: 2: MII_BMSR=0x00007949 [ 9.899946] libphy: 1: MII_BMSR=0x0000796d [ 9.904786] ravb e6800000.ethernet eth0: Link is Up - Unknown/Unknown - flow control off # 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 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? > > Could you please also test with another link partner? With a different link partner it seems that the problem remains: root@Debian:~# dmesg | egrep '(ravb|libphy|Mic)' [ 2.023596] libphy: Fixed MDIO Bus: probed [ 3.243061] ravb e6800000.ethernet: ignoring dependency for device, assuming no driver [ 3.255157] libphy: ravb_mii: probed [ 3.263732] ravb e6800000.ethernet eth0: Base address at 0xe6800000, 2e:09:0a:03:85:38, IRQ 104. [ 3.543377] 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) [ 3.561950] libphy: 1: MII_BMSR=0x00007949 [ 3.566436] libphy: 2: MII_BMSR=0x00007949 [ 12.798173] libphy: 1: MII_BMSR=0x0000796d [ 12.802974] ravb e6800000.ethernet eth0: Link is Up - Unknown/Unknown - flow control off root@Debian:~# 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 > > 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 > > > > >
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@gmail.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; > Thanks, that seems to resolve the problem I observed. Using the same link partner as for the tests above I see: * 4950c2ba49cc + fix (above) + patch to debug values of MII_BMSR in genphy_update_link() # dmesg | egrep '(libphy|ravb|Micr)' [ 2.028167] libphy: Fixed MDIO Bus: probed [ 3.255678] ravb e6800000.ethernet: ignoring dependency for device, assuming no driver [ 3.267865] libphy: ravb_mii: probed [ 3.276210] ravb e6800000.ethernet eth0: Base address at 0xe6800000, 2e:09:0a:03:85:38, IRQ 104. [ 3.563438] 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) [ 3.581951] libphy: 1: MII_BMSR=0x00007949 [ 3.586439] libphy: 2: MII_BMSR=0x00007949 [ 9.985009] libphy: 1: MII_BMSR=0x0000796d [ 9.990101] ravb e6800000.ethernet eth0: Link is Up - 1Gbps/Full - flow control off # 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 * 4950c2ba49cc + fix (above) + patch to debug values of MII_BMSR in genphy_update_link() + 100Mbit/s limit patch # dmesg | egrep '(libphy|ravb|Micr)' [ 2.026010] libphy: Fixed MDIO Bus: probed [ 3.251741] ravb e6800000.ethernet: ignoring dependency for device, assuming no driver [ 3.264054] libphy: ravb_mii: probed [ 3.272573] ravb e6800000.ethernet eth0: Base address at 0xe6800000, 2e:09:0a:03:85:38, IRQ 104. [ 3.559364] 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) [ 3.577913] libphy: 1: MII_BMSR=0x00007949 [ 3.582407] libphy: 2: MII_BMSR=0x00007949 [ 5.663264] libphy: 1: MII_BMSR=0x0000796d [ 5.668004] 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
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@gmail.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; >> > > Thanks, that seems to resolve the problem I observed. Great, thanks for reporting and testing! I'll submit this fix then. Heiner
diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c index de2b6333e..7669a01b0 100644 --- a/drivers/net/phy/phy_device.c +++ b/drivers/net/phy/phy_device.c @@ -1723,10 +1723,8 @@ int genphy_update_link(struct phy_device *phydev) if (status < 0) return status; - if ((status & BMSR_LSTATUS) == 0) - phydev->link = 0; - else - phydev->link = 1; + phydev->link = status & BMSR_LSTATUS ? 1 : 0; + phydev->autoneg_complete = status & BMSR_ANEGCOMPLETE ? 1 : 0; return 0; } @@ -1757,7 +1755,7 @@ int genphy_read_status(struct phy_device *phydev) linkmode_zero(phydev->lp_advertising); - if (phydev->autoneg == AUTONEG_ENABLE && phydev->link) { + if (phydev->autoneg == AUTONEG_ENABLE && phydev->autoneg_complete) { if (linkmode_test_bit(ETHTOOL_LINK_MODE_1000baseT_Half_BIT, phydev->supported) || linkmode_test_bit(ETHTOOL_LINK_MODE_1000baseT_Full_BIT, diff --git a/include/linux/phy.h b/include/linux/phy.h index 34084892a..cfabd1a7c 100644 --- a/include/linux/phy.h +++ b/include/linux/phy.h @@ -390,6 +390,7 @@ struct phy_device { unsigned autoneg:1; /* The most recently read link state */ unsigned link:1; + unsigned autoneg_complete:1; /* Interrupts are enabled */ unsigned interrupts:1;
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@gmail.com> --- drivers/net/phy/phy_device.c | 8 +++----- include/linux/phy.h | 1 + 2 files changed, 4 insertions(+), 5 deletions(-)