Skip to contentSkip to conversation

bnx2x: fixes

PATCH Superseded Dmitry Kravkov 3 participants 10 messages Aug 5, 2013 – Aug 12, 2013

Subsystems (3)
Networking Network drivers Broadcom BNX2X 10 gigabit Ethernet driver

v4 of 5 Superseded Latest is v5 →

lore
Share & tools
lore

Dmitry Kravkov

author
#0 lore raw

v4 cover bnx2x: fixes

Hello Dave

Please consider applying the series of bnx2x fixes to net:
* statistics may cause FW assert
* missing fairness configuration in DCB flow
* memory leak in sriov related part
* Illegal PTE access
* Pagefault crash in shutdown flow with cnic
v1->v2
* fixed sparse error pointed by Joe Perches
* added missing signed-off from Sergei Shtylyov
v2->v3
* added missing signed-off from Sergei Shtylyov
* fixed formatting from Sergei Shtylyov
v3->v4
* patch 1/6: fixed declaration order
* patch 2/6 replaced with: protect flows using set_bit constraints

Thanks
Dmitry

author
#1 lore raw

PATCH v4 1/6 bnx2x: properly initialize statistic counters

This prevent second statistics query be sent before first one is complete. This is required since two outstanding queries may cause FW assert.

Signed-off-by Dmitry Kravkov <[email protected]>
Signed-off-by Ariel Elior <[email protected]>
Signed-off-by Eilon Greenstein <[email protected]>
---
 drivers/net/ethernet/broadcom/bnx2x/bnx2x_stats.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)
