[PATCH net] net: phy: rtl8211f: do a double read to get real time link status

[PATCH net] net: phy: rtl8211f: do a double read to get real time link status

[Yonglong Liu]

[   27.232781] hns3 0000: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.244449] hns3 0000:bd:00.3: invalid speed (-1)
[   27.253904] hns3 0000:bd:00.3 eth7: failed to adjust link.
[   27.259379] RTL8211F Gigabit Ethernet mii-0000:bd:00.3:07: PHY state change UP -> RUNNING
[   27.924903] hns3 0000:bd:00.3 eth7: link up
[   28.280479] RTL8211F Gigabit Ethernet mii-0000:bd:00.3:07: PHY state change RUNNING -> NOLINK
[   29.208452] hns3 0000:bd:00.3 eth7: link down
[   32.376745] RTL8211F Gigabit Ethernet mii-0000:bd:00.3:07: PHY state change NOLINK -> RUNNING
[   33.208448] hns3 0000:bd:00.3 eth7: link up
[   35.253821] hns3 0000:bd:00.3 eth7: net stop
[   35.258270] hns3 0000: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-0000:bd:00.3 read  phy:0x07 reg:0x00 val:0x1040
  kworker/u257:1-670   [015] ....    27.232805: mdio_access: mii-0000:bd:00.3 read  phy:0x07 reg:0x04 val:0x01e1
  kworker/u257:1-670   [015] ....    27.232815: mdio_access: mii-0000:bd:00.3 write phy:0x07 reg:0x04 val:0x05e1
  kworker/u257:1-670   [015] ....    27.232869: mdio_access: mii-0000:bd:00.3 read  phy:0x07 reg:0x01 val:0x79ad
  kworker/u257:1-670   [015] ....    27.232904: mdio_access: mii-0000:bd:00.3 read  phy:0x07 reg:0x09 val:0x0200
  kworker/u257:1-670   [015] ....    27.232940: mdio_access: mii-0000:bd:00.3 read  phy:0x07 reg:0x00 val:0x1040
  kworker/u257:1-670   [015] ....    27.232949: mdio_access: mii-0000:bd:00.3 write phy:0x07 reg:0x00 val:0x1240
  kworker/u257:1-670   [015] ....    27.233003: mdio_access: mii-0000:bd:00.3 read  phy:0x07 reg:0x01 val:0x79ad
  kworker/u257:1-670   [015] ....    27.233039: mdio_access: mii-0000:bd:00.3 read  phy:0x07 reg:0x0a val:0x3002
  kworker/u257:1-670   [015] ....    27.233074: mdio_access: mii-0000:bd:00.3 read  phy:0x07 reg:0x09 val:0x0200
  kworker/u257:1-670   [015] ....    27.233110: mdio_access: mii-0000:bd:00.3 read  phy:0x07 reg:0x05 val:0x0000
  kworker/u257:1-670   [000] ....    28.280475: mdio_access: mii-0000:bd:00.3 read  phy:0x07 reg:0x01 val:0x7989
  kworker/u257:1-670   [000] ....    29.304471: mdio_access: mii-0000: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 <liuyonglong@huawei.com>
---
 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(0x00008201),
@@ -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

Re: [PATCH net] net: phy: rtl8211f: do a double read to get real time link status

[Heiner Kallweit]

