Message ID | 1565183772-44268-1-git-send-email-liuyonglong@huawei.com |
---|---|
State | Changes Requested |
Delegated to: | David Miller |
Headers | show |
Series | [net] net: phy: rtl8211f: do a double read to get real time link status | expand |
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
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.
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.
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.
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:
> @@ -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
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
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
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
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
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
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",
[ 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(+)