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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <06cd6f53-e0af-4bdf-a684-68fc55b9b436@lunn.ch>
Date: Tue, 17 Oct 2023 15:59:03 +0200
From: Andrew Lunn <andrew@...n.ch>
To: Jijie Shao <shaojijie@...wei.com>
Cc: yisen.zhuang@...wei.com, salil.mehta@...wei.com, davem@...emloft.net,
	edumazet@...gle.com, kuba@...nel.org, pabeni@...hat.com,
	shenjian15@...wei.com, wangjie125@...wei.com,
	liuyonglong@...wei.com, wangpeiyang1@...wei.com,
	netdev@...r.kernel.org, stable@...r.kernel.org,
	linux-kernel@...r.kernel.org
Subject: 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:
> > 
> > on 2023/7/30 2:23, Andrew Lunn wrote:
> > > >      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