On 07.08.2019 15:16, Yonglong Liu wrote:
> [   27.232781] hns3 0000: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.244449] hns3 0000:bd:00.3: invalid speed (-1)
> [   27.253904] hns3 0000:bd:00.3 eth7: failed to adjust link.
> [   27.259379] RTL8211F Gigabit Ethernet mii-0000:bd:00.3:07: PHY state change UP -> RUNNING
> [   27.924903] hns3 0000:bd:00.3 eth7: link up
> [   28.280479] RTL8211F Gigabit Ethernet mii-0000:bd:00.3:07: PHY state change RUNNING -> NOLINK
> [   29.208452] hns3 0000:bd:00.3 eth7: link down
> [   32.376745] RTL8211F Gigabit Ethernet mii-0000:bd:00.3:07: PHY state change NOLINK -> RUNNING
> [   33.208448] hns3 0000:bd:00.3 eth7: link up
> [   35.253821] hns3 0000:bd:00.3 eth7: net stop
> [   35.258270] hns3 0000: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-0000:bd:00.3 read  phy:0x07 reg:0x00 val:0x1040
>   kworker/u257:1-670   [015] ....    27.232805: mdio_access: mii-0000:bd:00.3 read  phy:0x07 reg:0x04 val:0x01e1
>   kworker/u257:1-670   [015] ....    27.232815: mdio_access: mii-0000:bd:00.3 write phy:0x07 reg:0x04 val:0x05e1
>   kworker/u257:1-670   [015] ....    27.232869: mdio_access: mii-0000:bd:00.3 read  phy:0x07 reg:0x01 val:0x79ad
>   kworker/u257:1-670   [015] ....    27.232904: mdio_access: mii-0000:bd:00.3 read  phy:0x07 reg:0x09 val:0x0200
>   kworker/u257:1-670   [015] ....    27.232940: mdio_access: mii-0000:bd:00.3 read  phy:0x07 reg:0x00 val:0x1040
>   kworker/u257:1-670   [015] ....    27.232949: mdio_access: mii-0000:bd:00.3 write phy:0x07 reg:0x00 val:0x1240
>   kworker/u257:1-670   [015] ....    27.233003: mdio_access: mii-0000:bd:00.3 read  phy:0x07 reg:0x01 val:0x79ad
>   kworker/u257:1-670   [015] ....    27.233039: mdio_access: mii-0000:bd:00.3 read  phy:0x07 reg:0x0a val:0x3002
>   kworker/u257:1-670   [015] ....    27.233074: mdio_access: mii-0000:bd:00.3 read  phy:0x07 reg:0x09 val:0x0200
>   kworker/u257:1-670   [015] ....    27.233110: mdio_access: mii-0000:bd:00.3 read  phy:0x07 reg:0x05 val:0x0000
>   kworker/u257:1-670   [000] ....    28.280475: mdio_access: mii-0000:bd:00.3 read  phy:0x07 reg:0x01 val:0x7989
>   kworker/u257:1-670   [000] ....    29.304471: mdio_access: mii-0000: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 <liuyonglong@huawei.com>
> ---
>  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=156413509900003&r=1&w=2 and a fix has
been applied already.

Heiner

Re: [PATCH net] net: phy: rtl8211f: do a double read to get real time link status

[Yonglong Liu]

