Adapter not recovery from EEH error injection

PATCH Superseded [email protected] 3 participants 7 messages May 28, 2014 – Jun 2, 2014

Subsystems (3)

Networking Network drivers Broadcom BNX2X 10 gigabit Ethernet driver

lore

Share & tools

lore

[email protected]

author

2014-05-28 16:00 UTC #0 lore raw

v2 cover Adapter not recovery from EEH error injection

Updated with Dmitry's comments.

Thanks,
Wendy

[email protected]

author

2014-05-28 16:00 UTC #1 lore raw

PATCH v2 1/2 bnx2x: Adapter not recovery from EEH error injection

When injecting EEH error to bnx2x adapter, adapter couldn't be recovery and caused recursive EEH errors. The patch fixes the issue.

Signed-off-by Wen Xiong <[email protected]>

---
 drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c +2 -2

Index: b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c
===================================================================
@@ -13279,8 +13279,8 @@ static int bnx2x_eeh_nic_unload(struct b
 	netdev_reset_tc(bp->dev);
 
 	del_timer_sync(&bp->timer);
-	cancel_delayed_work(&bp->sp_task);
-	cancel_delayed_work(&bp->period_task);
+	cancel_delayed_work_sync(&bp->sp_task);
+	cancel_delayed_work_sync(&bp->period_task);
 
 	spin_lock_bh(&bp->stats_lock);
 	bp->stats_state = STATS_STATE_DISABLED;

Dmitry Kravkov

Reviewed-by on v2 1/2

2014-05-30 12:33 UTC #2 lore raw

RE: [PATCH V2 1/2] bnx2x: Adapter not recovery from EEH error injection

Quoted 25 lines

-----Original Message-----
From: [email protected] [mailto:netdev- [email protected]] On Behalf Of [email protected]
Sent: Wednesday, May 28, 2014 6:57 PM
To: David Miller
Cc: Ariel Elior; netdev; Wen Xiong
Subject: [PATCH V2 1/2] bnx2x: Adapter not recovery from EEH error injection
When injecting EEH error to bnx2x adapter, adapter couldn't be recovery and caused recursive EEH errors. The patch fixes the issue.
Signed-off-by Wen Xiong <[email protected]>
---
 drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
Index: b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c
===================================================================
--- a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c 2014-05-22
18:42:48.000000000 -0500
+++ b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c 2014-05-22
18:44:36.757765539 -0500

drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c static int bnx2x_eeh_nic_unload(struct b · line 13286

Quoted 10 lines

drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c +2 -2

@@ -13279,8 +13279,8 @@ static int bnx2x_eeh_nic_unload(struct b
      netdev_reset_tc(bp->dev);
 
      del_timer_sync(&bp->timer);
-     cancel_delayed_work(&bp->sp_task);
-     cancel_delayed_work(&bp->period_task);
+     cancel_delayed_work_sync(&bp->sp_task);
+     cancel_delayed_work_sync(&bp->period_task);
 
      spin_lock_bh(&bp->stats_lock);
      bp->stats_state = STATS_STATE_DISABLED;

Thanks.

Reviewed-by Dmitry Kravkov <[email protected]>

Quoted 5 lines

--
--
To unsubscribe from this list: send the line "unsubscribe netdev" in the body of a message to [email protected] More majordomo info at
http://vger.kernel.org/majordomo-info.html

________________________________

This message and any attached documents contain information from QLogic Corporation or its wholly-owned subsidiaries that may be confidential. If you are not the intended recipient, you may not read, copy, distribute, or use this information. If you have received this transmission in error, please notify the sender immediately by reply e-mail and then delete this message.

[email protected]

author

2014-05-28 16:00 UTC #3 lore raw

PATCH v2 2/2 bnx2x: Fix kernel crash and data miscompare after EEH recovery

In reply to [email protected]

A rmb() is required to ensure that the CQE is not read before it is written by the adapter DMA. PCI ordering rules will make sure the other fields are written before the marker at the end of struct eth_fast_path_rx_cqe but without rmb() a weakly ordered processor can process stale data.

Without the barrier we have observed various crashes including bnx2x_tpa_start being called on queues not stopped (resulting in message start of bin not in stop) and NULL pointer exceptions from bnx2x_rx_int.

Signed-off-by Milton Miller <[email protected]>

Signed-off-by Wen Xiong <[email protected]>

---
 drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c |   12 ++++++++++++
 1 file changed, 12 insertions(+)

drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c +12 -0

Index: b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c
===================================================================
@@ -906,6 +906,18 @@ static int bnx2x_rx_int(struct bnx2x_fas
 		bd_prod = RX_BD(bd_prod);
 		bd_cons = RX_BD(bd_cons);
 
+		/* A rmb() is required to ensure that the CQE is not read
+		 * before it is written by the adapter DMA.  PCI ordering
+		 * rules will make sure the other fields are written before
+		 * the marker at the end of struct eth_fast_path_rx_cqe
+		 * but without rmb() a weakly ordered processor can process
+		 * stale data.  Without the barrier TPA state-machine might
+		 * enter inconsistent state and kernel stack might be
+		 * provided with incorrect packet description - these lead
+		 * to various kernel crashed.
+		*/
+		rmb();
+
 		cqe_fp_flags = cqe_fp->type_error_flags;
 		cqe_fp_type = cqe_fp_flags & ETH_FAST_PATH_RX_CQE_TYPE;

Dmitry Kravkov

Acked-by on v2 2/2

2014-05-28 19:23 UTC #4 lore raw

RE: [PATCH V2 2/2] bnx2x: Fix kernel crash and data miscompare after EEH recovery

Quoted 40 lines

-----Original Message-----
From: [email protected] [mailto:netdev- [email protected]] On Behalf Of [email protected]
Sent: Wednesday, May 28, 2014 6:57 PM
To: David Miller
Cc: Ariel Elior; netdev; Milton Miller; Wen Xiong
Subject: [PATCH V2 2/2] bnx2x: Fix kernel crash and data miscompare after EEH recovery
A rmb() is required to ensure that the CQE is not read before it is written by the adapter DMA. PCI ordering rules will make sure the other fields are written before the marker at the end of struct eth_fast_path_rx_cqe but without rmb() a weakly ordered processor can process stale data.
Without the barrier we have observed various crashes including bnx2x_tpa_start being called on queues not stopped (resulting in message start of bin not in stop) and NULL pointer exceptions from bnx2x_rx_int.
Signed-off-by Milton Miller <[email protected]>
Signed-off-by Wen Xiong <[email protected]>
---
 drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c |   12 ++++++++++++
 1 file changed, 12 insertions(+)
Index: b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c
===================================================================
--- a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c 2014-05-23 10:34:21.000000000 -0500
+++ b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c 2014-05-28 10:54:26.627766086 -0500
drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c +5 -0
@@ -906,6 +906,18 @@ static int bnx2x_rx_int(struct bnx2x_fas
              bd_prod = RX_BD(bd_prod);
              bd_cons = RX_BD(bd_cons);
 
+             /* A rmb() is required to ensure that the CQE is not read
+              * before it is written by the adapter DMA.  PCI ordering
+              * rules will make sure the other fields are written before
+              * the marker at the end of struct eth_fast_path_rx_cqe
+              * but without rmb() a weakly ordered processor can process

drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c static int bnx2x_rx_int(struct bnx2x_fas · line 923

Quoted 10 lines

drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c +7 -0

+              * stale data.  Without the barrier TPA state-machine might
+              * enter inconsistent state and kernel stack might be
+              * provided with incorrect packet description - these lead
+              * to various kernel crashed.
+             */
+             rmb();
+
              cqe_fp_flags = cqe_fp->type_error_flags;
              cqe_fp_type = cqe_fp_flags &
 ETH_FAST_PATH_RX_CQE_TYPE;

The subject states 'EEH recovery', but looks like this is not the only case. Other than that:

Acked-by Dmitry Kravkov <[email protected]>

________________________________

[email protected]

author

2014-05-29 14:51 UTC #5 lore raw

Re: [PATCH V2 2/2] bnx2x: Fix kernel crash and data miscompare after EEH recovery

Quoting Dmitry Kravkov <[email protected]>:

Quoted 52 lines

Quoted 40 lines
-----Original Message-----
From: [email protected] [mailto:netdev- [email protected]] On Behalf Of [email protected]
Sent: Wednesday, May 28, 2014 6:57 PM
To: David Miller
Cc: Ariel Elior; netdev; Milton Miller; Wen Xiong
Subject: [PATCH V2 2/2] bnx2x: Fix kernel crash and data miscompare after EEH recovery
A rmb() is required to ensure that the CQE is not read before it is written by the adapter DMA. PCI ordering rules will make sure the other fields are written before the marker at the end of struct eth_fast_path_rx_cqe but without rmb() a weakly ordered processor can process stale data.
Without the barrier we have observed various crashes including bnx2x_tpa_start being called on queues not stopped (resulting in message start of bin not in stop) and NULL pointer exceptions from bnx2x_rx_int.
Signed-off-by Milton Miller <[email protected]>
Signed-off-by Wen Xiong <[email protected]>
---
 drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c |   12 ++++++++++++
 1 file changed, 12 insertions(+)
Index: b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c
===================================================================
--- a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c 2014-05-23 10:34:21.000000000 -0500
+++ b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c 2014-05-28 10:54:26.627766086 -0500
drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c +5 -0
@@ -906,6 +906,18 @@ static int bnx2x_rx_int(struct bnx2x_fas
              bd_prod = RX_BD(bd_prod);
              bd_cons = RX_BD(bd_cons);
 
+             /* A rmb() is required to ensure that the CQE is not read
+              * before it is written by the adapter DMA.  PCI ordering
+              * rules will make sure the other fields are written before
+              * the marker at the end of struct eth_fast_path_rx_cqe
+              * but without rmb() a weakly ordered processor can process
drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c static int bnx2x_rx_int(struct bnx2x_fas · line 923
Quoted 10 lines
drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c +7 -0
+              * stale data.  Without the barrier TPA state-machine might
+              * enter inconsistent state and kernel stack might be
+              * provided with incorrect packet description - these lead
+              * to various kernel crashed.
+             */
+             rmb();
+
              cqe_fp_flags = cqe_fp->type_error_flags;
              cqe_fp_type = cqe_fp_flags &
 ETH_FAST_PATH_RX_CQE_TYPE;
The subject states 'EEH recovery', but looks like this is not the only case. Other than that:

Quoted 3 lines

Acked-by Dmitry Kravkov <[email protected]>

Hi Dmitry,

Can you check if you have any comments for 1/2 patch in this series? If you don't, can you ack the 1/2 patch as well?

Thanks,
Wendy

Quoted 3 lines

________________________________
This message and any attached documents contain information from QLogic Corporation or its wholly-owned subsidiaries that may be confidential. If you are not the intended recipient, you may not read, copy, distribute, or use this information. If you have received this transmission in error, please notify the sender immediately by reply e-mail and then delete this message.

David Miller

maintainer

2014-06-02 02:26 UTC #6 lore raw

Re: [PATCH V2 2/2] bnx2x: Fix kernel crash and data miscompare after EEH recovery

In reply to [email protected]

From: [email protected]
Date: Wed, 28 May 2014 10:56:43 -0500

Quoted 24 lines

A rmb() is required to ensure that the CQE is not read before it is written by the adapter DMA. PCI ordering rules will make sure the other fields are written before the marker at the end of struct eth_fast_path_rx_cqe but without rmb() a weakly ordered processor can process stale data.
Without the barrier we have observed various crashes including bnx2x_tpa_start being called on queues not stopped (resulting in message start of bin not in stop) and NULL pointer exceptions from bnx2x_rx_int.
Signed-off-by Milton Miller <[email protected]>
Signed-off-by Wen Xiong <[email protected]>
---
 drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c |   12 ++++++++++++
 1 file changed, 12 insertions(+)
Index: b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c
===================================================================
drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c +0 -0
@@ -906,6 +906,18 @@ static int bnx2x_rx_int(struct bnx2x_fas
 		bd_prod = RX_BD(bd_prod);
 		bd_cons = RX_BD(bd_cons);
 

drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c static int bnx2x_rx_int(struct bnx2x_fas · line 918

Quoted 10 lines

drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c +10 -0

+		/* A rmb() is required to ensure that the CQE is not read
+		 * before it is written by the adapter DMA.  PCI ordering
+		 * rules will make sure the other fields are written before
+		 * the marker at the end of struct eth_fast_path_rx_cqe
+		 * but without rmb() a weakly ordered processor can process
+		 * stale data.  Without the barrier TPA state-machine might
+		 * enter inconsistent state and kernel stack might be
+		 * provided with incorrect packet description - these lead
+		 * to various kernel crashed.
+		*/

Missing a space before the final "*/", this is not formatted correctly.

Please fix this and resubmit the whole series.