bnx2x: Fix error recovering in switch configuration
PATCH 1/1 bnx2x: Fix error recovering in switch configuration
As the BCM57810 and other I/O adapters are connected through a PCIe switch, the bnx2x driver causes unexpected system hang/crash while handling PCIe switch errors, if its error handler is called after other drivers' handlers.
In this case, after numbers of bnx2x_tx_timout(), the bnx2x_nic_unload() is called, frees up resources and calls bnx2x_napi_disable(). Then when EEH calls its error handler, the bnx2x_io_error_detected() and bnx2x_io_slot_reset() also calling bnx2x_napi_disable() and freeing the resources.
This patch will:
- reduce the numbers of bnx2x_panic_dump() while in bnx2x_tx_timeout(), avoid filling up dmesg buffer.
- use checking new napi_enable flags to prevent calling disable again which causing system hangs.
- cheking if fp->page_pool already freed avoid system crash.
--- drivers/net/ethernet/broadcom/bnx2x/bnx2x.h | 4 ++++ .../net/ethernet/broadcom/bnx2x/bnx2x_cmn.c | 24 ++++++++++++++++++- .../net/ethernet/broadcom/bnx2x/bnx2x_cmn.h | 3 +++ 3 files changed, 30 insertions(+), 1 deletion(-)
drivers/net/ethernet/broadcom/bnx2x/bnx2x.h +4 -0
@@ -1509,6 +1509,10 @@ struct bnx2x {bool cnic_loaded;struct cnic_eth_dev *(*cnic_probe)(struct net_device *); +bool napi_enable; +bool cnic_napi_enable; +int tx_timeout_cnt; +/* Flag that indicates that we can start looking for FCoE L2 queue * completions in the default status block. */
drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c +23 -1
@@ -1860,37 +1860,49 @@ static int bnx2x_setup_irqs(struct bnx2x *bp)static void bnx2x_napi_enable_cnic (struct bnx2x *bp) {int i; +if (bp->cnic_napi_enable) +return ;for_each_rx_queue_cnic (bp, i) {napi_enable (&bnx2x_fp (bp, i, napi)); } + bp->cnic_napi_enable = true; }static void bnx2x_napi_enable (struct bnx2x *bp) {int i; +if (bp->napi_enable) +return ;for_each_eth_queue (bp, i) {napi_enable (&bnx2x_fp (bp, i, napi)); } + bp->napi_enable = true; }static void bnx2x_napi_disable_cnic (struct bnx2x *bp) {int i; +if (!bp->cnic_napi_enable) +return ;for_each_rx_queue_cnic (bp, i) {napi_disable (&bnx2x_fp (bp, i, napi)); } + bp->cnic_napi_enable = false; }static void bnx2x_napi_disable (struct bnx2x *bp) {int i; +if (!bp->napi_enable) +return ;for_each_eth_queue (bp, i) {napi_disable (&bnx2x_fp (bp, i, napi)); } + bp->napi_enable = false; }void bnx2x_netif_start (struct bnx2x *bp)
@@ -2554,6 +2566,7 @@ int bnx2x_load_cnic(struct bnx2x *bp) }/* Add all CNIC NAPI objects */ + bp->cnic_napi_enable = false;bnx2x_add_all_napi_cnic (bp);DP (NETIF_MSG_IFUP ,"cnic napi added\n" );bnx2x_napi_enable_cnic (bp);
@@ -2701,7 +2714,9 @@ int bnx2x_nic_load(struct bnx2x *bp, int load_mode)*/ bnx2x_setup_tc (bp->dev, bp->max_cos); + bp->tx_timeout_cnt =0 ;/* Add all NAPI objects */ + bp->napi_enable = false;bnx2x_add_all_napi (bp);DP (NETIF_MSG_IFUP ,"napi added\n" );bnx2x_napi_enable (bp);
@@ -4982,7 +4997,14 @@ void bnx2x_tx_timeout(struct net_device *dev, unsigned int txqueue)*/ if (!bp->panic)#ifndef BNX2X_STOP_ON_ERROR -bnx2x_panic_dump (bp, false); + { +if (++bp->tx_timeout_cnt >3 ) { +bnx2x_panic_dump (bp, false); + bp->tx_timeout_cnt =0 ; + }else { +netdev_err (bp->dev,"TX timeout %d times\n" , bp->tx_timeout_cnt); + } + }#else bnx2x_panic ();#endif
drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.h +3 -0
@@ -1018,6 +1018,9 @@ static inline void bnx2x_free_rx_sge_range(struct bnx2x *bp,if (fp->mode ==TPA_MODE_DISABLED )return ; +if (!fp->page_pool.page) +return ; +for (i =0 ; i < last; i++)bnx2x_free_rx_sge (bp, fp, i);
Signature · 1 line
2.25.1
Jakub Kicinski
maintainerQuoted 25 lines · On Thu, 25 Aug 2022 20:00:29 +0000 Thinh Tran wrote:
As the BCM57810 and other I/O adapters are connected through a PCIe switch, the bnx2x driver causes unexpected system hang/crash while handling PCIe switch errors, if its error handler is called after other drivers' handlers.
In this case, after numbers of
bnx2x_tx_timout(), thebnx2x_nic_unload()is called, frees up resources and callsbnx2x_napi_disable(). Then when EEH calls its error handler, thebnx2x_io_error_detected()andbnx2x_io_slot_reset()also callingbnx2x_napi_disable()and freeing the resources.This patch will:
- reduce the numbers ofbnx2x_panic_dump()while inbnx2x_tx_timeout(), avoid filling up dmesg buffer.
- use checking newnapi_enableflags to prevent calling disable again which causing system hangs.
- cheking iffp->page_poolalready freed avoid system crash.drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c +0
-0
drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c static int bnx2x_setup_irqs(struct bnx2x *bp) · line 1863
Quoted 4 lines
drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c +1
-0@@ -1860,37 +1860,49 @@ static int bnx2x_setup_irqs(struct bnx2x *bp)static void bnx2x_napi_enable_cnic (struct bnx2x *bp) {int i; +if (bp->cnic_napi_enable)
empty line between variables and code, pls
drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c static int bnx2x_setup_irqs(struct bnx2x *bp) · line 1869
Quoted 6 lines
drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c +2
-0+return ;for_each_rx_queue_cnic (bp, i) {napi_enable (&bnx2x_fp (bp, i, napi)); } + bp->cnic_napi_enable = true;
The concept of not calling napi_enable() / disable() feels a little wrong. It's the state of the driver, not the NAPI that's the problem so perhaps you could a appropriately named bool for that (IDK, maybe nic_stopped) and prevent coming into the NAPI handling functions completely?
Is all other code in the driver on the path in question really idempotent?
Quoted 60 lines
drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c +12
-0}static void bnx2x_napi_enable (struct bnx2x *bp) {int i; +if (bp->napi_enable) +return ;for_each_eth_queue (bp, i) {napi_enable (&bnx2x_fp (bp, i, napi)); } + bp->napi_enable = true; }static void bnx2x_napi_disable_cnic (struct bnx2x *bp) {int i; +if (!bp->cnic_napi_enable) +return ;for_each_rx_queue_cnic (bp, i) {napi_disable (&bnx2x_fp (bp, i, napi)); } + bp->cnic_napi_enable = false; }static void bnx2x_napi_disable (struct bnx2x *bp) {int i; +if (!bp->napi_enable) +return ;for_each_eth_queue (bp, i) {napi_disable (&bnx2x_fp (bp, i, napi)); } + bp->napi_enable = false; }void bnx2x_netif_start (struct bnx2x *bp)@@ -2554,6 +2566,7 @@ int bnx2x_load_cnic(struct bnx2x *bp) }/* Add all CNIC NAPI objects */ + bp->cnic_napi_enable = false;bnx2x_add_all_napi_cnic (bp);DP (NETIF_MSG_IFUP ,"cnic napi added\n" );bnx2x_napi_enable_cnic (bp);@@ -2701,7 +2714,9 @@ int bnx2x_nic_load(struct bnx2x *bp, int load_mode)*/ bnx2x_setup_tc (bp->dev, bp->max_cos); + bp->tx_timeout_cnt =0 ;/* Add all NAPI objects */ + bp->napi_enable = false;bnx2x_add_all_napi (bp);DP (NETIF_MSG_IFUP ,"napi added\n" );bnx2x_napi_enable (bp);@@ -4982,7 +4997,14 @@ void bnx2x_tx_timeout(struct net_device *dev, unsigned int txqueue)*/ if (!bp->panic)
drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c void bnx2x_tx_timeout(struct net_device *dev, unsigned int txqueue) · line 5007
Quoted 10 lines
drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c +8
-1#ifndef BNX2X_STOP_ON_ERROR -bnx2x_panic_dump (bp, false); + { +if (++bp->tx_timeout_cnt >3 ) { +bnx2x_panic_dump (bp, false); + bp->tx_timeout_cnt =0 ; + }else { +netdev_err (bp->dev,"TX timeout %d times\n" , bp->tx_timeout_cnt); + } + }
Please put this code in a helper function so that the oddly looking brackets are not needed.
drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.h static inline void bnx2x_free_rx_sge_range(struct bnx2x *bp, · line 1022
Quoted 13 lines
drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c +0
-0#else bnx2x_panic ();#endif drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.h +2
-0@@ -1018,6 +1018,9 @@ static inline void bnx2x_free_rx_sge_range(struct bnx2x *bp,if (fp->mode ==TPA_MODE_DISABLED )return ; +if (!fp->page_pool.page) +return ;
See, another thing that's not idempotent. Better to bail higher up, in the callee.
Quoted 3 lines
drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.h +0
-0for (i =0 ; i < last; i++)bnx2x_free_rx_sge (bp, fp, i);
Thanks for reviewing it.
Quoted 21 lines · Jakub Kicinski
Quoted 19 lines · On Thu, 25 Aug 2022 20:00:29 +0000 Thinh Tran wrote:
As the BCM57810 and other I/O adapters are connected through a PCIe switch, the bnx2x driver causes unexpected system hang/crash while handling PCIe switch errors, if its error handler is called after other drivers' handlers.
In this case, after numbers of
bnx2x_tx_timout(), thebnx2x_nic_unload()is called, frees up resources and callsbnx2x_napi_disable(). Then when EEH calls its error handler, thebnx2x_io_error_detected()andbnx2x_io_slot_reset()also callingbnx2x_napi_disable()and freeing the resources.This patch will:
- reduce the numbers ofbnx2x_panic_dump()while inbnx2x_tx_timeout(), avoid filling up dmesg buffer.
- use checking newnapi_enableflags to prevent calling disable again which causing system hangs.
- cheking iffp->page_poolalready freed avoid system crash.
Quoted 12 lines
drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c static int bnx2x_setup_irqs(struct bnx2x *bp) · line 1863
Quoted 9 lines
drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c +1
-0@@ -1860,37 +1860,49 @@ static int bnx2x_setup_irqs(struct bnx2x *bp)static void bnx2x_napi_enable_cnic (struct bnx2x *bp) {int i; +if (bp->cnic_napi_enable)empty line between variables and code, pls
Will correct it.
Quoted 13 lines
drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c static int bnx2x_setup_irqs(struct bnx2x *bp) · line 1869
Quoted 6 lines
drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c +2
-0+return ;for_each_rx_queue_cnic (bp, i) {napi_enable (&bnx2x_fp (bp, i, napi)); } + bp->cnic_napi_enable = true;The concept of not calling
napi_enable()/disable()feels a little wrong. It's the state of the driver, not the NAPI that's the problem so perhaps you could a appropriately named bool for that (IDK, maybenic_stopped) and prevent coming into the NAPI handling functions completely?
Quoted 3 lines
Quoted 1 line
Is all other code in the driver on the path in question
really idempotent?
For the states of the driver, it already has bnx2x_netif_start()
and bnx2x_netif_stop() to handle NAPI functions.
But the bnx2x_nic_load() directly calls bnx2x_napi_enable() and _disable() in case of errors, and is being called from various functions while the driver in different states.
I will work with the suggestion.
Quoted 85 lines
Quoted 60 lines
} static void bnx2x_napi_enable(struct bnx2x *bp) { int i;drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c +12
-0+if (bp->napi_enable) +return ;for_each_eth_queue (bp, i) {napi_enable (&bnx2x_fp (bp, i, napi)); } + bp->napi_enable = true; }static void bnx2x_napi_disable_cnic (struct bnx2x *bp) {int i; +if (!bp->cnic_napi_enable) +return ;for_each_rx_queue_cnic (bp, i) {napi_disable (&bnx2x_fp (bp, i, napi)); } + bp->cnic_napi_enable = false; }static void bnx2x_napi_disable (struct bnx2x *bp) {int i; +if (!bp->napi_enable) +return ;for_each_eth_queue (bp, i) {napi_disable (&bnx2x_fp (bp, i, napi)); } + bp->napi_enable = false; }void bnx2x_netif_start (struct bnx2x *bp)@@ -2554,6 +2566,7 @@ int bnx2x_load_cnic(struct bnx2x *bp) }/* Add all CNIC NAPI objects */ + bp->cnic_napi_enable = false;bnx2x_add_all_napi_cnic (bp);DP (NETIF_MSG_IFUP ,"cnic napi added\n" );bnx2x_napi_enable_cnic (bp);@@ -2701,7 +2714,9 @@ int bnx2x_nic_load(struct bnx2x *bp, int load_mode)*/ bnx2x_setup_tc (bp->dev, bp->max_cos); + bp->tx_timeout_cnt =0 ;/* Add all NAPI objects */ + bp->napi_enable = false;bnx2x_add_all_napi (bp);DP (NETIF_MSG_IFUP ,"napi added\n" );bnx2x_napi_enable (bp);@@ -4982,7 +4997,14 @@ void bnx2x_tx_timeout(struct net_device *dev, unsigned int txqueue)*/ if (!bp->panic)drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c void bnx2x_tx_timeout(struct net_device *dev, unsigned int txqueue) · line 5007
Quoted 10 lines
drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c +8
-1#ifndef BNX2X_STOP_ON_ERROR -bnx2x_panic_dump (bp, false); + { +if (++bp->tx_timeout_cnt >3 ) { +bnx2x_panic_dump (bp, false); + bp->tx_timeout_cnt =0 ; + }else { +netdev_err (bp->dev,"TX timeout %d times\n" , bp->tx_timeout_cnt); + } + }Please put this code in a helper function so that the oddly looking brackets are not needed.
Quoted 11 lines
drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c +0
-0#else bnx2x_panic ();#endif drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.h +0
-0@@ -1018,6 +1018,9 @@ static inline void bnx2x_free_rx_sge_range(struct bnx2x *bp,if (fp->mode ==TPA_MODE_DISABLED )return ;
Quoted 6 lines
drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.h static inline void bnx2x_free_rx_sge_range(struct bnx2x *bp, · line 1022
Quoted 2 lines
drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.h +2
-0+if (!fp->page_pool.page) +return ;See, another thing that's not idempotent. Better to bail higher up, in the callee.
Will correct it.
Quoted 4 lines
drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.h static inline void bnx2x_free_rx_sge_range(struct bnx2x *bp, · line 1026
Quoted 3 lines
drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.h +0
-0for (i =0 ; i < last; i++)bnx2x_free_rx_sge (bp, fp, i);
Thinh Tran
PATCH v2 1/1 bnx2x: Fix error recovering in switch configuration
In reply to Jakub KicinskiAs the BCM57810 and other I/O adapters are connected through a PCIe switch, the bnx2x driver causes unexpected system hang/crash while handling PCIe switch errors, if its error handler is called after other drivers' handlers.
In this case, after numbers of bnx2x_tx_timout(), the bnx2x_nic_unload() is called, frees up resources and calls bnx2x_napi_disable(). Then when EEH calls its error handler, the bnx2x_io_error_detected() and bnx2x_io_slot_reset() also calling bnx2x_napi_disable() and freeing the resources.
v2:
- Check the state of the NIC before calling disable nappi and freeing the IRQ
- Prevent recurrence of TX timeout by turning off the carrier, calling netif_carrier_off() in bnx2x_tx_timeout()
- Check and bail out early if fp->page_pool already freed
--- drivers/net/ethernet/broadcom/bnx2x/bnx2x.h | 2 + .../net/ethernet/broadcom/bnx2x/bnx2x_cmn.c | 27 +++++++++---- .../net/ethernet/broadcom/bnx2x/bnx2x_cmn.h | 3 ++ .../net/ethernet/broadcom/bnx2x/bnx2x_main.c | 38 +++++++++---------- .../net/ethernet/broadcom/bnx2x/bnx2x_vfpf.c | 17 +++++---- 5 files changed, 53 insertions(+), 34 deletions(-)
drivers/net/ethernet/broadcom/bnx2x/bnx2x.h +2 -0
@@ -1509,6 +1509,8 @@ struct bnx2x {bool cnic_loaded;struct cnic_eth_dev *(*cnic_probe)(struct net_device *); +bool nic_stopped; +/* Flag that indicates that we can start looking for FCoE L2 queue * completions in the default status block. */
drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c +19 -8
@@ -2705,6 +2705,7 @@ int bnx2x_nic_load(struct bnx2x *bp, int load_mode)bnx2x_add_all_napi (bp);DP (NETIF_MSG_IFUP ,"napi added\n" );bnx2x_napi_enable (bp); + bp->nic_stopped = false;if (IS_PF (bp)) {/* set pf load just before approaching the MCP */
@@ -2950,6 +2951,7 @@ int bnx2x_nic_load(struct bnx2x *bp, int load_mode) load_error1:bnx2x_napi_disable (bp);bnx2x_del_all_napi (bp); + bp->nic_stopped = true;/* clear pf_load status, as it was already set */ if (IS_PF (bp))
@@ -3085,14 +3087,17 @@ int bnx2x_nic_unload(struct bnx2x *bp, int unload_mode, bool keep_link)if (!CHIP_IS_E1x (bp))bnx2x_pf_disable (bp); -/* Disable HW interrupts, NAPI */ -bnx2x_netif_stop (bp,1 ); -/* Delete all NAPI objects */ -bnx2x_del_all_napi (bp); -if (CNIC_LOADED (bp)) -bnx2x_del_all_napi_cnic (bp); -/* Release IRQs */ -bnx2x_free_irq (bp); +if (!bp->nic_stopped) { +/* Disable HW interrupts, NAPI */ +bnx2x_netif_stop (bp,1 ); +/* Delete all NAPI objects */ +bnx2x_del_all_napi (bp); +if (CNIC_LOADED (bp)) +bnx2x_del_all_napi_cnic (bp); +/* Release IRQs */ +bnx2x_free_irq (bp); + bp->nic_stopped = true; + }/* Report UNLOAD_DONE to MCP */ bnx2x_send_unload_done (bp, false);
@@ -4977,6 +4982,12 @@ void bnx2x_tx_timeout(struct net_device *dev, unsigned int txqueue) {struct bnx2x *bp =netdev_priv (dev); +/* Immediately indicate link as down */ + bp->link_vars.link_up =0 ; + bp->force_link_down = true; +netif_carrier_off (dev); +BNX2X_ERR ("Indicating link is down due to Tx-timeout\n" ); +/* We want the information of the dump logged, * but calling bnx2x_panic() would kill all chances of recovery. */
drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.h +3 -0
@@ -1015,6 +1015,9 @@ static inline void bnx2x_free_rx_sge_range(struct bnx2x *bp, {int i; +if (!fp->page_pool.page) +return ; +if (fp->mode ==TPA_MODE_DISABLED )return ;
drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c +19 -19
@@ -9475,15 +9475,18 @@ void bnx2x_chip_cleanup(struct bnx2x *bp, int unload_mode, bool keep_link) } } -/* Disable HW interrupts, NAPI */ -bnx2x_netif_stop (bp,1 ); -/* Delete all NAPI objects */ -bnx2x_del_all_napi (bp); -if (CNIC_LOADED (bp)) -bnx2x_del_all_napi_cnic (bp); +if (!bp->nic_stopped) { +/* Disable HW interrupts, NAPI */ +bnx2x_netif_stop (bp,1 ); +/* Delete all NAPI objects */ +bnx2x_del_all_napi (bp); +if (CNIC_LOADED (bp)) +bnx2x_del_all_napi_cnic (bp); -/* Release IRQs */ -bnx2x_free_irq (bp); +/* Release IRQs */ +bnx2x_free_irq (bp); + bp->nic_stopped = true; + }/* Reset the chip, unless PCI function is offline. If we reach this * point following a PCI error handling, it means device is really
@@ -10274,12 +10277,6 @@ static void bnx2x_sp_rtnl_task(struct work_struct *work) bp->sp_rtnl_state =0 ;smp_mb (); -/* Immediately indicate link as down */ - bp->link_vars.link_up =0 ; - bp->force_link_down = true; -netif_carrier_off (bp->dev); -BNX2X_ERR ("Indicating link is down due to Tx-timeout\n" ); -bnx2x_nic_unload (bp,UNLOAD_NORMAL , true);/* When ret value shows failure of allocation failure, * the nic is rebooted again. If open still fails, a error
@@ -14256,13 +14253,16 @@ static pci_ers_result_t bnx2x_io_slot_reset(struct pci_dev *pdev) }bnx2x_drain_tx_queues (bp);bnx2x_send_unload_req (bp,UNLOAD_RECOVERY ); -bnx2x_netif_stop (bp,1 ); -bnx2x_del_all_napi (bp); +if (!bp->nic_stopped) { +bnx2x_netif_stop (bp,1 ); +bnx2x_del_all_napi (bp); -if (CNIC_LOADED (bp)) -bnx2x_del_all_napi_cnic (bp); +if (CNIC_LOADED (bp)) +bnx2x_del_all_napi_cnic (bp); -bnx2x_free_irq (bp); +bnx2x_free_irq (bp); + bp->nic_stopped = true; + }/* Report UNLOAD_DONE to MCP */ bnx2x_send_unload_done (bp, true);
drivers/net/ethernet/broadcom/bnx2x/bnx2x_vfpf.c +10 -7
@@ -529,13 +529,16 @@ void bnx2x_vfpf_close_vf(struct bnx2x *bp)bnx2x_vfpf_finalize (bp, &req->first_tlv); free_irq: -/* Disable HW interrupts, NAPI */ -bnx2x_netif_stop (bp,0 ); -/* Delete all NAPI objects */ -bnx2x_del_all_napi (bp); - -/* Release IRQs */ -bnx2x_free_irq (bp); +if (!bp->nic_stopped) { +/* Disable HW interrupts, NAPI */ +bnx2x_netif_stop (bp,0 ); +/* Delete all NAPI objects */ +bnx2x_del_all_napi (bp); + +/* Release IRQs */ +bnx2x_free_irq (bp); + bp->nic_stopped = true; + } } static void bnx2x_leading_vfq_init(struct bnx2x *bp, struct bnx2x_virtf *vf,
Signature · 1 line
2.25.1
Manish Chopra
maintainerHello Thinh,
Quoted 46 lines
-----Original Message-----
From: Thinh Tran <[email protected]>
Sent: Saturday, September 17, 2022 1:21 AM
To: [email protected]
Cc: [email protected]; Ariel Elior <[email protected]>; [email protected]; Manish Chopra <[email protected]>; Sudarsana Reddy Kalluru <[email protected]>; [email protected]; [email protected]; Thinh Tran <[email protected]>
Subject: [EXT] [PATCH v2] bnx2x: Fix error recovering in switch configurationExternal Email
----------------------------------------------------------------------
As the BCM57810 and other I/O adapters are connected through a PCIe switch, the bnx2x driver causes unexpected system hang/crash while handling PCIe switch errors, if its error handler is called after other drivers' handlers.In this case, after numbers of
bnx2x_tx_timout(), thebnx2x_nic_unload()is called, frees up resources and callsbnx2x_napi_disable(). Then when EEH calls its error handler, thebnx2x_io_error_detected()andbnx2x_io_slot_reset()also callingbnx2x_napi_disable()and freeing the resources.Signed-off-by Thinh Tran <[email protected]>v2:
- Check the state of the NIC before calling disable nappi and freeing the IRQ
- Prevent recurrence of TX timeout by turning off the carrier, callingnetif_carrier_off()inbnx2x_tx_timeout()
- Check and bail out early iffp->page_poolalready freed--- drivers/net/ethernet/broadcom/bnx2x/bnx2x.h | 2 + .../net/ethernet/broadcom/bnx2x/bnx2x_cmn.c | 27 +++++++++---- .../net/ethernet/broadcom/bnx2x/bnx2x_cmn.h | 3 ++ .../net/ethernet/broadcom/bnx2x/bnx2x_main.c | 38 +++++++++---------- .../net/ethernet/broadcom/bnx2x/bnx2x_vfpf.c | 17 +++++---- 5 files changed, 53 insertions(+), 34 deletions(-)drivers/net/ethernet/broadcom/bnx2x/bnx2x.h +0
-0
drivers/net/ethernet/broadcom/bnx2x/bnx2x.h struct bnx2x { · line 1513
Quoted 5 lines
drivers/net/ethernet/broadcom/bnx2x/bnx2x.h +2
-0@@ -1509,6 +1509,8 @@ struct bnx2x {bool cnic_loaded;struct cnic_eth_dev *(*cnic_probe)(struct net_device *); +bool nic_stopped; +
There is an already 'state' variable which holds the NIC state whether it was opened or closed. Perhaps we could use that easily in bnx2x_io_slot_reset() to prevent multiple NAPI disablement instead of adding a newer one. Please see below for ex -
drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c +8 -5
@@ -14256,13 +14256,16 @@ static pci_ers_result_t bnx2x_io_slot_reset(struct pci_dev *pdev) }bnx2x_drain_tx_queues (bp);bnx2x_send_unload_req (bp,UNLOAD_RECOVERY ); -bnx2x_netif_stop (bp,1 ); -bnx2x_del_all_napi (bp); -if (CNIC_LOADED (bp)) -bnx2x_del_all_napi_cnic (bp); +if (bp->state ==BNX2X_STATE_OPEN ) { +bnx2x_netif_stop (bp,1 ); +bnx2x_del_all_napi (bp); -bnx2x_free_irq (bp); +if (CNIC_LOADED (bp)) +bnx2x_del_all_napi_cnic (bp); + +bnx2x_free_irq (bp); + }/* Report UNLOAD_DONE to MCP */ bnx2x_send_unload_done (bp, true);
Thanks,
Manish
Hi Manish,
Thanks for reviewing the patch.
Quoted 51 lines · Manish Chopra
Hello Thinh,
Quoted 49 lines
-----Original Message-----
From: Thinh Tran <[email protected]>
Sent: Saturday, September 17, 2022 1:21 AM
To: [email protected]
Cc: [email protected]; Ariel Elior <[email protected]>; [email protected]; Manish Chopra <[email protected]>; Sudarsana Reddy Kalluru <[email protected]>; [email protected]; [email protected]; Thinh Tran <[email protected]>
Subject: [EXT] [PATCH v2] bnx2x: Fix error recovering in switch configurationExternal Email
----------------------------------------------------------------------
As the BCM57810 and other I/O adapters are connected through a PCIe switch, the bnx2x driver causes unexpected system hang/crash while handling PCIe switch errors, if its error handler is called after other drivers' handlers.In this case, after numbers of
bnx2x_tx_timout(), thebnx2x_nic_unload()is called, frees up resources and callsbnx2x_napi_disable(). Then when EEH calls its error handler, thebnx2x_io_error_detected()andbnx2x_io_slot_reset()also callingbnx2x_napi_disable()and freeing the resources.Signed-off-by Thinh Tran <[email protected]>v2:
- Check the state of the NIC before calling disable nappi and freeing the IRQ
- Prevent recurrence of TX timeout by turning off the carrier, callingnetif_carrier_off()inbnx2x_tx_timeout()
- Check and bail out early iffp->page_poolalready freed--- drivers/net/ethernet/broadcom/bnx2x/bnx2x.h | 2 + .../net/ethernet/broadcom/bnx2x/bnx2x_cmn.c | 27 +++++++++---- .../net/ethernet/broadcom/bnx2x/bnx2x_cmn.h | 3 ++ .../net/ethernet/broadcom/bnx2x/bnx2x_main.c | 38 +++++++++---------- .../net/ethernet/broadcom/bnx2x/bnx2x_vfpf.c | 17 +++++---- 5 files changed, 53 insertions(+), 34 deletions(-)drivers/net/ethernet/broadcom/bnx2x/bnx2x.h +0
-0@@ -1509,6 +1509,8 @@ struct bnx2x {bool cnic_loaded;struct cnic_eth_dev *(*cnic_probe)(struct net_device *);
Quoted 4 lines
drivers/net/ethernet/broadcom/bnx2x/bnx2x.h struct bnx2x { · line 1513
Quoted 2 lines
drivers/net/ethernet/broadcom/bnx2x/bnx2x.h +2
-0+bool nic_stopped; +There is an already 'state' variable which holds the NIC state whether it was opened or closed. > Perhaps we could use that easily in
bnx2x_io_slot_reset()to prevent
multiple NAPI disablement
Quoted 2 lines
instead of adding a newer one. Please see below for ex -
In my early debug, I did checked for bp->state variable but I was unsure what the NIC state was safe to disable the NAPI. Thanks for the hint.
Quoted 27 lines
drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c +8
-5@@ -14256,13 +14256,16 @@ static pci_ers_result_t bnx2x_io_slot_reset(struct pci_dev *pdev) }bnx2x_drain_tx_queues (bp);bnx2x_send_unload_req (bp,UNLOAD_RECOVERY ); -bnx2x_netif_stop (bp,1 ); -bnx2x_del_all_napi (bp); -if (CNIC_LOADED (bp)) -bnx2x_del_all_napi_cnic (bp); +if (bp->state ==BNX2X_STATE_OPEN ) { +bnx2x_netif_stop (bp,1 ); +bnx2x_del_all_napi (bp); -bnx2x_free_irq (bp); +if (CNIC_LOADED (bp)) +bnx2x_del_all_napi_cnic (bp); + +bnx2x_free_irq (bp); + }/* Report UNLOAD_DONE to MCP */ bnx2x_send_unload_done (bp, true);
Quoted 2 lines
Thanks,
Manish
It requires a specific system setup, I will test with v3 patch when the system is available.
Thanks,
Thinh Tran
Hi Manish,
I think we need the extra variable, nic_stopped, or perhaps defining a different state to determine the disablement of NAPI and freed IRQs. Since device gets the error, the NIC state is set to BNX2X_STATE_CLOSING_WAIT4_HALT (0x4000)
14132 static int bnx2x_eeh_nic_unload(struct bnx2x *bp) 14133 {
14134 bp->state = BNX2X_STATE_CLOSING_WAIT4_HALT;
14135
and stays in this state in EEH handler and driver reset code paths, when both trying to disabling the NAPI.
Quoted 91 lines · Thinh Tran
Hi Manish,
Thanks for reviewing the patch.Quoted 51 lines · Manish Chopra
Hello Thinh,
Quoted 49 lines
-----Original Message-----
From: Thinh Tran <[email protected]>
Sent: Saturday, September 17, 2022 1:21 AM
To: [email protected]
Cc: [email protected]; Ariel Elior <[email protected]>; [email protected]; Manish Chopra <[email protected]>; Sudarsana Reddy Kalluru <[email protected]>; [email protected]; [email protected]; Thinh Tran <[email protected]>
Subject: [EXT] [PATCH v2] bnx2x: Fix error recovering in switch configurationExternal Email
----------------------------------------------------------------------
As the BCM57810 and other I/O adapters are connected through a PCIe switch, the bnx2x driver causes unexpected system hang/crash while handling PCIe switch errors, if its error handler is called after other drivers' handlers.In this case, after numbers of
bnx2x_tx_timout(), thebnx2x_nic_unload()is called, frees up resources and callsbnx2x_napi_disable(). Then when EEH calls its error handler, thebnx2x_io_error_detected()andbnx2x_io_slot_reset()also callingbnx2x_napi_disable()and freeing the resources.Signed-off-by Thinh Tran <[email protected]>v2:
- Check the state of the NIC before calling disable nappi and freeing the IRQ
- Prevent recurrence of TX timeout by turning off the carrier, callingnetif_carrier_off()inbnx2x_tx_timeout()
- Check and bail out early iffp->page_poolalready freed
---drivers/net/ethernet/broadcom/bnx2x/bnx2x.h | 2 + .../net/ethernet/broadcom/bnx2x/bnx2x_cmn.c | 27 +++++++++---- .../net/ethernet/broadcom/bnx2x/bnx2x_cmn.h | 3 ++ .../net/ethernet/broadcom/bnx2x/bnx2x_main.c | 38 +++++++++---------- .../net/ethernet/broadcom/bnx2x/bnx2x_vfpf.c | 17 +++++---- 5 files changed, 53 insertions(+), 34 deletions(-)drivers/net/ethernet/broadcom/bnx2x/bnx2x.h +0
-0@@ -1509,6 +1509,8 @@ struct bnx2x { bool cnic_loaded; struct cnic_eth_dev *(*cnic_probe)(struct net_device *);Quoted 4 lines
drivers/net/ethernet/broadcom/bnx2x/bnx2x.h struct bnx2x { · line 1513
Quoted 2 lines
drivers/net/ethernet/broadcom/bnx2x/bnx2x.h +2
-0+ bool nic_stopped; +There is an already 'state' variable which holds the NIC state whether it was opened or closed. > Perhaps we could use that easily in
bnx2x_io_slot_reset()to preventmultiple NAPI disablement
Quoted 2 lines
instead of adding a newer one. Please see below for ex -
In my early debug, I did checked for
bp->statevariable but I was unsure what the NIC state was safe to disable the NAPI. Thanks for the hint.Quoted 25 lines
drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c +8
-5@@ -14256,13 +14256,16 @@ static pci_ers_result_t bnx2x_io_slot_reset(struct pci_dev *pdev) } bnx2x_drain_tx_queues(bp); bnx2x_send_unload_req(bp, UNLOAD_RECOVERY); - bnx2x_netif_stop(bp, 1); - bnx2x_del_all_napi(bp); - if (CNIC_LOADED(bp)) - bnx2x_del_all_napi_cnic(bp); + if (bp->state == BNX2X_STATE_OPEN) { + bnx2x_netif_stop(bp, 1); + bnx2x_del_all_napi(bp); - bnx2x_free_irq(bp); + if (CNIC_LOADED(bp)) + bnx2x_del_all_napi_cnic(bp); + + bnx2x_free_irq(bp); + }
Quoted 3 lines
Quoted 3 lines
drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c +0
-0/* Report UNLOAD_DONE to MCP */
bnx2x_send_unload_done(bp, true);
Using the v2 patch, I checked the NIC state at places before and after disabling the NAPI,
BNX2X_ERR("before state flag = 0x%x \n", bp->state);
if (!bp->nic_stopped) {
BNX2X_ERR("after state flag = 0x%x \n", bp->state);
bnx2x_netif_stop(bp, 1);
bnx2x_del_all_napi(bp);
....the NIC state is always BNX2X_STATE_CLOSING_WAIT4_HALT (0x4000) when device get error injection.
1- When EEH handler code path was called first, the bnx2x_io_slot_reset() does disablement the NAPI and done.
[28197.100795] bnx2x: [bnx2x_io_slot_reset:14212(eth3)]IO slot reset initializing... [28197.100959] bnx2x 0201:50:00.0: enabling device (0140 -> 0142) [28197.107249] bnx2x: [bnx2x_io_slot_reset:14228(eth3)]IO slot reset --> driver unload [28199.106281] bnx2x: [bnx2x_clean_tx_queue:1205(eth3)]timeout waiting for queue[1]: txdata->tx_pkt_prod(7270) != txdata->tx_pkt_cons(7269) [28201.106281] bnx2x: [bnx2x_clean_tx_queue:1205(eth3)]timeout waiting for queue[3]: txdata->tx_pkt_prod(10977) != txdata->tx_pkt_cons(10969) [28203.106280] bnx2x: [bnx2x_clean_tx_queue:1205(eth3)]timeout waiting for queue[4]: txdata->tx_pkt_prod(9532) != txdata->tx_pkt_cons(9514) [28205.106284] bnx2x: [bnx2x_clean_tx_queue:1205(eth3)]timeout waiting for queue[6]: txdata->tx_pkt_prod(2359) != txdata->tx_pkt_cons(2331) [28205.314280] bnx2x: [bnx2x_io_slot_reset:14246(eth3)]before -- state flag = 0x4000 <---- check NIC state [28205.314282] bnx2x: [bnx2x_io_slot_reset:14248(eth3)]after -- state flag = 0x4000 <---- check NIC state and disable the NAPI [28205.405009] PCI 0201:50:00.0#500000: EEH: bnx2x driver reports: 'recovered' ........ snip ...... [28213.525014] EEH: Finished:'slot_reset' with aggregate recovery state:'recovered' [28213.525016] EEH: Notify device driver to resume [28213.525017] EEH: Beginning: 'resume' [28213.525018] PCI 0201:50:00.0#500000: EEH: Invoking bnx2x->resume() [28225.115287] bnx2x 0201:50:00.0 eth3: using MSI-X IRQs: sp 101 fp[0] 103 ... fp[7] 110 [28225.696425] PCI 0201:50:00.0#500000: EEH: bnx2x driver reports:
'none'
2- When the driver reset code path was called first, the bnx2x_chip_cleanup() does the disablement the NAPI, but NIC is resumed after the EEH handler code path was called.
[13376.774278] bnx2x: [bnx2x_state_wait:312(eth3)]timeout waiting
for state 2
[13376.774280] bnx2x: [bnx2x_func_stop:9118(eth3)]FUNC_STOP ramrod
failed. Running a dry transaction
[13376.774339] bnx2x: [bnx2x_chip_cleanup:9467(eth3)]before state
flag = 0x4000
[13376.774341] bnx2x: [bnx2x_chip_cleanup:9469(eth3)]after state
flag = 0x4000
[13376.774344] bnx2x: [bnx2x_igu_int_disable:891(eth3)]BUG! Proper
val not read from IGU!
[13391.774280] bnx2x: [bnx2x_fw_command:3044(eth3)]FW failed to respond!
........ snip .....
[13442.118385] PCI 0201:50:00.0#500000: EEH: Invoking
bnx2x->slot_reset()
[13442.118388] bnx2x: [bnx2x_io_slot_reset:14212(eth3)]IO slot reset
initializing...
[13442.118509] bnx2x 0201:50:00.0: enabling device (0140 -> 0142)
[13442.124825] bnx2x: [bnx2x_io_slot_reset:14228(eth3)]IO slot reset
--> driver unload
[13442.154290] bnx2x: [bnx2x_io_slot_reset:14246(eth3)]before --
state flag = 0x4000 <--- check NIC state
[13442.274291] PCI 0201:50:00.0#500000: EEH: bnx2x driver reports:
'recovered'
....... snip .........
[13442.424290] EEH: Finished:'slot_reset' with aggregate recovery
state:'recovered'
[13442.424291] EEH: Notify device driver to resume
[13442.424292] EEH: Beginning: 'resume'
[13442.424293] PCI 0201:50:00.0#500000: EEH: Invoking bnx2x->resume()
[13443.015280] bnx2x 0201:50:00.0 eth3: using MSI-X IRQs: sp 61
fp[0] 63 ... fp[7] 75
[13443.616423] PCI 0201:50:00.0#500000: EEH: bnx2x driver reports: 'none'
Quoted 7 lines
Quoted 2 lines
Thanks,
ManishIt requires a specific system setup, I will test with v3 patch when the system is available.
Thanks,
Thinh Tran
Any suggestions?
Thanks,
Thinh Tran
Hi Manish,
Do you have any other suggestion or using the extra variable, nic_stopped, in v2 patch is efficient enough to fix the issue?
Thanks,
Thinh Tran
Quoted 198 lines · Thinh Tran
Hi Manish,
I think we need the extra variable,
nic_stopped, or perhaps defining a different state to determine the disablement of NAPI and freed IRQs. Since device gets the error, the NIC state is set toBNX2X_STATE_CLOSING_WAIT4_HALT(0x4000)14132 static int
bnx2x_eeh_nic_unload(struct bnx2x *bp)14133 {
14134bp->state=BNX2X_STATE_CLOSING_WAIT4_HALT; 14135and stays in this state in EEH handler and driver reset code paths, when both trying to disabling the NAPI.
Quoted 92 lines · Thinh Tran
Hi Manish,
Thanks for reviewing the patch.Quoted 52 lines · Manish Chopra
Hello Thinh,
Quoted 50 lines
-----Original Message-----
From: Thinh Tran <[email protected]>
Sent: Saturday, September 17, 2022 1:21 AM
To: [email protected]
Cc: [email protected]; Ariel Elior <[email protected]>; [email protected]; Manish Chopra <[email protected]>; Sudarsana Reddy Kalluru <[email protected]>; [email protected]; [email protected]; Thinh Tran <[email protected]>
Subject: [EXT] [PATCH v2] bnx2x: Fix error recovering in switch configurationExternal Email
----------------------------------------------------------------------
As the BCM57810 and other I/O adapters are connected through a PCIe switch, the bnx2x driver causes unexpected system hang/crash while handling PCIe switch errors, if its error handler is called after other drivers' handlers.In this case, after numbers of
bnx2x_tx_timout(), thebnx2x_nic_unload()is called, frees up resources and callsbnx2x_napi_disable(). Then when EEH calls its error handler, thebnx2x_io_error_detected()andbnx2x_io_slot_reset()also callingbnx2x_napi_disable()and freeing the resources.Signed-off-by Thinh Tran <[email protected]>v2:
- Check the state of the NIC before calling disable nappi and freeing the IRQ
- Prevent recurrence of TX timeout by turning off the carrier, callingnetif_carrier_off()inbnx2x_tx_timeout()
- Check and bail out early iffp->page_poolalready freed
---drivers/net/ethernet/broadcom/bnx2x/bnx2x.h | 2 + .../net/ethernet/broadcom/bnx2x/bnx2x_cmn.c | 27 +++++++++---- .../net/ethernet/broadcom/bnx2x/bnx2x_cmn.h | 3 ++ .../net/ethernet/broadcom/bnx2x/bnx2x_main.c | 38 +++++++++---------- .../net/ethernet/broadcom/bnx2x/bnx2x_vfpf.c | 17 +++++---- 5 files changed, 53 insertions(+), 34 deletions(-)drivers/net/ethernet/broadcom/bnx2x/bnx2x.h +0
-0@@ -1509,6 +1509,8 @@ struct bnx2x { bool cnic_loaded; struct cnic_eth_dev *(*cnic_probe)(struct net_device *);Quoted 4 lines
drivers/net/ethernet/broadcom/bnx2x/bnx2x.h struct bnx2x { · line 1513
Quoted 2 lines
drivers/net/ethernet/broadcom/bnx2x/bnx2x.h +2
-0+ bool nic_stopped; +There is an already 'state' variable which holds the NIC state whether it was opened or closed. > Perhaps we could use that easily in
bnx2x_io_slot_reset()to preventmultiple NAPI disablement
Quoted 2 lines
instead of adding a newer one. Please see below for ex -
In my early debug, I did checked for
bp->statevariable but I was unsure what the NIC state was safe to disable the NAPI. Thanks for the hint.Quoted 25 lines
drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c +8
-5@@ -14256,13 +14256,16 @@ static pci_ers_result_t bnx2x_io_slot_reset(struct pci_dev *pdev) } bnx2x_drain_tx_queues(bp); bnx2x_send_unload_req(bp, UNLOAD_RECOVERY); - bnx2x_netif_stop(bp, 1); - bnx2x_del_all_napi(bp); - if (CNIC_LOADED(bp)) - bnx2x_del_all_napi_cnic(bp); + if (bp->state == BNX2X_STATE_OPEN) { + bnx2x_netif_stop(bp, 1); + bnx2x_del_all_napi(bp); - bnx2x_free_irq(bp); + if (CNIC_LOADED(bp)) + bnx2x_del_all_napi_cnic(bp); + + bnx2x_free_irq(bp); + }Quoted 3 lines
Quoted 3 lines
drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c +0
-0/* Report UNLOAD_DONE to MCP */
bnx2x_send_unload_done(bp, true);Using the v2 patch, I checked the NIC state at places before and after disabling the NAPI,
BNX2X_ERR("before state flag = 0x%x \n", bp->state);
if (!bp->nic_stopped) {
BNX2X_ERR("after state flag = 0x%x \n", bp->state);bnx2x_netif_stop(bp, 1);
bnx2x_del_all_napi(bp);
....the NIC state is always
BNX2X_STATE_CLOSING_WAIT4_HALT(0x4000) when device get error injection.1- When EEH handler code path was called first, the
bnx2x_io_slot_reset()does disablement the NAPI and done.[28197.100795] bnx2x: [bnx2x_io_slot_reset:14212(eth3)]IO slot reset initializing... [28197.100959] bnx2x 0201:50:00.0: enabling device (0140 -> 0142) [28197.107249] bnx2x: [bnx2x_io_slot_reset:14228(eth3)]IO slot reset --> driver unload [28199.106281] bnx2x: [bnx2x_clean_tx_queue:1205(eth3)]timeout waiting for queue[1]: txdata->tx_pkt_prod(7270) != txdata->tx_pkt_cons(7269) [28201.106281] bnx2x: [bnx2x_clean_tx_queue:1205(eth3)]timeout waiting for queue[3]: txdata->tx_pkt_prod(10977) != txdata->tx_pkt_cons(10969) [28203.106280] bnx2x: [bnx2x_clean_tx_queue:1205(eth3)]timeout waiting for queue[4]: txdata->tx_pkt_prod(9532) != txdata->tx_pkt_cons(9514) [28205.106284] bnx2x: [bnx2x_clean_tx_queue:1205(eth3)]timeout waiting for queue[6]: txdata->tx_pkt_prod(2359) != txdata->tx_pkt_cons(2331) [28205.314280] bnx2x: [bnx2x_io_slot_reset:14246(eth3)]before -- state flag = 0x4000 <---- check NIC state [28205.314282] bnx2x: [bnx2x_io_slot_reset:14248(eth3)]after -- state flag = 0x4000 <---- check NIC state and disable the NAPI [28205.405009] PCI 0201:50:00.0#500000: EEH: bnx2x driver reports: 'recovered' ........ snip ...... [28213.525014] EEH: Finished:'slot_reset' with aggregate recovery state:'recovered' [28213.525016] EEH: Notify device driver to resume [28213.525017] EEH: Beginning: 'resume' [28213.525018] PCI 0201:50:00.0#500000: EEH: Invoking bnx2x->resume() [28225.115287] bnx2x 0201:50:00.0 eth3: using MSI-X IRQs: sp 101 fp[0] 103 ... fp[7] 110 [28225.696425] PCI 0201:50:00.0#500000: EEH: bnx2x driver reports:'none'
2- When the driver reset code path was called first, the
bnx2x_chip_cleanup()does the disablement the NAPI, but NIC is resumed after the EEH handler code path was called.[13376.774278] bnx2x: [bnx2x_state_wait:312(eth3)]timeout waiting for state 2 [13376.774280] bnx2x: [bnx2x_func_stop:9118(eth3)]FUNC_STOP ramrod failed. Running a dry transaction [13376.774339] bnx2x: [bnx2x_chip_cleanup:9467(eth3)]before state flag = 0x4000 [13376.774341] bnx2x: [bnx2x_chip_cleanup:9469(eth3)]after state flag = 0x4000 [13376.774344] bnx2x: [bnx2x_igu_int_disable:891(eth3)]BUG! Proper val not read from IGU! [13391.774280] bnx2x: [bnx2x_fw_command:3044(eth3)]FW failed to respond! ........ snip ..... [13442.118385] PCI 0201:50:00.0#500000: EEH: Invoking bnx2x->slot_reset() [13442.118388] bnx2x: [bnx2x_io_slot_reset:14212(eth3)]IO slot reset initializing... [13442.118509] bnx2x 0201:50:00.0: enabling device (0140 -> 0142) [13442.124825] bnx2x: [bnx2x_io_slot_reset:14228(eth3)]IO slot reset --> driver unload [13442.154290] bnx2x: [bnx2x_io_slot_reset:14246(eth3)]before -- state flag = 0x4000 <--- check NIC state [13442.274291] PCI 0201:50:00.0#500000: EEH: bnx2x driver reports: 'recovered' ....... snip ......... [13442.424290] EEH: Finished:'slot_reset' with aggregate recovery state:'recovered' [13442.424291] EEH: Notify device driver to resume [13442.424292] EEH: Beginning: 'resume' [13442.424293] PCI 0201:50:00.0#500000: EEH: Invoking bnx2x->resume() [13443.015280] bnx2x 0201:50:00.0 eth3: using MSI-X IRQs: sp 61 fp[0] 63 ... fp[7] 75 [13443.616423] PCI 0201:50:00.0#500000: EEH: bnx2x driver reports:'none'
Quoted 7 lines
Quoted 2 lines
Thanks,
ManishIt requires a specific system setup, I will test with v3 patch when the system is available.
Thanks,
Thinh TranAny suggestions?
Thanks,
Thinh Tran
Manish Chopra
maintainerHello Thinh,
Quoted 11 lines
-----Original Message-----
From: Thinh Tran <[email protected]>
Sent: Tuesday, October 11, 2022 3:31 AM
To: Manish Chopra <[email protected]>
Cc: [email protected]; Ariel Elior <[email protected]>; [email protected]; Sudarsana Reddy Kalluru <[email protected]>; [email protected]; [email protected]; Alok Prasad <[email protected]>; [email protected]
Subject: Re: [EXT] [PATCH v2] bnx2x: Fix error recovering in switch configuration
Quoted 3 lines
Hi Manish,
Do you have any other suggestion or using the extra variable,nic_stopped, in v2 patch is efficient enough to fix the issue?
I am fine with this if using existing 'state' var is not a suitable option here. Thanks.
Quoted 228 lines
Thanks,
Thinh TranQuoted 12 lines · Thinh Tran
Hi Manish,
I think we need the extra variable,
nic_stopped, or perhaps defining a different state to determine the disablement of NAPI and freed IRQs. Since device gets the error, the NIC state is set toBNX2X_STATE_CLOSING_WAIT4_HALT(0x4000)14132 static int
bnx2x_eeh_nic_unload(struct bnx2x *bp)14133 {
14134bp->state=BNX2X_STATE_CLOSING_WAIT4_HALT; 14135Quoted 17 lines
and stays in this state in EEH handler and driver reset code paths, when both trying to disabling the NAPI.
Quoted 13 lines · Thinh Tran
Hi Manish,
Thanks for reviewing the patch.Quoted 8 lines · Manish Chopra
Hello Thinh,
Quoted 6 lines
-----Original Message-----
From: Thinh Tran <[email protected]>
Sent: Saturday, September 17, 2022 1:21 AM
To: [email protected]
Cc: [email protected]; Ariel Elior <[email protected]>; [email protected]; Manish Chopra <[email protected]>;Sudarsana
Quoted 97 lines
Quoted 85 lines
Quoted 46 lines
Quoted 46 lines
Reddy Kalluru <[email protected]>; [email protected]; [email protected]; Thinh Tran <[email protected]>
Subject: [EXT] [PATCH v2] bnx2x: Fix error recovering in switch configurationExternal Email
-------------------------------------------------------------------
--- As the BCM57810 and other I/O adapters are connected through a PCIe switch, the bnx2x driver causes unexpected system hang/crash while handling PCIe switch errors, if its error handler is called after other drivers' handlers.In this case, after numbers of
bnx2x_tx_timout(), thebnx2x_nic_unload()is called, frees up resources and callsbnx2x_napi_disable(). Then when EEH calls its error handler, thebnx2x_io_error_detected()andbnx2x_io_slot_reset()also callingbnx2x_napi_disable()and freeing the resources.Signed-off-by Thinh Tran <[email protected]>v2:
- Check the state of the NIC before calling disable nappi and freeing the IRQ
- Prevent recurrence of TX timeout by turning off the carrier, callingnetif_carrier_off()inbnx2x_tx_timeout()
- Check and bail out early iffp->page_poolalready freed
---drivers/net/ethernet/broadcom/bnx2x/bnx2x.h | 2 + .../net/ethernet/broadcom/bnx2x/bnx2x_cmn.c | 27 +++++++++---- .../net/ethernet/broadcom/bnx2x/bnx2x_cmn.h | 3 ++ .../net/ethernet/broadcom/bnx2x/bnx2x_main.c | 38 +++++++++---------- .../net/ethernet/broadcom/bnx2x/bnx2x_vfpf.c | 17 +++++---- 5 files changed, 53 insertions(+), 34 deletions(-)drivers/net/ethernet/broadcom/bnx2x/bnx2x.h +0
-0@@ -1509,6 +1509,8 @@ struct bnx2x { bool cnic_loaded; struct cnic_eth_dev *(*cnic_probe)(struct net_device *);Quoted 6 lines
drivers/net/ethernet/broadcom/bnx2x/bnx2x.h struct bnx2x { · line 1513
Quoted 2 lines
drivers/net/ethernet/broadcom/bnx2x/bnx2x.h +2
-0+ bool nic_stopped; +There is an already 'state' variable which holds the NIC state whether it was opened or closed. > Perhaps we could use that easily in
bnx2x_io_slot_reset()to preventmultiple NAPI disablement
Quoted 2 lines
instead of adding a newer one. Please see below for ex -
In my early debug, I did checked for
bp->statevariable but I was unsure what the NIC state was safe to disable the NAPI. Thanks for the hint.Quoted 26 lines
drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c +8
-5@@ -14256,13 +14256,16 @@ static pci_ers_result_t bnx2x_io_slot_reset(struct pci_dev *pdev) } bnx2x_drain_tx_queues(bp); bnx2x_send_unload_req(bp, UNLOAD_RECOVERY); - bnx2x_netif_stop(bp, 1); - bnx2x_del_all_napi(bp); - if (CNIC_LOADED(bp)) - bnx2x_del_all_napi_cnic(bp); + if (bp->state == BNX2X_STATE_OPEN) { + bnx2x_netif_stop(bp, 1); + bnx2x_del_all_napi(bp); - bnx2x_free_irq(bp); + if (CNIC_LOADED(bp)) + bnx2x_del_all_napi_cnic(bp); + + bnx2x_free_irq(bp); + }Quoted 3 lines
Quoted 3 lines
drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c +0
-0/* Report UNLOAD_DONE to MCP */
bnx2x_send_unload_done(bp, true);Using the v2 patch, I checked the NIC state at places before and after disabling the NAPI,
BNX2X_ERR("before state flag = 0x%x \n", bp->state); if (!bp->nic_stopped) {
BNX2X_ERR("after state flag = 0x%x \n", bp->state);bnx2x_netif_stop(bp, 1);
bnx2x_del_all_napi(bp);
....Quoted 1 line
the NIC state is always
BNX2X_STATE_CLOSING_WAIT4_HALT(0x4000)when
Quoted 94 lines
device get error injection.
1- When EEH handler code path was called first, the
bnx2x_io_slot_reset()does disablement the NAPI and done.[28197.100795] bnx2x: [bnx2x_io_slot_reset:14212(eth3)]IO slot reset initializing... [28197.100959] bnx2x 0201:50:00.0: enabling device (0140 -> 0142) [28197.107249] bnx2x: [bnx2x_io_slot_reset:14228(eth3)]IO slot reset --> driver unload [28199.106281] bnx2x: [bnx2x_clean_tx_queue:1205(eth3)]timeout waiting for queue[1]: txdata->tx_pkt_prod(7270) != txdata->tx_pkt_cons(7269) [28201.106281] bnx2x: [bnx2x_clean_tx_queue:1205(eth3)]timeout waiting for queue[3]: txdata->tx_pkt_prod(10977) != txdata->tx_pkt_cons(10969) [28203.106280] bnx2x: [bnx2x_clean_tx_queue:1205(eth3)]timeout waiting for queue[4]: txdata->tx_pkt_prod(9532) != txdata->tx_pkt_cons(9514) [28205.106284] bnx2x: [bnx2x_clean_tx_queue:1205(eth3)]timeout waiting for queue[6]: txdata->tx_pkt_prod(2359) != txdata->tx_pkt_cons(2331) [28205.314280] bnx2x: [bnx2x_io_slot_reset:14246(eth3)]before -- state flag = 0x4000 <---- check NIC state [28205.314282] bnx2x: [bnx2x_io_slot_reset:14248(eth3)]after -- state flag = 0x4000 <---- check NIC state and disable the NAPI [28205.405009] PCI 0201:50:00.0#500000: EEH: bnx2x driver reports: 'recovered' ........ snip ...... [28213.525014] EEH: Finished:'slot_reset' with aggregate recovery state:'recovered' [28213.525016] EEH: Notify device driver to resume [28213.525017] EEH: Beginning: 'resume' [28213.525018] PCI 0201:50:00.0#500000: EEH: Invoking bnx2x->resume() [28225.115287] bnx2x 0201:50:00.0 eth3: using MSI-X IRQs: sp 101 fp[0] 103 ... fp[7] 110 [28225.696425] PCI 0201:50:00.0#500000: EEH: bnx2x driver reports:'none'
2- When the driver reset code path was called first, the
bnx2x_chip_cleanup()does the disablement the NAPI, but NIC is resumed after the EEH handler code path was called.[13376.774278] bnx2x: [bnx2x_state_wait:312(eth3)]timeout waiting for state 2 [13376.774280] bnx2x: [bnx2x_func_stop:9118(eth3)]FUNC_STOP ramrod failed. Running a dry transaction [13376.774339] bnx2x: [bnx2x_chip_cleanup:9467(eth3)]before state flag = 0x4000 [13376.774341] bnx2x: [bnx2x_chip_cleanup:9469(eth3)]after state flag = 0x4000 [13376.774344] bnx2x: [bnx2x_igu_int_disable:891(eth3)]BUG! Proper val not read from IGU! [13391.774280] bnx2x: [bnx2x_fw_command:3044(eth3)]FW failed to respond! ........ snip ..... [13442.118385] PCI 0201:50:00.0#500000: EEH: Invoking bnx2x->slot_reset() [13442.118388] bnx2x: [bnx2x_io_slot_reset:14212(eth3)]IO slot reset initializing... [13442.118509] bnx2x 0201:50:00.0: enabling device (0140 -> 0142) [13442.124825] bnx2x: [bnx2x_io_slot_reset:14228(eth3)]IO slot reset --> driver unload [13442.154290] bnx2x: [bnx2x_io_slot_reset:14246(eth3)]before -- state flag = 0x4000 <--- check NIC state [13442.274291] PCI 0201:50:00.0#500000: EEH: bnx2x driver reports: 'recovered' ....... snip ......... [13442.424290] EEH: Finished:'slot_reset' with aggregate recovery state:'recovered' [13442.424291] EEH: Notify device driver to resume [13442.424292] EEH: Beginning: 'resume' [13442.424293] PCI 0201:50:00.0#500000: EEH: Invoking bnx2x->resume() [13443.015280] bnx2x 0201:50:00.0 eth3: using MSI-X IRQs: sp 61 fp[0] 63 ... fp[7] 75 [13443.616423] PCI 0201:50:00.0#500000: EEH: bnx2x driver reports:'none'
Quoted 8 lines
Quoted 2 lines
Thanks,
ManishIt requires a specific system setup, I will test with v3 patch when the system is available.
Thanks,
Thinh TranAny suggestions?
Thanks,
Thinh Tran
Hi Manish,
Quoted 2 lines · Manish Chopra
I am fine with this if using existing 'state' var is not a suitable option here. Thanks.
Thanks for the review
Thinh Tran
PATCH v3 1/1 bnx2x: Fix error recovering in switch configuration
In reply to Thinh TranAs the BCM57810 and other I/O adapters are connected through a PCIe switch, the bnx2x driver causes unexpected system hang/crash while handling PCIe switch errors, if its error handler is called after other drivers' handlers.
In this case, after numbers of bnx2x_tx_timout(), the bnx2x_nic_unload() is called, frees up resources and calls bnx2x_napi_disable(). Then when EEH calls its error handler, the bnx2x_io_error_detected() and bnx2x_io_slot_reset() also calling bnx2x_napi_disable() and freeing the resources.
v3:
- no changes, just repatched to the latest driver level
- updated the reviewed-by Manish in October, 2022
v2:
- Check the state of the NIC before calling disable nappi and freeing the IRQ
- Prevent recurrence of TX timeout by turning off the carrier, calling netif_carrier_off() in bnx2x_tx_timeout()
- Check and bail out early if fp->page_pool already freed
--- drivers/net/ethernet/broadcom/bnx2x/bnx2x.h | 2 + .../net/ethernet/broadcom/bnx2x/bnx2x_cmn.c | 27 +++++++++---- .../net/ethernet/broadcom/bnx2x/bnx2x_cmn.h | 3 ++ .../net/ethernet/broadcom/bnx2x/bnx2x_main.c | 38 +++++++++---------- .../net/ethernet/broadcom/bnx2x/bnx2x_vfpf.c | 17 +++++---- 5 files changed, 53 insertions(+), 34 deletions(-)
drivers/net/ethernet/broadcom/bnx2x/bnx2x.h +2 -0
@@ -1508,6 +1508,8 @@ struct bnx2x {bool cnic_loaded;struct cnic_eth_dev *(*cnic_probe)(struct net_device *); +bool nic_stopped; +/* Flag that indicates that we can start looking for FCoE L2 queue * completions in the default status block. */
drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c +19 -8
@@ -2715,6 +2715,7 @@ int bnx2x_nic_load(struct bnx2x *bp, int load_mode)bnx2x_add_all_napi (bp);DP (NETIF_MSG_IFUP ,"napi added\n" );bnx2x_napi_enable (bp); + bp->nic_stopped = false;if (IS_PF (bp)) {/* set pf load just before approaching the MCP */
@@ -2960,6 +2961,7 @@ int bnx2x_nic_load(struct bnx2x *bp, int load_mode) load_error1:bnx2x_napi_disable (bp);bnx2x_del_all_napi (bp); + bp->nic_stopped = true;/* clear pf_load status, as it was already set */ if (IS_PF (bp))
@@ -3095,14 +3097,17 @@ int bnx2x_nic_unload(struct bnx2x *bp, int unload_mode, bool keep_link)if (!CHIP_IS_E1x (bp))bnx2x_pf_disable (bp); -/* Disable HW interrupts, NAPI */ -bnx2x_netif_stop (bp,1 ); -/* Delete all NAPI objects */ -bnx2x_del_all_napi (bp); -if (CNIC_LOADED (bp)) -bnx2x_del_all_napi_cnic (bp); -/* Release IRQs */ -bnx2x_free_irq (bp); +if (!bp->nic_stopped) { +/* Disable HW interrupts, NAPI */ +bnx2x_netif_stop (bp,1 ); +/* Delete all NAPI objects */ +bnx2x_del_all_napi (bp); +if (CNIC_LOADED (bp)) +bnx2x_del_all_napi_cnic (bp); +/* Release IRQs */ +bnx2x_free_irq (bp); + bp->nic_stopped = true; + }/* Report UNLOAD_DONE to MCP */ bnx2x_send_unload_done (bp, false);
@@ -4987,6 +4992,12 @@ void bnx2x_tx_timeout(struct net_device *dev, unsigned int txqueue) {struct bnx2x *bp =netdev_priv (dev); +/* Immediately indicate link as down */ + bp->link_vars.link_up =0 ; + bp->force_link_down = true; +netif_carrier_off (dev); +BNX2X_ERR ("Indicating link is down due to Tx-timeout\n" ); +/* We want the information of the dump logged, * but calling bnx2x_panic() would kill all chances of recovery. */
drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.h +3 -0
@@ -1015,6 +1015,9 @@ static inline void bnx2x_free_rx_sge_range(struct bnx2x *bp, {int i; +if (!fp->page_pool.page) +return ; +if (fp->mode ==TPA_MODE_DISABLED )return ;
drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c +19 -19
@@ -9474,15 +9474,18 @@ void bnx2x_chip_cleanup(struct bnx2x *bp, int unload_mode, bool keep_link) } } -/* Disable HW interrupts, NAPI */ -bnx2x_netif_stop (bp,1 ); -/* Delete all NAPI objects */ -bnx2x_del_all_napi (bp); -if (CNIC_LOADED (bp)) -bnx2x_del_all_napi_cnic (bp); +if (!bp->nic_stopped) { +/* Disable HW interrupts, NAPI */ +bnx2x_netif_stop (bp,1 ); +/* Delete all NAPI objects */ +bnx2x_del_all_napi (bp); +if (CNIC_LOADED (bp)) +bnx2x_del_all_napi_cnic (bp); -/* Release IRQs */ -bnx2x_free_irq (bp); +/* Release IRQs */ +bnx2x_free_irq (bp); + bp->nic_stopped = true; + }/* Reset the chip, unless PCI function is offline. If we reach this * point following a PCI error handling, it means device is really
@@ -10273,12 +10276,6 @@ static void bnx2x_sp_rtnl_task(struct work_struct *work) bp->sp_rtnl_state =0 ;smp_mb (); -/* Immediately indicate link as down */ - bp->link_vars.link_up =0 ; - bp->force_link_down = true; -netif_carrier_off (bp->dev); -BNX2X_ERR ("Indicating link is down due to Tx-timeout\n" ); -bnx2x_nic_unload (bp,UNLOAD_NORMAL , true);/* When ret value shows failure of allocation failure, * the nic is rebooted again. If open still fails, a error
@@ -14238,13 +14235,16 @@ static pci_ers_result_t bnx2x_io_slot_reset(struct pci_dev *pdev) }bnx2x_drain_tx_queues (bp);bnx2x_send_unload_req (bp,UNLOAD_RECOVERY ); -bnx2x_netif_stop (bp,1 ); -bnx2x_del_all_napi (bp); +if (!bp->nic_stopped) { +bnx2x_netif_stop (bp,1 ); +bnx2x_del_all_napi (bp); -if (CNIC_LOADED (bp)) -bnx2x_del_all_napi_cnic (bp); +if (CNIC_LOADED (bp)) +bnx2x_del_all_napi_cnic (bp); -bnx2x_free_irq (bp); +bnx2x_free_irq (bp); + bp->nic_stopped = true; + }/* Report UNLOAD_DONE to MCP */ bnx2x_send_unload_done (bp, true);
drivers/net/ethernet/broadcom/bnx2x/bnx2x_vfpf.c +10 -7
@@ -529,13 +529,16 @@ void bnx2x_vfpf_close_vf(struct bnx2x *bp)bnx2x_vfpf_finalize (bp, &req->first_tlv); free_irq: -/* Disable HW interrupts, NAPI */ -bnx2x_netif_stop (bp,0 ); -/* Delete all NAPI objects */ -bnx2x_del_all_napi (bp); - -/* Release IRQs */ -bnx2x_free_irq (bp); +if (!bp->nic_stopped) { +/* Disable HW interrupts, NAPI */ +bnx2x_netif_stop (bp,0 ); +/* Delete all NAPI objects */ +bnx2x_del_all_napi (bp); + +/* Release IRQs */ +bnx2x_free_irq (bp); + bp->nic_stopped = true; + } } static void bnx2x_leading_vfq_init(struct bnx2x *bp, struct bnx2x_virtf *vf,
Signature · 1 line
2.25.1
Quoted 14 lines · Thinh Tran
As the BCM57810 and other I/O adapters are connected through a PCIe switch, the bnx2x driver causes unexpected system hang/crash while handling PCIe switch errors, if its error handler is called after other drivers' handlers.
In this case, after numbers of
bnx2x_tx_timout(), thebnx2x_nic_unload()is called, frees up resources and callsbnx2x_napi_disable(). Then when EEH calls its error handler, thebnx2x_io_error_detected()andbnx2x_io_slot_reset()also callingbnx2x_napi_disable()and freeing the resources.
Quoted 4 lines
Signed-off-by Thinh Tran <[email protected]>Reviewed-by Manish Chopra <[email protected]>Tested-by Abdul Haleem <[email protected]>Tested-by David Christensen <[email protected]>
Jakub Kicinski
maintainerQuoted 9 lines · On Wed, 19 Jul 2023 22:02:01 +0000 Thinh Tran wrote:
drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c +1
-8-/* Disable HW interrupts, NAPI */ -bnx2x_netif_stop (bp,1 ); -/* Delete all NAPI objects */ -bnx2x_del_all_napi (bp); -if (CNIC_LOADED (bp)) -bnx2x_del_all_napi_cnic (bp); -/* Release IRQs */ -bnx2x_free_irq (bp); +if (!bp->nic_stopped) {
drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c int bnx2x_nic_unload(struct bnx2x *bp, int unload_mode, bool keep_link) · line 3110
Quoted 10 lines
drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c +10
-0+/* Disable HW interrupts, NAPI */ +bnx2x_netif_stop (bp,1 ); +/* Delete all NAPI objects */ +bnx2x_del_all_napi (bp); +if (CNIC_LOADED (bp)) +bnx2x_del_all_napi_cnic (bp); +/* Release IRQs */ +bnx2x_free_irq (bp); + bp->nic_stopped = true; + }
You're sprinkling this if around the same piece of code in multiple places. Please factor it out into a function, and add return early from it if needed. If possible please keep the code symmetric (i.e. also factor out the inverse of this code for starting the NIC).
Signature · 1 line
pw-bot: cr
Quoted 5 lines · Jakub Kicinski
You're sprinkling this if around the same piece of code in multiple places. Please factor it out into a function, and add return early from it if needed. If possible please keep the code symmetric (i.e. also factor out the inverse of this code for starting the NIC).
Thank you for reviewing. I'll work for creating a separate function for it, and one for the inverse as well if it's possible.
Thinh Tran
PATCH v4 1/1 bnx2x: Fix error recovering in switch configuration
In reply to Thinh TranAs the BCM57810 and other I/O adapters are connected through a PCIe switch, the bnx2x driver causes unexpected system hang/crash while handling PCIe switch errors, if its error handler is called after other drivers' handlers.
In this case, after numbers of bnx2x_tx_timout(), the bnx2x_nic_unload() is called, frees up resources and calls bnx2x_napi_disable(). Then when EEH calls its error handler, the bnx2x_io_error_detected() and bnx2x_io_slot_reset() also calling bnx2x_napi_disable() and freeing the resources.
v4:
- factoring common code into new function bnx2x_stop_nic() that disables and releases IRQs and NAPIs
v3:
- no changes, just repatched to the latest driver level
- updated the reviewed-by Manish in October, 2022
v2:
- Check the state of the NIC before calling disable nappi and freeing the IRQ
- Prevent recurrence of TX timeout by turning off the carrier, calling netif_carrier_off() in bnx2x_tx_timeout()
- Check and bail out early if fp->page_pool already freed
--- drivers/net/ethernet/broadcom/bnx2x/bnx2x.h | 2 ++ .../net/ethernet/broadcom/bnx2x/bnx2x_cmn.c | 18 +++++++------ .../net/ethernet/broadcom/bnx2x/bnx2x_cmn.h | 18 +++++++++++++ .../net/ethernet/broadcom/bnx2x/bnx2x_main.c | 25 +++---------------- .../net/ethernet/broadcom/bnx2x/bnx2x_vfpf.c | 9 ++----- 5 files changed, 36 insertions(+), 36 deletions(-)
drivers/net/ethernet/broadcom/bnx2x/bnx2x.h +2 -0
@@ -1508,6 +1508,8 @@ struct bnx2x {bool cnic_loaded;struct cnic_eth_dev *(*cnic_probe)(struct net_device *); +bool nic_stopped; +/* Flag that indicates that we can start looking for FCoE L2 queue * completions in the default status block. */
drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c +10 -8
@@ -2715,6 +2715,7 @@ int bnx2x_nic_load(struct bnx2x *bp, int load_mode)bnx2x_add_all_napi (bp);DP (NETIF_MSG_IFUP ,"napi added\n" );bnx2x_napi_enable (bp); + bp->nic_stopped = false;if (IS_PF (bp)) {/* set pf load just before approaching the MCP */
@@ -2960,6 +2961,7 @@ int bnx2x_nic_load(struct bnx2x *bp, int load_mode) load_error1:bnx2x_napi_disable (bp);bnx2x_del_all_napi (bp); + bp->nic_stopped = true;/* clear pf_load status, as it was already set */ if (IS_PF (bp))
@@ -3095,14 +3097,8 @@ int bnx2x_nic_unload(struct bnx2x *bp, int unload_mode, bool keep_link)if (!CHIP_IS_E1x (bp))bnx2x_pf_disable (bp); -/* Disable HW interrupts, NAPI */ -bnx2x_netif_stop (bp,1 ); -/* Delete all NAPI objects */ -bnx2x_del_all_napi (bp); -if (CNIC_LOADED (bp)) -bnx2x_del_all_napi_cnic (bp); -/* Release IRQs */ -bnx2x_free_irq (bp); +/* Disable HW interrupts, delete NAPIs, Release IRQs */ +bnx2x_stop_nic (bp);/* Report UNLOAD_DONE to MCP */ bnx2x_send_unload_done (bp, false);
@@ -4987,6 +4983,12 @@ void bnx2x_tx_timeout(struct net_device *dev, unsigned int txqueue) {struct bnx2x *bp =netdev_priv (dev); +/* Immediately indicate link as down */ + bp->link_vars.link_up =0 ; + bp->force_link_down = true; +netif_carrier_off (dev); +BNX2X_ERR ("Indicating link is down due to Tx-timeout\n" ); +/* We want the information of the dump logged, * but calling bnx2x_panic() would kill all chances of recovery. */
drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.h +18 -0
@@ -1015,6 +1015,9 @@ static inline void bnx2x_free_rx_sge_range(struct bnx2x *bp, {int i; +if (!fp->page_pool.page) +return ; +if (fp->mode ==TPA_MODE_DISABLED )return ;
@@ -1399,5 +1402,20 @@ void bnx2x_set_os_driver_state(struct bnx2x *bp, u32 state);*/ int bnx2x_nvram_read (struct bnx2x *bp,u32 offset,u8 *ret_buf,int buf_size); +static inline void bnx2x_stop_nic (struct bnx2x *bp) +{ +if (!bp->nic_stopped) { +/* Disable HW interrupts, NAPI */ +bnx2x_netif_stop (bp,1 ); +/* Delete all NAPI objects */ +bnx2x_del_all_napi (bp); +if (CNIC_LOADED (bp)) +bnx2x_del_all_napi_cnic (bp); +/* Release IRQs */ +bnx2x_free_irq (bp); + bp->nic_stopped = true; + } +} +#endif /* BNX2X_CMN_H */
drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c +4 -21
@@ -9474,15 +9474,8 @@ void bnx2x_chip_cleanup(struct bnx2x *bp, int unload_mode, bool keep_link) } } -/* Disable HW interrupts, NAPI */ -bnx2x_netif_stop (bp,1 ); -/* Delete all NAPI objects */ -bnx2x_del_all_napi (bp); -if (CNIC_LOADED (bp)) -bnx2x_del_all_napi_cnic (bp); - -/* Release IRQs */ -bnx2x_free_irq (bp); +/* Disable HW interrupts, delete NAPIs, Release IRQs */ +bnx2x_stop_nic (bp);/* Reset the chip, unless PCI function is offline. If we reach this * point following a PCI error handling, it means device is really
@@ -10273,12 +10266,6 @@ static void bnx2x_sp_rtnl_task(struct work_struct *work) bp->sp_rtnl_state =0 ;smp_mb (); -/* Immediately indicate link as down */ - bp->link_vars.link_up =0 ; - bp->force_link_down = true; -netif_carrier_off (bp->dev); -BNX2X_ERR ("Indicating link is down due to Tx-timeout\n" ); -bnx2x_nic_unload (bp,UNLOAD_NORMAL , true);/* When ret value shows failure of allocation failure, * the nic is rebooted again. If open still fails, a error
@@ -14238,13 +14225,9 @@ static pci_ers_result_t bnx2x_io_slot_reset(struct pci_dev *pdev) }bnx2x_drain_tx_queues (bp);bnx2x_send_unload_req (bp,UNLOAD_RECOVERY ); -bnx2x_netif_stop (bp,1 ); -bnx2x_del_all_napi (bp); - -if (CNIC_LOADED (bp)) -bnx2x_del_all_napi_cnic (bp); -bnx2x_free_irq (bp); +/* Disable HW interrupts, delete NAPIs, Release IRQs */ +bnx2x_stop_nic (bp);/* Report UNLOAD_DONE to MCP */ bnx2x_send_unload_done (bp, true);
drivers/net/ethernet/broadcom/bnx2x/bnx2x_vfpf.c +2 -7
@@ -529,13 +529,8 @@ void bnx2x_vfpf_close_vf(struct bnx2x *bp)bnx2x_vfpf_finalize (bp, &req->first_tlv); free_irq: -/* Disable HW interrupts, NAPI */ -bnx2x_netif_stop (bp,0 ); -/* Delete all NAPI objects */ -bnx2x_del_all_napi (bp); - -/* Release IRQs */ -bnx2x_free_irq (bp); +/* Disable HW interrupts, delete NAPIs, Release IRQs */ +bnx2x_stop_nic (bp); } static void bnx2x_leading_vfq_init(struct bnx2x *bp, struct bnx2x_virtf *vf,
Signature · 1 line
2.31.1
Jakub Kicinski
maintainerQuoted 13 lines · On Fri, 28 Jul 2023 16:11:33 -0500 Thinh Tran wrote:
As the BCM57810 and other I/O adapters are connected through a PCIe switch, the bnx2x driver causes unexpected system hang/crash while handling PCIe switch errors, if its error handler is called after other drivers' handlers.
In this case, after numbers of
bnx2x_tx_timout(), thebnx2x_nic_unload()is called, frees up resources and callsbnx2x_napi_disable(). Then when EEH calls its error handler, thebnx2x_io_error_detected()andbnx2x_io_slot_reset()also callingbnx2x_napi_disable()and freeing the resources.
Quoted 6 lines
Signed-off-by Thinh Tran <[email protected]>Reviewed-by Manish Chopra <[email protected]>Tested-by Abdul Haleem <[email protected]>Tested-by David Christensen <[email protected]>Reviewed-by Simon Horman <[email protected]>
nit: no empty lines between tags
There should be a "---" line between the tags and changelog.
Quoted 19 lines
v4:
- factoring common code into new functionbnx2x_stop_nic()that disables and releases IRQs and NAPIs
v3:
- no changes, just repatched to the latest driver level
- updated the reviewed-by Manish in October, 2022v2:
- Check the state of the NIC before calling disable nappi and freeing the IRQ
- Prevent recurrence of TX timeout by turning off the carrier, callingnetif_carrier_off()inbnx2x_tx_timeout()
- Check and bail out early iffp->page_poolalready freed--- drivers/net/ethernet/broadcom/bnx2x/bnx2x.h | 2 ++drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c +0
-0@@ -3095,14 +3097,8 @@ int bnx2x_nic_unload(struct bnx2x *bp, int unload_mode, bool keep_link)if (!CHIP_IS_E1x (bp))
drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c int bnx2x_nic_unload(struct bnx2x *bp, int unload_mode, bool keep_link) · line 3105
Quoted 10 lines
drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c +0
-8bnx2x_pf_disable (bp); -/* Disable HW interrupts, NAPI */ -bnx2x_netif_stop (bp,1 ); -/* Delete all NAPI objects */ -bnx2x_del_all_napi (bp); -if (CNIC_LOADED (bp)) -bnx2x_del_all_napi_cnic (bp); -/* Release IRQs */ -bnx2x_free_irq (bp);
Could you split the change into two patches - one factoring out the code into bnx2x_stop_nic() and the other adding the nic_stopped variable? First one should be pure code refactoring with no functional changes. That'd make the reviewing process easier.
Quoted 6 lines
drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c +2
-0+/* Disable HW interrupts, delete NAPIs, Release IRQs */ +bnx2x_stop_nic (bp);/* Report UNLOAD_DONE to MCP */ bnx2x_send_unload_done (bp, false);
drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c void bnx2x_tx_timeout(struct net_device *dev, unsigned int txqueue) · line 4990
Quoted 8 lines
drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c +5
-0@@ -4987,6 +4983,12 @@ void bnx2x_tx_timeout(struct net_device *dev, unsigned int txqueue) {struct bnx2x *bp =netdev_priv (dev); +/* Immediately indicate link as down */ + bp->link_vars.link_up =0 ; + bp->force_link_down = true; +netif_carrier_off (dev); +BNX2X_ERR ("Indicating link is down due to Tx-timeout\n" );
Is this code move to make the shutdown more immediate? That could also be a separate patch.
Quoted 18 lines
drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c +0
-0/* We want the information of the dump logged, * but calling bnx2x_panic() would kill all chances of recovery. */ drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.h +3
-0@@ -1015,6 +1015,9 @@ static inline void bnx2x_free_rx_sge_range(struct bnx2x *bp, {int i; +if (!fp->page_pool.page) +return ; +if (fp->mode ==TPA_MODE_DISABLED )return ;
drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.h void bnx2x_set_os_driver_state(struct bnx2x *bp, u32 state); · line 1405
Quoted 4 lines
drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.h +1
-0@@ -1399,5 +1402,20 @@ void bnx2x_set_os_driver_state(struct bnx2x *bp, u32 state);*/ int bnx2x_nvram_read (struct bnx2x *bp,u32 offset,u8 *ret_buf,int buf_size); +static inline void bnx2x_stop_nic (struct bnx2x *bp)
can't it live in bnx2x_cmn.c ? Why make it a static inline?
Quoted 5 lines
drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.h +5
-0+{ +if (!bp->nic_stopped) { +/* Disable HW interrupts, NAPI */ +bnx2x_netif_stop (bp,1 ); +/* Delete all NAPI objects */
drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.h void bnx2x_set_os_driver_state(struct bnx2x *bp, u32 state); · line 1420
Quoted 10 lines
drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.h +9
-0+bnx2x_del_all_napi (bp); +if (CNIC_LOADED (bp)) +bnx2x_del_all_napi_cnic (bp); +/* Release IRQs */ +bnx2x_free_irq (bp); + bp->nic_stopped = true; + } +} +
nit: double new line
drivers/net/ethernet/broadcom/bnx2x/bnx2x_vfpf.c void bnx2x_vfpf_close_vf(struct bnx2x *bp) · line 533
Quoted 8 lines
drivers/net/ethernet/broadcom/bnx2x/bnx2x_vfpf.c +0
-2@@ -529,13 +529,8 @@ void bnx2x_vfpf_close_vf(struct bnx2x *bp)bnx2x_vfpf_finalize (bp, &req->first_tlv); free_irq: -/* Disable HW interrupts, NAPI */ -bnx2x_netif_stop (bp,0 );
This used to say
bnx2x_netif_stop(bp, 0);
but bnx2x_stop_nic() will do:
bnx2x_netif_stop(bp, 1);
is it okay to shut down the HW here ? (whatever that entails)
Quoted 7 lines
drivers/net/ethernet/broadcom/bnx2x/bnx2x_vfpf.c +2
-5-/* Delete all NAPI objects */ -bnx2x_del_all_napi (bp); - -/* Release IRQs */ -bnx2x_free_irq (bp); +/* Disable HW interrupts, delete NAPIs, Release IRQs */ +bnx2x_stop_nic (bp);
Signature · 1 line
pw-bot: cr
Paolo Abeni
maintainerQuoted 69 lines · Jakub Kicinski
Quoted 13 lines · On Fri, 28 Jul 2023 16:11:33 -0500 Thinh Tran wrote:
As the BCM57810 and other I/O adapters are connected through a PCIe switch, the bnx2x driver causes unexpected system hang/crash while handling PCIe switch errors, if its error handler is called after other drivers' handlers.
In this case, after numbers of
bnx2x_tx_timout(), thebnx2x_nic_unload()is called, frees up resources and callsbnx2x_napi_disable(). Then when EEH calls its error handler, thebnx2x_io_error_detected()andbnx2x_io_slot_reset()also callingbnx2x_napi_disable()and freeing the resources.Quoted 6 lines
Signed-off-by Thinh Tran <[email protected]>Reviewed-by Manish Chopra <[email protected]>Tested-by Abdul Haleem <[email protected]>Tested-by David Christensen <[email protected]>Reviewed-by Simon Horman <[email protected]>nit: no empty lines between tags
There should be a "---" line between the tags and changelog.
Quoted 19 lines
v4:
- factoring common code into new functionbnx2x_stop_nic()that disables and releases IRQs and NAPIs
v3:
- no changes, just repatched to the latest driver level
- updated the reviewed-by Manish in October, 2022v2:
- Check the state of the NIC before calling disable nappi and freeing the IRQ
- Prevent recurrence of TX timeout by turning off the carrier, callingnetif_carrier_off()inbnx2x_tx_timeout()
- Check and bail out early iffp->page_poolalready freed--- drivers/net/ethernet/broadcom/bnx2x/bnx2x.h | 2 ++drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c +0
-0@@ -3095,14 +3097,8 @@ int bnx2x_nic_unload(struct bnx2x *bp, int unload_mode, bool keep_link)if (!CHIP_IS_E1x (bp))drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c int bnx2x_nic_unload(struct bnx2x *bp, int unload_mode, bool keep_link) · line 3105
Quoted 10 lines
drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c +0
-8bnx2x_pf_disable (bp); -/* Disable HW interrupts, NAPI */ -bnx2x_netif_stop (bp,1 ); -/* Delete all NAPI objects */ -bnx2x_del_all_napi (bp); -if (CNIC_LOADED (bp)) -bnx2x_del_all_napi_cnic (bp); -/* Release IRQs */ -bnx2x_free_irq (bp);Could you split the change into two patches - one factoring out the code into
bnx2x_stop_nic()and the other adding thenic_stoppedvariable? First one should be pure code refactoring with no functional changes. That'd make the reviewing process easier.Quoted 9 lines
drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c +2
-0+/* Disable HW interrupts, delete NAPIs, Release IRQs */ +bnx2x_stop_nic (bp);/* Report UNLOAD_DONE to MCP */ bnx2x_send_unload_done (bp, false);@@ -4987,6 +4983,12 @@ void bnx2x_tx_timeout(struct net_device *dev, unsigned int txqueue) {struct bnx2x *bp =netdev_priv (dev);
Quoted 8 lines
drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c void bnx2x_tx_timeout(struct net_device *dev, unsigned int txqueue) · line 4990
Quoted 5 lines
drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c +5
-0+/* Immediately indicate link as down */ + bp->link_vars.link_up =0 ; + bp->force_link_down = true; +netif_carrier_off (dev); +BNX2X_ERR ("Indicating link is down due to Tx-timeout\n" );Is this code move to make the shutdown more immediate? That could also be a separate patch.
Note that the original code run under the rtnl lock and this is not lockless, it that safe?
Cheers,
Paolo
Hi,
Thanks for the review.
Quoted 15 lines · Paolo Abeni
Quoted 4 lines · Jakub Kicinski
Quoted 4 lines
drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c +0
-0@@ -4987,6 +4983,12 @@ void bnx2x_tx_timeout(struct net_device *dev, unsigned int txqueue) {struct bnx2x *bp =netdev_priv (dev);Quoted 8 lines
drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c void bnx2x_tx_timeout(struct net_device *dev, unsigned int txqueue) · line 4990
Quoted 5 lines
drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c +5
-0+/* Immediately indicate link as down */ + bp->link_vars.link_up =0 ; + bp->force_link_down = true; +netif_carrier_off (dev); +BNX2X_ERR ("Indicating link is down due to Tx-timeout\n" );Is this code move to make the shutdown more immediate? That could also be a separate patch.
Quoted 3 lines
Note that the original code run under the rtnl lock and this is not lockless, it that safe?
Yes, it is safe.
The caller, dev_watchdog() is a holding a spin_lock of the net_device.
Quoted 4 lines
Cheers,
Paolo
Thanks,
Thinh Tran
Thank you for thorough review.
Quoted 5 lines · Jakub Kicinski
Could you split the change into two patches - one factoring out the code into
bnx2x_stop_nic()and the other adding thenic_stoppedvariable? First one should be pure code refactoring with no functional changes. That'd make the reviewing process easier.
Sorry, I misunderstood comments in the reviewing of v3 asking to factor the code. Should I keep the changes I made, or should I summit a new patch with factored code?
Quoted 10 lines
Quoted 10 lines
drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c +2
-0+/* Disable HW interrupts, delete NAPIs, Release IRQs */ +bnx2x_stop_nic (bp);/* Report UNLOAD_DONE to MCP */ bnx2x_send_unload_done (bp, false);@@ -4987,6 +4983,12 @@ void bnx2x_tx_timeout(struct net_device *dev, unsigned int txqueue) {struct bnx2x *bp =netdev_priv (dev);
Quoted 8 lines
drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c void bnx2x_tx_timeout(struct net_device *dev, unsigned int txqueue) · line 4990
Quoted 5 lines
drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c +5
-0+/* Immediately indicate link as down */ + bp->link_vars.link_up =0 ; + bp->force_link_down = true; +netif_carrier_off (dev); +BNX2X_ERR ("Indicating link is down due to Tx-timeout\n" );Is this code move to make the shutdown more immediate? That could also be a separate patch.
Moving the code to here has the effect of disabling the link, preventing the excessive output of debug information from bnx2x_panic(). While bnx2x_panic() offers valuable debugging details, the excessive dumping overwhelms the dmesg buffer, they become unreadable. I will exclude this part from the patch. A separate patch will be created to improve providing debug information
Quoted 18 lines
Quoted 18 lines
drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c +0
-0/* We want the information of the dump logged, * but calling bnx2x_panic() would kill all chances of recovery. */ drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.h +3
-0@@ -1015,6 +1015,9 @@ static inline void bnx2x_free_rx_sge_range(struct bnx2x *bp, {int i; +if (!fp->page_pool.page) +return ; +if (fp->mode ==TPA_MODE_DISABLED )return ;
Quoted 7 lines
drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.h void bnx2x_set_os_driver_state(struct bnx2x *bp, u32 state); · line 1405
Quoted 5 lines
drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.h +1
-0@@ -1399,5 +1402,20 @@ void bnx2x_set_os_driver_state(struct bnx2x *bp, u32 state);*/ int bnx2x_nvram_read (struct bnx2x *bp,u32 offset,u8 *ret_buf,int buf_size); +static inline void bnx2x_stop_nic (struct bnx2x *bp)can't it live in
bnx2x_cmn.c?
It's in common header file for also being used by bnx2x_vfpf.c.
Why make it a static inline?
Quoted 1 line
Just make it inlined where it is called.
Quoted 29 lines
Quoted 5 lines
drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.h +5
-0+{ +if (!bp->nic_stopped) { +/* Disable HW interrupts, NAPI */ +bnx2x_netif_stop (bp,1 ); +/* Delete all NAPI objects */ drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.h void bnx2x_set_os_driver_state(struct bnx2x *bp, u32 state); · line 1420
Quoted 10 lines
drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.h +9
-0+bnx2x_del_all_napi (bp); +if (CNIC_LOADED (bp)) +bnx2x_del_all_napi_cnic (bp); +/* Release IRQs */ +bnx2x_free_irq (bp); + bp->nic_stopped = true; + } +} +nit: double new line
drivers/net/ethernet/broadcom/bnx2x/bnx2x_vfpf.c void bnx2x_vfpf_close_vf(struct bnx2x *bp) · line 533
Quoted 8 lines
drivers/net/ethernet/broadcom/bnx2x/bnx2x_vfpf.c +0
-2@@ -529,13 +529,8 @@ void bnx2x_vfpf_close_vf(struct bnx2x *bp)bnx2x_vfpf_finalize (bp, &req->first_tlv); free_irq: -/* Disable HW interrupts, NAPI */ -bnx2x_netif_stop (bp,0 );This used to say
Quoted 8 lines
bnx2x_netif_stop(bp, 0);but
bnx2x_stop_nic()will do:bnx2x_netif_stop(bp, 1);is it okay to shut down the HW here ? (whatever that entails)
My mistake, I didn't notice this before. The second parameter is for deciding if the hardware should stop sending interrupts or not. I'm not familiar with the virtual function code path, but I'll correct it to make sure things are consistent.
Quoted 7 lines
drivers/net/ethernet/broadcom/bnx2x/bnx2x_vfpf.c void bnx2x_vfpf_close_vf(struct bnx2x *bp) · line 533
Quoted 7 lines
drivers/net/ethernet/broadcom/bnx2x/bnx2x_vfpf.c +2
-5-/* Delete all NAPI objects */ -bnx2x_del_all_napi (bp); - -/* Release IRQs */ -bnx2x_free_irq (bp); +/* Disable HW interrupts, delete NAPIs, Release IRQs */ +bnx2x_stop_nic (bp);
Thanks,
Thinh Tran
Jakub Kicinski
maintainerQuoted 9 lines · On Mon, 7 Aug 2023 16:08:50 -0500 Thinh Tran wrote:
Quoted 4 lines
Could you split the change into two patches - one factoring out the code into
bnx2x_stop_nic()and the other adding thenic_stoppedvariable? First one should be pure code refactoring with no functional changes. That'd make the reviewing process easier.Sorry, I misunderstood comments in the reviewing of v3 asking to factor the code.
Should I keep the changes I made, or should I summit a new patch with factored code?
I am not sure what you're asking.
In v5 I'm hoping to see 3 patches (as a single series!) patch 1 - factor out the disabling into a helper patch 2 - introduce the bp->nic_stopped checking patch 3 - changes to bnx2x_tx_timeout()
Jakub Kicinski
maintainerQuoted 4 lines · On Mon, 7 Aug 2023 16:08:50 -0500 Thinh Tran wrote:
Quoted 3 lines
Quoted 1 line
+static inline void
bnx2x_stop_nic(struct bnx2x *bp)can't it live in
bnx2x_cmn.c?It's in common header file for also being used by
bnx2x_vfpf.c.
And bnx2x_cmn.c is the common source file. I don't see why it has to be a static inline.
Quoted 3 lines
Quoted 1 line
Quoted 1 line
Why make it a static inline?
Just make it inlined where it is called.
v5 cover bnx2x: Fix error recovering in switch configuration
In reply to Thinh TranAs the BCM57810 and other I/O adapters are connected through a PCIe switch, the bnx2x driver causes unexpected system hang/crash while handling PCIe switch errors, if its error handler is called after other drivers' handlers.
In this case, after numbers of bnx2x_tx_timout(), the bnx2x_nic_unload() is called, frees up resources and calls bnx2x_napi_disable(). Then when EEH calls its error handler, the bnx2x_io_error_detected() and bnx2x_io_slot_reset() also calling bnx2x_napi_disable() and freeing the resources.
v5:
- Breaking down into a series of individual patches
v4:
- factoring common code into new function bnx2x_stop_nic() that disables and releases IRQs and NAPIs
v3:
- no changes, just repatched to the latest driver level
- updated the reviewed-by Manish in October, 2022
v2:
- Check the state of the NIC before calling disable nappi and freeing the IRQ
- Prevent recurrence of TX timeout by turning off the carrier, calling netif_carrier_off() in bnx2x_tx_timeout()
- Check and bail out early if fp->page_pool already freed
Thinh Tran (4):
bnx2x: new the bp->nic_stopped variable for checking NIC status
bnx2x: factor out common code to bnx2x_stop_nic()
bnx2x: Prevent access to a freed page in page_pool
bnx2x: prevent excessive debug information during a TX timeout
drivers/net/ethernet/broadcom/bnx2x/bnx2x.h | 2 ++ .../net/ethernet/broadcom/bnx2x/bnx2x_cmn.c | 32 ++++++++++++++----- .../net/ethernet/broadcom/bnx2x/bnx2x_cmn.h | 4 +++ .../net/ethernet/broadcom/bnx2x/bnx2x_main.c | 26 +++------------ .../net/ethernet/broadcom/bnx2x/bnx2x_vfpf.c | 9 ++---- 5 files changed, 36 insertions(+), 37 deletions(-)
Signature · 1 line
2.27.0
PATCH v5 1/4 bnx2x: new the bp->nic_stopped variable for checking NIC status
Introducing 'nic_stopped' to verify the NIC's status before disabling NAPI and freeing IRQs
--- drivers/net/ethernet/broadcom/bnx2x/bnx2x.h | 2 ++ .../net/ethernet/broadcom/bnx2x/bnx2x_cmn.c | 21 +++++++----- .../net/ethernet/broadcom/bnx2x/bnx2x_main.c | 32 +++++++++++-------- .../net/ethernet/broadcom/bnx2x/bnx2x_vfpf.c | 17 ++++++---- 4 files changed, 44 insertions(+), 28 deletions(-)
drivers/net/ethernet/broadcom/bnx2x/bnx2x.h +2 -0
@@ -1509,6 +1509,8 @@ struct bnx2x {bool cnic_loaded;struct cnic_eth_dev *(*cnic_probe)(struct net_device *); +bool nic_stopped; +/* Flag that indicates that we can start looking for FCoE L2 queue * completions in the default status block. */
drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c +13 -8
@@ -2703,6 +2703,7 @@ int bnx2x_nic_load(struct bnx2x *bp, int load_mode)bnx2x_add_all_napi (bp);DP (NETIF_MSG_IFUP ,"napi added\n" );bnx2x_napi_enable (bp); + bp->nic_stopped = false;if (IS_PF (bp)) {/* set pf load just before approaching the MCP */
@@ -2948,6 +2949,7 @@ load_error2: load_error1:bnx2x_napi_disable (bp);bnx2x_del_all_napi (bp); + bp->nic_stopped = true;/* clear pf_load status, as it was already set */ if (IS_PF (bp))
@@ -3083,14 +3085,17 @@ int bnx2x_nic_unload(struct bnx2x *bp, int unload_mode, bool keep_link)if (!CHIP_IS_E1x (bp))bnx2x_pf_disable (bp); -/* Disable HW interrupts, NAPI */ -bnx2x_netif_stop (bp,1 ); -/* Delete all NAPI objects */ -bnx2x_del_all_napi (bp); -if (CNIC_LOADED (bp)) -bnx2x_del_all_napi_cnic (bp); -/* Release IRQs */ -bnx2x_free_irq (bp); +if (!bp->nic_stopped) { +/* Disable HW interrupts, NAPI */ +bnx2x_netif_stop (bp,1 ); +/* Delete all NAPI objects */ +bnx2x_del_all_napi (bp); +if (CNIC_LOADED (bp)) +bnx2x_del_all_napi_cnic (bp); +/* Release IRQs */ +bnx2x_free_irq (bp); + bp->nic_stopped = true; + }/* Report UNLOAD_DONE to MCP */ bnx2x_send_unload_done (bp, false);
drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c +19 -13
@@ -9475,15 +9475,18 @@ unload_error: } } -/* Disable HW interrupts, NAPI */ -bnx2x_netif_stop (bp,1 ); -/* Delete all NAPI objects */ -bnx2x_del_all_napi (bp); -if (CNIC_LOADED (bp)) -bnx2x_del_all_napi_cnic (bp); +if (!bp->nic_stopped) { +/* Disable HW interrupts, NAPI */ +bnx2x_netif_stop (bp,1 ); +/* Delete all NAPI objects */ +bnx2x_del_all_napi (bp); +if (CNIC_LOADED (bp)) +bnx2x_del_all_napi_cnic (bp); -/* Release IRQs */ -bnx2x_free_irq (bp); +/* Release IRQs */ +bnx2x_free_irq (bp); + bp->nic_stopped = true; + }/* Reset the chip, unless PCI function is offline. If we reach this * point following a PCI error handling, it means device is really
@@ -14256,13 +14259,16 @@ static pci_ers_result_t bnx2x_io_slot_reset(struct pci_dev *pdev) }bnx2x_drain_tx_queues (bp);bnx2x_send_unload_req (bp,UNLOAD_RECOVERY ); -bnx2x_netif_stop (bp,1 ); -bnx2x_del_all_napi (bp); +if (!bp->nic_stopped) { +bnx2x_netif_stop (bp,1 ); +bnx2x_del_all_napi (bp); -if (CNIC_LOADED (bp)) -bnx2x_del_all_napi_cnic (bp); +if (CNIC_LOADED (bp)) +bnx2x_del_all_napi_cnic (bp); -bnx2x_free_irq (bp); +bnx2x_free_irq (bp); + bp->nic_stopped = true; + }/* Report UNLOAD_DONE to MCP */ bnx2x_send_unload_done (bp, true);
drivers/net/ethernet/broadcom/bnx2x/bnx2x_vfpf.c +10 -7
@@ -529,13 +529,16 @@ void bnx2x_vfpf_close_vf(struct bnx2x *bp)bnx2x_vfpf_finalize (bp, &req->first_tlv); free_irq: -/* Disable HW interrupts, NAPI */ -bnx2x_netif_stop (bp,0 ); -/* Delete all NAPI objects */ -bnx2x_del_all_napi (bp); - -/* Release IRQs */ -bnx2x_free_irq (bp); +if (!bp->nic_stopped) { +/* Disable HW interrupts, NAPI */ +bnx2x_netif_stop (bp,0 ); +/* Delete all NAPI objects */ +bnx2x_del_all_napi (bp); + +/* Release IRQs */ +bnx2x_free_irq (bp); + bp->nic_stopped = true; + } } static void bnx2x_leading_vfq_init(struct bnx2x *bp, struct bnx2x_virtf *vf,
Signature · 1 line
2.27.0
PATCH v5 2/4 bnx2x: factor out common code to bnx2x_stop_nic()
In reply to Thinh TranRefactoring common code into a new function named bnx2x_stop_nic()
--- .../net/ethernet/broadcom/bnx2x/bnx2x_cmn.c | 27 +++++++++++-------- .../net/ethernet/broadcom/bnx2x/bnx2x_cmn.h | 1 + .../net/ethernet/broadcom/bnx2x/bnx2x_main.c | 26 +++--------------- .../net/ethernet/broadcom/bnx2x/bnx2x_vfpf.c | 12 ++------- 4 files changed, 23 insertions(+), 43 deletions(-)
drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c +16 -11
@@ -3085,17 +3085,8 @@ int bnx2x_nic_unload(struct bnx2x *bp, int unload_mode, bool keep_link)if (!CHIP_IS_E1x (bp))bnx2x_pf_disable (bp); -if (!bp->nic_stopped) { -/* Disable HW interrupts, NAPI */ -bnx2x_netif_stop (bp,1 ); -/* Delete all NAPI objects */ -bnx2x_del_all_napi (bp); -if (CNIC_LOADED (bp)) -bnx2x_del_all_napi_cnic (bp); -/* Release IRQs */ -bnx2x_free_irq (bp); - bp->nic_stopped = true; - } +/* Disable HW interrupts, delete NAPIs, Release IRQs */ +bnx2x_stop_nic (bp,1 );/* Report UNLOAD_DONE to MCP */ bnx2x_send_unload_done (bp, false);
@@ -5127,3 +5118,17 @@ void bnx2x_schedule_sp_rtnl(struct bnx2x *bp, enum sp_rtnl_flag flag, flag);schedule_delayed_work (&bp->sp_rtnl_task,0 ); } +void bnx2x_stop_nic (struct bnx2x *bp,int disable_hw) +{ +if (!bp->nic_stopped) { +/* Disable HW interrupts, NAPI */ +bnx2x_netif_stop (bp, disable_hw); +/* Delete all NAPI objects */ +bnx2x_del_all_napi (bp); +if (CNIC_LOADED (bp)) +bnx2x_del_all_napi_cnic (bp); +/* Release IRQs */ +bnx2x_free_irq (bp); + bp->nic_stopped = true; + } +}
drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.h +1 -0
@@ -553,6 +553,7 @@ void bnx2x_free_skbs(struct bnx2x *bp);void bnx2x_netif_stop (struct bnx2x *bp,int disable_hw);void bnx2x_netif_start (struct bnx2x *bp);int bnx2x_load_cnic (struct bnx2x *bp); +void bnx2x_stop_nic (struct bnx2x *bp,int disable_hw);/** * bnx2x_enable_msix - set msix configuration.
drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c +4 -22
@@ -9475,18 +9475,8 @@ unload_error: } } -if (!bp->nic_stopped) { -/* Disable HW interrupts, NAPI */ -bnx2x_netif_stop (bp,1 ); -/* Delete all NAPI objects */ -bnx2x_del_all_napi (bp); -if (CNIC_LOADED (bp)) -bnx2x_del_all_napi_cnic (bp); - -/* Release IRQs */ -bnx2x_free_irq (bp); - bp->nic_stopped = true; - } +/* Disable HW interrupts, delete NAPIs, Release IRQs */ +bnx2x_stop_nic (bp,1 );/* Reset the chip, unless PCI function is offline. If we reach this * point following a PCI error handling, it means device is really
@@ -14259,16 +14249,8 @@ static pci_ers_result_t bnx2x_io_slot_reset(struct pci_dev *pdev) }bnx2x_drain_tx_queues (bp);bnx2x_send_unload_req (bp,UNLOAD_RECOVERY ); -if (!bp->nic_stopped) { -bnx2x_netif_stop (bp,1 ); -bnx2x_del_all_napi (bp); - -if (CNIC_LOADED (bp)) -bnx2x_del_all_napi_cnic (bp); - -bnx2x_free_irq (bp); - bp->nic_stopped = true; - } +/* Disable HW interrupts, delete NAPIs, Release IRQs */ +bnx2x_stop_nic (bp,1 );/* Report UNLOAD_DONE to MCP */ bnx2x_send_unload_done (bp, true);
drivers/net/ethernet/broadcom/bnx2x/bnx2x_vfpf.c +2 -10
@@ -529,16 +529,8 @@ void bnx2x_vfpf_close_vf(struct bnx2x *bp)bnx2x_vfpf_finalize (bp, &req->first_tlv); free_irq: -if (!bp->nic_stopped) { -/* Disable HW interrupts, NAPI */ -bnx2x_netif_stop (bp,0 ); -/* Delete all NAPI objects */ -bnx2x_del_all_napi (bp); - -/* Release IRQs */ -bnx2x_free_irq (bp); - bp->nic_stopped = true; - } +/* Disable HW interrupts, delete NAPIs, Release IRQs */ +bnx2x_stop_nic (bp,0 ); } static void bnx2x_leading_vfq_init(struct bnx2x *bp, struct bnx2x_virtf *vf,
Signature · 1 line
2.27.0
Jakub Kicinski
maintainerRe: [Patch v5 2/4] bnx2x: factor out common code to bnx2x_stop_nic()
drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c void bnx2x_schedule_sp_rtnl(struct bnx2x *bp, enum sp_rtnl_flag flag, · line 5122
Quoted 3 lines · On Fri, 11 Aug 2023 15:15:10 -0500 Thinh Tran wrote:
}
drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c +2
-0+void bnx2x_stop_nic (struct bnx2x *bp,int disable_hw) +{
nit: empty line between closing bracket of previous function and the beginning of the new one, please (checkpatch --strict should hopefully point that out, although I haven't confirmed).
PATCH v5 3/4 bnx2x: Prevent access to a freed page in page_pool
In reply to Thinh Tran--- drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.h | 3 +++ 1 file changed, 3 insertions(+)
drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.h +3 -0
@@ -1016,6 +1016,9 @@ static inline void bnx2x_free_rx_sge_range(struct bnx2x *bp, {int i; +if (!fp->page_pool.page) +return ; +if (fp->mode ==TPA_MODE_DISABLED )return ;
Signature · 1 line
2.27.0
PATCH v5 4/4 bnx2x: prevent excessive debug information during a TX timeout
In reply to Thinh Tran--- drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c | 6 ++++++ drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c | 6 ------ 2 files changed, 6 insertions(+), 6 deletions(-)
drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c +6 -0
@@ -4971,6 +4971,12 @@ void bnx2x_tx_timeout(struct net_device *dev, unsigned int txqueue) {struct bnx2x *bp =netdev_priv (dev); +/* Immediately indicate link as down */ + bp->link_vars.link_up =0 ; + bp->force_link_down = true; +netif_carrier_off (dev); +BNX2X_ERR ("Indicating link is down due to Tx-timeout\n" ); +/* We want the information of the dump logged, * but calling bnx2x_panic() would kill all chances of recovery. */
drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c +0 -6
@@ -10267,12 +10267,6 @@ static void bnx2x_sp_rtnl_task(struct work_struct *work) bp->sp_rtnl_state =0 ;smp_mb (); -/* Immediately indicate link as down */ - bp->link_vars.link_up =0 ; - bp->force_link_down = true; -netif_carrier_off (bp->dev); -BNX2X_ERR ("Indicating link is down due to Tx-timeout\n" ); -bnx2x_nic_unload (bp,UNLOAD_NORMAL , true);/* When ret value shows failure of allocation failure, * the nic is rebooted again. If open still fails, a error
Signature · 1 line
2.27.0
Jakub Kicinski
maintainerQuoted 11 lines · On Fri, 11 Aug 2023 15:15:08 -0500 Thinh Tran wrote:
As the BCM57810 and other I/O adapters are connected through a PCIe switch, the bnx2x driver causes unexpected system hang/crash while handling PCIe switch errors, if its error handler is called after other drivers' handlers.
In this case, after numbers of
bnx2x_tx_timout(), thebnx2x_nic_unload()is called, frees up resources and callsbnx2x_napi_disable(). Then when EEH calls its error handler, thebnx2x_io_error_detected()andbnx2x_io_slot_reset()also callingbnx2x_napi_disable()and freeing the resources.
It's looking fairly reasonable, thanks!
We do need commit messages describing motivations and impact of each individual commit, tho, and those commits which are not refactoring / improvements but prevent crashes need to have a Fixes tag pointing to the first commit in history where problem may occur (sometimes it's the first commit of the entire history).
Signature · 1 line
pw-bot: cr
Quoted 9 lines · Jakub Kicinski
It's looking fairly reasonable, thanks!
We do need commit messages describing motivations and impact of each individual commit, tho, and those commits which are not refactoring / improvements but prevent crashes need to have a Fixes tag pointing to the first commit in history where problem may occur (sometimes it's the first commit of the entire history).
Thanks the review. I'll make the correction and resubmit them shortly. Thinh Tran
v6 cover bnx2x: Fix error recovering in switch configuration
In reply to Thinh TranWhile injecting PCIe errors to the upstream PCIe switch of a BCM57810 NIC, system hangs/crashes were observed.
After several calls to bnx2x_tx_timout() complete, bnx2x_nic_unload() is called to free up HW resources and bnx2x_napi_disable() is called to release NAPI objects. Later, when the EEH driver calls bnx2x_io_slot_reset() to complete the recovery process, bnx2x attempts to disable NAPI again by calling bnx2x_napi_disable() and freeing resources which have already been freed, resulting in a hang or crash.
This patch set introduces a new flag to track the HW resource and NAPI allocation state, refactor duplicated code into a single function, check page pool allocation status before freeing, and reduces debug output when a TX timeout event occurs.
v6:
- Clarifying and updating commit messages
v5:
- Breaking down into a series of individual patches
v4:
- factoring common code into new function bnx2x_stop_nic() that disables and releases IRQs and NAPIs
v3:
- no changes, just repatched to the latest driver level
- updated the reviewed-by Manish in October, 2022
v2:
- Check the state of the NIC before calling disable nappi and freeing the IRQ
- Prevent recurrence of TX timeout by turning off the carrier, calling netif_carrier_off() in bnx2x_tx_timeout()
- Check and bail out early if fp->page_pool already freed
Thinh Tran (4):
bnx2x: new the bp->nic_stopped variable for checking NIC status
bnx2x: factor out common code to bnx2x_stop_nic()
bnx2x: Prevent access to a freed page in page_pool
bnx2x: prevent excessive debug information during a TX timeout
drivers/net/ethernet/broadcom/bnx2x/bnx2x.h | 2 ++ .../net/ethernet/broadcom/bnx2x/bnx2x_cmn.c | 33 ++++++++++++++----- .../net/ethernet/broadcom/bnx2x/bnx2x_cmn.h | 4 +++ .../net/ethernet/broadcom/bnx2x/bnx2x_main.c | 26 +++------------ .../net/ethernet/broadcom/bnx2x/bnx2x_vfpf.c | 9 ++--- 5 files changed, 37 insertions(+), 37 deletions(-)
Signature · 1 line
2.27.0
PATCH v6 1/4 bnx2x: new flag for track HW resource
Add a new flag, nic_stopped, to track NIC resource usage (HW interrupts, NAPI objection allocation).
--- drivers/net/ethernet/broadcom/bnx2x/bnx2x.h | 2 ++ .../net/ethernet/broadcom/bnx2x/bnx2x_cmn.c | 21 +++++++----- .../net/ethernet/broadcom/bnx2x/bnx2x_main.c | 32 +++++++++++-------- .../net/ethernet/broadcom/bnx2x/bnx2x_vfpf.c | 17 ++++++---- 4 files changed, 44 insertions(+), 28 deletions(-)
drivers/net/ethernet/broadcom/bnx2x/bnx2x.h +2 -0
@@ -1508,6 +1508,8 @@ struct bnx2x {bool cnic_loaded;struct cnic_eth_dev *(*cnic_probe)(struct net_device *); +bool nic_stopped; +/* Flag that indicates that we can start looking for FCoE L2 queue * completions in the default status block. */
drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c +13 -8
@@ -2715,6 +2715,7 @@ int bnx2x_nic_load(struct bnx2x *bp, int load_mode)bnx2x_add_all_napi (bp);DP (NETIF_MSG_IFUP ,"napi added\n" );bnx2x_napi_enable (bp); + bp->nic_stopped = false;if (IS_PF (bp)) {/* set pf load just before approaching the MCP */
@@ -2960,6 +2961,7 @@ int bnx2x_nic_load(struct bnx2x *bp, int load_mode) load_error1:bnx2x_napi_disable (bp);bnx2x_del_all_napi (bp); + bp->nic_stopped = true;/* clear pf_load status, as it was already set */ if (IS_PF (bp))
@@ -3095,14 +3097,17 @@ int bnx2x_nic_unload(struct bnx2x *bp, int unload_mode, bool keep_link)if (!CHIP_IS_E1x (bp))bnx2x_pf_disable (bp); -/* Disable HW interrupts, NAPI */ -bnx2x_netif_stop (bp,1 ); -/* Delete all NAPI objects */ -bnx2x_del_all_napi (bp); -if (CNIC_LOADED (bp)) -bnx2x_del_all_napi_cnic (bp); -/* Release IRQs */ -bnx2x_free_irq (bp); +if (!bp->nic_stopped) { +/* Disable HW interrupts, NAPI */ +bnx2x_netif_stop (bp,1 ); +/* Delete all NAPI objects */ +bnx2x_del_all_napi (bp); +if (CNIC_LOADED (bp)) +bnx2x_del_all_napi_cnic (bp); +/* Release IRQs */ +bnx2x_free_irq (bp); + bp->nic_stopped = true; + }/* Report UNLOAD_DONE to MCP */ bnx2x_send_unload_done (bp, false);
drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c +19 -13
@@ -9474,15 +9474,18 @@ void bnx2x_chip_cleanup(struct bnx2x *bp, int unload_mode, bool keep_link) } } -/* Disable HW interrupts, NAPI */ -bnx2x_netif_stop (bp,1 ); -/* Delete all NAPI objects */ -bnx2x_del_all_napi (bp); -if (CNIC_LOADED (bp)) -bnx2x_del_all_napi_cnic (bp); +if (!bp->nic_stopped) { +/* Disable HW interrupts, NAPI */ +bnx2x_netif_stop (bp,1 ); +/* Delete all NAPI objects */ +bnx2x_del_all_napi (bp); +if (CNIC_LOADED (bp)) +bnx2x_del_all_napi_cnic (bp); -/* Release IRQs */ -bnx2x_free_irq (bp); +/* Release IRQs */ +bnx2x_free_irq (bp); + bp->nic_stopped = true; + }/* Reset the chip, unless PCI function is offline. If we reach this * point following a PCI error handling, it means device is really
@@ -14238,13 +14241,16 @@ static pci_ers_result_t bnx2x_io_slot_reset(struct pci_dev *pdev) }bnx2x_drain_tx_queues (bp);bnx2x_send_unload_req (bp,UNLOAD_RECOVERY ); -bnx2x_netif_stop (bp,1 ); -bnx2x_del_all_napi (bp); +if (!bp->nic_stopped) { +bnx2x_netif_stop (bp,1 ); +bnx2x_del_all_napi (bp); -if (CNIC_LOADED (bp)) -bnx2x_del_all_napi_cnic (bp); +if (CNIC_LOADED (bp)) +bnx2x_del_all_napi_cnic (bp); -bnx2x_free_irq (bp); +bnx2x_free_irq (bp); + bp->nic_stopped = true; + }/* Report UNLOAD_DONE to MCP */ bnx2x_send_unload_done (bp, true);
drivers/net/ethernet/broadcom/bnx2x/bnx2x_vfpf.c +10 -7
@@ -529,13 +529,16 @@ void bnx2x_vfpf_close_vf(struct bnx2x *bp)bnx2x_vfpf_finalize (bp, &req->first_tlv); free_irq: -/* Disable HW interrupts, NAPI */ -bnx2x_netif_stop (bp,0 ); -/* Delete all NAPI objects */ -bnx2x_del_all_napi (bp); - -/* Release IRQs */ -bnx2x_free_irq (bp); +if (!bp->nic_stopped) { +/* Disable HW interrupts, NAPI */ +bnx2x_netif_stop (bp,0 ); +/* Delete all NAPI objects */ +bnx2x_del_all_napi (bp); + +/* Release IRQs */ +bnx2x_free_irq (bp); + bp->nic_stopped = true; + } } static void bnx2x_leading_vfq_init(struct bnx2x *bp, struct bnx2x_virtf *vf,
Signature · 1 line
2.27.0
PATCH v6 2/4 bnx2x: factor out common code to bnx2x_stop_nic()
In reply to Thinh TranRefactor common code which disables and releases HW interrupts, deletes NAPI objects, into a new bnx2x_stop_nic() function.
--- .../net/ethernet/broadcom/bnx2x/bnx2x_cmn.c | 28 +++++++++++-------- .../net/ethernet/broadcom/bnx2x/bnx2x_cmn.h | 1 + .../net/ethernet/broadcom/bnx2x/bnx2x_main.c | 26 +++-------------- .../net/ethernet/broadcom/bnx2x/bnx2x_vfpf.c | 12 ++------ 4 files changed, 24 insertions(+), 43 deletions(-)
drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c +17 -11
@@ -3097,17 +3097,8 @@ int bnx2x_nic_unload(struct bnx2x *bp, int unload_mode, bool keep_link)if (!CHIP_IS_E1x (bp))bnx2x_pf_disable (bp); -if (!bp->nic_stopped) { -/* Disable HW interrupts, NAPI */ -bnx2x_netif_stop (bp,1 ); -/* Delete all NAPI objects */ -bnx2x_del_all_napi (bp); -if (CNIC_LOADED (bp)) -bnx2x_del_all_napi_cnic (bp); -/* Release IRQs */ -bnx2x_free_irq (bp); - bp->nic_stopped = true; - } +/* Disable HW interrupts, delete NAPIs, Release IRQs */ +bnx2x_stop_nic (bp,1 );/* Report UNLOAD_DONE to MCP */ bnx2x_send_unload_done (bp, false);
@@ -5139,3 +5130,18 @@ void bnx2x_schedule_sp_rtnl(struct bnx2x *bp, enum sp_rtnl_flag flag, flag);schedule_delayed_work (&bp->sp_rtnl_task,0 ); } + +void bnx2x_stop_nic (struct bnx2x *bp,int disable_hw) +{ +if (!bp->nic_stopped) { +/* Disable HW interrupts, NAPI */ +bnx2x_netif_stop (bp, disable_hw); +/* Delete all NAPI objects */ +bnx2x_del_all_napi (bp); +if (CNIC_LOADED (bp)) +bnx2x_del_all_napi_cnic (bp); +/* Release IRQs */ +bnx2x_free_irq (bp); + bp->nic_stopped = true; + } +}
drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.h +1 -0
@@ -553,6 +553,7 @@ void bnx2x_free_skbs(struct bnx2x *bp);void bnx2x_netif_stop (struct bnx2x *bp,int disable_hw);void bnx2x_netif_start (struct bnx2x *bp);int bnx2x_load_cnic (struct bnx2x *bp); +void bnx2x_stop_nic (struct bnx2x *bp,int disable_hw);/** * bnx2x_enable_msix - set msix configuration.
drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c +4 -22
@@ -9474,18 +9474,8 @@ void bnx2x_chip_cleanup(struct bnx2x *bp, int unload_mode, bool keep_link) } } -if (!bp->nic_stopped) { -/* Disable HW interrupts, NAPI */ -bnx2x_netif_stop (bp,1 ); -/* Delete all NAPI objects */ -bnx2x_del_all_napi (bp); -if (CNIC_LOADED (bp)) -bnx2x_del_all_napi_cnic (bp); - -/* Release IRQs */ -bnx2x_free_irq (bp); - bp->nic_stopped = true; - } +/* Disable HW interrupts, delete NAPIs, Release IRQs */ +bnx2x_stop_nic (bp,1 );/* Reset the chip, unless PCI function is offline. If we reach this * point following a PCI error handling, it means device is really
@@ -14241,16 +14231,8 @@ static pci_ers_result_t bnx2x_io_slot_reset(struct pci_dev *pdev) }bnx2x_drain_tx_queues (bp);bnx2x_send_unload_req (bp,UNLOAD_RECOVERY ); -if (!bp->nic_stopped) { -bnx2x_netif_stop (bp,1 ); -bnx2x_del_all_napi (bp); - -if (CNIC_LOADED (bp)) -bnx2x_del_all_napi_cnic (bp); - -bnx2x_free_irq (bp); - bp->nic_stopped = true; - } +/* Disable HW interrupts, delete NAPIs, Release IRQs */ +bnx2x_stop_nic (bp,1 );/* Report UNLOAD_DONE to MCP */ bnx2x_send_unload_done (bp, true);
drivers/net/ethernet/broadcom/bnx2x/bnx2x_vfpf.c +2 -10
@@ -529,16 +529,8 @@ void bnx2x_vfpf_close_vf(struct bnx2x *bp)bnx2x_vfpf_finalize (bp, &req->first_tlv); free_irq: -if (!bp->nic_stopped) { -/* Disable HW interrupts, NAPI */ -bnx2x_netif_stop (bp,0 ); -/* Delete all NAPI objects */ -bnx2x_del_all_napi (bp); - -/* Release IRQs */ -bnx2x_free_irq (bp); - bp->nic_stopped = true; - } +/* Disable HW interrupts, delete NAPIs, Release IRQs */ +bnx2x_stop_nic (bp,0 ); } static void bnx2x_leading_vfq_init(struct bnx2x *bp, struct bnx2x_virtf *vf,
Signature · 1 line
2.27.0
PATCH v6 3/4 bnx2x: Prevent access to a freed page in page_pool
In reply to Thinh TranVerify page pool allocations before freeing.
Fixes: Fixes: 523224a3b3cd ("bnx2x, cnic, bnx2i: use new FW/HSI")
--- drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.h | 3 +++ 1 file changed, 3 insertions(+)
drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.h +3 -0
@@ -1016,6 +1016,9 @@ static inline void bnx2x_free_rx_sge_range(struct bnx2x *bp, {int i; +if (!fp->page_pool.page) +return ; +if (fp->mode ==TPA_MODE_DISABLED )return ;
Signature · 1 line
2.27.0
PATCH v6 4/4 bnx2x: prevent excessive debug information during a TX timeout
In reply to Thinh TranReduce debug output when a TX timeout occurs to avoid overflowing the system log buffer.
--- drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c | 6 ++++++ drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c | 6 ------ 2 files changed, 6 insertions(+), 6 deletions(-)
drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c +6 -0
@@ -4983,6 +4983,12 @@ void bnx2x_tx_timeout(struct net_device *dev, unsigned int txqueue) {struct bnx2x *bp =netdev_priv (dev); +/* Immediately indicate link as down */ + bp->link_vars.link_up =0 ; + bp->force_link_down = true; +netif_carrier_off (dev); +BNX2X_ERR ("Indicating link is down due to Tx-timeout\n" ); +/* We want the information of the dump logged, * but calling bnx2x_panic() would kill all chances of recovery. */
drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c +0 -6
@@ -10266,12 +10266,6 @@ static void bnx2x_sp_rtnl_task(struct work_struct *work) bp->sp_rtnl_state =0 ;smp_mb (); -/* Immediately indicate link as down */ - bp->link_vars.link_up =0 ; - bp->force_link_down = true; -netif_carrier_off (bp->dev); -BNX2X_ERR ("Indicating link is down due to Tx-timeout\n" ); -bnx2x_nic_unload (bp,UNLOAD_NORMAL , true);/* When ret value shows failure of allocation failure, * the nic is rebooted again. If open still fails, a error
Signature · 1 line
2.27.0
Manish Chopra
maintainerQuoted 130 lines
-----Original Message-----
From: Thinh Tran <[email protected]>
Sent: Friday, August 26, 2022 1:30 AM
To: [email protected]
Cc: [email protected]; Ariel Elior <[email protected]>; [email protected]; Manish Chopra <[email protected]>; Sudarsana Reddy Kalluru <[email protected]>; Thinh Tran <[email protected]>
Subject: [EXT] [PATCH] bnx2x: Fix error recovering in switch configurationExternal Email
----------------------------------------------------------------------
As the BCM57810 and other I/O adapters are connected through a PCIe switch, the bnx2x driver causes unexpected system hang/crash while handling PCIe switch errors, if its error handler is called after other drivers' handlers.In this case, after numbers of
bnx2x_tx_timout(), thebnx2x_nic_unload()is called, frees up resources and callsbnx2x_napi_disable(). Then when EEH calls its error handler, thebnx2x_io_error_detected()andbnx2x_io_slot_reset()also callingbnx2x_napi_disable()and freeing the resources.This patch will:
- reduce the numbers ofbnx2x_panic_dump()while inbnx2x_tx_timeout(), avoid filling up dmesg buffer.
- use checking newnapi_enableflags to prevent calling disable again which causing system hangs.
- cheking iffp->page_poolalready freed avoid system crash.Signed-off-by Thinh Tran <[email protected]>--- drivers/net/ethernet/broadcom/bnx2x/bnx2x.h | 4 ++++ .../net/ethernet/broadcom/bnx2x/bnx2x_cmn.c | 24 ++++++++++++++++++- .../net/ethernet/broadcom/bnx2x/bnx2x_cmn.h | 3 +++ 3 files changed, 30 insertions(+), 1 deletion(-)drivers/net/ethernet/broadcom/bnx2x/bnx2x.h +4
-0@@ -1509,6 +1509,10 @@ struct bnx2x {bool cnic_loaded;struct cnic_eth_dev *(*cnic_probe)(struct net_device *); +bool napi_enable; +bool cnic_napi_enable; +int tx_timeout_cnt; +/* Flag that indicates that we can start looking for FCoE L2 queue * completions in the default status block. */ drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c +15
-0@@ -1860,37 +1860,49 @@ static int bnx2x_setup_irqs(struct bnx2x *bp)static void bnx2x_napi_enable_cnic (struct bnx2x *bp) {int i; +if (bp->cnic_napi_enable) +return ;for_each_rx_queue_cnic (bp, i) {napi_enable (&bnx2x_fp (bp, i, napi)); } + bp->cnic_napi_enable = true; }static void bnx2x_napi_enable (struct bnx2x *bp) {int i; +if (bp->napi_enable) +return ;for_each_eth_queue (bp, i) {napi_enable (&bnx2x_fp (bp, i, napi)); } + bp->napi_enable = true; }static void bnx2x_napi_disable_cnic (struct bnx2x *bp) {int i; +if (!bp->cnic_napi_enable) +return ;for_each_rx_queue_cnic (bp, i) {napi_disable (&bnx2x_fp (bp, i, napi)); } + bp->cnic_napi_enable = false; }static void bnx2x_napi_disable (struct bnx2x *bp) {int i; +if (!bp->napi_enable) +return ;for_each_eth_queue (bp, i) {napi_disable (&bnx2x_fp (bp, i, napi)); } + bp->napi_enable = false; } void bnx2x_netif_start(struct bnx2x *bp) @@ -2554,6 +2566,7 @@ int bnx2x_load_cnic(struct bnx2x *bp) }/* Add all CNIC NAPI objects */ + bp->cnic_napi_enable = false;bnx2x_add_all_napi_cnic (bp);DP (NETIF_MSG_IFUP ,"cnic napi added\n" );bnx2x_napi_enable_cnic (bp);@@ -2701,7 +2714,9 @@ int bnx2x_nic_load(struct bnx2x *bp, intload_mode) */ bnx2x_setup_tc (bp->dev, bp->max_cos); + bp->tx_timeout_cnt =0 ;/* Add all NAPI objects */ + bp->napi_enable = false;bnx2x_add_all_napi (bp);DP (NETIF_MSG_IFUP ,"napi added\n" );drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c +0
-0bnx2x_napi_enable (bp);@@ -4982,7 +4997,14 @@ void bnx2x_tx_timeout(struct net_device *dev,unsigned int txqueue) */ if (!bp->panic) #ifndef BNX2X_STOP_ON_ERROR
drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c void bnx2x_tx_timeout(struct net_device *dev, · line 5009
Quoted 10 lines
drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c +8
-1-bnx2x_panic_dump (bp, false); + { +if (++bp->tx_timeout_cnt >3 ) { +bnx2x_panic_dump (bp, false); + bp->tx_timeout_cnt =0 ; + }else { +netdev_err (bp->dev,"TX timeout %d times\n" , bp- >tx_timeout_cnt); + } +
Hello Trinh,bnx2x_panic_dump() dumps quite of some useful debug (fastpath, hw related) info to look at in the logs,
I think they should be dumped at very first occurrence of tx timeout rather than waiting on next few tx timeout event (which may not even occur in some cases). One another way to prevent it could be by turning off the
carrier (netif_carrier_off()) at very first place in bnx2x_tx_timeout() to prevent such repeated occurrences of tx timeout on the netdev.
Thanks,
Manish
Quoted 147 lines · Manish Chopra
Quoted 130 lines
-----Original Message-----
From: Thinh Tran <[email protected]>
Sent: Friday, August 26, 2022 1:30 AM
To: [email protected]
Cc: [email protected]; Ariel Elior <[email protected]>; [email protected]; Manish Chopra <[email protected]>; Sudarsana Reddy Kalluru <[email protected]>; Thinh Tran <[email protected]>
Subject: [EXT] [PATCH] bnx2x: Fix error recovering in switch configurationExternal Email
----------------------------------------------------------------------
As the BCM57810 and other I/O adapters are connected through a PCIe switch, the bnx2x driver causes unexpected system hang/crash while handling PCIe switch errors, if its error handler is called after other drivers' handlers.In this case, after numbers of
bnx2x_tx_timout(), thebnx2x_nic_unload()is called, frees up resources and callsbnx2x_napi_disable(). Then when EEH calls its error handler, thebnx2x_io_error_detected()andbnx2x_io_slot_reset()also callingbnx2x_napi_disable()and freeing the resources.This patch will:
- reduce the numbers ofbnx2x_panic_dump()while inbnx2x_tx_timeout(), avoid filling up dmesg buffer.
- use checking newnapi_enableflags to prevent calling disable again which causing system hangs.
- cheking iffp->page_poolalready freed avoid system crash.Signed-off-by Thinh Tran <[email protected]>--- drivers/net/ethernet/broadcom/bnx2x/bnx2x.h | 4 ++++ .../net/ethernet/broadcom/bnx2x/bnx2x_cmn.c | 24 ++++++++++++++++++- .../net/ethernet/broadcom/bnx2x/bnx2x_cmn.h | 3 +++ 3 files changed, 30 insertions(+), 1 deletion(-)drivers/net/ethernet/broadcom/bnx2x/bnx2x.h +4
-0@@ -1509,6 +1509,10 @@ struct bnx2x {bool cnic_loaded;struct cnic_eth_dev *(*cnic_probe)(struct net_device *); +bool napi_enable; +bool cnic_napi_enable; +int tx_timeout_cnt; +/* Flag that indicates that we can start looking for FCoE L2 queue * completions in the default status block. */ drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c +15
-0@@ -1860,37 +1860,49 @@ static int bnx2x_setup_irqs(struct bnx2x *bp)static void bnx2x_napi_enable_cnic (struct bnx2x *bp) {int i; +if (bp->cnic_napi_enable) +return ;for_each_rx_queue_cnic (bp, i) {napi_enable (&bnx2x_fp (bp, i, napi)); } + bp->cnic_napi_enable = true; }static void bnx2x_napi_enable (struct bnx2x *bp) {int i; +if (bp->napi_enable) +return ;for_each_eth_queue (bp, i) {napi_enable (&bnx2x_fp (bp, i, napi)); } + bp->napi_enable = true; }static void bnx2x_napi_disable_cnic (struct bnx2x *bp) {int i; +if (!bp->cnic_napi_enable) +return ;for_each_rx_queue_cnic (bp, i) {napi_disable (&bnx2x_fp (bp, i, napi)); } + bp->cnic_napi_enable = false; }static void bnx2x_napi_disable (struct bnx2x *bp) {int i; +if (!bp->napi_enable) +return ;for_each_eth_queue (bp, i) {napi_disable (&bnx2x_fp (bp, i, napi)); } + bp->napi_enable = false; } void bnx2x_netif_start(struct bnx2x *bp) @@ -2554,6 +2566,7 @@ int bnx2x_load_cnic(struct bnx2x *bp) }/* Add all CNIC NAPI objects */ + bp->cnic_napi_enable = false;bnx2x_add_all_napi_cnic (bp);DP (NETIF_MSG_IFUP ,"cnic napi added\n" );bnx2x_napi_enable_cnic (bp);@@ -2701,7 +2714,9 @@ int bnx2x_nic_load(struct bnx2x *bp, intload_mode) */ bnx2x_setup_tc (bp->dev, bp->max_cos); + bp->tx_timeout_cnt =0 ;/* Add all NAPI objects */ + bp->napi_enable = false;bnx2x_add_all_napi (bp);DP (NETIF_MSG_IFUP ,"napi added\n" );bnx2x_napi_enable(bp);drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c +0
-0@@ -4982,7 +4997,14 @@ void bnx2x_tx_timeout(struct net_device *dev,unsigned int txqueue) */ if (!bp->panic) #ifndef BNX2X_STOP_ON_ERRORdrivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c void bnx2x_tx_timeout(struct net_device *dev, · line 5009
Quoted 10 lines
drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c +8
-1-bnx2x_panic_dump (bp, false); + { +if (++bp->tx_timeout_cnt >3 ) { +bnx2x_panic_dump (bp, false); + bp->tx_timeout_cnt =0 ; + }else { +netdev_err (bp->dev,"TX timeout %d times\n" , bp- > tx_timeout_cnt); + } +Hello Trinh,
bnx2x_panic_dump()dumps quite of some useful debug (fastpath, hw related) info to look at in the logs,
I think they should be dumped at very first occurrence of tx timeout rather than waiting on next few tx timeout event (which may not even occur in some cases). One another way to prevent it could be by turning off the
carrier (netif_carrier_off()) at very first place inbnx2x_tx_timeout()to prevent such repeated occurrences of tx timeout on the netdev.
Quoted 2 lines
Thanks,
Manish
will do
Thanks,
Thinh Tran