On 2019/8/8 0:47, Heiner Kallweit wrote:
> On 07.08.2019 15:16, Yonglong Liu wrote:
>> [   27.232781] hns3 0000: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.244449] hns3 0000:bd:00.3: invalid speed (-1)
>> [   27.253904] hns3 0000:bd:00.3 eth7: failed to adjust link.
>> [   27.259379] RTL8211F Gigabit Ethernet mii-0000:bd:00.3:07: PHY state change UP -> RUNNING
>> [   27.924903] hns3 0000:bd:00.3 eth7: link up
>> [   28.280479] RTL8211F Gigabit Ethernet mii-0000:bd:00.3:07: PHY state change RUNNING -> NOLINK
>> [   29.208452] hns3 0000:bd:00.3 eth7: link down
>> [   32.376745] RTL8211F Gigabit Ethernet mii-0000:bd:00.3:07: PHY state change NOLINK -> RUNNING
>> [   33.208448] hns3 0000:bd:00.3 eth7: link up
>> [   35.253821] hns3 0000:bd:00.3 eth7: net stop
>> [   35.258270] hns3 0000: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-0000:bd:00.3 read  phy:0x07 reg:0x00 val:0x1040
>>   kworker/u257:1-670   [015] ....    27.232805: mdio_access: mii-0000:bd:00.3 read  phy:0x07 reg:0x04 val:0x01e1
>>   kworker/u257:1-670   [015] ....    27.232815: mdio_access: mii-0000:bd:00.3 write phy:0x07 reg:0x04 val:0x05e1
>>   kworker/u257:1-670   [015] ....    27.232869: mdio_access: mii-0000:bd:00.3 read  phy:0x07 reg:0x01 val:0x79ad
>>   kworker/u257:1-670   [015] ....    27.232904: mdio_access: mii-0000:bd:00.3 read  phy:0x07 reg:0x09 val:0x0200
>>   kworker/u257:1-670   [015] ....    27.232940: mdio_access: mii-0000:bd:00.3 read  phy:0x07 reg:0x00 val:0x1040
>>   kworker/u257:1-670   [015] ....    27.232949: mdio_access: mii-0000:bd:00.3 write phy:0x07 reg:0x00 val:0x1240
>>   kworker/u257:1-670   [015] ....    27.233003: mdio_access: mii-0000:bd:00.3 read  phy:0x07 reg:0x01 val:0x79ad
>>   kworker/u257:1-670   [015] ....    27.233039: mdio_access: mii-0000:bd:00.3 read  phy:0x07 reg:0x0a val:0x3002
>>   kworker/u257:1-670   [015] ....    27.233074: mdio_access: mii-0000:bd:00.3 read  phy:0x07 reg:0x09 val:0x0200
>>   kworker/u257:1-670   [015] ....    27.233110: mdio_access: mii-0000:bd:00.3 read  phy:0x07 reg:0x05 val:0x0000
>>   kworker/u257:1-670   [000] ....    28.280475: mdio_access: mii-0000:bd:00.3 read  phy:0x07 reg:0x01 val:0x7989
>>   kworker/u257:1-670   [000] ....    29.304471: mdio_access: mii-0000: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 <liuyonglong@huawei.com>
>> ---
>>  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=156413509900003&r=1&w=2 and a fix has
> been applied already.
> 
> Heiner
> 
> .
> 

In https://marc.info/?t=156413509900003&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.

Re: [PATCH net] net: phy: rtl8211f: do a double read to get real time link status

[Heiner Kallweit]

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 0000: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.244449] hns3 0000:bd:00.3: invalid speed (-1)
>>> [   27.253904] hns3 0000:bd:00.3 eth7: failed to adjust link.
>>> [   27.259379] RTL8211F Gigabit Ethernet mii-0000:bd:00.3:07: PHY state change UP -> RUNNING
>>> [   27.924903] hns3 0000:bd:00.3 eth7: link up
>>> [   28.280479] RTL8211F Gigabit Ethernet mii-0000:bd:00.3:07: PHY state change RUNNING -> NOLINK
>>> [   29.208452] hns3 0000:bd:00.3 eth7: link down
>>> [   32.376745] RTL8211F Gigabit Ethernet mii-0000:bd:00.3:07: PHY state change NOLINK -> RUNNING
>>> [   33.208448] hns3 0000:bd:00.3 eth7: link up
>>> [   35.253821] hns3 0000:bd:00.3 eth7: net stop
>>> [   35.258270] hns3 0000: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-0000:bd:00.3 read  phy:0x07 reg:0x00 val:0x1040
>>>   kworker/u257:1-670   [015] ....    27.232805: mdio_access: mii-0000:bd:00.3 read  phy:0x07 reg:0x04 val:0x01e1
>>>   kworker/u257:1-670   [015] ....    27.232815: mdio_access: mii-0000:bd:00.3 write phy:0x07 reg:0x04 val:0x05e1
>>>   kworker/u257:1-670   [015] ....    27.232869: mdio_access: mii-0000:bd:00.3 read  phy:0x07 reg:0x01 val:0x79ad
>>>   kworker/u257:1-670   [015] ....    27.232904: mdio_access: mii-0000:bd:00.3 read  phy:0x07 reg:0x09 val:0x0200
>>>   kworker/u257:1-670   [015] ....    27.232940: mdio_access: mii-0000:bd:00.3 read  phy:0x07 reg:0x00 val:0x1040
>>>   kworker/u257:1-670   [015] ....    27.232949: mdio_access: mii-0000:bd:00.3 write phy:0x07 reg:0x00 val:0x1240
>>>   kworker/u257:1-670   [015] ....    27.233003: mdio_access: mii-0000:bd:00.3 read  phy:0x07 reg:0x01 val:0x79ad
>>>   kworker/u257:1-670   [015] ....    27.233039: mdio_access: mii-0000:bd:00.3 read  phy:0x07 reg:0x0a val:0x3002
>>>   kworker/u257:1-670   [015] ....    27.233074: mdio_access: mii-0000:bd:00.3 read  phy:0x07 reg:0x09 val:0x0200
>>>   kworker/u257:1-670   [015] ....    27.233110: mdio_access: mii-0000:bd:00.3 read  phy:0x07 reg:0x05 val:0x0000
>>>   kworker/u257:1-670   [000] ....    28.280475: mdio_access: mii-0000:bd:00.3 read  phy:0x07 reg:0x01 val:0x7989
>>>   kworker/u257:1-670   [000] ....    29.304471: mdio_access: mii-0000: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 <liuyonglong@huawei.com>
>>> ---
>>>  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=156413509900003&r=1&w=2 and a fix has
>> been applied already.
>>
>> Heiner
>>
>> .
>>
> 
> In https://marc.info/?t=156413509900003&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.

