[PATCH] e1000e: Link flap workaround option for false IRP events

[PATCH] e1000e: Link flap workaround option for false IRP events

[Mark Pearson]

Issue is seen on some Lenovo desktop workstations where there
is a false IRP event which triggers a link flap.
Condition is rare and only seen on networks where link speed
may differ along the path between nodes (e.g 10M/100M)

Intel are not able to determine root cause but provided a
workaround that does fix the issue. Tested extensively at Lenovo.

Adding a module option to enable this workaround for users
who are impacted by this issue.

Signed-off-by: Mark Pearson <mpearson-lenovo@squebb.ca>
---
 drivers/net/ethernet/intel/e1000e/netdev.c | 19 +++++++++++++++++++
 1 file changed, 19 insertions(+)

diff --git a/drivers/net/ethernet/intel/e1000e/netdev.c b/drivers/net/ethernet/intel/e1000e/netdev.c
index 286155efcedf..06774fb4b2dd 100644
--- a/drivers/net/ethernet/intel/e1000e/netdev.c
+++ b/drivers/net/ethernet/intel/e1000e/netdev.c
@@ -37,6 +37,10 @@ static int debug = -1;
 module_param(debug, int, 0);
 MODULE_PARM_DESC(debug, "Debug level (0=none,...,16=all)");
 
