| Message ID | 20230728211133.2240873-1-thinhtr@linux.vnet.ibm.com (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [v4] bnx2x: Fix error recovering in switch configuration | expand |
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(), 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. > > > Signed-off-by: Thinh Tran <thinhtr@linux.vnet.ibm.com> > Reviewed-by: Manish Chopra <manishc@marvell.com> > Tested-by: Abdul Haleem <abdhalee@in.ibm.com> > Tested-by: David Christensen <drc@linux.vnet.ibm.com> > > Reviewed-by: Simon Horman <simon.horman@corigine.com> nit: no empty lines between tags There should be a "---" line between the tags and changelog. > 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 ++ > @@ -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); 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. > + /* 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"); Is this code move to make the shutdown more immediate? That could also be a separate patch. > /* We want the information of the dump logged, > * but calling bnx2x_panic() would kill all chances of recovery. > */ > diff --git a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.h b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.h > index d8b1824c334d..f5ecbe8d604a 100644 > --- a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.h > +++ b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.h > @@ -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) can't it live in bnx2x_cmn.c ? Why make it a static inline? > +{ > + 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; > + } > +} > + > nit: double new line > --- a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_vfpf.c > +++ b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_vfpf.c > @@ -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) > - /* 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);
On Mon, 2023-07-31 at 17:47 -0700, Jakub Kicinski wrote: > 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(), 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. > > > > > > Signed-off-by: Thinh Tran <thinhtr@linux.vnet.ibm.com> > > Reviewed-by: Manish Chopra <manishc@marvell.com> > > Tested-by: Abdul Haleem <abdhalee@in.ibm.com> > > Tested-by: David Christensen <drc@linux.vnet.ibm.com> > > > > Reviewed-by: Simon Horman <simon.horman@corigine.com> > > nit: no empty lines between tags > > There should be a "---" line between the tags and changelog. > > > 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 ++ > > > @@ -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); > > 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. > > > + /* 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"); > > 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
Thank you for thorough review. On 7/31/2023 7:47 PM, Jakub Kicinski wrote: > > 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. 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? > >> + /* 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"); > > 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 > >> /* We want the information of the dump logged, >> * but calling bnx2x_panic() would kill all chances of recovery. >> */ >> diff --git a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.h b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.h >> index d8b1824c334d..f5ecbe8d604a 100644 >> --- a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.h >> +++ b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.h >> @@ -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) > > 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? > Just make it inlined where it is called. >> +{ >> + 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; >> + } >> +} >> + >> > > nit: double new line > >> --- a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_vfpf.c >> +++ b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_vfpf.c >> @@ -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) > 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. >> - /* 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
Hi, Thanks for the review. On 8/1/2023 3:30 AM, Paolo Abeni wrote: > On Mon, 2023-07-31 at 17:47 -0700, Jakub Kicinski wrote: >>> @@ -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. > > 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. > Cheers, > > Paolo > Thanks, Thinh Tran
On Mon, 7 Aug 2023 16:08:50 -0500 Thinh Tran wrote: > > 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. > > 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()
On Mon, 7 Aug 2023 16:08:50 -0500 Thinh Tran wrote: > >> +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. >>> Why make it a static inline? > > Just make it inlined where it is called.
diff --git a/drivers/net/ethernet/broadcom/bnx2x/bnx2x.h b/drivers/net/ethernet/broadcom/bnx2x/bnx2x.h index 8bcde0a6e011..e2a4e1088b7f 100644 --- a/drivers/net/ethernet/broadcom/bnx2x/bnx2x.h +++ b/drivers/net/ethernet/broadcom/bnx2x/bnx2x.h @@ -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. */ diff --git a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c index 6ea5521074d3..b4143c427936 100644 --- a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c +++ b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.c @@ -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. */ diff --git a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.h b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.h index d8b1824c334d..f5ecbe8d604a 100644 --- a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.h +++ b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_cmn.h @@ -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 */ diff --git a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c index 1e7a6f1d4223..e40c83b8b32e 100644 --- a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c +++ b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_main.c @@ -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); diff --git a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_vfpf.c b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_vfpf.c index 0657a0f5170f..d8af7c453172 100644 --- a/drivers/net/ethernet/broadcom/bnx2x/bnx2x_vfpf.c +++ b/drivers/net/ethernet/broadcom/bnx2x/bnx2x_vfpf.c @@ -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,