Re: [PATCH net] net: phy: rtl8211f: do a double read to get real time link status

[Yonglong Liu]

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 0000: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.244449] hns3 0000:bd:00.3: invalid speed (-1)
>>>> [   27.253904] hns3 0000:bd:00.3 eth7: failed to adjust link.
>>>> [   27.259379] RTL8211F Gigabit Ethernet mii-0000:bd:00.3:07: PHY state change UP -> RUNNING
>>>> [   27.924903] hns3 0000:bd:00.3 eth7: link up
>>>> [   28.280479] RTL8211F Gigabit Ethernet mii-0000:bd:00.3:07: PHY state change RUNNING -> NOLINK
>>>> [   29.208452] hns3 0000:bd:00.3 eth7: link down
>>>> [   32.376745] RTL8211F Gigabit Ethernet mii-0000:bd:00.3:07: PHY state change NOLINK -> RUNNING
>>>> [   33.208448] hns3 0000:bd:00.3 eth7: link up
>>>> [   35.253821] hns3 0000:bd:00.3 eth7: net stop
>>>> [   35.258270] hns3 0000: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-0000:bd:00.3 read  phy:0x07 reg:0x00 val:0x1040
>>>>   kworker/u257:1-670   [015] ....    27.232805: mdio_access: mii-0000:bd:00.3 read  phy:0x07 reg:0x04 val:0x01e1
>>>>   kworker/u257:1-670   [015] ....    27.232815: mdio_access: mii-0000:bd:00.3 write phy:0x07 reg:0x04 val:0x05e1
>>>>   kworker/u257:1-670   [015] ....    27.232869: mdio_access: mii-0000:bd:00.3 read  phy:0x07 reg:0x01 val:0x79ad
>>>>   kworker/u257:1-670   [015] ....    27.232904: mdio_access: mii-0000:bd:00.3 read  phy:0x07 reg:0x09 val:0x0200
>>>>   kworker/u257:1-670   [015] ....    27.232940: mdio_access: mii-0000:bd:00.3 read  phy:0x07 reg:0x00 val:0x1040
>>>>   kworker/u257:1-670   [015] ....    27.232949: mdio_access: mii-0000:bd:00.3 write phy:0x07 reg:0x00 val:0x1240
>>>>   kworker/u257:1-670   [015] ....    27.233003: mdio_access: mii-0000:bd:00.3 read  phy:0x07 reg:0x01 val:0x79ad
>>>>   kworker/u257:1-670   [015] ....    27.233039: mdio_access: mii-0000:bd:00.3 read  phy:0x07 reg:0x0a val:0x3002
>>>>   kworker/u257:1-670   [015] ....    27.233074: mdio_access: mii-0000:bd:00.3 read  phy:0x07 reg:0x09 val:0x0200
>>>>   kworker/u257:1-670   [015] ....    27.233110: mdio_access: mii-0000:bd:00.3 read  phy:0x07 reg:0x05 val:0x0000
>>>>   kworker/u257:1-670   [000] ....    28.280475: mdio_access: mii-0000:bd:00.3 read  phy:0x07 reg:0x01 val:0x7989
>>>>   kworker/u257:1-670   [000] ....    29.304471: mdio_access: mii-0000: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 <liuyonglong@huawei.com>
>>>> ---
>>>>  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=156413509900003&r=1&w=2 and a fix has
>>> been applied already.
>>>
>>> Heiner
>>>
>>> .
>>>
>>
>> In https://marc.info/?t=156413509900003&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

