Message-ID: <316a020a-aa49-700e-3735-f5f810adaaed@intel.com>
Date: Tue, 4 Mar 2025 12:48:06 +0200
From: "Lifshits, Vitaly" <vitaly.lifshits@...el.com>
To: Mark Pearson <mpearson-lenovo@...ebb.ca>, Andrew Lunn <andrew@...n.ch>
CC: <anthony.l.nguyen@...el.com>, <przemyslaw.kitszel@...el.com>,
	<andrew+netdev@...n.ch>, <davem@...emloft.net>, <edumazet@...gle.com>,
	<kuba@...nel.org>, <pabeni@...hat.com>, <intel-wired-lan@...ts.osuosl.org>,
	<netdev@...r.kernel.org>, <linux-kernel@...r.kernel.org>
Subject: Re: [Intel-wired-lan] [PATCH] e1000e: Link flap workaround option for
 false IRP events



On 3/3/2025 5:34 AM, Mark Pearson wrote:
> Hi Andrew,
> 
> On Sun, Mar 2, 2025, at 11:13 AM, Andrew Lunn wrote:
>> 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.
> 
> Yep - I'll look to add this.
> 
>>
>> 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!
>>
> I don't think we did tried those - it was never suggested that I can recall (the original debug started 6 months+ ago). I don't know fully what testing Intel did in their lab once the issue was reproduced there.
> 
> If you have any particular recommendations we can try that - with a note that we have to run a soak for ~1 week to have confidence if a change made a difference (the issue can reproduce between 1 to 2 days).

Personally I doubt that it is related to EEE since there was no real 
link flap.

I suggest to try replacing the register read for a short delay or 
reading the PHY STATUS register instead.

> 
>>> 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?
>>
> 
> I'm fairly certain there is no actual link bounce but I don't know the reason for the interrupt or why it was triggered.
> 
> Vitaly, do you have a way of getting these answers from the Intel team that worked on this? I don't think I'll be able to get any answers, unfortunately.

You are correct, from what we saw there was no real link flap there. 
Only a false link status change interrupt.

> 
>>> 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.
>>
> Ack - I'll aim to do that, as best I can.
> 
> I think as Vitaly notes, the read should not be introduced for the general case, in case it misses link bounces in other situations?
> 
> Does that confirm that the module option, so it can be selectively enabled, is a reasonable workaround solution. Let me know if there are other ideas.
> 
> Thanks
> Mark
> 

Powered by blists - more mailing lists