drivers/net/ethernet/broadcom/bnx2x/bnx2x_stats.c +6 -1
index 98366ab..a22ad61 100644
@@ -1589,12 +1589,17 @@ void bnx2x_memset_stats(struct bnx2x *bp)
 
 void bnx2x_stats_init(struct bnx2x *bp)
 {
-	int /*abs*/port = BP_PORT(bp);
+	struct stats_counter *counters = &bp->fw_stats_data->storm_counters;
 	int mb_idx = BP_FW_MB_IDX(bp);
+	int port = BP_PORT(bp);
 
 	bp->stats_pending = 0;
 	bp->executer_idx = 0;
 	bp->stats_counter = 0;
+	counters->xstats_counter = cpu_to_le16(0xFFFF);
+	counters->tstats_counter = cpu_to_le16(0xFFFF);
+	counters->ustats_counter = cpu_to_le16(0xFFFF);
+	counters->cstats_counter = cpu_to_le16(0xFFFF);
 
 	/* port and func stats for management */
 	if (!BP_NOMCP(bp)) {
Signature · 1 line
1.8.1.4
#2 lore raw

Re: [PATCH net v4 1/6] bnx2x: properly initialize statistic counters

Quoted 20 lines · Dmitry Kravkov

This prevent second statistics query be sent before first one is complete. This is required since two outstanding queries may cause FW assert.

Signed-off-by Dmitry Kravkov <[email protected]>
Signed-off-by Ariel Elior <[email protected]>
Signed-off-by Eilon Greenstein <[email protected]>
---
 drivers/net/ethernet/broadcom/bnx2x/bnx2x_stats.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)
drivers/net/ethernet/broadcom/bnx2x/bnx2x_stats.c +1 -1
index 98366ab..a22ad61 100644
@@ -1589,12 +1589,17 @@ void bnx2x_memset_stats(struct bnx2x *bp)
 
 void bnx2x_stats_init(struct bnx2x *bp)
 {
-	int /*abs*/port = BP_PORT(bp);
+	struct stats_counter *counters = &bp->fw_stats_data->storm_counters;

drivers/net/ethernet/broadcom/bnx2x/bnx2x_stats.c void bnx2x_memset_stats(struct bnx2x *bp) · line 1602

Quoted 10 lines
drivers/net/ethernet/broadcom/bnx2x/bnx2x_stats.c +5 -0
 	int mb_idx = BP_FW_MB_IDX(bp);
+	int port = BP_PORT(bp);
 
 	bp->stats_pending = 0;
 	bp->executer_idx = 0;
 	bp->stats_counter = 0;
+	counters->xstats_counter = cpu_to_le16(0xFFFF);
+	counters->tstats_counter = cpu_to_le16(0xFFFF);
+	counters->ustats_counter = cpu_to_le16(0xFFFF);
+	counters->cstats_counter = cpu_to_le16(0xFFFF);

I think the following code executed a little later becomes redundant:

static void bnx2x_prep_fw_stats_req(struct bnx2x *bp) {
[...]

	/* prepare to the first stats ramrod (will be completed with
	 * the counters equal to zero) - init counters to somethig different.
	 */
	memset(&bp->fw_stats_data->storm_counters, 0xff,
	       sizeof(struct stats_counter));
author
#3 lore raw

RE: [PATCH net v4 1/6] bnx2x: properly initialize statistic counters

Quoted 43 lines

-----Original Message-----
From: Benjamin Poirier [mailto:[email protected]]
Sent: Monday, August 12, 2013 11:08 PM
To: Dmitry Kravkov
Cc: [email protected]; [email protected]; Eilon Greenstein; Ariel Elior
Subject: Re: [PATCH net v4 1/6] bnx2x: properly initialize statistic counters

Quoted 21 lines · Dmitry Kravkov

This prevent second statistics query be sent before first one is complete. This is required since two outstanding queries may cause FW assert.

Signed-off-by Dmitry Kravkov <[email protected]>
Signed-off-by Ariel Elior <[email protected]>
Signed-off-by Eilon Greenstein <[email protected]>
---
 drivers/net/ethernet/broadcom/bnx2x/bnx2x_stats.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)
drivers/net/ethernet/broadcom/bnx2x/bnx2x_stats.c +1 -1
index 98366ab..a22ad61 100644
@@ -1589,12 +1589,17 @@ void bnx2x_memset_stats(struct bnx2x *bp)
 
 void bnx2x_stats_init(struct bnx2x *bp)  {
-	int /*abs*/port = BP_PORT(bp);
+	struct stats_counter *counters = &bp->fw_stats_data-
 storm_counters;

drivers/net/ethernet/broadcom/bnx2x/bnx2x_stats.c void bnx2x_memset_stats(struct bnx2x *bp) · line 1602

Quoted 10 lines
drivers/net/ethernet/broadcom/bnx2x/bnx2x_stats.c +5 -0
 	int mb_idx = BP_FW_MB_IDX(bp);
+	int port = BP_PORT(bp);
 
 	bp->stats_pending = 0;
 	bp->executer_idx = 0;
 	bp->stats_counter = 0;
+	counters->xstats_counter = cpu_to_le16(0xFFFF);
+	counters->tstats_counter = cpu_to_le16(0xFFFF);
+	counters->ustats_counter = cpu_to_le16(0xFFFF);
+	counters->cstats_counter = cpu_to_le16(0xFFFF);

I think the following code executed a little later becomes redundant:

Quoted 6 lines

static void bnx2x_prep_fw_stats_req(struct bnx2x *bp) { [...]

	/* prepare to the first stats ramrod (will be completed with
	 * the counters equal to zero) - init counters to somethig different.
	 */
	memset(&bp->fw_stats_data->storm_counters, 0xff,
	       sizeof(struct stats_counter));

You absolutely right. Thanks!

author
#4 lore raw

PATCH v4 2/6 bnx2x: protect different statistics flows

In reply to Dmitry Kravkov

Add locking to protect different statistics flows from running simultaneously.
This in order to serialize statistics requests sent to FW, otherwise two outstanding queries may cause FW assert.

Signed-off-by Dmitry Kravkov <[email protected]>
Signed-off-by Ariel Elior <[email protected]>
Signed-off-by Eilon Greenstein <[email protected]>
---
 drivers/net/ethernet/broadcom/bnx2x/bnx2x.h       |  3 +
 drivers/net/ethernet/broadcom/bnx2x/bnx2x_stats.c | 88 +++++++++++++++++++----
 2 files changed, 79 insertions(+), 12 deletions(-)
drivers/net/ethernet/broadcom/bnx2x/bnx2x.h +3 -0
index d80e34b..c7ffff3 100644
@@ -1830,6 +1830,9 @@ struct bnx2x {
 
 	int fp_array_size;
 	u32 dump_preset_idx;
+	bool					stats_started;
+	unsigned long				stats_flags;
+#define BNX2X_STATS_UPDATE_BIT			0
 };
 
 /* Tx queues may be less or equal to Rx queues */
drivers/net/ethernet/broadcom/bnx2x/bnx2x_stats.c +76 -12

179 lines not on this page · raw message

Signature · 1 line
1.8.1.4
maintainer
#5 lore raw

Re: [PATCH net v4 2/6] bnx2x: protect different statistics flows

From: "Dmitry Kravkov" <[email protected]>
Date: Tue, 6 Aug 2013 00:35:40 +0300

Quoted 8 lines

Add locking to protect different statistics flows from running simultaneously.
This in order to serialize statistics requests sent to FW, otherwise two outstanding queries may cause FW assert.

Signed-off-by Dmitry Kravkov <[email protected]>
Signed-off-by Ariel Elior <[email protected]>
Signed-off-by Eilon Greenstein <[email protected]>

Please do not reinvent the wheel, use a standard semaphore and down_timeout().

Anytime you feel that you have to implement your own fancy locking primitives, you are doing it wrong.

author
#6 lore raw

PATCH v4 3/6 bnx2x: update fairness parameters following DCB negotiation

In reply to Dmitry Kravkov

ETS can be enabled as a result of DCB negotiation, then fairness must be recalculated after each negotiation.

Signed-off-by Dmitry Kravkov <[email protected]>
Signed-off-by Ariel Elior <[email protected]>
Signed-off-by Eilon Greenstein <[email protected]>
---
 drivers/net/ethernet/broadcom/bnx2x/bnx2x.h      |  2 ++
 drivers/net/ethernet/broadcom/bnx2x/bnx2x_dcb.c  |  4 ++++
 drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c | 30 ++++++++++++++----------
 3 files changed, 24 insertions(+), 12 deletions(-)
drivers/net/ethernet/broadcom/bnx2x/bnx2x.h +2 -0
index c7ffff3..062480a 100644
@@ -2454,4 +2454,6 @@ enum bnx2x_pci_bus_speed {
 	BNX2X_PCI_LINK_SPEED_5000 = 5000,
 	BNX2X_PCI_LINK_SPEED_8000 = 8000
 };
+
+void bnx2x_set_local_cmng(struct bnx2x *bp);
 #endif /* bnx2x.h */
drivers/net/ethernet/broadcom/bnx2x/bnx2x_dcb.c +4 -0
index 0c94df4..f9122f2 100644
@@ -753,6 +753,10 @@ void bnx2x_dcbx_set_params(struct bnx2x *bp, u32 state)
 		bnx2x_pfc_set_pfc(bp);
 
 		bnx2x_dcbx_update_ets_params(bp);
+
+		/* ets may affect cmng configuration: reinit it in hw */
+		bnx2x_set_local_cmng(bp);
+
 		bnx2x_dcbx_resume_hw_tx(bp);
 
 		return;
drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c +18 -12
index e06186c..90a84af 100644
@@ -2476,7 +2476,7 @@ static void bnx2x_cmng_fns_init(struct bnx2x *bp, u8 read_cfg, u8 cmng_type)
 
 	input.port_rate = bp->link_vars.line_speed;
 
-	if (cmng_type == CMNG_FNS_MINMAX) {
+	if (cmng_type == CMNG_FNS_MINMAX && input.port_rate) {
 		int vn;
 
 		/* read mf conf from shmem */

@@ -2533,6 +2533,21 @@ static void storm_memset_cmng(struct bnx2x *bp,
 	}
 }
 
+/* init cmng mode in HW according to local configuration */
+void bnx2x_set_local_cmng(struct bnx2x *bp)
+{
+	int cmng_fns = bnx2x_get_cmng_fns_mode(bp);
+
+	if (cmng_fns != CMNG_FNS_NONE) {
+		bnx2x_cmng_fns_init(bp, false, cmng_fns);
+		storm_memset_cmng(bp, &bp->cmng, BP_PORT(bp));
+	} else {
+		/* rate shaping and fairness are disabled */
+		DP(NETIF_MSG_IFUP,
+		   "single function mode without fairness\n");
+	}
+}
+
 /* This function is called upon link interrupt */
 static void bnx2x_link_attn(struct bnx2x *bp)
 {

@@ -2568,17 +2583,8 @@ static void bnx2x_link_attn(struct bnx2x *bp)
 			bnx2x_stats_handle(bp, STATS_EVENT_LINK_UP);
 	}
 
-	if (bp->link_vars.link_up && bp->link_vars.line_speed) {
-		int cmng_fns = bnx2x_get_cmng_fns_mode(bp);
-
-		if (cmng_fns != CMNG_FNS_NONE) {
-			bnx2x_cmng_fns_init(bp, false, cmng_fns);
-			storm_memset_cmng(bp, &bp->cmng, BP_PORT(bp));
-		} else
-			/* rate shaping and fairness are disabled */
-			DP(NETIF_MSG_IFUP,
-			   "single function mode without fairness\n");
-	}
+	if (bp->link_vars.link_up && bp->link_vars.line_speed)
+		bnx2x_set_local_cmng(bp);
 
 	__bnx2x_link_report(bp);
Signature · 1 line
1.8.1.4
author
#7 lore raw

PATCH v4 4/6 bnx2x: fix memory leak in VF

In reply to Dmitry Kravkov

From: Ariel Elior <[email protected]>

Signed-off-by Ariel Elior <[email protected]>
Signed-off-by Dmitry Kravkov <[email protected]>
Signed-off-by Eilon Greenstein <[email protected]>
---
 drivers/net/ethernet/broadcom/bnx2x/bnx2x_sriov.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
drivers/net/ethernet/broadcom/bnx2x/bnx2x_sriov.c +1 -1
index 95861ef..44104fb 100644
@@ -3463,7 +3463,7 @@ int bnx2x_vf_pci_alloc(struct bnx2x *bp)
 alloc_mem_err:
 	BNX2X_PCI_FREE(bp->vf2pf_mbox, bp->vf2pf_mbox_mapping,
 		       sizeof(struct bnx2x_vf_mbx_msg));
-	BNX2X_PCI_FREE(bp->vf2pf_mbox, bp->vf2pf_mbox_mapping,
+	BNX2X_PCI_FREE(bp->vf2pf_mbox, bp->pf2vf_bulletin_mapping,
 		       sizeof(union pf_vf_bulletin));
 	return -ENOMEM;
 }
Signature · 1 line
1.8.1.4
author
#8 lore raw

PATCH v4 5/6 bnx2x: fix PTE write access error

In reply to Dmitry Kravkov

From: Barak Witkowsky <[email protected]>

PTE write access error might occur in MF_ALLOWED mode when IOMMU is active. The patch adds rmmod HSI indicating to MFW to stop running queries which might trigger this failure.

Signed-off-by Barak Witkowsky <[email protected]>
Signed-off-by Dmitry Kravkov <[email protected]>
Signed-off-by Ariel Elior <[email protected]>
Signed-off-by Eilon Greenstein <[email protected]>
---
 drivers/net/ethernet/broadcom/bnx2x/bnx2x.h      | 1 +
 drivers/net/ethernet/broadcom/bnx2x/bnx2x_hsi.h  | 5 +++++
 drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c | 9 +++++++++
 3 files changed, 15 insertions(+)
drivers/net/ethernet/broadcom/bnx2x/bnx2x.h +1 -0
index 062480a..b82793e 100644
@@ -1502,6 +1502,7 @@ struct bnx2x {
 #define BC_SUPPORTS_DCBX_MSG_NON_PMF	(1 << 21)
 #define IS_VF_FLAG			(1 << 22)
 #define INTERRUPTS_ENABLED_FLAG		(1 << 23)
+#define BC_SUPPORTS_RMMOD_CMD		(1 << 24)
 
 #define BP_NOMCP(bp)			((bp)->flags & NO_MCP_FLAG)
drivers/net/ethernet/broadcom/bnx2x/bnx2x_hsi.h +5 -0
index 5018e52..32767f6 100644
@@ -1300,6 +1300,9 @@ struct drv_func_mb {
 
 	#define DRV_MSG_CODE_EEE_RESULTS_ACK            0xda000000
 
+	#define DRV_MSG_CODE_RMMOD                      0xdb000000
+	#define REQ_BC_VER_4_RMMOD_CMD                  0x0007080f
+
 	#define DRV_MSG_CODE_SET_MF_BW                  0xe0000000
 	#define REQ_BC_VER_4_SET_MF_BW                  0x00060202
 	#define DRV_MSG_CODE_SET_MF_BW_ACK              0xe1000000

@@ -1372,6 +1375,8 @@ struct drv_func_mb {
 
 	#define FW_MSG_CODE_EEE_RESULS_ACK              0xda100000
 
+	#define FW_MSG_CODE_RMMOD_ACK                   0xdb100000
+
 	#define FW_MSG_CODE_SET_MF_BW_SENT              0xe0000000
 	#define FW_MSG_CODE_SET_MF_BW_DONE              0xe1000000
drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c +9 -0
index 90a84af..5c22081 100644
@@ -10368,6 +10368,10 @@ static void bnx2x_get_common_hwinfo(struct bnx2x *bp)
 
 	bp->flags |= (val >= REQ_BC_VER_4_DCBX_ADMIN_MSG_NON_PMF) ?
 			BC_SUPPORTS_DCBX_MSG_NON_PMF : 0;
+
+	bp->flags |= (val >= REQ_BC_VER_4_RMMOD_CMD) ?
+			BC_SUPPORTS_RMMOD_CMD : 0;
+
 	boot_mode = SHMEM_RD(bp,
 			dev_info.port_feature_config[BP_PORT(bp)].mba_config) &
 			PORT_FEATURE_MBA_BOOT_AGENT_TYPE_MASK;

@@ -12823,6 +12827,11 @@ static void __bnx2x_remove(struct pci_dev *pdev,
 	bnx2x_dcbnl_update_applist(bp, true);
 #endif
 
+	if (IS_PF(bp) &&
+	    !BP_NOMCP(bp) &&
+	    (bp->flags & BC_SUPPORTS_RMMOD_CMD))
+		bnx2x_fw_command(bp, DRV_MSG_CODE_RMMOD, 0);
+
 	/* Close the interface - either directly or implicitly */
 	if (remove_netdev) {
 		unregister_netdev(dev);
Signature · 1 line
1.8.1.4
author
#9 lore raw

PATCH v4 6/6 bnx2x: prevent crash in shutdown flow with CNIC

In reply to Dmitry Kravkov

From: Yuval Mintz <[email protected]>

There might be a crash as during shutdown flow CNIC might try to access resources already freed by bnx2x.
Change bnx2x_close() into dev_close() in __bnx2x_remove (shutdown flow) to guarantee CNIC is notified of the device's change of status.

Signed-off-by Yuval Mintz <[email protected]>
Signed-off-by Dmitry Kravkov <[email protected]>
Signed-off-by Ariel Elior <[email protected]>
Signed-off-by Eilon Greenstein <[email protected]>
---
 drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)
drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c +1 -2
index 5c22081..abafb30 100644
@@ -12837,8 +12837,7 @@ static void __bnx2x_remove(struct pci_dev *pdev,
 		unregister_netdev(dev);
 	} else {
 		rtnl_lock();
-		if (netif_running(dev))
-			bnx2x_close(dev);
+		dev_close(dev);
 		rtnl_unlock();
 	}
Signature · 1 line
1.8.1.4