[Heiner Kallweit]

On 08.08.2019 08:21, Yonglong Liu wrote:
> 
> 
> 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 0000: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.244449] hns3 0000:bd:00.3: invalid speed (-1)
>>>>> [   27.253904] hns3 0000:bd:00.3 eth7: failed to adjust link.
>>>>> [   27.259379] RTL8211F Gigabit Ethernet mii-0000:bd:00.3:07: PHY state change UP -> RUNNING
>>>>> [   27.924903] hns3 0000:bd:00.3 eth7: link up
>>>>> [   28.280479] RTL8211F Gigabit Ethernet mii-0000:bd:00.3:07: PHY state change RUNNING -> NOLINK
>>>>> [   29.208452] hns3 0000:bd:00.3 eth7: link down
>>>>> [   32.376745] RTL8211F Gigabit Ethernet mii-0000:bd:00.3:07: PHY state change NOLINK -> RUNNING
>>>>> [   33.208448] hns3 0000:bd:00.3 eth7: link up
>>>>> [   35.253821] hns3 0000:bd:00.3 eth7: net stop
>>>>> [   35.258270] hns3 0000: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-0000:bd:00.3 read  phy:0x07 reg:0x00 val:0x1040
>>>>>   kworker/u257:1-670   [015] ....    27.232805: mdio_access: mii-0000:bd:00.3 read  phy:0x07 reg:0x04 val:0x01e1
>>>>>   kworker/u257:1-670   [015] ....    27.232815: mdio_access: mii-0000:bd:00.3 write phy:0x07 reg:0x04 val:0x05e1
>>>>>   kworker/u257:1-670   [015] ....    27.232869: mdio_access: mii-0000:bd:00.3 read  phy:0x07 reg:0x01 val:0x79ad
>>>>>   kworker/u257:1-670   [015] ....    27.232904: mdio_access: mii-0000:bd:00.3 read  phy:0x07 reg:0x09 val:0x0200
>>>>>   kworker/u257:1-670   [015] ....    27.232940: mdio_access: mii-0000:bd:00.3 read  phy:0x07 reg:0x00 val:0x1040
>>>>>   kworker/u257:1-670   [015] ....    27.232949: mdio_access: mii-0000:bd:00.3 write phy:0x07 reg:0x00 val:0x1240
>>>>>   kworker/u257:1-670   [015] ....    27.233003: mdio_access: mii-0000:bd:00.3 read  phy:0x07 reg:0x01 val:0x79ad
>>>>>   kworker/u257:1-670   [015] ....    27.233039: mdio_access: mii-0000:bd:00.3 read  phy:0x07 reg:0x0a val:0x3002
>>>>>   kworker/u257:1-670   [015] ....    27.233074: mdio_access: mii-0000:bd:00.3 read  phy:0x07 reg:0x09 val:0x0200
>>>>>   kworker/u257:1-670   [015] ....    27.233110: mdio_access: mii-0000:bd:00.3 read  phy:0x07 reg:0x05 val:0x0000
>>>>>   kworker/u257:1-670   [000] ....    28.280475: mdio_access: mii-0000:bd:00.3 read  phy:0x07 reg:0x01 val:0x7989
>>>>>   kworker/u257:1-670   [000] ....    29.304471: mdio_access: mii-0000: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 <liuyonglong@huawei.com>
>>>>> ---
>>>>>  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=156413509900003&r=1&w=2 and a fix has
>>>> been applied already.
>>>>
>>>> Heiner
>>>>
>>>> .
>>>>
>>>
>>> In https://marc.info/?t=156413509900003&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.
> 
> 
Can you try the following? This delay should give thy PHY enough time
to clear both bits before the following read is done.

