Re: [Intel-wired-lan] [PATCH] e1000e: Link flap workaround option for false IRP events

[Andrew Lunn]

On Sun, Mar 02, 2025 at 03:09:35PM +0200, Lifshits, Vitaly wrote:
> 
> 
> Hi Mark,
> 
> > Hi Andrew
> > 
> > On Thu, Feb 27, 2025, at 11:07 AM, Andrew Lunn wrote:
> > > > > > +			e1e_rphy(hw, PHY_REG(772, 26), &phy_data);
> > > > > 
> > > > > Please add some #define for these magic numbers, so we have some idea
> > > > > what PHY register you are actually reading. That in itself might help
> > > > > explain how the workaround actually works.
> > > > > 
> > > > 
> > > > I don't know what this register does I'm afraid - that's Intel knowledge and has not been shared.
> > > 
> > > What PHY is it? Often it is just a COTS PHY, and the datasheet might
> > > be available.
> > > 
> > > Given your setup description, pause seems like the obvious thing to
> > > check. When trying to debug this, did you look at pause settings?
> > > Knowing what this register is might also point towards pause, or
> > > something totally different.
> > > 
> > > 	Andrew
> > 
> > For the PHY - do you know a way of determining this easily? I can reach out to the platform team but that will take some time. I'm not seeing anything in the kernel logs, but if there's a recommended way of confirming that would be appreciated.
> 
> The PHY is I219 PHY.
> The datasheet is indeed accessible to the public:
> https://cdrdv2-public.intel.com/612523/ethernet-connection-i219-datasheet.pdf

Thanks for the link.

So it is reading page 772, register 26. Page 772 is all about LPI. So
we can have a #define for that. Register 26 is Memories Power. So we
can also have an #define for that.

However, that does not really help explain how this helps prevent an
interrupt. I assume playing with EEE settings was also played
with. Not that is register appears to have anything to do with EEE!

> Reading this register was suggested for debug purposes to understand if
> there is some misconfiguration. We did not find any misconfiguration.
> The issue as we discovered was a link status change interrupt caused the
> CSME to reset the adapter causing the link flap.
> 
> We were unable to determine what causes the link status change interrupt in
> the first place. As stated in the comment, it was only ever observed on
> Lenovo P5/P7systems and we couldn't ever reproduce on other systems. The
> reproduction in our lab was on a P5 system as well.
> 
> 
> Regarding the suggested workaround, there isn’t a clear understanding why it
> works. We suspect that reading a PHY register is probably prevents the CSME
> from resetting the PHY when it handles the LSC interrupt it gets. However,
> it can also be a matter of slight timing variations.

I don't follow what you are saying here. As far as i can see, the
interrupt handler will triggers a read of the BMCR to determine the
link status. It should not matter if there is a spurious interrupt,
the BMCR should report the truth. So does BMCR actually indicate the
link did go down? I also see there is the usual misunderstanding with
how BMCR is latching. It should not be read twice, processed once, it
should be processed each time, otherwise you miss quick link down/up
events.

> We communicated that this solution is not likely to be accepted to the
> kernel as is, and the initial responses on the mailing list demonstrate the
> pushback.

What it has done is start a discussion towards an acceptable
solution. Which is a good thing. But at the moment, the discussion
does not have sufficient details.

Please could somebody describe the chain of events which results in
the link down, and subsequent link up. Is the interrupt spurious, or
does BMCR really indicate the link went down and up again?

> On a different topic, I suggest removing the part of the comment below:
> * Intel unable to determine root cause.
> The issue went through joint debug by Intel and Lenovo, and no obvious spec
> violations by either party were found. There doesn’t seem to be value in
> including this information in the comments of upstream code.

I partially agree. Assuming the root cause is not found, and a
workaround is used, i expect a commit message with a detailed
description of the chain of events which results in the link
flap. Then a statement that the root cause is unknown, and lastly the
commit message should say the introduced change, for unknown reasons,
solves the issue, and is considered safe because.... Ideally the
workaround should be safe for everybody, and can be just enabled.

	Andrew