Re: [PATCH v3 net-next] net: phy: realtek: read actual speed on rtl8211f to detect downshift
Hi, Russell: I found this message in kernel log, thanks! On 2020/11/26 1:07, Russell King - ARM Linux admin wrote: On Thu, Nov 26, 2020 at 12:57:37AM +0800, Yonglong Liu wrote: Hi, Antonio: Could you help to provide a downshift warning message when this happen? It's a little strange that the adv and the lpa support 1000M, but finally the link speed is 100M. That is an identifying feature of downshift. Downshift can happen at either end of the link, and since we must not change the "Advertised link modes" since this is what userspace configured, if a downshift occurs at the local end, then you will get the ethtool output you provide, where the speed does not agree with the reported advertisements. You should already be getting a warning in the kernel log when this happens; phy_check_downshift() which is part of the phylib core code will check this every time the link comes up. You should already have a message "Downshift occurred ..." in your kernel log. Please check.
Re: [PATCH v3 net-next] net: phy: realtek: read actual speed on rtl8211f to detect downshift
Hi, Antonio:
Could you help to provide a downshift warning message when this
happen?
It's a little strange that the adv and the lpa support 1000M, but
finally the link speed is 100M.
Settings for eth5:
Supported ports: [ TP ]
Supported link modes: 10baseT/Half 10baseT/Full
100baseT/Half 100baseT/Full
1000baseT/Full
Supported pause frame use: Symmetric Receive-only
Supports auto-negotiation: Yes
Supported FEC modes: Not reported
*Advertised link modes: 10baseT/Half 10baseT/Full
100baseT/Half 100baseT/Full
1000baseT/Full*
Advertised pause frame use: Symmetric
Advertised auto-negotiation: Yes
Advertised FEC modes: Not reported
*Link partner advertised link modes: 10baseT/Half 10baseT/Full
100baseT/Half 100baseT/Full
1000baseT/Full*
Link partner advertised pause frame use: Symmetric
Link partner advertised auto-negotiation: Yes
Link partner advertised FEC modes: Not reported
*Speed: 100Mb/s*
Duplex: Full
Port: MII
PHYAD: 3
Transceiver: internal
Auto-negotiation: on
Current message level: 0x0036 (54)
probe link ifdown ifup
Link detected: yes
On 2020/11/25 23:03, Yonglong Liu wrote:
Tested-by: Yonglong Liu
On 2020/11/25 7:07, Antonio Borneo wrote:
The rtl8211f supports downshift and before commit 5502b218e001
("net: phy: use phy_resolve_aneg_linkmode in genphy_read_status")
the read-back of register MII_CTRL1000 was used to detect the
negotiated link speed.
The code added in commit d445dff2df60 ("net: phy: realtek: read
actual speed to detect downshift") is working fine also for this
phy and it's trivial re-using it to restore the downshift
detection on rtl8211f.
Add the phy specific read_status() pointing to the existing
function rtlgen_read_status().
Signed-off-by: Antonio Borneo
Link:
https://lore.kernel.org/r/478f871a-583d-01f1-9cc5-2eea56d8c...@huawei.com
---
To: Andrew Lunn
To: Heiner Kallweit
To: Russell King
To: "David S. Miller"
To: Jakub Kicinski
To: net...@vger.kernel.org
To: Yonglong Liu
To: Willy Liu
Cc: linux...@huawei.com
Cc: Salil Mehta
Cc: linux-st...@st-md-mailman.stormreply.com
Cc: linux-kernel@vger.kernel.org
In-Reply-To: <20201124143848.874894-1-antonio.bor...@st.com>
V1 => V2
move from a generic implementation affecting every phy
to a rtl8211f specific implementation
V2 => V3
rebase on netdev-next, resolving minor conflict after
merge of 8b43357fff61
---
drivers/net/phy/realtek.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/net/phy/realtek.c b/drivers/net/phy/realtek.c
index f71eda945c6a..99ecd6c4c15a 100644
--- a/drivers/net/phy/realtek.c
+++ b/drivers/net/phy/realtek.c
@@ -729,6 +729,7 @@ static struct phy_driver realtek_drvs[] = {
PHY_ID_MATCH_EXACT(0x001cc916),
.name = "RTL8211F Gigabit Ethernet",
.config_init = &rtl8211f_config_init,
+ .read_status = rtlgen_read_status,
.config_intr = &rtl8211f_config_intr,
.handle_interrupt = rtl8211f_handle_interrupt,
.suspend = genphy_suspend,
base-commit: 1d155dfdf50efc2b0793bce93c06d1a5b23d0877
___
Linuxarm mailing list
linux...@huawei.com
http://hulk.huawei.com/mailman/listinfo/linuxarm
.
Re: [PATCH v3 net-next] net: phy: realtek: read actual speed on rtl8211f to detect downshift
Tested-by: Yonglong Liu
On 2020/11/25 7:07, Antonio Borneo wrote:
The rtl8211f supports downshift and before commit 5502b218e001
("net: phy: use phy_resolve_aneg_linkmode in genphy_read_status")
the read-back of register MII_CTRL1000 was used to detect the
negotiated link speed.
The code added in commit d445dff2df60 ("net: phy: realtek: read
actual speed to detect downshift") is working fine also for this
phy and it's trivial re-using it to restore the downshift
detection on rtl8211f.
Add the phy specific read_status() pointing to the existing
function rtlgen_read_status().
Signed-off-by: Antonio Borneo
Link: https://lore.kernel.org/r/478f871a-583d-01f1-9cc5-2eea56d8c...@huawei.com
---
To: Andrew Lunn
To: Heiner Kallweit
To: Russell King
To: "David S. Miller"
To: Jakub Kicinski
To: net...@vger.kernel.org
To: Yonglong Liu
To: Willy Liu
Cc: linux...@huawei.com
Cc: Salil Mehta
Cc: linux-st...@st-md-mailman.stormreply.com
Cc: linux-kernel@vger.kernel.org
In-Reply-To: <20201124143848.874894-1-antonio.bor...@st.com>
V1 => V2
move from a generic implementation affecting every phy
to a rtl8211f specific implementation
V2 => V3
rebase on netdev-next, resolving minor conflict after
merge of 8b43357fff61
---
drivers/net/phy/realtek.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/drivers/net/phy/realtek.c b/drivers/net/phy/realtek.c
index f71eda945c6a..99ecd6c4c15a 100644
--- a/drivers/net/phy/realtek.c
+++ b/drivers/net/phy/realtek.c
@@ -729,6 +729,7 @@ static struct phy_driver realtek_drvs[] = {
PHY_ID_MATCH_EXACT(0x001cc916),
.name = "RTL8211F Gigabit Ethernet",
.config_init= &rtl8211f_config_init,
+ .read_status= rtlgen_read_status,
.config_intr= &rtl8211f_config_intr,
.handle_interrupt = rtl8211f_handle_interrupt,
.suspend= genphy_suspend,
base-commit: 1d155dfdf50efc2b0793bce93c06d1a5b23d0877
Re: [question] net: phy: rtl8211f: link speed shows 1000Mb/s but actual link speed in phy is 100Mb/s
On 2020/5/13 9:59, Andrew Lunn wrote: > On Wed, May 13, 2020 at 09:34:13AM +0800, Yonglong Liu wrote: >> Hi, Andrew: >> Thanks for your reply! >> >> On 2020/5/12 22:00, Andrew Lunn wrote: >>> On Tue, May 12, 2020 at 08:48:21PM +0800, Yonglong Liu wrote: >>>> I use two devices, both support 1000M speed, they are directly connected >>>> with a network cable. Two devices enable autoneg, and then do the following >>>> test repeatedly: >>>>ifconfig eth5 down >>>>ifconfig eth5 up >>>>sleep $((RANDOM%6)) >>>>ifconfig eth5 down >>>>ifconfig eth5 up >>>>sleep 10 >>>> >>>> With low probability, one device A link up with 100Mb/s, the other B link >>>> up with >>>> 1000Mb/s(the actual link speed read from phy is 100Mb/s), and the network >>>> can >>>> not work. >>>> >>>> device A: >>>> Settings for eth5: >>>> Supported ports: [ TP ] >>>> Supported link modes: 10baseT/Half 10baseT/Full >>>> 100baseT/Half 100baseT/Full >>>> 1000baseT/Full >>>> Supported pause frame use: Symmetric Receive-only >>>> Supports auto-negotiation: Yes >>>> Supported FEC modes: Not reported >>>> Advertised link modes: 10baseT/Half 10baseT/Full >>>> 100baseT/Half 100baseT/Full >>>> 1000baseT/Full >>>> Advertised pause frame use: Symmetric >>>> Advertised auto-negotiation: Yes >>>> Advertised FEC modes: Not reported >>>> Link partner advertised link modes: 10baseT/Half 10baseT/Full >>>> 100baseT/Half 100baseT/Full >>>> Link partner advertised pause frame use: Symmetric >>>> Link partner advertised auto-negotiation: Yes >>>> Link partner advertised FEC modes: Not reported >>>> Speed: 100Mb/s >>>> Duplex: Full >>>> Port: MII >>>> PHYAD: 3 >>>> Transceiver: internal >>>> Auto-negotiation: on >>>> Current message level: 0x0036 (54) >>>>probe link ifdown ifup >>>> Link detected: yes >>>> >>>> The regs value read from mdio are: >>>> reg 9 = 0x200 >>>> reg a = 0 >>>> >>>> device B: >>>> Settings for eth5: >>>> Supported ports: [ TP ] >>>> Supported link modes: 10baseT/Half 10baseT/Full >>>> 100baseT/Half 100baseT/Full >>>> 1000baseT/Full >>>> Supported pause frame use: Symmetric Receive-only >>>> Supports auto-negotiation: Yes >>>> Supported FEC modes: Not reported >>>> Advertised link modes: 10baseT/Half 10baseT/Full >>>> 100baseT/Half 100baseT/Full >>>> 1000baseT/Full >>>> Advertised pause frame use: Symmetric >>>> Advertised auto-negotiation: Yes >>>> Advertised FEC modes: Not reported >>>> Link partner advertised link modes: 10baseT/Half 10baseT/Full >>>> 100baseT/Half 100baseT/Full >>>> 1000baseT/Full >>>> Link partner advertised pause frame use: Symmetric >>>> Link partner advertised auto-negotiation: Yes >>>> Link partner advertised FEC modes: Not reported >>>> Speed: 1000Mb/s >>>> Duplex: Full >>>> Port: MII >>>> PHYAD: 3 >>>> Transceiver: internal >>>> Auto-negotiation: on >>>> Current message level: 0x0036 (54) >>>>probe link ifdown ifup >>>> Link detected: yes >>>> >>>> The regs value read from mdio are: >>>> reg 9 = 0 >>>> reg a = 0x800 >>>> >>>> I had talk to the FAE of rtl8211f, they said if negotiation failed with >>>> 1000Mb/s, >>>> rtl8211f
Re: [question] net: phy: rtl8211f: link speed shows 1000Mb/s but actual link speed in phy is 100Mb/s
Hi, Andrew: Thanks for your reply! On 2020/5/12 22:00, Andrew Lunn wrote: > On Tue, May 12, 2020 at 08:48:21PM +0800, Yonglong Liu wrote: >> I use two devices, both support 1000M speed, they are directly connected >> with a network cable. Two devices enable autoneg, and then do the following >> test repeatedly: >> ifconfig eth5 down >> ifconfig eth5 up >> sleep $((RANDOM%6)) >> ifconfig eth5 down >> ifconfig eth5 up >> sleep 10 >> >> With low probability, one device A link up with 100Mb/s, the other B link up >> with >> 1000Mb/s(the actual link speed read from phy is 100Mb/s), and the network can >> not work. >> >> device A: >> Settings for eth5: >> Supported ports: [ TP ] >> Supported link modes: 10baseT/Half 10baseT/Full >> 100baseT/Half 100baseT/Full >> 1000baseT/Full >> Supported pause frame use: Symmetric Receive-only >> Supports auto-negotiation: Yes >> Supported FEC modes: Not reported >> Advertised link modes: 10baseT/Half 10baseT/Full >> 100baseT/Half 100baseT/Full >> 1000baseT/Full >> Advertised pause frame use: Symmetric >> Advertised auto-negotiation: Yes >> Advertised FEC modes: Not reported >> Link partner advertised link modes: 10baseT/Half 10baseT/Full >> 100baseT/Half 100baseT/Full >> Link partner advertised pause frame use: Symmetric >> Link partner advertised auto-negotiation: Yes >> Link partner advertised FEC modes: Not reported >> Speed: 100Mb/s >> Duplex: Full >> Port: MII >> PHYAD: 3 >> Transceiver: internal >> Auto-negotiation: on >> Current message level: 0x0036 (54) >>probe link ifdown ifup >> Link detected: yes >> >> The regs value read from mdio are: >> reg 9 = 0x200 >> reg a = 0 >> >> device B: >> Settings for eth5: >> Supported ports: [ TP ] >> Supported link modes: 10baseT/Half 10baseT/Full >> 100baseT/Half 100baseT/Full >> 1000baseT/Full >> Supported pause frame use: Symmetric Receive-only >> Supports auto-negotiation: Yes >> Supported FEC modes: Not reported >> Advertised link modes: 10baseT/Half 10baseT/Full >> 100baseT/Half 100baseT/Full >> 1000baseT/Full >> Advertised pause frame use: Symmetric >> Advertised auto-negotiation: Yes >> Advertised FEC modes: Not reported >> Link partner advertised link modes: 10baseT/Half 10baseT/Full >> 100baseT/Half 100baseT/Full >> 1000baseT/Full >> Link partner advertised pause frame use: Symmetric >> Link partner advertised auto-negotiation: Yes >> Link partner advertised FEC modes: Not reported >> Speed: 1000Mb/s >> Duplex: Full >> Port: MII >> PHYAD: 3 >> Transceiver: internal >> Auto-negotiation: on >> Current message level: 0x0036 (54) >>probe link ifdown ifup >> Link detected: yes >> >> The regs value read from mdio are: >> reg 9 = 0 >> reg a = 0x800 >> >> I had talk to the FAE of rtl8211f, they said if negotiation failed with >> 1000Mb/s, >> rtl8211f will change reg 9 to 0, than try to negotiation with 100Mb/s. >> >> The problem happened as: >> ifconfig eth5 up -> phy_start -> phy_start_aneg -> >> phy_modify_changed(MII_CTRL1000) >> (this time both A and B, reg 9 = 0x200) -> wait for link up -> (B: reg 9 >> changed to 0) >> -> link up. > > This sounds like downshift, but not correctly working. 1Gbps requires > that 4 pairs in the cable work. If a 1Gbps link is negotiated, but > then does not establish because one of the pairs is broken, some PHYs > will try to 'downshift'. They drop down to 100Mbps, which only > requires two pairs of the cable to work. To do this, the PHY should > change what it is advertising, to no longer advertise 1G, just 100M > and 10M. The link partner should t
[question] net: phy: rtl8211f: link speed shows 1000Mb/s but actual link speed in phy is 100Mb/s
I use two devices, both support 1000M speed, they are directly connected with a network cable. Two devices enable autoneg, and then do the following test repeatedly: ifconfig eth5 down ifconfig eth5 up sleep $((RANDOM%6)) ifconfig eth5 down ifconfig eth5 up sleep 10 With low probability, one device A link up with 100Mb/s, the other B link up with 1000Mb/s(the actual link speed read from phy is 100Mb/s), and the network can not work. device A: Settings for eth5: Supported ports: [ TP ] Supported link modes: 10baseT/Half 10baseT/Full 100baseT/Half 100baseT/Full 1000baseT/Full Supported pause frame use: Symmetric Receive-only Supports auto-negotiation: Yes Supported FEC modes: Not reported Advertised link modes: 10baseT/Half 10baseT/Full 100baseT/Half 100baseT/Full 1000baseT/Full Advertised pause frame use: Symmetric Advertised auto-negotiation: Yes Advertised FEC modes: Not reported Link partner advertised link modes: 10baseT/Half 10baseT/Full 100baseT/Half 100baseT/Full Link partner advertised pause frame use: Symmetric Link partner advertised auto-negotiation: Yes Link partner advertised FEC modes: Not reported Speed: 100Mb/s Duplex: Full Port: MII PHYAD: 3 Transceiver: internal Auto-negotiation: on Current message level: 0x0036 (54) probe link ifdown ifup Link detected: yes The regs value read from mdio are: reg 9 = 0x200 reg a = 0 device B: Settings for eth5: Supported ports: [ TP ] Supported link modes: 10baseT/Half 10baseT/Full 100baseT/Half 100baseT/Full 1000baseT/Full Supported pause frame use: Symmetric Receive-only Supports auto-negotiation: Yes Supported FEC modes: Not reported Advertised link modes: 10baseT/Half 10baseT/Full 100baseT/Half 100baseT/Full 1000baseT/Full Advertised pause frame use: Symmetric Advertised auto-negotiation: Yes Advertised FEC modes: Not reported Link partner advertised link modes: 10baseT/Half 10baseT/Full 100baseT/Half 100baseT/Full 1000baseT/Full Link partner advertised pause frame use: Symmetric Link partner advertised auto-negotiation: Yes Link partner advertised FEC modes: Not reported Speed: 1000Mb/s Duplex: Full Port: MII PHYAD: 3 Transceiver: internal Auto-negotiation: on Current message level: 0x0036 (54) probe link ifdown ifup Link detected: yes The regs value read from mdio are: reg 9 = 0 reg a = 0x800 I had talk to the FAE of rtl8211f, they said if negotiation failed with 1000Mb/s, rtl8211f will change reg 9 to 0, than try to negotiation with 100Mb/s. The problem happened as: ifconfig eth5 up -> phy_start -> phy_start_aneg -> phy_modify_changed(MII_CTRL1000) (this time both A and B, reg 9 = 0x200) -> wait for link up -> (B: reg 9 changed to 0) -> link up. I think this is the bug of the rtl8211f itself, any one have an idea to avoid this bug? When link up, update phydev->advertising before notify the eth driver, is this method suitable? (phydev->advertising is config from user, if user just set one speed 1000M, it's hard to )
[PATCH net] net: phy: Fix "link partner" information disappear issue
Some drivers just call phy_ethtool_ksettings_set() to set the
links, for those phy drivers that use genphy_read_status(), if
autoneg is on, and the link is up, than execute "ethtool -s
ethx autoneg on" will cause "link partner" information disappear.
The call trace is phy_ethtool_ksettings_set()->phy_start_aneg()
->linkmode_zero(phydev->lp_advertising)->genphy_read_status(),
the link didn't change, so genphy_read_status() just return, and
phydev->lp_advertising is zero now.
This patch moves the clear operation of lp_advertising from
phy_start_aneg() to genphy_read_lpa()/genphy_c45_read_lpa(), and
if autoneg on and autoneg not complete, just clear what the
generic functions care about.
Fixes: 88d6272acaaa ("net: phy: avoid unneeded MDIO reads in
genphy_read_status")
Signed-off-by: Yonglong Liu
---
drivers/net/phy/phy-c45.c| 2 ++
drivers/net/phy/phy.c| 3 ---
drivers/net/phy/phy_device.c | 11 ++-
3 files changed, 12 insertions(+), 4 deletions(-)
diff --git a/drivers/net/phy/phy-c45.c b/drivers/net/phy/phy-c45.c
index 7935593..a1caeee 100644
--- a/drivers/net/phy/phy-c45.c
+++ b/drivers/net/phy/phy-c45.c
@@ -323,6 +323,8 @@ int genphy_c45_read_pma(struct phy_device *phydev)
{
int val;
+ linkmode_zero(phydev->lp_advertising);
+
val = phy_read_mmd(phydev, MDIO_MMD_PMAPMD, MDIO_CTRL1);
if (val < 0)
return val;
diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index 119e6f4..105d389b 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c
@@ -572,9 +572,6 @@ int phy_start_aneg(struct phy_device *phydev)
if (AUTONEG_DISABLE == phydev->autoneg)
phy_sanitize_settings(phydev);
- /* Invalidate LP advertising flags */
- linkmode_zero(phydev->lp_advertising);
-
err = phy_config_aneg(phydev);
if (err < 0)
goto out_unlock;
diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 9d2bbb1..adb66a2 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -1787,7 +1787,14 @@ int genphy_read_lpa(struct phy_device *phydev)
{
int lpa, lpagb;
- if (phydev->autoneg == AUTONEG_ENABLE && phydev->autoneg_complete) {
+ if (phydev->autoneg == AUTONEG_ENABLE) {
+ if (!phydev->autoneg_complete) {
+ mii_stat1000_mod_linkmode_lpa_t(phydev->lp_advertising,
+ 0);
+ mii_lpa_mod_linkmode_lpa_t(phydev->lp_advertising, 0);
+ return 0;
+ }
+
if (phydev->is_gigabit_capable) {
lpagb = phy_read(phydev, MII_STAT1000);
if (lpagb < 0)
@@ -1815,6 +1822,8 @@ int genphy_read_lpa(struct phy_device *phydev)
return lpa;
mii_lpa_mod_linkmode_lpa_t(phydev->lp_advertising, lpa);
+ } else {
+ linkmode_zero(phydev->lp_advertising);
}
return 0;
--
2.8.1
Re: [RFC PATCH V2 net] net: phy: Fix "link partner" information disappear issue
On 2019/10/16 4:02, Heiner Kallweit wrote:
> On 14.10.2019 14:56, Yonglong Liu wrote:
>> Some drivers just call phy_ethtool_ksettings_set() to set the
>> links, for those phy drivers that use genphy_read_status(), if
>> autoneg is on, and the link is up, than execute "ethtool -s
>> ethx autoneg on" will cause "link partner" information disappear.
>>
>> The call trace is phy_ethtool_ksettings_set()->phy_start_aneg()
>> ->linkmode_zero(phydev->lp_advertising)->genphy_read_status(),
>> the link didn't change, so genphy_read_status() just return, and
>> phydev->lp_advertising is zero now.
>>
>> This patch moves the clear operation of lp_advertising from
>> phy_start_aneg() to genphy_read_lpa()/genphy_c45_read_lpa(), and
>> if autoneg on and autoneg not complete, just clear what the
>> generic functions care about.
>>
>> Fixes: 88d6272acaaa ("net: phy: avoid unneeded MDIO reads in
>> genphy_read_status")
>> Signed-off-by: Yonglong Liu
>>
> Looks good to me, two small nits below.
>
>
Thanks! Will fix the two small nits and send it to "net" branch.
>> ---
>> change log:
>> V2: moves the clear operation of lp_advertising from
>> phy_start_aneg() to genphy_read_lpa()/genphy_c45_read_lpa(), and
>> if autoneg on and autoneg not complete, just clear what the
>> generic functions care about. Suggested by Heiner Kallweit.
>> ---
>> ---
> This line seems to be duplicated.
>
>> drivers/net/phy/phy-c45.c| 2 ++
>> drivers/net/phy/phy.c| 3 ---
>> drivers/net/phy/phy_device.c | 12 +++-
>> 3 files changed, 13 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/net/phy/phy-c45.c b/drivers/net/phy/phy-c45.c
>> index 7935593..a1caeee 100644
>> --- a/drivers/net/phy/phy-c45.c
>> +++ b/drivers/net/phy/phy-c45.c
>> @@ -323,6 +323,8 @@ int genphy_c45_read_pma(struct phy_device *phydev)
>> {
>> int val;
>>
>> +linkmode_zero(phydev->lp_advertising);
>> +
>> val = phy_read_mmd(phydev, MDIO_MMD_PMAPMD, MDIO_CTRL1);
>> if (val < 0)
>> return val;
>> diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
>> index 119e6f4..105d389b 100644
>> --- a/drivers/net/phy/phy.c
>> +++ b/drivers/net/phy/phy.c
>> @@ -572,9 +572,6 @@ int phy_start_aneg(struct phy_device *phydev)
>> if (AUTONEG_DISABLE == phydev->autoneg)
>> phy_sanitize_settings(phydev);
>>
>> -/* Invalidate LP advertising flags */
>> -linkmode_zero(phydev->lp_advertising);
>> -
>> err = phy_config_aneg(phydev);
>> if (err < 0)
>> goto out_unlock;
>> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
>> index 9d2bbb1..4b43466 100644
>> --- a/drivers/net/phy/phy_device.c
>> +++ b/drivers/net/phy/phy_device.c
>> @@ -1787,7 +1787,14 @@ int genphy_read_lpa(struct phy_device *phydev)
>> {
>> int lpa, lpagb;
>>
>> -if (phydev->autoneg == AUTONEG_ENABLE && phydev->autoneg_complete) {
>> +if (phydev->autoneg == AUTONEG_ENABLE) {
>> +if (!phydev->autoneg_complete) {
>> +mii_stat1000_mod_linkmode_lpa_t(phydev->lp_advertising,
>> +0);
>> +mii_lpa_mod_linkmode_lpa_t(phydev->lp_advertising, 0);
>> +return 0;
>> +}
>> +
>> if (phydev->is_gigabit_capable) {
>> lpagb = phy_read(phydev, MII_STAT1000);
>> if (lpagb < 0)
>> @@ -1816,6 +1823,9 @@ int genphy_read_lpa(struct phy_device *phydev)
>>
>> mii_lpa_mod_linkmode_lpa_t(phydev->lp_advertising, lpa);
>> }
>> +else {
>
> "} else {" should be on one line.
>
>> +linkmode_zero(phydev->lp_advertising);
>> +}
>>
>> return 0;
>> }
>>
>
>
> .
>
[RFC PATCH V2 net] net: phy: Fix "link partner" information disappear issue
Some drivers just call phy_ethtool_ksettings_set() to set the
links, for those phy drivers that use genphy_read_status(), if
autoneg is on, and the link is up, than execute "ethtool -s
ethx autoneg on" will cause "link partner" information disappear.
The call trace is phy_ethtool_ksettings_set()->phy_start_aneg()
->linkmode_zero(phydev->lp_advertising)->genphy_read_status(),
the link didn't change, so genphy_read_status() just return, and
phydev->lp_advertising is zero now.
This patch moves the clear operation of lp_advertising from
phy_start_aneg() to genphy_read_lpa()/genphy_c45_read_lpa(), and
if autoneg on and autoneg not complete, just clear what the
generic functions care about.
Fixes: 88d6272acaaa ("net: phy: avoid unneeded MDIO reads in
genphy_read_status")
Signed-off-by: Yonglong Liu
---
change log:
V2: moves the clear operation of lp_advertising from
phy_start_aneg() to genphy_read_lpa()/genphy_c45_read_lpa(), and
if autoneg on and autoneg not complete, just clear what the
generic functions care about. Suggested by Heiner Kallweit.
---
---
drivers/net/phy/phy-c45.c| 2 ++
drivers/net/phy/phy.c| 3 ---
drivers/net/phy/phy_device.c | 12 +++-
3 files changed, 13 insertions(+), 4 deletions(-)
diff --git a/drivers/net/phy/phy-c45.c b/drivers/net/phy/phy-c45.c
index 7935593..a1caeee 100644
--- a/drivers/net/phy/phy-c45.c
+++ b/drivers/net/phy/phy-c45.c
@@ -323,6 +323,8 @@ int genphy_c45_read_pma(struct phy_device *phydev)
{
int val;
+ linkmode_zero(phydev->lp_advertising);
+
val = phy_read_mmd(phydev, MDIO_MMD_PMAPMD, MDIO_CTRL1);
if (val < 0)
return val;
diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index 119e6f4..105d389b 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c
@@ -572,9 +572,6 @@ int phy_start_aneg(struct phy_device *phydev)
if (AUTONEG_DISABLE == phydev->autoneg)
phy_sanitize_settings(phydev);
- /* Invalidate LP advertising flags */
- linkmode_zero(phydev->lp_advertising);
-
err = phy_config_aneg(phydev);
if (err < 0)
goto out_unlock;
diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 9d2bbb1..4b43466 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -1787,7 +1787,14 @@ int genphy_read_lpa(struct phy_device *phydev)
{
int lpa, lpagb;
- if (phydev->autoneg == AUTONEG_ENABLE && phydev->autoneg_complete) {
+ if (phydev->autoneg == AUTONEG_ENABLE) {
+ if (!phydev->autoneg_complete) {
+ mii_stat1000_mod_linkmode_lpa_t(phydev->lp_advertising,
+ 0);
+ mii_lpa_mod_linkmode_lpa_t(phydev->lp_advertising, 0);
+ return 0;
+ }
+
if (phydev->is_gigabit_capable) {
lpagb = phy_read(phydev, MII_STAT1000);
if (lpagb < 0)
@@ -1816,6 +1823,9 @@ int genphy_read_lpa(struct phy_device *phydev)
mii_lpa_mod_linkmode_lpa_t(phydev->lp_advertising, lpa);
}
+ else {
+ linkmode_zero(phydev->lp_advertising);
+ }
return 0;
}
--
2.8.1
Re: [RFC PATCH net] net: phy: Fix "link partner" information disappear issue
On 2019/10/11 3:17, Heiner Kallweit wrote:
> On 10.10.2019 11:30, Yonglong Liu wrote:
>> Some drivers just call phy_ethtool_ksettings_set() to set the
>> links, for those phy drivers that use genphy_read_status(), if
>> autoneg is on, and the link is up, than execute "ethtool -s
>> ethx autoneg on" will cause "link partner" information disappear.
>>
>> The call trace is phy_ethtool_ksettings_set()->phy_start_aneg()
>> ->linkmode_zero(phydev->lp_advertising)->genphy_read_status(),
>> the link didn't change, so genphy_read_status() just return, and
>> phydev->lp_advertising is zero now.
>>
> I think that clearing link partner advertising info in
> phy_start_aneg() is questionable. If advertising doesn't change
> then phy_config_aneg() basically is a no-op. Instead we may have
> to clear the link partner advertising info in genphy_read_lpa()
> if aneg is disabled or aneg isn't completed (basically the same
> as in genphy_c45_read_lpa()). Something like:
>
> if (!phydev->autoneg_complete) { /* also covers case that aneg is disabled */
> linkmode_zero(phydev->lp_advertising);
> } else if (phydev->autoneg == AUTONEG_ENABLE) {
> ...
> }
>
If clear the link partner advertising info in genphy_read_lpa() and
genphy_c45_read_lpa(), for the drivers that use genphy_read_status()
is ok, but for those drivers that use there own read_status() may
have problem, like aqr_read_status(), it will update lp_advertising
first, and than call genphy_c45_read_status(), so will cause
lp_advertising lost.
Another question, please see genphy_c45_read_status(), if clear the
link partner advertising info in genphy_c45_read_lpa(), if autoneg is
off, phydev->lp_advertising will not clear.
>> This patch call genphy_read_lpa() before the link state judgement
>> to fix this problem.
>>
>> Fixes: 88d6272acaaa ("net: phy: avoid unneeded MDIO reads in
>> genphy_read_status")
>> Signed-off-by: Yonglong Liu
>> ---
>> drivers/net/phy/phy_device.c | 8
>> 1 file changed, 4 insertions(+), 4 deletions(-)
>>
>> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
>> index 9d2bbb1..ef3073c 100644
>> --- a/drivers/net/phy/phy_device.c
>> +++ b/drivers/net/phy/phy_device.c
>> @@ -1839,6 +1839,10 @@ int genphy_read_status(struct phy_device *phydev)
>> if (err)
>> return err;
>>
>> +err = genphy_read_lpa(phydev);
>> +if (err < 0)
>> +return err;
>> +
>> /* why bother the PHY if nothing can have changed */
>> if (phydev->autoneg == AUTONEG_ENABLE && old_link && phydev->link)
>> return 0;
>> @@ -1848,10 +1852,6 @@ int genphy_read_status(struct phy_device *phydev)
>> phydev->pause = 0;
>> phydev->asym_pause = 0;
>>
>> -err = genphy_read_lpa(phydev);
>> -if (err < 0)
>> -return err;
>> -
>> if (phydev->autoneg == AUTONEG_ENABLE && phydev->autoneg_complete) {
>> phy_resolve_aneg_linkmode(phydev);
>> } else if (phydev->autoneg == AUTONEG_DISABLE) {
>>
>
>
> .
>
[RFC PATCH net] net: phy: Fix "link partner" information disappear issue
Some drivers just call phy_ethtool_ksettings_set() to set the
links, for those phy drivers that use genphy_read_status(), if
autoneg is on, and the link is up, than execute "ethtool -s
ethx autoneg on" will cause "link partner" information disappear.
The call trace is phy_ethtool_ksettings_set()->phy_start_aneg()
->linkmode_zero(phydev->lp_advertising)->genphy_read_status(),
the link didn't change, so genphy_read_status() just return, and
phydev->lp_advertising is zero now.
This patch call genphy_read_lpa() before the link state judgement
to fix this problem.
Fixes: 88d6272acaaa ("net: phy: avoid unneeded MDIO reads in
genphy_read_status")
Signed-off-by: Yonglong Liu
---
drivers/net/phy/phy_device.c | 8
1 file changed, 4 insertions(+), 4 deletions(-)
diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
index 9d2bbb1..ef3073c 100644
--- a/drivers/net/phy/phy_device.c
+++ b/drivers/net/phy/phy_device.c
@@ -1839,6 +1839,10 @@ int genphy_read_status(struct phy_device *phydev)
if (err)
return err;
+ err = genphy_read_lpa(phydev);
+ if (err < 0)
+ return err;
+
/* why bother the PHY if nothing can have changed */
if (phydev->autoneg == AUTONEG_ENABLE && old_link && phydev->link)
return 0;
@@ -1848,10 +1852,6 @@ int genphy_read_status(struct phy_device *phydev)
phydev->pause = 0;
phydev->asym_pause = 0;
- err = genphy_read_lpa(phydev);
- if (err < 0)
- return err;
-
if (phydev->autoneg == AUTONEG_ENABLE && phydev->autoneg_complete) {
phy_resolve_aneg_linkmode(phydev);
} else if (phydev->autoneg == AUTONEG_DISABLE) {
--
2.8.1
[PATCH net-next] net: hns: add phy_attached_info() to the hns driver
This patch adds the call to phy_attached_info() to the hns driver to identify which exact PHY drivers is in use. Suggested-by: Heiner Kallweit Signed-off-by: Yonglong Liu --- drivers/net/ethernet/hisilicon/hns/hns_enet.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/drivers/net/ethernet/hisilicon/hns/hns_enet.c b/drivers/net/ethernet/hisilicon/hns/hns_enet.c index 1545536..a48396d 100644 --- a/drivers/net/ethernet/hisilicon/hns/hns_enet.c +++ b/drivers/net/ethernet/hisilicon/hns/hns_enet.c @@ -1182,6 +1182,8 @@ int hns_nic_init_phy(struct net_device *ndev, struct hnae_handle *h) if (unlikely(ret)) return -ENODEV; + phy_attached_info(phy_dev); + return 0; } -- 2.8.1
Re: [PATCH net] net: hns: add phy_attached_info() to the hns driver
Please ignore this patch, it is not bugfix, should send to net-next. Sorry for the noise. On 2019/8/17 9:56, Yonglong Liu wrote: > This patch add the call to phy_attached_info() to the hns driver > to identify which exact PHY drivers is in use. > > Signed-off-by: Yonglong Liu > --- > drivers/net/ethernet/hisilicon/hns/hns_enet.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/drivers/net/ethernet/hisilicon/hns/hns_enet.c > b/drivers/net/ethernet/hisilicon/hns/hns_enet.c > index 2235dd5..ab5118d 100644 > --- a/drivers/net/ethernet/hisilicon/hns/hns_enet.c > +++ b/drivers/net/ethernet/hisilicon/hns/hns_enet.c > @@ -1182,6 +1182,8 @@ int hns_nic_init_phy(struct net_device *ndev, struct > hnae_handle *h) > if (unlikely(ret)) > return -ENODEV; > > + phy_attached_info(phy_dev); > + > return 0; > } > >
[PATCH net] net: hns: add phy_attached_info() to the hns driver
This patch add the call to phy_attached_info() to the hns driver to identify which exact PHY drivers is in use. Signed-off-by: Yonglong Liu --- drivers/net/ethernet/hisilicon/hns/hns_enet.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/drivers/net/ethernet/hisilicon/hns/hns_enet.c b/drivers/net/ethernet/hisilicon/hns/hns_enet.c index 2235dd5..ab5118d 100644 --- a/drivers/net/ethernet/hisilicon/hns/hns_enet.c +++ b/drivers/net/ethernet/hisilicon/hns/hns_enet.c @@ -1182,6 +1182,8 @@ int hns_nic_init_phy(struct net_device *ndev, struct hnae_handle *h) if (unlikely(ret)) return -ENODEV; + phy_attached_info(phy_dev); + return 0; } -- 2.8.1
Re: [PATCH net] net: phy: rtl8211f: do a double read to get real time link status
On 2019/8/10 4:05, Heiner Kallweit wrote:
> On 09.08.2019 06:57, Yonglong Liu wrote:
>>
>>
>> On 2019/8/9 4:34, Andrew Lunn wrote:
>>> On Thu, Aug 08, 2019 at 10:01:39PM +0200, Heiner Kallweit wrote:
>>>> On 08.08.2019 21:40, Andrew Lunn wrote:
>>>>>> @@ -568,6 +568,11 @@ int phy_start_aneg(struct phy_device *phydev)
>>>>>> if (err < 0)
>>>>>> goto out_unlock;
>>>>>>
>>>>>> +/* The PHY may not yet have cleared aneg-completed and link-up
>>>>>> bit
>>>>>> + * w/o this delay when the following read is done.
>>>>>> + */
>>>>>> +usleep_range(1000, 2000);
>>>>>> +
>>>>>
>>>>> Hi Heiner
>>>>>
>>>>> Does 802.3 C22 say anything about this?
>>>>>
>>>> C22 says:
>>>> "The Auto-Negotiation process shall be restarted by setting bit 0.9 to a
>>>> logic one. This bit is self-
>>>> clearing, and a PHY shall return a value of one in bit 0.9 until the
>>>> Auto-Negotiation process has been
>>>> initiated."
>>>>
>>>> Maybe we should read bit 0.9 in genphy_update_link() after having read
>>>> BMSR and report
>>>> aneg-complete and link-up as false (no matter of their current value) if
>>>> 0.9 is set.
>>>
>>> Yes. That sounds sensible.
>>>
>>> Andrew
>>>
>>> .
>>>
>>
>> Hi Heiner:
>> I have test more than 50 times, it works. Previously less
>> than 20 times must be recurrence. so I think this patch solved the
>> problem.
>> And I checked about 40 times of the time gap between read
>> and autoneg started, all of them is more than 2ms, as below:
>>
>> kworker/u257:1-670 [015] 27.182632: mdio_access:
>> mii-:bd:00.3 write phy:0x07 reg:0x00 val:0x1240
>> kworker/u257:1-670 [015] 27.184670: mdio_access:
>> mii-:bd:00.3 read phy:0x07 reg:0x01 val:0x7989
>>
>>
>
> Instead of using this fixed delay, the following experimental patch
> considers that fact that between triggering aneg start and actual
> start of aneg (incl. clearing aneg-complete bit) Clause 22 requires
> a PHY to keep bit 0.9 (aneg restart) set.
> Could you please test this instead of the fixed-delay patch?
>
> Thanks, Heiner
>
> diff --git a/drivers/net/phy/phy_device.c b/drivers/net/phy/phy_device.c
> index b039632de..163295dbc 100644
> --- a/drivers/net/phy/phy_device.c
> +++ b/drivers/net/phy/phy_device.c
> @@ -1741,7 +1741,17 @@ EXPORT_SYMBOL(genphy_aneg_done);
> */
> int genphy_update_link(struct phy_device *phydev)
> {
> - int status;
> + int status = 0, bmcr;
> +
> + bmcr = phy_read(phydev, MII_BMCR);
> + if (bmcr < 0)
> + return bmcr;
> +
> + /* Autoneg is being started, therefore disregard BMSR value and
> + * report link as down.
> + */
> + if (bmcr & BMCR_ANRESTART)
> + goto done;
>
> /* The link state is latched low so that momentary link
>* drops can be detected. Do not double-read the status
>
Hi Heiner:
Have test 50+ times, this patch can solved the problem too!
Share the mdio trace after executing ifconfig ethx up:
# tracer: nop
#
# entries-in-buffer/entries-written: 60/60 #P:128
#
# _-=> irqs-off
# / _=> need-resched
#| / _---=> hardirq/softirq
#|| / _--=> preempt-depth
#||| / delay
# TASK-PID CPU# TIMESTAMP FUNCTION
# | | | | |
ifconfig-1174 [005] 27.026691: mdio_access: mii-:bd:00.3
read phy:0x07 reg:0x00 val:0x1040
kworker/u257:1-670 [020] 27.026734: mdio_access: mii-:bd:00.3
read phy:0x07 reg:0x04 val:0x01e1
kworker/u257:1-670 [020] 27.026744: mdio_access: mii-:bd:00.3
write phy:0x07 reg:0x04 val:0x05e1
kworker/u257:1-670 [020] 27.026799: mdio_access: mii-:bd:00.3
read phy:0x07 reg:0x01 val:0x79ad
kworker/u257:1-670 [020] 27.026834: mdio_access: mii-:bd:00.3
read phy:0x07 reg:0x09 val:0x0200
kworker/u257:1-670 [020] 27.026869: mdio_access: mii-:bd:00.3
read phy:0x07 reg:0x00 val:0x1040
kworker/u257:1-670 [020] 27.026879: mdio_access: mii-:bd:00.3
write phy:0x07 reg:0x00 val:0x1240
kw
Re: [PATCH net] net: phy: rtl8211f: do a double read to get real time link status
On 2019/8/9 4:34, Andrew Lunn wrote: > On Thu, Aug 08, 2019 at 10:01:39PM +0200, Heiner Kallweit wrote: >> On 08.08.2019 21:40, Andrew Lunn wrote: @@ -568,6 +568,11 @@ int phy_start_aneg(struct phy_device *phydev) if (err < 0) goto out_unlock; + /* The PHY may not yet have cleared aneg-completed and link-up bit + * w/o this delay when the following read is done. + */ + usleep_range(1000, 2000); + >>> >>> Hi Heiner >>> >>> Does 802.3 C22 say anything about this? >>> >> C22 says: >> "The Auto-Negotiation process shall be restarted by setting bit 0.9 to a >> logic one. This bit is self- >> clearing, and a PHY shall return a value of one in bit 0.9 until the >> Auto-Negotiation process has been >> initiated." >> >> Maybe we should read bit 0.9 in genphy_update_link() after having read BMSR >> and report >> aneg-complete and link-up as false (no matter of their current value) if 0.9 >> is set. > > Yes. That sounds sensible. > > Andrew > > . > Hi Heiner: I have test more than 50 times, it works. Previously less than 20 times must be recurrence. so I think this patch solved the problem. And I checked about 40 times of the time gap between read and autoneg started, all of them is more than 2ms, as below: kworker/u257:1-670 [015] 27.182632: mdio_access: mii-:bd:00.3 write phy:0x07 reg:0x00 val:0x1240 kworker/u257:1-670 [015] 27.184670: mdio_access: mii-:bd:00.3 read phy:0x07 reg:0x01 val:0x7989
Re: [PATCH net] net: phy: rtl8211f: do a double read to get real time link status
On 2019/8/8 14:11, Heiner Kallweit wrote: > On 08.08.2019 03:15, Yonglong Liu wrote: >> >> >> On 2019/8/8 0:47, Heiner Kallweit wrote: >>> On 07.08.2019 15:16, Yonglong Liu wrote: >>>> [ 27.232781] hns3 :bd:00.3 eth7: net open >>>> [ 27.237303] 8021q: adding VLAN 0 to HW filter on device eth7 >>>> [ 27.242972] IPv6: ADDRCONF(NETDEV_CHANGE): eth7: link becomes ready >>>> [ 27.29] hns3 :bd:00.3: invalid speed (-1) >>>> [ 27.253904] hns3 :bd:00.3 eth7: failed to adjust link. >>>> [ 27.259379] RTL8211F Gigabit Ethernet mii-:bd:00.3:07: PHY state >>>> change UP -> RUNNING >>>> [ 27.924903] hns3 :bd:00.3 eth7: link up >>>> [ 28.280479] RTL8211F Gigabit Ethernet mii-:bd:00.3:07: PHY state >>>> change RUNNING -> NOLINK >>>> [ 29.208452] hns3 :bd:00.3 eth7: link down >>>> [ 32.376745] RTL8211F Gigabit Ethernet mii-:bd:00.3:07: PHY state >>>> change NOLINK -> RUNNING >>>> [ 33.208448] hns3 :bd:00.3 eth7: link up >>>> [ 35.253821] hns3 :bd:00.3 eth7: net stop >>>> [ 35.258270] hns3 :bd:00.3 eth7: link down >>>> >>>> When using rtl8211f in polling mode, may get a invalid speed, >>>> because of reading a fake link up and autoneg complete status >>>> immediately after starting autoneg: >>>> >>>> ifconfig-1176 [007] 27.232763: mdio_access: >>>> mii-:bd:00.3 read phy:0x07 reg:0x00 val:0x1040 >>>> kworker/u257:1-670 [015] 27.232805: mdio_access: >>>> mii-:bd:00.3 read phy:0x07 reg:0x04 val:0x01e1 >>>> kworker/u257:1-670 [015] 27.232815: mdio_access: >>>> mii-:bd:00.3 write phy:0x07 reg:0x04 val:0x05e1 >>>> kworker/u257:1-670 [015] 27.232869: mdio_access: >>>> mii-:bd:00.3 read phy:0x07 reg:0x01 val:0x79ad >>>> kworker/u257:1-670 [015] 27.232904: mdio_access: >>>> mii-:bd:00.3 read phy:0x07 reg:0x09 val:0x0200 >>>> kworker/u257:1-670 [015] 27.232940: mdio_access: >>>> mii-:bd:00.3 read phy:0x07 reg:0x00 val:0x1040 >>>> kworker/u257:1-670 [015] 27.232949: mdio_access: >>>> mii-:bd:00.3 write phy:0x07 reg:0x00 val:0x1240 >>>> kworker/u257:1-670 [015] 27.233003: mdio_access: >>>> mii-:bd:00.3 read phy:0x07 reg:0x01 val:0x79ad >>>> kworker/u257:1-670 [015] 27.233039: mdio_access: >>>> mii-:bd:00.3 read phy:0x07 reg:0x0a val:0x3002 >>>> kworker/u257:1-670 [015] 27.233074: mdio_access: >>>> mii-:bd:00.3 read phy:0x07 reg:0x09 val:0x0200 >>>> kworker/u257:1-670 [015] 27.233110: mdio_access: >>>> mii-:bd:00.3 read phy:0x07 reg:0x05 val:0x >>>> kworker/u257:1-670 [000] 28.280475: mdio_access: >>>> mii-:bd:00.3 read phy:0x07 reg:0x01 val:0x7989 >>>> kworker/u257:1-670 [000] 29.304471: mdio_access: >>>> mii-:bd:00.3 read phy:0x07 reg:0x01 val:0x7989 >>>> >>>> According to the datasheet of rtl8211f, to get the real time >>>> link status, need to read MII_BMSR twice. >>>> >>>> This patch add a read_status hook for rtl8211f, and do a fake >>>> phy_read before genphy_read_status(), so that can get real link >>>> status in genphy_read_status(). >>>> >>>> Signed-off-by: Yonglong Liu >>>> --- >>>> drivers/net/phy/realtek.c | 13 + >>>> 1 file changed, 13 insertions(+) >>>> >>> Is this an accidental resubmit? Because we discussed this in >>> https://marc.info/?t=15641350993&r=1&w=2 and a fix has >>> been applied already. >>> >>> Heiner >>> >>> . >>> >> >> In https://marc.info/?t=15641350993&r=1&w=2 , the invalid speed >> recurrence rate is almost 100%, and I had test the solution about >> 5 times and it works. But yesterday it happen again suddenly, and than >> I fount that the recurrence rate reduce to 10%. This time we get 0x79ad >> after autoneg started which is not 0x798d from last discussion. >> >> >> > OK, I'll have a look. > However the approach is wrong. The double read is related to the latching > of link-down events. This is done by all PHY's and not specific to RT8211F. > Also it's not related to the problem. I assume any sufficient delay would > do instead of the read. > > . > So you will send a new patch to fix this problem? I am waiting for it, and can do a full test this time.
Re: [PATCH net] net: phy: rtl8211f: do a double read to get real time link status
On 2019/8/8 0:47, Heiner Kallweit wrote: > On 07.08.2019 15:16, Yonglong Liu wrote: >> [ 27.232781] hns3 :bd:00.3 eth7: net open >> [ 27.237303] 8021q: adding VLAN 0 to HW filter on device eth7 >> [ 27.242972] IPv6: ADDRCONF(NETDEV_CHANGE): eth7: link becomes ready >> [ 27.29] hns3 :bd:00.3: invalid speed (-1) >> [ 27.253904] hns3 :bd:00.3 eth7: failed to adjust link. >> [ 27.259379] RTL8211F Gigabit Ethernet mii-:bd:00.3:07: PHY state >> change UP -> RUNNING >> [ 27.924903] hns3 :bd:00.3 eth7: link up >> [ 28.280479] RTL8211F Gigabit Ethernet mii-:bd:00.3:07: PHY state >> change RUNNING -> NOLINK >> [ 29.208452] hns3 :bd:00.3 eth7: link down >> [ 32.376745] RTL8211F Gigabit Ethernet mii-:bd:00.3:07: PHY state >> change NOLINK -> RUNNING >> [ 33.208448] hns3 :bd:00.3 eth7: link up >> [ 35.253821] hns3 :bd:00.3 eth7: net stop >> [ 35.258270] hns3 :bd:00.3 eth7: link down >> >> When using rtl8211f in polling mode, may get a invalid speed, >> because of reading a fake link up and autoneg complete status >> immediately after starting autoneg: >> >> ifconfig-1176 [007] 27.232763: mdio_access: >> mii-:bd:00.3 read phy:0x07 reg:0x00 val:0x1040 >> kworker/u257:1-670 [015] 27.232805: mdio_access: >> mii-:bd:00.3 read phy:0x07 reg:0x04 val:0x01e1 >> kworker/u257:1-670 [015] 27.232815: mdio_access: >> mii-:bd:00.3 write phy:0x07 reg:0x04 val:0x05e1 >> kworker/u257:1-670 [015] 27.232869: mdio_access: >> mii-:bd:00.3 read phy:0x07 reg:0x01 val:0x79ad >> kworker/u257:1-670 [015] 27.232904: mdio_access: >> mii-:bd:00.3 read phy:0x07 reg:0x09 val:0x0200 >> kworker/u257:1-670 [015] 27.232940: mdio_access: >> mii-:bd:00.3 read phy:0x07 reg:0x00 val:0x1040 >> kworker/u257:1-670 [015] 27.232949: mdio_access: >> mii-:bd:00.3 write phy:0x07 reg:0x00 val:0x1240 >> kworker/u257:1-670 [015] 27.233003: mdio_access: >> mii-:bd:00.3 read phy:0x07 reg:0x01 val:0x79ad >> kworker/u257:1-670 [015] 27.233039: mdio_access: >> mii-:bd:00.3 read phy:0x07 reg:0x0a val:0x3002 >> kworker/u257:1-670 [015] 27.233074: mdio_access: >> mii-:bd:00.3 read phy:0x07 reg:0x09 val:0x0200 >> kworker/u257:1-670 [015] 27.233110: mdio_access: >> mii-:bd:00.3 read phy:0x07 reg:0x05 val:0x >> kworker/u257:1-670 [000] 28.280475: mdio_access: >> mii-:bd:00.3 read phy:0x07 reg:0x01 val:0x7989 >> kworker/u257:1-670 [000] 29.304471: mdio_access: >> mii-:bd:00.3 read phy:0x07 reg:0x01 val:0x7989 >> >> According to the datasheet of rtl8211f, to get the real time >> link status, need to read MII_BMSR twice. >> >> This patch add a read_status hook for rtl8211f, and do a fake >> phy_read before genphy_read_status(), so that can get real link >> status in genphy_read_status(). >> >> Signed-off-by: Yonglong Liu >> --- >> drivers/net/phy/realtek.c | 13 + >> 1 file changed, 13 insertions(+) >> > Is this an accidental resubmit? Because we discussed this in > https://marc.info/?t=15641350993&r=1&w=2 and a fix has > been applied already. > > Heiner > > . > In https://marc.info/?t=15641350993&r=1&w=2 , the invalid speed recurrence rate is almost 100%, and I had test the solution about 5 times and it works. But yesterday it happen again suddenly, and than I fount that the recurrence rate reduce to 10%. This time we get 0x79ad after autoneg started which is not 0x798d from last discussion.
[PATCH net] net: phy: rtl8211f: do a double read to get real time link status
[ 27.232781] hns3 :bd:00.3 eth7: net open
[ 27.237303] 8021q: adding VLAN 0 to HW filter on device eth7
[ 27.242972] IPv6: ADDRCONF(NETDEV_CHANGE): eth7: link becomes ready
[ 27.29] hns3 :bd:00.3: invalid speed (-1)
[ 27.253904] hns3 :bd:00.3 eth7: failed to adjust link.
[ 27.259379] RTL8211F Gigabit Ethernet mii-:bd:00.3:07: PHY state change
UP -> RUNNING
[ 27.924903] hns3 :bd:00.3 eth7: link up
[ 28.280479] RTL8211F Gigabit Ethernet mii-:bd:00.3:07: PHY state change
RUNNING -> NOLINK
[ 29.208452] hns3 :bd:00.3 eth7: link down
[ 32.376745] RTL8211F Gigabit Ethernet mii-:bd:00.3:07: PHY state change
NOLINK -> RUNNING
[ 33.208448] hns3 :bd:00.3 eth7: link up
[ 35.253821] hns3 :bd:00.3 eth7: net stop
[ 35.258270] hns3 :bd:00.3 eth7: link down
When using rtl8211f in polling mode, may get a invalid speed,
because of reading a fake link up and autoneg complete status
immediately after starting autoneg:
ifconfig-1176 [007] 27.232763: mdio_access: mii-:bd:00.3
read phy:0x07 reg:0x00 val:0x1040
kworker/u257:1-670 [015] 27.232805: mdio_access: mii-:bd:00.3
read phy:0x07 reg:0x04 val:0x01e1
kworker/u257:1-670 [015] 27.232815: mdio_access: mii-:bd:00.3
write phy:0x07 reg:0x04 val:0x05e1
kworker/u257:1-670 [015] 27.232869: mdio_access: mii-:bd:00.3
read phy:0x07 reg:0x01 val:0x79ad
kworker/u257:1-670 [015] 27.232904: mdio_access: mii-:bd:00.3
read phy:0x07 reg:0x09 val:0x0200
kworker/u257:1-670 [015] 27.232940: mdio_access: mii-:bd:00.3
read phy:0x07 reg:0x00 val:0x1040
kworker/u257:1-670 [015] 27.232949: mdio_access: mii-:bd:00.3
write phy:0x07 reg:0x00 val:0x1240
kworker/u257:1-670 [015] 27.233003: mdio_access: mii-:bd:00.3
read phy:0x07 reg:0x01 val:0x79ad
kworker/u257:1-670 [015] 27.233039: mdio_access: mii-:bd:00.3
read phy:0x07 reg:0x0a val:0x3002
kworker/u257:1-670 [015] 27.233074: mdio_access: mii-:bd:00.3
read phy:0x07 reg:0x09 val:0x0200
kworker/u257:1-670 [015] 27.233110: mdio_access: mii-:bd:00.3
read phy:0x07 reg:0x05 val:0x
kworker/u257:1-670 [000] 28.280475: mdio_access: mii-:bd:00.3
read phy:0x07 reg:0x01 val:0x7989
kworker/u257:1-670 [000] 29.304471: mdio_access: mii-:bd:00.3
read phy:0x07 reg:0x01 val:0x7989
According to the datasheet of rtl8211f, to get the real time
link status, need to read MII_BMSR twice.
This patch add a read_status hook for rtl8211f, and do a fake
phy_read before genphy_read_status(), so that can get real link
status in genphy_read_status().
Signed-off-by: Yonglong Liu
---
drivers/net/phy/realtek.c | 13 +
1 file changed, 13 insertions(+)
diff --git a/drivers/net/phy/realtek.c b/drivers/net/phy/realtek.c
index a669945..92e27d5 100644
--- a/drivers/net/phy/realtek.c
+++ b/drivers/net/phy/realtek.c
@@ -256,6 +256,18 @@ static int rtl8366rb_config_init(struct phy_device *phydev)
return ret;
}
+static int rtl8211f_read_status(struct phy_device *phydev)
+{
+ int status;
+
+ /* do a fake read */
+ status = phy_read(phydev, MII_BMSR);
+ if (status < 0)
+ return status;
+
+ return genphy_read_status(phydev);
+}
+
static struct phy_driver realtek_drvs[] = {
{
PHY_ID_MATCH_EXACT(0x8201),
@@ -325,6 +337,7 @@ static struct phy_driver realtek_drvs[] = {
.resume = genphy_resume,
.read_page = rtl821x_read_page,
.write_page = rtl821x_write_page,
+ .read_status= rtl8211f_read_status,
}, {
PHY_ID_MATCH_EXACT(0x001cc800),
.name = "Generic Realtek PHY",
--
2.8.1
[RFC] net: phy: read link status twice when phy_check_link_status()
According to the datasheet of Marvell phy and Realtek phy, the copper link status should read twice, or it may get a fake link up status, and cause up->down->up at the first time when link up. This happens more oftem at Realtek phy. I add a fake status read, and can solve this problem. I also see that in genphy_update_link(), had delete the fake read in polling mode, so I don't know whether my solution is correct. Or provide a phydev->drv->read_status functions for the phy I used is more acceptable? Signed-off-by: Yonglong Liu --- drivers/net/phy/phy.c | 8 1 file changed, 8 insertions(+) diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c index ef7aa73..0c03edc 100644 --- a/drivers/net/phy/phy.c +++ b/drivers/net/phy/phy.c @@ -1,4 +1,7 @@ // SPDX-License-Identifier: GPL-2.0+ + err = phy_read_status(phydev); + if (err) + return err; /* Framework for configuring and reading PHY devices * Based on code in sungem_phy.c and gianfar_phy.c * @@ -525,6 +528,11 @@ static int phy_check_link_status(struct phy_device *phydev) WARN_ON(!mutex_is_locked(&phydev->lock)); + /* Do a fake read */ + err = phy_read(phydev, MII_BMSR); + if (err < 0) + return err; + err = phy_read_status(phydev); if (err) return err; -- 2.8.1
[PATCH net] net: hns: fix LED configuration for marvell phy
Since commit(net: phy: marvell: change default m88e1510 LED configuration),
the active LED of Hip07 devices is always off, because Hip07 just
use 2 LEDs.
This patch adds a phy_register_fixup_for_uid() for m88e1510 to
correct the LED configuration.
Fixes: 02468ec1 ("net: phy: marvell: change default m88e1510 LED
configuration")
Signed-off-by: Yonglong Liu
Reviewed-by: linyunsheng
---
drivers/net/ethernet/hisilicon/hns/hns_enet.c | 23 ++-
1 file changed, 22 insertions(+), 1 deletion(-)
diff --git a/drivers/net/ethernet/hisilicon/hns/hns_enet.c
b/drivers/net/ethernet/hisilicon/hns/hns_enet.c
index 2235dd5..5b213eb 100644
--- a/drivers/net/ethernet/hisilicon/hns/hns_enet.c
+++ b/drivers/net/ethernet/hisilicon/hns/hns_enet.c
@@ -11,6 +11,7 @@
#include
#include
#include
+#include
#include
#include
#include
@@ -1149,6 +1150,13 @@ static void hns_nic_adjust_link(struct net_device *ndev)
}
}
+static int hns_phy_marvell_fixup(struct phy_device *phydev)
+{
+ phydev->dev_flags |= MARVELL_PHY_LED0_LINK_LED1_ACTIVE;
+
+ return 0;
+}
+
/**
*hns_nic_init_phy - init phy
*@ndev: net device
@@ -1174,6 +1182,16 @@ int hns_nic_init_phy(struct net_device *ndev, struct
hnae_handle *h)
if (h->phy_if != PHY_INTERFACE_MODE_XGMII) {
phy_dev->dev_flags = 0;
+ /* register the PHY fixup (for Marvell 88E1510) */
+ ret = phy_register_fixup_for_uid(MARVELL_PHY_ID_88E1510,
+MARVELL_PHY_ID_MASK,
+hns_phy_marvell_fixup);
+ /* we can live without it, so just issue a warning */
+ if (ret)
+ netdev_warn(ndev,
+ "Cannot register PHY fixup, ret=%d\n",
+ ret);
+
ret = phy_connect_direct(ndev, phy_dev, hns_nic_adjust_link,
h->phy_if);
} else {
@@ -2430,8 +2448,11 @@ static int hns_nic_dev_remove(struct platform_device
*pdev)
hns_nic_uninit_ring_data(priv);
priv->ring_data = NULL;
- if (ndev->phydev)
+ if (ndev->phydev) {
+ phy_unregister_fixup_for_uid(MARVELL_PHY_ID_88E1510,
+MARVELL_PHY_ID_MASK);
phy_disconnect(ndev->phydev);
+ }
if (!IS_ERR_OR_NULL(priv->ae_handle))
hnae_put_handle(priv->ae_handle);
--
2.8.1
[PATCH net] net: hns: add support for vlan TSO
The hip07 chip support vlan TSO, this patch adds NETIF_F_TSO and NETIF_F_TSO6 flags to vlan_features to improve the performance after adding vlan to the net ports. Signed-off-by: Yonglong Liu --- drivers/net/ethernet/hisilicon/hns/hns_enet.c | 1 + 1 file changed, 1 insertion(+) diff --git a/drivers/net/ethernet/hisilicon/hns/hns_enet.c b/drivers/net/ethernet/hisilicon/hns/hns_enet.c index fe879c0..2235dd5 100644 --- a/drivers/net/ethernet/hisilicon/hns/hns_enet.c +++ b/drivers/net/ethernet/hisilicon/hns/hns_enet.c @@ -2370,6 +2370,7 @@ static int hns_nic_dev_probe(struct platform_device *pdev) ndev->hw_features |= NETIF_F_IP_CSUM | NETIF_F_IPV6_CSUM | NETIF_F_RXCSUM | NETIF_F_SG | NETIF_F_GSO | NETIF_F_GRO | NETIF_F_TSO | NETIF_F_TSO6; + ndev->vlan_features |= NETIF_F_TSO | NETIF_F_TSO6; ndev->max_mtu = MAC_MAX_MTU_V2 - (ETH_HLEN + ETH_FCS_LEN + VLAN_HLEN); break; -- 2.8.1
[PATCH net] net: hns: Fix loopback test failed at copper ports
When doing a loopback test at copper ports, the serdes loopback
and the phy loopback will fail, because of the adjust link had
not finished, and phy not ready.
Adds sleep between adjust link and test process to fix it.
Signed-off-by: Yonglong Liu
---
drivers/net/ethernet/hisilicon/hns/hns_ethtool.c | 4
1 file changed, 4 insertions(+)
diff --git a/drivers/net/ethernet/hisilicon/hns/hns_ethtool.c
b/drivers/net/ethernet/hisilicon/hns/hns_ethtool.c
index ce15d23..188c3f6 100644
--- a/drivers/net/ethernet/hisilicon/hns/hns_ethtool.c
+++ b/drivers/net/ethernet/hisilicon/hns/hns_ethtool.c
@@ -339,6 +339,7 @@ static int __lb_setup(struct net_device *ndev,
static int __lb_up(struct net_device *ndev,
enum hnae_loop loop_mode)
{
+#define NIC_LB_TEST_WAIT_PHY_LINK_TIME 300
struct hns_nic_priv *priv = netdev_priv(ndev);
struct hnae_handle *h = priv->ae_handle;
int speed, duplex;
@@ -365,6 +366,9 @@ static int __lb_up(struct net_device *ndev,
h->dev->ops->adjust_link(h, speed, duplex);
+ /* wait adjust link done and phy ready */
+ msleep(NIC_LB_TEST_WAIT_PHY_LINK_TIME);
+
return 0;
}
--
2.8.1
[PATCH net 3/6] net: hns: Fix probabilistic memory overwrite when HNS driver initialized
When reboot the system again and again, may cause a memory overwrite. [ 15.638922] systemd[1]: Reached target Swap. [ 15.667561] tun: Universal TUN/TAP device driver, 1.6 [ 15.676756] Bridge firewalling registered [ 17.344135] Unable to handle kernel paging request at virtual address 00020040 [ 17.352179] Mem abort info: [ 17.355007] ESR = 0x9604 [ 17.358105] Exception class = DABT (current EL), IL = 32 bits [ 17.364112] SET = 0, FnV = 0 [ 17.367209] EA = 0, S1PTW = 0 [ 17.370393] Data abort info: [ 17.373315] ISV = 0, ISS = 0x0004 [ 17.377206] CM = 0, WnR = 0 [ 17.380214] user pgtable: 4k pages, 48-bit VAs, pgdp = (ptrval) [ 17.386926] [00020040] pgd= [ 17.391878] Internal error: Oops: 9604 [#1] SMP [ 17.396824] CPU: 23 PID: 95 Comm: kworker/u130:0 Tainted: GE 4.19.25-1.2.78.aarch64 #1 [ 17.414175] Hardware name: Huawei TaiShan 2280 /BC11SPCD, BIOS 1.54 08/16/2018 [ 17.425615] Workqueue: events_unbound async_run_entry_fn [ 17.435151] pstate: 0005 (nzcv daif -PAN -UAO) [ 17.444139] pc : __mutex_lock.isra.1+0x74/0x540 [ 17.453002] lr : __mutex_lock.isra.1+0x3c/0x540 [ 17.461701] sp : 000100d9bb60 [ 17.469146] x29: 000100d9bb60 x28: [ 17.478547] x27: x26: 802fb8945000 [ 17.488063] x25: x24: 802fa32081a8 [ 17.497381] x23: 0002 x22: 801fa2b15220 [ 17.506701] x21: 09809000 x20: 802fa23a0888 [ 17.515980] x19: 801fa2b15220 x18: [ 17.525272] x17: 0002 x16: 0002 [ 17.534511] x15: x14: [ 17.543652] x13: 08d95db8 x12: 000d [ 17.552780] x11: 08d95d90 x10: 0b00 [ 17.561819] x9 : 000100d9bb90 x8 : 802fb89d6560 [ 17.570829] x7 : 0004 x6 : 0004a1801d05 [ 17.579839] x5 : x4 : [ 17.588852] x3 : 802fb89d5a00 x2 : [ 17.597734] x1 : 0002 x0 : 0002 [ 17.606631] Process kworker/u130:0 (pid: 95, stack limit = 0x(ptrval)) [ 17.617438] Call trace: [ 17.623349] __mutex_lock.isra.1+0x74/0x540 [ 17.630927] __mutex_lock_slowpath+0x24/0x30 [ 17.638602] mutex_lock+0x50/0x60 [ 17.645295] drain_workqueue+0x34/0x198 [ 17.652623] __sas_drain_work+0x7c/0x168 [ 17.659903] sas_drain_work+0x60/0x68 [ 17.666947] hisi_sas_scan_finished+0x30/0x40 [hisi_sas_main] [ 17.676129] do_scsi_scan_host+0x70/0xb0 [ 17.683534] do_scan_async+0x20/0x228 [ 17.690586] async_run_entry_fn+0x4c/0x1d0 [ 17.697997] process_one_work+0x1b4/0x3f8 [ 17.705296] worker_thread+0x54/0x470 Every time the call trace is not the same, but the overwrite address is always the same: Unable to handle kernel paging request at virtual address 00020040 The root cause is, when write the reg XGMAC_MAC_TX_LF_RF_CONTROL_REG, didn't use the io_base offset. Signed-off-by: Yonglong Liu --- drivers/net/ethernet/hisilicon/hns/hns_dsaf_xgmac.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/drivers/net/ethernet/hisilicon/hns/hns_dsaf_xgmac.c b/drivers/net/ethernet/hisilicon/hns/hns_dsaf_xgmac.c index ba43169..a60f207 100644 --- a/drivers/net/ethernet/hisilicon/hns/hns_dsaf_xgmac.c +++ b/drivers/net/ethernet/hisilicon/hns/hns_dsaf_xgmac.c @@ -129,7 +129,7 @@ static void hns_xgmac_lf_rf_control_init(struct mac_driver *mac_drv) dsaf_set_bit(val, XGMAC_UNIDIR_EN_B, 0); dsaf_set_bit(val, XGMAC_RF_TX_EN_B, 1); dsaf_set_field(val, XGMAC_LF_RF_INSERT_M, XGMAC_LF_RF_INSERT_S, 0); - dsaf_write_reg(mac_drv, XGMAC_MAC_TX_LF_RF_CONTROL_REG, val); + dsaf_write_dev(mac_drv, XGMAC_MAC_TX_LF_RF_CONTROL_REG, val); } /** -- 2.8.1