diff --git a/drivers/net/phy/phy.c b/drivers/net/phy/phy.c
index ef7aa738e..32f327a44 100644
--- a/drivers/net/phy/phy.c
+++ b/drivers/net/phy/phy.c
@@ -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);
+
 	if (phy_is_started(phydev))
 		err = phy_check_link_status(phydev);
 out_unlock:
-- 
2.22.0

Re: [PATCH net] net: phy: rtl8211f: do a double read to get real time link status

[Andrew Lunn]

> @@ -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?

If this PHY is broken with respect to the standard, i would prefer the
workaround is in the PHY specific driver code, not generic core code.

	   Andrew

Re: [PATCH net] net: phy: rtl8211f: do a double read to get real time link status

[Heiner Kallweit]

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.

> If this PHY is broken with respect to the standard, i would prefer the
> workaround is in the PHY specific driver code, not generic core code.
> 
Based on the C22 statement above the PHY may not be broken and the typical time between
two MDIO accesses is sufficient for the PHY to clear the bits. I think of MDIO bus access
functions in network chips that have a 10us-20us delay after each MDIO access.
On HNS3 this may not be the case.

> 	   Andrew
> 
Heiner

Re: [PATCH net] net: phy: rtl8211f: do a double read to get real time link status

[Andrew Lunn]

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

Re: [PATCH net] net: phy: rtl8211f: do a double read to get real time link status

[Yonglong Liu]

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-0000:bd:00.3 write phy:0x07 reg:0x00 val:0x1240
  kworker/u257:1-670   [015] ....    27.184670: mdio_access: mii-0000: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

[Heiner Kallweit]

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-0000:bd:00.3 write phy:0x07 reg:0x00 val:0x1240
>   kworker/u257:1-670   [015] ....    27.184670: mdio_access: mii-0000: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
-- 
2.22.0

Re: [PATCH net] net: phy: rtl8211f: do a double read to get real time link status

