Received-SPF: pass (google.com: domain of linux-kernel+bounces-174741-linux.lists.archive=gmail.com@vger.kernel.org designates 147.75.199.223 as permitted sender) client-ip=147.75.199.223;
Precedence: bulk
MIME-Version: 1.0
References: <20240503101836.32755-1-en-wei.wu@canonical.com>
 <83a2c15e-12ef-4a33-a1f1-8801acb78724@lunn.ch> <514e990b-50c6-419b-96f2-09c3d04a2fda@intel.com>
 <334396b5-0acc-43f7-b046-30bcdab1b6fb@intel.com> <cc58ecfc-53f1-4154-bc38-e73964a59e16@lunn.ch>
In-Reply-To: <cc58ecfc-53f1-4154-bc38-e73964a59e16@lunn.ch>
From: En-Wei WU <en-wei.wu@canonical.com>
Date: Thu, 9 May 2024 19:40:02 +0200
Message-ID: <CAMqyJG2Xnn7VtT1CrCXK7ojuUmP+ig8uwB30uK3nprPo5hLiUQ@mail.gmail.com>
Subject: Re: [Intel-wired-lan] [PATCH v2 2/2] e1000e: fix link fluctuations problem
To: Andrew Lunn <andrew@lunn.ch>
Cc: "Ruinskiy, Dima" <dima.ruinskiy@intel.com>, Sasha Neftin <sasha.neftin@intel.com>, 
	netdev@vger.kernel.org, rickywu0421@gmail.com, linux-kernel@vger.kernel.org, 
	edumazet@google.com, intel-wired-lan@lists.osuosl.org, kuba@kernel.org, 
	anthony.l.nguyen@intel.com, pabeni@redhat.com, davem@davemloft.net, 
	"Lifshits, Vitaly" <vitaly.lifshits@intel.com>, "naamax.meir" <naamax.meir@linux.intel.com>, 
	"Avivi, Amir" <amir.avivi@intel.com>, "Keller, Jacob E" <jacob.e.keller@intel.com>
Content-Type: text/plain; charset="UTF-8"

> En-Wei, My recommendation is not to accept these patches. If you think
> there is a HW/PHY problem - open a ticket on Intel PAE.

> I concur. I am wary of changing the behavior of some driver
> fundamentals, to satisfy a particular validation/certification flow, if
> there is no real functionality problem. It can open a big Pandora box.
OK. Thanks for your help. I think we can end this patchset now.

> It is normally a little over 1 second. I
> forget the exact number. But is the PHY being polled once a second,
> rather than being interrupt driven?
If I read the code correctly, the PHY is polled every 2 seconds by the
e1000e watchdog. But if an interrupt occurs and it's a
link-status-change interrupt, the watchdog will be called immediately
and the PHY is polled.

> What does it think the I219-LM is advertising? Is it advertising 1000BaseT_Half?
> But why would auto-neg resolve to that if 1000BaseT_Full is available?
I'm also interested in it. I'll do some checking later to see what's
advertising by us and the link partner.

> Agreed. Root cause this, which looks like a real problem, rather than
> apply a band-aid for a test system.
OK. I think there is a clue which is related to auto-negotiation. I'll
work on it later.

Thank all of you for your help, I really appreciate it.

On Thu, 9 May 2024 at 15:46, Andrew Lunn <andrew@lunn.ch> wrote:
>
> On Thu, May 09, 2024 at 12:13:27PM +0300, Ruinskiy, Dima wrote:
> > On 08/05/2024 8:05, Sasha Neftin wrote:
> > > On 07/05/2024 15:31, Andrew Lunn wrote:
> > > > On Fri, May 03, 2024 at 06:18:36PM +0800, Ricky Wu wrote:
> > > > > As described in https://bugzilla.kernel.org/show_bug.cgi?id=218642,
> > > > > Intel I219-LM reports link up -> link down -> link up after hot-plugging
> > > > > the Ethernet cable.
> > > >
> > > > Please could you quote some parts of 802.3 which state this is a
> > > > problem. How is this breaking the standard.
> > > >
> > > >     Andrew
> > >
> > > In I219-* parts used LSI PHY. This PHY is compliant with the 802.3 IEEE
> > > standard if I recall correctly. Auto-negotiation and link establishment
> > > are processed following the IEEE standard and could vary from platform
> > > to platform but are not violent to the IEEE standard.
> > >
> > > En-Wei, My recommendation is not to accept these patches. If you think
> > > there is a HW/PHY problem - open a ticket on Intel PAE.
> > >
> > > Sasha
> >
> > I concur. I am wary of changing the behavior of some driver fundamentals, to
> > satisfy a particular validation/certification flow, if there is no real
> > functionality problem. It can open a big Pandora box.
> >
> > Checking the Bugzilla report again, I am not sure we understand the issue
> > fully:
> >
> > [  143.141006] e1000e 0000:00:1f.6 enp0s31f6: NIC Link is Up 1000 Mbps Half
> > Duplex, Flow Control: None
> > [  143.144878] e1000e 0000:00:1f.6 enp0s31f6: NIC Link is Down
> > [  146.838980] e1000e 0000:00:1f.6 enp0s31f6: NIC Link is Up 1000 Mbps Full
> > Duplex, Flow Control: None
> >
> > This looks like a very quick link "flap", following by proper link
> > establishment ~3.7 seconds later. These ~3.7 seconds are in line of what
> > link auto-negotiation would take (auto-negotiation is the default mode for
> > this driver).
>
> That actually seems slow. It is normally a little over 1 second. I
> forget the exact number. But is the PHY being polled once a second,
> rather than being interrupt driven?
>
> > The first print (1000 Mbps Half Duplex) actually makes no
> > sense - it cannot be real link status since 1000/Half is not a supported
> > speed.
>
> It would be interesting to see what the link partner sees. What does
> it think the I219-LM is advertising? Is it advertising 1000BaseT_Half?
> But why would auto-neg resolve to that if 1000BaseT_Full is available?
>
> > So it seems to me that actually the first "link up" is an
> > incorrect/incomplete/premature reading, not the "link down".
>
> Agreed. Root cause this, which looks like a real problem, rather than
> apply a band-aid for a test system.
>
>       Andrew