* [PATCH net v5 0/5] bnx2x: fixes
@ 2013-08-12 23:24 Dmitry Kravkov
2013-08-12 23:24 ` [PATCH net v5 1/5] bnx2x: protect different statistics flows Dmitry Kravkov
` (6 more replies)
0 siblings, 7 replies; 16+ messages in thread
From: Dmitry Kravkov @ 2013-08-12 23:24 UTC (permalink / raw)
To: davem, netdev; +Cc: eilong, Dmitry Kravkov
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
v4->v5
* patch 2/6: replace proprietary locking with semaphore
* droped 1/6: since adds redundant code from Benjamin Poirier
Thanks
Dmitry
^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH net v5 1/5] bnx2x: protect different statistics flows
2013-08-12 23:24 [PATCH net v5 0/5] bnx2x: fixes Dmitry Kravkov
@ 2013-08-12 23:24 ` Dmitry Kravkov
2013-08-13 0:12 ` Stephen Hemminger
2013-08-13 17:54 ` Neal Cardwell
2013-08-12 23:25 ` [PATCH net v5 2/5] bnx2x: update fairness parameters following DCB negotiation Dmitry Kravkov
` (5 subsequent siblings)
6 siblings, 2 replies; 16+ messages in thread
From: Dmitry Kravkov @ 2013-08-12 23:24 UTC (permalink / raw)
To: davem, netdev; +Cc: eilong, Dmitry Kravkov, Ariel Elior
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 <dmitry@broadcom.com>
Signed-off-by: Ariel Elior <ariele@broadcom.com>
Signed-off-by: Eilon Greenstein <eilong@broadcom.com>
---
drivers/net/ethernet/broadcom/bnx2x/bnx2x.h | 2 +
drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c | 1 +
drivers/net/ethernet/broadcom/bnx2x/bnx2x_stats.c | 66 ++++++++++++++++++-----
3 files changed, 57 insertions(+), 12 deletions(-)
diff --git a/drivers/net/ethernet/broadcom/bnx2x/bnx2x.h b/drivers/net/ethernet/broadcom/bnx2x/bnx2x.h
index d80e34b..98be67f 100644
--- a/drivers/net/ethernet/broadcom/bnx2x/bnx2x.h
+++ b/drivers/net/ethernet/broadcom/bnx2x/bnx2x.h
@@ -1830,6 +1830,8 @@ struct bnx2x {
int fp_array_size;
u32 dump_preset_idx;
+ bool stats_started;
+ struct semaphore stats_sema;
};
/* Tx queues may be less or equal to Rx queues */
diff --git a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c
index e06186c..6e1e9e7 100644
--- a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c
+++ b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c
@@ -11524,6 +11524,7 @@ static int bnx2x_init_bp(struct bnx2x *bp)
mutex_init(&bp->port.phy_mutex);
mutex_init(&bp->fw_mb_mutex);
spin_lock_init(&bp->stats_lock);
+ sema_init(&bp->stats_sema, 1);
INIT_DELAYED_WORK(&bp->sp_task, bnx2x_sp_task);
INIT_DELAYED_WORK(&bp->sp_rtnl_task, bnx2x_sp_rtnl_task);
diff --git a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_stats.c b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_stats.c
index 98366ab..d63d132 100644
--- a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_stats.c
+++ b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_stats.c
@@ -221,7 +221,8 @@ static int bnx2x_stats_comp(struct bnx2x *bp)
* Statistics service functions
*/
-static void bnx2x_stats_pmf_update(struct bnx2x *bp)
+/* should be called under stats_sema */
+static void __bnx2x_stats_pmf_update(struct bnx2x *bp)
{
struct dmae_command *dmae;
u32 opcode;
@@ -518,7 +519,8 @@ static void bnx2x_func_stats_init(struct bnx2x *bp)
*stats_comp = 0;
}
-static void bnx2x_stats_start(struct bnx2x *bp)
+/* should be called under stats_sema */
+static void __bnx2x_stats_start(struct bnx2x *bp)
{
/* vfs travel through here as part of the statistics FSM, but no action
* is required
@@ -534,13 +536,34 @@ static void bnx2x_stats_start(struct bnx2x *bp)
bnx2x_hw_stats_post(bp);
bnx2x_storm_stats_post(bp);
+
+ bp->stats_started = true;
+}
+
+static void bnx2x_stats_start(struct bnx2x *bp)
+{
+ if (down_timeout(&bp->stats_sema, HZ/10))
+ BNX2X_ERR("Unable to acquire stats lock\n");
+ __bnx2x_stats_start(bp);
+ up(&bp->stats_sema);
}
static void bnx2x_stats_pmf_start(struct bnx2x *bp)
{
+ if (down_timeout(&bp->stats_sema, HZ/10))
+ BNX2X_ERR("Unable to acquire stats lock\n");
bnx2x_stats_comp(bp);
- bnx2x_stats_pmf_update(bp);
- bnx2x_stats_start(bp);
+ __bnx2x_stats_pmf_update(bp);
+ __bnx2x_stats_start(bp);
+ up(&bp->stats_sema);
+}
+
+static void bnx2x_stats_pmf_update(struct bnx2x *bp)
+{
+ if (down_timeout(&bp->stats_sema, HZ/10))
+ BNX2X_ERR("Unable to acquire stats lock\n");
+ __bnx2x_stats_pmf_update(bp);
+ up(&bp->stats_sema);
}
static void bnx2x_stats_restart(struct bnx2x *bp)
@@ -550,8 +573,11 @@ static void bnx2x_stats_restart(struct bnx2x *bp)
*/
if (IS_VF(bp))
return;
+ if (down_timeout(&bp->stats_sema, HZ/10))
+ BNX2X_ERR("Unable to acquire stats lock\n");
bnx2x_stats_comp(bp);
- bnx2x_stats_start(bp);
+ __bnx2x_stats_start(bp);
+ up(&bp->stats_sema);
}
static void bnx2x_bmac_stats_update(struct bnx2x *bp)
@@ -888,9 +914,7 @@ static int bnx2x_storm_stats_validate_counters(struct bnx2x *bp)
/* Make sure we use the value of the counter
* used for sending the last stats ramrod.
*/
- spin_lock_bh(&bp->stats_lock);
cur_stats_counter = bp->stats_counter - 1;
- spin_unlock_bh(&bp->stats_lock);
/* are storm stats valid? */
if (le16_to_cpu(counters->xstats_counter) != cur_stats_counter) {
@@ -1227,12 +1251,18 @@ static void bnx2x_stats_update(struct bnx2x *bp)
{
u32 *stats_comp = bnx2x_sp(bp, stats_comp);
- if (bnx2x_edebug_stats_stopped(bp))
+ /* we run update from timer context, so give up
+ * if somebody is in the middle of transition
+ */
+ if (down_trylock(&bp->stats_sema))
return;
+ if (bnx2x_edebug_stats_stopped(bp) || !bp->stats_started)
+ goto out;
+
if (IS_PF(bp)) {
if (*stats_comp != DMAE_COMP_VAL)
- return;
+ goto out;
if (bp->port.pmf)
bnx2x_hw_stats_update(bp);
@@ -1242,7 +1272,7 @@ static void bnx2x_stats_update(struct bnx2x *bp)
BNX2X_ERR("storm stats were not updated for 3 times\n");
bnx2x_panic();
}
- return;
+ goto out;
}
} else {
/* vf doesn't collect HW statistics, and doesn't get completions
@@ -1256,7 +1286,7 @@ static void bnx2x_stats_update(struct bnx2x *bp)
/* vf is done */
if (IS_VF(bp))
- return;
+ goto out;
if (netif_msg_timer(bp)) {
struct bnx2x_eth_stats *estats = &bp->eth_stats;
@@ -1267,6 +1297,9 @@ static void bnx2x_stats_update(struct bnx2x *bp)
bnx2x_hw_stats_post(bp);
bnx2x_storm_stats_post(bp);
+
+out:
+ up(&bp->stats_sema);
}
static void bnx2x_port_stats_stop(struct bnx2x *bp)
@@ -1332,6 +1365,11 @@ static void bnx2x_stats_stop(struct bnx2x *bp)
{
int update = 0;
+ if (down_timeout(&bp->stats_sema, HZ/10))
+ BNX2X_ERR("Unable to acquire stats lock\n");
+
+ bp->stats_started = false;
+
bnx2x_stats_comp(bp);
if (bp->port.pmf)
@@ -1348,6 +1386,8 @@ static void bnx2x_stats_stop(struct bnx2x *bp)
bnx2x_hw_stats_post(bp);
bnx2x_stats_comp(bp);
}
+
+ up(&bp->stats_sema);
}
static void bnx2x_stats_do_nothing(struct bnx2x *bp)
@@ -1376,15 +1416,17 @@ static const struct {
void bnx2x_stats_handle(struct bnx2x *bp, enum bnx2x_stats_event event)
{
enum bnx2x_stats_state state;
+ void (*action)(struct bnx2x *bp);
if (unlikely(bp->panic))
return;
spin_lock_bh(&bp->stats_lock);
state = bp->stats_state;
bp->stats_state = bnx2x_stats_stm[state][event].next_state;
+ action = bnx2x_stats_stm[state][event].action;
spin_unlock_bh(&bp->stats_lock);
- bnx2x_stats_stm[state][event].action(bp);
+ action(bp);
if ((event != STATS_EVENT_UPDATE) || netif_msg_timer(bp))
DP(BNX2X_MSG_STATS, "state %d -> event %d -> state %d\n",
--
1.8.1.4
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH net v5 2/5] bnx2x: update fairness parameters following DCB negotiation
2013-08-12 23:24 [PATCH net v5 0/5] bnx2x: fixes Dmitry Kravkov
2013-08-12 23:24 ` [PATCH net v5 1/5] bnx2x: protect different statistics flows Dmitry Kravkov
@ 2013-08-12 23:25 ` Dmitry Kravkov
2013-08-12 23:25 ` [PATCH net v5 3/5] bnx2x: fix memory leak in VF Dmitry Kravkov
` (4 subsequent siblings)
6 siblings, 0 replies; 16+ messages in thread
From: Dmitry Kravkov @ 2013-08-12 23:25 UTC (permalink / raw)
To: davem, netdev; +Cc: eilong, Dmitry Kravkov, Ariel Elior
ETS can be enabled as a result of DCB negotiation, then
fairness must be recalculated after each negotiation.
Signed-off-by: Dmitry Kravkov <dmitry@broadcom.com>
Signed-off-by: Ariel Elior <ariele@broadcom.com>
Signed-off-by: Eilon Greenstein <eilong@broadcom.com>
---
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(-)
diff --git a/drivers/net/ethernet/broadcom/bnx2x/bnx2x.h b/drivers/net/ethernet/broadcom/bnx2x/bnx2x.h
index 98be67f..f07a7ff 100644
--- a/drivers/net/ethernet/broadcom/bnx2x/bnx2x.h
+++ b/drivers/net/ethernet/broadcom/bnx2x/bnx2x.h
@@ -2453,4 +2453,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 */
diff --git a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_dcb.c b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_dcb.c
index 0c94df4..f9122f2 100644
--- a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_dcb.c
+++ b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_dcb.c
@@ -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;
diff --git a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c
index 6e1e9e7..78b7195 100644
--- a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c
+++ b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c
@@ -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);
--
1.8.1.4
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH net v5 3/5] bnx2x: fix memory leak in VF
2013-08-12 23:24 [PATCH net v5 0/5] bnx2x: fixes Dmitry Kravkov
2013-08-12 23:24 ` [PATCH net v5 1/5] bnx2x: protect different statistics flows Dmitry Kravkov
2013-08-12 23:25 ` [PATCH net v5 2/5] bnx2x: update fairness parameters following DCB negotiation Dmitry Kravkov
@ 2013-08-12 23:25 ` Dmitry Kravkov
2013-08-12 23:25 ` [PATCH net v5 4/5] bnx2x: fix PTE write access error Dmitry Kravkov
` (3 subsequent siblings)
6 siblings, 0 replies; 16+ messages in thread
From: Dmitry Kravkov @ 2013-08-12 23:25 UTC (permalink / raw)
To: davem, netdev; +Cc: eilong, Ariel Elior, Dmitry Kravkov
From: Ariel Elior <ariele@broadcom.com>
Signed-off-by: Ariel Elior <ariele@broadcom.com>
Signed-off-by: Dmitry Kravkov <dmitry@broadcom.com>
Signed-off-by: Eilon Greenstein <eilong@broadcom.com>
---
drivers/net/ethernet/broadcom/bnx2x/bnx2x_sriov.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_sriov.c b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_sriov.c
index 95861ef..44104fb 100644
--- a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_sriov.c
+++ b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_sriov.c
@@ -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;
}
--
1.8.1.4
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH net v5 4/5] bnx2x: fix PTE write access error
2013-08-12 23:24 [PATCH net v5 0/5] bnx2x: fixes Dmitry Kravkov
` (2 preceding siblings ...)
2013-08-12 23:25 ` [PATCH net v5 3/5] bnx2x: fix memory leak in VF Dmitry Kravkov
@ 2013-08-12 23:25 ` Dmitry Kravkov
2013-08-12 23:25 ` [PATCH net v5 5/5] bnx2x: prevent crash in shutdown flow with CNIC Dmitry Kravkov
` (2 subsequent siblings)
6 siblings, 0 replies; 16+ messages in thread
From: Dmitry Kravkov @ 2013-08-12 23:25 UTC (permalink / raw)
To: davem, netdev; +Cc: eilong, Barak Witkowsky, Dmitry Kravkov, Ariel Elior
From: Barak Witkowsky <barak@broadcom.com>
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 <barak@broadcom.com>
Signed-off-by: Dmitry Kravkov <dmitry@broadcom.com>
Signed-off-by: Ariel Elior <ariele@broadcom.com>
Signed-off-by: Eilon Greenstein <eilong@broadcom.com>
---
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(+)
diff --git a/drivers/net/ethernet/broadcom/bnx2x/bnx2x.h b/drivers/net/ethernet/broadcom/bnx2x/bnx2x.h
index f07a7ff..ce9b387 100644
--- a/drivers/net/ethernet/broadcom/bnx2x/bnx2x.h
+++ b/drivers/net/ethernet/broadcom/bnx2x/bnx2x.h
@@ -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)
diff --git a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_hsi.h b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_hsi.h
index 5018e52..32767f6 100644
--- a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_hsi.h
+++ b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_hsi.h
@@ -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
diff --git a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c
index 78b7195..339c388 100644
--- a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c
+++ b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c
@@ -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;
@@ -12824,6 +12828,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);
--
1.8.1.4
^ permalink raw reply related [flat|nested] 16+ messages in thread
* [PATCH net v5 5/5] bnx2x: prevent crash in shutdown flow with CNIC
2013-08-12 23:24 [PATCH net v5 0/5] bnx2x: fixes Dmitry Kravkov
` (3 preceding siblings ...)
2013-08-12 23:25 ` [PATCH net v5 4/5] bnx2x: fix PTE write access error Dmitry Kravkov
@ 2013-08-12 23:25 ` Dmitry Kravkov
2013-08-13 12:30 ` [PATCH net v5 0/5] bnx2x: fixes Benjamin Poirier
2013-08-13 23:04 ` David Miller
6 siblings, 0 replies; 16+ messages in thread
From: Dmitry Kravkov @ 2013-08-12 23:25 UTC (permalink / raw)
To: davem, netdev; +Cc: eilong, Yuval Mintz, Dmitry Kravkov, Ariel Elior
From: Yuval Mintz <yuvalmin@broadcom.com>
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 <yuvalmin@broadcom.com>
Signed-off-by: Dmitry Kravkov <dmitry@broadcom.com>
Signed-off-by: Ariel Elior <ariele@broadcom.com>
Signed-off-by: Eilon Greenstein <eilong@broadcom.com>
---
drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c
index 339c388..955d6cf 100644
--- a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c
+++ b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c
@@ -12838,8 +12838,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();
}
--
1.8.1.4
^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH net v5 1/5] bnx2x: protect different statistics flows
2013-08-12 23:24 ` [PATCH net v5 1/5] bnx2x: protect different statistics flows Dmitry Kravkov
@ 2013-08-13 0:12 ` Stephen Hemminger
2013-08-13 4:24 ` David Miller
2013-08-13 17:54 ` Neal Cardwell
1 sibling, 1 reply; 16+ messages in thread
From: Stephen Hemminger @ 2013-08-13 0:12 UTC (permalink / raw)
To: Dmitry Kravkov; +Cc: davem, netdev, eilong, Ariel Elior
On Tue, 13 Aug 2013 02:24:59 +0300
"Dmitry Kravkov" <dmitry@broadcom.com> wrote:
> + bool stats_started;
> + struct semaphore stats_sema;
Is there a reason to use a counting semaphore? Do you expect it to
be held across user process boundary? or want count > 1?
Use of semaphores as a locking primitive is discouraged,
instead us a mutex.
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH net v5 1/5] bnx2x: protect different statistics flows
2013-08-13 0:12 ` Stephen Hemminger
@ 2013-08-13 4:24 ` David Miller
2013-08-13 17:57 ` Stephen Hemminger
0 siblings, 1 reply; 16+ messages in thread
From: David Miller @ 2013-08-13 4:24 UTC (permalink / raw)
To: stephen; +Cc: dmitry, netdev, eilong, ariele
From: Stephen Hemminger <stephen@networkplumber.org>
Date: Mon, 12 Aug 2013 17:12:08 -0700
> On Tue, 13 Aug 2013 02:24:59 +0300
> "Dmitry Kravkov" <dmitry@broadcom.com> wrote:
>
>> + bool stats_started;
>> + struct semaphore stats_sema;
>
> Is there a reason to use a counting semaphore? Do you expect it to
> be held across user process boundary? or want count > 1?
>
> Use of semaphores as a locking primitive is discouraged,
> instead us a mutex.
Would you please look at the feedback I gave these guys to the
previous iteration of these changes?
They were using custom locking primitives and semaphores gave
the best approximation to the semantics they were looking for.
Thanks.
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH net v5 0/5] bnx2x: fixes
2013-08-12 23:24 [PATCH net v5 0/5] bnx2x: fixes Dmitry Kravkov
` (4 preceding siblings ...)
2013-08-12 23:25 ` [PATCH net v5 5/5] bnx2x: prevent crash in shutdown flow with CNIC Dmitry Kravkov
@ 2013-08-13 12:30 ` Benjamin Poirier
2013-08-13 12:38 ` Ariel Elior
2013-08-13 23:04 ` David Miller
6 siblings, 1 reply; 16+ messages in thread
From: Benjamin Poirier @ 2013-08-13 12:30 UTC (permalink / raw)
To: Dmitry Kravkov, ariele; +Cc: davem, netdev, eilong
On 2013/08/13 02:24, Dmitry Kravkov wrote:
> 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
> v4->v5
> * patch 2/6: replace proprietary locking with semaphore
> * droped 1/6: since adds redundant code from Benjamin Poirier
I'm confused. Wasn't "[PATCH net v4 1/6] bnx2x: properly initialize
statistic counters" supposed to fix a race condition? According to
earlier communication with Ariel:
In this issue a race condition at driver startup causes a second
statistics query to be sent before the first one completes,
resulting in a firmware assert and a stuck chip. A patch was
sent upstream fixing this:
http://patchwork.ozlabs.org/patch/264810/
^ permalink raw reply [flat|nested] 16+ messages in thread
* RE: [PATCH net v5 0/5] bnx2x: fixes
2013-08-13 12:30 ` [PATCH net v5 0/5] bnx2x: fixes Benjamin Poirier
@ 2013-08-13 12:38 ` Ariel Elior
2013-08-13 14:07 ` Benjamin Poirier
0 siblings, 1 reply; 16+ messages in thread
From: Ariel Elior @ 2013-08-13 12:38 UTC (permalink / raw)
To: Benjamin Poirier, Dmitry Kravkov; +Cc: davem, netdev, Eilon Greenstein
> I'm confused. Wasn't "[PATCH net v4 1/6] bnx2x: properly initialize
> statistic counters" supposed to fix a race condition? According to
> earlier communication with Ariel:
> In this issue a race condition at driver startup causes a second
> statistics query to be sent before the first one completes,
> resulting in a firmware assert and a stuck chip. A patch was
> sent upstream fixing this:
> http://patchwork.ozlabs.org/patch/264810/
As I explicitly mentioned in the communique which you quoted above, the patch was sent but not yet accepted.
This is precisely the upstream process - patches are being sent, reviewed and sometimes rejected and revised.
If you have further questions please address them to me - the technical forum is no place for this kind of discussion.
Thanks,
Ariel
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH net v5 0/5] bnx2x: fixes
2013-08-13 12:38 ` Ariel Elior
@ 2013-08-13 14:07 ` Benjamin Poirier
2013-08-13 14:16 ` Dmitry Kravkov
0 siblings, 1 reply; 16+ messages in thread
From: Benjamin Poirier @ 2013-08-13 14:07 UTC (permalink / raw)
To: Ariel Elior; +Cc: Dmitry Kravkov, davem, netdev, Eilon Greenstein
On 2013/08/13 12:38, Ariel Elior wrote:
> > I'm confused. Wasn't "[PATCH net v4 1/6] bnx2x: properly initialize
> > statistic counters" supposed to fix a race condition? According to
> > earlier communication with Ariel:
> > In this issue a race condition at driver startup causes a second
> > statistics query to be sent before the first one completes,
> > resulting in a firmware assert and a stuck chip. A patch was
> > sent upstream fixing this:
> > http://patchwork.ozlabs.org/patch/264810/
>
> As I explicitly mentioned in the communique which you quoted above, the patch was sent but not yet accepted.
> This is precisely the upstream process - patches are being sent, reviewed and sometimes rejected and revised.
> If you have further questions please address them to me - the technical forum is no place for this kind of discussion.
> Thanks,
> Ariel
>
It's a technical question about a patch which was sent upstream. Where
should the discussion happen if not upstream?
Let me rephrase my question:
I'm confused. I thought that "[PATCH net v4 1/6] bnx2x: properly
initialize statistic counters" was meant to fix a race condition at
driver startup which causes a second statistics query to be sent before
the first one completes, resulting in a firmware assert and a stuck
chip. Am I mistaken and there is no such race condition, or is it
addressed by the other patches in this series?
^ permalink raw reply [flat|nested] 16+ messages in thread
* RE: [PATCH net v5 0/5] bnx2x: fixes
2013-08-13 14:07 ` Benjamin Poirier
@ 2013-08-13 14:16 ` Dmitry Kravkov
2013-08-13 17:44 ` Benjamin Poirier
0 siblings, 1 reply; 16+ messages in thread
From: Dmitry Kravkov @ 2013-08-13 14:16 UTC (permalink / raw)
To: Benjamin Poirier, Ariel Elior; +Cc: davem, netdev, Eilon Greenstein
> -----Original Message-----
> From: netdev-owner@vger.kernel.org [mailto:netdev-
> owner@vger.kernel.org] On Behalf Of Benjamin Poirier
> Sent: Tuesday, August 13, 2013 5:07 PM
> To: Ariel Elior
> Cc: Dmitry Kravkov; davem@davemloft.net; netdev@vger.kernel.org; Eilon
> Greenstein
> Subject: Re: [PATCH net v5 0/5] bnx2x: fixes
>
> On 2013/08/13 12:38, Ariel Elior wrote:
> > > I'm confused. Wasn't "[PATCH net v4 1/6] bnx2x: properly initialize
> > > statistic counters" supposed to fix a race condition? According to
> > > earlier communication with Ariel:
> > > In this issue a race condition at driver startup causes a second
> > > statistics query to be sent before the first one completes,
> > > resulting in a firmware assert and a stuck chip. A patch was
> > > sent upstream fixing this:
> > > http://patchwork.ozlabs.org/patch/264810/
> >
> > As I explicitly mentioned in the communique which you quoted above,
> the patch was sent but not yet accepted.
> > This is precisely the upstream process - patches are being sent, reviewed
> and sometimes rejected and revised.
> > If you have further questions please address them to me - the technical
> forum is no place for this kind of discussion.
> > Thanks,
> > Ariel
> >
>
> It's a technical question about a patch which was sent upstream. Where
> should the discussion happen if not upstream?
>
> Let me rephrase my question:
>
> I'm confused. I thought that "[PATCH net v4 1/6] bnx2x: properly initialize
> statistic counters" was meant to fix a race condition at driver startup which
> causes a second statistics query to be sent before the first one completes,
> resulting in a firmware assert and a stuck chip. Am I mistaken and there is no
> such race condition, or is it addressed by the other patches in this series?
>
There is such condition, but it's not caused by wrongly initialized counted -
the initialization done in a correct way. Patch "[PATCH net v5 1/5] bnx2x: protect
different statistics flows" - prevents from two outstanding queries to be sent
simultaneously.
Thanks
Dmitry
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH net v5 0/5] bnx2x: fixes
2013-08-13 14:16 ` Dmitry Kravkov
@ 2013-08-13 17:44 ` Benjamin Poirier
0 siblings, 0 replies; 16+ messages in thread
From: Benjamin Poirier @ 2013-08-13 17:44 UTC (permalink / raw)
To: Dmitry Kravkov; +Cc: Ariel Elior, davem, netdev, Eilon Greenstein
On 2013/08/13 14:16, Dmitry Kravkov wrote:
[...]
> >
> > Let me rephrase my question:
> >
> > I'm confused. I thought that "[PATCH net v4 1/6] bnx2x: properly initialize
> > statistic counters" was meant to fix a race condition at driver startup which
> > causes a second statistics query to be sent before the first one completes,
> > resulting in a firmware assert and a stuck chip. Am I mistaken and there is no
> > such race condition, or is it addressed by the other patches in this series?
> >
> There is such condition, but it's not caused by wrongly initialized counted -
> the initialization done in a correct way. Patch "[PATCH net v5 1/5] bnx2x: protect
> different statistics flows" - prevents from two outstanding queries to be sent
> simultaneously.
Thank you for the clarification.
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH net v5 1/5] bnx2x: protect different statistics flows
2013-08-12 23:24 ` [PATCH net v5 1/5] bnx2x: protect different statistics flows Dmitry Kravkov
2013-08-13 0:12 ` Stephen Hemminger
@ 2013-08-13 17:54 ` Neal Cardwell
1 sibling, 0 replies; 16+ messages in thread
From: Neal Cardwell @ 2013-08-13 17:54 UTC (permalink / raw)
To: Dmitry Kravkov; +Cc: David Miller, Netdev, eilong, Ariel Elior
On Mon, Aug 12, 2013 at 7:24 PM, Dmitry Kravkov <dmitry@broadcom.com> wrote:
> 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 <dmitry@broadcom.com>
> Signed-off-by: Ariel Elior <ariele@broadcom.com>
> Signed-off-by: Eilon Greenstein <eilong@broadcom.com>
> ---
> drivers/net/ethernet/broadcom/bnx2x/bnx2x.h | 2 +
> drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c | 1 +
> drivers/net/ethernet/broadcom/bnx2x/bnx2x_stats.c | 66 ++++++++++++++++++-----
> 3 files changed, 57 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/net/ethernet/broadcom/bnx2x/bnx2x.h b/drivers/net/ethernet/broadcom/bnx2x/bnx2x.h
> index d80e34b..98be67f 100644
Acked-by: Neal Cardwell <ncardwell@google.com>
Thanks!
neal
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH net v5 1/5] bnx2x: protect different statistics flows
2013-08-13 4:24 ` David Miller
@ 2013-08-13 17:57 ` Stephen Hemminger
0 siblings, 0 replies; 16+ messages in thread
From: Stephen Hemminger @ 2013-08-13 17:57 UTC (permalink / raw)
To: David Miller; +Cc: dmitry, netdev, eilong, ariele
On Mon, 12 Aug 2013 21:24:02 -0700 (PDT)
David Miller <davem@davemloft.net> wrote:
> From: Stephen Hemminger <stephen@networkplumber.org>
> Date: Mon, 12 Aug 2013 17:12:08 -0700
>
> > On Tue, 13 Aug 2013 02:24:59 +0300
> > "Dmitry Kravkov" <dmitry@broadcom.com> wrote:
> >
> >> + bool stats_started;
> >> + struct semaphore stats_sema;
> >
> > Is there a reason to use a counting semaphore? Do you expect it to
> > be held across user process boundary? or want count > 1?
> >
> > Use of semaphores as a locking primitive is discouraged,
> > instead us a mutex.
>
> Would you please look at the feedback I gave these guys to the
> previous iteration of these changes?
>
> They were using custom locking primitives and semaphores gave
> the best approximation to the semantics they were looking for.
Your right in this case sempahore makes sense because it is
used to hold off process while hardware responds.
^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH net v5 0/5] bnx2x: fixes
2013-08-12 23:24 [PATCH net v5 0/5] bnx2x: fixes Dmitry Kravkov
` (5 preceding siblings ...)
2013-08-13 12:30 ` [PATCH net v5 0/5] bnx2x: fixes Benjamin Poirier
@ 2013-08-13 23:04 ` David Miller
6 siblings, 0 replies; 16+ messages in thread
From: David Miller @ 2013-08-13 23:04 UTC (permalink / raw)
To: dmitry; +Cc: netdev, eilong
From: "Dmitry Kravkov" <dmitry@broadcom.com>
Date: Tue, 13 Aug 2013 02:24:58 +0300
> 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
Series applied, thanks.
^ permalink raw reply [flat|nested] 16+ messages in thread
end of thread, other threads:[~2013-08-13 23:04 UTC | newest]
Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2013-08-12 23:24 [PATCH net v5 0/5] bnx2x: fixes Dmitry Kravkov
2013-08-12 23:24 ` [PATCH net v5 1/5] bnx2x: protect different statistics flows Dmitry Kravkov
2013-08-13 0:12 ` Stephen Hemminger
2013-08-13 4:24 ` David Miller
2013-08-13 17:57 ` Stephen Hemminger
2013-08-13 17:54 ` Neal Cardwell
2013-08-12 23:25 ` [PATCH net v5 2/5] bnx2x: update fairness parameters following DCB negotiation Dmitry Kravkov
2013-08-12 23:25 ` [PATCH net v5 3/5] bnx2x: fix memory leak in VF Dmitry Kravkov
2013-08-12 23:25 ` [PATCH net v5 4/5] bnx2x: fix PTE write access error Dmitry Kravkov
2013-08-12 23:25 ` [PATCH net v5 5/5] bnx2x: prevent crash in shutdown flow with CNIC Dmitry Kravkov
2013-08-13 12:30 ` [PATCH net v5 0/5] bnx2x: fixes Benjamin Poirier
2013-08-13 12:38 ` Ariel Elior
2013-08-13 14:07 ` Benjamin Poirier
2013-08-13 14:16 ` Dmitry Kravkov
2013-08-13 17:44 ` Benjamin Poirier
2013-08-13 23:04 ` David Miller
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.