igb: fix ptp suspend/resume issue

PATCH Superseded Jacob Keller 3 participants 13 messages May 11, 2016 – May 17, 2016

Subsystems (2)

Network drivers Intel Ethernet

lore

Share & tools

lore

Jacob Keller

author

2016-05-11 23:18 UTC #0 lore raw

v2 cover igb: fix ptp suspend/resume issue

This patch series (properly) fixes the issue with igb's workqueue item for overflow check from causing a surprise remove event. To do this, properly suspend the workqueue items in suspend and then resume them again during the resume flow.

The patch series has a few extra steps to reduce code duplication and implement suspend and resume properly, which makes the overall fix a bit more complicated, and thus review is welcome.

A smaller fix would be to implement suspend and resume irrespective of the current igb_ptp_stop and igb_ptp_init but this seems more likely to introduce bugs especially if either function ever changes in the future.

In addition, the ptp_flags variable is added mostly to simplify the work of writing several complex MAC type checks in the ptp code while doing this.

Jacob Keller (5):
igb: introduce ptp_flags variable and use it to replace IGB_FLAG_PTP
igb: introduce IGB_PTP_OVERFLOW_CHECK flag
igb: introduce igb_ptp_resume function
igb: implement igb_ptp_suspend
igb: call igb_ptp_suspend/igb_ptp_resume during suspend/resume cycle

 drivers/net/ethernet/intel/igb/igb.h      |   8 ++-
 drivers/net/ethernet/intel/igb/igb_main.c |   4 +-
 drivers/net/ethernet/intel/igb/igb_ptp.c  | 110 ++++++++++++++++--------------
 3 files changed, 68 insertions(+), 54 deletions(-)

Signature · 1 line

2.8.1.493.g7b22612

Jacob Keller

author

2016-05-11 23:18 UTC #1 lore raw

PATCH v2 1/5 igb: introduce ptp_flags variable and use it to replace IGB_FLAG_PTP

Upcoming patches will introduce new PTP specific flags. To avoid cluttering the normal flags variable, introduce PTP specific "ptp_flags" variable for this purpose, and move IGB_FLAG_PTP to become IGB_PTP_ENABLED.

Signed-off-by Jacob Keller <[email protected]>

---
 drivers/net/ethernet/intel/igb/igb.h     | 5 ++++-
 drivers/net/ethernet/intel/igb/igb_ptp.c | 8 ++++----
 2 files changed, 8 insertions(+), 5 deletions(-)

drivers/net/ethernet/intel/igb/igb.h +4 -1

