Re: [PATCH net 5/6] net: hns3: fix wrong print link down up

On Tue, Oct 17, 2023 at 09:03:01PM +0800, Jijie Shao wrote:

on 2023/7/31 17:10, Jijie Shao wrote:

quoted

on 2023/7/30 2:23, Andrew Lunn wrote:

quoted

quoted

     Now i wounder if you are fixing the wrong thing. Maybe you
should be
     fixing the PHY so it does not report up and then down? You
say 'very
     snall intervals', which should in fact be 1 second. So is the PHY
     reporting link for a number of poll intervals? 1min to 10 minutes?

               Andrew

Yes, according to the log records, the phy polls every second,
but the link status changes take time.
Generally, it takes 10 seconds for the phy to detect link down,
but occasionally it takes several minutes to detect link down,

What PHY driver is this?

It is not so clear what should actually happen with auto-neg turned
off. With it on, and the link going down, the PHY should react after
about 1 second. It is not supposed to react faster than that, although
some PHYs allow fast link down notification to be configured.

Have you checked 802.3 to see what it says about auto-neg off and link
down detection?

I personally would not suppress this behaviour in the MAC
driver. Otherwise you are going to have funny combinations of special
cases of a feature which very few people actually use, making your
maintenance costs higher.

        Andrew

Hi Andrew,
We've rewritten the commit log to explain this problem,
Would you please take some time to review that?

The following is the new commit log:
This patch is to correct a wrong log info "link down/up" in hns3 driver.
When setting autoneg off without changing speed and duplex, the link
should be not changed. However in hns3 driver, it print link down/up once
incorrectly. We trace the phy machine state and find the phy change form
PHY_UP to PHY_RUNNING. No other state of PHY occurs during this process.
MDIO trace also indicate the link is on. The wrong log info and mdio
trace are showed as followed:

[  843.720783][  T367] hns3 0000:35:00.0 eth1: set link(phy): autoneg=0,
speed=10, duplex=1
[  843.736087][  T367] hns3 0000:35:00.0 eth1: link down
[  843.773506][   T17] RTL8211F Gigabit Ethernet mii-0000:35:00.0:02: PHY
state change UP -> RUNNING
[  844.674668][   T31] hns3 0000:35:00.0 eth1: link up

I still think this is totally valid and correct.

When you turn auto-neg off the link partner is going to react to that,
it might drop the link. After a while, the link partner will give up
trying to perform auto-neg and might fall back to 10/Half. At which
point, the link might allow traffic flow. However, in this example,
you have a duplex mis-match, so it might not work correctly.

Turning off auto-neg is something you need to do at both ends, and you
need to then force both ends to the same settings. Link down is
expected. I would actually be suppressed if no link down events were
reported.

	Andrew