* [PATCH net-next] net: phy: fix autoneg mismatch case in genphy_read_status
@ 2019-04-02 18:43 Heiner Kallweit
2019-04-02 20:40 ` Andrew Lunn
` (2 more replies)
0 siblings, 3 replies; 10+ messages in thread
From: Heiner Kallweit @ 2019-04-02 18:43 UTC (permalink / raw)
To: Andrew Lunn, Florian Fainelli, David Miller; +Cc: netdev
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(-)
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;
--
2.21.0
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH net-next] net: phy: fix autoneg mismatch case in genphy_read_status
2019-04-02 18:43 [PATCH net-next] net: phy: fix autoneg mismatch case in genphy_read_status Heiner Kallweit
@ 2019-04-02 20:40 ` Andrew Lunn
2019-04-04 4:48 ` David Miller
2019-04-05 16:16 ` Simon Horman
2 siblings, 0 replies; 10+ messages in thread
From: Andrew Lunn @ 2019-04-02 20:40 UTC (permalink / raw)
To: Heiner Kallweit; +Cc: Florian Fainelli, David Miller, netdev
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH net-next] net: phy: fix autoneg mismatch case in genphy_read_status
2019-04-02 18:43 [PATCH net-next] net: phy: fix autoneg mismatch case in genphy_read_status Heiner Kallweit
2019-04-02 20:40 ` Andrew Lunn
@ 2019-04-04 4:48 ` David Miller
2019-04-05 16:16 ` Simon Horman
2 siblings, 0 replies; 10+ messages in thread
From: David Miller @ 2019-04-04 4:48 UTC (permalink / raw)
To: hkallweit1; +Cc: andrew, f.fainelli, netdev
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.
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH net-next] net: phy: fix autoneg mismatch case in genphy_read_status
2019-04-02 18:43 [PATCH net-next] net: phy: fix autoneg mismatch case in genphy_read_status Heiner Kallweit
2019-04-02 20:40 ` Andrew Lunn
2019-04-04 4:48 ` David Miller
@ 2019-04-05 16:16 ` Simon Horman
2019-04-05 17:40 ` Heiner Kallweit
2019-04-07 15:09 ` Heiner Kallweit
2 siblings, 2 replies; 10+ messages in thread
From: Simon Horman @ 2019-04-05 16:16 UTC (permalink / raw)
To: Heiner Kallweit
Cc: Andrew Lunn, Florian Fainelli, David Miller, netdev, linux-renesas-soc
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH net-next] net: phy: fix autoneg mismatch case in genphy_read_status
2019-04-05 16:16 ` Simon Horman
@ 2019-04-05 17:40 ` Heiner Kallweit
2019-04-06 17:57 ` Heiner Kallweit
2019-04-08 7:46 ` Simon Horman
2019-04-07 15:09 ` Heiner Kallweit
1 sibling, 2 replies; 10+ messages in thread
From: Heiner Kallweit @ 2019-04-05 17:40 UTC (permalink / raw)
To: Simon Horman
Cc: Andrew Lunn, Florian Fainelli, David Miller, netdev, linux-renesas-soc
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
>
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH net-next] net: phy: fix autoneg mismatch case in genphy_read_status
2019-04-05 17:40 ` Heiner Kallweit
@ 2019-04-06 17:57 ` Heiner Kallweit
2019-04-08 7:46 ` Simon Horman
1 sibling, 0 replies; 10+ messages in thread
From: Heiner Kallweit @ 2019-04-06 17:57 UTC (permalink / raw)
To: Simon Horman
Cc: Andrew Lunn, Florian Fainelli, David Miller, netdev, linux-renesas-soc
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
>>
>>
>
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH net-next] net: phy: fix autoneg mismatch case in genphy_read_status
2019-04-05 16:16 ` Simon Horman
2019-04-05 17:40 ` Heiner Kallweit
@ 2019-04-07 15:09 ` Heiner Kallweit
2019-04-08 7:47 ` Simon Horman
1 sibling, 1 reply; 10+ messages in thread
From: Heiner Kallweit @ 2019-04-07 15:09 UTC (permalink / raw)
To: Simon Horman
Cc: Andrew Lunn, Florian Fainelli, David Miller, netdev, linux-renesas-soc
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;
--
2.21.0
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH net-next] net: phy: fix autoneg mismatch case in genphy_read_status
2019-04-05 17:40 ` Heiner Kallweit
2019-04-06 17:57 ` Heiner Kallweit
@ 2019-04-08 7:46 ` Simon Horman
1 sibling, 0 replies; 10+ messages in thread
From: Simon Horman @ 2019-04-08 7:46 UTC (permalink / raw)
To: Heiner Kallweit
Cc: Andrew Lunn, Florian Fainelli, David Miller, netdev, linux-renesas-soc
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
> >
> >
>
^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH net-next] net: phy: fix autoneg mismatch case in genphy_read_status
2019-04-07 15:09 ` Heiner Kallweit
@ 2019-04-08 7:47 ` Simon Horman
2019-04-08 17:29 ` Heiner Kallweit
0 siblings, 1 reply; 10+ messages in thread
From: Simon Horman @ 2019-04-08 7:47 UTC (permalink / raw)
To: Heiner Kallweit
Cc: Andrew Lunn, Florian Fainelli, David Miller, netdev, linux-renesas-soc
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
^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH net-next] net: phy: fix autoneg mismatch case in genphy_read_status
2019-04-08 7:47 ` Simon Horman
@ 2019-04-08 17:29 ` Heiner Kallweit
0 siblings, 0 replies; 10+ messages in thread
From: Heiner Kallweit @ 2019-04-08 17:29 UTC (permalink / raw)
To: Simon Horman
Cc: Andrew Lunn, Florian Fainelli, David Miller, netdev, linux-renesas-soc
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
^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2019-04-08 17:29 UTC | newest]
Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-04-02 18:43 [PATCH net-next] net: phy: fix autoneg mismatch case in genphy_read_status Heiner Kallweit
2019-04-02 20:40 ` Andrew Lunn
2019-04-04 4:48 ` David Miller
2019-04-05 16:16 ` Simon Horman
2019-04-05 17:40 ` Heiner Kallweit
2019-04-06 17:57 ` Heiner Kallweit
2019-04-08 7:46 ` Simon Horman
2019-04-07 15:09 ` Heiner Kallweit
2019-04-08 7:47 ` Simon Horman
2019-04-08 17:29 ` Heiner Kallweit
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.