index b9609afa5ca3..1e18a9eb16e0 100644
@@ -445,6 +445,7 @@ struct igb_adapter {
 	unsigned long ptp_tx_start;
 	unsigned long last_rx_ptp_check;
 	unsigned long last_rx_timestamp;
+	unsigned int ptp_flags;
 	spinlock_t tmreg_lock;
 	struct cyclecounter cc;
 	struct timecounter tc;

@@ -474,12 +475,14 @@ struct igb_adapter {
 	u16 eee_advert;
 };
 
+/* flags controlling PTP/1588 function */
+#define IGB_PTP_ENABLED		BIT(0)
+
 #define IGB_FLAG_HAS_MSI		BIT(0)
 #define IGB_FLAG_DCA_ENABLED		BIT(1)
 #define IGB_FLAG_QUAD_PORT_A		BIT(2)
 #define IGB_FLAG_QUEUE_PAIRS		BIT(3)
 #define IGB_FLAG_DMAC			BIT(4)
-#define IGB_FLAG_PTP			BIT(5)
 #define IGB_FLAG_RSS_FIELD_IPV4_UDP	BIT(6)
 #define IGB_FLAG_RSS_FIELD_IPV6_UDP	BIT(7)
 #define IGB_FLAG_WOL_SUPPORTED		BIT(8)

drivers/net/ethernet/intel/igb/igb_ptp.c +4 -4

index f097c5a8ab93..88e3b40415a1 100644
@@ -684,7 +684,7 @@ void igb_ptp_rx_hang(struct igb_adapter *adapter)
 	u32 tsyncrxctl = rd32(E1000_TSYNCRXCTL);
 	unsigned long rx_event;
 
-	if (hw->mac.type != e1000_82576)
+	if (!(adapter->ptp_flags & IGB_PTP_ENABLED))
 		return;
 
 	/* If we don't have a valid timestamp in the registers, just update the

@@ -1156,7 +1156,7 @@ void igb_ptp_init(struct igb_adapter *adapter)
 	} else {
 		dev_info(&adapter->pdev->dev, "added PHC on %s\n",
 			 adapter->netdev->name);
-		adapter->flags |= IGB_FLAG_PTP;
+		adapter->ptp_flags |= IGB_PTP_ENABLED;
 	}
 }

@@ -1194,7 +1194,7 @@ void igb_ptp_stop(struct igb_adapter *adapter)
 		ptp_clock_unregister(adapter->ptp_clock);
 		dev_info(&adapter->pdev->dev, "removed PHC on %s\n",
 			 adapter->netdev->name);
-		adapter->flags &= ~IGB_FLAG_PTP;
+		adapter->ptp_flags &= ~IGB_PTP_ENABLED;
 	}
 }

@@ -1209,7 +1209,7 @@ void igb_ptp_reset(struct igb_adapter *adapter)
 	struct e1000_hw *hw = &adapter->hw;
 	unsigned long flags;
 
-	if (!(adapter->flags & IGB_FLAG_PTP))
+	if (!(adapter->ptp_flags & IGB_PTP_ENABLED))
 		return;
 
 	/* reset the tstamp_config */

Signature · 1 line

2.8.1.493.g7b22612

Jacob Keller

author

2016-05-11 23:18 UTC #2 lore raw

PATCH v2 2/5 igb: introduce IGB_PTP_OVERFLOW_CHECK flag

In reply to Jacob Keller

Don't continue to use complex MAC type checks for handling various cases where we have overflow check code. Make this code more obvious by introducing a flag which is enabled for hardware that needs these checks.

Signed-off-by Jacob Keller <[email protected]>

---
 drivers/net/ethernet/intel/igb/igb.h     |  1 +
 drivers/net/ethernet/intel/igb/igb_ptp.c | 21 ++++++++-------------
 2 files changed, 9 insertions(+), 13 deletions(-)

drivers/net/ethernet/intel/igb/igb.h +1 -0

index 1e18a9eb16e0..38daaaba0cdb 100644
@@ -477,6 +477,7 @@ struct igb_adapter {
 
 /* flags controlling PTP/1588 function */
 #define IGB_PTP_ENABLED		BIT(0)
+#define IGB_PTP_OVERFLOW_CHECK	BIT(1)
 
 #define IGB_FLAG_HAS_MSI		BIT(0)
 #define IGB_FLAG_DCA_ENABLED		BIT(1)

drivers/net/ethernet/intel/igb/igb_ptp.c +8 ~~-13~~

index 88e3b40415a1..e2c494ad1be0 100644
@@ -1066,6 +1066,7 @@ void igb_ptp_init(struct igb_adapter *adapter)
 		adapter->cc.shift = IGB_82576_TSYNC_SHIFT;
 		/* Dial the nominal frequency. */
 		wr32(E1000_TIMINCA, INCPERIOD_82576 | INCVALUE_82576);
+		adapter->ptp_flags |= IGB_PTP_OVERFLOW_CHECK;
 		break;
 	case e1000_82580:
 	case e1000_i354:

@@ -1086,6 +1087,7 @@ void igb_ptp_init(struct igb_adapter *adapter)
 		adapter->cc.shift = 0;
 		/* Enable the timer functions by clearing bit 31. */
 		wr32(E1000_TSAUXC, 0x0);
+		adapter->ptp_flags |= IGB_PTP_OVERFLOW_CHECK;
 		break;
 	case e1000_i210:
 	case e1000_i211:

@@ -1131,7 +1133,9 @@ void igb_ptp_init(struct igb_adapter *adapter)
 	} else {
 		timecounter_init(&adapter->tc, &adapter->cc,
 				 ktime_to_ns(ktime_get_real()));
+	}
 
+	if (adapter->ptp_flags & IGB_PTP_OVERFLOW_CHECK) {
 		INIT_DELAYED_WORK(&adapter->ptp_overflow_work,
 				  igb_ptp_overflow_check);

@@ -1168,20 +1172,11 @@ void igb_ptp_init(struct igb_adapter *adapter)
  **/
 void igb_ptp_stop(struct igb_adapter *adapter)
 {
-	switch (adapter->hw.mac.type) {
-	case e1000_82576:
-	case e1000_82580:
-	case e1000_i354:
-	case e1000_i350:
-		cancel_delayed_work_sync(&adapter->ptp_overflow_work);
-		break;
-	case e1000_i210:
-	case e1000_i211:
-		/* No delayed work to cancel. */
-		break;
-	default:
+	if (!(adapter->ptp_flags & IGB_PTP_ENABLED))
 		return;
-	}
+
+	if (adapter->ptp_flags & IGB_PTP_OVERFLOW_CHECK)
+		cancel_delayed_work_sync(&adapter->ptp_overflow_work);
 
 	cancel_work_sync(&adapter->ptp_tx_work);
 	if (adapter->ptp_tx_skb) {

Signature · 1 line

2.8.1.493.g7b22612

Jacob Keller

author

2016-05-11 23:18 UTC #3 lore raw

PATCH v2 3/5 igb: introduce igb_ptp_resume function

In reply to Jacob Keller

Modify igb_ptp_init to take advantage of igb_ptp_resume, and remove duplicated work that was occurring in both igb_ptp_reset and igb_ptp_init.

In total, resetting the TSAUXC register, and resetting the system time both happen in igb_ptp_reset already, which the new igb_ptp_resume calls. igb_ptp_resume also takes care of starting the delayed work item for overflow checks, as well.

Signed-off-by Jacob Keller <[email protected]>

---
 drivers/net/ethernet/intel/igb/igb.h     |  1 +
 drivers/net/ethernet/intel/igb/igb_ptp.c | 57 ++++++++++++++++----------------
 2 files changed, 29 insertions(+), 29 deletions(-)

drivers/net/ethernet/intel/igb/igb.h +1 -0

index 38daaaba0cdb..fcba5ebba5ab 100644
@@ -550,6 +550,7 @@ void igb_set_fw_version(struct igb_adapter *);
 void igb_ptp_init(struct igb_adapter *adapter);
 void igb_ptp_stop(struct igb_adapter *adapter);
 void igb_ptp_reset(struct igb_adapter *adapter);
+void igb_ptp_resume(struct igb_adapter *adapter);
 void igb_ptp_rx_hang(struct igb_adapter *adapter);
 void igb_ptp_rx_rgtstamp(struct igb_q_vector *q_vector, struct sk_buff *skb);
 void igb_ptp_rx_pktstamp(struct igb_q_vector *q_vector, unsigned char *va,

drivers/net/ethernet/intel/igb/igb_ptp.c +28 ~~-29~~

100 lines not on this page · raw message

Signature · 1 line

2.8.1.493.g7b22612

Jacob Keller

author

2016-05-11 23:18 UTC #4 lore raw

PATCH v2 4/5 igb: implement igb_ptp_suspend

In reply to Jacob Keller

Make igb_ptp_stop take advantage of this new function to reduce code duplication.

Signed-off-by Jacob Keller <[email protected]>

---
 drivers/net/ethernet/intel/igb/igb.h     |  1 +
 drivers/net/ethernet/intel/igb/igb_ptp.c | 22 +++++++++++++++++-----
 2 files changed, 18 insertions(+), 5 deletions(-)

drivers/net/ethernet/intel/igb/igb.h +1 -0

index fcba5ebba5ab..8f3e99cef9c2 100644
@@ -550,6 +550,7 @@ void igb_set_fw_version(struct igb_adapter *);
 void igb_ptp_init(struct igb_adapter *adapter);
 void igb_ptp_stop(struct igb_adapter *adapter);
 void igb_ptp_reset(struct igb_adapter *adapter);
+void igb_ptp_suspend(struct igb_adapter *adapter);
 void igb_ptp_resume(struct igb_adapter *adapter);
 void igb_ptp_rx_hang(struct igb_adapter *adapter);
 void igb_ptp_rx_rgtstamp(struct igb_q_vector *q_vector, struct sk_buff *skb);

drivers/net/ethernet/intel/igb/igb_ptp.c +17 -5

index 6c7d05d35894..a54c512deb45 100644
@@ -1162,12 +1162,13 @@ void igb_ptp_init(struct igb_adapter *adapter)
 }
 
 /**
- * igb_ptp_stop - Disable PTP device and stop the overflow check.
- * @adapter: Board private structure.
+ * igb_ptp_suspend - Disable PTP work items and prepare for suspend
+ * @adapter: Board private structure
  *
- * This function stops the PTP support and cancels the delayed work.
- **/
-void igb_ptp_stop(struct igb_adapter *adapter)
+ * This function stops the overflow check work and PTP Tx timestamp work, and
+ * will prepare the device for OS suspend.
+ */
+void igb_ptp_suspend(struct igb_adapter *adapter)
 {
 	if (!(adapter->ptp_flags & IGB_PTP_ENABLED))
 		return;

@@ -1181,6 +1182,17 @@ void igb_ptp_stop(struct igb_adapter *adapter)
 		adapter->ptp_tx_skb = NULL;
 		clear_bit_unlock(__IGB_PTP_TX_IN_PROGRESS, &adapter->state);
 	}
+}
+
+/**
+ * igb_ptp_stop - Disable PTP device and stop the overflow check.
+ * @adapter: Board private structure.
+ *
+ * This function stops the PTP support and cancels the delayed work.
+ **/
+void igb_ptp_stop(struct igb_adapter *adapter)
+{
+	igb_ptp_suspend(adapter);
 
 	if (adapter->ptp_clock) {
 		ptp_clock_unregister(adapter->ptp_clock);

Signature · 1 line

2.8.1.493.g7b22612

Jacob Keller

author

2016-05-11 23:18 UTC #5 lore raw

PATCH v2 5/5 igb: call igb_ptp_suspend/igb_ptp_resume during suspend/resume cycle

In reply to Jacob Keller

Properly stop the extra workqueue items and ensure that we resume cleanly. This is better than using igb_ptp_init and igb_ptp_stop since these functions destroy the PHC device, which will cause other problems if we do so.

Reported-By Vidya Sagar <[email protected]>

Signed-off-by Jacob Keller <[email protected]>

---
 drivers/net/ethernet/intel/igb/igb_main.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

drivers/net/ethernet/intel/igb/igb_main.c +3 -1

index 21727692bef6..16b9eb9b94b5 100644
@@ -7527,6 +7527,8 @@ static int __igb_shutdown(struct pci_dev *pdev, bool *enable_wake,
 	if (netif_running(netdev))
 		__igb_close(netdev, true);
 
+	igb_ptp_suspend(adapter);
+
 	igb_clear_interrupt_scheme(adapter);
 
 #ifdef CONFIG_PM

@@ -7637,7 +7639,7 @@ static int igb_resume(struct device *dev)
 		return -ENOMEM;
 	}
 
-	igb_reset(adapter);
+	igb_ptp_resume(adapter);
 
 	/* let the f/w know that the h/w is now under the control of the
 	 * driver.

Signature · 1 line

2.8.1.493.g7b22612

Brown

2016-05-17 01:57 UTC #6 lore raw

v2 cover igb: fix ptp suspend/resume issue

In reply to Jacob Keller

Quoted 31 lines

From: Intel-wired-lan [mailto:intel-wired-lan-bounces at lists.osuosl.org] On Behalf Of Jacob Keller
Sent: Wednesday, May 11, 2016 4:18 PM
To: Intel Wired LAN <[email protected]>
Cc: Vidya Sagar <[email protected]>
Subject: [Intel-wired-lan] [net v2 0/5] igb: fix ptp suspend/resume issue
This patch series (properly) fixes the issue with igb's workqueue item for overflow check from causing a surprise remove event. To do this, properly suspend the workqueue items in suspend and then resume them again during the resume flow.
The patch series has a few extra steps to reduce code duplication and implement suspend and resume properly, which makes the overall fix a bit more complicated, and thus review is welcome.
A smaller fix would be to implement suspend and resume irrespective of the current igb_ptp_stop and igb_ptp_init but this seems more likely to introduce bugs especially if either function ever changes in the future.
In addition, the ptp_flags variable is added mostly to simplify the work of writing several complex MAC type checks in the ptp code while doing this.
Jacob Keller (5):
igb: introduce ptp_flags variable and use it to replace IGB_FLAG_PTP
igb: introduce IGB_PTP_OVERFLOW_CHECK flag
igb: introduce igb_ptp_resume function
igb: implement igb_ptp_suspend
igb: call igb_ptp_suspend/igb_ptp_resume during suspend/resume cycle

Quoted 5 lines

 drivers/net/ethernet/intel/igb/igb.h      |   8 ++-
 drivers/net/ethernet/intel/igb/igb_main.c |   4 +-
 drivers/net/ethernet/intel/igb/igb_ptp.c  | 110 ++++++++++++++++-------------
-
 3 files changed, 68 insertions(+), 54 deletions(-)

I have not isolated it to the exact patch yet, but one of the patches in this series is causing my systems to lock up with a call trace. I am currently unable to capture the trace in any form other than a bitmap (which I'll send to Jacob but am not attaching here.) The trace is really several splats a few minutes apart. The exact text / procedure calls of the first one seems to vary, but it seems to be in a wakeup routing with "do_page_fault", "? _raw_spin_lock_irq", "? timecounter_read", "? _raw_spin_lock_irqsave", "igb_ptp_gettime_82576" and "igb_ptp_overflow_check" showing up prominently in at least a few instances. Usually it moves to the next trace before I can get a snapshot. The follow on trace is where it usually stops with a RIP:, bunch of hex, stack info and a Call Trace saying "arch_cpu_idle", "default_idle_call", "cpu_startup_entry" and "start_secondary" called out.

Jeff Kirsher

2016-05-17 02:29 UTC #7 lore raw

v2 cover igb: fix ptp suspend/resume issue

Quoted 43 lines · Aaron F

Quoted 1 line
From: Intel-wired-lan [mailto:intel-wired-lan-bounces at lists.osuosl.org]
On
Quoted 5 lines
Behalf Of Jacob Keller
Sent: Wednesday, May 11, 2016 4:18 PM
To: Intel Wired LAN <[email protected]>
Cc: Vidya Sagar <[email protected]>
Subject: [Intel-wired-lan] [net v2 0/5] igb: fix ptp suspend/resume
issue
Quoted 8 lines
?
This patch series (properly) fixes the issue with igb's workqueue item for overflow check from causing a surprise remove event. To do this, properly suspend the workqueue items in suspend and then resume them again during the resume flow.
?
The patch series has a few extra steps to reduce code duplication and implement suspend and resume properly, which makes the overall fix a
bit
Quoted 5 lines
more complicated, and thus review is welcome.
?
A smaller fix would be to implement suspend and resume irrespective of the current igb_ptp_stop and igb_ptp_init but this seems more likely to introduce bugs especially if either function ever changes in the
future.
Quoted 2 lines
?
In addition, the ptp_flags variable is added mostly to simplify the
work
Quoted 10 lines
of writing several complex MAC type checks in the ptp code while doing this.
?
Jacob Keller (5):
?? igb: introduce ptp_flags variable and use it to replace IGB_FLAG_PTP ?? igb: introduce IGB_PTP_OVERFLOW_CHECK flag
?? igb: introduce igb_ptp_resume function
?? igb: implement igb_ptp_suspend
?? igb: call igb_ptp_suspend/igb_ptp_resume during suspend/resume cycle ?
Quoted 3 lines
? drivers/net/ethernet/intel/igb/igb.h????? |?? 8 ++-
? drivers/net/ethernet/intel/igb/igb_main.c |?? 4 +-
? drivers/net/ethernet/intel/igb/igb_ptp.c? | 110 ++++++++++++++++----
---------
Quoted 2 lines
-
? 3 files changed, 68 insertions(+), 54 deletions(-)

Quoted 14 lines

I have not isolated it to the exact patch yet, but one of the patches in this series is causing my systems to lock up with a call trace.? I am currently unable to capture the trace in any form other than a bitmap (which I'll send to Jacob but am not attaching here.)? The trace is really several splats a few minutes apart.? The exact text / procedure calls of the first one seems to vary, but it seems to be in a wakeup routing with "do_page_fault", "? _raw_spin_lock_irq", "? timecounter_read", "? _raw_spin_lock_irqsave", "igb_ptp_gettime_82576" and "igb_ptp_overflow_check" showing up prominently in at least a few instances.? Usually it moves to the next trace before I can get a snapshot.? The follow on trace is where it usually stops with a RIP:, bunch of hex, stack info and a Call Trace saying "arch_cpu_idle", "default_idle_call", "cpu_startup_entry" and "start_secondary" called out.

Andrew thought it was with patch 3 in the series, at least that is what his initial git bisect was telling him.

I am going to go ahead and drop the entire series for now, so that we can
work offline to resolve the issue.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: This is a digitally signed message part
URL: <http://lists.osuosl.org/pipermail/intel-wired-lan/attachments/20160516/17129f83/attachment.asc>

Keller

2016-05-17 20:46 UTC #8 lore raw

v2 cover igb: fix ptp suspend/resume issue

Quoted 88 lines · Jeff Kirsher

Quoted 79 lines · Aaron F
Quoted 4 lines
From: Intel-wired-lan [mailto:intel-wired-lan-bounces at lists.osuos l.org]
On
Quoted 7 lines
Behalf Of Jacob Keller
Sent: Wednesday, May 11, 2016 4:18 PM
To: Intel Wired LAN <[email protected]>
Cc: Vidya Sagar <[email protected]>
Subject: [Intel-wired-lan] [net v2 0/5] igb: fix ptp suspend/resume
issue
Quoted 14 lines
?
This patch series (properly) fixes the issue with igb's workqueue item
for overflow check from causing a surprise remove event. To do this,
properly suspend the workqueue items in suspend and then resume them
again during the resume flow.
?
The patch series has a few extra steps to reduce code duplication and
implement suspend and resume properly, which makes the overall fix a
bit
Quoted 8 lines
more complicated, and thus review is welcome.
?
A smaller fix would be to implement suspend and resume irrespective of
the current igb_ptp_stop and igb_ptp_init but this seems more likely to
introduce bugs especially if either function ever changes in the
future.
Quoted 4 lines
?
In addition, the ptp_flags variable is added mostly to simplify the
work
Quoted 14 lines
of writing several complex MAC type checks in the ptp code while doing
this.
?
Jacob Keller (5):
?? igb: introduce ptp_flags variable and use it to replace IGB_FLAG_PTP
?? igb: introduce IGB_PTP_OVERFLOW_CHECK flag
?? igb: introduce igb_ptp_resume function
?? igb: implement igb_ptp_suspend
?? igb: call igb_ptp_suspend/igb_ptp_resume during suspend/resume cycle
?
Quoted 4 lines
? drivers/net/ethernet/intel/igb/igb.h????? |?? 8 ++-
? drivers/net/ethernet/intel/igb/igb_main.c |?? 4 +-
? drivers/net/ethernet/intel/igb/igb_ptp.c? | 110
++++++++++++++++----
---------
Quoted 3 lines
-
? 3 files changed, 68 insertions(+), 54 deletions(-)
I have not isolated it to the exact patch yet, but one of the patches in
this series is causing my systems to lock up with a call trace.? I am
currently unable to capture the trace in any form other than a bitmap
(which I'll send to Jacob but am not attaching here.)? The trace is really several splats a few minutes apart.? The exact text / procedure
calls of the first one seems to vary, but it seems to be in a wakeup
routing with "do_page_fault", "? _raw_spin_lock_irq", "? timecounter_read", "? _raw_spin_lock_irqsave", "igb_ptp_gettime_82576"
and "igb_ptp_overflow_check" showing up prominently in at least a
Quoted 8 lines
few
instances.? Usually it moves to the next trace before I can get a snapshot.? The follow on trace is where it usually stops with a RIP:,
bunch of hex, stack info and a Call Trace saying "arch_cpu_idle", "default_idle_call", "cpu_startup_entry" and "start_secondary" called
out.

Quoted 7 lines

Andrew thought it was with patch 3 in the series, at least that is what his
initial git bisect was telling him.
I am going to go ahead and drop the entire series for now, so that we can
work offline to resolve the issue.

Yep. I'm investigating the traces. I'll focus my initial efforts at patch 3.

Thanks,
Jake

Brown

2016-05-17 21:05 UTC #9 lore raw

v2 cover igb: fix ptp suspend/resume issue

Quoted 100 lines

From: Keller, Jacob E
Sent: Tuesday, May 17, 2016 1:47 PM
To: Kirsher, Jeffrey T <[email protected]>; Brown, Aaron F <[email protected]>; intel-wired-lan at lists.osuosl.org
Cc: sagar.tv at gmail.com
Subject: Re: [Intel-wired-lan] [net v2 0/5] igb: fix ptp suspend/resume issue
Quoted 92 lines · Jeff Kirsher
Quoted 79 lines · Aaron F
Quoted 4 lines
From: Intel-wired-lan [mailto:intel-wired-lan-bounces at lists.osuos l.org]
On
Quoted 7 lines
Behalf Of Jacob Keller
Sent: Wednesday, May 11, 2016 4:18 PM
To: Intel Wired LAN <[email protected]>
Cc: Vidya Sagar <[email protected]>
Subject: [Intel-wired-lan] [net v2 0/5] igb: fix ptp suspend/resume
issue
Quoted 10 lines
This patch series (properly) fixes the issue with igb's workqueue item
for overflow check from causing a surprise remove event. To do this,
properly suspend the workqueue items in suspend and then resume them
again during the resume flow.
Quoted 4 lines
The patch series has a few extra steps to reduce code duplication and
implement suspend and resume properly, which makes the overall fix a
bit
Quoted 8 lines
more complicated, and thus review is welcome.
A smaller fix would be to implement suspend and resume irrespective of
the current igb_ptp_stop and igb_ptp_init but this seems more likely to
introduce bugs especially if either function ever changes in the
future.
Quoted 4 lines
In addition, the ptp_flags variable is added mostly to simplify the
work
Quoted 14 lines
of writing several complex MAC type checks in the ptp code while doing
this.
Jacob Keller (5):
?? igb: introduce ptp_flags variable and use it to replace IGB_FLAG_PTP
?? igb: introduce IGB_PTP_OVERFLOW_CHECK flag
?? igb: introduce igb_ptp_resume function
?? igb: implement igb_ptp_suspend
?? igb: call igb_ptp_suspend/igb_ptp_resume during suspend/resume cycle
Quoted 4 lines
? drivers/net/ethernet/intel/igb/igb.h????? |?? 8 ++-
? drivers/net/ethernet/intel/igb/igb_main.c |?? 4 +-
? drivers/net/ethernet/intel/igb/igb_ptp.c? | 110
++++++++++++++++----
---------
Quoted 3 lines
-
? 3 files changed, 68 insertions(+), 54 deletions(-)
I have not isolated it to the exact patch yet, but one of the patches in
this series is causing my systems to lock up with a call trace.? I am
currently unable to capture the trace in any form other than a bitmap
(which I'll send to Jacob but am not attaching here.)? The trace is really several splats a few minutes apart.? The exact text / procedure
calls of the first one seems to vary, but it seems to be in a wakeup
routing with "do_page_fault", "? _raw_spin_lock_irq", "? timecounter_read", "? _raw_spin_lock_irqsave", "igb_ptp_gettime_82576"
and "igb_ptp_overflow_check" showing up prominently in at least a
Quoted 8 lines
few
instances.? Usually it moves to the next trace before I can get a snapshot.? The follow on trace is where it usually stops with a RIP:,
bunch of hex, stack info and a Call Trace saying "arch_cpu_idle", "default_idle_call", "cpu_startup_entry" and "start_secondary" called
out.
Andrew thought it was with patch 3 in the series, at least that is what his
initial git bisect was telling him.

Quoted 7 lines

Quoted 3 lines
I am going to go ahead and drop the entire series for now, so that we can
work offline to resolve the issue.
Yep. I'm investigating the traces. I'll focus my initial efforts at patch 3.

I can confirm Andrew's bisection and that the issue does not start occurring until patch 3 is included.

Quoted 3 lines

Thanks,
Jake

Keller

2016-05-17 21:54 UTC #10 lore raw

v2 cover igb: fix ptp suspend/resume issue

Quoted 133 lines · Aaron F

Quoted 133 lines
From: Keller, Jacob E
Sent: Tuesday, May 17, 2016 1:47 PM
To: Kirsher, Jeffrey T <[email protected]>; Brown, Aaron F
<[email protected]>; intel-wired-lan at lists.osuosl.org
Cc: sagar.tv at gmail.com
Subject: Re: [Intel-wired-lan] [net v2 0/5] igb: fix ptp suspend/resume issue
Quoted 121 lines · Jeff Kirsher
Quoted 102 lines · Aaron F
Quoted 7 lines
From: Intel-wired-lan [mailto:intel-wired-lan-bounces at lists.o suos
l.org]
On
Quoted 8 lines
Behalf Of Jacob Keller
Sent: Wednesday, May 11, 2016 4:18 PM
To: Intel Wired LAN <[email protected]>
Cc: Vidya Sagar <[email protected]>
Subject: [Intel-wired-lan] [net v2 0/5] igb: fix ptp suspend/resume
issue
Quoted 14 lines
This patch series (properly) fixes the issue with igb's workqueue
item
for overflow check from causing a surprise remove event. To do
this,
properly suspend the workqueue items in suspend and then resume
them
again during the resume flow.
Quoted 6 lines
The patch series has a few extra steps to reduce code duplication
and
implement suspend and resume properly, which makes the overall
fix a
bit
Quoted 10 lines
more complicated, and thus review is welcome.
A smaller fix would be to implement suspend and resume irrespective of
the current igb_ptp_stop and igb_ptp_init but this seems more likely to
introduce bugs especially if either function ever changes in the
future.
Quoted 6 lines
In addition, the ptp_flags variable is added mostly to simplify
the
work
Quoted 17 lines
of writing several complex MAC type checks in the ptp code while
doing
this.
Jacob Keller (5):
?? igb: introduce ptp_flags variable and use it to replace IGB_FLAG_PTP
?? igb: introduce IGB_PTP_OVERFLOW_CHECK flag
?? igb: introduce igb_ptp_resume function
?? igb: implement igb_ptp_suspend
?? igb: call igb_ptp_suspend/igb_ptp_resume during suspend/resume
cycle
Quoted 4 lines
? drivers/net/ethernet/intel/igb/igb.h????? |?? 8 ++-
? drivers/net/ethernet/intel/igb/igb_main.c |?? 4 +-
? drivers/net/ethernet/intel/igb/igb_ptp.c? | 110
++++++++++++++++----
---------
Quoted 4 lines
-
? 3 files changed, 68 insertions(+), 54 deletions(-)
I have not isolated it to the exact patch yet, but one of the patches in
this series is causing my systems to lock up with a call trace.? I
am
currently unable to capture the trace in any form other than a bitmap
(which I'll send to Jacob but am not attaching here.)? The trace is
really several splats a few minutes apart.? The exact text / procedure
calls of the first one seems to vary, but it seems to be in a wakeup
routing with "do_page_fault", "? _raw_spin_lock_irq", "? timecounter_read", "? _raw_spin_lock_irqsave", "igb_ptp_gettime_82576"
and "igb_ptp_overflow_check" showing up prominently in at least a
few
instances.? Usually it moves to the next trace before I can get
Quoted 8 lines
a
snapshot.? The follow on trace is where it usually stops with a RIP:,
bunch of hex, stack info and a Call Trace saying "arch_cpu_idle",
"default_idle_call", "cpu_startup_entry" and "start_secondary" called
out.
Andrew thought it was with patch 3 in the series, at least that is
what his
initial git bisect was telling him.
I am going to go ahead and drop the entire series for now, so that we
can
work offline to resolve the issue.

Quoted 4 lines

Quoted 2 lines
Yep. I'm investigating the traces. I'll focus my initial efforts at patch 3.
I can confirm Andrew's bisection and that the issue does not start occurring until patch 3 is included.

Does this just occur from loading? or does it occur after some action on your part?

Thanks,
Jake

Brown

2016-05-17 22:01 UTC #11 lore raw

v2 cover igb: fix ptp suspend/resume issue

Quoted 141 lines

From: Keller, Jacob E
Sent: Tuesday, May 17, 2016 2:54 PM
To: Kirsher, Jeffrey T <[email protected]>; Brown, Aaron F <[email protected]>; intel-wired-lan at lists.osuosl.org
Cc: sagar.tv at gmail.com
Subject: Re: [Intel-wired-lan] [net v2 0/5] igb: fix ptp suspend/resume issue
Quoted 133 lines · Aaron F
Quoted 133 lines
From: Keller, Jacob E
Sent: Tuesday, May 17, 2016 1:47 PM
To: Kirsher, Jeffrey T <[email protected]>; Brown, Aaron F
<[email protected]>; intel-wired-lan at lists.osuosl.org
Cc: sagar.tv at gmail.com
Subject: Re: [Intel-wired-lan] [net v2 0/5] igb: fix ptp suspend/resume issue
Quoted 121 lines · Jeff Kirsher
Quoted 102 lines · Aaron F
Quoted 7 lines
From: Intel-wired-lan [mailto:intel-wired-lan-bounces at lists.o suos
l.org]
On
Quoted 8 lines
Behalf Of Jacob Keller
Sent: Wednesday, May 11, 2016 4:18 PM
To: Intel Wired LAN <[email protected]>
Cc: Vidya Sagar <[email protected]>
Subject: [Intel-wired-lan] [net v2 0/5] igb: fix ptp suspend/resume
issue
Quoted 14 lines
This patch series (properly) fixes the issue with igb's workqueue
item
for overflow check from causing a surprise remove event. To do
this,
properly suspend the workqueue items in suspend and then resume
them
again during the resume flow.
Quoted 6 lines
The patch series has a few extra steps to reduce code duplication
and
implement suspend and resume properly, which makes the overall
fix a
bit
Quoted 10 lines
more complicated, and thus review is welcome.
A smaller fix would be to implement suspend and resume irrespective of
the current igb_ptp_stop and igb_ptp_init but this seems more likely to
introduce bugs especially if either function ever changes in the
future.
Quoted 6 lines
In addition, the ptp_flags variable is added mostly to simplify
the
work
Quoted 17 lines
of writing several complex MAC type checks in the ptp code while
doing
this.
Jacob Keller (5):
?? igb: introduce ptp_flags variable and use it to replace IGB_FLAG_PTP
?? igb: introduce IGB_PTP_OVERFLOW_CHECK flag
?? igb: introduce igb_ptp_resume function
?? igb: implement igb_ptp_suspend
?? igb: call igb_ptp_suspend/igb_ptp_resume during suspend/resume
cycle
Quoted 4 lines
? drivers/net/ethernet/intel/igb/igb.h????? |?? 8 ++-
? drivers/net/ethernet/intel/igb/igb_main.c |?? 4 +-
? drivers/net/ethernet/intel/igb/igb_ptp.c? | 110
++++++++++++++++----
---------
Quoted 4 lines
-
? 3 files changed, 68 insertions(+), 54 deletions(-)
I have not isolated it to the exact patch yet, but one of the patches in
this series is causing my systems to lock up with a call trace.? I
am
currently unable to capture the trace in any form other than a bitmap
(which I'll send to Jacob but am not attaching here.)? The trace is
really several splats a few minutes apart.? The exact text / procedure
calls of the first one seems to vary, but it seems to be in a wakeup
routing with "do_page_fault", "? _raw_spin_lock_irq", "? timecounter_read", "? _raw_spin_lock_irqsave", "igb_ptp_gettime_82576"
and "igb_ptp_overflow_check" showing up prominently in at least a
few
instances.? Usually it moves to the next trace before I can get
Quoted 8 lines
a
snapshot.? The follow on trace is where it usually stops with a RIP:,
bunch of hex, stack info and a Call Trace saying "arch_cpu_idle",
"default_idle_call", "cpu_startup_entry" and "start_secondary" called
out.
Andrew thought it was with patch 3 in the series, at least that is
what his
initial git bisect was telling him.
I am going to go ahead and drop the entire series for now, so that we
can
work offline to resolve the issue.

Quoted 8 lines

Quoted 4 lines
Quoted 2 lines
Yep. I'm investigating the traces. I'll focus my initial efforts at patch 3.
I can confirm Andrew's bisection and that the issue does not start occurring until patch 3 is included.
Does this just occur from loading? or does it occur after some action on your part?

I initially ran into it just loading the driver and launching a BAT. But after trying to figure out which section of the BAT was breaking it, I noticed another system I had simply loaded the driver on would lock up after a while as well. I do not know if it shows up sooner or later if the interface is getting traffic across it, but it shows up either way.

Quoted 3 lines

Thanks,
Jake

Keller

2016-05-17 22:04 UTC #12 lore raw

v2 cover igb: fix ptp suspend/resume issue

Quoted 10 lines · Aaron F

Quoted 10 lines
From: Keller, Jacob E
Sent: Tuesday, May 17, 2016 2:54 PM
To: Kirsher, Jeffrey T <[email protected]>; Brown, Aaron F
<[email protected]>; intel-wired-lan at lists.osuosl.org
Cc: sagar.tv at gmail.com
Subject: Re: [Intel-wired-lan] [net v2 0/5] igb: fix ptp suspend/resume issue

Quoted 6 lines

Quoted 6 lines · Aaron F
Quoted 5 lines
Quoted 5 lines
From: Keller, Jacob E

Sent: Tuesday, May 17, 2016 1:47 PM

Quoted 77 lines

Quoted 77 lines
Quoted 77 lines
Quoted 77 lines
To: Kirsher, Jeffrey T <[email protected]>; Brown, Aaron
F
<[email protected]>; intel-wired-lan at lists.osuosl.org
Cc: sagar.tv at gmail.com
Subject: Re: [Intel-wired-lan] [net v2 0/5] igb: fix ptp suspend/resume issue
Quoted 68 lines · Jeff Kirsher
Quoted 65 lines · Aaron F
Quoted 10 lines
From: Intel-wired-lan [mailto:intel-wired-lan-bounces at lis ts.o
suos
l.org]
On
Quoted 9 lines
Behalf Of Jacob Keller
Sent: Wednesday, May 11, 2016 4:18 PM
To: Intel Wired LAN <[email protected]>
Cc: Vidya Sagar <[email protected]>
Subject: [Intel-wired-lan] [net v2 0/5] igb: fix ptp suspend/resume
issue
Quoted 16 lines
This patch series (properly) fixes the issue with igb's workqueue
item
for overflow check from causing a surprise remove event. To
do
this,
properly suspend the workqueue items in suspend and then resume
them
again during the resume flow.
Quoted 6 lines
The patch series has a few extra steps to reduce code duplication
and
implement suspend and resume properly, which makes the overall
fix a
bit
Quoted 5 lines
more complicated, and thus review is welcome.
Quoted 8 lines
A smaller fix would be to implement suspend and resume irrespective of
the current igb_ptp_stop and igb_ptp_init but this seems more
likely to
introduce bugs especially if either function ever changes in
the
future.
Quoted 7 lines
In addition, the ptp_flags variable is added mostly to simplify
the

Quoted 7 lines

Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
work
Quoted 6 lines
of writing several complex MAC type checks in the ptp code

while

Quoted 80 lines

Quoted 73 lines
Quoted 73 lines
Quoted 73 lines
Quoted 66 lines
Quoted 52 lines
Quoted 14 lines
doing
this.
Jacob Keller (5):
?? igb: introduce ptp_flags variable and use it to replace
IGB_FLAG_PTP
?? igb: introduce IGB_PTP_OVERFLOW_CHECK flag ?? igb: introduce igb_ptp_resume function ?? igb: implement igb_ptp_suspend
?? igb: call igb_ptp_suspend/igb_ptp_resume during suspend/resume
cycle
Quoted 4 lines
? drivers/net/ethernet/intel/igb/igb.h????? |?? 8 ++-
? drivers/net/ethernet/intel/igb/igb_main.c |?? 4 +-
? drivers/net/ethernet/intel/igb/igb_ptp.c? | 110
++++++++++++++++----
---------
Quoted 5 lines
-
? 3 files changed, 68 insertions(+), 54 deletions(-)
I have not isolated it to the exact patch yet, but one of the
patches in
this series is causing my systems to lock up with a call trace.? I
am
currently unable to capture the trace in any form other than a
bitmap
(which I'll send to Jacob but am not attaching here.)? The trace is
really several splats a few minutes apart.? The exact text /
procedure
calls of the first one seems to vary, but it seems to be in a
wakeup
routing with "do_page_fault", "? _raw_spin_lock_irq", "? timecounter_read", "? _raw_spin_lock_irqsave", "igb_ptp_gettime_82576"
and "igb_ptp_overflow_check" showing up prominently in at least
a
few
instances.? Usually it moves to the next trace before I can get
a
snapshot.? The follow on trace is where it usually stops
Quoted 8 lines
with a
RIP:,
bunch of hex, stack info and a Call Trace saying
"arch_cpu_idle",
"default_idle_call", "cpu_startup_entry" and
"start_secondary"
called
out.
Andrew thought it was with patch 3 in the series, at least that
is
what his
initial git bisect was telling him.
Quoted 4 lines
I am going to go ahead and drop the entire series for now, so that we
can
work offline to resolve the issue.
Yep. I'm investigating the traces. I'll focus my initial efforts at
patch 3.
Quoted 7 lines
Quoted 3 lines
I can confirm Andrew's bisection and that the issue does not start
occurring until patch 3 is included.
Does this just occur from loading? or does it occur after some action
on your part?

Quoted 6 lines

I initially ran into it just loading the driver and launching a BAT.??But after trying to figure out which section of the BAT was breaking it, I noticed another system I had simply loaded the driver on would lock up after a while as well.??I do not know if it shows up sooner or later if the interface is getting traffic across it, but it shows up either way.

Ok, so you're not doing suspend resume, and it's just loading the driver and waiting.

Thanks,
Jake