+static int false_irp_workaround;
+module_param(false_irp_workaround, int, 0);
+MODULE_PARM_DESC(false_irp_workaround, "Enable workaround for rare false IRP event causing link flap");
+
 static const struct e1000_info *e1000_info_tbl[] = {
 	[board_82571]		= &e1000_82571_info,
 	[board_82572]		= &e1000_82572_info,
@@ -1757,6 +1761,21 @@ static irqreturn_t e1000_intr_msi(int __always_unused irq, void *data)
 	/* read ICR disables interrupts using IAM */
 	if (icr & E1000_ICR_LSC) {
 		hw->mac.get_link_status = true;
+
+		/*
+		 * False IRP workaround
+		 * Issue seen on Lenovo P5 and P7 workstations where if there
+		 * are different link speeds in the network a false IRP event
+		 * is received, leading to a link flap.
+		 * Intel unable to determine root cause. This read prevents
+		 * the issue occurring
+		 */
+		if (false_irp_workaround) {
+			u16 phy_data;
+
+			e1e_rphy(hw, PHY_REG(772, 26), &phy_data);
+		}
+
 		/* ICH8 workaround-- Call gig speed drop workaround on cable
 		 * disconnect (LSC) before accessing any PHY registers
 		 */
-- 
2.43.0

Re: [PATCH] e1000e: Link flap workaround option for false IRP events

[Andrew Lunn]

On Wed, Feb 26, 2025 at 02:44:12PM -0500, Mark Pearson wrote:
> Issue is seen on some Lenovo desktop workstations where there
> is a false IRP event which triggers a link flap.
> Condition is rare and only seen on networks where link speed
> may differ along the path between nodes (e.g 10M/100M)
> 
> Intel are not able to determine root cause but provided a
> workaround that does fix the issue. Tested extensively at Lenovo.
> 
> Adding a module option to enable this workaround for users
> who are impacted by this issue.

Why is a module option needed? Does the workaround itself introduce
issues? Please describe those issues?

In general, module options are not liked. So please include in the
commit message why a module option is the only option.
 
> Signed-off-by: Mark Pearson <mpearson-lenovo@squebb.ca>
> ---
>  drivers/net/ethernet/intel/e1000e/netdev.c | 19 +++++++++++++++++++
>  1 file changed, 19 insertions(+)
> 
> diff --git a/drivers/net/ethernet/intel/e1000e/netdev.c b/drivers/net/ethernet/intel/e1000e/netdev.c
> index 286155efcedf..06774fb4b2dd 100644
> --- a/drivers/net/ethernet/intel/e1000e/netdev.c
> +++ b/drivers/net/ethernet/intel/e1000e/netdev.c
> @@ -37,6 +37,10 @@ static int debug = -1;
>  module_param(debug, int, 0);
>  MODULE_PARM_DESC(debug, "Debug level (0=none,...,16=all)");
>  
> +static int false_irp_workaround;
> +module_param(false_irp_workaround, int, 0);
> +MODULE_PARM_DESC(false_irp_workaround, "Enable workaround for rare false IRP event causing link flap");
> +
>  static const struct e1000_info *e1000_info_tbl[] = {
>  	[board_82571]		= &e1000_82571_info,
>  	[board_82572]		= &e1000_82572_info,
> @@ -1757,6 +1761,21 @@ static irqreturn_t e1000_intr_msi(int __always_unused irq, void *data)
>  	/* read ICR disables interrupts using IAM */
>  	if (icr & E1000_ICR_LSC) {
>  		hw->mac.get_link_status = true;
> +
> +		/*
> +		 * False IRP workaround
> +		 * Issue seen on Lenovo P5 and P7 workstations where if there
> +		 * are different link speeds in the network a false IRP event
> +		 * is received, leading to a link flap.
> +		 * Intel unable to determine root cause. This read prevents
> +		 * the issue occurring
> +		 */
> +		if (false_irp_workaround) {
> +			u16 phy_data;
> +
> +			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.

    Andrew

---
pw-bot: cr

Re: [PATCH] e1000e: Link flap workaround option for false IRP events

[Mark Pearson]

Hi Andrew,

On Wed, Feb 26, 2025, at 5:52 PM, Andrew Lunn wrote:
> On Wed, Feb 26, 2025 at 02:44:12PM -0500, Mark Pearson wrote:
>> Issue is seen on some Lenovo desktop workstations where there
>> is a false IRP event which triggers a link flap.
>> Condition is rare and only seen on networks where link speed
>> may differ along the path between nodes (e.g 10M/100M)
>> 
>> Intel are not able to determine root cause but provided a
>> workaround that does fix the issue. Tested extensively at Lenovo.
>> 
>> Adding a module option to enable this workaround for users
>> who are impacted by this issue.
>
> Why is a module option needed? Does the workaround itself introduce
> issues? Please describe those issues?
>
> In general, module options are not liked. So please include in the
> commit message why a module option is the only option.
> 

Understood. 

The reason for the module option is I'm playing it safe, as Intel couldn't determine root cause.
The aim of the patch is to keep the effect to a minimum whilst allowing users who are impacted to turn on the workaround, if they are encountering the issue.

Issue details:
We have seen the issue when running high level traffic on a network involving at least two nodes and also having two different network speeds are need. For example:
[Lenovo WS] <---1G link---> Network switch <---100M link--->[traffic source]
The link flap can take a day or two to reproduce - it's rare.

We worked for a long time with the Intel networking team to try and root cause the issue but unfortunately, despite being able to reproduce the issue in their lab, they decided to not pursue the investigation. They suggested the register read as a workaround and we confirmed it fixes the problem (setup ran for weeks without issue - we haven't seen any side issues). Unfortunately nobody can explain why the fix works.

I don't think the workaround should be implemented as a general case without support from Intel. 
I considered a DMI quirk, but without root cause I do worry about unknown side effects.
There is also the possibility of the issue showing up on other platforms we don't know of yet - and I wanted a way to be able to easily enable it if needed (e.g be able to tell a customer - try enabling this and see if it fixes it).

A module option seemed like a good compromise, but I'm happy to consider alternatives if there are any recommendations.

>> Signed-off-by: Mark Pearson <mpearson-lenovo@squebb.ca>
>> ---
>>  drivers/net/ethernet/intel/e1000e/netdev.c | 19 +++++++++++++++++++
>>  1 file changed, 19 insertions(+)
>> 
>> diff --git a/drivers/net/ethernet/intel/e1000e/netdev.c b/drivers/net/ethernet/intel/e1000e/netdev.c
>> index 286155efcedf..06774fb4b2dd 100644
>> --- a/drivers/net/ethernet/intel/e1000e/netdev.c
>> +++ b/drivers/net/ethernet/intel/e1000e/netdev.c
>> @@ -37,6 +37,10 @@ static int debug = -1;
>>  module_param(debug, int, 0);
>>  MODULE_PARM_DESC(debug, "Debug level (0=none,...,16=all)");
>>  
>> +static int false_irp_workaround;
>> +module_param(false_irp_workaround, int, 0);
>> +MODULE_PARM_DESC(false_irp_workaround, "Enable workaround for rare false IRP event causing link flap");
>> +
>>  static const struct e1000_info *e1000_info_tbl[] = {
>>  	[board_82571]		= &e1000_82571_info,
>>  	[board_82572]		= &e1000_82572_info,
>> @@ -1757,6 +1761,21 @@ static irqreturn_t e1000_intr_msi(int __always_unused irq, void *data)
>>  	/* read ICR disables interrupts using IAM */
>>  	if (icr & E1000_ICR_LSC) {
>>  		hw->mac.get_link_status = true;
>> +
>> +		/*
>> +		 * False IRP workaround
>> +		 * Issue seen on Lenovo P5 and P7 workstations where if there
>> +		 * are different link speeds in the network a false IRP event
>> +		 * is received, leading to a link flap.
>> +		 * Intel unable to determine root cause. This read prevents
>> +		 * the issue occurring
>> +		 */
>> +		if (false_irp_workaround) {
>> +			u16 phy_data;
>> +
>> +			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.

This approach, with magic numbers, is used all over the place in the driver and related modules, presumably contributed previously by Intel engineers. Can I push back on this request with a note that Intel would need to provide the register definitions for their components first.

Thanks for the review. I'll give it a couple of days to see if any other feedback, and push a v2 with updated commit description.

Mark

Re: [PATCH] e1000e: Link flap workaround option for false IRP events

[Andrew Lunn]

> The reason for the module option is I'm playing it safe, as Intel couldn't determine root cause.

> The aim of the patch is to keep the effect to a minimum whilst
> allowing users who are impacted to turn on the workaround, if they
> are encountering the issue.

And how do such users determine this module parameter exists? You need
to be a pretty savvy user to know about them, and how the set them,
etc.

> Issue details:

> We have seen the issue when running high level traffic on a network
> involving at least two nodes and also having two different network
> speeds are need. For example:

> [Lenovo WS] <---1G link---> Network switch <---100M link--->[traffic source]
> The link flap can take a day or two to reproduce - it's rare.

By flap, you mean down/up? So the network dies for a couple of seconds
while autoneg happens? And this repeats at 1 to 2 day intervals?

Or does the link go down, and stay down, and it needs user
intervention to get the link back?

	Andrew

Re: [PATCH] e1000e: Link flap workaround option for false IRP events

[Andrew Lunn]

> >> +			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

Re: [PATCH] e1000e: Link flap workaround option for false IRP events

[Mark Pearson]

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.

We did look at at the pause pieces - which I agree seems like an obvious candidate given the speed mismatch on the network.
Experts on the Intel networking team did reproduce the issue in their lab and looked at this for many weeks without determining root cause. I wish it was as obvious as pause control configuration :)

Thanks
Mark

Re: [PATCH] e1000e: Link flap workaround option for false IRP events

[Andrew Lunn]

> For the PHY - do you know a way of determining this easily?

Add printk()s to e1000e_get_phy_type_from_id().

	Andrew

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

[Lifshits, Vitaly]

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

> 
> We did look at at the pause pieces - which I agree seems like an obvious candidate given the speed mismatch on the network.
> Experts on the Intel networking team did reproduce the issue in their lab and looked at this for many weeks without determining root cause. I wish it was as obvious as pause control configuration :)
> 
> Thanks
> Mark
> 

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.
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. We do understand the frustration of end-users that may 
experience the problem. A couple of suggestions that can make it look 
less “out-of-the-blue” are: try a short delay instead of the register 
read, or read a more common register like PHY STATUS instead.
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.

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

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

[Mark Pearson]

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).

>> 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.

>> 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

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

[Lifshits, Vitaly]

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
> 

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

[Andrew Lunn]

> > > 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 tend to agree. Despite the group of registers being called LPI, it
appears this one has nothing to do with LPI? It would probably been
better to have it in page 776, General Registers, but that region is
full.

> > > 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.
 
So if BMCR shows no state change, why is the driver doing anything?

I really would like to understand the chain of events. Once we
understand the chain of events, we can probably come up with a change
somewhere in the chain to break it, so the spurious interrupt is
ignored.

	Andrew

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

[Mark Pearson]

Thanks Vitaly,

On Tue, Mar 4, 2025, at 5:48 AM, Lifshits, Vitaly wrote:
> 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.
>

Ack - we'll try that, and collect some other debug registers in the process.
Will update with findings - this may take a while :)

Thanks
Mark

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

[Andrew Lunn]

> > I suggest to try replacing the register read for a short delay or 
> > reading the PHY STATUS register instead.
> >
> 
> Ack - we'll try that, and collect some other debug registers in the process.
> Will update with findings - this may take a while :)

Please be careful with which register you choice. Because the link
status bit in BMSR is latching, you should not be reading it and
discarding the result.

Reading register 2 or 3 should be totally safe.

Another thing to keep in mind, you cannot unconditionally read a paged
register in this particular PHY, because the e1000e is used with a
number of different PHYs. That register does not exist in other PHYs,
and the action of selecting the page performs a register write, which
for some other PHY could be destructive. So i would suggest you keep
to registers defined in 802.3 C22.

	Andrew

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

[Mark Pearson]

Hi Vitaly,

On Sun, Mar 2, 2025, at 8:09 AM, 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
>
>> 
>> We did look at at the pause pieces - which I agree seems like an obvious candidate given the speed mismatch on the network.
>> Experts on the Intel networking team did reproduce the issue in their lab and looked at this for many weeks without determining root cause. I wish it was as obvious as pause control configuration :)
>> 
>> Thanks
>> Mark
>> 
>
> 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.
> 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. We do understand the frustration of end-users that may 
> experience the problem. A couple of suggestions that can make it look 
> less “out-of-the-blue” are: try a short delay instead of the register 
> read, or read a more common register like PHY STATUS instead.
> On a different topic, I suggest removing the part of the comment below:
> * Intel unable to determine root cause.

Thank you for the details.

I agree that the Intel networking team communicated to us that the solution provided to us was not suitable for upstream (just adding in the read directly) - I agreed with that and add the part about it being a module option to make it, hopefully, more palatable.
 
The problem is that Intel have stated to us, in writing, that they would not investigate further and would not work on an upstream appropriate solution (a requirement for us - out of tree patches are not suitable as I'm sure we all agree).
 
I'm not going to go into the internal communication details/escalations/etc here - but this is the only reason I'm doing this patch. I would much rather Intel had continued the debug exercise.

> 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'm OK to remove that statement, but I do want to highlight that it is factually correct.
My intent with this patch was not to dump on Intel or cause offence, but to provide context as to why a Lenovo employee is doing a somewhat peculiar workaround for an Intel networking device :)

I was planning on updating the commit message so I will look to make it less inflammatory.

Thanks for chiming in and providing the details. I'm treading very carefully in what is an extremely awkward position to be put. 

Mark