[Yonglong Liu]

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-0000:bd:00.3 write phy:0x07 reg:0x00 val:0x1240
>>   kworker/u257:1-670   [015] ....    27.184670: mdio_access: mii-0000: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-0000:bd:00.3 read  phy:0x07 reg:0x00 val:0x1040
  kworker/u257:1-670   [020] ....    27.026734: mdio_access: mii-0000:bd:00.3 read  phy:0x07 reg:0x04 val:0x01e1
  kworker/u257:1-670   [020] ....    27.026744: mdio_access: mii-0000:bd:00.3 write phy:0x07 reg:0x04 val:0x05e1
  kworker/u257:1-670   [020] ....    27.026799: mdio_access: mii-0000:bd:00.3 read  phy:0x07 reg:0x01 val:0x79ad
  kworker/u257:1-670   [020] ....    27.026834: mdio_access: mii-0000:bd:00.3 read  phy:0x07 reg:0x09 val:0x0200
  kworker/u257:1-670   [020] ....    27.026869: mdio_access: mii-0000:bd:00.3 read  phy:0x07 reg:0x00 val:0x1040
  kworker/u257:1-670   [020] ....    27.026879: mdio_access: mii-0000:bd:00.3 write phy:0x07 reg:0x00 val:0x1240
  kworker/u257:1-670   [020] ....    27.026932: mdio_access: mii-0000:bd:00.3 read  phy:0x07 reg:0x00 val:0x1240
  kworker/u257:1-670   [020] ....    28.031770: mdio_access: mii-0000:bd:00.3 read  phy:0x07 reg:0x00 val:0x1040
  kworker/u257:1-670   [020] ....    28.031837: mdio_access: mii-0000:bd:00.3 read  phy:0x07 reg:0x01 val:0x7989
  kworker/u257:1-670   [020] ....    29.055837: mdio_access: mii-0000:bd:00.3 read  phy:0x07 reg:0x00 val:0x1040
  kworker/u257:1-670   [020] ....    29.055873: mdio_access: mii-0000:bd:00.3 read  phy:0x07 reg:0x01 val:0x7989
  kworker/u257:0-8     [004] ....    30.079840: mdio_access: mii-0000:bd:00.3 read  phy:0x07 reg:0x00 val:0x1040
  kworker/u257:0-8     [004] ....    30.079875: mdio_access: mii-0000:bd:00.3 read  phy:0x07 reg:0x01 val:0x7989
  kworker/u257:0-8     [004] ....    31.103771: mdio_access: mii-0000:bd:00.3 read  phy:0x07 reg:0x00 val:0x1040
  kworker/u257:0-8     [004] ....    31.103839: mdio_access: mii-0000:bd:00.3 read  phy:0x07 reg:0x01 val:0x79a9
  kworker/u257:0-8     [004] ....    31.103906: mdio_access: mii-0000:bd:00.3 read  phy:0x07 reg:0x0a val:0x7803
  kworker/u257:0-8     [004] ....    31.103973: mdio_access: mii-0000:bd:00.3 read  phy:0x07 reg:0x09 val:0x0200
  kworker/u257:0-8     [004] ....    31.104041: mdio_access: mii-0000:bd:00.3 read  phy:0x07 reg:0x05 val:0xc1e1
  kworker/u257:0-8     [004] ....    32.127814: mdio_access: mii-0000:bd:00.3 read  phy:0x07 reg:0x00 val:0x1040
  kworker/u257:0-8     [004] ....    32.127881: mdio_access: mii-0000:bd:00.3 read  phy:0x07 reg:0x01 val:0x79ad
  kworker/u257:0-8     [004] ....    32.127948: mdio_access: mii-0000:bd:00.3 read  phy:0x07 reg:0x0a val:0x4800
  kworker/u257:0-8     [004] ....    32.128015: mdio_access: mii-0000:bd:00.3 read  phy:0x07 reg:0x09 val:0x0200
  kworker/u257:0-8     [004] ....    32.128082: mdio_access: mii-0000:bd:00.3 read  phy:0x07 reg:0x05 val:0xc1e1
  kworker/u257:0-8     [004] ....    33.151775: mdio_access: mii-0000:bd:00.3 read  phy:0x07 reg:0x00 val:0x1040
  kworker/u257:0-8     [004] ....    33.151815: mdio_access: mii-0000:bd:00.3 read  phy:0x07 reg:0x01 val:0x79ad
  kworker/u257:1-670   [021] ....    34.175771: mdio_access: mii-0000:bd:00.3 read  phy:0x07 reg:0x00 val:0x1040
  kworker/u257:1-670   [021] ....    34.175838: mdio_access: mii-0000:bd:00.3 read  phy:0x07 reg:0x01 val:0x79ad
        ifconfig-1177  [005] ....    35.052340: mdio_access: mii-0000:bd:00.3 read  phy:0x07 reg:0x00 val:0x1040
        ifconfig-1177  [005] ....    35.052350: mdio_access: mii-0000:bd:00.3 write phy:0x07 reg:0x00 val:0x1840